RE: [RFC/RFT usb-next v1 5/6] usb: chipidea: do not set the "phy" field in struct usb_hcd

[Peter Chen]

 
> > Martin, you design has no problem for most of cases, but some hardware
> > needs special sequence for phy control. I will give an example below.
> great to hear that this should work for most devices!
> 
> >> it would be great if you could explain the issue behind this (and
> >> thereby answer the
> >> question: why we would not want the HCD core to manage the PHY states)!
> >>
> >>
> > Eg, taking Qualcomm USB2 controller as an example, it even does not
> > allow chipidea core to manage its power operation, see
> > CI_HDRC_OVERRIDE_PHY_CONTROL at chipidea driver (usb/chipidea/core.c).
> Its phy_power_on is called after ehci controller reset has finished.
> > (usb/chipidea/ci_hdrc_msm.c).
> I see, thank you for explaining this!
> 
> what do you think about replacing the two following fields from struct 
> usb_hcd:
>   struct usb_phy *usb_phy;
>   struct phy *phy;
> with:
>   /*
>* do not manage the PHY state in the HCD core, instead let the driver 
> handle
>* this (for example if the PHY can only be turned on after a specific 
> event)
>*/
>   bool skip_phy_initialization;
> 
> maybe I should also do this together with my other series which adds the PHY
> wrapper to the HCD core (or even as a separate series, which would be merged
> before this and the PHY wrapper series). what do you think?
> 
> 
 
Hi Martin,

I think it is better to do this in one series.

Peter

Re: [RFC/RFT usb-next v1 5/6] usb: chipidea: do not set the "phy" field in struct usb_hcd

[Martin Blumenstingl]

Hi Peter,

On Mon, Jan 29, 2018 at 4:30 AM, Peter Chen  wrote:
>
>> >
>> >>
>> >> Now that usb_add_hcd parses all generic PHYs anyways the code which
>> >> skips initialization of a single PHY will go away.
>> >> Remove the code which sets struct usb_hcd's phy field from the
>> >> chipidea driver as this field will go away soon.
>> >>
>> >> Signed-off-by: Martin Blumenstingl
>> >> 
>> >> ---
>> >>  drivers/usb/chipidea/host.c | 4 +---
>> >>  1 file changed, 1 insertion(+), 3 deletions(-)
>> >>
>> >> diff --git a/drivers/usb/chipidea/host.c
>> >> b/drivers/usb/chipidea/host.c index 19d60ed7e41f..fc324767cb0f 100644
>> >> --- a/drivers/usb/chipidea/host.c
>> >> +++ b/drivers/usb/chipidea/host.c
>> >> @@ -124,9 +124,7 @@ static int host_start(struct ci_hdrc *ci)
>> >>
>> >>   hcd->power_budget = ci->platdata->power_budget;
>> >>   hcd->tpl_support = ci->platdata->tpl_support;
>> >> - if (ci->phy)
>> >> - hcd->phy = ci->phy;
>> >> - else
>> >> + if (!ci->phy)
>> >>   hcd->usb_phy = ci->usb_phy;
>> >>
>> >
>> > The reason hcd->phy is initialized by chipidea core is we do not need
>> > HCD core to touch PHY, and PHY operation is shared for both device and host
>> mode for chipidea.
>> Chunfeng wanted me to drop the mtu3 patch because I forgot about device mode 
>> in
>> the mtu3 driver.
>> however, the chipidea driver seems to be different because I'm not dropping 
>> the
>> whole phy_{init,power_on,power_off,exit} code from it, but only a "flag" 
>> that tells the
>> HCD core to skip managing the USB PHY
>>
>> > If I understand correct, your HCD core PHY wrapper patch set will do
>> > PHY operation if there is a "phy" node under controller's? If it is
>> > correct, you may supply one way to let the HCD core bypass phy operations 
>> > for
>> some USB controllers, eg dual-role controllers.
>> > Thanks.
>> could you please explain why the HCD core should not manage the the PHYs when
>> the chipidea driver is used?
>>
>> my understanding is that all phy_{init,power_on,power_off,exit}
>> operations are ref-counted internally in the PHY framework this means that 
>> if the
>> chipidea driver calls phy_{init,power_on} first then the same functions 
>> called from
>> within the HCD core are no-ops (except for the ref-counting) so I think it 
>> should not
>> change anything - however, I have no hardware to actually prove that.
>
> Martin, you design has no problem for most of cases, but some hardware needs 
> special
> sequence for phy control. I will give an example below.
great to hear that this should work for most devices!

