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.
			  
			  Thank you!

- patch 05/16		- moves SoC identification to SYSC driver
- patch 06-08/16	- updates USB PHY control driver for USB
			  support
- patch 09/16		- update documentation for USBHS
- patch 10-12/16	- updates the USB PHY driver for USB support
- patch 13-15/16	- updates the device tree with USB support
- patch 16/16		- enables the reset control driver

Thank you,
Claudiu Beznea

Claudiu Beznea (16):
  clk: renesas: r9a08g045: Add clocks, resets and power domains for USB
  dt-bindings: soc: renesas: renesas,rzg2l-sysc: Add #reset-cells for
    RZ/G3S
  dt-bindings: reset: renesas,r9a08g045-sysc: Add reset IDs for RZ/G3S
    SYSC reset
  soc: renesas: Add SYSC driver for Renesas RZ/G3S
  soc: renesas: sysc: Move RZ/G3S SoC detection on SYSC driver
  dt-bindings: reset: renesas,rzg2l-usbphy-ctrl: Document RZ/G3S SoC
  reset: rzg2l-usbphy-ctrl: Get reset control array
  reset: rzg2l-usbphy-ctrl: Add support for RZ/G3S
  dt-bindings: usb: renesas,usbhs: Document RZ/G3S SoC
  phy: renesas: rcar-gen3-usb2: Add support to initialize the bus
  dt-bindings: phy: renesas,usb2-phy: Document RZ/G3S phy bindings
  phy: renesas: rcar-gen3-usb2: Add support for the RZ/G3S SoC
  arm64: dts: renesas: Add #reset-cells to system controller node
  arm64: dts: renesas: r9a08g045: Add USB support
  arm64: dts: renesas: rzg3s-smarc: Enable USB support
  arm64: defconfig: Enable RZ/G3S SYSC reset driver

 .../bindings/phy/renesas,usb2-phy.yaml        |   4 +-
 .../reset/renesas,rzg2l-usbphy-ctrl.yaml      |  35 +++-
 .../soc/renesas/renesas,rzg2l-sysc.yaml       |  16 ++
 .../bindings/usb/renesas,usbhs.yaml           |   2 +
 arch/arm64/boot/dts/renesas/r9a08g045.dtsi    | 120 +++++++++++++
 arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi  |  61 +++++++
 arch/arm64/configs/defconfig                  |   1 +
 drivers/clk/renesas/r9a08g045-cpg.c           |  17 ++
 drivers/phy/renesas/phy-rcar-gen3-usb2.c      |  60 ++++++-
 drivers/reset/Kconfig                         |   7 +
 drivers/reset/Makefile                        |   1 +
 drivers/reset/reset-rzg2l-usbphy-ctrl.c       |   3 +-
 drivers/reset/reset-rzg3s-sysc.c              | 140 ++++++++++++++++
 drivers/soc/renesas/Makefile                  |   1 +
 drivers/soc/renesas/renesas-soc.c             |  12 --
 drivers/soc/renesas/rzg3s-sysc.c              | 158 ++++++++++++++++++
 .../reset/renesas,r9a08g045-sysc.h            |  10 ++
 include/linux/soc/renesas/rzg3s-sysc-reset.h  |  24 +++
 18 files changed, 648 insertions(+), 24 deletions(-)
 create mode 100644 drivers/reset/reset-rzg3s-sysc.c
 create mode 100644 drivers/soc/renesas/rzg3s-sysc.c
 create mode 100644 include/dt-bindings/reset/renesas,r9a08g045-sysc.h
 create mode 100644 include/linux/soc/renesas/rzg3s-sysc-reset.h

-- 
2.39.2

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?

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?

[...]

Kind regards
Uffe

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

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

On Fri, 30 Aug 2024 at 10:22, claudiu beznea <claudiu.beznea@tuxon.dev> wrote:
>
> 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               |
>          +-------------------------+
>
> 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).

Thanks for clarifying!

For my understanding, in what register range do these bits belong? Is
it the USB logic or in the PM domain logic, or something else.

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

Reference counting a reset signal sounds a bit weird to me, but I
guess it can work. :-)

To sum up from my side;

As long as it's fine that we may end up asserting/de-asserting the
reset-signal, without actually knowing if the PM domain is getting
turn-on/off, then using a reset-control like what you propose seems
okay to me.

