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

[Marek Vasut]

On 4/21/20 10:09 AM, Neil Armstrong wrote:
> 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.

Thanks