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

[Claudiu Beznea <claudiu.beznea@tuxon.dev>]

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