| Message ID | 3da2492c244962c27b21aad87bfa6bf74f568f1d.1575376664.git-series.andrew@aj.id.au |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | ipmi: kcs-bmc: Rework bindings to clean up DT warnings | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success |
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
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
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
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"; };
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(-)