>> it would be great if you could explain the issue behind this (and thereby 
>> answer the
>> question: why we would not want the HCD core to manage the PHY states)!
>>
>>
> Eg, taking Qualcomm USB2 controller as an example, it even does not allow 
> chipidea core
> to manage its power operation, see CI_HDRC_OVERRIDE_PHY_CONTROL at chipidea 
> driver
> (usb/chipidea/core.c). Its phy_power_on is called after ehci controller reset 
> has finished.
> (usb/chipidea/ci_hdrc_msm.c).
I see, thank you for explaining this!

what do you think about replacing the two following fields from struct usb_hcd:
  struct usb_phy *usb_phy;
  struct phy *phy;
with:
  /*
   * do not manage the PHY state in the HCD core, instead let the driver handle
   * this (for example if the PHY can only be turned on after a specific event)
   */
  bool skip_phy_initialization;

maybe I should also do this together with my other series which adds
the PHY wrapper to the HCD core (or even as a separate series, which
would be merged before this and the PHY wrapper series). what do you
think?


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

RE: [RFC/RFT usb-next v1 5/6] usb: chipidea: do not set the "phy" field in struct usb_hcd

[Peter Chen]

 
> >
> >>
> >> Now that usb_add_hcd parses all generic PHYs anyways the code which
> >> skips initialization of a single PHY will go away.
> >> Remove the code which sets struct usb_hcd's phy field from the
> >> chipidea driver as this field will go away soon.
> >>
> >> Signed-off-by: Martin Blumenstingl
> >> 
> >> ---
> >>  drivers/usb/chipidea/host.c | 4 +---
> >>  1 file changed, 1 insertion(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/usb/chipidea/host.c
> >> b/drivers/usb/chipidea/host.c index 19d60ed7e41f..fc324767cb0f 100644
> >> --- a/drivers/usb/chipidea/host.c
> >> +++ b/drivers/usb/chipidea/host.c
> >> @@ -124,9 +124,7 @@ static int host_start(struct ci_hdrc *ci)
> >>
> >>   hcd->power_budget = ci->platdata->power_budget;
> >>   hcd->tpl_support = ci->platdata->tpl_support;
> >> - if (ci->phy)
> >> - hcd->phy = ci->phy;
> >> - else
> >> + if (!ci->phy)
> >>   hcd->usb_phy = ci->usb_phy;
> >>
> >
> > The reason hcd->phy is initialized by chipidea core is we do not need
> > HCD core to touch PHY, and PHY operation is shared for both device and host
> mode for chipidea.
> Chunfeng wanted me to drop the mtu3 patch because I forgot about device mode 
> in
> the mtu3 driver.
> however, the chipidea driver seems to be different because I'm not dropping 
> the
> whole phy_{init,power_on,power_off,exit} code from it, but only a "flag" that 
> tells the
> HCD core to skip managing the USB PHY
> 
> > If I understand correct, your HCD core PHY wrapper patch set will do
> > PHY operation if there is a "phy" node under controller's? If it is
> > correct, you may supply one way to let the HCD core bypass phy operations 
> > for
> some USB controllers, eg dual-role controllers.
> > Thanks.
> could you please explain why the HCD core should not manage the the PHYs when
> the chipidea driver is used?
> 
> my understanding is that all phy_{init,power_on,power_off,exit}
> operations are ref-counted internally in the PHY framework this means that if 
> the
> chipidea driver calls phy_{init,power_on} first then the same functions 
> called from
> within the HCD core are no-ops (except for the ref-counting) so I think it 
> should not
> change anything - however, I have no hardware to actually prove that.

Martin, you design has no problem for most of cases, but some hardware needs 
special
sequence for phy control. I will give an example below.

> it would be great if you could explain the issue behind this (and thereby 
> answer the
> question: why we would not want the HCD core to manage the PHY states)!
> 
> 
Eg, taking Qualcomm USB2 controller as an example, it even does not allow 
chipidea core
to manage its power operation, see CI_HDRC_OVERRIDE_PHY_CONTROL at chipidea 
driver
(usb/chipidea/core.c). Its phy_power_on is called after ehci controller reset 
has finished. 
(usb/chipidea/ci_hdrc_msm.c).

Peter

Re: [RFC/RFT usb-next v1 5/6] usb: chipidea: do not set the "phy" field in struct usb_hcd

[Martin Blumenstingl]

Hi Peter,

On Fri, Jan 26, 2018 at 10:06 AM, Peter Chen  wrote:
>
>>
>> Now that usb_add_hcd parses all generic PHYs anyways the code which skips
>> initialization of a single PHY will go away.
>> Remove the code which sets struct usb_hcd's phy field from the chipidea 
>> driver as
>> this field will go away soon.
>>
>> Signed-off-by: Martin Blumenstingl 
>> ---
>>  drivers/usb/chipidea/host.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c index
>> 19d60ed7e41f..fc324767cb0f 100644
>> --- a/drivers/usb/chipidea/host.c
>> +++ b/drivers/usb/chipidea/host.c
>> @@ -124,9 +124,7 @@ static int host_start(struct ci_hdrc *ci)
>>
>>   hcd->power_budget = ci->platdata->power_budget;
>>   hcd->tpl_support = ci->platdata->tpl_support;
>> - if (ci->phy)
>> - hcd->phy = ci->phy;
>> - else
>> + if (!ci->phy)
>>   hcd->usb_phy = ci->usb_phy;
>>
>
> The reason hcd->phy is initialized by chipidea core is we do not need HCD 
> core to
> touch PHY, and PHY operation is shared for both device and host mode for 
> chipidea.
Chunfeng wanted me to drop the mtu3 patch because I forgot about
device mode in the mtu3 driver.
however, the chipidea driver seems to be different because I'm not
dropping the whole phy_{init,power_on,power_off,exit} code from it,
but only a "flag" that tells the HCD core to skip managing the USB PHY

> If I understand correct, your HCD core PHY wrapper patch set will do PHY 
> operation if
> there is a "phy" node under controller's? If it is correct, you may supply 
> one way to let
> the HCD core bypass phy operations for some USB controllers, eg dual-role 
> controllers.
> Thanks.
could you please explain why the HCD core should not manage the the
PHYs when the chipidea driver is used?

my understanding is that all phy_{init,power_on,power_off,exit}
operations are ref-counted internally in the PHY framework
this means that if the chipidea driver calls phy_{init,power_on} first
then the same functions called from within the HCD core are no-ops
(except for the ref-counting)
so I think it should not change anything - however, I have no hardware
to actually prove that.
it would be great if you could explain the issue behind this (and
thereby answer the question: why we would not want the HCD core to
manage the PHY states)!


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

RE: [RFC/RFT usb-next v1 5/6] usb: chipidea: do not set the "phy" field in struct usb_hcd

[Peter Chen]

 
> 
> Now that usb_add_hcd parses all generic PHYs anyways the code which skips
> initialization of a single PHY will go away.
> Remove the code which sets struct usb_hcd's phy field from the chipidea 
> driver as
> this field will go away soon.
> 
> Signed-off-by: Martin Blumenstingl 
> ---
>  drivers/usb/chipidea/host.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c index
> 19d60ed7e41f..fc324767cb0f 100644
> --- a/drivers/usb/chipidea/host.c
> +++ b/drivers/usb/chipidea/host.c
> @@ -124,9 +124,7 @@ static int host_start(struct ci_hdrc *ci)
> 
>   hcd->power_budget = ci->platdata->power_budget;
>   hcd->tpl_support = ci->platdata->tpl_support;
> - if (ci->phy)
> - hcd->phy = ci->phy;
> - else
> + if (!ci->phy)
>   hcd->usb_phy = ci->usb_phy;
> 

The reason hcd->phy is initialized by chipidea core is we do not need HCD core 
to
touch PHY, and PHY operation is shared for both device and host mode for 
chipidea.

If I understand correct, your HCD core PHY wrapper patch set will do PHY 
operation if
there is a "phy" node under controller's? If it is correct, you may supply one 
way to let
the HCD core bypass phy operations for some USB controllers, eg dual-role 
controllers.
Thanks.

Peter

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