[v4,4/4] phy: add phy-hi6220-usb

Message ID 1423726646-30336-5-git-send-email-zhangfei.gao@linaro.org
State New
Headers show

Commit Message

zhangfei Feb. 12, 2015, 7:37 a.m.
Add usb phy controller for hi6220 platform

Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
---
 drivers/phy/Kconfig          |   9 ++
 drivers/phy/Makefile         |   1 +
 drivers/phy/phy-hi6220-usb.c | 306 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 316 insertions(+)
 create mode 100644 drivers/phy/phy-hi6220-usb.c

Comments

zhangfei Feb. 18, 2015, 5:44 a.m. | #1
Hi, Kishon

On 02/18/2015 01:35 PM, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Thursday 12 February 2015 01:07 PM, Zhangfei Gao wrote:
>> Add usb phy controller for hi6220 platform
>>
>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
>> ---
>>   drivers/phy/Kconfig          |   9 ++
>>   drivers/phy/Makefile         |   1 +
>>   drivers/phy/phy-hi6220-usb.c | 306
>> +++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 316 insertions(+)
>>   create mode 100644 drivers/phy/phy-hi6220-usb.c
>
> why is this driver in drivers/phy when it doesn't use the generic PHY
> framework at all?
>

Balbi recommended "new drivers only on drivers/phy/", including usb phy.
So Move drivers/usb/phy/phy-hi6220-usb.c to 
drivers/phy/phy-hi6220-usb.c, required by Balbi.

Thanks
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
zhangfei Feb. 20, 2015, 3:07 a.m. | #2
Hi, Balbi

On 02/18/2015 10:35 PM, Felipe Balbi wrote:
> On Wed, Feb 18, 2015 at 01:44:21PM +0800, zhangfei wrote:
>> Hi, Kishon
>>
>> On 02/18/2015 01:35 PM, Kishon Vijay Abraham I wrote:
>>> Hi,
>>>
>>> On Thursday 12 February 2015 01:07 PM, Zhangfei Gao wrote:
>>>> Add usb phy controller for hi6220 platform
>>>>
>>>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
>>>> ---
>>>>   drivers/phy/Kconfig          |   9 ++
>>>>   drivers/phy/Makefile         |   1 +
>>>>   drivers/phy/phy-hi6220-usb.c | 306
>>>> +++++++++++++++++++++++++++++++++++++++++++
>>>>   3 files changed, 316 insertions(+)
>>>>   create mode 100644 drivers/phy/phy-hi6220-usb.c
>>>
>>> why is this driver in drivers/phy when it doesn't use the generic PHY
>>> framework at all?
>>>
>>
>> Balbi recommended "new drivers only on drivers/phy/", including usb
>> phy.
>
> but it should use the API too. It's not only about a directory, you need
> to use the new API.
>
>> So Move drivers/usb/phy/phy-hi6220-usb.c to
>> drivers/phy/phy-hi6220-usb.c, required by Balbi.
>
> you're reading what I stated the way you like.

Sorry for my bad understanding.

Still not clear about the otg_set_peripheral, which is required in 
phy-hi6220-usb.c

1. drivers/usb/dwc2/gadget.c use
otg_set_peripheral(hsotg->uphy->otg, &hsotg->gadget);

2. include/linux/phy/phy.h
struct phy do not have member otg, while struct usb_phy has.

Could you give more hints?

Thanks
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
zhangfei Feb. 20, 2015, 10:27 a.m. | #3
On 02/20/2015 12:38 PM, Felipe Balbi wrote:
> On Fri, Feb 20, 2015 at 11:07:21AM +0800, zhangfei wrote:
>> Hi, Balbi
>>
>> On 02/18/2015 10:35 PM, Felipe Balbi wrote:
>>> On Wed, Feb 18, 2015 at 01:44:21PM +0800, zhangfei wrote:
>>>> Hi, Kishon
>>>>
>>>> On 02/18/2015 01:35 PM, Kishon Vijay Abraham I wrote:
>>>>> Hi,
>>>>>
>>>>> On Thursday 12 February 2015 01:07 PM, Zhangfei Gao wrote:
>>>>>> Add usb phy controller for hi6220 platform
>>>>>>
>>>>>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
>>>>>> ---
>>>>>>   drivers/phy/Kconfig          |   9 ++
>>>>>>   drivers/phy/Makefile         |   1 +
>>>>>>   drivers/phy/phy-hi6220-usb.c | 306
>>>>>> +++++++++++++++++++++++++++++++++++++++++++
>>>>>>   3 files changed, 316 insertions(+)
>>>>>>   create mode 100644 drivers/phy/phy-hi6220-usb.c
>>>>>
>>>>> why is this driver in drivers/phy when it doesn't use the generic PHY
>>>>> framework at all?
>>>>>
>>>>
>>>> Balbi recommended "new drivers only on drivers/phy/", including usb
>>>> phy.
>>>
>>> but it should use the API too. It's not only about a directory, you need
>>> to use the new API.
>>>
>>>> So Move drivers/usb/phy/phy-hi6220-usb.c to
>>>> drivers/phy/phy-hi6220-usb.c, required by Balbi.
>>>
>>> you're reading what I stated the way you like.
>>
>> Sorry for my bad understanding.
>>
>> Still not clear about the otg_set_peripheral, which is required in
>> phy-hi6220-usb.c
>>
>> 1. drivers/usb/dwc2/gadget.c use
>> otg_set_peripheral(hsotg->uphy->otg, &hsotg->gadget);
>>
>> 2. include/linux/phy/phy.h
>> struct phy do not have member otg, while struct usb_phy has.
>>
>> Could you give more hints?
>
> your set_peripheral doesn't do anything, just holds a pointer. Might as
> well not implement it. I'll review your driver more fully tomorrow.
>
> There a few things which must be changed.
>

