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

[Neil Armstrong]

On 20/04/2020 16:03, Marek Vasut wrote:
> On 4/20/20 3:56 PM, Neil Armstrong wrote:
>> On 20/04/2020 15:52, Marek Vasut wrote:
>>> On 4/20/20 3:49 PM, Neil Armstrong wrote:
>>>> On 20/04/2020 15:47, Marek Vasut wrote:
>>>>> On 4/20/20 3:46 PM, Neil Armstrong wrote:
>>>>>> Power on/off the PHYs to enable power to the USB ports, fixing USB support
>>>>>> on Khadas VIM3/VIM3L boards.
>>>>>>
>>>>>> Signed-off-by: Neil Armstrong <narmstrong at baylibre.com>
>>>>>> ---
>>>>>>  drivers/usb/dwc3/dwc3-meson-g12a.c | 16 ++++++++++++++++
>>>>>>  1 file changed, 16 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c b/drivers/usb/dwc3/dwc3-meson-g12a.c
>>>>>> index d4453f8784..8f4a2f3f82 100644
>>>>>> --- a/drivers/usb/dwc3/dwc3-meson-g12a.c
>>>>>> +++ b/drivers/usb/dwc3/dwc3-meson-g12a.c
>>>>>> @@ -408,6 +408,15 @@ static int dwc3_meson_g12a_probe(struct udevice *dev)
>>>>>>  			goto err_phy_init;
>>>>>>  	}
>>>>>>  
>>>>>> +	for (i = 0 ; i < PHY_COUNT ; ++i) {
>>>>>
>>>>> 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

> 
>>>>>
>>>>>> +		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.

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.

Neil