This adds DT bindings for the Microsemi/Microchip SGPIO controller,
bindings microchip,sparx5-sgpio, mscc,ocelot-sgpio and
mscc,luton-sgpio.
Signed-off-by: Lars Povlsen <[email protected]>
---
.../pinctrl/microchip,sparx5-sgpio.yaml | 140 ++++++++++++++++++
1 file changed, 140 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml
diff --git a/Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml b/Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml
new file mode 100644
index 000000000000..fc41495800ed
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml
@@ -0,0 +1,140 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pinctrl/microchip,sparx5-sgpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microsemi/Microchip Serial GPIO controller
+
+maintainers:
+ - Lars Povlsen <[email protected]>
+
+description: |
+ By using a serial interface, the SIO controller significantly extend
+ the number of available GPIOs with a minimum number of additional
+ pins on the device. The primary purpose of the SIO controllers is to
+ connect control signals from SFP modules and to act as an LED
+ controller.
+
+properties:
+ $nodename:
+ pattern: "^gpio@[0-9a-f]+$"
+
+ compatible:
+ enum:
+ - microchip,sparx5-sgpio
+ - mscc,ocelot-sgpio
+ - mscc,luton-sgpio
+
+ "#address-cells":
+ const: 1
+
+ "#size-cells":
+ const: 0
+
+ reg:
+ maxItems: 1
+
+ clocks:
+ maxItems: 1
+
+ microchip,sgpio-port-ranges:
+ description: This is a sequence of tuples, defining intervals of
+ enabled ports in the serial input stream. The enabled ports must
+ match the hardware configuration in order for signals to be
+ properly written/read to/from the controller holding
+ registers. Being tuples, then number of arguments must be
+ even. The tuples mast be ordered (low, high) and are
+ inclusive.
+ $ref: /schemas/types.yaml#/definitions/uint32-matrix
+ items:
+ items:
+ - description: |
+ "low" indicates start bit number of range
+ minimum: 0
+ maximum: 31
+ - description: |
+ "high" indicates end bit number of range
+ minimum: 0
+ maximum: 31
+ minItems: 1
+ maxItems: 32
+
+ microchip,sgpio-frequency:
+ description: The sgpio controller frequency (Hz). This dictates
+ the serial bitstream speed, which again affects the latency in
+ getting control signals back and forth between external shift
+ registers. The speed must be no larger than half the system
+ clock, and larger than zero.
+ $ref: /schemas/types.yaml#/definitions/uint32
+ minimum: 1
+ default: 12500000
+
+patternProperties:
+ "^gpio@[0-1]$":
+ type: object
+ properties:
+ compatible:
+ const: microchip,sparx5-sgpio-bank
+
+ reg:
+ description: |
+ The GPIO bank number. "0" is designates the input pin bank,
+ "1" the output bank.
+ maxItems: 1
+
+ gpio-controller: true
+
+ '#gpio-cells':
+ const: 3
+
+ ngpios:
+ minimum: 1
+ maximum: 128
+
+ required:
+ - compatible
+ - reg
+ - gpio-controller
+ - '#gpio-cells'
+ - ngpios
+
+ additionalProperties: false
+
+additionalProperties: false
+
+required:
+ - compatible
+ - reg
+ - clocks
+ - microchip,sgpio-port-ranges
+ - "#address-cells"
+ - "#size-cells"
+
+examples:
+ - |
+ sgpio2: gpio@1101059c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "microchip,sparx5-sgpio";
+ clocks = <&sys_clk>;
+ pinctrl-0 = <&sgpio2_pins>;
+ pinctrl-names = "default";
+ reg = <0x1101059c 0x100>;
+ microchip,sgpio-port-ranges = <0 0>, <16 18>, <28 31>;
+ microchip,sgpio-frequency = <25000000>;
+ sgpio_in2: gpio@0 {
+ reg = <0>;
+ compatible = "microchip,sparx5-sgpio-bank";
+ gpio-controller;
+ #gpio-cells = <3>;
+ ngpios = <96>;
+ };
+ sgpio_out2: gpio@1 {
+ compatible = "microchip,sparx5-sgpio-bank";
+ reg = <1>;
+ gpio-controller;
+ #gpio-cells = <3>;
+ ngpios = <96>;
+ };
+ };
--
2.25.1
Hi Lars!
This is overall looking fine. Except for the 3 cell business. I just can't
wrap my head around why that is needed.
On Thu, Oct 8, 2020 at 3:05 PM Lars Povlsen <[email protected]> wrote:
> + '#gpio-cells':
> + const: 3
So at the very least needs a description making it crystal clear why each
cell is needed, and used for since the standard bindings are not used.
+ sgpio_in2: gpio@0 {
+ reg = <0>;
+ compatible = "microchip,sparx5-sgpio-bank";
+ gpio-controller;
+ #gpio-cells = <3>;
+ ngpios = <96>;
+ };
So here reg = 0 and the out port has reg 1. Isn't that what you also put
in the second cell of the GPIO phandle? Then why? The driver
can very well just parse its own reg property and fill that in.
When you obtain a phandle like that:
gpios = <&sgpio_in2 11 0 GPIO_OUT_LOW>;
Isn't that 0 just duplicating the "reg"? Just parse reg when you set up
your driver state and put it as variable in the state container for your
driver state for this particular gpio_chip. No need to get it from
the phandle.
Yours,
Linus Walleij
Linus Walleij writes:
> Hi Lars!
>
> This is overall looking fine. Except for the 3 cell business. I just can't
> wrap my head around why that is needed.
>
> On Thu, Oct 8, 2020 at 3:05 PM Lars Povlsen <[email protected]> wrote:
>
>> + '#gpio-cells':
>> + const: 3
>
> So at the very least needs a description making it crystal clear why each
> cell is needed, and used for since the standard bindings are not used.
>
> + sgpio_in2: gpio@0 {
> + reg = <0>;
> + compatible = "microchip,sparx5-sgpio-bank";
> + gpio-controller;
> + #gpio-cells = <3>;
> + ngpios = <96>;
> + };
>
> So here reg = 0 and the out port has reg 1. Isn't that what you also put
> in the second cell of the GPIO phandle? Then why? The driver
> can very well just parse its own reg property and fill that in.
Linus,
NO! The second cell is the second dimension - NOT the direction. As I
wrote previously, the direction is now inherent from the handle, ie. the
"reg" value of the handle.
The hardware describe a "port" and a "bit index" addressing, where the
second cell in
gpios = <&sgpio_in2 11 0 GPIO_OUT_LOW>;
is the "bit index" - not the "reg" from the phandle.
In the example above, note
ngpios = <96>;
As the "port" is [0; 31], this defines "bit index" to be [0; 2], so the
(input) GPIO cells will be:
p0b0, p0b1, p0b2
...
p31b0, p31b1, p31b2
being identical to
<&sgpio_inX 0 0 GPIO_OUT_LOW>
<&sgpio_inX 0 1 GPIO_OUT_LOW>
<&sgpio_inX 0 2 GPIO_OUT_LOW>
...
<&sgpio_inX 31 0 GPIO_OUT_LOW>
<&sgpio_inX 31 1 GPIO_OUT_LOW>
<&sgpio_inX 31 2 GPIO_OUT_LOW>
('X' being the SGPIO controller instance).
So no, there *really* is a need for a 3-cell GPIO specifier (or whatever
its called).
Hope this is clearer now...
---Lars
>
> When you obtain a phandle like that:
>
> gpios = <&sgpio_in2 11 0 GPIO_OUT_LOW>;
>
> Isn't that 0 just duplicating the "reg"? Just parse reg when you set up
> your driver state and put it as variable in the state container for your
> driver state for this particular gpio_chip. No need to get it from
> the phandle.
>
> Yours,
> Linus Walleij
--
Lars Povlsen,
Microchip
On Thu, Oct 08, 2020 at 03:05:13PM +0200, Lars Povlsen wrote:
> This adds DT bindings for the Microsemi/Microchip SGPIO controller,
> bindings microchip,sparx5-sgpio, mscc,ocelot-sgpio and
> mscc,luton-sgpio.
>
> Signed-off-by: Lars Povlsen <[email protected]>
> ---
> .../pinctrl/microchip,sparx5-sgpio.yaml | 140 ++++++++++++++++++
> 1 file changed, 140 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml
>
> diff --git a/Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml b/Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml
> new file mode 100644
> index 000000000000..fc41495800ed
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml
> @@ -0,0 +1,140 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pinctrl/microchip,sparx5-sgpio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microsemi/Microchip Serial GPIO controller
> +
> +maintainers:
> + - Lars Povlsen <[email protected]>
> +
> +description: |
> + By using a serial interface, the SIO controller significantly extend
> + the number of available GPIOs with a minimum number of additional
> + pins on the device. The primary purpose of the SIO controllers is to
> + connect control signals from SFP modules and to act as an LED
> + controller.
> +
> +properties:
> + $nodename:
> + pattern: "^gpio@[0-9a-f]+$"
> +
> + compatible:
> + enum:
> + - microchip,sparx5-sgpio
> + - mscc,ocelot-sgpio
> + - mscc,luton-sgpio
> +
> + "#address-cells":
> + const: 1
> +
> + "#size-cells":
> + const: 0
> +
> + reg:
> + maxItems: 1
> +
> + clocks:
> + maxItems: 1
> +
> + microchip,sgpio-port-ranges:
> + description: This is a sequence of tuples, defining intervals of
> + enabled ports in the serial input stream. The enabled ports must
> + match the hardware configuration in order for signals to be
> + properly written/read to/from the controller holding
> + registers. Being tuples, then number of arguments must be
> + even. The tuples mast be ordered (low, high) and are
> + inclusive.
> + $ref: /schemas/types.yaml#/definitions/uint32-matrix
> + items:
> + items:
> + - description: |
> + "low" indicates start bit number of range
> + minimum: 0
> + maximum: 31
> + - description: |
> + "high" indicates end bit number of range
> + minimum: 0
> + maximum: 31
> + minItems: 1
> + maxItems: 32
> +
> + microchip,sgpio-frequency:
> + description: The sgpio controller frequency (Hz). This dictates
> + the serial bitstream speed, which again affects the latency in
> + getting control signals back and forth between external shift
> + registers. The speed must be no larger than half the system
> + clock, and larger than zero.
> + $ref: /schemas/types.yaml#/definitions/uint32
Perhaps use 'bus-frequency' here. If not, needs unit suffix. Either way,
you can drop the type in those cases.
With that,
Reviewed-by: Rob Herring <[email protected]>
> + minimum: 1
> + default: 12500000
> +
> +patternProperties:
> + "^gpio@[0-1]$":
> + type: object
> + properties:
> + compatible:
> + const: microchip,sparx5-sgpio-bank
> +
> + reg:
> + description: |
> + The GPIO bank number. "0" is designates the input pin bank,
> + "1" the output bank.
> + maxItems: 1
> +
> + gpio-controller: true
> +
> + '#gpio-cells':
> + const: 3
> +
> + ngpios:
> + minimum: 1
> + maximum: 128
> +
> + required:
> + - compatible
> + - reg
> + - gpio-controller
> + - '#gpio-cells'
> + - ngpios
> +
> + additionalProperties: false
> +
> +additionalProperties: false
> +
> +required:
> + - compatible
> + - reg
> + - clocks
> + - microchip,sgpio-port-ranges
> + - "#address-cells"
> + - "#size-cells"
> +
> +examples:
> + - |
> + sgpio2: gpio@1101059c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + compatible = "microchip,sparx5-sgpio";
> + clocks = <&sys_clk>;
> + pinctrl-0 = <&sgpio2_pins>;
> + pinctrl-names = "default";
> + reg = <0x1101059c 0x100>;
> + microchip,sgpio-port-ranges = <0 0>, <16 18>, <28 31>;
> + microchip,sgpio-frequency = <25000000>;
> + sgpio_in2: gpio@0 {
> + reg = <0>;
> + compatible = "microchip,sparx5-sgpio-bank";
> + gpio-controller;
> + #gpio-cells = <3>;
> + ngpios = <96>;
> + };
> + sgpio_out2: gpio@1 {
> + compatible = "microchip,sparx5-sgpio-bank";
> + reg = <1>;
> + gpio-controller;
> + #gpio-cells = <3>;
> + ngpios = <96>;
> + };
> + };
> --
> 2.25.1
On Fri, Oct 9, 2020 at 12:00 PM Lars Povlsen <[email protected]> wrote:
> > So here reg = 0 and the out port has reg 1. Isn't that what you also put
> > in the second cell of the GPIO phandle? Then why? The driver
> > can very well just parse its own reg property and fill that in.
>
> NO! The second cell is the second dimension - NOT the direction. As I
> wrote previously, the direction is now inherent from the handle, ie. the
> "reg" value of the handle.
OK I get it ... I think :)
> The hardware describe a "port" and a "bit index" addressing, where the
> second cell in
>
> gpios = <&sgpio_in2 11 0 GPIO_OUT_LOW>;
>
> is the "bit index" - not the "reg" from the phandle.
As long as the bindings specify exactly what is meant by bit index
and the tupe (port, bit_index) is what uniquely addresses a certain
GPIO line then it is fine I suppose.
> In the example above, note
>
> ngpios = <96>;
>
> As the "port" is [0; 31], this defines "bit index" to be [0; 2], so the
> (input) GPIO cells will be:
>
> p0b0, p0b1, p0b2
> ...
> p31b0, p31b1, p31b2
>
> being identical to
>
> <&sgpio_inX 0 0 GPIO_OUT_LOW>
> <&sgpio_inX 0 1 GPIO_OUT_LOW>
> <&sgpio_inX 0 2 GPIO_OUT_LOW>
> ...
> <&sgpio_inX 31 0 GPIO_OUT_LOW>
> <&sgpio_inX 31 1 GPIO_OUT_LOW>
> <&sgpio_inX 31 2 GPIO_OUT_LOW>
>
> ('X' being the SGPIO controller instance).
So 32 possible ports with 3 possible bit indexes on each?
This constraint should go into the bindings as well so it becomes
impossible to put in illegal port numbers or bit indices.
(Use the YAML min/max constraints, I suppose?)
> So no, there *really* is a need for a 3-cell GPIO specifier (or whatever
> its called).
If that is the natural way to address the hardware lines
and what is used in the documentation then it's fine, it's just so
unorthodox that I have to push back on it a bit you know.
Yours,
Linus Walleij
Linus Walleij writes:
> On Fri, Oct 9, 2020 at 12:00 PM Lars Povlsen <[email protected]> wrote:
>
>> > So here reg = 0 and the out port has reg 1. Isn't that what you also put
>> > in the second cell of the GPIO phandle? Then why? The driver
>> > can very well just parse its own reg property and fill that in.
>>
>> NO! The second cell is the second dimension - NOT the direction. As I
>> wrote previously, the direction is now inherent from the handle, ie. the
>> "reg" value of the handle.
>
> OK I get it ... I think :)
Great!
>
>> The hardware describe a "port" and a "bit index" addressing, where the
>> second cell in
>>
>> gpios = <&sgpio_in2 11 0 GPIO_OUT_LOW>;
>>
>> is the "bit index" - not the "reg" from the phandle.
>
> As long as the bindings specify exactly what is meant by bit index
> and the tupe (port, bit_index) is what uniquely addresses a certain
> GPIO line then it is fine I suppose.
>
Yes, that is confirmed.
>> In the example above, note
>>
>> ngpios = <96>;
>>
>> As the "port" is [0; 31], this defines "bit index" to be [0; 2], so the
>> (input) GPIO cells will be:
>>
>> p0b0, p0b1, p0b2
>> ...
>> p31b0, p31b1, p31b2
>>
>> being identical to
>>
>> <&sgpio_inX 0 0 GPIO_OUT_LOW>
>> <&sgpio_inX 0 1 GPIO_OUT_LOW>
>> <&sgpio_inX 0 2 GPIO_OUT_LOW>
>> ...
>> <&sgpio_inX 31 0 GPIO_OUT_LOW>
>> <&sgpio_inX 31 1 GPIO_OUT_LOW>
>> <&sgpio_inX 31 2 GPIO_OUT_LOW>
>>
>> ('X' being the SGPIO controller instance).
>
> So 32 possible ports with 3 possible bit indexes on each?
> This constraint should go into the bindings as well so it becomes
> impossible to put in illegal port numbers or bit indices.
>
> (Use the YAML min/max constraints, I suppose?)
>
Yes, I will to see if constraints in the GPIO args is possible.
>> So no, there *really* is a need for a 3-cell GPIO specifier (or whatever
>> its called).
>
> If that is the natural way to address the hardware lines
> and what is used in the documentation then it's fine, it's just so
> unorthodox that I have to push back on it a bit you know.
>
Yes, this piece of hw is certainly not a stock GPIO controller, so that
was kinda expected. But I think we ended up with an abstraction that
fits as good as possible.
I will send a new (last?) revision that includes the suggestions from
Rob tomorrow.
Thank you for your time and comments (also Rob!)
---Lars
--
Lars Povlsen,
Microchip