Re: [PATCH 00/16] Add initial USB support for the Renesas RZ/G3S SoC

[claudiu beznea <claudiu.beznea@tuxon.dev>]

On 31.08.2024 08:13, Biju Das wrote:
> Hi Claudiu,
> 
>> -----Original Message-----
>> From: claudiu beznea <claudiu.beznea@tuxon.dev>
>> Sent: Friday, August 30, 2024 9:23 AM
>> Subject: Re: [PATCH 00/16] Add initial USB support for the Renesas RZ/G3S SoC
>>
>> Hi, Ulf,
>>
>> On 29.08.2024 18:26, Ulf Hansson wrote:
>>> On Thu, 22 Aug 2024 at 17:28, Claudiu <claudiu.beznea@tuxon.dev> wrote:
>>>>
>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>
>>>> Hi,
>>>>
>>>> Series adds initial USB support for the Renesas RZ/G3S SoC.
>>>>
>>>> Series is split as follows:
>>>>
>>>> - patch 01/16           - add clock reset and power domain support for USB
>>>> - patch 02-04/16        - add reset control support for a USB signal
>>>>                           that need to be controlled before/after
>>>>                           the power to USB area is turned on/off.
>>>>
>>>>                           Philipp, Ulf, Geert, all,
>>>>
>>>>                           I detailed my approach for this in patch
>>>>                           04/16, please have a look and let me know
>>>>                           your input.
>>>
>>> I have looked briefly. Your suggested approach may work, but I have a
>>> few thoughts, see below.
>>>
>>> If I understand correctly, it is the consumer driver for the device
>>> that is attached to the USB power domain that becomes responsible for
>>> asserting/de-asserting this new signal. Right?
>>
>> Right!
>>
>>>
>>> In this regard, please note that the consumer driver doesn't really
>>> know when the power domain really gets powered-on/off. Calling
>>> pm_runtime_get|put*() is dealing with the reference counting. For
>>> example, a call to pm_runtime_get*() just makes sure that the PM
>>> domain gets-or-remains powered-on. Could this be a problem from the
>>> reset-signal point of view?
>>
>> It should be safe. From the HW manual I understand the hardware block is something like the following:
>>
>>
>>                   USB area
>>          +-------------------------+
>>          |                         |
>>          | PHY --->USB controller  |
>> SYSC --> |  ^                      |
>>          |  |                      |
>>          | PHY reset               |
>>          +-------------------------+
> 
> How USB PWRRDY signal is connected to USB? 

HW manual mentions this in the chapter describing the SYS_USB_PWRRDY register:

Controls PWRRDY terminal of USB

0: PWRRDY

1: PWRRDY down

When turning off the *USB region* power, set this bit to 1.

When turning on the *USB region* power, set this bit to 0

By USB region I get the that it is related to the SoC area where resides
all the USB IPs. I cannot tell more than what is in the hardware manual.


> 
> USB block consists of PHY control, PHY, USB HOST and USB OTG Controller IPs.
> 
> Is it connected to top level block or connected to each IP's for turning off the USB region power?

I cannot tell more than it is in the hardware manual.

> 
> ? Or Just PHY (HW manual mentions for AWO, the USB PWRRDY signal->USB PHY PWRRDY signal control)?
> 
> If the USBPWRRDY signal is connected across modules with this reset signal approach
> then you may need to update bindings [1] with that reset signal
> 
> [1] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20240822152801.602318-12-claudiu.beznea.uj@bp.renesas.com/

If that is true, then this signal may need to be routed to the PHY for
better hardware description.

>  
> 
> Cheers,
> Biju
> 
>>
>> Where:
>> - SYSC is the system controller that controls the new signal for which
>>   I'm requesting opinions in this series
>> - PHY reset: is the block controlling the PHYs
>> - PHY: is the block controlling the USB PHYs
>> - USB controller: is the USB controller
>>
>> Currently, I passed the SYSC signal handling to the PHY reset driver; w/o PHY reset the rest of the
>> USB logic cannot work (neither PHY block nor USB controller).
>>
>> Currently, the PHY reset driver call pm_runtime_resume_and_get() in probe and pm_runtime_put() in
>> remove. The struct reset_control_ops::{assert, deassert} only set specific bits in registers (no
>> pm_runtime* calls).
>>
>> The PHY driver is taking its PHY reset in probe and release it in remove().
>> With this approach the newly introduced SYSC signal will be de-asserted/asserted only in the PHY reset
>> probe/remove (either if it is handled though PM domain or reset control signal).
>>
>> If the SYSC signal would be passed to all the blocks in the USB area (and it would be handled though
>> PM domains) it should be no problem either, AFAICT, because of reference counting the
>> pm_runtime_get|put*() is taking care of. As the PHY reset is the root node the in the devices node
>> tree for USB the reference counting should work, too (I may miss something though, please correct me
>> if I'm wrong).
>>
>> If the SYSC signal would be handled though a reset control driver (as proposed in this series) and we
>> want to pass this reference to all the blocks in the USB area then we can request the reset signal as
>> shared and, AFAIK, this is also reference counted. The devices node tree should help with the order,
>> too, if I'm not wrong.
>>
>> Thank you for looking at this,
>> Claudiu Beznea
>>
>>>
>>> [...]
>>>
>>> Kind regards
>>> Uffe