Message-ID: <20241210184542.GA4077820-robh@kernel.org>
Date: Tue, 10 Dec 2024 12:45:42 -0600
From: Rob Herring <robh@...nel.org>
To: Claudiu <claudiu.beznea@...on.dev>
Cc: vkoul@...nel.org, kishon@...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, yoshihiro.shimoda.uh@...esas.com,
	christophe.jaillet@...adoo.fr, 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,
	Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
Subject: Re: [PATCH v2 01/15] dt-bindings: soc: renesas: renesas,rzg2l-sysc:
 Add #renesas,sysc-signal-cells

On Tue, Nov 26, 2024 at 11:20:36AM +0200, Claudiu wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@...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@...30200 {
> 	// ...
> 	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@...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@...20000 {
> -            compatible = "renesas,r9a07g044-sysc";
> +            compatible = "renesas,r9a07g044-sysc", "syscon";

What happens on a new kernel and a DT without this change?

Rob

Powered by blists - more mailing lists