Thanks in advance.

We need this call back set_peripheral setting otg->gadget, which used in 
usb_gadget_connect/disconnect(otg->gadget).

The workable method test here is not provide phy-names = "usb2-phy";
Then dwc2 will still use hsotg->uphy instead of hsotg->phy.
Though devm_phy_create is used in phy-hi6220-usb.c, phy_ops is not used 
in fact.

Thanks

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
zhangfei Feb. 20, 2015, 3:44 p.m. | #4
Hi, Balbi

On 02/20/2015 10:41 PM, Felipe Balbi wrote:

>> +static void hi6220_start_peripheral(struct hi6220_priv *priv, bool on)
>> +{
>> +	struct usb_otg *otg = priv->phy.otg;
>> +
>> +	if (!otg->gadget)
>> +		return;
>> +
>> +	if (on)
>> +		usb_gadget_connect(otg->gadget);
>> +	else
>> +		usb_gadget_disconnect(otg->gadget);
>
> why is the PHY fiddling with pullups ?

We use this to enable/disable otg gadget mode.

The gpio_id & gpio_vbus are used to distinguish otg gadget mode or host 
mode.
When micro usb or otg device attached to otg, gpio_vbus falling down.
And gpio_id = 1 is micro usb, gpio_id = 0 is otg device.

So when micro usb attached, we enable gadget mode; while micro usb 
detached, we disable gadget mode, and dwc2 will automatically set to 
host mode.

>
>> +}
>> +
>> +static void hi6220_detect_work(struct work_struct *work)
>> +{
>> +	struct hi6220_priv *priv =
>> +		container_of(work, struct hi6220_priv, work.work);
>> +	int gpio_id, gpio_vbus;
>> +	enum usb_otg_state state;
>> +
>> +	if (!gpio_is_valid(priv->gpio_id) || !gpio_is_valid(priv->gpio_vbus))
>> +		return;
>> +
>> +	gpio_id = gpio_get_value_cansleep(priv->gpio_id);
>> +	gpio_vbus = gpio_get_value_cansleep(priv->gpio_vbus);
>
> looks like this should be using extcon
Not used extcon before.
However, we need gpio_vbus interrupt.
Checked phy-tahvo.c and phy-omap-otg.c, not find extcon related with 
interrupt.
Will investigate tomorrow.

>
>> +
>> +	if (gpio_vbus == 0) {
>> +		if (gpio_id == 1)
>> +			state = OTG_STATE_B_PERIPHERAL;
>> +		else
>> +			state = OTG_STATE_A_HOST;
>> +	} else {
>> +		state = OTG_STATE_A_HOST;
>> +	}
>> +
>> +	if (priv->state != state) {
>> +		hi6220_start_peripheral(priv, state == OTG_STATE_B_PERIPHERAL);
>> +		priv->state = state;
>> +	}
>> +}
>> +
>> +static irqreturn_t hiusb_gpio_intr(int irq, void *data)
>> +{
>> +	struct hi6220_priv *priv = (struct hi6220_priv *)data;
>> +
>> +	/* add debounce time */
>> +	schedule_delayed_work(&priv->work, msecs_to_jiffies(100));
>
> this is really bad. We have threaded interrupt support, right ?

Since we use two gpio to distinguish gadget mode or host mode.
Debounce time can introduce more accuracy.
I think threaded interrupt can not be used for adding debounce time.
Here add debounce is just for safety.

Thanks

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
zhangfei Feb. 21, 2015, 3:03 p.m. | #5
Hi, Balbi

On 02/21/2015 12:06 AM, Felipe Balbi wrote:
> Hi,
>
> On Fri, Feb 20, 2015 at 11:44:37PM +0800, zhangfei wrote:
>> Hi, Balbi
>>
>> On 02/20/2015 10:41 PM, Felipe Balbi wrote:
>>
>>>> +static void hi6220_start_peripheral(struct hi6220_priv *priv, bool on)
>>>> +{
>>>> +	struct usb_otg *otg = priv->phy.otg;
>>>> +
>>>> +	if (!otg->gadget)
>>>> +		return;
>>>> +
>>>> +	if (on)
>>>> +		usb_gadget_connect(otg->gadget);
>>>> +	else
>>>> +		usb_gadget_disconnect(otg->gadget);
>>>
>>> why is the PHY fiddling with pullups ?
>>
>> We use this to enable/disable otg gadget mode.
>
> I got that, but the pullups don't belong to the PHY, they belong to the
> gadget.
>
>> The gpio_id & gpio_vbus are used to distinguish otg gadget mode or
>> host mode.
>> When micro usb or otg device attached to otg, gpio_vbus falling down.
>> And gpio_id = 1 is micro usb, gpio_id = 0 is otg device.
>
> all of that I understood clearly :-)
>
>> So when micro usb attached, we enable gadget mode; while micro usb
>> detached, we disable gadget mode, and dwc2 will automatically set to
>> host mode.
>
> that's all fine, I'm concerned about letting the PHY fiddle with
> something it doesn't own. If I am to change pullups rules in udc-core,
> this is likely to break down miserably and I don't want to have to go
> through that.

Thanks for the clarifying.

How about using usb_gadget_vbus_connect/disconnect, which are used in 
many files under drivers/usb/phy.
There is no vbus_session in dwc2/gadget.c, I thought it would be same as 
pullup.

However, usb_gadget_vbus_connect still need para gadget, where should we 
put this file, drivers/usb/phy or drivers/phy

>
>>>> +static void hi6220_detect_work(struct work_struct *work)
>>>> +{
>>>> +	struct hi6220_priv *priv =
>>>> +		container_of(work, struct hi6220_priv, work.work);
>>>> +	int gpio_id, gpio_vbus;
>>>> +	enum usb_otg_state state;
>>>> +
>>>> +	if (!gpio_is_valid(priv->gpio_id) || !gpio_is_valid(priv->gpio_vbus))
>>>> +		return;
>>>> +
>>>> +	gpio_id = gpio_get_value_cansleep(priv->gpio_id);
>>>> +	gpio_vbus = gpio_get_value_cansleep(priv->gpio_vbus);
>>>
>>> looks like this should be using extcon
>> Not used extcon before.
>> However, we need gpio_vbus interrupt.
>> Checked phy-tahvo.c and phy-omap-otg.c, not find extcon related with
>> interrupt.
>> Will investigate tomorrow.
>
> drivers/extcon/extcon-gpio.c
I think there is no need to use extcon, gpio is clear enough.
extcon-gpio.c even do not support dt.

>
>>>> +	if (gpio_vbus == 0) {
>>>> +		if (gpio_id == 1)
>>>> +			state = OTG_STATE_B_PERIPHERAL;
>>>> +		else
>>>> +			state = OTG_STATE_A_HOST;
>>>> +	} else {
>>>> +		state = OTG_STATE_A_HOST;
>>>> +	}
>>>> +
>>>> +	if (priv->state != state) {
>>>> +		hi6220_start_peripheral(priv, state == OTG_STATE_B_PERIPHERAL);
>>>> +		priv->state = state;
>>>> +	}
>>>> +}
>>>> +
>>>> +static irqreturn_t hiusb_gpio_intr(int irq, void *data)
>>>> +{
>>>> +	struct hi6220_priv *priv = (struct hi6220_priv *)data;
>>>> +
>>>> +	/* add debounce time */
>>>> +	schedule_delayed_work(&priv->work, msecs_to_jiffies(100));
>>>
>>> this is really bad. We have threaded interrupt support, right ?
>>
>> Since we use two gpio to distinguish gadget mode or host mode.
>> Debounce time can introduce more accuracy.
>
> gpio_set_debounce() ?
Not all gpio.c support set_debounce, including gpio-pl061.c.

>
>> I think threaded interrupt can not be used for adding debounce time.
>> Here add debounce is just for safety.
>
> add the debounce to the gpio itself.

Here the debounce added only for safety.
gpio_id may mis-report when unplug usb, but it is correct for plug usb & 
otg device.
So debounce can be omitted.
If you think using delayed work for debounce is ugly, it is fine switch 
to threaded_irq.

Thanks

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
zhangfei Feb. 22, 2015, 3:10 a.m. | #6
Hi, Balbi

On 02/22/2015 12:21 AM, Felipe Balbi wrote:
> Hi,
>
> On Sat, Feb 21, 2015 at 11:03:05PM +0800, zhangfei wrote:
>>>>>> +static void hi6220_start_peripheral(struct hi6220_priv *priv, bool on)
>>>>>> +{
>>>>>> +	struct usb_otg *otg = priv->phy.otg;
>>>>>> +
>>>>>> +	if (!otg->gadget)
>>>>>> +		return;
>>>>>> +
>>>>>> +	if (on)
>>>>>> +		usb_gadget_connect(otg->gadget);
>>>>>> +	else
>>>>>> +		usb_gadget_disconnect(otg->gadget);
>>>>>
>>>>> why is the PHY fiddling with pullups ?
>>>>
>>>> We use this to enable/disable otg gadget mode.
>>>
>>> I got that, but the pullups don't belong to the PHY, they belong to the
>>> gadget.
>>>
>>>> The gpio_id & gpio_vbus are used to distinguish otg gadget mode or
>>>> host mode.
>>>> When micro usb or otg device attached to otg, gpio_vbus falling down.
>>>> And gpio_id = 1 is micro usb, gpio_id = 0 is otg device.
>>>
>>> all of that I understood clearly :-)
>>>
>>>> So when micro usb attached, we enable gadget mode; while micro usb
>>>> detached, we disable gadget mode, and dwc2 will automatically set to
>>>> host mode.
>>>
>>> that's all fine, I'm concerned about letting the PHY fiddle with
>>> something it doesn't own. If I am to change pullups rules in udc-core,
>>> this is likely to break down miserably and I don't want to have to go
>>> through that.
>>
>> Thanks for the clarifying.
>
> no problem.
>
>> How about using usb_gadget_vbus_connect/disconnect, which are used in many
>> files under drivers/usb/phy.
>> There is no vbus_session in dwc2/gadget.c, I thought it would be same as
>> pullup.
>>
>> However, usb_gadget_vbus_connect still need para gadget, where should we put
>> this file, drivers/usb/phy or drivers/phy
>
> drivers/phy, if the framework misses anything you need, it's a great
> opportunity to give back to the community by extending the framework.

Sorry, I am a little confused.
I need some concrete suggestion for the next step of this patch, which 
is required for the community board, hikey board.

Do you mean in the future we need use hsotg->phy instead of hsotg->uphy.
         struct phy *phy;
         struct usb_phy *uphy;
usb_phy has many members that struct phy does not have, including otg.
struct usb_otg          *otg;
Is that mean we need port such member from usb_phy to phy.

Besides, are you ok with using usb_gadget_vbus_connect/disconnect.

>
> Scratching one's own itch kinda thing...
>
>>>>>> +static void hi6220_detect_work(struct work_struct *work)
>>>>>> +{
>>>>>> +	struct hi6220_priv *priv =
>>>>>> +		container_of(work, struct hi6220_priv, work.work);
>>>>>> +	int gpio_id, gpio_vbus;
>>>>>> +	enum usb_otg_state state;
>>>>>> +
>>>>>> +	if (!gpio_is_valid(priv->gpio_id) || !gpio_is_valid(priv->gpio_vbus))
>>>>>> +		return;
>>>>>> +
>>>>>> +	gpio_id = gpio_get_value_cansleep(priv->gpio_id);
>>>>>> +	gpio_vbus = gpio_get_value_cansleep(priv->gpio_vbus);
>>>>>
>>>>> looks like this should be using extcon
>>>> Not used extcon before.
>>>> However, we need gpio_vbus interrupt.
>>>> Checked phy-tahvo.c and phy-omap-otg.c, not find extcon related with
>>>> interrupt.
>>>> Will investigate tomorrow.
>>>
>>> drivers/extcon/extcon-gpio.c
>> I think there is no need to use extcon, gpio is clear enough.
>> extcon-gpio.c even do not support dt.
>
> well, add DT. The whole idea of free software is that we improve on
> things we already have. EXTCON is *the* API to handle such things.

I think I am still not understanding extcon-gpio, not sure why need use 
this API here.

Here two gpio requires, one gpio as interrupt, in the interrupt handler, 
we detect the gpio status judging the otg status.
extcon-gpio.c use the interrupt, then can we also use the gpio interrupt.
Using extcon-gpio is used for saving gpio_request?

>
> Quite frankly, though, Roger Quadros (now in Cc) has been working to get
> DT support on extcon-gpio, so perhaps wait for that and base your
> patches on top of his.
>
> Now your statement that GPIO is clear enough is completely bogus to me.
>
> Why do we have fixed regulators with GPIO enable signals, right ? Might
> as well just fiddle with the GPIO directly, right ? Wrong, the idea of
> using these frameworks is to encapsulate implementation details and make
> sure that if things change from one board to another, we can still use
> our SW without major modifications.
>
>>>>>> +	if (gpio_vbus == 0) {
>>>>>> +		if (gpio_id == 1)
>>>>>> +			state = OTG_STATE_B_PERIPHERAL;
>>>>>> +		else
>>>>>> +			state = OTG_STATE_A_HOST;
>>>>>> +	} else {
>>>>>> +		state = OTG_STATE_A_HOST;
>>>>>> +	}
>>>>>> +
>>>>>> +	if (priv->state != state) {
>>>>>> +		hi6220_start_peripheral(priv, state == OTG_STATE_B_PERIPHERAL);
>>>>>> +		priv->state = state;
>>>>>> +	}
>>>>>> +}
>>>>>> +
>>>>>> +static irqreturn_t hiusb_gpio_intr(int irq, void *data)
>>>>>> +{
>>>>>> +	struct hi6220_priv *priv = (struct hi6220_priv *)data;
>>>>>> +
>>>>>> +	/* add debounce time */
>>>>>> +	schedule_delayed_work(&priv->work, msecs_to_jiffies(100));
>>>>>
>>>>> this is really bad. We have threaded interrupt support, right ?
>>>>
>>>> Since we use two gpio to distinguish gadget mode or host mode.
>>>> Debounce time can introduce more accuracy.
>>>
>>> gpio_set_debounce() ?
>> Not all gpio.c support set_debounce, including gpio-pl061.c.
>
> then the framework should implement it in SW. That was the whole idea of
> adding set_debounce() to gpiolib. If the controller can't handle it,
> gpiolib handles it in SW. Again, encapsulating details.
>
>>>> I think threaded interrupt can not be used for adding debounce time.
>>>> Here add debounce is just for safety.
>>>
>>> add the debounce to the gpio itself.
>>
>> Here the debounce added only for safety.
>> gpio_id may mis-report when unplug usb, but it is correct for plug usb & otg
>> device.
>> So debounce can be omitted.
>> If you think using delayed work for debounce is ugly, it is fine switch to
>> threaded_irq.
>
> debounce might be very well needed. We *do* want to filter out the
> transient period. I'm just telling you there are better ways of doing
> so; and your response to that is "let's just remove it" and I'm not
> really comfortable with that attitude.
>
> That's the attitude of a lazy person which, I hope, you are not ;)

Understand.
What I mean here is gpio_id is not used when unplug, it is only used 
after plug.

Thanks
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
zhangfei Feb. 25, 2015, 1:28 p.m. | #7
On 02/23/2015 11:36 PM, Felipe Balbi wrote:
> Hi,
>
> On Sun, Feb 22, 2015 at 11:10:36AM +0800, zhangfei wrote:
>>>>>>>> +static void hi6220_start_peripheral(struct hi6220_priv *priv, bool on)
>>>>>>>> +{
>>>>>>>> +	struct usb_otg *otg = priv->phy.otg;
>>>>>>>> +
>>>>>>>> +	if (!otg->gadget)
>>>>>>>> +		return;
>>>>>>>> +
>>>>>>>> +	if (on)
>>>>>>>> +		usb_gadget_connect(otg->gadget);
>>>>>>>> +	else
>>>>>>>> +		usb_gadget_disconnect(otg->gadget);
>>>>>>>
>>>>>>> why is the PHY fiddling with pullups ?
>>>>>>
>>>>>> We use this to enable/disable otg gadget mode.
>>>>>
>>>>> I got that, but the pullups don't belong to the PHY, they belong to the
>>>>> gadget.
>>>>>
>>>>>> The gpio_id & gpio_vbus are used to distinguish otg gadget mode or
>>>>>> host mode.
>>>>>> When micro usb or otg device attached to otg, gpio_vbus falling down.
>>>>>> And gpio_id = 1 is micro usb, gpio_id = 0 is otg device.
>>>>>
>>>>> all of that I understood clearly :-)
>>>>>
>>>>>> So when micro usb attached, we enable gadget mode; while micro usb
>>>>>> detached, we disable gadget mode, and dwc2 will automatically set to
>>>>>> host mode.
>>>>>
>>>>> that's all fine, I'm concerned about letting the PHY fiddle with
>>>>> something it doesn't own. If I am to change pullups rules in udc-core,
>>>>> this is likely to break down miserably and I don't want to have to go
>>>>> through that.
>>>>
>>>> Thanks for the clarifying.
>>>
>>> no problem.
>>>
>>>> How about using usb_gadget_vbus_connect/disconnect, which are used in many
>>>> files under drivers/usb/phy.
>>>> There is no vbus_session in dwc2/gadget.c, I thought it would be same as
>>>> pullup.
>>>>
>>>> However, usb_gadget_vbus_connect still need para gadget, where should we put
>>>> this file, drivers/usb/phy or drivers/phy
>>>
>>> drivers/phy, if the framework misses anything you need, it's a great
>>> opportunity to give back to the community by extending the framework.
>>
>> Sorry, I am a little confused.
>> I need some concrete suggestion for the next step of this patch, which is
>> required for the community board, hikey board.
>>
>> Do you mean in the future we need use hsotg->phy instead of hsotg->uphy.
>>          struct phy *phy;
>>          struct usb_phy *uphy;
>
> yes, we need to move everybody to use struct phy, instead of struct
> usb_phy.
>
>> usb_phy has many members that struct phy does not have, including otg.
>> struct usb_otg          *otg;
>> Is that mean we need port such member from usb_phy to phy.
>
> This means we have a little ground work to do before we can add your phy
> driver to that framework, right ? As I said, if the framework is missing
> anything, work with Kishon (generic phy maintainer) to add those missing
> pieces generically.

OK, got it.
>
>> Besides, are you ok with using usb_gadget_vbus_connect/disconnect.
>>
>>>
>>> Scratching one's own itch kinda thing...
>>>
>>>>>>>> +static void hi6220_detect_work(struct work_struct *work)
>>>>>>>> +{
>>>>>>>> +	struct hi6220_priv *priv =
>>>>>>>> +		container_of(work, struct hi6220_priv, work.work);
>>>>>>>> +	int gpio_id, gpio_vbus;
>>>>>>>> +	enum usb_otg_state state;
>>>>>>>> +
>>>>>>>> +	if (!gpio_is_valid(priv->gpio_id) || !gpio_is_valid(priv->gpio_vbus))
>>>>>>>> +		return;
>>>>>>>> +
>>>>>>>> +	gpio_id = gpio_get_value_cansleep(priv->gpio_id);
>>>>>>>> +	gpio_vbus = gpio_get_value_cansleep(priv->gpio_vbus);
>>>>>>>
>>>>>>> looks like this should be using extcon
>>>>>> Not used extcon before.
>>>>>> However, we need gpio_vbus interrupt.
>>>>>> Checked phy-tahvo.c and phy-omap-otg.c, not find extcon related with
>>>>>> interrupt.
>>>>>> Will investigate tomorrow.
>>>>>
>>>>> drivers/extcon/extcon-gpio.c
>>>> I think there is no need to use extcon, gpio is clear enough.
>>>> extcon-gpio.c even do not support dt.
>>>
>>> well, add DT. The whole idea of free software is that we improve on
>>> things we already have. EXTCON is *the* API to handle such things.
>>
>> I think I am still not understanding extcon-gpio, not sure why need
>> use this API here.
>
> because extcon is the API to use for external connectors. The same way
> you use regulator framework to control that single GPIO tied to an
> enable signal of a fixed regulator, you use extcon when you need to read
> that gpio signal tied to id pin of the USB connector.
>
>> Here two gpio requires, one gpio as interrupt, in the interrupt
>> handler, we detect the gpio status judging the otg status.
>> extcon-gpio.c use the interrupt, then can we also use the gpio
>> interrupt.  Using extcon-gpio is used for saving gpio_request?
>
> extcon is used to hide gpio_request from dwc2. dwc2 only knows about
> extcon, not gpios. extcon will request the gpio and use it as interrupt
> source. When an IRQ fires, it will read the gpio state and decide if it
> should broadcast a message to tell dwc2 to become host or peripheral.

Thanks for the kind education, understand now.





--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
zhangfei Feb. 26, 2015, 8:48 a.m. | #8
Hi, Roger

On 02/24/2015 06:13 PM, Roger Quadros wrote:
>>> On Sat, Feb 21, 2015 at 11:03:05PM +0800, zhangfei wrote:
>>>>>>>> +static void hi6220_start_peripheral(struct hi6220_priv *priv, bool on)
>>>>>>>> +{
>>>>>>>> +    struct usb_otg *otg = priv->phy.otg;
>>>>>>>> +
>>>>>>>> +    if (!otg->gadget)
>>>>>>>> +        return;
>>>>>>>> +
>>>>>>>> +    if (on)
>>>>>>>> +        usb_gadget_connect(otg->gadget);
>>>>>>>> +    else
>>>>>>>> +        usb_gadget_disconnect(otg->gadget);
>>>>>>>
>>>>>>> why is the PHY fiddling with pullups ?
>>>>>>
>>>>>> We use this to enable/disable otg gadget mode.
>>>>>
>>>>> I got that, but the pullups don't belong to the PHY, they belong to the
>>>>> gadget.
>>>>>
>>>>>> The gpio_id & gpio_vbus are used to distinguish otg gadget mode or
>>>>>> host mode.
>>>>>> When micro usb or otg device attached to otg, gpio_vbus falling down.
>>>>>> And gpio_id = 1 is micro usb, gpio_id = 0 is otg device.
>>>>>
>>>>> all of that I understood clearly :-)
>>>>>
>>>>>> So when micro usb attached, we enable gadget mode; while micro usb
>>>>>> detached, we disable gadget mode, and dwc2 will automatically set to
>>>>>> host mode.
>>>>>
>>>>> that's all fine, I'm concerned about letting the PHY fiddle with
>>>>> something it doesn't own. If I am to change pullups rules in udc-core,
>>>>> this is likely to break down miserably and I don't want to have to go
>>>>> through that.
>>>>
>>>> Thanks for the clarifying.
>>>
>>> no problem.
>>>
>>>> How about using usb_gadget_vbus_connect/disconnect, which are used in many
>>>> files under drivers/usb/phy.
>>>> There is no vbus_session in dwc2/gadget.c, I thought it would be same as
>>>> pullup.
>>>>
>>>> However, usb_gadget_vbus_connect still need para gadget, where should we put
>>>> this file, drivers/usb/phy or drivers/phy
>>>
>>> drivers/phy, if the framework misses anything you need, it's a great
>>> opportunity to give back to the community by extending the framework.
>>
>> Sorry, I am a little confused.
>> I need some concrete suggestion for the next step of this patch, which is required for the community board, hikey board.
>>
>> Do you mean in the future we need use hsotg->phy instead of hsotg->uphy.
>>          struct phy *phy;
>>          struct usb_phy *uphy;
>> usb_phy has many members that struct phy does not have, including otg.
>> struct usb_otg          *otg;
>> Is that mean we need port such member from usb_phy to phy.
>
> In my opinion otg structure should belong to the USB core part that takes care
> of the OTG/DRD state machine. We still don't have a clear solution here and
> I'm currently investigating this.
> My current work is to get Dual role functionality working with DWC3 controller and TI
> platforms.
>
> Currently phy drivers take care of OTG operation themselves but there is an opportunity
> to share code and centralize USB role switching.
> The USB core should be the owner of the Host controller, Gadget controller and the OTG phy
> and should take care of the that.

Good idea.
If you have any patch, I will be very happy to verify.

How about adding these things in drivers/phy/phy-core.c, it is also 
sharable, though not in usb core.

Just tried adding one member struct usb_otg otg to struct phy, since not 
find any good member can hold usb_otg.
In drivers/phy/phy-core.c, adding extcon_register_interest, 
phy_vbus_notifier, phy_set_peripheral, it works for me, dwc2 on hikey board.

>
>>
>> Besides, are you ok with using usb_gadget_vbus_connect/disconnect.
>
> I don't think PHY is the right place for this even though older drivers seem to be doing so.
> But at the same time there is nowhere else to add this at the moment.
> The right place should be the USB core that is aware of host/gadget, phy and the state of the bus.

Understand.

>>> Scratching one's own itch kinda thing...
>>>
>>>>>>>> +static void hi6220_detect_work(struct work_struct *work)
>>>>>>>> +{
>>>>>>>> +    struct hi6220_priv *priv =
>>>>>>>> +        container_of(work, struct hi6220_priv, work.work);
>>>>>>>> +    int gpio_id, gpio_vbus;
>>>>>>>> +    enum usb_otg_state state;
>>>>>>>> +
>>>>>>>> +    if (!gpio_is_valid(priv->gpio_id) || !gpio_is_valid(priv->gpio_vbus))
>>>>>>>> +        return;
>>>>>>>> +
>>>>>>>> +    gpio_id = gpio_get_value_cansleep(priv->gpio_id);
>>>>>>>> +    gpio_vbus = gpio_get_value_cansleep(priv->gpio_vbus);
>>>>>>>
>>>>>>> looks like this should be using extcon
>>>>>> Not used extcon before.
>>>>>> However, we need gpio_vbus interrupt.
>>>>>> Checked phy-tahvo.c and phy-omap-otg.c, not find extcon related with
>>>>>> interrupt.
>>>>>> Will investigate tomorrow.
>>>>>
>>>>> drivers/extcon/extcon-gpio.c
>>>> I think there is no need to use extcon, gpio is clear enough.
>>>> extcon-gpio.c even do not support dt.
>>>
>>> well, add DT. The whole idea of free software is that we improve on
>>> things we already have. EXTCON is *the* API to handle such things.
>>
>
> I wrote the extcon-gpio-usb.c driver for exactly your use case. It is
> queued for v4.1
> https://lkml.org/lkml/2015/2/2/187

That's great, thanks.
>
> It takes care of debouncing for you. Although currently it supports only ID gpio,
> it should be very easy to extend to VBUS sense GPIO.
>
>> I think I am still not understanding extcon-gpio, not sure why need use this API here.
>
> several reasons. Let me list a few.
> 1) Code reuse. Every PHY driver doesn't need to implement GPIO/interrupt handling and debouncing.
> It just registers what cable events it wants to hear and gets a notification.
> 2) The events (ID/VBUS) are not only interesting for the PHY driver but also the controller
> driver and the OTG state machine (whenever it exists at a common place) ;).
> 3) standardization because of common API.

Thanks for the explanation.

Zhangfei
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index ccad880..40a1ef1 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -162,6 +162,15 @@  config PHY_HIX5HD2_SATA
 	help
 	  Support for SATA PHY on Hisilicon hix5hd2 Soc.
 
+config PHY_HI6220_USB
+	tristate "hi6220 USB PHY support"
+	select USB_PHY
+	select MFD_SYSCON
+	help
+	  Enable this to support the HISILICON HI6220 USB PHY.
+
+	  To compile this driver as a module, choose M here.
+
 config PHY_SUN4I_USB
 	tristate "Allwinner sunxi SoC USB PHY driver"
 	depends on ARCH_SUNXI && HAS_IOMEM && OF
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index aa74f96..ec43c2d 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -19,6 +19,7 @@  obj-$(CONFIG_TI_PIPE3)			+= phy-ti-pipe3.o
 obj-$(CONFIG_TWL4030_USB)		+= phy-twl4030-usb.o
 obj-$(CONFIG_PHY_EXYNOS5250_SATA)	+= phy-exynos5250-sata.o
 obj-$(CONFIG_PHY_HIX5HD2_SATA)		+= phy-hix5hd2-sata.o
+obj-$(CONFIG_PHY_HI6220_USB)		+= phy-hi6220-usb.o
 obj-$(CONFIG_PHY_SUN4I_USB)		+= phy-sun4i-usb.o
 obj-$(CONFIG_PHY_SAMSUNG_USB2)		+= phy-exynos-usb2.o
 phy-exynos-usb2-y			+= phy-samsung-usb2.o
diff --git a/drivers/phy/phy-hi6220-usb.c b/drivers/phy/phy-hi6220-usb.c
new file mode 100644
index 0000000..0d9f5ac
--- /dev/null
+++ b/drivers/phy/phy-hi6220-usb.c
@@ -0,0 +1,306 @@ 
+/*
+ * Copyright (c) 2015 Linaro Ltd.
+ * Copyright (c) 2015 Hisilicon Limited.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/clk.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/usb/gadget.h>
+#include <linux/usb/otg.h>
+
+#define SC_PERIPH_CTRL4			0x00c
+
+#define CTRL4_PICO_SIDDQ		BIT(6)
+#define CTRL4_PICO_OGDISABLE		BIT(8)
+#define CTRL4_PICO_VBUSVLDEXT		BIT(10)
+#define CTRL4_PICO_VBUSVLDEXTSEL	BIT(11)
+#define CTRL4_OTG_PHY_SEL		BIT(21)
+
+#define SC_PERIPH_CTRL5			0x010
+
+#define CTRL5_USBOTG_RES_SEL		BIT(3)
+#define CTRL5_PICOPHY_ACAENB		BIT(4)
+#define CTRL5_PICOPHY_BC_MODE		BIT(5)
+#define CTRL5_PICOPHY_CHRGSEL		BIT(6)
+#define CTRL5_PICOPHY_VDATSRCEND	BIT(7)
+#define CTRL5_PICOPHY_VDATDETENB	BIT(8)
+#define CTRL5_PICOPHY_DCDENB		BIT(9)
+#define CTRL5_PICOPHY_IDDIG		BIT(10)
+
+#define SC_PERIPH_CTRL8			0x018
+#define SC_PERIPH_RSTEN0		0x300
+#define SC_PERIPH_RSTDIS0		0x304
+
+#define RST0_USBOTG_BUS			BIT(4)
+#define RST0_POR_PICOPHY		BIT(5)
+#define RST0_USBOTG			BIT(6)
+#define RST0_USBOTG_32K			BIT(7)
+
+#define EYE_PATTERN_PARA		0x7053348c
+
+struct hi6220_priv {
+	struct usb_phy phy;
+	struct delayed_work work;
+	struct regmap *reg;
+	struct clk *clk;
+	struct regulator *vcc;
+	struct device *dev;
+	int gpio_vbus;
+	int gpio_id;
+	enum usb_otg_state state;
+};
+
+static void hi6220_start_peripheral(struct hi6220_priv *priv, bool on)
+{
+	struct usb_otg *otg = priv->phy.otg;
+
+	if (!otg->gadget)
+		return;
+
+	if (on)
+		usb_gadget_connect(otg->gadget);
+	else
+		usb_gadget_disconnect(otg->gadget);
+}
+
+static void hi6220_detect_work(struct work_struct *work)
+{
+	struct hi6220_priv *priv =
+		container_of(work, struct hi6220_priv, work.work);
+	int gpio_id, gpio_vbus;
+	enum usb_otg_state state;
+
+	if (!gpio_is_valid(priv->gpio_id) || !gpio_is_valid(priv->gpio_vbus))
+		return;
+
+	gpio_id = gpio_get_value_cansleep(priv->gpio_id);
+	gpio_vbus = gpio_get_value_cansleep(priv->gpio_vbus);
+
+	if (gpio_vbus == 0) {
+		if (gpio_id == 1)
+			state = OTG_STATE_B_PERIPHERAL;
+		else
+			state = OTG_STATE_A_HOST;
+	} else {
+		state = OTG_STATE_A_HOST;
+	}
+
+	if (priv->state != state) {
+		hi6220_start_peripheral(priv, state == OTG_STATE_B_PERIPHERAL);
+		priv->state = state;
+	}
+}
+
+static irqreturn_t hiusb_gpio_intr(int irq, void *data)
+{
+	struct hi6220_priv *priv = (struct hi6220_priv *)data;
+
+	/* add debounce time */
+	schedule_delayed_work(&priv->work, msecs_to_jiffies(100));
+	return IRQ_HANDLED;
+}
+
+static int hi6220_set_peripheral(struct usb_otg *otg, struct usb_gadget *gadget)
+{
+	otg->gadget = gadget;
+	return 0;
+}
+
+static int hi6220_phy_setup(struct hi6220_priv *priv, bool on)
+{
+	struct regmap *reg = priv->reg;
+	u32 val, mask;
+	int ret;
+
+	if (priv->reg == NULL)
+		return 0;
+
+	if (on) {
+		val = RST0_USBOTG_BUS | RST0_POR_PICOPHY |
+		      RST0_USBOTG | RST0_USBOTG_32K;
+		mask = val;
+		ret = regmap_update_bits(reg, SC_PERIPH_RSTDIS0, mask, val);
+		if (ret)
+			goto out;
+
+		val = CTRL5_USBOTG_RES_SEL | CTRL5_PICOPHY_ACAENB;
+		mask = val | CTRL5_PICOPHY_BC_MODE;
+		ret = regmap_update_bits(reg, SC_PERIPH_CTRL5, mask, val);
+		if (ret)
+			goto out;
+
+		val =  CTRL4_PICO_VBUSVLDEXT | CTRL4_PICO_VBUSVLDEXTSEL |
+		       CTRL4_OTG_PHY_SEL;
+		mask = val | CTRL4_PICO_SIDDQ | CTRL4_PICO_OGDISABLE;
+		ret = regmap_update_bits(reg, SC_PERIPH_CTRL4, mask, val);
+		if (ret)
+			goto out;
+
+		ret = regmap_write(reg, SC_PERIPH_CTRL8, EYE_PATTERN_PARA);
+		if (ret)
+			goto out;
+	} else {
+		val = CTRL4_PICO_SIDDQ;
+		mask = val;
+		ret = regmap_update_bits(reg, SC_PERIPH_CTRL4, mask, val);
+		if (ret)
+			goto out;
+
+		val = RST0_USBOTG_BUS | RST0_POR_PICOPHY |
+		      RST0_USBOTG | RST0_USBOTG_32K;
+		mask = val;
+		ret = regmap_update_bits(reg, SC_PERIPH_RSTEN0, mask, val);
+		if (ret)
+			goto out;
+	}
+
+	return 0;
+out:
+	dev_err(priv->dev, "failed to setup phy ret: %d\n", ret);
+	return ret;
+}
+
+static int hi6220_phy_probe(struct platform_device *pdev)
+{
+	struct hi6220_priv *priv;
+	struct usb_otg *otg;
+	struct device_node *np = pdev->dev.of_node;
+	struct device *dev = &pdev->dev;
+	int ret, irq;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	otg = devm_kzalloc(dev, sizeof(*otg), GFP_KERNEL);
+	if (!otg)
+		return -ENOMEM;
+
+	priv->dev = dev;
+	priv->phy.dev = &pdev->dev;
+	priv->phy.otg = otg;
+	priv->phy.label = "hi6220";
+	priv->phy.type = USB_PHY_TYPE_USB2;
+	otg->set_peripheral = hi6220_set_peripheral;
+	platform_set_drvdata(pdev, priv);
+
+	priv->gpio_vbus = of_get_named_gpio(np, "hisilicon,vbus-gpios", 0);
+	if (priv->gpio_vbus == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+	if (!gpio_is_valid(priv->gpio_vbus)) {
+		dev_err(dev, "invalid gpio %d\n", priv->gpio_vbus);
+		return -ENODEV;
+	}
+
+	priv->gpio_id = of_get_named_gpio(np, "hisilicon,id-gpios", 0);
+	if (priv->gpio_id == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+	if (!gpio_is_valid(priv->gpio_id)) {
+		dev_err(dev, "invalid gpio %d\n", priv->gpio_id);
+		return -ENODEV;
+	}
+
+	priv->reg = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
+					"hisilicon,peripheral-syscon");
+	if (IS_ERR(priv->reg))
+		priv->reg = NULL;
+
+	ret = devm_gpio_request_one(dev, priv->gpio_vbus,
+				    GPIOF_IN, "gpio_vbus");
+	if (ret < 0) {
+		dev_err(dev, "gpio request failed for gpio_vbus\n");
+		return ret;
+	}
+
+	ret = devm_gpio_request_one(dev, priv->gpio_id, GPIOF_IN, "gpio_id");
+	if (ret < 0) {
+		dev_err(dev, "gpio request failed for gpio_id\n");
+		return ret;
+	}
+
+	priv->vcc = devm_regulator_get(dev, "vcc");
+	if (IS_ERR(priv->vcc)) {
+		if (PTR_ERR(priv->vcc) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+		dev_info(dev, "No regulator found\n");
+	} else {
+		ret = regulator_enable(priv->vcc);
+		if (ret) {
+			dev_err(dev, "Failed to enable regulator\n");
+			return -ENODEV;
+		}
+	}
+
+	priv->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(priv->clk)) {
+		regulator_disable(priv->vcc);
+		return PTR_ERR(priv->clk);
+	}
+	clk_prepare_enable(priv->clk);
+	INIT_DELAYED_WORK(&priv->work, hi6220_detect_work);
+
+	irq = gpio_to_irq(priv->gpio_vbus);
+	ret = devm_request_irq(dev, gpio_to_irq(priv->gpio_vbus),
+			       hiusb_gpio_intr, IRQF_NO_SUSPEND |
+			       IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
+			       "vbus_gpio_intr", priv);
+	if (ret) {
+		dev_err(dev, "request gpio irq failed.\n");
+		goto err_irq;
+	}
+
+	hi6220_phy_setup(priv, true);
+	ret = usb_add_phy_dev(&priv->phy);
+	if (ret) {
+		dev_err(dev, "Can't register transceiver\n");
+		goto err_irq;
+	}
+	schedule_delayed_work(&priv->work, 0);
+
+	return 0;
+err_irq:
+	cancel_delayed_work_sync(&priv->work);
+	clk_disable_unprepare(priv->clk);
+	regulator_disable(priv->vcc);
+	return ret;
+}
+
+static int hi6220_phy_remove(struct platform_device *pdev)
+{
+	struct hi6220_priv *priv = platform_get_drvdata(pdev);
+
+	cancel_delayed_work_sync(&priv->work);
+	hi6220_phy_setup(priv, false);
+	clk_disable_unprepare(priv->clk);
+	regulator_disable(priv->vcc);
+	return 0;
+}
+
+static const struct of_device_id hi6220_phy_of_match[] = {
+	{.compatible = "hisilicon,hi6220-usb-phy",},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, hi6220_phy_of_match);
+
+static struct platform_driver hi6220_phy_driver = {
+	.probe	= hi6220_phy_probe,
+	.remove	= hi6220_phy_remove,
+	.driver = {
+		.name	= "hi6220-usb-phy",
+		.of_match_table	= hi6220_phy_of_match,
+	}
+};
+module_platform_driver(hi6220_phy_driver);
+
+MODULE_DESCRIPTION("HISILICON HI6220 USB PHY driver");
+MODULE_ALIAS("platform:hi6220-usb-phy");
+MODULE_LICENSE("GPL");