From: Rob Herring <robh@kernel.org>
To: ChiaWei Wang <chiawei_wang@aspeedtech.com>
Cc: "joel@jms.id.au" <joel@jms.id.au>,
"andrew@aj.id.au" <andrew@aj.id.au>,
"jk@codeconstruct.com.au" <jk@codeconstruct.com.au>,
"a.kartashev@yadro.com" <a.kartashev@yadro.com>,
"patrick.rudolph@9elements.com" <patrick.rudolph@9elements.com>,
"dphadke@linux.microsoft.com" <dphadke@linux.microsoft.com>,
"linux-aspeed@lists.ozlabs.org" <linux-aspeed@lists.ozlabs.org>,
"openbmc@lists.ozlabs.org" <openbmc@lists.ozlabs.org>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Ryan Chen <ryan_chen@aspeedtech.com>
Subject: Re: [PATCH v5 1/4] dt-bindings: aspeed: Add eSPI controller
Date: Wed, 18 May 2022 13:26:10 -0500 [thread overview]
Message-ID: <20220518182610.GI3302100-robh@kernel.org> (raw)
In-Reply-To: <HK0PR06MB3779B567DE70F859E28F66C991D19@HK0PR06MB3779.apcprd06.prod.outlook.com>
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
next prev parent reply other threads:[~2022-05-18 18:26 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-16 0:54 [PATCH v5 0/4] arm: aspeed: Add eSPI support Chia-Wei Wang
2022-05-16 0:54 ` [PATCH v5 1/4] dt-bindings: aspeed: Add eSPI controller Chia-Wei Wang
2022-05-17 18:31 ` Rob Herring
2022-05-18 0:15 ` ChiaWei Wang
2022-05-18 18:26 ` Rob Herring [this message]
2022-05-19 3:10 ` ChiaWei Wang
2022-05-18 6:18 ` Patrick Rudolph
2022-05-16 0:54 ` [PATCH v5 2/4] MAINTAINER: Add ASPEED eSPI driver entry Chia-Wei Wang
2022-05-16 0:54 ` [PATCH v5 3/4] soc: aspeed: Add eSPI driver Chia-Wei Wang
2022-05-18 19:12 ` Christophe JAILLET
2022-05-19 3:42 ` ChiaWei Wang
2022-05-16 0:54 ` [PATCH v5 4/4] ARM: dts: aspeed: Add eSPI node Chia-Wei Wang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220518182610.GI3302100-robh@kernel.org \
--to=robh@kernel.org \
--cc=a.kartashev@yadro.com \
--cc=andrew@aj.id.au \
--cc=chiawei_wang@aspeedtech.com \
--cc=devicetree@vger.kernel.org \
--cc=dphadke@linux.microsoft.com \
--cc=jk@codeconstruct.com.au \
--cc=joel@jms.id.au \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-aspeed@lists.ozlabs.org \
--cc=linux-kernel@vger.kernel.org \
--cc=openbmc@lists.ozlabs.org \
--cc=patrick.rudolph@9elements.com \
--cc=ryan_chen@aspeedtech.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).