[1/3] dt-bindings: ipmi: aspeed: Introduce a v2 binding for KCS
diff mbox series

Message ID 3da2492c244962c27b21aad87bfa6bf74f568f1d.1575376664.git-series.andrew@aj.id.au
State Changes Requested
Headers show
Series
  • ipmi: kcs-bmc: Rework bindings to clean up DT warnings
Related show

Checks

Context Check Description
robh/checkpatch success

Commit Message

Andrew Jeffery Dec. 3, 2019, 12:38 p.m. UTC
The v2 binding utilises reg and renames some of the v1 properties.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt | 20 +++++---
 1 file changed, 14 insertions(+), 6 deletions(-)

Comments

Rob Herring Dec. 3, 2019, 2:31 p.m. UTC | #1
On Tue, Dec 3, 2019 at 6:36 AM Andrew Jeffery <andrew@aj.id.au> wrote:
>
> The v2 binding utilises reg and renames some of the v1 properties.
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt | 20 +++++---
>  1 file changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt b/Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt
> index d98a9bf45d6c..76b180ebbde4 100644
> --- a/Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt
> +++ b/Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt
> @@ -1,9 +1,10 @@
> -* Aspeed KCS (Keyboard Controller Style) IPMI interface
> +# Aspeed KCS (Keyboard Controller Style) IPMI interface
>
>  The Aspeed SOCs (AST2400 and AST2500) are commonly used as BMCs
>  (Baseboard Management Controllers) and the KCS interface can be
>  used to perform in-band IPMI communication with their host.
>
> +## v1
>  Required properties:
>  - compatible : should be one of
>      "aspeed,ast2400-kcs-bmc"
> @@ -12,14 +13,21 @@ Required properties:
>  - kcs_chan : The LPC channel number in the controller
>  - kcs_addr : The host CPU IO map address
>
> +## v2
> +Required properties:
> +- compatible : should be one of
> +    "aspeed,ast2400-kcs-bmc-v2"
> +    "aspeed,ast2500-kcs-bmc-v2"
> +- reg : The address and size of the IDR, ODR and STR registers
> +- interrupts : interrupt generated by the controller
> +- slave-reg : The host CPU IO map address

aspeed,slave-reg

