| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <fa9b3449-ea3e-4482-b7eb-96999445cea5@tuxon.dev>
Date: Fri, 30 Aug 2024 14:32:06 +0300
From: claudiu beznea <claudiu.beznea@...on.dev>
To: Ulf Hansson <ulf.hansson@...aro.org>
Cc: vkoul@...nel.org, kishon@...nel.org, robh@...nel.org, krzk+dt@...nel.org,
conor+dt@...nel.org, p.zabel@...gutronix.de, geert+renesas@...der.be,
magnus.damm@...il.com, gregkh@...uxfoundation.org, mturquette@...libre.com,
sboyd@...nel.org, yoshihiro.shimoda.uh@...esas.com,
biju.das.jz@...renesas.com, linux-phy@...ts.infradead.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-renesas-soc@...r.kernel.org, linux-usb@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-clk@...r.kernel.org,
linux-pm@...r.kernel.org, Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
Subject: Re: [PATCH 00/16] Add initial USB support for the Renesas RZ/G3S SoC
On 30.08.2024 13:14, Ulf Hansson wrote:
> On Fri, 30 Aug 2024 at 10:22, claudiu beznea <claudiu.beznea@...on.dev> wrote:
>>
>> Hi, Ulf,
>>
>> On 29.08.2024 18:26, Ulf Hansson wrote:
>>> On Thu, 22 Aug 2024 at 17:28, Claudiu <claudiu.beznea@...on.dev> wrote:
>>>>
>>>> From: Claudiu Beznea <claudiu.beznea.uj@...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@...00000 {
+ 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@...00000 {
+ 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
Powered by blists - more mailing lists