diff mbox series

[RFC] dt-bindings: Add AST2600 i3c controller binding

Message ID 5c047dd91390b9ee4cd8bca3ff107db37a7be4ac.1676273912.git.jk@codeconstruct.com.au (mailing list archive)
State Superseded
Headers show
Series [RFC] dt-bindings: Add AST2600 i3c controller binding | expand

Commit Message

Jeremy Kerr Feb. 13, 2023, 7:41 a.m. UTC 
This change adds a devicetree binding for the ast2600 i3c controller
hardware. This is heavily based on the designware i3c hardware, plus a
reset facility and two platform-specific properties:

 - sda-pullup-ohms: to specify the value of the configurable pullup
   resistors on the SDA line

 - global-regs: to reference the (ast2600-specific) i3c global register
   block, and the device index to use within it.

Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
---
RFC: the example in this depends on some not-yet-accepted patches for
the clock and reset linkages:

  https://lore.kernel.org/linux-devicetree/cover.1676267865.git.jk@codeconstruct.com.au/T/

I'm also keen to get some review on the pullup configuration too.

---
 .../bindings/i3c/aspeed,ast2600-i3c.yaml      | 75 +++++++++++++++++++
 1 file changed, 75 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i3c/aspeed,ast2600-i3c.yaml

Comments

Krzysztof Kozlowski Feb. 13, 2023, 8:42 a.m. UTC | #1 
On 13/02/2023 08:41, Jeremy Kerr wrote:
> This change adds a devicetree binding for the ast2600 i3c controller

1. Do not use "This commit/patch".
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

2. Use subject prefixes matching the subsystem (which you can get for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching).

3. Subject: drop second/last, redundant "binding". The "dt-bindings"
prefix is already stating that these are bindings.

4. Where is the driver? Where is the DTS? Why do we want unused binding
in the kernel?

> hardware. This is heavily based on the designware i3c hardware, plus a
> reset facility and two platform-specific properties:
> 
>  - sda-pullup-ohms: to specify the value of the configurable pullup
>    resistors on the SDA line
> 
>  - global-regs: to reference the (ast2600-specific) i3c global register
>    block, and the device index to use within it.
> 
> Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
> ---
> RFC: the example in this depends on some not-yet-accepted patches for
> the clock and reset linkages:
> 
>   https://lore.kernel.org/linux-devicetree/cover.1676267865.git.jk@codeconstruct.com.au/T/
> 
> I'm also keen to get some review on the pullup configuration too.
> 
> ---
>  .../bindings/i3c/aspeed,ast2600-i3c.yaml      | 75 +++++++++++++++++++
>  1 file changed, 75 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i3c/aspeed,ast2600-i3c.yaml
> 
> diff --git a/Documentation/devicetree/bindings/i3c/aspeed,ast2600-i3c.yaml b/Documentation/devicetree/bindings/i3c/aspeed,ast2600-i3c.yaml
> new file mode 100644
> index 000000000000..ef28a8b77c94
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i3c/aspeed,ast2600-i3c.yaml
> @@ -0,0 +1,75 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/i3c/aspeed,ast2600-i3c.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ASPEED AST2600 i3c controller
> +
> +maintainers:
> +  - Jeremy Kerr <jk@codeconstruct.com.au>
> +
> +allOf:
> +  - $ref: i3c.yaml#
> +
> +properties:
> +  compatible:
> +    const: aspeed,ast2600-i3c
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  resets:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  sda-pullup-ohms:
> +    enum: [545, 750, 2000]
> +    default: 2000
> +    description: |
> +      Value of SDA pullup resistor in Ohms

Why this is property of i3c, not pinctrl?

> +
> +  global-regs:

Missing vendor prefix

> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    description: |
> +      A (phandle, controller index) reference to the i3c global register set
> +      used for this device.

Missing items description:
https://elixir.bootlin.com/linux/v5.18-rc1/source/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml#L42


> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - interrupts
> +  - global-regs
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/ast2600-clock.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    i3c-master@2000 {
> +        #address-cells = <3>;
> +        #size-cells = <0>;
> +        compatible = "aspeed,ast2600-i3c";
> +        reg = <0x2000 0x1000>;

compatible and reg are first properties.

> +        clocks = <&syscon ASPEED_CLK_GATE_I3C0CLK>;
> +        resets = <&syscon ASPEED_RESET_I3C0>;
> +        global-regs = <&i3c_global 0>;
> +        pinctrl-names = "default";
> +        pinctrl-0 = <&pinctrl_i3c1_default>;
> +        interrupts = <GIC_SPI 102 IRQ_TYPE_LEVEL_HIGH>;
> +    };
> +
> +    i3c_global: i3c-global@0 {

Drop node, unrelated.

> +        compatible = "aspeed,ast2600-i3c-global", "simple-mfd", "syscon";
> +        resets = <&syscon ASPEED_RESET_I3C_DMA>;
> +        reg = <0x0 0x1000>;
> +    };
> +...

