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

[Neil Armstrong]

On 20/04/2020 17:40, Marek Vasut wrote:
> On 4/20/20 4:47 PM, Neil Armstrong wrote:
>> 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.
> 
> OK, thanks for clarifying. Now, can you document this in the commit
> message, so we have this information stored somewhere for future
> reference ? This would be real useful.
> 

Yes sure.

Neil