Re: [PATCH v2 01/15] dt-bindings: soc: renesas: renesas,rzg2l-sysc: Add #renesas,sysc-signal-cells

[Rob Herring]

On Wed, Dec 11, 2024 at 6:23 AM Claudiu Beznea <claudiu.beznea@xxxxxxxxx> 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@xxxxxxxxxxxxxx>
> >>
> >> 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@xxxxxxxxxxxxxx>
> >> ---
> >>
> >> 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