Best regards,
Krzysztof
Jeremy Kerr Feb. 13, 2023, 8:55 a.m. UTC | #2 
Hi Krzysztof,

> 1. Do not use "This commit/patch".
> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

OK.

> 2. Use subject prefixes matching the subsystem (which you can get for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the
> directory your patch is touching).

So "dt-bindings: i3c:" instead of just "d-bindings:" here.

> 3. Subject: drop second/last, redundant "binding". The "dt-bindings"
> prefix is already stating that these are bindings.

OK.

> 4. Where is the driver? Where is the DTS? Why do we want unused
> binding in the kernel?

The driver is coming next, but I wanted to sort out the structure of the
binding before committing to how the driver consumes the DT data - hence
the RFC.

Cheers,


Jeremy
Krzysztof Kozlowski Feb. 13, 2023, 9:05 a.m. UTC | #3 
On 13/02/2023 09:55, Jeremy Kerr wrote:
>> 4. Where is the driver? Where is the DTS? Why do we want unused
>> binding in the kernel?
> 
> The driver is coming next, but I wanted to sort out the structure of the
> binding before committing to how the driver consumes the DT data - hence
> the RFC.

You should clearly communicate that driver is coming... Anyway binding
comes with the driver, otherwise how can we check that you actually
implemented it? Please send patches, not RFC. RFC means you are
uncertain this is even correct and you ask for generic discussion. So
generic discussion comment - implement how other recent i3c bindings are
implemented. This is basic device, there is nothing special here.

Since you did not respond to rest of comments, I assume you are going to
implement them fully - including dropping the questioned properties.

Best regards,
Krzysztof
Jeremy Kerr Feb. 13, 2023, 9:21 a.m. UTC | #4 
Hi Krzysztof,

> You should clearly communicate that driver is coming...

OK.

> Anyway binding comes with the driver, otherwise how can we check that
> you actually implemented it?

I'll include this with the driver once we're past the RFC reviews.

> Please send patches, not RFC. RFC means you are uncertain this is even
> correct and you ask for generic discussion.

Yes, that's essentially what I'm looking for with this change -
particularly with the pullup config, which (as you say) could arguably
be a pinctrl config instead.

If we do decide to do this with pinctrl config, we'll need a separate
driver (and minimal binding) for the global register set to act as the pin
controller. That fundamentally changes the structure here - hence RFC.

> generic discussion comment - implement how other recent i3c bindings
> are implemented. This is basic device, there is nothing special here.

I'd say the global register set makes the binding layout a bit quirky,
as you've identified already.

> Since you did not respond to rest of comments, I assume you are going
> to implement them fully - including dropping the questioned
> properties.

Yep! Some of those will then be unneeded in this binding after going to a
pinctrl approach, and I'll make the fixes as you suggested.

Cheers,


Jeremy
Krzysztof Kozlowski Feb. 13, 2023, 9:35 a.m. UTC | #5 
On 13/02/2023 10:21, Jeremy Kerr wrote:
> Hi Krzysztof,
> 
>> You should clearly communicate that driver is coming...
> 
> OK.
> 
>> Anyway binding comes with the driver, otherwise how can we check that
>> you actually implemented it?
> 
> I'll include this with the driver once we're past the RFC reviews.
> 
>> Please send patches, not RFC. RFC means you are uncertain this is even
>> correct and you ask for generic discussion.
> 
> Yes, that's essentially what I'm looking for with this change -
> particularly with the pullup config, which (as you say) could arguably
> be a pinctrl config instead.

Depends, there was just a short sentence. If this is external resistor
on the board, why this device needs such property (and none of other
devices need...)? If this is internal pull up of I3C (and there is no
other pin configuration possible, no other pins), it looks reasonable to
me to have it here. But I am all guessing it...


Best regards,
Krzysztof
Jeremy Kerr Feb. 13, 2023, 2:04 p.m. UTC | #6 
Hi Krzysztof,

> > Yes, that's essentially what I'm looking for with this change -
> > particularly with the pullup config, which (as you say) could
> > arguably
> > be a pinctrl config instead.
> 
> Depends, there was just a short sentence. If this is external
> resistor
> on the board, why this device needs such property (and none of other
> devices need...)? If this is internal pull up of I3C (and there is no
> other pin configuration possible, no other pins), it looks reasonable
> to me to have it here. But I am all guessing it...

It's the second case: there is a configurable pullup resistor in each of
the i3c controllers (or, more accurately: in the ast2600's glue
between the SoC and the I3C IP block).