>
>  Example:
>
> -    kcs3: kcs3@0 {
> -        compatible = "aspeed,ast2500-kcs-bmc";
> -        reg = <0x0 0x80>;
> +    kcs3: kcs@24 {
> +        compatible = "aspeed,ast2500-kcs-bmc-v2";
> +        reg = <0x24 0x1>, <0x30 0x1>, <0x3c 0x1>;

What are the other registers in this address space? I'm not so sure
this is an improvement if you end up with a bunch of nodes with single
registers.

>          interrupts = <8>;
> -        kcs_chan = <3>;
> -        kcs_addr = <0xCA2>;
> +        slave-reg = <0xca2>;
>          status = "okay";
>      };
> --
> git-series 0.9.1
Rob Herring Dec. 5, 2019, 5:50 p.m. UTC | #2
On Wed, Dec 4, 2019 at 11:12 PM Andrew Jeffery <andrew@aj.id.au> wrote:
>
>
>
> On Wed, 4 Dec 2019, at 01:01, Rob Herring wrote:
> > On Tue, Dec 3, 2019 at 6:36 AM Andrew Jeffery <andrew@aj.id.au> wrote:
> > >
> > > The v2 binding utilises reg and renames some of the v1 properties.
> > >
> > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > > ---
> > >  Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt | 20 +++++---
> > >  1 file changed, 14 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt b/Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt
> > > index d98a9bf45d6c..76b180ebbde4 100644
> > > --- a/Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt
> > > +++ b/Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt
> > > @@ -1,9 +1,10 @@
> > > -* Aspeed KCS (Keyboard Controller Style) IPMI interface
> > > +# Aspeed KCS (Keyboard Controller Style) IPMI interface
> > >
> > >  The Aspeed SOCs (AST2400 and AST2500) are commonly used as BMCs
> > >  (Baseboard Management Controllers) and the KCS interface can be
> > >  used to perform in-band IPMI communication with their host.
> > >
> > > +## v1
> > >  Required properties:
> > >  - compatible : should be one of
> > >      "aspeed,ast2400-kcs-bmc"
> > > @@ -12,14 +13,21 @@ Required properties:
> > >  - kcs_chan : The LPC channel number in the controller
> > >  - kcs_addr : The host CPU IO map address
> > >
> > > +## v2
> > > +Required properties:
> > > +- compatible : should be one of
> > > +    "aspeed,ast2400-kcs-bmc-v2"
> > > +    "aspeed,ast2500-kcs-bmc-v2"
> > > +- reg : The address and size of the IDR, ODR and STR registers
> > > +- interrupts : interrupt generated by the controller
> > > +- slave-reg : The host CPU IO map address
> >
> > aspeed,slave-reg
>
> I don't agree, as it's not an aspeed-specific behaviour. This property
> controls where the device appears in the host's LPC IO address space,
> which is a common problem for any LPC IO device exposed by the BMC
> to the host.

Then document it as such. Common properties go into common binding documents.

> > >  Example:
> > >
> > > -    kcs3: kcs3@0 {
> > > -        compatible = "aspeed,ast2500-kcs-bmc";
> > > -        reg = <0x0 0x80>;
> > > +    kcs3: kcs@24 {
> > > +        compatible = "aspeed,ast2500-kcs-bmc-v2";
> > > +        reg = <0x24 0x1>, <0x30 0x1>, <0x3c 0x1>;
> >
> > What are the other registers in this address space? I'm not so sure
> > this is an improvement if you end up with a bunch of nodes with single
> > registers.
>
> Put into practice the bindings give the following patch: on the AST2500:

Okay, that's an unfortunate interleaving, but seems fine.

>
> diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi
> index e8feb8b66a2f..5d51f469cbf0 100644
> --- a/arch/arm/boot/dts/aspeed-g5.dtsi
> +++ b/arch/arm/boot/dts/aspeed-g5.dtsi
> @@ -399,22 +399,22 @@
>                                         #size-cells = <1>;
>                                         ranges = <0x0 0x0 0x80>;
>
> -                                       kcs1: kcs1@0 {
> -                                               compatible = "aspeed,ast2500-kcs-bmc";
> +                                       kcs1: kcs@24 {
> +                                               compatible = "aspeed,ast2500-kcs-bmc-v2";
> +                                               reg = <0x24 0x1>, <0x30 0x1>, <0x3c 0x1>;
>                                                 interrupts = <8>;
> -                                               kcs_chan = <1>;
>                                                 status = "disabled";
>                                         };
> -                                       kcs2: kcs2@0 {
> -                                               compatible = "aspeed,ast2500-kcs-bmc";
> +                                       kcs2: kcs@28 {
> +                                               compatible = "aspeed,ast2500-kcs-bmc-v2";
> +                                               reg = <0x28 0x1>, <0x34 0x1>, <0x40 0x1>;
>                                                 interrupts = <8>;
> -                                               kcs_chan = <2>;
>                                                 status = "disabled";
>                                         };
> -                                       kcs3: kcs3@0 {
> -                                               compatible = "aspeed,ast2500-kcs-bmc";
> +                                       kcs3: kcs@2c {
> +                                               compatible = "aspeed,ast2500-kcs-bmc-v2";
> +                                               reg = <0x2c 0x1>, <0x38 0x1>, <0x44 0x1>;
>                                                 interrupts = <8>;
> -                                               kcs_chan = <3>;
>                                                 status = "disabled";
>                                         };
>                                 };
> @@ -428,10 +428,10 @@
>                                         #size-cells = <1>;
>                                         ranges = <0x0 0x80 0x1e0>;
>
> -                                       kcs4: kcs4@0 {
> -                                               compatible = "aspeed,ast2500-kcs-bmc";
> +                                       kcs4: kcs@94 {
> +                                               compatible = "aspeed,ast2500-kcs-bmc-v2";
> +                                               reg = <0x94 0x1>, <0x98 0x1>, <0x9c 0x1>;
>                                                 interrupts = <8>;
> -                                               kcs_chan = <4>;
>                                                 status = "disabled";
>                                         };
>
> The aim is to fix these warnings which appear for every aspeed-based devicetree:
>
>         arch/arm/boot/dts/aspeed-g5.dtsi:376.19-381.8: Warning (unit_address_vs_reg): /ahb/apb/lpc@1e789000/lpc-bmc@0/kcs1@0: node has a unit name, but no reg property
>         arch/arm/boot/dts/aspeed-g5.dtsi:382.19-387.8: Warning (unit_address_vs_reg): /ahb/apb/lpc@1e789000/lpc-bmc@0/kcs2@0: node has a unit name, but no reg property
>         arch/arm/boot/dts/aspeed-g5.dtsi:388.19-393.8: Warning (unit_address_vs_reg): /ahb/apb/lpc@1e789000/lpc-bmc@0/kcs3@0: node has a unit name, but no reg property
>         arch/arm/boot/dts/aspeed-g5.dtsi:405.19-410.8: Warning (unit_address_vs_reg): /ahb/apb/lpc@1e789000/lpc-host@80/kcs4@0: node has a unit name, but no reg property
>         arch/arm/boot/dts/aspeed-g5.dtsi:376.19-381.8: Warning (unique_unit_address): /ahb/apb/lpc@1e789000/lpc-bmc@0/kcs1@0: duplicate unit-address (also used in node /ahb/apb/lpc@1e789000/lpc-bmc@0/kcs2@0)
>         arch/arm/boot/dts/aspeed-g5.dtsi:376.19-381.8: Warning (unique_unit_address): /ahb/apb/lpc@1e789000/lpc-bmc@0/kcs1@0: duplicate unit-address (also used in node /ahb/apb/lpc@1e789000/lpc-bmc@0/kcs3@0)
>         arch/arm/boot/dts/aspeed-g5.dtsi:382.19-387.8: Warning (unique_unit_address): /ahb/apb/lpc@1e789000/lpc-bmc@0/kcs2@0: duplicate unit-address (also used in node /ahb/apb/lpc@1e789000/lpc-bmc@0/kcs3@0)
>         arch/arm/boot/dts/aspeed-g5.dtsi:405.19-410.8: Warning (unique_unit_address): /ahb/apb/lpc@1e789000/lpc-host@80/kcs4@0: duplicate unit-address (also used in node /ahb/apb/lpc@1e789000/lpc-host@80/lpc-ctrl@0)
>
> Andrew
Andrew Jeffery Dec. 5, 2019, 11:19 p.m. UTC | #3
On Fri, 6 Dec 2019, at 04:20, Rob Herring wrote:
> On Wed, Dec 4, 2019 at 11:12 PM Andrew Jeffery <andrew@aj.id.au> wrote:
> >
> >
> >
> > On Wed, 4 Dec 2019, at 01:01, Rob Herring wrote:
> > > On Tue, Dec 3, 2019 at 6:36 AM Andrew Jeffery <andrew@aj.id.au> wrote:
> > > >
> > > > The v2 binding utilises reg and renames some of the v1 properties.
> > > >
> > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > > > ---
> > > >  Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt | 20 +++++---
> > > >  1 file changed, 14 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt b/Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt
> > > > index d98a9bf45d6c..76b180ebbde4 100644
> > > > --- a/Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt
> > > > +++ b/Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt
> > > > @@ -1,9 +1,10 @@
> > > > -* Aspeed KCS (Keyboard Controller Style) IPMI interface
> > > > +# Aspeed KCS (Keyboard Controller Style) IPMI interface
> > > >
> > > >  The Aspeed SOCs (AST2400 and AST2500) are commonly used as BMCs
> > > >  (Baseboard Management Controllers) and the KCS interface can be
> > > >  used to perform in-band IPMI communication with their host.
> > > >
> > > > +## v1
> > > >  Required properties:
> > > >  - compatible : should be one of
> > > >      "aspeed,ast2400-kcs-bmc"
> > > > @@ -12,14 +13,21 @@ Required properties:
> > > >  - kcs_chan : The LPC channel number in the controller
> > > >  - kcs_addr : The host CPU IO map address
> > > >
> > > > +## v2
> > > > +Required properties:
> > > > +- compatible : should be one of
> > > > +    "aspeed,ast2400-kcs-bmc-v2"
> > > > +    "aspeed,ast2500-kcs-bmc-v2"
> > > > +- reg : The address and size of the IDR, ODR and STR registers
> > > > +- interrupts : interrupt generated by the controller
> > > > +- slave-reg : The host CPU IO map address
> > >
> > > aspeed,slave-reg
> >
> > I don't agree, as it's not an aspeed-specific behaviour. This property
> > controls where the device appears in the host's LPC IO address space,
> > which is a common problem for any LPC IO device exposed by the BMC
> > to the host.
> 
> Then document it as such. Common properties go into common binding documents.

Fair call.

> 
> > > >  Example:
> > > >
> > > > -    kcs3: kcs3@0 {
> > > > -        compatible = "aspeed,ast2500-kcs-bmc";
> > > > -        reg = <0x0 0x80>;
> > > > +    kcs3: kcs@24 {
> > > > +        compatible = "aspeed,ast2500-kcs-bmc-v2";
> > > > +        reg = <0x24 0x1>, <0x30 0x1>, <0x3c 0x1>;
> > >
> > > What are the other registers in this address space? I'm not so sure
> > > this is an improvement if you end up with a bunch of nodes with single
> > > registers.
> >
> > Put into practice the bindings give the following patch: on the AST2500:
> 
> Okay, that's an unfortunate interleaving, but seems fine.

"Unfortunate" is a good description for the entire register layout of the LPC
slave controller.

I'll send a v2.

Thanks,

Andrew

Patch
diff mbox series

diff --git a/Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt b/Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt
index d98a9bf45d6c..76b180ebbde4 100644
--- a/Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt
+++ b/Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt
@@ -1,9 +1,10 @@ 
-* Aspeed KCS (Keyboard Controller Style) IPMI interface
+# Aspeed KCS (Keyboard Controller Style) IPMI interface
 
 The Aspeed SOCs (AST2400 and AST2500) are commonly used as BMCs
 (Baseboard Management Controllers) and the KCS interface can be
 used to perform in-band IPMI communication with their host.
 
+## v1
 Required properties:
 - compatible : should be one of
     "aspeed,ast2400-kcs-bmc"
@@ -12,14 +13,21 @@  Required properties:
 - kcs_chan : The LPC channel number in the controller
 - kcs_addr : The host CPU IO map address
 
+## v2
+Required properties:
+- compatible : should be one of
+    "aspeed,ast2400-kcs-bmc-v2"
+    "aspeed,ast2500-kcs-bmc-v2"
+- reg : The address and size of the IDR, ODR and STR registers
+- interrupts : interrupt generated by the controller
+- slave-reg : The host CPU IO map address
 
 Example:
 
-    kcs3: kcs3@0 {
-        compatible = "aspeed,ast2500-kcs-bmc";
-        reg = <0x0 0x80>;
+    kcs3: kcs@24 {
+        compatible = "aspeed,ast2500-kcs-bmc-v2";
+        reg = <0x24 0x1>, <0x30 0x1>, <0x3c 0x1>;
         interrupts = <8>;
-        kcs_chan = <3>;
-        kcs_addr = <0xCA2>;
+        slave-reg = <0xca2>;
         status = "okay";
     };