| Message ID | 20241126092050.1825607-1-claudiu.beznea.uj@bp.renesas.com |
|---|---|
| Headers | show |
| Series | Add initial USB support for the Renesas RZ/G3S SoC | expand |
Hi Claudiu, CC Ulf Thanks for your patch! On Tue, Nov 26, 2024 at 10:21 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > The RZ/G3S system controller (SYSC) has registers to control signals that > are routed to various IPs. These signals must be controlled during > configuration of the respective IPs. One such signal is the USB PWRRDY, > which connects the SYSC and the USB PHY. This signal must to be controlled > before and after the power to the USB PHY is turned off/on. > > Other similar signals include the following (according to the RZ/G3S > hardware manual): > > * PCIe: > - ALLOW_ENTER_L1 signal controlled through the SYS_PCIE_CFG register > - PCIE_RST_RSM_B signal controlled through the SYS_PCIE_RST_RSM_B > register > - MODE_RXTERMINATION signal controlled through SYS_PCIE_PHY register > > * SPI: > - SEL_SPI_OCTA signal controlled through SYS_IPCONT_SEL_SPI_OCTA > register > > * I2C/I3C: > - af_bypass I2C signals controlled through SYS_I2Cx_CFG registers > (x=0..3) > - af_bypass I3C signal controlled through SYS_I3C_CFG register > > * Ethernet: > - FEC_GIGA_ENABLE Ethernet signals controlled through SYS_GETHx_CFG > registers (x=0..1) > > Add #renesas,sysc-signal-cells DT property to allow different SYSC signals > consumers to manage these signals. > > The goal is to enable consumers to specify the required access data for > these signals (through device tree) and let their respective drivers > control these signals via the syscon regmap provided by the system > controller driver. For example, the USB PHY will describe this relation > using the following DT property: > > usb2_phy1: usb-phy@11e30200 { > // ... > renesas,sysc-signal = <&sysc 0xd70 0x1>; > // ... > }; IIUIC, the consumer driver will appear to control the SYSC bits directly, but due to the use of custom validating regmap accessors and reference counting in the SYSC driver, this is safe? The extra safety requires duplicating the register bits in both DT and the SYSC driver. Both usb-phy nodes on RZG3S use the same renesas,sysc-signal, so the reference counting is indeed needed. They are in different power domains, could that be an issue w.r.t. ordering? I am not a big fan of describing register bits in DT, but for the other SYSC users you list above, syscon+regmap seems to be a valid solution. For USB and PCIe control, the situation is different. I more liked the approach with "reset IDs" you had in v1, as it abstracts the DT description from the register bits, and the USB and PCIe reset bits use a different polarity (on RZ/G3S). If future SoC integration changes the polarity, you have to handle that in the consumer (USB or PCIe) driver, too. Unfortunately such "reset IDs" are only suitable for use with the reset or pmdomain frameworks, which didn't survive the earlier discussions. One other option would be to let SYSC expose regulators? While that would work for USB and PCIe control, we would still need syscon+regmap for the other bits. So the more I think about it, the more I like your (clever) solution... > Along with it, add the syscon to the compatible list as it will be > requested by the consumer drivers. The syscon was added to the rest of > system controller variants as these are similar with RZ/G3S and can > benefit from the implementation proposed in this series. > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi, Geert, On 28.11.2024 17:46, Geert Uytterhoeven wrote: > Hi Claudiu, > > CC Ulf > > Thanks for your patch! > > On Tue, Nov 26, 2024 at 10:21 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> The RZ/G3S system controller (SYSC) has registers to control signals that >> are routed to various IPs. These signals must be controlled during >> configuration of the respective IPs. One such signal is the USB PWRRDY, >> which connects the SYSC and the USB PHY. This signal must to be controlled >> before and after the power to the USB PHY is turned off/on. >> >> Other similar signals include the following (according to the RZ/G3S >> hardware manual): >> >> * PCIe: >> - ALLOW_ENTER_L1 signal controlled through the SYS_PCIE_CFG register >> - PCIE_RST_RSM_B signal controlled through the SYS_PCIE_RST_RSM_B >> register >> - MODE_RXTERMINATION signal controlled through SYS_PCIE_PHY register >> >> * SPI: >> - SEL_SPI_OCTA signal controlled through SYS_IPCONT_SEL_SPI_OCTA >> register >> >> * I2C/I3C: >> - af_bypass I2C signals controlled through SYS_I2Cx_CFG registers >> (x=0..3) >> - af_bypass I3C signal controlled through SYS_I3C_CFG register >> >> * Ethernet: >> - FEC_GIGA_ENABLE Ethernet signals controlled through SYS_GETHx_CFG >> registers (x=0..1) >> >> Add #renesas,sysc-signal-cells DT property to allow different SYSC signals >> consumers to manage these signals. >> >> The goal is to enable consumers to specify the required access data for >> these signals (through device tree) and let their respective drivers >> control these signals via the syscon regmap provided by the system >> controller driver. For example, the USB PHY will describe this relation >> using the following DT property: >> >> usb2_phy1: usb-phy@11e30200 { >> // ... >> renesas,sysc-signal = <&sysc 0xd70 0x1>; >> // ... >> }; > > IIUIC, the consumer driver will appear to control the SYSC bits > directly, but due to the use of custom validating regmap accessors > and reference counting in the SYSC driver, this is safe? I'm not sure I fully understand the safety concern. > The extra safety requires duplicating the register bits in both DT > and the SYSC driver. One other option I saw was to have common defines for registers that could have been shared b/w driver and DTSes. But it looked better to me the way it has been presented in this series. > Both usb-phy nodes on RZG3S use the same renesas,sysc-signal, so the > reference counting is indeed needed. They are in different power > domains, could that be an issue w.r.t. ordering? In chapter "32.4.2.1 USB/PHY related pins", section "When either Port1 or Port2 is unused" of the RZ/G3S HW manual it is mentioned "Since USB_VDD18 / USB_VDD33 are common to 2 Port PHY, it is necessary to supply power even when one of the ports is not in use". (From the discussions w/ the internal HW team) The PWRRDY is an (software controlled) indicator to the USB PHY that power supply is ready.
Hi Claudiu, On Fri, Nov 29, 2024 at 9:21 AM Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote: > On 28.11.2024 17:46, Geert Uytterhoeven wrote: > > On Tue, Nov 26, 2024 at 10:21 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > >> > >> The RZ/G3S system controller (SYSC) has registers to control signals that > >> are routed to various IPs. These signals must be controlled during > >> configuration of the respective IPs. One such signal is the USB PWRRDY, > >> which connects the SYSC and the USB PHY. This signal must to be controlled > >> before and after the power to the USB PHY is turned off/on. > >> > >> Other similar signals include the following (according to the RZ/G3S > >> hardware manual): > >> > >> * PCIe: > >> - ALLOW_ENTER_L1 signal controlled through the SYS_PCIE_CFG register > >> - PCIE_RST_RSM_B signal controlled through the SYS_PCIE_RST_RSM_B > >> register > >> - MODE_RXTERMINATION signal controlled through SYS_PCIE_PHY register > >> > >> * SPI: > >> - SEL_SPI_OCTA signal controlled through SYS_IPCONT_SEL_SPI_OCTA > >> register > >> > >> * I2C/I3C: > >> - af_bypass I2C signals controlled through SYS_I2Cx_CFG registers > >> (x=0..3) > >> - af_bypass I3C signal controlled through SYS_I3C_CFG register > >> > >> * Ethernet: > >> - FEC_GIGA_ENABLE Ethernet signals controlled through SYS_GETHx_CFG > >> registers (x=0..1) > >> > >> Add #renesas,sysc-signal-cells DT property to allow different SYSC signals > >> consumers to manage these signals. > >> > >> The goal is to enable consumers to specify the required access data for > >> these signals (through device tree) and let their respective drivers > >> control these signals via the syscon regmap provided by the system > >> controller driver. For example, the USB PHY will describe this relation > >> using the following DT property: > >> > >> usb2_phy1: usb-phy@11e30200 { > >> // ... > >> renesas,sysc-signal = <&sysc 0xd70 0x1>; > >> // ... > >> }; > > > > IIUIC, the consumer driver will appear to control the SYSC bits > > directly, but due to the use of custom validating regmap accessors > > and reference counting in the SYSC driver, this is safe? > > I'm not sure I fully understand the safety concern. Sorry for my bad expression, this was more like a rhetorical question. I meant that it is safe because: 1. Consumers cannot perform arbitrary register accesses, 2. The reference counting guarantees correct operation, despite both usb-phy nodes using the same renesas,sysc-signal. So everything is fine. > > The extra safety requires duplicating the register bits in both DT > > and the SYSC driver. > > One other option I saw was to have common defines for registers that could > have been shared b/w driver and DTSes. But it looked better to me the way > it has been presented in this series. > > > Both usb-phy nodes on RZG3S use the same renesas,sysc-signal, so the > > reference counting is indeed needed. They are in different power > > domains, could that be an issue w.r.t. ordering? > > In chapter "32.4.2.1 USB/PHY related pins", section "When either Port1 or > Port2 is unused" of the RZ/G3S HW manual it is mentioned "Since USB_VDD18 / > USB_VDD33 are common to 2 Port PHY, it is necessary to supply power even > when one of the > ports is not in use". Does that mean you have to power the other PHY on through the CPG_BUS_PERI_COM_MSTOP register, too? (I know you haven't added R9A08G045_PD_USBx to the USB nodes yet, as #power-domain-cells is still 0). > (From the discussions w/ the internal HW team) The PWRRDY is an (software > controlled) indicator to the USB PHY that power supply is ready. > > From that and [1] I get that both PHYs are powered by the same regulators > (USB_VDD18/USB_VDD33) and the USB PWRRDY signal need to be set before/after > the USB PHY power off/on. Because of this I consider the order doesn't matter. > > [1] https://gcdnb.pbrd.co/images/0a1zYBFZXZVb.png Gr{oetje,eeting}s, Geert
Hi, Geert, On 29.11.2024 10:38, Geert Uytterhoeven wrote: > Hi Claudiu, > > On Fri, Nov 29, 2024 at 9:21 AM Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote: >> On 28.11.2024 17:46, Geert Uytterhoeven wrote: >>> On Tue, Nov 26, 2024 at 10:21 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: >>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>>> >>>> The RZ/G3S system controller (SYSC) has registers to control signals that >>>> are routed to various IPs. These signals must be controlled during >>>> configuration of the respective IPs. One such signal is the USB PWRRDY, >>>> which connects the SYSC and the USB PHY. This signal must to be controlled >>>> before and after the power to the USB PHY is turned off/on. >>>> >>>> Other similar signals include the following (according to the RZ/G3S >>>> hardware manual): >>>> >>>> * PCIe: >>>> - ALLOW_ENTER_L1 signal controlled through the SYS_PCIE_CFG register >>>> - PCIE_RST_RSM_B signal controlled through the SYS_PCIE_RST_RSM_B >>>> register >>>> - MODE_RXTERMINATION signal controlled through SYS_PCIE_PHY register >>>> >>>> * SPI: >>>> - SEL_SPI_OCTA signal controlled through SYS_IPCONT_SEL_SPI_OCTA >>>> register >>>> >>>> * I2C/I3C: >>>> - af_bypass I2C signals controlled through SYS_I2Cx_CFG registers >>>> (x=0..3) >>>> - af_bypass I3C signal controlled through SYS_I3C_CFG register >>>> >>>> * Ethernet: >>>> - FEC_GIGA_ENABLE Ethernet signals controlled through SYS_GETHx_CFG >>>> registers (x=0..1) >>>> >>>> Add #renesas,sysc-signal-cells DT property to allow different SYSC signals >>>> consumers to manage these signals. >>>> >>>> The goal is to enable consumers to specify the required access data for >>>> these signals (through device tree) and let their respective drivers >>>> control these signals via the syscon regmap provided by the system >>>> controller driver. For example, the USB PHY will describe this relation >>>> using the following DT property: >>>> >>>> usb2_phy1: usb-phy@11e30200 { >>>> // ... >>>> renesas,sysc-signal = <&sysc 0xd70 0x1>; >>>> // ... >>>> }; >>> >>> IIUIC, the consumer driver will appear to control the SYSC bits >>> directly, but due to the use of custom validating regmap accessors >>> and reference counting in the SYSC driver, this is safe? >> >> I'm not sure I fully understand the safety concern. > > Sorry for my bad expression, this was more like a rhetorical question. > I meant that it is safe because: > 1. Consumers cannot perform arbitrary register accesses, > 2. The reference counting guarantees correct operation, despite > both usb-phy nodes using the same renesas,sysc-signal. > > So everything is fine. > >>> The extra safety requires duplicating the register bits in both DT >>> and the SYSC driver. >> >> One other option I saw was to have common defines for registers that could >> have been shared b/w driver and DTSes. But it looked better to me the way >> it has been presented in this series. >> >>> Both usb-phy nodes on RZG3S use the same renesas,sysc-signal, so the >>> reference counting is indeed needed. They are in different power >>> domains, could that be an issue w.r.t. ordering? >> >> In chapter "32.4.2.1 USB/PHY related pins", section "When either Port1 or >> Port2 is unused" of the RZ/G3S HW manual it is mentioned "Since USB_VDD18 / >> USB_VDD33 are common to 2 Port PHY, it is necessary to supply power even >> when one of the >> ports is not in use". > > Does that mean you have to power the other PHY on through the > CPG_BUS_PERI_COM_MSTOP register, too? I don't know at the moment if there is hard requirement b/w USB PWRRDY and the USB PHY CPG MSTOP bit. I'll have to ask further internally. Thank you, Claudiu > (I know you haven't added R9A08G045_PD_USBx to the USB nodes yet, > as #power-domain-cells is still 0). > >> (From the discussions w/ the internal HW team) The PWRRDY is an (software >> controlled) indicator to the USB PHY that power supply is ready. >> >> From that and [1] I get that both PHYs are powered by the same regulators >> (USB_VDD18/USB_VDD33) and the USB PWRRDY signal need to be set before/after >> the USB PHY power off/on. Because of this I consider the order doesn't matter. >> >> [1] https://gcdnb.pbrd.co/images/0a1zYBFZXZVb.png > > Gr{oetje,eeting}s, > > Geert >
On Tue, Nov 26, 2024 at 11:20:36AM +0200, Claudiu wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > The RZ/G3S system controller (SYSC) has registers to control signals that > are routed to various IPs. These signals must be controlled during > configuration of the respective IPs. One such signal is the USB PWRRDY, > which connects the SYSC and the USB PHY. This signal must to be controlled > before and after the power to the USB PHY is turned off/on. > > Other similar signals include the following (according to the RZ/G3S > hardware manual): > > * PCIe: > - ALLOW_ENTER_L1 signal controlled through the SYS_PCIE_CFG register > - PCIE_RST_RSM_B signal controlled through the SYS_PCIE_RST_RSM_B > register > - MODE_RXTERMINATION signal controlled through SYS_PCIE_PHY register > > * SPI: > - SEL_SPI_OCTA signal controlled through SYS_IPCONT_SEL_SPI_OCTA > register > > * I2C/I3C: > - af_bypass I2C signals controlled through SYS_I2Cx_CFG registers > (x=0..3) > - af_bypass I3C signal controlled through SYS_I3C_CFG register > > * Ethernet: > - FEC_GIGA_ENABLE Ethernet signals controlled through SYS_GETHx_CFG > registers (x=0..1) > > Add #renesas,sysc-signal-cells DT property to allow different SYSC signals > consumers to manage these signals. > > The goal is to enable consumers to specify the required access data for > these signals (through device tree) and let their respective drivers > control these signals via the syscon regmap provided by the system > controller driver. For example, the USB PHY will describe this relation > using the following DT property: > > usb2_phy1: usb-phy@11e30200 { > // ... > renesas,sysc-signal = <&sysc 0xd70 0x1>; > // ... > }; > > Along with it, add the syscon to the compatible list as it will be > requested by the consumer drivers. The syscon was added to the rest of > system controller variants as these are similar with RZ/G3S and can > benefit from the implementation proposed in this series. > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > --- > > Changes in v2: > - none; this patch is new > > > .../soc/renesas/renesas,rzg2l-sysc.yaml | 23 ++++++++++++++----- > 1 file changed, 17 insertions(+), 6 deletions(-) > > diff --git a/Documentation/devicetree/bindings/soc/renesas/renesas,rzg2l-sysc.yaml b/Documentation/devicetree/bindings/soc/renesas/renesas,rzg2l-sysc.yaml > index 4386b2c3fa4d..90f827e8de3e 100644 > --- a/Documentation/devicetree/bindings/soc/renesas/renesas,rzg2l-sysc.yaml > +++ b/Documentation/devicetree/bindings/soc/renesas/renesas,rzg2l-sysc.yaml > @@ -19,11 +19,13 @@ description: > > properties: > compatible: > - enum: > - - renesas,r9a07g043-sysc # RZ/G2UL and RZ/Five > - - renesas,r9a07g044-sysc # RZ/G2{L,LC} > - - renesas,r9a07g054-sysc # RZ/V2L > - - renesas,r9a08g045-sysc # RZ/G3S > + items: > + - enum: > + - renesas,r9a07g043-sysc # RZ/G2UL and RZ/Five > + - renesas,r9a07g044-sysc # RZ/G2{L,LC} > + - renesas,r9a07g054-sysc # RZ/V2L > + - renesas,r9a08g045-sysc # RZ/G3S > + - const: syscon > > reg: > maxItems: 1 > @@ -42,9 +44,17 @@ properties: > - const: cm33stbyr_int > - const: ca55_deny > > + "#renesas,sysc-signal-cells": > + description: > + The number of cells needed to configure a SYSC controlled signal. First > + cell specifies the SYSC offset of the configuration register, second cell > + specifies the bitmask in register. > + const: 2 If there's only one possible value, then just fix the size in the users. We don't need #foo-cells until things are really generic. Plus patch 8 already ignores this based on the schema. And there's implications to defining them. For example, the pattern is that the consumer property name is renesas,sysc-signals, not renesas,sysc-signal. Maybe someone wants to create a 'h/w (signal) control' subsystem (and binding) that is just 'read, assert, or deassert a h/w signal'. Perhaps even the reset subsystem could be morphed into that as I think there would be a lot of overlap. Maybe that would cut down on a lot of these syscon phandle properties. I would find that a lot more acceptable than the generic 'syscons' and '#syscon-cells' binding that was proposed at some point. > + > required: > - compatible > - reg > + - "#renesas,sysc-signal-cells" New required properties are an ABI break. > > additionalProperties: false > > @@ -53,7 +63,7 @@ examples: > #include <dt-bindings/interrupt-controller/arm-gic.h> > > sysc: system-controller@11020000 { > - compatible = "renesas,r9a07g044-sysc"; > + compatible = "renesas,r9a07g044-sysc", "syscon"; What happens on a new kernel and a DT without this change? Rob
Hi, Rob, On 10.12.2024 20:45, Rob Herring wrote: > On Tue, Nov 26, 2024 at 11:20:36AM +0200, Claudiu wrote: >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> The RZ/G3S system controller (SYSC) has registers to control signals that >> are routed to various IPs. These signals must be controlled during >> configuration of the respective IPs. One such signal is the USB PWRRDY, >> which connects the SYSC and the USB PHY. This signal must to be controlled >> before and after the power to the USB PHY is turned off/on. >> >> Other similar signals include the following (according to the RZ/G3S >> hardware manual): >> >> * PCIe: >> - ALLOW_ENTER_L1 signal controlled through the SYS_PCIE_CFG register >> - PCIE_RST_RSM_B signal controlled through the SYS_PCIE_RST_RSM_B >> register >> - MODE_RXTERMINATION signal controlled through SYS_PCIE_PHY register >> >> * SPI: >> - SEL_SPI_OCTA signal controlled through SYS_IPCONT_SEL_SPI_OCTA >> register >> >> * I2C/I3C: >> - af_bypass I2C signals controlled through SYS_I2Cx_CFG registers >> (x=0..3) >> - af_bypass I3C signal controlled through SYS_I3C_CFG register >> >> * Ethernet: >> - FEC_GIGA_ENABLE Ethernet signals controlled through SYS_GETHx_CFG >> registers (x=0..1) >> >> Add #renesas,sysc-signal-cells DT property to allow different SYSC signals >> consumers to manage these signals. >> >> The goal is to enable consumers to specify the required access data for >> these signals (through device tree) and let their respective drivers >> control these signals via the syscon regmap provided by the system >> controller driver. For example, the USB PHY will describe this relation >> using the following DT property: >> >> usb2_phy1: usb-phy@11e30200 { >> // ... >> renesas,sysc-signal = <&sysc 0xd70 0x1>; >> // ... >> }; >> >> Along with it, add the syscon to the compatible list as it will be >> requested by the consumer drivers. The syscon was added to the rest of >> system controller variants as these are similar with RZ/G3S and can >> benefit from the implementation proposed in this series. >> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> --- >> >> Changes in v2: >> - none; this patch is new >> >> >> .../soc/renesas/renesas,rzg2l-sysc.yaml | 23 ++++++++++++++----- >> 1 file changed, 17 insertions(+), 6 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/soc/renesas/renesas,rzg2l-sysc.yaml b/Documentation/devicetree/bindings/soc/renesas/renesas,rzg2l-sysc.yaml >> index 4386b2c3fa4d..90f827e8de3e 100644 >> --- a/Documentation/devicetree/bindings/soc/renesas/renesas,rzg2l-sysc.yaml >> +++ b/Documentation/devicetree/bindings/soc/renesas/renesas,rzg2l-sysc.yaml >> @@ -19,11 +19,13 @@ description: >> >> properties: >> compatible: >> - enum: >> - - renesas,r9a07g043-sysc # RZ/G2UL and RZ/Five >> - - renesas,r9a07g044-sysc # RZ/G2{L,LC} >> - - renesas,r9a07g054-sysc # RZ/V2L >> - - renesas,r9a08g045-sysc # RZ/G3S >> + items: >> + - enum: >> + - renesas,r9a07g043-sysc # RZ/G2UL and RZ/Five >> + - renesas,r9a07g044-sysc # RZ/G2{L,LC} >> + - renesas,r9a07g054-sysc # RZ/V2L >> + - renesas,r9a08g045-sysc # RZ/G3S >> + - const: syscon >> >> reg: >> maxItems: 1 >> @@ -42,9 +44,17 @@ properties: >> - const: cm33stbyr_int >> - const: ca55_deny >> >> + "#renesas,sysc-signal-cells": >> + description: >> + The number of cells needed to configure a SYSC controlled signal. First >> + cell specifies the SYSC offset of the configuration register, second cell >> + specifies the bitmask in register. >> + const: 2 > > If there's only one possible value, then just fix the size in the users. > We don't need #foo-cells until things are really generic. Plus patch > 8 already ignores this based on the schema. And there's implications to > defining them. For example, the pattern is that the consumer property > name is renesas,sysc-signals, not renesas,sysc-signal. OK, I'll fix the size in users. > > Maybe someone wants to create a 'h/w (signal) control' subsystem (and > binding) that is just 'read, assert, or deassert a h/w signal'. Perhaps Until then, is it OK for you to keep it as proposed here? > even the reset subsystem could be morphed into that as I think there > would be a lot of overlap. The USB PWRRDY signal handling has been initially implemented though a reset controller driver but, after discussion with Philipp it has been concluded that it should be handled differently, since it is not a reset signal. > Maybe that would cut down on a lot of these > syscon phandle properties. I would find that a lot more acceptable than > the generic 'syscons' and '#syscon-cells' binding that was proposed at > some point. > > >> + >> required: >> - compatible >> - reg >> + - "#renesas,sysc-signal-cells" > > New required properties are an ABI break. I've added it as in the old DTs the system-controller node is disabled. With that, do you consider it OK to keep it? > >> >> additionalProperties: false >> >> @@ -53,7 +63,7 @@ examples: >> #include <dt-bindings/interrupt-controller/arm-gic.h> >> >> sysc: system-controller@11020000 { >> - compatible = "renesas,r9a07g044-sysc"; >> + compatible = "renesas,r9a07g044-sysc", "syscon"; > > What happens on a new kernel and a DT without this change? The older DT have the system-controller node disabled, thus nothing will be probed for it. Thank you for your review, Claudiu > > Rob
On Wed, Dec 11, 2024 at 6:23 AM Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote: > > Hi, Rob, > > On 10.12.2024 20:45, Rob Herring wrote: > > On Tue, Nov 26, 2024 at 11:20:36AM +0200, Claudiu wrote: > >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > >> > >> The RZ/G3S system controller (SYSC) has registers to control signals that > >> are routed to various IPs. These signals must be controlled during > >> configuration of the respective IPs. One such signal is the USB PWRRDY, > >> which connects the SYSC and the USB PHY. This signal must to be controlled > >> before and after the power to the USB PHY is turned off/on. > >> > >> Other similar signals include the following (according to the RZ/G3S > >> hardware manual): > >> > >> * PCIe: > >> - ALLOW_ENTER_L1 signal controlled through the SYS_PCIE_CFG register > >> - PCIE_RST_RSM_B signal controlled through the SYS_PCIE_RST_RSM_B > >> register > >> - MODE_RXTERMINATION signal controlled through SYS_PCIE_PHY register > >> > >> * SPI: > >> - SEL_SPI_OCTA signal controlled through SYS_IPCONT_SEL_SPI_OCTA > >> register > >> > >> * I2C/I3C: > >> - af_bypass I2C signals controlled through SYS_I2Cx_CFG registers > >> (x=0..3) > >> - af_bypass I3C signal controlled through SYS_I3C_CFG register > >> > >> * Ethernet: > >> - FEC_GIGA_ENABLE Ethernet signals controlled through SYS_GETHx_CFG > >> registers (x=0..1) > >> > >> Add #renesas,sysc-signal-cells DT property to allow different SYSC signals > >> consumers to manage these signals. > >> > >> The goal is to enable consumers to specify the required access data for > >> these signals (through device tree) and let their respective drivers > >> control these signals via the syscon regmap provided by the system > >> controller driver. For example, the USB PHY will describe this relation > >> using the following DT property: > >> > >> usb2_phy1: usb-phy@11e30200 { > >> // ... > >> renesas,sysc-signal = <&sysc 0xd70 0x1>; > >> // ... > >> }; > >> > >> Along with it, add the syscon to the compatible list as it will be > >> requested by the consumer drivers. The syscon was added to the rest of > >> system controller variants as these are similar with RZ/G3S and can > >> benefit from the implementation proposed in this series. > >> > >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > >> --- > >> > >> Changes in v2: > >> - none; this patch is new > >> > >> > >> .../soc/renesas/renesas,rzg2l-sysc.yaml | 23 ++++++++++++++----- > >> 1 file changed, 17 insertions(+), 6 deletions(-) > >> > >> diff --git a/Documentation/devicetree/bindings/soc/renesas/renesas,rzg2l-sysc.yaml b/Documentation/devicetree/bindings/soc/renesas/renesas,rzg2l-sysc.yaml > >> index 4386b2c3fa4d..90f827e8de3e 100644 > >> --- a/Documentation/devicetree/bindings/soc/renesas/renesas,rzg2l-sysc.yaml > >> +++ b/Documentation/devicetree/bindings/soc/renesas/renesas,rzg2l-sysc.yaml > >> @@ -19,11 +19,13 @@ description: > >> > >> properties: > >> compatible: > >> - enum: > >> - - renesas,r9a07g043-sysc # RZ/G2UL and RZ/Five > >> - - renesas,r9a07g044-sysc # RZ/G2{L,LC} > >> - - renesas,r9a07g054-sysc # RZ/V2L > >> - - renesas,r9a08g045-sysc # RZ/G3S > >> + items: > >> + - enum: > >> + - renesas,r9a07g043-sysc # RZ/G2UL and RZ/Five > >> + - renesas,r9a07g044-sysc # RZ/G2{L,LC} > >> + - renesas,r9a07g054-sysc # RZ/V2L > >> + - renesas,r9a08g045-sysc # RZ/G3S > >> + - const: syscon > >> > >> reg: > >> maxItems: 1 > >> @@ -42,9 +44,17 @@ properties: > >> - const: cm33stbyr_int > >> - const: ca55_deny > >> > >> + "#renesas,sysc-signal-cells": > >> + description: > >> + The number of cells needed to configure a SYSC controlled signal. First > >> + cell specifies the SYSC offset of the configuration register, second cell > >> + specifies the bitmask in register. > >> + const: 2 > > > > If there's only one possible value, then just fix the size in the users. > > We don't need #foo-cells until things are really generic. Plus patch > > 8 already ignores this based on the schema. And there's implications to > > defining them. For example, the pattern is that the consumer property > > name is renesas,sysc-signals, not renesas,sysc-signal. > > OK, I'll fix the size in users. You already did for the one in this series. > > > > Maybe someone wants to create a 'h/w (signal) control' subsystem (and > > binding) that is just 'read, assert, or deassert a h/w signal'. Perhaps > > Until then, is it OK for you to keep it as proposed here? Yes. > > even the reset subsystem could be morphed into that as I think there > > would be a lot of overlap. > > The USB PWRRDY signal handling has been initially implemented though a > reset controller driver but, after discussion with Philipp it has been > concluded that it should be handled differently, since it is not a reset > signal. Every reset is a signal, but every signal is not a reset. > > Maybe that would cut down on a lot of these > > syscon phandle properties. I would find that a lot more acceptable than > > the generic 'syscons' and '#syscon-cells' binding that was proposed at > > some point. > > > > > >> + > >> required: > >> - compatible > >> - reg > >> + - "#renesas,sysc-signal-cells" > > > > New required properties are an ABI break. > > I've added it as in the old DTs the system-controller node is disabled. Ok, so it depends if the consumers treat this node as required or not. Or maybe they are all disabled too. > With that, do you consider it OK to keep it? No, as we're dropping the property aren't we? Rob
On 11.12.2024 14:46, Rob Herring wrote: > On Wed, Dec 11, 2024 at 6:23 AM Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote: >> >> Hi, Rob, >> >> On 10.12.2024 20:45, Rob Herring wrote: >>> On Tue, Nov 26, 2024 at 11:20:36AM +0200, Claudiu wrote: >>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>>> >>>> The RZ/G3S system controller (SYSC) has registers to control signals that >>>> are routed to various IPs. These signals must be controlled during >>>> configuration of the respective IPs. One such signal is the USB PWRRDY, >>>> which connects the SYSC and the USB PHY. This signal must to be controlled >>>> before and after the power to the USB PHY is turned off/on. >>>> >>>> Other similar signals include the following (according to the RZ/G3S >>>> hardware manual): >>>> >>>> * PCIe: >>>> - ALLOW_ENTER_L1 signal controlled through the SYS_PCIE_CFG register >>>> - PCIE_RST_RSM_B signal controlled through the SYS_PCIE_RST_RSM_B >>>> register >>>> - MODE_RXTERMINATION signal controlled through SYS_PCIE_PHY register >>>> >>>> * SPI: >>>> - SEL_SPI_OCTA signal controlled through SYS_IPCONT_SEL_SPI_OCTA >>>> register >>>> >>>> * I2C/I3C: >>>> - af_bypass I2C signals controlled through SYS_I2Cx_CFG registers >>>> (x=0..3) >>>> - af_bypass I3C signal controlled through SYS_I3C_CFG register >>>> >>>> * Ethernet: >>>> - FEC_GIGA_ENABLE Ethernet signals controlled through SYS_GETHx_CFG >>>> registers (x=0..1) >>>> >>>> Add #renesas,sysc-signal-cells DT property to allow different SYSC signals >>>> consumers to manage these signals. >>>> >>>> The goal is to enable consumers to specify the required access data for >>>> these signals (through device tree) and let their respective drivers >>>> control these signals via the syscon regmap provided by the system >>>> controller driver. For example, the USB PHY will describe this relation >>>> using the following DT property: >>>> >>>> usb2_phy1: usb-phy@11e30200 { >>>> // ... >>>> renesas,sysc-signal = <&sysc 0xd70 0x1>; >>>> // ... >>>> }; >>>> >>>> Along with it, add the syscon to the compatible list as it will be >>>> requested by the consumer drivers. The syscon was added to the rest of >>>> system controller variants as these are similar with RZ/G3S and can >>>> benefit from the implementation proposed in this series. >>>> >>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>>> --- >>>> >>>> Changes in v2: >>>> - none; this patch is new >>>> >>>> >>>> .../soc/renesas/renesas,rzg2l-sysc.yaml | 23 ++++++++++++++----- >>>> 1 file changed, 17 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/Documentation/devicetree/bindings/soc/renesas/renesas,rzg2l-sysc.yaml b/Documentation/devicetree/bindings/soc/renesas/renesas,rzg2l-sysc.yaml >>>> index 4386b2c3fa4d..90f827e8de3e 100644 >>>> --- a/Documentation/devicetree/bindings/soc/renesas/renesas,rzg2l-sysc.yaml >>>> +++ b/Documentation/devicetree/bindings/soc/renesas/renesas,rzg2l-sysc.yaml >>>> @@ -19,11 +19,13 @@ description: >>>> >>>> properties: >>>> compatible: >>>> - enum: >>>> - - renesas,r9a07g043-sysc # RZ/G2UL and RZ/Five >>>> - - renesas,r9a07g044-sysc # RZ/G2{L,LC} >>>> - - renesas,r9a07g054-sysc # RZ/V2L >>>> - - renesas,r9a08g045-sysc # RZ/G3S >>>> + items: >>>> + - enum: >>>> + - renesas,r9a07g043-sysc # RZ/G2UL and RZ/Five >>>> + - renesas,r9a07g044-sysc # RZ/G2{L,LC} >>>> + - renesas,r9a07g054-sysc # RZ/V2L >>>> + - renesas,r9a08g045-sysc # RZ/G3S >>>> + - const: syscon >>>> >>>> reg: >>>> maxItems: 1 >>>> @@ -42,9 +44,17 @@ properties: >>>> - const: cm33stbyr_int >>>> - const: ca55_deny >>>> >>>> + "#renesas,sysc-signal-cells": >>>> + description: >>>> + The number of cells needed to configure a SYSC controlled signal. First >>>> + cell specifies the SYSC offset of the configuration register, second cell >>>> + specifies the bitmask in register. >>>> + const: 2 >>> >>> If there's only one possible value, then just fix the size in the users. >>> We don't need #foo-cells until things are really generic. Plus patch >>> 8 already ignores this based on the schema. And there's implications to >>> defining them. For example, the pattern is that the consumer property >>> name is renesas,sysc-signals, not renesas,sysc-signal. >> >> OK, I'll fix the size in users. > > You already did for the one in this series. > >>> >>> Maybe someone wants to create a 'h/w (signal) control' subsystem (and >>> binding) that is just 'read, assert, or deassert a h/w signal'. Perhaps >> >> Until then, is it OK for you to keep it as proposed here? > > Yes. > >>> even the reset subsystem could be morphed into that as I think there >>> would be a lot of overlap. >> >> The USB PWRRDY signal handling has been initially implemented though a >> reset controller driver but, after discussion with Philipp it has been >> concluded that it should be handled differently, since it is not a reset >> signal. > > Every reset is a signal, but every signal is not a reset. > >>> Maybe that would cut down on a lot of these >>> syscon phandle properties. I would find that a lot more acceptable than >>> the generic 'syscons' and '#syscon-cells' binding that was proposed at >>> some point. >>> >>> >>>> + >>>> required: >>>> - compatible >>>> - reg >>>> + - "#renesas,sysc-signal-cells" >>> >>> New required properties are an ABI break. >> >> I've added it as in the old DTs the system-controller node is disabled. > > Ok, so it depends if the consumers treat this node as required or not. The only current consumer is the the RZ/G3S USB PHY which is added along with this series. > Or maybe they are all disabled too. > >> With that, do you consider it OK to keep it? > > No, as we're dropping the property aren't we? You're right! Stupid question from me, sorry. Thank you for your review, Claudiu > > Rob
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: - patches 01-05/15 - add SYSC driver support; this is necessary for USB PHY as the USB PHY driver need to touch a register in the SYSC address space, in the initialization phase - patch 06/15 - updates the USBHS documentation for RZ/G3S - patches 07-10/15 - updates the USB PHY support to handle the SYSC USB PWRRDY signal. Along with it a fix for the DT bindings and one for the PHY driver were added; fixes are RZ/G3S USB related - patch 11/15 - document the USB PHY Ctrl support - patches 12-15/15 - add device tree support Merge strategy, if any: - patches 01-05/15,12-14/15 can go through Renesas tree - patch 06/14 can go though USB tree - patches 07-10/14 can go through PHY tree - patch 11/15 can go though reset controller tree Thank you, Claudiu Beznea Changes in v2: - dropped v1 patches already applied - added fixes patches (07/14 and 09/14) - dropped the approach of handling the USB PWRRDY though a reset controller driver and introduced the signal concept for the SYSC driver; because of this, most of the work done in v1 was dropped - per patch changes are listed in individual patches, if any Christophe JAILLET (1): phy: renesas: rcar-gen3-usb2: Fix an error handling path in rcar_gen3_phy_usb2_probe() Claudiu Beznea (14): dt-bindings: soc: renesas: renesas,rzg2l-sysc: Add #renesas,sysc-signal-cells soc: renesas: Add SYSC driver for Renesas RZ family soc: renesas: rz-sysc: Enable SYSC driver for RZ/G3S soc: renesas: rz-sysc: Add SoC detection support soc: renesas: rz-sysc: Move RZ/G3S SoC detection to the SYSC driver dt-bindings: usb: renesas,usbhs: Document RZ/G3S SoC dt-bindings: phy: renesas,usb2-phy: Mark resets as required for RZ/G3S dt-bindings: phy: renesas,usb2-phy: Add renesas,sysc-signal phy: renesas: rcar-gen3-usb2: Add support for PWRRDY dt-bindings: reset: renesas,rzg2l-usbphy-ctrl: Document RZ/G3S support arm64: dts: renesas: Add #renesas,sysc-signal-cells to system controller node arm64: dts: renesas: r9a08g045: Enable the system controller arm64: dts: renesas: r9a08g045: Add USB support arm64: dts: renesas: rzg3s-smarc: Enable USB support .../bindings/phy/renesas,usb2-phy.yaml | 26 +- .../reset/renesas,rzg2l-usbphy-ctrl.yaml | 1 + .../soc/renesas/renesas,rzg2l-sysc.yaml | 23 +- .../bindings/usb/renesas,usbhs.yaml | 2 + arch/arm64/boot/dts/renesas/r9a07g043.dtsi | 3 +- arch/arm64/boot/dts/renesas/r9a07g044.dtsi | 3 +- arch/arm64/boot/dts/renesas/r9a07g054.dtsi | 3 +- arch/arm64/boot/dts/renesas/r9a08g045.dtsi | 123 +++++- arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi | 57 +++ drivers/phy/renesas/phy-rcar-gen3-usb2.c | 77 +++- drivers/soc/renesas/Kconfig | 8 + drivers/soc/renesas/Makefile | 2 + drivers/soc/renesas/r9a08g045-sysc.c | 43 +++ drivers/soc/renesas/renesas-soc.c | 12 - drivers/soc/renesas/rz-sysc.c | 350 ++++++++++++++++++ drivers/soc/renesas/rz-sysc.h | 70 ++++ 16 files changed, 778 insertions(+), 25 deletions(-) create mode 100644 drivers/soc/renesas/r9a08g045-sysc.c create mode 100644 drivers/soc/renesas/rz-sysc.c create mode 100644 drivers/soc/renesas/rz-sysc.h