[PATCH] usb: dwc3-meson-g12a: add power-on/off of the PHYs

[Marek Vasut]

On 4/20/20 4:07 PM, Neil Armstrong wrote:
[...]

>>>>>> Doesn't checkpatch complain about this extra space before semicolon ?
>>>>>
>>>>> Nop, even in --strict
>>>>
>>>> Shouldn't it though ? I thought this extra space was forbidden.
>>>
>>> No idea, this line is copied from the for loop doing the phy init,
>>> itself copied from the Linux code passing all checkpatch checks.
>>
>> Could you change it to for (i = 0; i < PHY_COUNT; i++) please ?
> 
> Sure

Thank you.

( I'm still surprised checkpatch doesn't warn about it, I was quite sure
it was one of the checks at some point, hmmmmm. )

>>>>>>
>>>>>>> +		if (!priv->phys[i].dev)
>>>>>>> +			continue;
>>>>>>> +
>>>>>>> +		ret = generic_phy_power_on(&priv->phys[i]);
>>>>>>
>>>>>> Do we really need to turn on all the PHYs ?
>>>>>>
>>>>>
>>>>> Yes
>>>>
>>>> Then should we have something like clk_bulk_*(), but for phys ?
>>>>
>>>
>>> Eventually, yes, but this goes beyond a fix.
>>
>> The merge window is open for quite a bit longer, so doing this in a
>> generic way would be appreciated.
>>
>> btw while you're at it, shouldn't the PHY_COUNT be replaced with some
>> PHY count detection by reading the DT ? ( I don't have the hardware and
>> I don't know the amlogic SoCs though )
>>
> 
> Hmm, the PHYs list can have some holes (i.e only the second PHY could be used on 3),
> and the name of the PHY determines it's type and position (PHY1 is a special OTG phy)
> this is why we loop over a fixed array and check if the PHY pointer exists.

Um, that sounds like a vaguely familiar problem. Is that an issue with
mapping between controller and a USB PHY ? Is that mapping fixed or is
it configured from DT ? And then, should all PHYs really be powered on,
or should only the PHY matching the currently used controller be powered
on ?

> This won't match a phy_bulk_code directly, I would still need to map the bulk PHYs to this
> array for the OTG and USB3 management.

OK