The pullup configuration is controlled by the SoC "global" i3c
registers; a block shared by all of the SoC's i3c controllers. So, any
driver implementation would need to set up that global register
configuration on i3c controller init.

So, I can see two options for the binding (and consequently the driver
implementation):

 1) the sda-pullup-ohms property on the controller binding, which a
 driver implementation could set directly through the global register
 set

 2) define a pin controller on the global register block, allowing other
 (standard) DT pinctrl definitions to control the pullup calue. This
 would need a new driver implementation for the pin controller, but that
 shouldn't be too complex to implement.

For the binding proposed here, I've chosen (1). We can handle all of the
other (non-pullup-related) global register configuration by treating the
globals as a simple generic syscon device.

I'm happy to try (2) instead, if that's the better approach. However,
that may be over-engineering the binding spec (and consequently, the
necessary driver implementation) for just setting a register value.

From your second point:

> (and there is no other pin configuration possible, no other pins)

This is a fairly small and isolated component of the global ast2600 pin
configuration; the pullup value is set separately from the
already-implemented SoC-wide pinctrl. Merging the pullup values into
that wouldn't really fit the hardware interface mode though; this is a
separate IP block linked to the i3c controllers.

Let me know if you have any preferences on the approach to a biding
structure.

And Andrew: let me know if your experience with the ast2600 SoC's
pinctrl would suggest either option.

Cheers,


Jeremy
Rob Herring (Arm) Feb. 13, 2023, 3:09 p.m. UTC | #7 
On Mon, 13 Feb 2023 15:41:52 +0800, Jeremy Kerr wrote:
> This change adds a devicetree binding for the ast2600 i3c controller
> hardware. This is heavily based on the designware i3c hardware, plus a
> reset facility and two platform-specific properties:
> 
>  - sda-pullup-ohms: to specify the value of the configurable pullup
>    resistors on the SDA line
> 
>  - global-regs: to reference the (ast2600-specific) i3c global register
>    block, and the device index to use within it.
> 
> Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
> ---
> RFC: the example in this depends on some not-yet-accepted patches for
> the clock and reset linkages:
> 
>   https://lore.kernel.org/linux-devicetree/cover.1676267865.git.jk@codeconstruct.com.au/T/
> 
> I'm also keen to get some review on the pullup configuration too.
> 
> ---
>  .../bindings/i3c/aspeed,ast2600-i3c.yaml      | 75 +++++++++++++++++++
>  1 file changed, 75 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i3c/aspeed,ast2600-i3c.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/i3c/aspeed,ast2600-i3c.example.dts:30.31-32 syntax error
FATAL ERROR: Unable to parse input tree
make[1]: *** [scripts/Makefile.lib:434: Documentation/devicetree/bindings/i3c/aspeed,ast2600-i3c.example.dtb] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1508: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/5c047dd91390b9ee4cd8bca3ff107db37a7be4ac.1676273912.git.jk@codeconstruct.com.au

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/i3c/aspeed,ast2600-i3c.yaml b/Documentation/devicetree/bindings/i3c/aspeed,ast2600-i3c.yaml
new file mode 100644
index 000000000000..ef28a8b77c94
--- /dev/null
+++ b/Documentation/devicetree/bindings/i3c/aspeed,ast2600-i3c.yaml
@@ -0,0 +1,75 @@ 
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/i3c/aspeed,ast2600-i3c.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ASPEED AST2600 i3c controller
+
+maintainers:
+  - Jeremy Kerr <jk@codeconstruct.com.au>
+
+allOf:
+  - $ref: i3c.yaml#
+
+properties:
+  compatible:
+    const: aspeed,ast2600-i3c
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  sda-pullup-ohms:
+    enum: [545, 750, 2000]
+    default: 2000
+    description: |
+      Value of SDA pullup resistor in Ohms
+
+  global-regs:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description: |
+      A (phandle, controller index) reference to the i3c global register set
+      used for this device.
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - interrupts
+  - global-regs
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/ast2600-clock.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    i3c-master@2000 {
+        #address-cells = <3>;
+        #size-cells = <0>;
+        compatible = "aspeed,ast2600-i3c";
+        reg = <0x2000 0x1000>;
+        clocks = <&syscon ASPEED_CLK_GATE_I3C0CLK>;
+        resets = <&syscon ASPEED_RESET_I3C0>;
+        global-regs = <&i3c_global 0>;
+        pinctrl-names = "default";
+        pinctrl-0 = <&pinctrl_i3c1_default>;
+        interrupts = <GIC_SPI 102 IRQ_TYPE_LEVEL_HIGH>;
+    };
+
+    i3c_global: i3c-global@0 {
+        compatible = "aspeed,ast2600-i3c-global", "simple-mfd", "syscon";
+        resets = <&syscon ASPEED_RESET_I3C_DMA>;
+        reg = <0x0 0x1000>;
+    };
+...