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

[Neil Armstrong]

On 20/04/2020 16:10, Marek Vasut wrote:
> 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 ? 

Mapping is done via DT, all PHYs are optional depending on the board layout.

> And then, should all PHYs really be powered on,
> or should only the PHY matching the currently used controller be powered
> on ?

All PHYs are connected to a glue that muxes 1 PHY between DWC2 and DWC3, and
the other USB2 PHYs (for now 1, but previous SoCs had 2) to DWC3, the USB3 PHY
is muxed between PCIe to dw-pcie and DWC3.

So we power-on only the PHYs specified in DT, thus the:
	if (!priv->phys[i].dev)
		continue;

I attached the USB complex diagram if you need it.

Neil

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: G12A USB.svg
Type: image/svg+xml
Size: 51866 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200420/80913a8d/attachment-0001.svg>