If not, there are two other options that can be considered I think.
*) Using the genpd on/off notifiers, to really allow the consumer
driver of the reset-control to know when the PM domain gets turned
on/off.
**) Move the entire reset handling into the PM domain provider, as it
obviously knows when the domain is getting turned on/off.

Thanks again for your explanations!

Kind regards
Uffe

On 30.08.2024 13:14, Ulf Hansson wrote:
> On Fri, 30 Aug 2024 at 10:22, claudiu beznea <claudiu.beznea@tuxon.dev> wrote:
>>
>> 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               |
>>          +-------------------------+
>>
>> 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).
> 
> Thanks for clarifying!
> 
> For my understanding, in what register range do these bits belong? Is
> it the USB logic or in the PM domain logic, or something else.

The PHY reset block is an individual hardware block with its own address
space, clocks and reset signals.

The PHY block may reside in the same address space with USB controller but
it can provide PHY support for an external USB controller, something like:

+--------------------------+
|  PHY ---> USB controller |
|   |                      |
+---|----------------------+
   \/
+---------------+
| USB controller|
+---------------+

Because of this the PHY block is modeled in Linux as a standalone driver.

And SYSC is an individual HW block with its own address space. This is
where it resides the control of the signal for which I'm asking for directions.

> 
>>
>> 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.
> 
> Reference counting a reset signal sounds a bit weird to me, but I
> guess it can work. :-)
> 
> To sum up from my side;
> 
> As long as it's fine that we may end up asserting/de-asserting the
> reset-signal, without actually knowing if the PM domain is getting
> turn-on/off, 

With my understanding of it, that should not happen, at least not with the
current implementation of the drivers involved in this.

> then using a reset-control like what you propose seems
> okay to me.

I would prefer this option, too.

> 
> If not, there are two other options that can be considered I think.
> *) Using the genpd on/off notifiers, to really allow the consumer
> driver of the reset-control to know when the PM domain gets turned
> on/off.
> **) Move the entire reset handling into the PM domain provider, as it
> obviously knows when the domain is getting turned on/off.

This option is what I've explored, tested on my side.

I explored it in 2 ways:

1/ SYSC modeled as an individual PM domain provider (this is more
   appropriate to how HW manual described the hardware) with this the PHY
   reset DT node would have to get 2 PM domains handlers (one for the
   current PM domain provider and the other one for SYSC):

+               phyrst: usbphy-ctrl@11e00000 {
+                       compatible = "renesas,r9a08g045-usbphy-ctrl";
+                       reg = <0 0x11e00000 0 0x10000>;
+                       clocks = <&cpg CPG_MOD R9A08G045_USB_PCLK>;
+                       resets = <&cpg R9A08G045_USB_PRESETN>;
+                       power-domain-names = "cpg", "sysc";
+                       power-domains = <&cpg R9A08G045_PD_USB_PHY>, <&sysc
R9A08G045_SYSC_PD_USB>;
+                       #reset-cells = <1>;
+                       status = "disabled";
+
+                       usb0_vbus_otg: regulator-vbus {
+                               regulator-name = "vbus";
+                       };
+               };
+

and the PHY reset driver will get bulky with powering on/off both of these,
at least with my current implementation, something like (and the following
code is in probe()):

