| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 19 May 2022 03:10:39 +0000
From: ChiaWei Wang <chiawei_wang@...eedtech.com>
To: Rob Herring <robh@...nel.org>
CC: "joel@....id.au" <joel@....id.au>,
"andrew@...id.au" <andrew@...id.au>,
"jk@...econstruct.com.au" <jk@...econstruct.com.au>,
"a.kartashev@...ro.com" <a.kartashev@...ro.com>,
"patrick.rudolph@...ements.com" <patrick.rudolph@...ements.com>,
"dphadke@...ux.microsoft.com" <dphadke@...ux.microsoft.com>,
"linux-aspeed@...ts.ozlabs.org" <linux-aspeed@...ts.ozlabs.org>,
"openbmc@...ts.ozlabs.org" <openbmc@...ts.ozlabs.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Ryan Chen <ryan_chen@...eedtech.com>
Subject: RE: [PATCH v5 1/4] dt-bindings: aspeed: Add eSPI controller
> From: Rob Herring <robh@...nel.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@...nel.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@...eedtech.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@...eedtech.com>
> > > > + - Ryan Chen <ryan_chen@...eedtech.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@...ee000 {
> > > > + 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
Powered by blists - more mailing lists