| Message ID | 20220516005412.4844-2-chiawei_wang@aspeedtech.com |
|---|---|
| State | New |
| Headers | show |
| Series | arm: aspeed: Add eSPI support | expand |
On Mon, May 16, 2022 at 08:54:09AM +0800, Chia-Wei Wang wrote: > Add dt-bindings for Aspeed eSPI controller > > Signed-off-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com> > --- > .../devicetree/bindings/soc/aspeed/espi.yaml | 162 ++++++++++++++++++ bindings/spi/ includes SPI slaves. Is there a reason this doesn't fit there? > 1 file changed, 162 insertions(+) > create mode 100644 Documentation/devicetree/bindings/soc/aspeed/espi.yaml > > diff --git a/Documentation/devicetree/bindings/soc/aspeed/espi.yaml b/Documentation/devicetree/bindings/soc/aspeed/espi.yaml > new file mode 100644 > index 000000000000..aa91ec8caf6a > --- /dev/null > +++ b/Documentation/devicetree/bindings/soc/aspeed/espi.yaml > @@ -0,0 +1,162 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +# # Copyright (c) 2021 Aspeed Technology Inc. > +%YAML 1.2 > +--- > +$id: "http://devicetree.org/schemas/soc/aspeed/espi.yaml#" > +$schema: "http://devicetree.org/meta-schemas/core.yaml#" > + > +title: Aspeed eSPI Controller > + > +maintainers: > + - Chia-Wei Wang <chiawei_wang@aspeedtech.com> > + - Ryan Chen <ryan_chen@aspeedtech.com> > + > +description: > + Aspeed eSPI controller implements a slave side eSPI endpoint device > + supporting the four eSPI channels, namely peripheral, virtual wire, > + out-of-band, and flash. > + > +properties: > + compatible: > + items: > + - enum: > + - aspeed,ast2500-espi > + - aspeed,ast2600-espi > + - const: simple-mfd > + - const: syscon > + > + reg: > + maxItems: 1 > + > + "#address-cells": > + const: 1 > + > + "#size-cells": > + const: 1 > + > + ranges: true > + > +patternProperties: > + "^espi-ctrl@[0-9a-f]+$": > + type: object > + > + description: Control of the four basic eSPI channels > + > + properties: > + compatible: > + items: > + - enum: > + - aspeed,ast2500-espi-ctrl > + - aspeed,ast2600-espi-ctrl > + > + interrupts: > + maxItems: 1 > + > + clocks: > + maxItems: 1 > + > + perif,memcyc-enable: What vendor is 'perif'? > + type: boolean > + description: Enable memory cycle over eSPI peripheral channel > + > + perif,memcyc-src-addr: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: The Host side address to be decoded into the memory cycle over eSPI peripheral channel > + > + perif,memcyc-size: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: The size of the memory region allocated for the memory cycle over eSPI peripheral channel > + minimum: 65536 This region is defined by the h/w or just some carveout of system memory? In the former, perhaps this should be part of 'reg'. In the latter case, use a /reserved-memory node and memory-region here. > + > + perif,dma-mode: > + type: boolean > + description: Enable DMA support for eSPI peripheral channel > + > + oob,dma-mode: What vendor is 'oob'? > + type: boolean > + description: Enable DMA support for eSPI out-of-band channel > + > + oob,dma-tx-desc-num: > + $ref: /schemas/types.yaml#/definitions/uint32 > + minimum: 2 > + maximum: 1023 > + description: The number of TX descriptors available for eSPI OOB DMA engine > + > + oob,dma-rx-desc-num: > + $ref: /schemas/types.yaml#/definitions/uint32 > + minimum: 2 > + maximum: 1023 > + description: The number of RX descriptors available for eSPI OOB DMA engine > + > + flash,dma-mode: > + type: boolean > + description: Enable DMA support for eSPI flash channel Why does this need to be in DT. It's configuration. > + > + flash,safs-mode: > + $ref: /schemas/types.yaml#/definitions/uint32 > + enum: [ 0, 1, 2 ] > + default: 0 > + description: Slave-Attached-Sharing-Flash mode, 0->Mix, 1->SW, 2->HW > + > + dependencies: > + perif,memcyc-src-addr: [ "perif,memcyc-enable" ] > + perif,memcyc-size: [ "perif,memcyc-enable" ] > + oob,dma-tx-desc-num: [ "oob,dma-mode" ] > + oob,dma-rx-desc-num: [ "oob,dma-mode" ] > + > + required: > + - compatible > + - interrupts > + - clocks > + > + "^espi-mmbi@[0-9a-f]+$": > + type: object > + > + description: Control of the PCH-BMC data exchange over eSPI peripheral memory cycle > + > + properties: > + compatible: > + const: aspeed,ast2600-espi-mmbi > + > + interrupts: > + maxItems: 1 > + > + required: > + - compatible > + - interrupts > + > +required: > + - compatible > + - reg > + - "#address-cells" > + - "#size-cells" > + - ranges > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + #include <dt-bindings/clock/ast2600-clock.h> > + > + espi: espi@1e6ee000 { > + compatible = "aspeed,ast2600-espi", "simple-mfd", "syscon"; > + reg = <0x1e6ee000 0x1000>; > + > + #address-cells = <1>; > + #size-cells = <1>; > + ranges = <0x0 0x1e6ee000 0x1000>; > + > + espi_ctrl: espi-ctrl@0 { > + compatible = "aspeed,ast2600-espi-ctrl"; > + reg = <0x0 0x800>; > + interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&syscon ASPEED_CLK_GATE_ESPICLK>; > + }; > + > + espi_mmbi: espi-mmbi@800 { > + compatible = "aspeed,ast2600-espi-mmbi"; > + reg = <0x800 0x50>; > + interrupts = <GIC_SPI 108 IRQ_TYPE_LEVEL_HIGH>; > + }; Why do you need these child nodes? Are the subblocks somehow useful on their own or reuseable in another configuration? If not, looks like this could all be 1 node. Rob
Hi Rob, > From: Rob Herring <robh@kernel.org> > Sent: Wednesday, May 18, 2022 2:32 AM > > On Mon, May 16, 2022 at 08:54:09AM +0800, Chia-Wei Wang wrote: > > Add dt-bindings for Aspeed eSPI controller > > > > Signed-off-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com> > > --- > > .../devicetree/bindings/soc/aspeed/espi.yaml | 162 > > ++++++++++++++++++ > > bindings/spi/ includes SPI slaves. Is there a reason this doesn't fit there? eSPI resues the timing and electrical specification of SPI but runs completely different protocol. Only the flash channel is related to SPI and the other 3 channels are for EC/BMC/SIO. Therefore, an eSPI driver does not fit into the SPI model. > > > 1 file changed, 162 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/soc/aspeed/espi.yaml > > > > diff --git a/Documentation/devicetree/bindings/soc/aspeed/espi.yaml > > b/Documentation/devicetree/bindings/soc/aspeed/espi.yaml > > new file mode 100644 > > index 000000000000..aa91ec8caf6a > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/soc/aspeed/espi.yaml > > @@ -0,0 +1,162 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) # # > > +Copyright (c) 2021 Aspeed Technology Inc. > > +%YAML 1.2 > > +--- > > +$id: "http://devicetree.org/schemas/soc/aspeed/espi.yaml#" > > +$schema: "http://devicetree.org/meta-schemas/core.yaml#" > > + > > +title: Aspeed eSPI Controller > > + > > +maintainers: > > + - Chia-Wei Wang <chiawei_wang@aspeedtech.com> > > + - Ryan Chen <ryan_chen@aspeedtech.com> > > + > > +description: > > + Aspeed eSPI controller implements a slave side eSPI endpoint device > > + supporting the four eSPI channels, namely peripheral, virtual wire, > > + out-of-band, and flash. > > + > > +properties: > > + compatible: > > + items: > > + - enum: > > + - aspeed,ast2500-espi > > + - aspeed,ast2600-espi > > + - const: simple-mfd > > + - const: syscon > > + > > + reg: > > + maxItems: 1 > > + > > + "#address-cells": > > + const: 1 > > + > > + "#size-cells": > > + const: 1 > > + > > + ranges: true > > + > > +patternProperties: > > + "^espi-ctrl@[0-9a-f]+$": > > + type: object > > + > > + description: Control of the four basic eSPI channels > > + > > + properties: > > + compatible: > > + items: > > + - enum: > > + - aspeed,ast2500-espi-ctrl > > + - aspeed,ast2600-espi-ctrl > > + > > + interrupts: > > + maxItems: 1 > > + > > + clocks: > > + maxItems: 1 > > + > > + perif,memcyc-enable: > > What vendor is 'perif'? It refers to the eSPI peripheral channel. > > > + type: boolean > > + description: Enable memory cycle over eSPI peripheral channel > > + > > + perif,memcyc-src-addr: > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + description: The Host side address to be decoded into the > > + memory cycle over eSPI peripheral channel > > + > > + perif,memcyc-size: > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + description: The size of the memory region allocated for the > memory cycle over eSPI peripheral channel > > + minimum: 65536 > > This region is defined by the h/w or just some carveout of system memory? In > the former, perhaps this should be part of 'reg'. In the latter case, use a > /reserved-memory node and memory-region here. The region is going to be allocated at runtime phase. It is a kind of shared memory between Host and BMC. > > > + > > + perif,dma-mode: > > + type: boolean > > + description: Enable DMA support for eSPI peripheral channel > > + > > + oob,dma-mode: > > What vendor is 'oob'? It refers to the eSPI out-of-band channel. > > > + type: boolean > > + description: Enable DMA support for eSPI out-of-band channel > > + > > + oob,dma-tx-desc-num: > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + minimum: 2 > > + maximum: 1023 > > + description: The number of TX descriptors available for eSPI > > + OOB DMA engine > > + > > + oob,dma-rx-desc-num: > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + minimum: 2 > > + maximum: 1023 > > + description: The number of RX descriptors available for eSPI > > + OOB DMA engine > > + > > + flash,dma-mode: > > + type: boolean > > + description: Enable DMA support for eSPI flash channel > > Why does this need to be in DT. It's configuration. The property is used to decide the operation mode (i.e. FIFO or DMA) of the eSPI flash channel. Is it a wrong idea to use the DTS property for? > > > + > > + flash,safs-mode: > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + enum: [ 0, 1, 2 ] > > + default: 0 > > + description: Slave-Attached-Sharing-Flash mode, 0->Mix, > > + 1->SW, 2->HW > > + > > + dependencies: > > + perif,memcyc-src-addr: [ "perif,memcyc-enable" ] > > + perif,memcyc-size: [ "perif,memcyc-enable" ] > > + oob,dma-tx-desc-num: [ "oob,dma-mode" ] > > + oob,dma-rx-desc-num: [ "oob,dma-mode" ] > > + > > + required: > > + - compatible > > + - interrupts > > + - clocks > > + > > + "^espi-mmbi@[0-9a-f]+$": > > + type: object > > + > > + description: Control of the PCH-BMC data exchange over eSPI > > + peripheral memory cycle > > + > > + properties: > > + compatible: > > + const: aspeed,ast2600-espi-mmbi > > + > > + interrupts: > > + maxItems: 1 > > + > > + required: > > + - compatible > > + - interrupts > > + > > +required: > > + - compatible > > + - reg > > + - "#address-cells" > > + - "#size-cells" > > + - ranges > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > + #include <dt-bindings/clock/ast2600-clock.h> > > + > > + espi: espi@1e6ee000 { > > + compatible = "aspeed,ast2600-espi", "simple-mfd", "syscon"; > > + reg = <0x1e6ee000 0x1000>; > > + > > + #address-cells = <1>; > > + #size-cells = <1>; > > + ranges = <0x0 0x1e6ee000 0x1000>; > > + > > + espi_ctrl: espi-ctrl@0 { > > + compatible = "aspeed,ast2600-espi-ctrl"; > > + reg = <0x0 0x800>; > > + interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>; > > + clocks = <&syscon ASPEED_CLK_GATE_ESPICLK>; > > + }; > > + > > + espi_mmbi: espi-mmbi@800 { > > + compatible = "aspeed,ast2600-espi-mmbi"; > > + reg = <0x800 0x50>; > > + interrupts = <GIC_SPI 108 IRQ_TYPE_LEVEL_HIGH>; > > + }; > > Why do you need these child nodes? Are the subblocks somehow useful on > their own or reuseable in another configuration? If not, looks like this could all > be 1 node. espi-mmbi has individual function and control registers. However, espi-mmbi is also a feature extended based on the memory cycle of eSPI peripheral channel. Thereby, it has dependency on the eSPI channel initialization conducted by espi-ctrl. The scenario is similar to the lpc-ctrl and other lpc-xxx drivers of Aspeed SoCs. Chiawei
On Mon, May 16, 2022 at 2:55 AM Chia-Wei Wang <chiawei_wang@aspeedtech.com> wrote: > > Add dt-bindings for Aspeed eSPI controller > > Signed-off-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com> > --- > .../devicetree/bindings/soc/aspeed/espi.yaml | 162 ++++++++++++++++++ > 1 file changed, 162 insertions(+) > create mode 100644 Documentation/devicetree/bindings/soc/aspeed/espi.yaml > > diff --git a/Documentation/devicetree/bindings/soc/aspeed/espi.yaml b/Documentation/devicetree/bindings/soc/aspeed/espi.yaml > new file mode 100644 > index 000000000000..aa91ec8caf6a > --- /dev/null > +++ b/Documentation/devicetree/bindings/soc/aspeed/espi.yaml > @@ -0,0 +1,162 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +# # Copyright (c) 2021 Aspeed Technology Inc. > +%YAML 1.2 > +--- > +$id: "http://devicetree.org/schemas/soc/aspeed/espi.yaml#" > +$schema: "http://devicetree.org/meta-schemas/core.yaml#" > + > +title: Aspeed eSPI Controller > + > +maintainers: > + - Chia-Wei Wang <chiawei_wang@aspeedtech.com> > + - Ryan Chen <ryan_chen@aspeedtech.com> > + > +description: > + Aspeed eSPI controller implements a slave side eSPI endpoint device > + supporting the four eSPI channels, namely peripheral, virtual wire, > + out-of-band, and flash. > + > +properties: > + compatible: > + items: > + - enum: > + - aspeed,ast2500-espi > + - aspeed,ast2600-espi > + - const: simple-mfd > + - const: syscon > + > + reg: > + maxItems: 1 > + > + "#address-cells": > + const: 1 > + > + "#size-cells": > + const: 1 > + > + ranges: true > + > +patternProperties: > + "^espi-ctrl@[0-9a-f]+$": > + type: object > + > + description: Control of the four basic eSPI channels > + > + properties: > + compatible: > + items: > + - enum: > + - aspeed,ast2500-espi-ctrl > + - aspeed,ast2600-espi-ctrl > + > + interrupts: > + maxItems: 1 > + > + clocks: > + maxItems: 1 > + > + perif,memcyc-enable: > + type: boolean > + description: Enable memory cycle over eSPI peripheral channel > + > + perif,memcyc-src-addr: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: The Host side address to be decoded into the memory cycle over eSPI peripheral channel > + > + perif,memcyc-size: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: The size of the memory region allocated for the memory cycle over eSPI peripheral channel > + minimum: 65536 > + > + perif,dma-mode: > + type: boolean > + description: Enable DMA support for eSPI peripheral channel > + > + oob,dma-mode: > + type: boolean > + description: Enable DMA support for eSPI out-of-band channel > + > + oob,dma-tx-desc-num: > + $ref: /schemas/types.yaml#/definitions/uint32 > + minimum: 2 > + maximum: 1023 > + description: The number of TX descriptors available for eSPI OOB DMA engine > + > + oob,dma-rx-desc-num: > + $ref: /schemas/types.yaml#/definitions/uint32 > + minimum: 2 > + maximum: 1023 > + description: The number of RX descriptors available for eSPI OOB DMA engine > + > + flash,dma-mode: > + type: boolean > + description: Enable DMA support for eSPI flash channel > + In my tests using this driver the FIFO mode didn't work, but the DMA mode worked fine. IMHO you should always use DMA mode to reduce CPU load and get rid of this property. > + flash,safs-mode: > + $ref: /schemas/types.yaml#/definitions/uint32 > + enum: [ 0, 1, 2 ] > + default: 0 > + description: Slave-Attached-Sharing-Flash mode, 0->Mix, 1->SW, 2->HW > + On the AST2500 the HW mode is not supported, is it? > + dependencies: > + perif,memcyc-src-addr: [ "perif,memcyc-enable" ] > + perif,memcyc-size: [ "perif,memcyc-enable" ] > + oob,dma-tx-desc-num: [ "oob,dma-mode" ] > + oob,dma-rx-desc-num: [ "oob,dma-mode" ] > + > + required: > + - compatible > + - interrupts > + - clocks > + > + "^espi-mmbi@[0-9a-f]+$": > + type: object > + > + description: Control of the PCH-BMC data exchange over eSPI peripheral memory cycle > + > + properties: > + compatible: > + const: aspeed,ast2600-espi-mmbi > + > + interrupts: > + maxItems: 1 > + > + required: > + - compatible > + - interrupts > + > +required: > + - compatible > + - reg > + - "#address-cells" > + - "#size-cells" > + - ranges > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + #include <dt-bindings/clock/ast2600-clock.h> > + > + espi: espi@1e6ee000 { > + compatible = "aspeed,ast2600-espi", "simple-mfd", "syscon"; > + reg = <0x1e6ee000 0x1000>; > + > + #address-cells = <1>; > + #size-cells = <1>; > + ranges = <0x0 0x1e6ee000 0x1000>; > + > + espi_ctrl: espi-ctrl@0 { > + compatible = "aspeed,ast2600-espi-ctrl"; > + reg = <0x0 0x800>; > + interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&syscon ASPEED_CLK_GATE_ESPICLK>; > + }; > + > + espi_mmbi: espi-mmbi@800 { > + compatible = "aspeed,ast2600-espi-mmbi"; > + reg = <0x800 0x50>; > + interrupts = <GIC_SPI 108 IRQ_TYPE_LEVEL_HIGH>; > + }; > + }; > -- > 2.25.1 > Regards, Patrick
On Wed, May 18, 2022 at 12:15:10AM +0000, ChiaWei Wang wrote: > Hi Rob, > > > From: Rob Herring <robh@kernel.org> > > Sent: Wednesday, May 18, 2022 2:32 AM > > > > On Mon, May 16, 2022 at 08:54:09AM +0800, Chia-Wei Wang wrote: > > > Add dt-bindings for Aspeed eSPI controller > > > > > > Signed-off-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com> > > > --- > > > .../devicetree/bindings/soc/aspeed/espi.yaml | 162 > > > ++++++++++++++++++ > > > > bindings/spi/ includes SPI slaves. Is there a reason this doesn't fit there? > > eSPI resues the timing and electrical specification of SPI but runs completely different protocol. > Only the flash channel is related to SPI and the other 3 channels are for EC/BMC/SIO. > Therefore, an eSPI driver does not fit into the SPI model. > > > > > > 1 file changed, 162 insertions(+) > > > create mode 100644 > > > Documentation/devicetree/bindings/soc/aspeed/espi.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/soc/aspeed/espi.yaml > > > b/Documentation/devicetree/bindings/soc/aspeed/espi.yaml > > > new file mode 100644 > > > index 000000000000..aa91ec8caf6a > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/soc/aspeed/espi.yaml > > > @@ -0,0 +1,162 @@ > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) # # > > > +Copyright (c) 2021 Aspeed Technology Inc. > > > +%YAML 1.2 > > > +--- > > > +$id: "http://devicetree.org/schemas/soc/aspeed/espi.yaml#" > > > +$schema: "http://devicetree.org/meta-schemas/core.yaml#" > > > + > > > +title: Aspeed eSPI Controller > > > + > > > +maintainers: > > > + - Chia-Wei Wang <chiawei_wang@aspeedtech.com> > > > + - Ryan Chen <ryan_chen@aspeedtech.com> > > > + > > > +description: > > > + Aspeed eSPI controller implements a slave side eSPI endpoint device > > > + supporting the four eSPI channels, namely peripheral, virtual wire, > > > + out-of-band, and flash. > > > + > > > +properties: > > > + compatible: > > > + items: > > > + - enum: > > > + - aspeed,ast2500-espi > > > + - aspeed,ast2600-espi > > > + - const: simple-mfd > > > + - const: syscon > > > + > > > + reg: > > > + maxItems: 1 > > > + > > > + "#address-cells": > > > + const: 1 > > > + > > > + "#size-cells": > > > + const: 1 > > > + > > > + ranges: true > > > + > > > +patternProperties: > > > + "^espi-ctrl@[0-9a-f]+$": > > > + type: object > > > + > > > + description: Control of the four basic eSPI channels > > > + > > > + properties: > > > + compatible: > > > + items: > > > + - enum: > > > + - aspeed,ast2500-espi-ctrl > > > + - aspeed,ast2600-espi-ctrl > > > + > > > + interrupts: > > > + maxItems: 1 > > > + > > > + clocks: > > > + maxItems: 1 > > > + > > > + perif,memcyc-enable: > > > > What vendor is 'perif'? > > It refers to the eSPI peripheral channel. The convention for properties is <vendor-prefix>,<property-name>. Fix your property names to follow this. > > > > > > + type: boolean > > > + description: Enable memory cycle over eSPI peripheral channel > > > + > > > + perif,memcyc-src-addr: > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > + description: The Host side address to be decoded into the > > > + memory cycle over eSPI peripheral channel > > > + > > > + perif,memcyc-size: > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > + description: The size of the memory region allocated for the > > memory cycle over eSPI peripheral channel > > > + minimum: 65536 > > > > This region is defined by the h/w or just some carveout of system memory? In > > the former, perhaps this should be part of 'reg'. In the latter case, use a > > /reserved-memory node and memory-region here. > > The region is going to be allocated at runtime phase. > It is a kind of shared memory between Host and BMC. Use /reserved-memory. > > > > > > + > > > + perif,dma-mode: > > > + type: boolean > > > + description: Enable DMA support for eSPI peripheral channel > > > + > > > + oob,dma-mode: > > > > What vendor is 'oob'? > > It refers to the eSPI out-of-band channel. > > > > > > + type: boolean > > > + description: Enable DMA support for eSPI out-of-band channel > > > + > > > + oob,dma-tx-desc-num: > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > + minimum: 2 > > > + maximum: 1023 > > > + description: The number of TX descriptors available for eSPI > > > + OOB DMA engine > > > + > > > + oob,dma-rx-desc-num: > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > + minimum: 2 > > > + maximum: 1023 > > > + description: The number of RX descriptors available for eSPI > > > + OOB DMA engine > > > + > > > + flash,dma-mode: > > > + type: boolean > > > + description: Enable DMA support for eSPI flash channel > > > > Why does this need to be in DT. It's configuration. > > The property is used to decide the operation mode (i.e. FIFO or DMA) of the eSPI flash channel. > Is it a wrong idea to use the DTS property for? > > > > > > + > > > + flash,safs-mode: > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > + enum: [ 0, 1, 2 ] > > > + default: 0 > > > + description: Slave-Attached-Sharing-Flash mode, 0->Mix, > > > + 1->SW, 2->HW > > > + > > > + dependencies: > > > + perif,memcyc-src-addr: [ "perif,memcyc-enable" ] > > > + perif,memcyc-size: [ "perif,memcyc-enable" ] > > > + oob,dma-tx-desc-num: [ "oob,dma-mode" ] > > > + oob,dma-rx-desc-num: [ "oob,dma-mode" ] > > > + > > > + required: > > > + - compatible > > > + - interrupts > > > + - clocks > > > + > > > + "^espi-mmbi@[0-9a-f]+$": > > > + type: object > > > + > > > + description: Control of the PCH-BMC data exchange over eSPI > > > + peripheral memory cycle > > > + > > > + properties: > > > + compatible: > > > + const: aspeed,ast2600-espi-mmbi > > > + > > > + interrupts: > > > + maxItems: 1 > > > + > > > + required: > > > + - compatible > > > + - interrupts > > > + > > > +required: > > > + - compatible > > > + - reg > > > + - "#address-cells" > > > + - "#size-cells" > > > + - ranges > > > + > > > +additionalProperties: false > > > + > > > +examples: > > > + - | > > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > > + #include <dt-bindings/clock/ast2600-clock.h> > > > + > > > + espi: espi@1e6ee000 { > > > + compatible = "aspeed,ast2600-espi", "simple-mfd", "syscon"; > > > + reg = <0x1e6ee000 0x1000>; > > > + > > > + #address-cells = <1>; > > > + #size-cells = <1>; > > > + ranges = <0x0 0x1e6ee000 0x1000>; > > > + > > > + espi_ctrl: espi-ctrl@0 { > > > + compatible = "aspeed,ast2600-espi-ctrl"; > > > + reg = <0x0 0x800>; > > > + interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>; > > > + clocks = <&syscon ASPEED_CLK_GATE_ESPICLK>; > > > + }; > > > + > > > + espi_mmbi: espi-mmbi@800 { > > > + compatible = "aspeed,ast2600-espi-mmbi"; > > > + reg = <0x800 0x50>; > > > + interrupts = <GIC_SPI 108 IRQ_TYPE_LEVEL_HIGH>; > > > + }; > > > > Why do you need these child nodes? Are the subblocks somehow useful on > > their own or reuseable in another configuration? If not, looks like this could all > > be 1 node. > > espi-mmbi has individual function and control registers. > However, espi-mmbi is also a feature extended based on the memory cycle of eSPI peripheral channel. > Thereby, it has dependency on the eSPI channel initialization conducted by espi-ctrl. > The scenario is similar to the lpc-ctrl and other lpc-xxx drivers of Aspeed SoCs. Doesn't LPC have independent downstream devices like a bus? Is this a bus where the ESPI controls access to MMBI and espi-ctrl devices? If so, then the devices need their own binding and descriptions. But it doesn't really look like that to me given the limited description. Rob
> From: Rob Herring <robh@kernel.org> > Sent: Thursday, May 19, 2022 2:26 AM > > On Wed, May 18, 2022 at 12:15:10AM +0000, ChiaWei Wang wrote: > > Hi Rob, > > > > > From: Rob Herring <robh@kernel.org> > > > Sent: Wednesday, May 18, 2022 2:32 AM > > > > > > On Mon, May 16, 2022 at 08:54:09AM +0800, Chia-Wei Wang wrote: > > > > Add dt-bindings for Aspeed eSPI controller > > > > > > > > Signed-off-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com> > > > > --- > > > > .../devicetree/bindings/soc/aspeed/espi.yaml | 162 > > > > ++++++++++++++++++ > > > > > > bindings/spi/ includes SPI slaves. Is there a reason this doesn't fit there? > > > > eSPI resues the timing and electrical specification of SPI but runs completely > different protocol. > > Only the flash channel is related to SPI and the other 3 channels are for > EC/BMC/SIO. > > Therefore, an eSPI driver does not fit into the SPI model. > > > > > > > > > 1 file changed, 162 insertions(+) create mode 100644 > > > > Documentation/devicetree/bindings/soc/aspeed/espi.yaml > > > > > > > > diff --git > > > > a/Documentation/devicetree/bindings/soc/aspeed/espi.yaml > > > > b/Documentation/devicetree/bindings/soc/aspeed/espi.yaml > > > > new file mode 100644 > > > > index 000000000000..aa91ec8caf6a > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/soc/aspeed/espi.yaml > > > > @@ -0,0 +1,162 @@ > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) # # > > > > +Copyright (c) 2021 Aspeed Technology Inc. > > > > +%YAML 1.2 > > > > +--- > > > > +$id: "http://devicetree.org/schemas/soc/aspeed/espi.yaml#" > > > > +$schema: "http://devicetree.org/meta-schemas/core.yaml#" > > > > + > > > > +title: Aspeed eSPI Controller > > > > + > > > > +maintainers: > > > > + - Chia-Wei Wang <chiawei_wang@aspeedtech.com> > > > > + - Ryan Chen <ryan_chen@aspeedtech.com> > > > > + > > > > +description: > > > > + Aspeed eSPI controller implements a slave side eSPI endpoint > > > > +device > > > > + supporting the four eSPI channels, namely peripheral, virtual > > > > +wire, > > > > + out-of-band, and flash. > > > > + > > > > +properties: > > > > + compatible: > > > > + items: > > > > + - enum: > > > > + - aspeed,ast2500-espi > > > > + - aspeed,ast2600-espi > > > > + - const: simple-mfd > > > > + - const: syscon > > > > + > > > > + reg: > > > > + maxItems: 1 > > > > + > > > > + "#address-cells": > > > > + const: 1 > > > > + > > > > + "#size-cells": > > > > + const: 1 > > > > + > > > > + ranges: true > > > > + > > > > +patternProperties: > > > > + "^espi-ctrl@[0-9a-f]+$": > > > > + type: object > > > > + > > > > + description: Control of the four basic eSPI channels > > > > + > > > > + properties: > > > > + compatible: > > > > + items: > > > > + - enum: > > > > + - aspeed,ast2500-espi-ctrl > > > > + - aspeed,ast2600-espi-ctrl > > > > + > > > > + interrupts: > > > > + maxItems: 1 > > > > + > > > > + clocks: > > > > + maxItems: 1 > > > > + > > > > + perif,memcyc-enable: > > > > > > What vendor is 'perif'? > > > > It refers to the eSPI peripheral channel. > > The convention for properties is <vendor-prefix>,<property-name>. Fix your > property names to follow this. Will revise as suggested. > > > > > > > > > > + type: boolean > > > > + description: Enable memory cycle over eSPI peripheral > > > > + channel > > > > + > > > > + perif,memcyc-src-addr: > > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > > + description: The Host side address to be decoded into the > > > > + memory cycle over eSPI peripheral channel > > > > + > > > > + perif,memcyc-size: > > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > > + description: The size of the memory region allocated for > > > > + the > > > memory cycle over eSPI peripheral channel > > > > + minimum: 65536 > > > > > > This region is defined by the h/w or just some carveout of system > > > memory? In the former, perhaps this should be part of 'reg'. In the > > > latter case, use a /reserved-memory node and memory-region here. > > > > The region is going to be allocated at runtime phase. > > It is a kind of shared memory between Host and BMC. > > Use /reserved-memory. Will revise as suggested. > > > > > > > > > > + > > > > + perif,dma-mode: > > > > + type: boolean > > > > + description: Enable DMA support for eSPI peripheral > > > > + channel > > > > + > > > > + oob,dma-mode: > > > > > > What vendor is 'oob'? > > > > It refers to the eSPI out-of-band channel. > > > > > > > > > + type: boolean > > > > + description: Enable DMA support for eSPI out-of-band > > > > + channel > > > > + > > > > + oob,dma-tx-desc-num: > > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > > + minimum: 2 > > > > + maximum: 1023 > > > > + description: The number of TX descriptors available for > > > > + eSPI OOB DMA engine > > > > + > > > > + oob,dma-rx-desc-num: > > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > > + minimum: 2 > > > > + maximum: 1023 > > > > + description: The number of RX descriptors available for > > > > + eSPI OOB DMA engine > > > > + > > > > + flash,dma-mode: > > > > + type: boolean > > > > + description: Enable DMA support for eSPI flash channel > > > > > > Why does this need to be in DT. It's configuration. > > > > The property is used to decide the operation mode (i.e. FIFO or DMA) of the > eSPI flash channel. > > Is it a wrong idea to use the DTS property for? > > > > > > > > > + > > > > + flash,safs-mode: > > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > > + enum: [ 0, 1, 2 ] > > > > + default: 0 > > > > + description: Slave-Attached-Sharing-Flash mode, 0->Mix, > > > > + 1->SW, 2->HW > > > > + > > > > + dependencies: > > > > + perif,memcyc-src-addr: [ "perif,memcyc-enable" ] > > > > + perif,memcyc-size: [ "perif,memcyc-enable" ] > > > > + oob,dma-tx-desc-num: [ "oob,dma-mode" ] > > > > + oob,dma-rx-desc-num: [ "oob,dma-mode" ] > > > > + > > > > + required: > > > > + - compatible > > > > + - interrupts > > > > + - clocks > > > > + > > > > + "^espi-mmbi@[0-9a-f]+$": > > > > + type: object > > > > + > > > > + description: Control of the PCH-BMC data exchange over eSPI > > > > + peripheral memory cycle > > > > + > > > > + properties: > > > > + compatible: > > > > + const: aspeed,ast2600-espi-mmbi > > > > + > > > > + interrupts: > > > > + maxItems: 1 > > > > + > > > > + required: > > > > + - compatible > > > > + - interrupts > > > > + > > > > +required: > > > > + - compatible > > > > + - reg > > > > + - "#address-cells" > > > > + - "#size-cells" > > > > + - ranges > > > > + > > > > +additionalProperties: false > > > > + > > > > +examples: > > > > + - | > > > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > > > + #include <dt-bindings/clock/ast2600-clock.h> > > > > + > > > > + espi: espi@1e6ee000 { > > > > + compatible = "aspeed,ast2600-espi", "simple-mfd", "syscon"; > > > > + reg = <0x1e6ee000 0x1000>; > > > > + > > > > + #address-cells = <1>; > > > > + #size-cells = <1>; > > > > + ranges = <0x0 0x1e6ee000 0x1000>; > > > > + > > > > + espi_ctrl: espi-ctrl@0 { > > > > + compatible = "aspeed,ast2600-espi-ctrl"; > > > > + reg = <0x0 0x800>; > > > > + interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>; > > > > + clocks = <&syscon ASPEED_CLK_GATE_ESPICLK>; > > > > + }; > > > > + > > > > + espi_mmbi: espi-mmbi@800 { > > > > + compatible = "aspeed,ast2600-espi-mmbi"; > > > > + reg = <0x800 0x50>; > > > > + interrupts = <GIC_SPI 108 IRQ_TYPE_LEVEL_HIGH>; > > > > + }; > > > > > > Why do you need these child nodes? Are the subblocks somehow useful > > > on their own or reuseable in another configuration? If not, looks > > > like this could all be 1 node. > > > > espi-mmbi has individual function and control registers. > > However, espi-mmbi is also a feature extended based on the memory cycle > of eSPI peripheral channel. > > Thereby, it has dependency on the eSPI channel initialization conducted by > espi-ctrl. > > The scenario is similar to the lpc-ctrl and other lpc-xxx drivers of Aspeed > SoCs. > > Doesn't LPC have independent downstream devices like a bus? No. We use MFD simply because the HW design places multiple devices in the same HW block. And they do share certain registers as the control bits are mixed. > Is this a bus where the ESPI controls access to MMBI and espi-ctrl devices? If so, then the > devices need their own binding and descriptions. But it doesn't really look like > that to me given the limited description. No. The eSPI HW block contains eSPI main device (the 4 channel) and eSPI MMBI devices. So I use MFD based on the same reason as LPC block. And MMBI will have individual binding and description in the later submission. If it is not appropriate way to use MFD, I will revise the implementation into one driver. Thanks for your feedback and suggestions. Chiawei
diff --git a/Documentation/devicetree/bindings/soc/aspeed/espi.yaml b/Documentation/devicetree/bindings/soc/aspeed/espi.yaml new file mode 100644 index 000000000000..aa91ec8caf6a --- /dev/null +++ b/Documentation/devicetree/bindings/soc/aspeed/espi.yaml @@ -0,0 +1,162 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +# # Copyright (c) 2021 Aspeed Technology Inc. +%YAML 1.2 +--- +$id: "http://devicetree.org/schemas/soc/aspeed/espi.yaml#" +$schema: "http://devicetree.org/meta-schemas/core.yaml#" + +title: Aspeed eSPI Controller + +maintainers: + - Chia-Wei Wang <chiawei_wang@aspeedtech.com> + - Ryan Chen <ryan_chen@aspeedtech.com> + +description: + Aspeed eSPI controller implements a slave side eSPI endpoint device + supporting the four eSPI channels, namely peripheral, virtual wire, + out-of-band, and flash. + +properties: + compatible: + items: + - enum: + - aspeed,ast2500-espi + - aspeed,ast2600-espi + - const: simple-mfd + - const: syscon + + reg: + maxItems: 1 + + "#address-cells": + const: 1 + + "#size-cells": + const: 1 + + ranges: true + +patternProperties: + "^espi-ctrl@[0-9a-f]+$": + type: object + + description: Control of the four basic eSPI channels + + properties: + compatible: + items: + - enum: + - aspeed,ast2500-espi-ctrl + - aspeed,ast2600-espi-ctrl + + interrupts: + maxItems: 1 + + clocks: + maxItems: 1 + + perif,memcyc-enable: + type: boolean + description: Enable memory cycle over eSPI peripheral channel + + perif,memcyc-src-addr: + $ref: /schemas/types.yaml#/definitions/uint32 + description: The Host side address to be decoded into the memory cycle over eSPI peripheral channel + + perif,memcyc-size: + $ref: /schemas/types.yaml#/definitions/uint32 + description: The size of the memory region allocated for the memory cycle over eSPI peripheral channel + minimum: 65536 + + perif,dma-mode: + type: boolean + description: Enable DMA support for eSPI peripheral channel + + oob,dma-mode: + type: boolean + description: Enable DMA support for eSPI out-of-band channel + + oob,dma-tx-desc-num: + $ref: /schemas/types.yaml#/definitions/uint32 + minimum: 2 + maximum: 1023 + description: The number of TX descriptors available for eSPI OOB DMA engine + + oob,dma-rx-desc-num: + $ref: /schemas/types.yaml#/definitions/uint32 + minimum: 2 + maximum: 1023 + description: The number of RX descriptors available for eSPI OOB DMA engine + + flash,dma-mode: + type: boolean + description: Enable DMA support for eSPI flash channel + + flash,safs-mode: + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [ 0, 1, 2 ] + default: 0 + description: Slave-Attached-Sharing-Flash mode, 0->Mix, 1->SW, 2->HW + + dependencies: + perif,memcyc-src-addr: [ "perif,memcyc-enable" ] + perif,memcyc-size: [ "perif,memcyc-enable" ] + oob,dma-tx-desc-num: [ "oob,dma-mode" ] + oob,dma-rx-desc-num: [ "oob,dma-mode" ] + + required: + - compatible + - interrupts + - clocks + + "^espi-mmbi@[0-9a-f]+$": + type: object + + description: Control of the PCH-BMC data exchange over eSPI peripheral memory cycle + + properties: + compatible: + const: aspeed,ast2600-espi-mmbi + + interrupts: + maxItems: 1 + + required: + - compatible + - interrupts + +required: + - compatible + - reg + - "#address-cells" + - "#size-cells" + - ranges + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/clock/ast2600-clock.h> + + espi: espi@1e6ee000 { + compatible = "aspeed,ast2600-espi", "simple-mfd", "syscon"; + reg = <0x1e6ee000 0x1000>; + + #address-cells = <1>; + #size-cells = <1>; + ranges = <0x0 0x1e6ee000 0x1000>; + + espi_ctrl: espi-ctrl@0 { + compatible = "aspeed,ast2600-espi-ctrl"; + reg = <0x0 0x800>; + interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&syscon ASPEED_CLK_GATE_ESPICLK>; + }; + + espi_mmbi: espi-mmbi@800 { + compatible = "aspeed,ast2600-espi-mmbi"; + reg = <0x800 0x50>; + interrupts = <GIC_SPI 108 IRQ_TYPE_LEVEL_HIGH>; + }; + };
Add dt-bindings for Aspeed eSPI controller Signed-off-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com> --- .../devicetree/bindings/soc/aspeed/espi.yaml | 162 ++++++++++++++++++ 1 file changed, 162 insertions(+) create mode 100644 Documentation/devicetree/bindings/soc/aspeed/espi.yaml