+       if (priv->set_power) {
+               priv->cpg_genpd_dev = dev_pm_domain_attach_by_name(dev, "cpg");
+               if (IS_ERR(priv->cpg_genpd_dev)) {
+                       dev_err_probe(dev, error, "Failed to attach CPG PM
domain!");
+                       error = PTR_ERR(priv->cpg_genpd_dev);
+                       goto err_pm_runtime_put;
+               }
+
+               priv->sysc_genpd_dev = dev_pm_domain_attach_by_name(dev,
"sysc");
+               if (IS_ERR(priv->sysc_genpd_dev)) {
+                       dev_err_probe(dev, error, "Failed to attach sysc PM
domain!");
+                       error = PTR_ERR(priv->sysc_genpd_dev);
+                       goto err_genpd_cpg_detach;
+               }
+
+               priv->cpg_genpd_dl = device_link_add(dev, priv->cpg_genpd_dev,
+                                                    DL_FLAG_PM_RUNTIME |
+                                                    DL_FLAG_STATELESS);
+               if (!priv->cpg_genpd_dl) {
+                       dev_err_probe(dev, -ENOMEM, "Failed to add CPG
genpd device link!");
+                       goto err_genpd_sysc_detach;
+               }
+
+               priv->sysc_genpd_dl = device_link_add(dev,
priv->sysc_genpd_dev,
+                                                     DL_FLAG_PM_RUNTIME |
+                                                     DL_FLAG_STATELESS);
+               if (!priv->sysc_genpd_dl) {
+                       dev_err_probe(dev, -ENOMEM, "Failed to add sysc
genpd device link!");
+                       goto err_genpd_cpg_dl_del;
+               }
+
+
+               error = pm_runtime_resume_and_get(priv->cpg_genpd_dev);
+               if (error) {
+                       dev_err_probe(dev, error, "Failed to runtime resume
cpg PM domain!");
+                       goto err_genpd_sysc_dl_del;
+               }
+
+               error = pm_runtime_resume_and_get(priv->sysc_genpd_dev);
+               if (error) {
+                       dev_err_probe(dev, error, "Failed to runtime resume
sysc PM domain!");
+                       goto err_genpd_cpg_off;
+               }
+       }
+

2/ SYSC being a PM domain provider parent of the CPG (current PM domain
   provider). With this the phy reset node is like proposed in this series
   (powered by CPG PM domain):

+               phyrst: usbphy-ctrl@11e00000 {
+                       compatible = "renesas,r9a08g045-usbphy-ctrl",
+                                    "renesas,rzg2l-usbphy-ctrl";
+                       reg = <0 0x11e00000 0 0x10000>;
+                       clocks = <&cpg CPG_MOD R9A08G045_USB_PCLK>;
+                       resets = <&cpg R9A08G045_USB_PRESETN>;
+                       power-domains = <&cpg R9A08G045_PD_USB_PHY>;
+                       #reset-cells = <1>;
+                       status = "disabled";
+
+                       usb0_vbus_otg: regulator-vbus {
+                               regulator-name = "vbus";
+                       };
+               };

And the USB SYSC PM domain is parent for all USB PM domains provided by CPG
(3 in this case). With this there should be some glue code b/w CPG (code in
drivers/clk/renesas/{rzg2l-cpg.c, r9a08g045-cpg.c}) and SYSC drivers (I
have something ugly locally, haven't tried to detach CPG code from SYSC
code at the moment).


> 
> Thanks again for your explanations!

Thank you, also, for looking into this,
Claudiu Beznea

> 
> Kind regards
> Uffe

[...]

> >
> > If not, there are two other options that can be considered I think.
> > *) Using the genpd on/off notifiers, to really allow the consumer
> > driver of the reset-control to know when the PM domain gets turned
> > on/off.
> > **) Move the entire reset handling into the PM domain provider, as it
> > obviously knows when the domain is getting turned on/off.
>
> This option is what I've explored, tested on my side.
>
> I explored it in 2 ways:
>
> 1/ SYSC modeled as an individual PM domain provider (this is more
>    appropriate to how HW manual described the hardware) with this the PHY
>    reset DT node would have to get 2 PM domains handlers (one for the
>    current PM domain provider and the other one for SYSC):
>
> +               phyrst: usbphy-ctrl@11e00000 {
> +                       compatible = "renesas,r9a08g045-usbphy-ctrl";
> +                       reg = <0 0x11e00000 0 0x10000>;
> +                       clocks = <&cpg CPG_MOD R9A08G045_USB_PCLK>;
> +                       resets = <&cpg R9A08G045_USB_PRESETN>;
> +                       power-domain-names = "cpg", "sysc";
> +                       power-domains = <&cpg R9A08G045_PD_USB_PHY>, <&sysc
> R9A08G045_SYSC_PD_USB>;
> +                       #reset-cells = <1>;
> +                       status = "disabled";
> +
> +                       usb0_vbus_otg: regulator-vbus {
> +                               regulator-name = "vbus";
> +                       };
> +               };
> +

According to what you have described earlier/above, modelling the SYSC
as a PM domain provider seems like a better description of the HW to
me. Although, as I said earlier, if you prefer the reset approach, I
would not object to that.

>
> and the PHY reset driver will get bulky with powering on/off both of these,
> at least with my current implementation, something like (and the following
> code is in probe()):
>
> +       if (priv->set_power) {
> +               priv->cpg_genpd_dev = dev_pm_domain_attach_by_name(dev, "cpg");
> +               if (IS_ERR(priv->cpg_genpd_dev)) {
> +                       dev_err_probe(dev, error, "Failed to attach CPG PM
> domain!");
> +                       error = PTR_ERR(priv->cpg_genpd_dev);
> +                       goto err_pm_runtime_put;
> +               }
> +
> +               priv->sysc_genpd_dev = dev_pm_domain_attach_by_name(dev,
> "sysc");
> +               if (IS_ERR(priv->sysc_genpd_dev)) {
> +                       dev_err_probe(dev, error, "Failed to attach sysc PM
> domain!");
> +                       error = PTR_ERR(priv->sysc_genpd_dev);
> +                       goto err_genpd_cpg_detach;
> +               }
> +
> +               priv->cpg_genpd_dl = device_link_add(dev, priv->cpg_genpd_dev,
> +                                                    DL_FLAG_PM_RUNTIME |
> +                                                    DL_FLAG_STATELESS);
> +               if (!priv->cpg_genpd_dl) {
> +                       dev_err_probe(dev, -ENOMEM, "Failed to add CPG
> genpd device link!");
> +                       goto err_genpd_sysc_detach;
> +               }
> +
> +               priv->sysc_genpd_dl = device_link_add(dev,
> priv->sysc_genpd_dev,
> +                                                     DL_FLAG_PM_RUNTIME |
> +                                                     DL_FLAG_STATELESS);
> +               if (!priv->sysc_genpd_dl) {
> +                       dev_err_probe(dev, -ENOMEM, "Failed to add sysc
> genpd device link!");
> +                       goto err_genpd_cpg_dl_del;
> +               }
> +
> +
> +               error = pm_runtime_resume_and_get(priv->cpg_genpd_dev);
> +               if (error) {
> +                       dev_err_probe(dev, error, "Failed to runtime resume
> cpg PM domain!");
> +                       goto err_genpd_sysc_dl_del;
> +               }
> +
> +               error = pm_runtime_resume_and_get(priv->sysc_genpd_dev);
> +               if (error) {
> +                       dev_err_probe(dev, error, "Failed to runtime resume
> sysc PM domain!");
> +                       goto err_genpd_cpg_off;
> +               }
> +       }

Indeed, the code above looks bulky.

Fortunately, we now have dev|devm_pm_domain_attach_list(), which
replaces all of the code above.

[...]

Kind regards
Uffe

On Sat, 31 Aug 2024 at 12:32, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> [...]
>
> > >
> > > If not, there are two other options that can be considered I think.
> > > *) Using the genpd on/off notifiers, to really allow the consumer
> > > driver of the reset-control to know when the PM domain gets turned
> > > on/off.
> > > **) Move the entire reset handling into the PM domain provider, as it
> > > obviously knows when the domain is getting turned on/off.
> >
> > This option is what I've explored, tested on my side.
> >
> > I explored it in 2 ways:
> >
> > 1/ SYSC modeled as an individual PM domain provider (this is more
> >    appropriate to how HW manual described the hardware) with this the PHY
> >    reset DT node would have to get 2 PM domains handlers (one for the
> >    current PM domain provider and the other one for SYSC):
> >
> > +               phyrst: usbphy-ctrl@11e00000 {
> > +                       compatible = "renesas,r9a08g045-usbphy-ctrl";
> > +                       reg = <0 0x11e00000 0 0x10000>;
> > +                       clocks = <&cpg CPG_MOD R9A08G045_USB_PCLK>;
> > +                       resets = <&cpg R9A08G045_USB_PRESETN>;
> > +                       power-domain-names = "cpg", "sysc";
> > +                       power-domains = <&cpg R9A08G045_PD_USB_PHY>, <&sysc
> > R9A08G045_SYSC_PD_USB>;
> > +                       #reset-cells = <1>;
> > +                       status = "disabled";
> > +
> > +                       usb0_vbus_otg: regulator-vbus {
> > +                               regulator-name = "vbus";
> > +                       };
> > +               };
> > +
>
> According to what you have described earlier/above, modelling the SYSC
> as a PM domain provider seems like a better description of the HW to
> me. Although, as I said earlier, if you prefer the reset approach, I
> would not object to that.

Following the discussion I believe I should take this back. If I
understand correctly, SYSC signal seems best to be modelled as a
reset.

 Although, it looks like the USB PM domain provider should rather be
the consumer of that reset, instead of having the reset being consumed
by the consumers of the USB PM domain.

Did that make sense?

[...]

Kind regards
Uffe

On 03.09.2024 13:35, Ulf Hansson wrote:
> On Sat, 31 Aug 2024 at 12:32, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>
>> [...]
>>
>>>>
>>>> If not, there are two other options that can be considered I think.
>>>> *) Using the genpd on/off notifiers, to really allow the consumer
>>>> driver of the reset-control to know when the PM domain gets turned
>>>> on/off.
>>>> **) Move the entire reset handling into the PM domain provider, as it
>>>> obviously knows when the domain is getting turned on/off.
>>>
>>> This option is what I've explored, tested on my side.
>>>
>>> I explored it in 2 ways:
>>>
>>> 1/ SYSC modeled as an individual PM domain provider (this is more
>>>    appropriate to how HW manual described the hardware) with this the PHY
>>>    reset DT node would have to get 2 PM domains handlers (one for the
>>>    current PM domain provider and the other one for SYSC):
>>>
>>> +               phyrst: usbphy-ctrl@11e00000 {
>>> +                       compatible = "renesas,r9a08g045-usbphy-ctrl";
>>> +                       reg = <0 0x11e00000 0 0x10000>;
>>> +                       clocks = <&cpg CPG_MOD R9A08G045_USB_PCLK>;
>>> +                       resets = <&cpg R9A08G045_USB_PRESETN>;
>>> +                       power-domain-names = "cpg", "sysc";
>>> +                       power-domains = <&cpg R9A08G045_PD_USB_PHY>, <&sysc
>>> R9A08G045_SYSC_PD_USB>;
>>> +                       #reset-cells = <1>;
>>> +                       status = "disabled";
>>> +
>>> +                       usb0_vbus_otg: regulator-vbus {
>>> +                               regulator-name = "vbus";
>>> +                       };
>>> +               };
>>> +
>>
>> According to what you have described earlier/above, modelling the SYSC
>> as a PM domain provider seems like a better description of the HW to
>> me. Although, as I said earlier, if you prefer the reset approach, I
>> would not object to that.
> 
> Following the discussion I believe I should take this back. If I
> understand correctly, SYSC signal seems best to be modelled as a
> reset.
> 
>  Although, it looks like the USB PM domain provider should rather be
> the consumer of that reset, instead of having the reset being consumed
> by the consumers of the USB PM domain.

The PM domain provider for USB is the provider for the rest of IPs. To work
like this the SYSC these signals should be handled in the USB domains power
on/off function. It's not impossible to have it implemented like this but
it will complicate a bit the code, AFAICT. This will not describe the
hardware, also.

With the information that we had up to yesterday, the connection b/w HW
blocks was something as follows:

                 USB area
              +--------------------------+
      sig     | PHY -> USB controller X  |
SYSC -------->|  ^                       |
              |  |                       |
              | PHY reset                |
              +--------------------------+

In this implementation the SYSC signal was connected to PHY reset block as
it is the root of the devices used in the USB setup and no USB
functionality can exist w/o the PHY reset being setup.

There is a new information arrived just yesterday from hardware team saying
this about SYSC signals: "When turning off USB PHY and PCIe PHY, if they
are not controlled, PHY may break" which may means that it is just
connected to the PHYs not to the USB area/region or PCIe area/region as
initially expressed in HW manual.

With that the HW connection b/w the USB devices and SYSC might become
something like:

                 USB area
              +--------------------------+
     sig   +--->PHY -> USB controller X  |
SYSC ------+  |  ^                       |
              |  |                       |
              | PHY reset                |
              +--------------------------+

I haven't got the chance to test this topology, though.

With this new information would you be OK to still have it as a reset
signal and connected only to the PHY driver ?

Thank you,
Claudiu Beznea

> 
> Did that make sense?
> 
> [...]
> 
> Kind regards
> Uffe

On Tue, 3 Sept 2024 at 12:58, claudiu beznea <claudiu.beznea@tuxon.dev> wrote:
>
>
>
> On 03.09.2024 13:35, Ulf Hansson wrote:
> > On Sat, 31 Aug 2024 at 12:32, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >>
> >> [...]
> >>
> >>>>
> >>>> If not, there are two other options that can be considered I think.
> >>>> *) Using the genpd on/off notifiers, to really allow the consumer
> >>>> driver of the reset-control to know when the PM domain gets turned
> >>>> on/off.
> >>>> **) Move the entire reset handling into the PM domain provider, as it
> >>>> obviously knows when the domain is getting turned on/off.
> >>>
> >>> This option is what I've explored, tested on my side.
> >>>
> >>> I explored it in 2 ways:
> >>>
> >>> 1/ SYSC modeled as an individual PM domain provider (this is more
> >>>    appropriate to how HW manual described the hardware) with this the PHY
> >>>    reset DT node would have to get 2 PM domains handlers (one for the
> >>>    current PM domain provider and the other one for SYSC):
> >>>
> >>> +               phyrst: usbphy-ctrl@11e00000 {
> >>> +                       compatible = "renesas,r9a08g045-usbphy-ctrl";
> >>> +                       reg = <0 0x11e00000 0 0x10000>;
> >>> +                       clocks = <&cpg CPG_MOD R9A08G045_USB_PCLK>;
> >>> +                       resets = <&cpg R9A08G045_USB_PRESETN>;
> >>> +                       power-domain-names = "cpg", "sysc";
> >>> +                       power-domains = <&cpg R9A08G045_PD_USB_PHY>, <&sysc
> >>> R9A08G045_SYSC_PD_USB>;
> >>> +                       #reset-cells = <1>;
> >>> +                       status = "disabled";
> >>> +
> >>> +                       usb0_vbus_otg: regulator-vbus {
> >>> +                               regulator-name = "vbus";
> >>> +                       };
> >>> +               };
> >>> +
> >>
> >> According to what you have described earlier/above, modelling the SYSC
> >> as a PM domain provider seems like a better description of the HW to
> >> me. Although, as I said earlier, if you prefer the reset approach, I
> >> would not object to that.
> >
> > Following the discussion I believe I should take this back. If I
> > understand correctly, SYSC signal seems best to be modelled as a
> > reset.
> >
> >  Although, it looks like the USB PM domain provider should rather be
> > the consumer of that reset, instead of having the reset being consumed
> > by the consumers of the USB PM domain.
>
> The PM domain provider for USB is the provider for the rest of IPs. To work
> like this the SYSC these signals should be handled in the USB domains power
> on/off function. It's not impossible to have it implemented like this but
> it will complicate a bit the code, AFAICT. This will not describe the
> hardware, also.
>
> With the information that we had up to yesterday, the connection b/w HW
> blocks was something as follows:
>
>                  USB area
>               +--------------------------+
>       sig     | PHY -> USB controller X  |
> SYSC -------->|  ^                       |
>               |  |                       |
>               | PHY reset                |
>               +--------------------------+
>
> In this implementation the SYSC signal was connected to PHY reset block as
> it is the root of the devices used in the USB setup and no USB
> functionality can exist w/o the PHY reset being setup.
>
> There is a new information arrived just yesterday from hardware team saying
> this about SYSC signals: "When turning off USB PHY and PCIe PHY, if they
> are not controlled, PHY may break" which may means that it is just
> connected to the PHYs not to the USB area/region or PCIe area/region as
> initially expressed in HW manual.
>
> With that the HW connection b/w the USB devices and SYSC might become
> something like:
>
>                  USB area
>               +--------------------------+
>      sig   +--->PHY -> USB controller X  |
> SYSC ------+  |  ^                       |
>               |  |                       |
>               | PHY reset                |
>               +--------------------------+
>
> I haven't got the chance to test this topology, though.
>
> With this new information would you be OK to still have it as a reset
> signal and connected only to the PHY driver ?

As long as it's a better description of the HW, I am fine with that too.

Although, please note that pm_runtime_get|put() doesn't give you full
controll of how the USB PM domain is being powered. So in that case,
it sounds like you need to use the genpd on/off notifiers too.

Kind regards
Uffe

On Thu, 22 Aug 2024 18:27:45 +0300, Claudiu wrote:
> 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.
> 
> [...]

Applied, thanks!

[10/16] phy: renesas: rcar-gen3-usb2: Add support to initialize the bus
        commit: 4eae16375357a2a7e8501be5469532f7636064b3
[11/16] dt-bindings: phy: renesas,usb2-phy: Document RZ/G3S phy bindings
        commit: f3c8498551146dfb014be0d85d3a7df98be16aa2
[12/16] phy: renesas: rcar-gen3-usb2: Add support for the RZ/G3S SoC
        commit: 3c2ea12a625dbf5a864f4920235fa1c739d06e7d

Best regards,
-- 
~Vinod