* [PATCH v2 0/2] mmc: Add support for the ASPEED SD controller @ 2019-07-12 3:32 Andrew Jeffery 2019-07-12 3:32 ` [PATCH v2 1/2] dt-bindings: mmc: Document Aspeed " Andrew Jeffery 2019-07-12 3:32 ` [PATCH v2 2/2] mmc: Add support for the ASPEED " Andrew Jeffery 0 siblings, 2 replies; 17+ messages in thread From: Andrew Jeffery @ 2019-07-12 3:32 UTC (permalink / raw) To: linux-mmc Cc: mark.rutland, devicetree, ulf.hansson, linux-aspeed, Andrew Jeffery, ryanchen.aspeed, adrian.hunter, linux-kernel, robh+dt, joel, linux-arm-kernel Hello, This is v2 of the ASPEED SD controller driver. v2 primarily addresses Rob's comments on the bindings in v1. The v1 series can be found here: https://lists.ozlabs.org/pipermail/linux-aspeed/2019-July/001988.html Please review! Andrew Andrew Jeffery (2): dt-bindings: mmc: Document Aspeed SD controller mmc: Add support for the ASPEED SD controller .../devicetree/bindings/mmc/aspeed,sdhci.yaml | 90 +++++ drivers/mmc/host/Kconfig | 12 + drivers/mmc/host/Makefile | 1 + drivers/mmc/host/sdhci-of-aspeed.c | 326 ++++++++++++++++++ 4 files changed, 429 insertions(+) create mode 100644 Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml create mode 100644 drivers/mmc/host/sdhci-of-aspeed.c -- 2.20.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 1/2] dt-bindings: mmc: Document Aspeed SD controller 2019-07-12 3:32 [PATCH v2 0/2] mmc: Add support for the ASPEED SD controller Andrew Jeffery @ 2019-07-12 3:32 ` Andrew Jeffery 2019-07-12 13:03 ` Rob Herring ` (2 more replies) 2019-07-12 3:32 ` [PATCH v2 2/2] mmc: Add support for the ASPEED " Andrew Jeffery 1 sibling, 3 replies; 17+ messages in thread From: Andrew Jeffery @ 2019-07-12 3:32 UTC (permalink / raw) To: linux-mmc Cc: mark.rutland, devicetree, ulf.hansson, linux-aspeed, Andrew Jeffery, ryanchen.aspeed, adrian.hunter, linux-kernel, robh+dt, joel, linux-arm-kernel The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the SDIO Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit data bus if only a single slot is enabled. Signed-off-by: Andrew Jeffery <andrew@aj.id.au> --- In v2: * Rename to aspeed,sdhci.yaml * Rename sd-controller compatible * Add `maxItems: 1` for reg properties * Move sdhci subnode description to patternProperties * Drop sdhci compatible requirement * #address-cells and #size-cells are required * Prevent additional properties * Implement explicit ranges in example * Remove slot property .../devicetree/bindings/mmc/aspeed,sdhci.yaml | 90 +++++++++++++++++++ 1 file changed, 90 insertions(+) create mode 100644 Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml new file mode 100644 index 000000000000..67a691c3348c --- /dev/null +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml @@ -0,0 +1,90 @@ +# SPDX-License-Identifier: GPL-2.0-or-later +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/mmc/aspeed,sdhci.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: ASPEED SD/SDIO/eMMC Controller + +maintainers: + - Andrew Jeffery <andrew@aj.id.au> + - Ryan Chen <ryanchen.aspeed@gmail.com> + +description: |+ + The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the SDIO + Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit data bus if + only a single slot is enabled. + + The two slots are supported by a common configuration area. As the SDHCIs for + the slots are dependent on the common configuration area, they are described + as child nodes. + +properties: + compatible: + enum: [ aspeed,ast2400-sd-controller, aspeed,ast2500-sd-controller ] + reg: + maxItems: 1 + description: Common configuration registers + ranges: true + clocks: + maxItems: 1 + description: The SD/SDIO controller clock gate + +patternProperties: + "^sdhci@[0-9a-f]+$": + type: object + properties: + compatible: + enum: [ aspeed,ast2400-sdhci, aspeed,ast2500-sdhci ] + reg: + maxItems: 1 + description: The SDHCI registers + clocks: + maxItems: 1 + description: The SD bus clock + interrupts: + maxItems: 1 + description: The SD interrupt shared between both slots + required: + - compatible + - reg + - clocks + - interrupts + +additionalProperties: false + +required: + - compatible + - reg + - "#address-cells" + - "#size-cells" + - ranges + - clocks + +examples: + - | + #include <dt-bindings/clock/aspeed-clock.h> + sdc@1e740000 { + compatible = "aspeed,ast2500-sd-controller"; + reg = <0x1e740000 0x100>; + #address-cells = <1>; + #size-cells = <1>; + ranges = <0 0x1e740000 0x10000>; + clocks = <&syscon ASPEED_CLK_GATE_SDCLK>; + + sdhci0: sdhci@100 { + compatible = "aspeed,ast2500-sdhci"; + reg = <0x100 0x100>; + interrupts = <26>; + sdhci,auto-cmd12; + clocks = <&syscon ASPEED_CLK_SDIO>; + }; + + sdhci1: sdhci@200 { + compatible = "aspeed,ast2500-sdhci"; + reg = <0x200 0x100>; + interrupts = <26>; + sdhci,auto-cmd12; + clocks = <&syscon ASPEED_CLK_SDIO>; + }; + }; -- 2.20.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: mmc: Document Aspeed SD controller 2019-07-12 3:32 ` [PATCH v2 1/2] dt-bindings: mmc: Document Aspeed " Andrew Jeffery @ 2019-07-12 13:03 ` Rob Herring 2019-07-12 13:10 ` Maxime Ripard 2019-07-15 22:16 ` Rob Herring 2 siblings, 0 replies; 17+ messages in thread From: Rob Herring @ 2019-07-12 13:03 UTC (permalink / raw) To: Andrew Jeffery Cc: Mark Rutland, devicetree, Ulf Hansson, linux-aspeed, Ryan Chen, linux-mmc, Adrian Hunter, linux-kernel, Joel Stanley, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE On Thu, Jul 11, 2019 at 9:32 PM Andrew Jeffery <andrew@aj.id.au> wrote: > > The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the > SDIO Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit > data bus if only a single slot is enabled. > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > --- > In v2: > > * Rename to aspeed,sdhci.yaml > * Rename sd-controller compatible > * Add `maxItems: 1` for reg properties > * Move sdhci subnode description to patternProperties > * Drop sdhci compatible requirement > * #address-cells and #size-cells are required > * Prevent additional properties > * Implement explicit ranges in example > * Remove slot property > > .../devicetree/bindings/mmc/aspeed,sdhci.yaml | 90 +++++++++++++++++++ > 1 file changed, 90 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml Reviewed-by: Rob Herring <robh@kernel.org> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: mmc: Document Aspeed SD controller 2019-07-12 3:32 ` [PATCH v2 1/2] dt-bindings: mmc: Document Aspeed " Andrew Jeffery 2019-07-12 13:03 ` Rob Herring @ 2019-07-12 13:10 ` Maxime Ripard 2019-07-15 2:30 ` Andrew Jeffery 2019-07-15 22:16 ` Rob Herring 2 siblings, 1 reply; 17+ messages in thread From: Maxime Ripard @ 2019-07-12 13:10 UTC (permalink / raw) To: Andrew Jeffery Cc: mark.rutland, devicetree, ulf.hansson, linux-aspeed, ryanchen.aspeed, linux-mmc, adrian.hunter, linux-kernel, robh+dt, joel, linux-arm-kernel [-- Attachment #1.1: Type: text/plain, Size: 3428 bytes --] Hi, On Fri, Jul 12, 2019 at 01:02:13PM +0930, Andrew Jeffery wrote: > The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the > SDIO Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit > data bus if only a single slot is enabled. > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > --- > In v2: > > * Rename to aspeed,sdhci.yaml > * Rename sd-controller compatible > * Add `maxItems: 1` for reg properties > * Move sdhci subnode description to patternProperties > * Drop sdhci compatible requirement > * #address-cells and #size-cells are required > * Prevent additional properties > * Implement explicit ranges in example > * Remove slot property > > .../devicetree/bindings/mmc/aspeed,sdhci.yaml | 90 +++++++++++++++++++ > 1 file changed, 90 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml > > diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml > new file mode 100644 > index 000000000000..67a691c3348c > --- /dev/null > +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml > @@ -0,0 +1,90 @@ > +# SPDX-License-Identifier: GPL-2.0-or-later > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/mmc/aspeed,sdhci.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: ASPEED SD/SDIO/eMMC Controller > + > +maintainers: > + - Andrew Jeffery <andrew@aj.id.au> > + - Ryan Chen <ryanchen.aspeed@gmail.com> > + > +description: |+ > + The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the SDIO > + Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit data bus if > + only a single slot is enabled. > + > + The two slots are supported by a common configuration area. As the SDHCIs for > + the slots are dependent on the common configuration area, they are described > + as child nodes. > + > +properties: > + compatible: > + enum: [ aspeed,ast2400-sd-controller, aspeed,ast2500-sd-controller ] > + reg: > + maxItems: 1 > + description: Common configuration registers > + ranges: true > + clocks: > + maxItems: 1 > + description: The SD/SDIO controller clock gate #address-cells and #size-cells have not been described here. > +patternProperties: > + "^sdhci@[0-9a-f]+$": > + type: object > + properties: > + compatible: > + enum: [ aspeed,ast2400-sdhci, aspeed,ast2500-sdhci ] > + reg: > + maxItems: 1 > + description: The SDHCI registers > + clocks: > + maxItems: 1 > + description: The SD bus clock > + interrupts: > + maxItems: 1 > + description: The SD interrupt shared between both slots > + required: > + - compatible > + - reg > + - clocks > + - interrupts > + > +additionalProperties: false But that means that it will generate a warning in your DT if you ever use them. > +required: > + - compatible > + - reg > + - "#address-cells" > + - "#size-cells" > + - ranges > + - clocks > + > +examples: > + - | > + #include <dt-bindings/clock/aspeed-clock.h> > + sdc@1e740000 { > + compatible = "aspeed,ast2500-sd-controller"; > + reg = <0x1e740000 0x100>; > + #address-cells = <1>; > + #size-cells = <1>; Starting with your example. Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: mmc: Document Aspeed SD controller 2019-07-12 13:10 ` Maxime Ripard @ 2019-07-15 2:30 ` Andrew Jeffery 2019-07-15 13:26 ` Rob Herring 0 siblings, 1 reply; 17+ messages in thread From: Andrew Jeffery @ 2019-07-15 2:30 UTC (permalink / raw) To: Maxime Ripard Cc: mark.rutland, devicetree, Ulf Hansson, linux-aspeed, Ryan Chen, linux-mmc, Adrian Hunter, linux-kernel, Rob Herring, Joel Stanley, linux-arm-kernel On Fri, 12 Jul 2019, at 22:41, Maxime Ripard wrote: > Hi, > > On Fri, Jul 12, 2019 at 01:02:13PM +0930, Andrew Jeffery wrote: > > The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the > > SDIO Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit > > data bus if only a single slot is enabled. > > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > > --- > > In v2: > > > > * Rename to aspeed,sdhci.yaml > > * Rename sd-controller compatible > > * Add `maxItems: 1` for reg properties > > * Move sdhci subnode description to patternProperties > > * Drop sdhci compatible requirement > > * #address-cells and #size-cells are required > > * Prevent additional properties > > * Implement explicit ranges in example > > * Remove slot property > > > > .../devicetree/bindings/mmc/aspeed,sdhci.yaml | 90 +++++++++++++++++++ > > 1 file changed, 90 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml > > > > diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml > > new file mode 100644 > > index 000000000000..67a691c3348c > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml > > @@ -0,0 +1,90 @@ > > +# SPDX-License-Identifier: GPL-2.0-or-later > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/mmc/aspeed,sdhci.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: ASPEED SD/SDIO/eMMC Controller > > + > > +maintainers: > > + - Andrew Jeffery <andrew@aj.id.au> > > + - Ryan Chen <ryanchen.aspeed@gmail.com> > > + > > +description: |+ > > + The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the SDIO > > + Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit data bus if > > + only a single slot is enabled. > > + > > + The two slots are supported by a common configuration area. As the SDHCIs for > > + the slots are dependent on the common configuration area, they are described > > + as child nodes. > > + > > +properties: > > + compatible: > > + enum: [ aspeed,ast2400-sd-controller, aspeed,ast2500-sd-controller ] > > + reg: > > + maxItems: 1 > > + description: Common configuration registers > > + ranges: true > > + clocks: > > + maxItems: 1 > > + description: The SD/SDIO controller clock gate > > #address-cells and #size-cells have not been described here. > > > +patternProperties: > > + "^sdhci@[0-9a-f]+$": > > + type: object > > + properties: > > + compatible: > > + enum: [ aspeed,ast2400-sdhci, aspeed,ast2500-sdhci ] > > + reg: > > + maxItems: 1 > > + description: The SDHCI registers > > + clocks: > > + maxItems: 1 > > + description: The SD bus clock > > + interrupts: > > + maxItems: 1 > > + description: The SD interrupt shared between both slots > > + required: > > + - compatible > > + - reg > > + - clocks > > + - interrupts > > + > > +additionalProperties: false > > But that means that it will generate a warning in your DT if you ever > use them. > > > +required: > > + - compatible > > + - reg > > + - "#address-cells" > > + - "#size-cells" > > + - ranges > > + - clocks > > + > > +examples: > > + - | > > + #include <dt-bindings/clock/aspeed-clock.h> > > + sdc@1e740000 { > > + compatible = "aspeed,ast2500-sd-controller"; > > + reg = <0x1e740000 0x100>; > > + #address-cells = <1>; > > + #size-cells = <1>; > > Starting with your example. Heh, right. Thanks. I was inspecting the output of the `dt_binding_check` and `dtbs_check` make targets, though maybe I overlooked this. The aspeed dtsis do generate a quite a number of warnings which make it hard to parse, so I'm going to send a series to clean that up too. Andrew > > Maxime > > -- > Maxime Ripard, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com > > Attachments: > * signature.asc _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: mmc: Document Aspeed SD controller 2019-07-15 2:30 ` Andrew Jeffery @ 2019-07-15 13:26 ` Rob Herring 0 siblings, 0 replies; 17+ messages in thread From: Rob Herring @ 2019-07-15 13:26 UTC (permalink / raw) To: Andrew Jeffery Cc: Mark Rutland, devicetree, Ulf Hansson, linux-aspeed, Maxime Ripard, Ryan Chen, linux-mmc, Adrian Hunter, linux-kernel, Joel Stanley, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE On Sun, Jul 14, 2019 at 8:30 PM Andrew Jeffery <andrew@aj.id.au> wrote: > > > > On Fri, 12 Jul 2019, at 22:41, Maxime Ripard wrote: > > Hi, > > > > On Fri, Jul 12, 2019 at 01:02:13PM +0930, Andrew Jeffery wrote: > > > The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the > > > SDIO Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit > > > data bus if only a single slot is enabled. > > > > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > > > --- > > > In v2: > > > > > > * Rename to aspeed,sdhci.yaml > > > * Rename sd-controller compatible > > > * Add `maxItems: 1` for reg properties > > > * Move sdhci subnode description to patternProperties > > > * Drop sdhci compatible requirement > > > * #address-cells and #size-cells are required > > > * Prevent additional properties > > > * Implement explicit ranges in example > > > * Remove slot property > > > > > > .../devicetree/bindings/mmc/aspeed,sdhci.yaml | 90 +++++++++++++++++++ > > > 1 file changed, 90 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml > > > new file mode 100644 > > > index 000000000000..67a691c3348c > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml > > > @@ -0,0 +1,90 @@ > > > +# SPDX-License-Identifier: GPL-2.0-or-later > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/mmc/aspeed,sdhci.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: ASPEED SD/SDIO/eMMC Controller > > > + > > > +maintainers: > > > + - Andrew Jeffery <andrew@aj.id.au> > > > + - Ryan Chen <ryanchen.aspeed@gmail.com> > > > + > > > +description: |+ > > > + The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the SDIO > > > + Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit data bus if > > > + only a single slot is enabled. > > > + > > > + The two slots are supported by a common configuration area. As the SDHCIs for > > > + the slots are dependent on the common configuration area, they are described > > > + as child nodes. > > > + > > > +properties: > > > + compatible: > > > + enum: [ aspeed,ast2400-sd-controller, aspeed,ast2500-sd-controller ] > > > + reg: > > > + maxItems: 1 > > > + description: Common configuration registers > > > + ranges: true > > > + clocks: > > > + maxItems: 1 > > > + description: The SD/SDIO controller clock gate > > > > #address-cells and #size-cells have not been described here. > > > > > +patternProperties: > > > + "^sdhci@[0-9a-f]+$": > > > + type: object > > > + properties: > > > + compatible: > > > + enum: [ aspeed,ast2400-sdhci, aspeed,ast2500-sdhci ] > > > + reg: > > > + maxItems: 1 > > > + description: The SDHCI registers > > > + clocks: > > > + maxItems: 1 > > > + description: The SD bus clock > > > + interrupts: > > > + maxItems: 1 > > > + description: The SD interrupt shared between both slots > > > + required: > > > + - compatible > > > + - reg > > > + - clocks > > > + - interrupts > > > + > > > +additionalProperties: false > > > > But that means that it will generate a warning in your DT if you ever > > use them. > > > > > +required: > > > + - compatible > > > + - reg > > > + - "#address-cells" > > > + - "#size-cells" > > > + - ranges > > > + - clocks > > > + > > > +examples: > > > + - | > > > + #include <dt-bindings/clock/aspeed-clock.h> > > > + sdc@1e740000 { > > > + compatible = "aspeed,ast2500-sd-controller"; > > > + reg = <0x1e740000 0x100>; > > > + #address-cells = <1>; > > > + #size-cells = <1>; > > > > Starting with your example. > > Heh, right. Thanks. I was inspecting the output of the `dt_binding_check` and > `dtbs_check` make targets, though maybe I overlooked this. The aspeed dtsis > do generate a quite a number of warnings which make it hard to parse, so I'm > going to send a series to clean that up too. FYI, This will run checks with only the schema file you specify: make dtbs_check DT_SCHEMA_FILES=path/to/schema/file Rob _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: mmc: Document Aspeed SD controller 2019-07-12 3:32 ` [PATCH v2 1/2] dt-bindings: mmc: Document Aspeed " Andrew Jeffery 2019-07-12 13:03 ` Rob Herring 2019-07-12 13:10 ` Maxime Ripard @ 2019-07-15 22:16 ` Rob Herring 2019-07-16 0:36 ` Andrew Jeffery 2 siblings, 1 reply; 17+ messages in thread From: Rob Herring @ 2019-07-15 22:16 UTC (permalink / raw) To: Andrew Jeffery Cc: Mark Rutland, devicetree, Ulf Hansson, linux-aspeed, Ryan Chen, linux-mmc, Adrian Hunter, linux-kernel, Joel Stanley, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE On Thu, Jul 11, 2019 at 9:32 PM Andrew Jeffery <andrew@aj.id.au> wrote: > > The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the > SDIO Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit > data bus if only a single slot is enabled. > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > --- > In v2: > > * Rename to aspeed,sdhci.yaml > * Rename sd-controller compatible > * Add `maxItems: 1` for reg properties > * Move sdhci subnode description to patternProperties > * Drop sdhci compatible requirement > * #address-cells and #size-cells are required > * Prevent additional properties > * Implement explicit ranges in example > * Remove slot property > > .../devicetree/bindings/mmc/aspeed,sdhci.yaml | 90 +++++++++++++++++++ > 1 file changed, 90 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml > > diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml > new file mode 100644 > index 000000000000..67a691c3348c > --- /dev/null > +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml > @@ -0,0 +1,90 @@ > +# SPDX-License-Identifier: GPL-2.0-or-later > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/mmc/aspeed,sdhci.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: ASPEED SD/SDIO/eMMC Controller > + > +maintainers: > + - Andrew Jeffery <andrew@aj.id.au> > + - Ryan Chen <ryanchen.aspeed@gmail.com> > + > +description: |+ > + The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the SDIO > + Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit data bus if > + only a single slot is enabled. > + > + The two slots are supported by a common configuration area. As the SDHCIs for > + the slots are dependent on the common configuration area, they are described > + as child nodes. > + > +properties: > + compatible: > + enum: [ aspeed,ast2400-sd-controller, aspeed,ast2500-sd-controller ] This is actually a list of 4 strings. Please reformat to 1 per line. Rob _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: mmc: Document Aspeed SD controller 2019-07-15 22:16 ` Rob Herring @ 2019-07-16 0:36 ` Andrew Jeffery 2019-07-16 14:57 ` Rob Herring 0 siblings, 1 reply; 17+ messages in thread From: Andrew Jeffery @ 2019-07-16 0:36 UTC (permalink / raw) To: Rob Herring Cc: Mark Rutland, devicetree, Ulf Hansson, linux-aspeed, Ryan Chen, linux-mmc, Adrian Hunter, linux-kernel, Joel Stanley, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE On Tue, 16 Jul 2019, at 07:47, Rob Herring wrote: > On Thu, Jul 11, 2019 at 9:32 PM Andrew Jeffery <andrew@aj.id.au> wrote: > > > > The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the > > SDIO Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit > > data bus if only a single slot is enabled. > > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > > --- > > In v2: > > > > * Rename to aspeed,sdhci.yaml > > * Rename sd-controller compatible > > * Add `maxItems: 1` for reg properties > > * Move sdhci subnode description to patternProperties > > * Drop sdhci compatible requirement > > * #address-cells and #size-cells are required > > * Prevent additional properties > > * Implement explicit ranges in example > > * Remove slot property > > > > .../devicetree/bindings/mmc/aspeed,sdhci.yaml | 90 +++++++++++++++++++ > > 1 file changed, 90 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml > > > > diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml > > new file mode 100644 > > index 000000000000..67a691c3348c > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml > > @@ -0,0 +1,90 @@ > > +# SPDX-License-Identifier: GPL-2.0-or-later > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/mmc/aspeed,sdhci.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: ASPEED SD/SDIO/eMMC Controller > > + > > +maintainers: > > + - Andrew Jeffery <andrew@aj.id.au> > > + - Ryan Chen <ryanchen.aspeed@gmail.com> > > + > > +description: |+ > > + The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the SDIO > > + Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit data bus if > > + only a single slot is enabled. > > + > > + The two slots are supported by a common configuration area. As the SDHCIs for > > + the slots are dependent on the common configuration area, they are described > > + as child nodes. > > + > > +properties: > > + compatible: > > + enum: [ aspeed,ast2400-sd-controller, aspeed,ast2500-sd-controller ] > > This is actually a list of 4 strings. Please reformat to 1 per line. On reflection that's obvious, but also a somewhat subtle interaction with the preference for no quotes (the obvious caveat being "except where required"). Thanks for pointing it out. I have been running `make dt_binding_check` and `make dtbs_check` over these, looks like I need to up my game a bit though. Do you do additional things in your workflow? Andrew _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: mmc: Document Aspeed SD controller 2019-07-16 0:36 ` Andrew Jeffery @ 2019-07-16 14:57 ` Rob Herring 2019-07-17 3:57 ` Andrew Jeffery 0 siblings, 1 reply; 17+ messages in thread From: Rob Herring @ 2019-07-16 14:57 UTC (permalink / raw) To: Andrew Jeffery Cc: Mark Rutland, devicetree, Ulf Hansson, linux-aspeed, Ryan Chen, linux-mmc, Adrian Hunter, linux-kernel, Joel Stanley, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE On Mon, Jul 15, 2019 at 6:36 PM Andrew Jeffery <andrew@aj.id.au> wrote: > > > > On Tue, 16 Jul 2019, at 07:47, Rob Herring wrote: > > On Thu, Jul 11, 2019 at 9:32 PM Andrew Jeffery <andrew@aj.id.au> wrote: > > > > > > The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the > > > SDIO Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit > > > data bus if only a single slot is enabled. > > > > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > > > --- > > > In v2: > > > > > > * Rename to aspeed,sdhci.yaml > > > * Rename sd-controller compatible > > > * Add `maxItems: 1` for reg properties > > > * Move sdhci subnode description to patternProperties > > > * Drop sdhci compatible requirement > > > * #address-cells and #size-cells are required > > > * Prevent additional properties > > > * Implement explicit ranges in example > > > * Remove slot property > > > > > > .../devicetree/bindings/mmc/aspeed,sdhci.yaml | 90 +++++++++++++++++++ > > > 1 file changed, 90 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml > > > new file mode 100644 > > > index 000000000000..67a691c3348c > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml > > > @@ -0,0 +1,90 @@ > > > +# SPDX-License-Identifier: GPL-2.0-or-later > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/mmc/aspeed,sdhci.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: ASPEED SD/SDIO/eMMC Controller > > > + > > > +maintainers: > > > + - Andrew Jeffery <andrew@aj.id.au> > > > + - Ryan Chen <ryanchen.aspeed@gmail.com> > > > + > > > +description: |+ > > > + The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the SDIO > > > + Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit data bus if > > > + only a single slot is enabled. > > > + > > > + The two slots are supported by a common configuration area. As the SDHCIs for > > > + the slots are dependent on the common configuration area, they are described > > > + as child nodes. > > > + > > > +properties: > > > + compatible: > > > + enum: [ aspeed,ast2400-sd-controller, aspeed,ast2500-sd-controller ] > > > > This is actually a list of 4 strings. Please reformat to 1 per line. > > On reflection that's obvious, but also a somewhat subtle interaction with the > preference for no quotes (the obvious caveat being "except where required"). It wasn't something I'd run into before. I'm working on a check, but unfortunately we can only check for quotes not needed and can't check for missing quotes. > Thanks for pointing it out. > > I have been running `make dt_binding_check` and `make dtbs_check` over > these, looks like I need to up my game a bit though. Do you do additional things > in your workflow? That should have thrown the warnings. If you aren't seeing those, do you have dtschema package installed (see Documentation/devicetree/writing-schema.md)? Or it could be erroring out on something else first. There's a few breakages that I'm trying to fix. Rob _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: mmc: Document Aspeed SD controller 2019-07-16 14:57 ` Rob Herring @ 2019-07-17 3:57 ` Andrew Jeffery 2019-07-17 13:43 ` Rob Herring 0 siblings, 1 reply; 17+ messages in thread From: Andrew Jeffery @ 2019-07-17 3:57 UTC (permalink / raw) To: Rob Herring Cc: Mark Rutland, devicetree, Ulf Hansson, linux-aspeed, Ryan Chen, linux-mmc, Adrian Hunter, linux-kernel, Joel Stanley, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE On Wed, 17 Jul 2019, at 00:27, Rob Herring wrote: > On Mon, Jul 15, 2019 at 6:36 PM Andrew Jeffery <andrew@aj.id.au> wrote: > > > > > > > > On Tue, 16 Jul 2019, at 07:47, Rob Herring wrote: > > > On Thu, Jul 11, 2019 at 9:32 PM Andrew Jeffery <andrew@aj.id.au> wrote: > > > > > > > > The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the > > > > SDIO Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit > > > > data bus if only a single slot is enabled. > > > > > > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > > > > --- > > > > In v2: > > > > > > > > * Rename to aspeed,sdhci.yaml > > > > * Rename sd-controller compatible > > > > * Add `maxItems: 1` for reg properties > > > > * Move sdhci subnode description to patternProperties > > > > * Drop sdhci compatible requirement > > > > * #address-cells and #size-cells are required > > > > * Prevent additional properties > > > > * Implement explicit ranges in example > > > > * Remove slot property > > > > > > > > .../devicetree/bindings/mmc/aspeed,sdhci.yaml | 90 +++++++++++++++++++ > > > > 1 file changed, 90 insertions(+) > > > > create mode 100644 Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml > > > > > > > > diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml > > > > new file mode 100644 > > > > index 000000000000..67a691c3348c > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml > > > > @@ -0,0 +1,90 @@ > > > > +# SPDX-License-Identifier: GPL-2.0-or-later > > > > +%YAML 1.2 > > > > +--- > > > > +$id: http://devicetree.org/schemas/mmc/aspeed,sdhci.yaml# > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > + > > > > +title: ASPEED SD/SDIO/eMMC Controller > > > > + > > > > +maintainers: > > > > + - Andrew Jeffery <andrew@aj.id.au> > > > > + - Ryan Chen <ryanchen.aspeed@gmail.com> > > > > + > > > > +description: |+ > > > > + The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the SDIO > > > > + Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit data bus if > > > > + only a single slot is enabled. > > > > + > > > > + The two slots are supported by a common configuration area. As the SDHCIs for > > > > + the slots are dependent on the common configuration area, they are described > > > > + as child nodes. > > > > + > > > > +properties: > > > > + compatible: > > > > + enum: [ aspeed,ast2400-sd-controller, aspeed,ast2500-sd-controller ] > > > > > > This is actually a list of 4 strings. Please reformat to 1 per line. > > > > On reflection that's obvious, but also a somewhat subtle interaction with the > > preference for no quotes (the obvious caveat being "except where required"). > > It wasn't something I'd run into before. I'm working on a check, but > unfortunately we can only check for quotes not needed and can't check > for missing quotes. > > > Thanks for pointing it out. > > > > I have been running `make dt_binding_check` and `make dtbs_check` over > > these, looks like I need to up my game a bit though. Do you do additional things > > in your workflow? > > That should have thrown the warnings. If you aren't seeing those, do > you have dtschema package installed (see > Documentation/devicetree/writing-schema.md)? I do have it installed, but as mentioned previously there's a fair few warnings emitted currently by the Aspeed devicetrees, so it might have got lost in the noise. I've started to clean that up, though probably need some direction there too. Separately I'm currently trying to track down an issue where I get errors on the Aspeed dts cpu nodes about failing to match the riscv CPU compatibles, it seems dt-validate isn't finding the ARM CPU compatible strings. It feels more annoying to track down that I'd like. > Or it could be erroring > out on something else first. There's a few breakages that I'm trying > to fix. Okay. I'll keep an eye on the dt-schema repo. Cheers, Andrew _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: mmc: Document Aspeed SD controller 2019-07-17 3:57 ` Andrew Jeffery @ 2019-07-17 13:43 ` Rob Herring 2019-07-18 1:49 ` Andrew Jeffery 0 siblings, 1 reply; 17+ messages in thread From: Rob Herring @ 2019-07-17 13:43 UTC (permalink / raw) To: Andrew Jeffery Cc: Mark Rutland, devicetree, Ulf Hansson, linux-aspeed, Ryan Chen, linux-mmc, Adrian Hunter, linux-kernel, Joel Stanley, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE On Tue, Jul 16, 2019 at 9:58 PM Andrew Jeffery <andrew@aj.id.au> wrote: > > > > On Wed, 17 Jul 2019, at 00:27, Rob Herring wrote: > > On Mon, Jul 15, 2019 at 6:36 PM Andrew Jeffery <andrew@aj.id.au> wrote: > > > > > > > > > > > > On Tue, 16 Jul 2019, at 07:47, Rob Herring wrote: > > > > On Thu, Jul 11, 2019 at 9:32 PM Andrew Jeffery <andrew@aj.id.au> wrote: > > > > > > > > > > The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the > > > > > SDIO Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit > > > > > data bus if only a single slot is enabled. > > > > > > > > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > > > > > --- > > > > > In v2: > > > > > > > > > > * Rename to aspeed,sdhci.yaml > > > > > * Rename sd-controller compatible > > > > > * Add `maxItems: 1` for reg properties > > > > > * Move sdhci subnode description to patternProperties > > > > > * Drop sdhci compatible requirement > > > > > * #address-cells and #size-cells are required > > > > > * Prevent additional properties > > > > > * Implement explicit ranges in example > > > > > * Remove slot property > > > > > > > > > > .../devicetree/bindings/mmc/aspeed,sdhci.yaml | 90 +++++++++++++++++++ > > > > > 1 file changed, 90 insertions(+) > > > > > create mode 100644 Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml > > > > > new file mode 100644 > > > > > index 000000000000..67a691c3348c > > > > > --- /dev/null > > > > > +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml > > > > > @@ -0,0 +1,90 @@ > > > > > +# SPDX-License-Identifier: GPL-2.0-or-later > > > > > +%YAML 1.2 > > > > > +--- > > > > > +$id: http://devicetree.org/schemas/mmc/aspeed,sdhci.yaml# > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > > + > > > > > +title: ASPEED SD/SDIO/eMMC Controller > > > > > + > > > > > +maintainers: > > > > > + - Andrew Jeffery <andrew@aj.id.au> > > > > > + - Ryan Chen <ryanchen.aspeed@gmail.com> > > > > > + > > > > > +description: |+ > > > > > + The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the SDIO > > > > > + Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit data bus if > > > > > + only a single slot is enabled. > > > > > + > > > > > + The two slots are supported by a common configuration area. As the SDHCIs for > > > > > + the slots are dependent on the common configuration area, they are described > > > > > + as child nodes. > > > > > + > > > > > +properties: > > > > > + compatible: > > > > > + enum: [ aspeed,ast2400-sd-controller, aspeed,ast2500-sd-controller ] > > > > > > > > This is actually a list of 4 strings. Please reformat to 1 per line. > > > > > > On reflection that's obvious, but also a somewhat subtle interaction with the > > > preference for no quotes (the obvious caveat being "except where required"). > > > > It wasn't something I'd run into before. I'm working on a check, but > > unfortunately we can only check for quotes not needed and can't check > > for missing quotes. > > > > > Thanks for pointing it out. > > > > > > I have been running `make dt_binding_check` and `make dtbs_check` over > > > these, looks like I need to up my game a bit though. Do you do additional things > > > in your workflow? > > > > That should have thrown the warnings. If you aren't seeing those, do > > you have dtschema package installed (see > > Documentation/devicetree/writing-schema.md)? > > I do have it installed, but as mentioned previously there's a fair few > warnings emitted currently by the Aspeed devicetrees, so it might have > got lost in the noise. I've started to clean that up, though probably need > some direction there too. > > Separately I'm currently trying to track down an issue where I get errors > on the Aspeed dts cpu nodes about failing to match the riscv CPU > compatibles, it seems dt-validate isn't finding the ARM CPU compatible > strings. It feels more annoying to track down that I'd like. There's a fix in today's linux-next for that and it should be in Linus' tree in a few days. Rob _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: mmc: Document Aspeed SD controller 2019-07-17 13:43 ` Rob Herring @ 2019-07-18 1:49 ` Andrew Jeffery 0 siblings, 0 replies; 17+ messages in thread From: Andrew Jeffery @ 2019-07-18 1:49 UTC (permalink / raw) To: Rob Herring Cc: Mark Rutland, devicetree, Ulf Hansson, linux-aspeed, Ryan Chen, linux-mmc, Adrian Hunter, linux-kernel, Joel Stanley, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE On Wed, 17 Jul 2019, at 23:13, Rob Herring wrote: > On Tue, Jul 16, 2019 at 9:58 PM Andrew Jeffery <andrew@aj.id.au> wrote: > > > > > > > > On Wed, 17 Jul 2019, at 00:27, Rob Herring wrote: > > > On Mon, Jul 15, 2019 at 6:36 PM Andrew Jeffery <andrew@aj.id.au> wrote: > > > > > > > > > > > > > > > > On Tue, 16 Jul 2019, at 07:47, Rob Herring wrote: > > > > > On Thu, Jul 11, 2019 at 9:32 PM Andrew Jeffery <andrew@aj.id.au> wrote: > > > > > > > > > > > > The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the > > > > > > SDIO Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit > > > > > > data bus if only a single slot is enabled. > > > > > > > > > > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > > > > > > --- > > > > > > In v2: > > > > > > > > > > > > * Rename to aspeed,sdhci.yaml > > > > > > * Rename sd-controller compatible > > > > > > * Add `maxItems: 1` for reg properties > > > > > > * Move sdhci subnode description to patternProperties > > > > > > * Drop sdhci compatible requirement > > > > > > * #address-cells and #size-cells are required > > > > > > * Prevent additional properties > > > > > > * Implement explicit ranges in example > > > > > > * Remove slot property > > > > > > > > > > > > .../devicetree/bindings/mmc/aspeed,sdhci.yaml | 90 +++++++++++++++++++ > > > > > > 1 file changed, 90 insertions(+) > > > > > > create mode 100644 Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml > > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml > > > > > > new file mode 100644 > > > > > > index 000000000000..67a691c3348c > > > > > > --- /dev/null > > > > > > +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml > > > > > > @@ -0,0 +1,90 @@ > > > > > > +# SPDX-License-Identifier: GPL-2.0-or-later > > > > > > +%YAML 1.2 > > > > > > +--- > > > > > > +$id: http://devicetree.org/schemas/mmc/aspeed,sdhci.yaml# > > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > > > + > > > > > > +title: ASPEED SD/SDIO/eMMC Controller > > > > > > + > > > > > > +maintainers: > > > > > > + - Andrew Jeffery <andrew@aj.id.au> > > > > > > + - Ryan Chen <ryanchen.aspeed@gmail.com> > > > > > > + > > > > > > +description: |+ > > > > > > + The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the SDIO > > > > > > + Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit data bus if > > > > > > + only a single slot is enabled. > > > > > > + > > > > > > + The two slots are supported by a common configuration area. As the SDHCIs for > > > > > > + the slots are dependent on the common configuration area, they are described > > > > > > + as child nodes. > > > > > > + > > > > > > +properties: > > > > > > + compatible: > > > > > > + enum: [ aspeed,ast2400-sd-controller, aspeed,ast2500-sd-controller ] > > > > > > > > > > This is actually a list of 4 strings. Please reformat to 1 per line. > > > > > > > > On reflection that's obvious, but also a somewhat subtle interaction with the > > > > preference for no quotes (the obvious caveat being "except where required"). > > > > > > It wasn't something I'd run into before. I'm working on a check, but > > > unfortunately we can only check for quotes not needed and can't check > > > for missing quotes. > > > > > > > Thanks for pointing it out. > > > > > > > > I have been running `make dt_binding_check` and `make dtbs_check` over > > > > these, looks like I need to up my game a bit though. Do you do additional things > > > > in your workflow? > > > > > > That should have thrown the warnings. If you aren't seeing those, do > > > you have dtschema package installed (see > > > Documentation/devicetree/writing-schema.md)? > > > > I do have it installed, but as mentioned previously there's a fair few > > warnings emitted currently by the Aspeed devicetrees, so it might have > > got lost in the noise. I've started to clean that up, though probably need > > some direction there too. > > > > Separately I'm currently trying to track down an issue where I get errors > > on the Aspeed dts cpu nodes about failing to match the riscv CPU > > compatibles, it seems dt-validate isn't finding the ARM CPU compatible > > strings. It feels more annoying to track down that I'd like. > > There's a fix in today's linux-next for that and it should be in > Linus' tree in a few days. > Thanks, I'll take a look. Andrew _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 2/2] mmc: Add support for the ASPEED SD controller 2019-07-12 3:32 [PATCH v2 0/2] mmc: Add support for the ASPEED SD controller Andrew Jeffery 2019-07-12 3:32 ` [PATCH v2 1/2] dt-bindings: mmc: Document Aspeed " Andrew Jeffery @ 2019-07-12 3:32 ` Andrew Jeffery 2019-07-25 13:18 ` Adrian Hunter 1 sibling, 1 reply; 17+ messages in thread From: Andrew Jeffery @ 2019-07-12 3:32 UTC (permalink / raw) To: linux-mmc Cc: mark.rutland, devicetree, ulf.hansson, linux-aspeed, Andrew Jeffery, ryanchen.aspeed, adrian.hunter, linux-kernel, robh+dt, joel, linux-arm-kernel Add a minimal driver for ASPEED's SD controller, which exposes two SDHCIs. The ASPEED design implements a common register set for the SDHCIs, and moves some of the standard configuration elements out to this common area (e.g. 8-bit mode, and card detect configuration which is not currently supported). The SD controller has a dedicated hardware interrupt that is shared between the slots. The common register set exposes information on which slot triggered the interrupt; early revisions of the patch introduced an irqchip for the register, but reality is it doesn't behave as an irqchip, and the result fits awkwardly into the irqchip APIs. Instead I've taken the simple approach of using the IRQ as a shared IRQ with some minor performance impact for the second slot. Ryan was the original author of the patch - I've taken his work and massaged it to drop the irqchip support and rework the devicetree integration. The driver has been smoke tested under qemu against a minimal SD controller model and lightly tested on an ast2500-evb. Signed-off-by: Ryan Chen <ryanchen.aspeed@gmail.com> Signed-off-by: Andrew Jeffery <andrew@aj.id.au> --- In v2: * Drop unnecesasry MODULE_DEVICE_TABLE() * Rename sd-controller compatible * Add IBM copyright * Drop unnecesary data assignment in of match table entries * Derive the slot from the SDHCI offset drivers/mmc/host/Kconfig | 12 ++ drivers/mmc/host/Makefile | 1 + drivers/mmc/host/sdhci-of-aspeed.c | 326 +++++++++++++++++++++++++++++ 3 files changed, 339 insertions(+) create mode 100644 drivers/mmc/host/sdhci-of-aspeed.c diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig index 931770f17087..2bb5e1264b3d 100644 --- a/drivers/mmc/host/Kconfig +++ b/drivers/mmc/host/Kconfig @@ -154,6 +154,18 @@ config MMC_SDHCI_OF_ARASAN If unsure, say N. +config MMC_SDHCI_OF_ASPEED + tristate "SDHCI OF support for the ASPEED SDHCI controller" + depends on MMC_SDHCI_PLTFM + depends on OF + help + This selects the ASPEED Secure Digital Host Controller Interface. + + If you have a controller with this interface, say Y or M here. You + also need to enable an appropriate bus interface. + + If unsure, say N. + config MMC_SDHCI_OF_AT91 tristate "SDHCI OF support for the Atmel SDMMC controller" depends on MMC_SDHCI_PLTFM diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile index 73578718f119..390ee162fe71 100644 --- a/drivers/mmc/host/Makefile +++ b/drivers/mmc/host/Makefile @@ -84,6 +84,7 @@ obj-$(CONFIG_MMC_SDHCI_ESDHC_IMX) += sdhci-esdhc-imx.o obj-$(CONFIG_MMC_SDHCI_DOVE) += sdhci-dove.o obj-$(CONFIG_MMC_SDHCI_TEGRA) += sdhci-tegra.o obj-$(CONFIG_MMC_SDHCI_OF_ARASAN) += sdhci-of-arasan.o +obj-$(CONFIG_MMC_SDHCI_OF_ASPEED) += sdhci-of-aspeed.o obj-$(CONFIG_MMC_SDHCI_OF_AT91) += sdhci-of-at91.o obj-$(CONFIG_MMC_SDHCI_OF_ESDHC) += sdhci-of-esdhc.o obj-$(CONFIG_MMC_SDHCI_OF_HLWD) += sdhci-of-hlwd.o diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c new file mode 100644 index 000000000000..9528e43c257d --- /dev/null +++ b/drivers/mmc/host/sdhci-of-aspeed.c @@ -0,0 +1,326 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* Copyright (C) 2019 ASPEED Technology Inc. */ +/* Copyright (C) 2019 IBM Corp. */ + +#include <linux/clk.h> +#include <linux/delay.h> +#include <linux/device.h> +#include <linux/io.h> +#include <linux/mmc/host.h> +#include <linux/module.h> +#include <linux/of_address.h> +#include <linux/of.h> +#include <linux/of_platform.h> +#include <linux/platform_device.h> +#include <linux/spinlock.h> + +#include "sdhci-pltfm.h" + +#define ASPEED_SDC_INFO 0x00 +#define ASPEED_SDC_S1MMC8 BIT(25) +#define ASPEED_SDC_S0MMC8 BIT(24) + +struct aspeed_sdc { + struct clk *clk; + struct resource *res; + + spinlock_t lock; + void __iomem *regs; +}; + +struct aspeed_sdhci { + struct aspeed_sdc *parent; + u32 width_mask; +}; + +static void aspeed_sdc_bus_width(struct aspeed_sdc *sdc, + struct aspeed_sdhci *sdhci, bool bus8) +{ + u32 info; + + /* Set/clear 8 bit mode */ + spin_lock(&sdc->lock); + info = readl(sdc->regs + ASPEED_SDC_INFO); + if (bus8) + info |= sdhci->width_mask; + else + info &= ~sdhci->width_mask; + writel(info, sdc->regs + ASPEED_SDC_INFO); + spin_unlock(&sdc->lock); +} + +static void aspeed_sdhci_set_clock(struct sdhci_host *host, unsigned int clock) +{ + unsigned long timeout; + int div; + u16 clk; + + if (clock == host->clock) + return; + + sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL); + + if (clock == 0) + goto out; + + for (div = 1; div < 256; div *= 2) { + if ((host->max_clk / div) <= clock) + break; + } + div >>= 1; + + clk = div << SDHCI_DIVIDER_SHIFT; + clk |= SDHCI_CLOCK_INT_EN; + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL); + + /* Wait max 20 ms */ + timeout = 20; + while (!((clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL)) + & SDHCI_CLOCK_INT_STABLE)) { + if (timeout == 0) { + pr_err("%s: Internal clock never stabilised.\n", + mmc_hostname(host->mmc)); + return; + } + timeout--; + mdelay(1); + } + + clk |= SDHCI_CLOCK_CARD_EN; + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL); + +out: + host->clock = clock; +} + +static void aspeed_sdhci_set_bus_width(struct sdhci_host *host, int width) +{ + struct sdhci_pltfm_host *pltfm_priv; + struct aspeed_sdhci *aspeed_sdhci; + struct aspeed_sdc *aspeed_sdc; + u8 ctrl; + + pltfm_priv = sdhci_priv(host); + aspeed_sdhci = sdhci_pltfm_priv(pltfm_priv); + aspeed_sdc = aspeed_sdhci->parent; + + /* Set/clear 8-bit mode */ + aspeed_sdc_bus_width(aspeed_sdc, aspeed_sdhci, + width == MMC_BUS_WIDTH_8); + + /* Set/clear 1 or 4 bit mode */ + ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL); + if (width == MMC_BUS_WIDTH_4) + ctrl |= SDHCI_CTRL_4BITBUS; + else + ctrl &= ~SDHCI_CTRL_4BITBUS; + sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL); +} + +static const struct sdhci_ops aspeed_sdhci_ops = { + .set_clock = aspeed_sdhci_set_clock, + .get_max_clock = sdhci_pltfm_clk_get_max_clock, + .set_bus_width = aspeed_sdhci_set_bus_width, + .get_timeout_clock = sdhci_pltfm_clk_get_max_clock, + .reset = sdhci_reset, + .set_uhs_signaling = sdhci_set_uhs_signaling, +}; + +static const struct sdhci_pltfm_data aspeed_sdc_pdata = { + .ops = &aspeed_sdhci_ops, + .quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN, + .quirks2 = SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN, +}; + +static inline int aspeed_sdhci_calculate_slot(struct aspeed_sdhci *dev, + struct resource *res) +{ + resource_size_t delta; + + if (!res || resource_type(res) != IORESOURCE_MEM) + return -EINVAL; + + if (res->start < dev->parent->res->start) + return -EINVAL; + + delta = res->start - dev->parent->res->start; + if (delta & (0x100 - 1)) + return -EINVAL; + + return (delta / 0x100) - 1; +} + +static int aspeed_sdhci_probe(struct platform_device *pdev) +{ + struct sdhci_pltfm_host *pltfm_host; + struct aspeed_sdhci *dev; + struct sdhci_host *host; + struct resource *res; + int slot; + int ret; + + host = sdhci_pltfm_init(pdev, &aspeed_sdc_pdata, sizeof(*dev)); + if (IS_ERR(host)) + return PTR_ERR(host); + + pltfm_host = sdhci_priv(host); + dev = sdhci_pltfm_priv(pltfm_host); + dev->parent = dev_get_drvdata(pdev->dev.parent); + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + slot = aspeed_sdhci_calculate_slot(dev, res); + if (slot < 0) + return slot; + dev_info(&pdev->dev, "Configuring for slot %d\n", slot); + dev->width_mask = !slot ? ASPEED_SDC_S0MMC8 : ASPEED_SDC_S1MMC8; + + sdhci_get_of_property(pdev); + + pltfm_host->clk = devm_clk_get(&pdev->dev, NULL); + if (IS_ERR(pltfm_host->clk)) + return PTR_ERR(pltfm_host->clk); + + ret = clk_prepare_enable(pltfm_host->clk); + if (ret) { + dev_err(&pdev->dev, "Unable to enable SDIO clock\n"); + goto err_pltfm_free; + } + + ret = mmc_of_parse(host->mmc); + if (ret) + goto err_sdhci_add; + + ret = sdhci_add_host(host); + if (ret) + goto err_sdhci_add; + + return 0; + +err_sdhci_add: + clk_disable_unprepare(pltfm_host->clk); +err_pltfm_free: + sdhci_pltfm_free(pdev); + return ret; +} + +static int aspeed_sdhci_remove(struct platform_device *pdev) +{ + struct sdhci_pltfm_host *pltfm_host; + struct sdhci_host *host; + int dead; + + host = platform_get_drvdata(pdev); + pltfm_host = sdhci_priv(host); + + dead = readl(host->ioaddr + SDHCI_INT_STATUS) == 0xffffffff; + + sdhci_remove_host(host, dead); + + clk_disable_unprepare(pltfm_host->clk); + + sdhci_pltfm_free(pdev); + + return 0; +} + +static const struct of_device_id aspeed_sdhci_of_match[] = { + { .compatible = "aspeed,ast2400-sdhci", }, + { .compatible = "aspeed,ast2500-sdhci", }, + { } +}; + +static struct platform_driver aspeed_sdhci_driver = { + .driver = { + .name = "sdhci-aspeed", + .of_match_table = aspeed_sdhci_of_match, + }, + .probe = aspeed_sdhci_probe, + .remove = aspeed_sdhci_remove, +}; + +module_platform_driver(aspeed_sdhci_driver); + +static int aspeed_sdc_probe(struct platform_device *pdev) + +{ + struct device_node *parent, *child; + struct aspeed_sdc *sdc; + int ret; + + sdc = devm_kzalloc(&pdev->dev, sizeof(*sdc), GFP_KERNEL); + if (!sdc) + return -ENOMEM; + + spin_lock_init(&sdc->lock); + + sdc->clk = devm_clk_get(&pdev->dev, NULL); + if (IS_ERR(sdc->clk)) + return PTR_ERR(sdc->clk); + + ret = clk_prepare_enable(sdc->clk); + if (ret) { + dev_err(&pdev->dev, "Unable to enable SDCLK\n"); + return ret; + } + + sdc->res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + sdc->regs = devm_ioremap_resource(&pdev->dev, sdc->res); + if (IS_ERR(sdc->regs)) { + ret = PTR_ERR(sdc->regs); + goto err_clk; + } + + dev_set_drvdata(&pdev->dev, sdc); + + parent = pdev->dev.of_node; + for_each_available_child_of_node(parent, child) { + struct platform_device *cpdev; + + cpdev = of_platform_device_create(child, NULL, &pdev->dev); + if (IS_ERR(cpdev)) { + of_node_put(child); + ret = PTR_ERR(pdev); + goto err_clk; + } + } + + return 0; + +err_clk: + clk_disable_unprepare(sdc->clk); + return ret; +} + +static int aspeed_sdc_remove(struct platform_device *pdev) +{ + struct aspeed_sdc *sdc = dev_get_drvdata(&pdev->dev); + + clk_disable_unprepare(sdc->clk); + + return 0; +} + +static const struct of_device_id aspeed_sdc_of_match[] = { + { .compatible = "aspeed,ast2400-sd-controller", }, + { .compatible = "aspeed,ast2500-sd-controller", }, + { } +}; + +MODULE_DEVICE_TABLE(of, aspeed_sdc_of_match); + +static struct platform_driver aspeed_sdc_driver = { + .driver = { + .name = "sd-controller-aspeed", + .pm = &sdhci_pltfm_pmops, + .of_match_table = aspeed_sdc_of_match, + }, + .probe = aspeed_sdc_probe, + .remove = aspeed_sdc_remove, +}; + +module_platform_driver(aspeed_sdc_driver); + +MODULE_DESCRIPTION("Driver for the ASPEED SD/SDIO/SDHCI Controllers"); +MODULE_AUTHOR("Ryan Chen <ryan_chen@aspeedtech.com>"); +MODULE_AUTHOR("Andrew Jeffery <andrew@aj.id.au>"); +MODULE_LICENSE("GPL v2"); -- 2.20.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] mmc: Add support for the ASPEED SD controller 2019-07-12 3:32 ` [PATCH v2 2/2] mmc: Add support for the ASPEED " Andrew Jeffery @ 2019-07-25 13:18 ` Adrian Hunter 2019-07-26 0:52 ` Andrew Jeffery 0 siblings, 1 reply; 17+ messages in thread From: Adrian Hunter @ 2019-07-25 13:18 UTC (permalink / raw) To: Andrew Jeffery, linux-mmc Cc: mark.rutland, devicetree, ulf.hansson, linux-aspeed, ryanchen.aspeed, linux-kernel, robh+dt, joel, linux-arm-kernel On 12/07/19 6:32 AM, Andrew Jeffery wrote: > Add a minimal driver for ASPEED's SD controller, which exposes two > SDHCIs. > > The ASPEED design implements a common register set for the SDHCIs, and > moves some of the standard configuration elements out to this common > area (e.g. 8-bit mode, and card detect configuration which is not > currently supported). > > The SD controller has a dedicated hardware interrupt that is shared > between the slots. The common register set exposes information on which > slot triggered the interrupt; early revisions of the patch introduced an > irqchip for the register, but reality is it doesn't behave as an > irqchip, and the result fits awkwardly into the irqchip APIs. Instead > I've taken the simple approach of using the IRQ as a shared IRQ with > some minor performance impact for the second slot. > > Ryan was the original author of the patch - I've taken his work and > massaged it to drop the irqchip support and rework the devicetree > integration. The driver has been smoke tested under qemu against a > minimal SD controller model and lightly tested on an ast2500-evb. > > Signed-off-by: Ryan Chen <ryanchen.aspeed@gmail.com> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> Looks fine. Few minor comments below. > --- > In v2: > > * Drop unnecesasry MODULE_DEVICE_TABLE() > * Rename sd-controller compatible > * Add IBM copyright > * Drop unnecesary data assignment in of match table entries > * Derive the slot from the SDHCI offset > > drivers/mmc/host/Kconfig | 12 ++ > drivers/mmc/host/Makefile | 1 + > drivers/mmc/host/sdhci-of-aspeed.c | 326 +++++++++++++++++++++++++++++ > 3 files changed, 339 insertions(+) > create mode 100644 drivers/mmc/host/sdhci-of-aspeed.c > > diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig > index 931770f17087..2bb5e1264b3d 100644 > --- a/drivers/mmc/host/Kconfig > +++ b/drivers/mmc/host/Kconfig > @@ -154,6 +154,18 @@ config MMC_SDHCI_OF_ARASAN > > If unsure, say N. > > +config MMC_SDHCI_OF_ASPEED > + tristate "SDHCI OF support for the ASPEED SDHCI controller" > + depends on MMC_SDHCI_PLTFM > + depends on OF > + help > + This selects the ASPEED Secure Digital Host Controller Interface. > + > + If you have a controller with this interface, say Y or M here. You > + also need to enable an appropriate bus interface. > + > + If unsure, say N. > + > config MMC_SDHCI_OF_AT91 > tristate "SDHCI OF support for the Atmel SDMMC controller" > depends on MMC_SDHCI_PLTFM > diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile > index 73578718f119..390ee162fe71 100644 > --- a/drivers/mmc/host/Makefile > +++ b/drivers/mmc/host/Makefile > @@ -84,6 +84,7 @@ obj-$(CONFIG_MMC_SDHCI_ESDHC_IMX) += sdhci-esdhc-imx.o > obj-$(CONFIG_MMC_SDHCI_DOVE) += sdhci-dove.o > obj-$(CONFIG_MMC_SDHCI_TEGRA) += sdhci-tegra.o > obj-$(CONFIG_MMC_SDHCI_OF_ARASAN) += sdhci-of-arasan.o > +obj-$(CONFIG_MMC_SDHCI_OF_ASPEED) += sdhci-of-aspeed.o > obj-$(CONFIG_MMC_SDHCI_OF_AT91) += sdhci-of-at91.o > obj-$(CONFIG_MMC_SDHCI_OF_ESDHC) += sdhci-of-esdhc.o > obj-$(CONFIG_MMC_SDHCI_OF_HLWD) += sdhci-of-hlwd.o > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c > new file mode 100644 > index 000000000000..9528e43c257d > --- /dev/null > +++ b/drivers/mmc/host/sdhci-of-aspeed.c > @@ -0,0 +1,326 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* Copyright (C) 2019 ASPEED Technology Inc. */ > +/* Copyright (C) 2019 IBM Corp. */ > + > +#include <linux/clk.h> > +#include <linux/delay.h> > +#include <linux/device.h> > +#include <linux/io.h> > +#include <linux/mmc/host.h> > +#include <linux/module.h> > +#include <linux/of_address.h> > +#include <linux/of.h> > +#include <linux/of_platform.h> > +#include <linux/platform_device.h> > +#include <linux/spinlock.h> > + > +#include "sdhci-pltfm.h" > + > +#define ASPEED_SDC_INFO 0x00 > +#define ASPEED_SDC_S1MMC8 BIT(25) > +#define ASPEED_SDC_S0MMC8 BIT(24) > + > +struct aspeed_sdc { > + struct clk *clk; > + struct resource *res; > + > + spinlock_t lock; > + void __iomem *regs; > +}; > + > +struct aspeed_sdhci { > + struct aspeed_sdc *parent; > + u32 width_mask; > +}; > + > +static void aspeed_sdc_bus_width(struct aspeed_sdc *sdc, > + struct aspeed_sdhci *sdhci, bool bus8) The function name threw me at first. I suggest: static void aspeed_sdhci_set_clr_8_bit_mode(struct aspeed_sdhci *aspeed_sdhci, bool bus8) { struct aspeed_sdc *aspeed_sdc = aspeed_sdhci->parent; > +{ > + u32 info; > + > + /* Set/clear 8 bit mode */ > + spin_lock(&sdc->lock); > + info = readl(sdc->regs + ASPEED_SDC_INFO); > + if (bus8) > + info |= sdhci->width_mask; > + else > + info &= ~sdhci->width_mask; > + writel(info, sdc->regs + ASPEED_SDC_INFO); > + spin_unlock(&sdc->lock); > +} > + > +static void aspeed_sdhci_set_clock(struct sdhci_host *host, unsigned int clock) > +{ > + unsigned long timeout; > + int div; > + u16 clk; > + > + if (clock == host->clock) > + return; > + > + sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL); > + > + if (clock == 0) > + goto out; > + > + for (div = 1; div < 256; div *= 2) { > + if ((host->max_clk / div) <= clock) > + break; > + } > + div >>= 1; > + > + clk = div << SDHCI_DIVIDER_SHIFT; Could call sdhci_enable_clk() here. > + clk |= SDHCI_CLOCK_INT_EN; > + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL); > + > + /* Wait max 20 ms */ > + timeout = 20; > + while (!((clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL)) > + & SDHCI_CLOCK_INT_STABLE)) { > + if (timeout == 0) { > + pr_err("%s: Internal clock never stabilised.\n", > + mmc_hostname(host->mmc)); > + return; > + } > + timeout--; > + mdelay(1); > + } > + > + clk |= SDHCI_CLOCK_CARD_EN; > + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL); > + > +out: > + host->clock = clock; > +} > + > +static void aspeed_sdhci_set_bus_width(struct sdhci_host *host, int width) > +{ > + struct sdhci_pltfm_host *pltfm_priv; > + struct aspeed_sdhci *aspeed_sdhci; > + struct aspeed_sdc *aspeed_sdc; > + u8 ctrl; > + > + pltfm_priv = sdhci_priv(host); > + aspeed_sdhci = sdhci_pltfm_priv(pltfm_priv); > + aspeed_sdc = aspeed_sdhci->parent; > + > + /* Set/clear 8-bit mode */ > + aspeed_sdc_bus_width(aspeed_sdc, aspeed_sdhci, > + width == MMC_BUS_WIDTH_8); > + > + /* Set/clear 1 or 4 bit mode */ > + ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL); > + if (width == MMC_BUS_WIDTH_4) > + ctrl |= SDHCI_CTRL_4BITBUS; > + else > + ctrl &= ~SDHCI_CTRL_4BITBUS; > + sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL); > +} > + > +static const struct sdhci_ops aspeed_sdhci_ops = { > + .set_clock = aspeed_sdhci_set_clock, > + .get_max_clock = sdhci_pltfm_clk_get_max_clock, > + .set_bus_width = aspeed_sdhci_set_bus_width, > + .get_timeout_clock = sdhci_pltfm_clk_get_max_clock, > + .reset = sdhci_reset, > + .set_uhs_signaling = sdhci_set_uhs_signaling, > +}; > + > +static const struct sdhci_pltfm_data aspeed_sdc_pdata = { Up to you, but this is for the aspeed_sdhci driver, so I would have expected it to be called aspeed_sdhci_pdata > + .ops = &aspeed_sdhci_ops, > + .quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN, > + .quirks2 = SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN, You don't use sdhci_set_clock() or sdhci_calc_clk(), so it doesn't look like SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN is needed. > +}; > + > +static inline int aspeed_sdhci_calculate_slot(struct aspeed_sdhci *dev, > + struct resource *res) > +{ > + resource_size_t delta; > + > + if (!res || resource_type(res) != IORESOURCE_MEM) > + return -EINVAL; > + > + if (res->start < dev->parent->res->start) > + return -EINVAL; > + > + delta = res->start - dev->parent->res->start; > + if (delta & (0x100 - 1)) > + return -EINVAL; > + > + return (delta / 0x100) - 1; > +} > + > +static int aspeed_sdhci_probe(struct platform_device *pdev) > +{ > + struct sdhci_pltfm_host *pltfm_host; > + struct aspeed_sdhci *dev; > + struct sdhci_host *host; > + struct resource *res; > + int slot; > + int ret; > + > + host = sdhci_pltfm_init(pdev, &aspeed_sdc_pdata, sizeof(*dev)); > + if (IS_ERR(host)) > + return PTR_ERR(host); > + > + pltfm_host = sdhci_priv(host); > + dev = sdhci_pltfm_priv(pltfm_host); > + dev->parent = dev_get_drvdata(pdev->dev.parent); > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + slot = aspeed_sdhci_calculate_slot(dev, res); > + if (slot < 0) > + return slot; > + dev_info(&pdev->dev, "Configuring for slot %d\n", slot); > + dev->width_mask = !slot ? ASPEED_SDC_S0MMC8 : ASPEED_SDC_S1MMC8; That implies that you only support 2 slots which begs the question why you don't validate slot. > + > + sdhci_get_of_property(pdev); > + > + pltfm_host->clk = devm_clk_get(&pdev->dev, NULL); > + if (IS_ERR(pltfm_host->clk)) > + return PTR_ERR(pltfm_host->clk); > + > + ret = clk_prepare_enable(pltfm_host->clk); > + if (ret) { > + dev_err(&pdev->dev, "Unable to enable SDIO clock\n"); > + goto err_pltfm_free; > + } > + > + ret = mmc_of_parse(host->mmc); > + if (ret) > + goto err_sdhci_add; > + > + ret = sdhci_add_host(host); > + if (ret) > + goto err_sdhci_add; > + > + return 0; > + > +err_sdhci_add: > + clk_disable_unprepare(pltfm_host->clk); > +err_pltfm_free: > + sdhci_pltfm_free(pdev); > + return ret; > +} > + > +static int aspeed_sdhci_remove(struct platform_device *pdev) > +{ > + struct sdhci_pltfm_host *pltfm_host; > + struct sdhci_host *host; > + int dead; > + > + host = platform_get_drvdata(pdev); > + pltfm_host = sdhci_priv(host); > + > + dead = readl(host->ioaddr + SDHCI_INT_STATUS) == 0xffffffff; 'dead' only makes sense for PCI. Just set it to zero. > + > + sdhci_remove_host(host, dead); > + > + clk_disable_unprepare(pltfm_host->clk); > + > + sdhci_pltfm_free(pdev); > + > + return 0; > +} > + > +static const struct of_device_id aspeed_sdhci_of_match[] = { > + { .compatible = "aspeed,ast2400-sdhci", }, > + { .compatible = "aspeed,ast2500-sdhci", }, > + { } > +}; > + > +static struct platform_driver aspeed_sdhci_driver = { > + .driver = { > + .name = "sdhci-aspeed", > + .of_match_table = aspeed_sdhci_of_match, > + }, > + .probe = aspeed_sdhci_probe, > + .remove = aspeed_sdhci_remove, > +}; > + > +module_platform_driver(aspeed_sdhci_driver); > + > +static int aspeed_sdc_probe(struct platform_device *pdev) > + > +{ > + struct device_node *parent, *child; > + struct aspeed_sdc *sdc; > + int ret; > + > + sdc = devm_kzalloc(&pdev->dev, sizeof(*sdc), GFP_KERNEL); > + if (!sdc) > + return -ENOMEM; > + > + spin_lock_init(&sdc->lock); > + > + sdc->clk = devm_clk_get(&pdev->dev, NULL); > + if (IS_ERR(sdc->clk)) > + return PTR_ERR(sdc->clk); > + > + ret = clk_prepare_enable(sdc->clk); > + if (ret) { > + dev_err(&pdev->dev, "Unable to enable SDCLK\n"); > + return ret; > + } > + > + sdc->res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + sdc->regs = devm_ioremap_resource(&pdev->dev, sdc->res); > + if (IS_ERR(sdc->regs)) { > + ret = PTR_ERR(sdc->regs); > + goto err_clk; > + } > + > + dev_set_drvdata(&pdev->dev, sdc); > + > + parent = pdev->dev.of_node; > + for_each_available_child_of_node(parent, child) { > + struct platform_device *cpdev; > + > + cpdev = of_platform_device_create(child, NULL, &pdev->dev); > + if (IS_ERR(cpdev)) { > + of_node_put(child); > + ret = PTR_ERR(pdev); > + goto err_clk; > + } > + } > + > + return 0; > + > +err_clk: > + clk_disable_unprepare(sdc->clk); > + return ret; > +} > + > +static int aspeed_sdc_remove(struct platform_device *pdev) > +{ > + struct aspeed_sdc *sdc = dev_get_drvdata(&pdev->dev); > + > + clk_disable_unprepare(sdc->clk); > + > + return 0; > +} > + > +static const struct of_device_id aspeed_sdc_of_match[] = { > + { .compatible = "aspeed,ast2400-sd-controller", }, > + { .compatible = "aspeed,ast2500-sd-controller", }, > + { } > +}; > + > +MODULE_DEVICE_TABLE(of, aspeed_sdc_of_match); > + > +static struct platform_driver aspeed_sdc_driver = { > + .driver = { > + .name = "sd-controller-aspeed", > + .pm = &sdhci_pltfm_pmops, > + .of_match_table = aspeed_sdc_of_match, > + }, > + .probe = aspeed_sdc_probe, > + .remove = aspeed_sdc_remove, > +}; > + > +module_platform_driver(aspeed_sdc_driver); > + > +MODULE_DESCRIPTION("Driver for the ASPEED SD/SDIO/SDHCI Controllers"); > +MODULE_AUTHOR("Ryan Chen <ryan_chen@aspeedtech.com>"); > +MODULE_AUTHOR("Andrew Jeffery <andrew@aj.id.au>"); > +MODULE_LICENSE("GPL v2"); > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] mmc: Add support for the ASPEED SD controller 2019-07-25 13:18 ` Adrian Hunter @ 2019-07-26 0:52 ` Andrew Jeffery 2019-07-26 5:56 ` Adrian Hunter 0 siblings, 1 reply; 17+ messages in thread From: Andrew Jeffery @ 2019-07-26 0:52 UTC (permalink / raw) To: Adrian Hunter, linux-mmc Cc: mark.rutland, devicetree, Ulf Hansson, linux-aspeed, Ryan Chen, linux-kernel, Rob Herring, Joel Stanley, linux-arm-kernel On Thu, 25 Jul 2019, at 22:49, Adrian Hunter wrote: > On 12/07/19 6:32 AM, Andrew Jeffery wrote: > > Add a minimal driver for ASPEED's SD controller, which exposes two > > SDHCIs. > > > > The ASPEED design implements a common register set for the SDHCIs, and > > moves some of the standard configuration elements out to this common > > area (e.g. 8-bit mode, and card detect configuration which is not > > currently supported). > > > > The SD controller has a dedicated hardware interrupt that is shared > > between the slots. The common register set exposes information on which > > slot triggered the interrupt; early revisions of the patch introduced an > > irqchip for the register, but reality is it doesn't behave as an > > irqchip, and the result fits awkwardly into the irqchip APIs. Instead > > I've taken the simple approach of using the IRQ as a shared IRQ with > > some minor performance impact for the second slot. > > > > Ryan was the original author of the patch - I've taken his work and > > massaged it to drop the irqchip support and rework the devicetree > > integration. The driver has been smoke tested under qemu against a > > minimal SD controller model and lightly tested on an ast2500-evb. > > > > Signed-off-by: Ryan Chen <ryanchen.aspeed@gmail.com> > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > > Looks fine. Few minor comments below. > > > --- > > In v2: > > > > * Drop unnecesasry MODULE_DEVICE_TABLE() > > * Rename sd-controller compatible > > * Add IBM copyright > > * Drop unnecesary data assignment in of match table entries > > * Derive the slot from the SDHCI offset > > > > drivers/mmc/host/Kconfig | 12 ++ > > drivers/mmc/host/Makefile | 1 + > > drivers/mmc/host/sdhci-of-aspeed.c | 326 +++++++++++++++++++++++++++++ > > 3 files changed, 339 insertions(+) > > create mode 100644 drivers/mmc/host/sdhci-of-aspeed.c > > > > diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig > > index 931770f17087..2bb5e1264b3d 100644 > > --- a/drivers/mmc/host/Kconfig > > +++ b/drivers/mmc/host/Kconfig > > @@ -154,6 +154,18 @@ config MMC_SDHCI_OF_ARASAN > > > > If unsure, say N. > > > > +config MMC_SDHCI_OF_ASPEED > > + tristate "SDHCI OF support for the ASPEED SDHCI controller" > > + depends on MMC_SDHCI_PLTFM > > + depends on OF > > + help > > + This selects the ASPEED Secure Digital Host Controller Interface. > > + > > + If you have a controller with this interface, say Y or M here. You > > + also need to enable an appropriate bus interface. > > + > > + If unsure, say N. > > + > > config MMC_SDHCI_OF_AT91 > > tristate "SDHCI OF support for the Atmel SDMMC controller" > > depends on MMC_SDHCI_PLTFM > > diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile > > index 73578718f119..390ee162fe71 100644 > > --- a/drivers/mmc/host/Makefile > > +++ b/drivers/mmc/host/Makefile > > @@ -84,6 +84,7 @@ obj-$(CONFIG_MMC_SDHCI_ESDHC_IMX) += sdhci-esdhc-imx.o > > obj-$(CONFIG_MMC_SDHCI_DOVE) += sdhci-dove.o > > obj-$(CONFIG_MMC_SDHCI_TEGRA) += sdhci-tegra.o > > obj-$(CONFIG_MMC_SDHCI_OF_ARASAN) += sdhci-of-arasan.o > > +obj-$(CONFIG_MMC_SDHCI_OF_ASPEED) += sdhci-of-aspeed.o > > obj-$(CONFIG_MMC_SDHCI_OF_AT91) += sdhci-of-at91.o > > obj-$(CONFIG_MMC_SDHCI_OF_ESDHC) += sdhci-of-esdhc.o > > obj-$(CONFIG_MMC_SDHCI_OF_HLWD) += sdhci-of-hlwd.o > > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c > > new file mode 100644 > > index 000000000000..9528e43c257d > > --- /dev/null > > +++ b/drivers/mmc/host/sdhci-of-aspeed.c > > @@ -0,0 +1,326 @@ > > +// SPDX-License-Identifier: GPL-2.0-or-later > > +/* Copyright (C) 2019 ASPEED Technology Inc. */ > > +/* Copyright (C) 2019 IBM Corp. */ > > + > > +#include <linux/clk.h> > > +#include <linux/delay.h> > > +#include <linux/device.h> > > +#include <linux/io.h> > > +#include <linux/mmc/host.h> > > +#include <linux/module.h> > > +#include <linux/of_address.h> > > +#include <linux/of.h> > > +#include <linux/of_platform.h> > > +#include <linux/platform_device.h> > > +#include <linux/spinlock.h> > > + > > +#include "sdhci-pltfm.h" > > + > > +#define ASPEED_SDC_INFO 0x00 > > +#define ASPEED_SDC_S1MMC8 BIT(25) > > +#define ASPEED_SDC_S0MMC8 BIT(24) > > + > > +struct aspeed_sdc { > > + struct clk *clk; > > + struct resource *res; > > + > > + spinlock_t lock; > > + void __iomem *regs; > > +}; > > + > > +struct aspeed_sdhci { > > + struct aspeed_sdc *parent; > > + u32 width_mask; > > +}; > > + > > +static void aspeed_sdc_bus_width(struct aspeed_sdc *sdc, > > + struct aspeed_sdhci *sdhci, bool bus8) > > The function name threw me at first. I suggest: > > static void aspeed_sdhci_set_clr_8_bit_mode(struct aspeed_sdhci *aspeed_sdhci, > bool bus8) I wasn't real happy with the name I picked, so was hoping to some degree to be bike-shedded on it. I'll fix it up. > { > struct aspeed_sdc *aspeed_sdc = aspeed_sdhci->parent; > > > +{ > > + u32 info; > > + > > + /* Set/clear 8 bit mode */ > > + spin_lock(&sdc->lock); > > + info = readl(sdc->regs + ASPEED_SDC_INFO); > > + if (bus8) > > + info |= sdhci->width_mask; > > + else > > + info &= ~sdhci->width_mask; > > + writel(info, sdc->regs + ASPEED_SDC_INFO); > > + spin_unlock(&sdc->lock); > > +} > > + > > +static void aspeed_sdhci_set_clock(struct sdhci_host *host, unsigned int clock) > > +{ > > + unsigned long timeout; > > + int div; > > + u16 clk; > > + > > + if (clock == host->clock) > > + return; > > + > > + sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL); > > + > > + if (clock == 0) > > + goto out; > > + > > + for (div = 1; div < 256; div *= 2) { > > + if ((host->max_clk / div) <= clock) > > + break; > > + } > > + div >>= 1; > > + > > + clk = div << SDHCI_DIVIDER_SHIFT; > > Could call sdhci_enable_clk() here. I'll look into it. > > > + clk |= SDHCI_CLOCK_INT_EN; > > + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL); > > + > > + /* Wait max 20 ms */ > > + timeout = 20; > > + while (!((clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL)) > > + & SDHCI_CLOCK_INT_STABLE)) { > > + if (timeout == 0) { > > + pr_err("%s: Internal clock never stabilised.\n", > > + mmc_hostname(host->mmc)); > > + return; > > + } > > + timeout--; > > + mdelay(1); > > + } > > + > > + clk |= SDHCI_CLOCK_CARD_EN; > > + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL); > > + > > +out: > > + host->clock = clock; > > +} > > + > > +static void aspeed_sdhci_set_bus_width(struct sdhci_host *host, int width) > > +{ > > + struct sdhci_pltfm_host *pltfm_priv; > > + struct aspeed_sdhci *aspeed_sdhci; > > + struct aspeed_sdc *aspeed_sdc; > > + u8 ctrl; > > + > > + pltfm_priv = sdhci_priv(host); > > + aspeed_sdhci = sdhci_pltfm_priv(pltfm_priv); > > + aspeed_sdc = aspeed_sdhci->parent; > > + > > + /* Set/clear 8-bit mode */ > > + aspeed_sdc_bus_width(aspeed_sdc, aspeed_sdhci, > > + width == MMC_BUS_WIDTH_8); > > + > > + /* Set/clear 1 or 4 bit mode */ > > + ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL); > > + if (width == MMC_BUS_WIDTH_4) > > + ctrl |= SDHCI_CTRL_4BITBUS; > > + else > > + ctrl &= ~SDHCI_CTRL_4BITBUS; > > + sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL); > > +} > > + > > +static const struct sdhci_ops aspeed_sdhci_ops = { > > + .set_clock = aspeed_sdhci_set_clock, > > + .get_max_clock = sdhci_pltfm_clk_get_max_clock, > > + .set_bus_width = aspeed_sdhci_set_bus_width, > > + .get_timeout_clock = sdhci_pltfm_clk_get_max_clock, > > + .reset = sdhci_reset, > > + .set_uhs_signaling = sdhci_set_uhs_signaling, > > +}; > > + > > +static const struct sdhci_pltfm_data aspeed_sdc_pdata = { > > Up to you, but this is for the aspeed_sdhci driver, so I would > have expected it to be called aspeed_sdhci_pdata Uh, yeah. Good catch. > > > + .ops = &aspeed_sdhci_ops, > > + .quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN, > > + .quirks2 = SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN, > > You don't use sdhci_set_clock() or sdhci_calc_clk(), so it doesn't > look like SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN is needed. I did look at that and was a bit suspicious about it, good to have my suspicion confirmed. > > > +}; > > + > > +static inline int aspeed_sdhci_calculate_slot(struct aspeed_sdhci *dev, > > + struct resource *res) > > +{ > > + resource_size_t delta; > > + > > + if (!res || resource_type(res) != IORESOURCE_MEM) > > + return -EINVAL; > > + > > + if (res->start < dev->parent->res->start) > > + return -EINVAL; > > + > > + delta = res->start - dev->parent->res->start; > > + if (delta & (0x100 - 1)) > > + return -EINVAL; > > + > > + return (delta / 0x100) - 1; > > +} > > + > > +static int aspeed_sdhci_probe(struct platform_device *pdev) > > +{ > > + struct sdhci_pltfm_host *pltfm_host; > > + struct aspeed_sdhci *dev; > > + struct sdhci_host *host; > > + struct resource *res; > > + int slot; > > + int ret; > > + > > + host = sdhci_pltfm_init(pdev, &aspeed_sdc_pdata, sizeof(*dev)); > > + if (IS_ERR(host)) > > + return PTR_ERR(host); > > + > > + pltfm_host = sdhci_priv(host); > > + dev = sdhci_pltfm_priv(pltfm_host); > > + dev->parent = dev_get_drvdata(pdev->dev.parent); > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + slot = aspeed_sdhci_calculate_slot(dev, res); > > + if (slot < 0) > > + return slot; > > + dev_info(&pdev->dev, "Configuring for slot %d\n", slot); > > + dev->width_mask = !slot ? ASPEED_SDC_S0MMC8 : ASPEED_SDC_S1MMC8; > > That implies that you only support 2 slots which begs the question why > you don't validate slot. I'm not sure what you mean here, but I'll dig into it. > > > + > > + sdhci_get_of_property(pdev); > > + > > + pltfm_host->clk = devm_clk_get(&pdev->dev, NULL); > > + if (IS_ERR(pltfm_host->clk)) > > + return PTR_ERR(pltfm_host->clk); > > + > > + ret = clk_prepare_enable(pltfm_host->clk); > > + if (ret) { > > + dev_err(&pdev->dev, "Unable to enable SDIO clock\n"); > > + goto err_pltfm_free; > > + } > > + > > + ret = mmc_of_parse(host->mmc); > > + if (ret) > > + goto err_sdhci_add; > > + > > + ret = sdhci_add_host(host); > > + if (ret) > > + goto err_sdhci_add; > > + > > + return 0; > > + > > +err_sdhci_add: > > + clk_disable_unprepare(pltfm_host->clk); > > +err_pltfm_free: > > + sdhci_pltfm_free(pdev); > > + return ret; > > +} > > + > > +static int aspeed_sdhci_remove(struct platform_device *pdev) > > +{ > > + struct sdhci_pltfm_host *pltfm_host; > > + struct sdhci_host *host; > > + int dead; > > + > > + host = platform_get_drvdata(pdev); > > + pltfm_host = sdhci_priv(host); > > + > > + dead = readl(host->ioaddr + SDHCI_INT_STATUS) == 0xffffffff; > > 'dead' only makes sense for PCI. Just set it to zero. Ack. Thanks for the feedback, I'll send a v3. Andrew > > > + > > + sdhci_remove_host(host, dead); > > + > > + clk_disable_unprepare(pltfm_host->clk); > > + > > + sdhci_pltfm_free(pdev); > > + > > + return 0; > > +} > > + > > +static const struct of_device_id aspeed_sdhci_of_match[] = { > > + { .compatible = "aspeed,ast2400-sdhci", }, > > + { .compatible = "aspeed,ast2500-sdhci", }, > > + { } > > +}; > > + > > +static struct platform_driver aspeed_sdhci_driver = { > > + .driver = { > > + .name = "sdhci-aspeed", > > + .of_match_table = aspeed_sdhci_of_match, > > + }, > > + .probe = aspeed_sdhci_probe, > > + .remove = aspeed_sdhci_remove, > > +}; > > + > > +module_platform_driver(aspeed_sdhci_driver); > > + > > +static int aspeed_sdc_probe(struct platform_device *pdev) > > + > > +{ > > + struct device_node *parent, *child; > > + struct aspeed_sdc *sdc; > > + int ret; > > + > > + sdc = devm_kzalloc(&pdev->dev, sizeof(*sdc), GFP_KERNEL); > > + if (!sdc) > > + return -ENOMEM; > > + > > + spin_lock_init(&sdc->lock); > > + > > + sdc->clk = devm_clk_get(&pdev->dev, NULL); > > + if (IS_ERR(sdc->clk)) > > + return PTR_ERR(sdc->clk); > > + > > + ret = clk_prepare_enable(sdc->clk); > > + if (ret) { > > + dev_err(&pdev->dev, "Unable to enable SDCLK\n"); > > + return ret; > > + } > > + > > + sdc->res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + sdc->regs = devm_ioremap_resource(&pdev->dev, sdc->res); > > + if (IS_ERR(sdc->regs)) { > > + ret = PTR_ERR(sdc->regs); > > + goto err_clk; > > + } > > + > > + dev_set_drvdata(&pdev->dev, sdc); > > + > > + parent = pdev->dev.of_node; > > + for_each_available_child_of_node(parent, child) { > > + struct platform_device *cpdev; > > + > > + cpdev = of_platform_device_create(child, NULL, &pdev->dev); > > + if (IS_ERR(cpdev)) { > > + of_node_put(child); > > + ret = PTR_ERR(pdev); > > + goto err_clk; > > + } > > + } > > + > > + return 0; > > + > > +err_clk: > > + clk_disable_unprepare(sdc->clk); > > + return ret; > > +} > > + > > +static int aspeed_sdc_remove(struct platform_device *pdev) > > +{ > > + struct aspeed_sdc *sdc = dev_get_drvdata(&pdev->dev); > > + > > + clk_disable_unprepare(sdc->clk); > > + > > + return 0; > > +} > > + > > +static const struct of_device_id aspeed_sdc_of_match[] = { > > + { .compatible = "aspeed,ast2400-sd-controller", }, > > + { .compatible = "aspeed,ast2500-sd-controller", }, > > + { } > > +}; > > + > > +MODULE_DEVICE_TABLE(of, aspeed_sdc_of_match); > > + > > +static struct platform_driver aspeed_sdc_driver = { > > + .driver = { > > + .name = "sd-controller-aspeed", > > + .pm = &sdhci_pltfm_pmops, > > + .of_match_table = aspeed_sdc_of_match, > > + }, > > + .probe = aspeed_sdc_probe, > > + .remove = aspeed_sdc_remove, > > +}; > > + > > +module_platform_driver(aspeed_sdc_driver); > > + > > +MODULE_DESCRIPTION("Driver for the ASPEED SD/SDIO/SDHCI Controllers"); > > +MODULE_AUTHOR("Ryan Chen <ryan_chen@aspeedtech.com>"); > > +MODULE_AUTHOR("Andrew Jeffery <andrew@aj.id.au>"); > > +MODULE_LICENSE("GPL v2"); > > > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] mmc: Add support for the ASPEED SD controller 2019-07-26 0:52 ` Andrew Jeffery @ 2019-07-26 5:56 ` Adrian Hunter 2019-07-26 6:47 ` Andrew Jeffery 0 siblings, 1 reply; 17+ messages in thread From: Adrian Hunter @ 2019-07-26 5:56 UTC (permalink / raw) To: Andrew Jeffery, linux-mmc Cc: mark.rutland, devicetree, Ulf Hansson, linux-aspeed, Ryan Chen, linux-kernel, Rob Herring, Joel Stanley, linux-arm-kernel On 26/07/19 3:52 AM, Andrew Jeffery wrote: > On Thu, 25 Jul 2019, at 22:49, Adrian Hunter wrote: >> On 12/07/19 6:32 AM, Andrew Jeffery wrote: >>> +static int aspeed_sdhci_probe(struct platform_device *pdev) >>> +{ >>> + struct sdhci_pltfm_host *pltfm_host; >>> + struct aspeed_sdhci *dev; >>> + struct sdhci_host *host; >>> + struct resource *res; >>> + int slot; >>> + int ret; >>> + >>> + host = sdhci_pltfm_init(pdev, &aspeed_sdc_pdata, sizeof(*dev)); >>> + if (IS_ERR(host)) >>> + return PTR_ERR(host); >>> + >>> + pltfm_host = sdhci_priv(host); >>> + dev = sdhci_pltfm_priv(pltfm_host); >>> + dev->parent = dev_get_drvdata(pdev->dev.parent); >>> + >>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>> + slot = aspeed_sdhci_calculate_slot(dev, res); >>> + if (slot < 0) >>> + return slot; >>> + dev_info(&pdev->dev, "Configuring for slot %d\n", slot); >>> + dev->width_mask = !slot ? ASPEED_SDC_S0MMC8 : ASPEED_SDC_S1MMC8; >> >> That implies that you only support 2 slots which begs the question why >> you don't validate slot. > > I'm not sure what you mean here, but I'll dig into it. I just meant, if you only support 2 slots: if (slot > 1) return -EINVAL; _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] mmc: Add support for the ASPEED SD controller 2019-07-26 5:56 ` Adrian Hunter @ 2019-07-26 6:47 ` Andrew Jeffery 0 siblings, 0 replies; 17+ messages in thread From: Andrew Jeffery @ 2019-07-26 6:47 UTC (permalink / raw) To: Adrian Hunter, linux-mmc Cc: mark.rutland, devicetree, Ulf Hansson, linux-aspeed, Ryan Chen, linux-kernel, Rob Herring, Joel Stanley, linux-arm-kernel On Fri, 26 Jul 2019, at 15:27, Adrian Hunter wrote: > On 26/07/19 3:52 AM, Andrew Jeffery wrote: > > On Thu, 25 Jul 2019, at 22:49, Adrian Hunter wrote: > >> On 12/07/19 6:32 AM, Andrew Jeffery wrote: > >>> +static int aspeed_sdhci_probe(struct platform_device *pdev) > >>> +{ > >>> + struct sdhci_pltfm_host *pltfm_host; > >>> + struct aspeed_sdhci *dev; > >>> + struct sdhci_host *host; > >>> + struct resource *res; > >>> + int slot; > >>> + int ret; > >>> + > >>> + host = sdhci_pltfm_init(pdev, &aspeed_sdc_pdata, sizeof(*dev)); > >>> + if (IS_ERR(host)) > >>> + return PTR_ERR(host); > >>> + > >>> + pltfm_host = sdhci_priv(host); > >>> + dev = sdhci_pltfm_priv(pltfm_host); > >>> + dev->parent = dev_get_drvdata(pdev->dev.parent); > >>> + > >>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > >>> + slot = aspeed_sdhci_calculate_slot(dev, res); > >>> + if (slot < 0) > >>> + return slot; > >>> + dev_info(&pdev->dev, "Configuring for slot %d\n", slot); > >>> + dev->width_mask = !slot ? ASPEED_SDC_S0MMC8 : ASPEED_SDC_S1MMC8; > >> > >> That implies that you only support 2 slots which begs the question why > >> you don't validate slot. > > > > I'm not sure what you mean here, but I'll dig into it. > > I just meant, if you only support 2 slots: > > if (slot > 1) > return -EINVAL; > Oh, sure. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2019-07-26 6:49 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-12 3:32 [PATCH v2 0/2] mmc: Add support for the ASPEED SD controller Andrew Jeffery 2019-07-12 3:32 ` [PATCH v2 1/2] dt-bindings: mmc: Document Aspeed " Andrew Jeffery 2019-07-12 13:03 ` Rob Herring 2019-07-12 13:10 ` Maxime Ripard 2019-07-15 2:30 ` Andrew Jeffery 2019-07-15 13:26 ` Rob Herring 2019-07-15 22:16 ` Rob Herring 2019-07-16 0:36 ` Andrew Jeffery 2019-07-16 14:57 ` Rob Herring 2019-07-17 3:57 ` Andrew Jeffery 2019-07-17 13:43 ` Rob Herring 2019-07-18 1:49 ` Andrew Jeffery 2019-07-12 3:32 ` [PATCH v2 2/2] mmc: Add support for the ASPEED " Andrew Jeffery 2019-07-25 13:18 ` Adrian Hunter 2019-07-26 0:52 ` Andrew Jeffery 2019-07-26 5:56 ` Adrian Hunter 2019-07-26 6:47 ` Andrew Jeffery
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).