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