AST2600-A2 EVB has the reference design for enabling SD bus
power and toggling SD bus signal voltage between 3.3v and 1.8v by
GPIO regulators.
This patch series provides the example for enabling regulators and
supporting SDR104 mode on AST2600-A2 EVB.
The description of the reference design of AST2600-A2 EVB is added
in the dts file.
This patch also include a helper for updating AST2600 sdhci capability
registers, and assert/deassert the reset signal for cleaning up AST2600
eMMC controller before eMMC is probed.
Changes from v2:
* Move the comment of the reference design from dt-bindings to device tree.
* Add clk-phase binding for eMMC controller.
* Reimplement aspeed_sdc_set_slot_capability().
* Separate the implementation of eMMC reset to another patch file.
* Fix yaml document error per the report of dt_binding_check and
dtbs_check.
Changes from v1:
* Add the device tree example for AST2600 A2 EVB in dt-bindings
document
* Add timing-phase for eMMC controller.
* Remove power-gpio and power-switch-gpio from sdhci driver, they should
be handled by regulator.
* Add a helper to update capability registers in the driver.
* Sync sdhci settings from device tree to SoC capability registers.
* Sync timing-phase from device tree to SoC Clock Phase Control
register
Please help to review.
Regards,
Steven
Steven Lee (5):
dt-bindings: mmc: sdhci-of-aspeed: Add an example for AST2600-A2 EVB
ARM: dts: aspeed: ast2600evb: Add comment for gpio regulator of sdhci
ARM: dts: aspeed: ast2600evb: Add phase correction for emmc
controller.
mmc: sdhci-of-aspeed: Add a helper for updating capability register.
mmc: sdhci-of-aspeed: Assert/Deassert reset signal before probing eMMC
.../devicetree/bindings/mmc/aspeed,sdhci.yaml | 101 ++++++++++++++++-
arch/arm/boot/dts/aspeed-ast2600-evb.dts | 18 ++-
drivers/mmc/host/sdhci-of-aspeed.c | 106 ++++++++++++++++--
3 files changed, 211 insertions(+), 14 deletions(-)
--
2.17.1
AST2600-A2 EVB has the reference design for enabling SD bus
power and toggling SD bus signal voltage by GPIO pins.
In the reference design, GPIOV0 of AST2600-A2 EVB is connected to
power load switch that providing 3.3v to SD1 bus vdd. GPIOV1 is
connected to a 1.8v and a 3.3v power load switch that providing
signal voltage to
SD1 bus.
If GPIOV0 is active high, SD1 bus is enabled. Otherwise, SD1 bus is
disabled.
If GPIOV1 is active high, 3.3v power load switch is enabled, SD1
signal voltage is 3.3v. Otherwise, 1.8v power load switch will be
enabled, SD1 signal voltage becomes 1.8v.
AST2600-A2 EVB also support toggling signal voltage for SD2 bus.
The design is the same as SD1 bus. It uses GPIOV2 as power-gpio and
GPIOV3 as power-switch-gpio.
Signed-off-by: Steven Lee <[email protected]>
---
.../devicetree/bindings/mmc/aspeed,sdhci.yaml | 101 +++++++++++++++++-
1 file changed, 97 insertions(+), 4 deletions(-)
diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
index 987b287f3bff..de7e61b3d37a 100644
--- a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
+++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
@@ -45,10 +45,16 @@ patternProperties:
properties:
compatible:
- enum:
- - aspeed,ast2400-sdhci
- - aspeed,ast2500-sdhci
- - aspeed,ast2600-sdhci
+ oneOf:
+ - items:
+ - enum:
+ - aspeed,ast2400-sdhci
+ - aspeed,ast2500-sdhci
+ - aspeed,ast2600-sdhci
+ - items:
+ - enum:
+ - aspeed,ast2600-sdhci
+ - const: sdhci
reg:
maxItems: 1
description: The SDHCI registers
@@ -104,3 +110,90 @@ examples:
clocks = <&syscon ASPEED_CLK_SDIO>;
};
};
+
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+ #include <dt-bindings/clock/aspeed-clock.h>
+ #include <dt-bindings/interrupt-controller/irq.h>
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+ vcc_sdhci0: regulator-vcc-sdhci0 {
+ compatible = "regulator-fixed";
+ regulator-name = "SDHCI0 Vcc";
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ gpios = <&gpio0 168
+ GPIO_ACTIVE_HIGH>;
+ enable-active-high;
+ };
+
+ vccq_sdhci0: regulator-vccq-sdhci0 {
+ compatible = "regulator-gpio";
+
+ regulator-name = "SDHCI0 VccQ";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <3300000>;
+ gpios = <&gpio0 169
+ GPIO_ACTIVE_HIGH>;
+ gpios-states = <1>;
+ states = <3300000 1>,
+ <1800000 0>;
+ };
+
+ vcc_sdhci1: regulator-vcc-sdhci1 {
+ compatible = "regulator-fixed";
+
+ regulator-name = "SDHCI1 Vcc";
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ gpios = <&gpio0 170
+ GPIO_ACTIVE_HIGH>;
+ enable-active-high;
+ };
+
+ vccq_sdhci1: regulator-vccq-sdhci1 {
+ compatible = "regulator-gpio";
+
+ regulator-name = "SDHCI1 VccQ";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <3300000>;
+ gpios = <&gpio0 171
+ GPIO_ACTIVE_HIGH>;
+ gpios-states = <1>;
+ states = <3300000 1>,
+ <1800000 0>;
+ };
+
+ sdc@1e740000 {
+ compatible = "aspeed,ast2600-sd-controller";
+ reg = <0x1e740000 0x100>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0 0x1e740000 0x20000>;
+ clocks = <&syscon ASPEED_CLK_GATE_SDCLK>;
+
+ sdhci@1e740100 {
+ compatible = "aspeed,ast2600-sdhci","sdhci";
+ reg = <0x100 0x100>;
+ interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
+ sdhci,auto-cmd12;
+ clocks = <&syscon ASPEED_CLK_SDIO>;
+ vmmc-supply = <&vcc_sdhci0>;
+ vqmmc-supply = <&vccq_sdhci0>;
+ sd-uhs-sdr104;
+ clk-phase-uhs-sdr104 = <180>, <180>;
+ };
+
+ sdhci@1e740200 {
+ compatible = "aspeed,ast2600-sdhci","sdhci";
+ reg = <0x200 0x100>;
+ interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
+ sdhci,auto-cmd12;
+ clocks = <&syscon ASPEED_CLK_SDIO>;
+ vmmc-supply = <&vcc_sdhci1>;
+ vqmmc-supply = <&vccq_sdhci1>;
+ sd-uhs-sdr104;
+ clk-phase-uhs-sdr104 = <0>, <0>;
+ };
+ };
+
+...
--
2.17.1
The patch add a new function aspeed_sdc_set_slot_capability() for
updating sdhci capability register.
Signed-off-by: Steven Lee <[email protected]>
---
drivers/mmc/host/sdhci-of-aspeed.c | 57 ++++++++++++++++++++++++++++++
1 file changed, 57 insertions(+)
diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
index d001c51074a0..4979f98ffb52 100644
--- a/drivers/mmc/host/sdhci-of-aspeed.c
+++ b/drivers/mmc/host/sdhci-of-aspeed.c
@@ -31,6 +31,11 @@
#define ASPEED_SDC_S0_PHASE_OUT_EN GENMASK(1, 0)
#define ASPEED_SDC_PHASE_MAX 31
+/* SDIO{10,20} */
+#define ASPEED_SDC_CAP1_1_8V (0 * 32 + 26)
+/* SDIO{14,24} */
+#define ASPEED_SDC_CAP2_SDR104 (1 * 32 + 1)
+
struct aspeed_sdc {
struct clk *clk;
struct resource *res;
@@ -70,8 +75,42 @@ struct aspeed_sdhci {
u32 width_mask;
struct mmc_clk_phase_map phase_map;
const struct aspeed_sdhci_phase_desc *phase_desc;
+
};
+/*
+ * The function sets the mirror register for updating
+ * capbilities of the current slot.
+ *
+ * slot | capability | caps_reg | mirror_reg
+ * -----|-------------|----------|------------
+ * 0 | CAP1_1_8V | SDIO140 | SDIO10
+ * 0 | CAP2_SDR104 | SDIO144 | SDIO14
+ * 1 | CAP1_1_8V | SDIO240 | SDIO20
+ * 1 | CAP2_SDR104 | SDIO244 | SDIO24
+ */
+static void aspeed_sdc_set_slot_capability(struct sdhci_host *host,
+ struct aspeed_sdc *sdc,
+ int capability,
+ bool enable,
+ u8 slot)
+{
+ u8 cap_reg;
+ u32 mirror_reg_offset, cap_val;
+
+ if (slot > 1)
+ return;
+
+ cap_reg = capability / 32;
+ cap_val = sdhci_readl(host, 0x40 + (cap_reg * 4));
+ if (enable)
+ cap_val |= BIT(capability % 32);
+ else
+ cap_val &= ~BIT(capability % 32);
+ mirror_reg_offset = ((slot + 1) * 0x10) + (cap_reg * 4);
+ writel(cap_val, sdc->regs + mirror_reg_offset);
+}
+
static void aspeed_sdc_configure_8bit_mode(struct aspeed_sdc *sdc,
struct aspeed_sdhci *sdhci,
bool bus8)
@@ -329,6 +368,7 @@ static int aspeed_sdhci_probe(struct platform_device *pdev)
{
const struct aspeed_sdhci_pdata *aspeed_pdata;
struct sdhci_pltfm_host *pltfm_host;
+ struct device_node *np = pdev->dev.of_node;
struct aspeed_sdhci *dev;
struct sdhci_host *host;
struct resource *res;
@@ -372,6 +412,23 @@ static int aspeed_sdhci_probe(struct platform_device *pdev)
sdhci_get_of_property(pdev);
+ if (of_property_read_bool(np, "mmc-hs200-1_8v") ||
+ of_property_read_bool(np, "sd-uhs-sdr104")) {
+ aspeed_sdc_set_slot_capability(host,
+ dev->parent,
+ ASPEED_SDC_CAP1_1_8V,
+ true,
+ slot);
+ }
+
+ if (of_property_read_bool(np, "sd-uhs-sdr104")) {
+ aspeed_sdc_set_slot_capability(host,
+ dev->parent,
+ ASPEED_SDC_CAP2_SDR104,
+ true,
+ slot);
+ }
+
pltfm_host->clk = devm_clk_get(&pdev->dev, NULL);
if (IS_ERR(pltfm_host->clk))
return PTR_ERR(pltfm_host->clk);
--
2.17.1
Add the description for describing the AST2600-A2 EVB reference design of
GPIO regulators.
Signed-off-by: Steven Lee <[email protected]>
---
arch/arm/boot/dts/aspeed-ast2600-evb.dts | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/arch/arm/boot/dts/aspeed-ast2600-evb.dts b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
index 2772796e215e..1ae0facc3d5f 100644
--- a/arch/arm/boot/dts/aspeed-ast2600-evb.dts
+++ b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
@@ -104,6 +104,21 @@
status = "okay";
};
+/*
+ * The signal voltage of sdhci0 and sdhci1 on AST2600-A2 EVB is able to be
+ * toggled by GPIO pins.
+ * In the reference design, GPIOV0 of AST2600-A2 EVB is connected to the
+ * power load switch that providing 3.3v to sdhci0 vdd, GPIOV1 is connected to
+ * a 1.8v and a 3.3v power load switch that providing signal voltage to
+ * sdhci0 bus.
+ * If GPIOV0 is active high, sdhci0 is enabled, otherwise, sdhci0 is disabled.
+ * If GPIOV1 is active high, 3.3v power load switch is enabled, sdhci0 signal
+ * voltage is 3.3v, otherwise, 1.8v power load switch will be enabled,
+ * sdhci0 signal voltage becomes 1.8v.
+ * AST2600-A2 EVB also support toggling signal voltage for sdhci1.
+ * The design is the same as sdhci0, it uses GPIOV2 as power-gpio and GPIOV3
+ * as power-switch-gpio.
+ */
&emmc {
non-removable;
bus-width = <4>;
--
2.17.1
For cleaning up the AST2600 eMMC controller, the reset signal should be
asserted and deasserted before it is probed.
Signed-off-by: Steven Lee <[email protected]>
---
drivers/mmc/host/sdhci-of-aspeed.c | 49 ++++++++++++++++++++++++------
1 file changed, 40 insertions(+), 9 deletions(-)
diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
index 4979f98ffb52..8ef06f32abff 100644
--- a/drivers/mmc/host/sdhci-of-aspeed.c
+++ b/drivers/mmc/host/sdhci-of-aspeed.c
@@ -13,6 +13,7 @@
#include <linux/of.h>
#include <linux/of_platform.h>
#include <linux/platform_device.h>
+#include <linux/reset.h>
#include <linux/spinlock.h>
#include "sdhci-pltfm.h"
@@ -36,9 +37,16 @@
/* SDIO{14,24} */
#define ASPEED_SDC_CAP2_SDR104 (1 * 32 + 1)
+#define PROBE_AFTER_ASSET_DEASSERT 0x1
+
+struct aspeed_sdc_info {
+ u32 flag;
+};
+
struct aspeed_sdc {
struct clk *clk;
struct resource *res;
+ struct reset_control *rst;
spinlock_t lock;
void __iomem *regs;
@@ -78,6 +86,10 @@ struct aspeed_sdhci {
};
+struct aspeed_sdc_info ast2600_sdc_info = {
+ .flag = PROBE_AFTER_ASSET_DEASSERT
+};
+
/*
* The function sets the mirror register for updating
* capbilities of the current slot.
@@ -533,11 +545,22 @@ static struct platform_driver aspeed_sdhci_driver = {
.remove = aspeed_sdhci_remove,
};
+static const struct of_device_id aspeed_sdc_of_match[] = {
+ { .compatible = "aspeed,ast2400-sd-controller", },
+ { .compatible = "aspeed,ast2500-sd-controller", },
+ { .compatible = "aspeed,ast2600-sd-controller", .data = &ast2600_sdc_info},
+ { }
+};
+
+MODULE_DEVICE_TABLE(of, aspeed_sdc_of_match);
+
static int aspeed_sdc_probe(struct platform_device *pdev)
{
struct device_node *parent, *child;
struct aspeed_sdc *sdc;
+ const struct of_device_id *match = NULL;
+ const struct aspeed_sdc_info *info = NULL;
int ret;
sdc = devm_kzalloc(&pdev->dev, sizeof(*sdc), GFP_KERNEL);
@@ -546,6 +569,23 @@ static int aspeed_sdc_probe(struct platform_device *pdev)
spin_lock_init(&sdc->lock);
+ match = of_match_device(aspeed_sdc_of_match, &pdev->dev);
+ if (!match)
+ return -ENODEV;
+
+ if (match->data)
+ info = match->data;
+
+ if (info) {
+ if (info->flag & PROBE_AFTER_ASSET_DEASSERT) {
+ sdc->rst = devm_reset_control_get(&pdev->dev, NULL);
+ if (!IS_ERR(sdc->rst)) {
+ reset_control_assert(sdc->rst);
+ reset_control_deassert(sdc->rst);
+ }
+ }
+ }
+
sdc->clk = devm_clk_get(&pdev->dev, NULL);
if (IS_ERR(sdc->clk))
return PTR_ERR(sdc->clk);
@@ -593,15 +633,6 @@ static int aspeed_sdc_remove(struct platform_device *pdev)
return 0;
}
-static const struct of_device_id aspeed_sdc_of_match[] = {
- { .compatible = "aspeed,ast2400-sd-controller", },
- { .compatible = "aspeed,ast2500-sd-controller", },
- { .compatible = "aspeed,ast2600-sd-controller", },
- { }
-};
-
-MODULE_DEVICE_TABLE(of, aspeed_sdc_of_match);
-
static struct platform_driver aspeed_sdc_driver = {
.driver = {
.name = "sd-controller-aspeed",
--
2.17.1
On Thu, 6 May 2021, at 19:33, Steven Lee wrote:
> Add the description for describing the AST2600-A2 EVB reference design of
> GPIO regulators.
>
> Signed-off-by: Steven Lee <[email protected]>
> ---
> arch/arm/boot/dts/aspeed-ast2600-evb.dts | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/arch/arm/boot/dts/aspeed-ast2600-evb.dts
> b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
> index 2772796e215e..1ae0facc3d5f 100644
> --- a/arch/arm/boot/dts/aspeed-ast2600-evb.dts
> +++ b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
> @@ -104,6 +104,21 @@
> status = "okay";
> };
>
> +/*
> + * The signal voltage of sdhci0 and sdhci1 on AST2600-A2 EVB is able to be
> + * toggled by GPIO pins.
> + * In the reference design, GPIOV0 of AST2600-A2 EVB is connected to the
> + * power load switch that providing 3.3v to sdhci0 vdd, GPIOV1 is connected to
> + * a 1.8v and a 3.3v power load switch that providing signal voltage to
> + * sdhci0 bus.
> + * If GPIOV0 is active high, sdhci0 is enabled, otherwise, sdhci0 is disabled.
> + * If GPIOV1 is active high, 3.3v power load switch is enabled, sdhci0 signal
> + * voltage is 3.3v, otherwise, 1.8v power load switch will be enabled,
> + * sdhci0 signal voltage becomes 1.8v.
> + * AST2600-A2 EVB also support toggling signal voltage for sdhci1.
> + * The design is the same as sdhci0, it uses GPIOV2 as power-gpio and GPIOV3
> + * as power-switch-gpio.
> + */
Okay, I think the comment is in the right place, but I feel this patch
should also add the regulator nodes and hook them up to the SDHCIs.
Given Rob isn't super keen on a second example in the binding document
I think you can just cut the example out and paste it in here.
Andrew
On Thu, May 06, 2021 at 06:03:08PM +0800, Steven Lee wrote:
> AST2600-A2 EVB has the reference design for enabling SD bus
> power and toggling SD bus signal voltage by GPIO pins.
>
> In the reference design, GPIOV0 of AST2600-A2 EVB is connected to
> power load switch that providing 3.3v to SD1 bus vdd. GPIOV1 is
> connected to a 1.8v and a 3.3v power load switch that providing
> signal voltage to
> SD1 bus.
>
> If GPIOV0 is active high, SD1 bus is enabled. Otherwise, SD1 bus is
> disabled.
> If GPIOV1 is active high, 3.3v power load switch is enabled, SD1
> signal voltage is 3.3v. Otherwise, 1.8v power load switch will be
> enabled, SD1 signal voltage becomes 1.8v.
>
> AST2600-A2 EVB also support toggling signal voltage for SD2 bus.
> The design is the same as SD1 bus. It uses GPIOV2 as power-gpio and
> GPIOV3 as power-switch-gpio.
>
> Signed-off-by: Steven Lee <[email protected]>
> ---
> .../devicetree/bindings/mmc/aspeed,sdhci.yaml | 101 +++++++++++++++++-
> 1 file changed, 97 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> index 987b287f3bff..de7e61b3d37a 100644
> --- a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> @@ -45,10 +45,16 @@ patternProperties:
>
> properties:
> compatible:
> - enum:
> - - aspeed,ast2400-sdhci
> - - aspeed,ast2500-sdhci
> - - aspeed,ast2600-sdhci
> + oneOf:
> + - items:
> + - enum:
> + - aspeed,ast2400-sdhci
> + - aspeed,ast2500-sdhci
> + - aspeed,ast2600-sdhci
> + - items:
> + - enum:
> + - aspeed,ast2600-sdhci
> + - const: sdhci
Why are you adding 'sdhci'. That's not useful as a compatible given how
many quirks different implementations have.
> reg:
> maxItems: 1
> description: The SDHCI registers
> @@ -104,3 +110,90 @@ examples:
> clocks = <&syscon ASPEED_CLK_SDIO>;
> };
> };
> +
> + - |
Why do we need another example?
> + #include <dt-bindings/gpio/gpio.h>
> + #include <dt-bindings/clock/aspeed-clock.h>
> + #include <dt-bindings/interrupt-controller/irq.h>
> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> + vcc_sdhci0: regulator-vcc-sdhci0 {
> + compatible = "regulator-fixed";
> + regulator-name = "SDHCI0 Vcc";
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> + gpios = <&gpio0 168
> + GPIO_ACTIVE_HIGH>;
> + enable-active-high;
> + };
> +
> + vccq_sdhci0: regulator-vccq-sdhci0 {
> + compatible = "regulator-gpio";
> +
> + regulator-name = "SDHCI0 VccQ";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <3300000>;
> + gpios = <&gpio0 169
> + GPIO_ACTIVE_HIGH>;
> + gpios-states = <1>;
> + states = <3300000 1>,
> + <1800000 0>;
> + };
> +
> + vcc_sdhci1: regulator-vcc-sdhci1 {
> + compatible = "regulator-fixed";
> +
> + regulator-name = "SDHCI1 Vcc";
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> + gpios = <&gpio0 170
> + GPIO_ACTIVE_HIGH>;
> + enable-active-high;
> + };
> +
> + vccq_sdhci1: regulator-vccq-sdhci1 {
> + compatible = "regulator-gpio";
> +
> + regulator-name = "SDHCI1 VccQ";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <3300000>;
> + gpios = <&gpio0 171
> + GPIO_ACTIVE_HIGH>;
> + gpios-states = <1>;
> + states = <3300000 1>,
> + <1800000 0>;
> + };
> +
> + sdc@1e740000 {
> + compatible = "aspeed,ast2600-sd-controller";
> + reg = <0x1e740000 0x100>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges = <0 0x1e740000 0x20000>;
> + clocks = <&syscon ASPEED_CLK_GATE_SDCLK>;
> +
> + sdhci@1e740100 {
> + compatible = "aspeed,ast2600-sdhci","sdhci";
> + reg = <0x100 0x100>;
> + interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
> + sdhci,auto-cmd12;
> + clocks = <&syscon ASPEED_CLK_SDIO>;
> + vmmc-supply = <&vcc_sdhci0>;
> + vqmmc-supply = <&vccq_sdhci0>;
> + sd-uhs-sdr104;
> + clk-phase-uhs-sdr104 = <180>, <180>;
> + };
> +
> + sdhci@1e740200 {
> + compatible = "aspeed,ast2600-sdhci","sdhci";
> + reg = <0x200 0x100>;
> + interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
> + sdhci,auto-cmd12;
> + clocks = <&syscon ASPEED_CLK_SDIO>;
> + vmmc-supply = <&vcc_sdhci1>;
> + vqmmc-supply = <&vccq_sdhci1>;
> + sd-uhs-sdr104;
> + clk-phase-uhs-sdr104 = <0>, <0>;
> + };
> + };
> +
> +...
> --
> 2.17.1
>
Hi Steven,
I have some minor comments. I expect you're going to do a v4 of the
series, so if you'd like to clean them up in the process I'd appreciate
it.
However, from a pragmatic standpoint I think the patch is in good shape.
On Thu, 6 May 2021, at 19:33, Steven Lee wrote:
> The patch add a new function aspeed_sdc_set_slot_capability() for
> updating sdhci capability register.
The commit message should explain why the patch is necessary and not
what it does, as what it does is contained in the diff.
It's okay to explain *how* the patch acheives its goals if the
implementation is subtle or complex.
Maybe the commit message could be something like:
```
Configure the SDHCIs as specified by the devicetree.
The hardware provides capability configuration registers for each SDHCI
in the global configuration space for the SD controller. Writes to the
global capability registers are mirrored to the capability registers in
the associated SDHCI. Configuration of the capabilities must be written
through the mirror registers prior to initialisation of the SDHCI.
```
>
> Signed-off-by: Steven Lee <[email protected]>
> ---
> drivers/mmc/host/sdhci-of-aspeed.c | 57 ++++++++++++++++++++++++++++++
> 1 file changed, 57 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci-of-aspeed.c
> b/drivers/mmc/host/sdhci-of-aspeed.c
> index d001c51074a0..4979f98ffb52 100644
> --- a/drivers/mmc/host/sdhci-of-aspeed.c
> +++ b/drivers/mmc/host/sdhci-of-aspeed.c
> @@ -31,6 +31,11 @@
> #define ASPEED_SDC_S0_PHASE_OUT_EN GENMASK(1, 0)
> #define ASPEED_SDC_PHASE_MAX 31
>
> +/* SDIO{10,20} */
> +#define ASPEED_SDC_CAP1_1_8V (0 * 32 + 26)
> +/* SDIO{14,24} */
> +#define ASPEED_SDC_CAP2_SDR104 (1 * 32 + 1)
> +
> struct aspeed_sdc {
> struct clk *clk;
> struct resource *res;
> @@ -70,8 +75,42 @@ struct aspeed_sdhci {
> u32 width_mask;
> struct mmc_clk_phase_map phase_map;
> const struct aspeed_sdhci_phase_desc *phase_desc;
> +
> };
>
> +/*
> + * The function sets the mirror register for updating
> + * capbilities of the current slot.
> + *
> + * slot | capability | caps_reg | mirror_reg
> + * -----|-------------|----------|------------
> + * 0 | CAP1_1_8V | SDIO140 | SDIO10
> + * 0 | CAP2_SDR104 | SDIO144 | SDIO14
> + * 1 | CAP1_1_8V | SDIO240 | SDIO20
> + * 1 | CAP2_SDR104 | SDIO244 | SDIO24
It would be nice to align the columns to improve readability.
> + */
> +static void aspeed_sdc_set_slot_capability(struct sdhci_host *host,
> + struct aspeed_sdc *sdc,
> + int capability,
> + bool enable,
> + u8 slot)
I prefer we don't take up so much vertical space here. I think this
could be just a couple of lines with multiple variables per line. We
can go to 100 chars per line.
> +{
> + u8 cap_reg;
> + u32 mirror_reg_offset, cap_val;
The rest of the driver follows "reverse christmas tree" (longest to
shortest declaration) style, so I prefer we try to maintain consistency
where we can. Essentially, declare them in this order:
u32 mirror_reg_offset;
u32 cap_val;
u8 cap_reg;
> +
> + if (slot > 1)
> + return;
> +
> + cap_reg = capability / 32;
> + cap_val = sdhci_readl(host, 0x40 + (cap_reg * 4));
> + if (enable)
> + cap_val |= BIT(capability % 32);
> + else
> + cap_val &= ~BIT(capability % 32);
> + mirror_reg_offset = ((slot + 1) * 0x10) + (cap_reg * 4);
> + writel(cap_val, sdc->regs + mirror_reg_offset);
> +}
> +
> static void aspeed_sdc_configure_8bit_mode(struct aspeed_sdc *sdc,
> struct aspeed_sdhci *sdhci,
> bool bus8)
> @@ -329,6 +368,7 @@ static int aspeed_sdhci_probe(struct platform_device *pdev)
> {
> const struct aspeed_sdhci_pdata *aspeed_pdata;
> struct sdhci_pltfm_host *pltfm_host;
> + struct device_node *np = pdev->dev.of_node;
Again here with the reverse-christmas-tree style, so:
const struct aspeed_sdhci_pdata *aspeed_pdata;
struct device_node *np = pdev->dev.of_node;
struct sdhci_pltfm_host *pltfm_host;
...
> struct aspeed_sdhci *dev;
> struct sdhci_host *host;
> struct resource *res;
> @@ -372,6 +412,23 @@ static int aspeed_sdhci_probe(struct platform_device *pdev)
>
> sdhci_get_of_property(pdev);
>
> + if (of_property_read_bool(np, "mmc-hs200-1_8v") ||
> + of_property_read_bool(np, "sd-uhs-sdr104")) {
> + aspeed_sdc_set_slot_capability(host,
> + dev->parent,
> + ASPEED_SDC_CAP1_1_8V,
> + true,
> + slot);
Again, this would be nicer if we compress it to as few lines as possible.
> + }
> +
> + if (of_property_read_bool(np, "sd-uhs-sdr104")) {
> + aspeed_sdc_set_slot_capability(host,
> + dev->parent,
> + ASPEED_SDC_CAP2_SDR104,
> + true,
> + slot);
As above.
Cheers,
Andrew
On Fri, 7 May 2021, at 13:00, Steven Lee wrote:
> The 05/07/2021 09:40, Andrew Jeffery wrote:
> >
> >
> > On Thu, 6 May 2021, at 19:33, Steven Lee wrote:
> > > Add the description for describing the AST2600-A2 EVB reference design of
> > > GPIO regulators.
> > >
> > > Signed-off-by: Steven Lee <[email protected]>
> > > ---
> > > arch/arm/boot/dts/aspeed-ast2600-evb.dts | 15 +++++++++++++++
> > > 1 file changed, 15 insertions(+)
> > >
> > > diff --git a/arch/arm/boot/dts/aspeed-ast2600-evb.dts
> > > b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
> > > index 2772796e215e..1ae0facc3d5f 100644
> > > --- a/arch/arm/boot/dts/aspeed-ast2600-evb.dts
> > > +++ b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
> > > @@ -104,6 +104,21 @@
> > > status = "okay";
> > > };
> > >
> > > +/*
> > > + * The signal voltage of sdhci0 and sdhci1 on AST2600-A2 EVB is able to be
> > > + * toggled by GPIO pins.
> > > + * In the reference design, GPIOV0 of AST2600-A2 EVB is connected to the
> > > + * power load switch that providing 3.3v to sdhci0 vdd, GPIOV1 is connected to
> > > + * a 1.8v and a 3.3v power load switch that providing signal voltage to
> > > + * sdhci0 bus.
> > > + * If GPIOV0 is active high, sdhci0 is enabled, otherwise, sdhci0 is disabled.
> > > + * If GPIOV1 is active high, 3.3v power load switch is enabled, sdhci0 signal
> > > + * voltage is 3.3v, otherwise, 1.8v power load switch will be enabled,
> > > + * sdhci0 signal voltage becomes 1.8v.
> > > + * AST2600-A2 EVB also support toggling signal voltage for sdhci1.
> > > + * The design is the same as sdhci0, it uses GPIOV2 as power-gpio and GPIOV3
> > > + * as power-switch-gpio.
> > > + */
> >
> > Okay, I think the comment is in the right place, but I feel this patch
> > should also add the regulator nodes and hook them up to the SDHCIs.
> >
> > Given Rob isn't super keen on a second example in the binding document
> > I think you can just cut the example out and paste it in here.
> >
>
> Hi Andrew,
>
> Since AST2600-A0 and AST2600-A1 don't have regulators, do you mean cut
> the example from dt-binding and paste it to aspeed-ast2600-evb.dts but
> comment out the example?
If the board design varies with the silicon revision we should probably
have devicetrees that are appropriate for each, so an
aspeed-ast2600-evb-a2.dts
#include "aspeed-ast2600-evb.dts" at the top and go from there.
Cheers,
Andrew
The 05/07/2021 09:40, Andrew Jeffery wrote:
>
>
> On Thu, 6 May 2021, at 19:33, Steven Lee wrote:
> > Add the description for describing the AST2600-A2 EVB reference design of
> > GPIO regulators.
> >
> > Signed-off-by: Steven Lee <[email protected]>
> > ---
> > arch/arm/boot/dts/aspeed-ast2600-evb.dts | 15 +++++++++++++++
> > 1 file changed, 15 insertions(+)
> >
> > diff --git a/arch/arm/boot/dts/aspeed-ast2600-evb.dts
> > b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
> > index 2772796e215e..1ae0facc3d5f 100644
> > --- a/arch/arm/boot/dts/aspeed-ast2600-evb.dts
> > +++ b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
> > @@ -104,6 +104,21 @@
> > status = "okay";
> > };
> >
> > +/*
> > + * The signal voltage of sdhci0 and sdhci1 on AST2600-A2 EVB is able to be
> > + * toggled by GPIO pins.
> > + * In the reference design, GPIOV0 of AST2600-A2 EVB is connected to the
> > + * power load switch that providing 3.3v to sdhci0 vdd, GPIOV1 is connected to
> > + * a 1.8v and a 3.3v power load switch that providing signal voltage to
> > + * sdhci0 bus.
> > + * If GPIOV0 is active high, sdhci0 is enabled, otherwise, sdhci0 is disabled.
> > + * If GPIOV1 is active high, 3.3v power load switch is enabled, sdhci0 signal
> > + * voltage is 3.3v, otherwise, 1.8v power load switch will be enabled,
> > + * sdhci0 signal voltage becomes 1.8v.
> > + * AST2600-A2 EVB also support toggling signal voltage for sdhci1.
> > + * The design is the same as sdhci0, it uses GPIOV2 as power-gpio and GPIOV3
> > + * as power-switch-gpio.
> > + */
>
> Okay, I think the comment is in the right place, but I feel this patch
> should also add the regulator nodes and hook them up to the SDHCIs.
>
> Given Rob isn't super keen on a second example in the binding document
> I think you can just cut the example out and paste it in here.
>
Hi Andrew,
Since AST2600-A0 and AST2600-A1 don't have regulators, do you mean cut
the example from dt-binding and paste it to aspeed-ast2600-evb.dts but
comment out the example?
> Andrew
The 05/07/2021 09:13, Rob Herring wrote:
> On Thu, May 06, 2021 at 06:03:08PM +0800, Steven Lee wrote:
> > AST2600-A2 EVB has the reference design for enabling SD bus
> > power and toggling SD bus signal voltage by GPIO pins.
> >
> > In the reference design, GPIOV0 of AST2600-A2 EVB is connected to
> > power load switch that providing 3.3v to SD1 bus vdd. GPIOV1 is
> > connected to a 1.8v and a 3.3v power load switch that providing
> > signal voltage to
> > SD1 bus.
> >
> > If GPIOV0 is active high, SD1 bus is enabled. Otherwise, SD1 bus is
> > disabled.
> > If GPIOV1 is active high, 3.3v power load switch is enabled, SD1
> > signal voltage is 3.3v. Otherwise, 1.8v power load switch will be
> > enabled, SD1 signal voltage becomes 1.8v.
> >
> > AST2600-A2 EVB also support toggling signal voltage for SD2 bus.
> > The design is the same as SD1 bus. It uses GPIOV2 as power-gpio and
> > GPIOV3 as power-switch-gpio.
> >
> > Signed-off-by: Steven Lee <[email protected]>
> > ---
> > .../devicetree/bindings/mmc/aspeed,sdhci.yaml | 101 +++++++++++++++++-
> > 1 file changed, 97 insertions(+), 4 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > index 987b287f3bff..de7e61b3d37a 100644
> > --- a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > @@ -45,10 +45,16 @@ patternProperties:
> >
> > properties:
> > compatible:
> > - enum:
> > - - aspeed,ast2400-sdhci
> > - - aspeed,ast2500-sdhci
> > - - aspeed,ast2600-sdhci
> > + oneOf:
> > + - items:
> > + - enum:
> > + - aspeed,ast2400-sdhci
> > + - aspeed,ast2500-sdhci
> > + - aspeed,ast2600-sdhci
> > + - items:
> > + - enum:
> > + - aspeed,ast2600-sdhci
> > + - const: sdhci
>
> Why are you adding 'sdhci'. That's not useful as a compatible given how
> many quirks different implementations have.
>
>
It is for passing the dtbs_check of the second example.
Without this definition, many device trees have the following
error:
['aspeed,ast2600-sdhci', 'sdhci'] is too long
Additional items are not allowed ('sdhci' was unexpected)
Regardless, I will remove it, and move the new example to device
tree.
Thanks.
> > reg:
> > maxItems: 1
> > description: The SDHCI registers
> > @@ -104,3 +110,90 @@ examples:
> > clocks = <&syscon ASPEED_CLK_SDIO>;
> > };
> > };
> > +
> > + - |
>
> Why do we need another example?
>
The original example is for AST2500 which doesn't support UHS mode.
The new example teaches users how to enable sdhci with UHS mode, add
gpio regulators, and adjust clock phase for AST2600-A2.
I will move the new example to ast2600 device tree per the
review comment.
https://lkml.org/lkml/2021/5/6/968
> > + #include <dt-bindings/gpio/gpio.h>
> > + #include <dt-bindings/clock/aspeed-clock.h>
> > + #include <dt-bindings/interrupt-controller/irq.h>
> > + #include <dt-bindings/interrupt-controller/arm-gic.h>
> > + vcc_sdhci0: regulator-vcc-sdhci0 {
> > + compatible = "regulator-fixed";
> > + regulator-name = "SDHCI0 Vcc";
> > + regulator-min-microvolt = <3300000>;
> > + regulator-max-microvolt = <3300000>;
> > + gpios = <&gpio0 168
> > + GPIO_ACTIVE_HIGH>;
> > + enable-active-high;
> > + };
> > +
> > + vccq_sdhci0: regulator-vccq-sdhci0 {
> > + compatible = "regulator-gpio";
> > +
> > + regulator-name = "SDHCI0 VccQ";
> > + regulator-min-microvolt = <1800000>;
> > + regulator-max-microvolt = <3300000>;
> > + gpios = <&gpio0 169
> > + GPIO_ACTIVE_HIGH>;
> > + gpios-states = <1>;
> > + states = <3300000 1>,
> > + <1800000 0>;
> > + };
> > +
> > + vcc_sdhci1: regulator-vcc-sdhci1 {
> > + compatible = "regulator-fixed";
> > +
> > + regulator-name = "SDHCI1 Vcc";
> > + regulator-min-microvolt = <3300000>;
> > + regulator-max-microvolt = <3300000>;
> > + gpios = <&gpio0 170
> > + GPIO_ACTIVE_HIGH>;
> > + enable-active-high;
> > + };
> > +
> > + vccq_sdhci1: regulator-vccq-sdhci1 {
> > + compatible = "regulator-gpio";
> > +
> > + regulator-name = "SDHCI1 VccQ";
> > + regulator-min-microvolt = <1800000>;
> > + regulator-max-microvolt = <3300000>;
> > + gpios = <&gpio0 171
> > + GPIO_ACTIVE_HIGH>;
> > + gpios-states = <1>;
> > + states = <3300000 1>,
> > + <1800000 0>;
> > + };
> > +
> > + sdc@1e740000 {
> > + compatible = "aspeed,ast2600-sd-controller";
> > + reg = <0x1e740000 0x100>;
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > + ranges = <0 0x1e740000 0x20000>;
> > + clocks = <&syscon ASPEED_CLK_GATE_SDCLK>;
> > +
> > + sdhci@1e740100 {
> > + compatible = "aspeed,ast2600-sdhci","sdhci";
> > + reg = <0x100 0x100>;
> > + interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
> > + sdhci,auto-cmd12;
> > + clocks = <&syscon ASPEED_CLK_SDIO>;
> > + vmmc-supply = <&vcc_sdhci0>;
> > + vqmmc-supply = <&vccq_sdhci0>;
> > + sd-uhs-sdr104;
> > + clk-phase-uhs-sdr104 = <180>, <180>;
> > + };
> > +
> > + sdhci@1e740200 {
> > + compatible = "aspeed,ast2600-sdhci","sdhci";
> > + reg = <0x200 0x100>;
> > + interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
> > + sdhci,auto-cmd12;
> > + clocks = <&syscon ASPEED_CLK_SDIO>;
> > + vmmc-supply = <&vcc_sdhci1>;
> > + vqmmc-supply = <&vccq_sdhci1>;
> > + sd-uhs-sdr104;
> > + clk-phase-uhs-sdr104 = <0>, <0>;
> > + };
> > + };
> > +
> > +...
> > --
> > 2.17.1
> >
The 05/07/2021 10:13, Andrew Jeffery wrote:
> Hi Steven,
>
> I have some minor comments. I expect you're going to do a v4 of the
> series, so if you'd like to clean them up in the process I'd appreciate
> it.
>
Yes, I am going to prepare v4 patch for meeting reviewer's expectation
including your comment in this patch.
I've learned a lot from your suggestion for driver upstream.
Many thanks!
> However, from a pragmatic standpoint I think the patch is in good shape.
>
> On Thu, 6 May 2021, at 19:33, Steven Lee wrote:
> > The patch add a new function aspeed_sdc_set_slot_capability() for
> > updating sdhci capability register.
>
> The commit message should explain why the patch is necessary and not
> what it does, as what it does is contained in the diff.
>
> It's okay to explain *how* the patch acheives its goals if the
> implementation is subtle or complex.
>
> Maybe the commit message could be something like:
>
>
> ```
> Configure the SDHCIs as specified by the devicetree.
>
> The hardware provides capability configuration registers for each SDHCI
> in the global configuration space for the SD controller. Writes to the
> global capability registers are mirrored to the capability registers in
> the associated SDHCI. Configuration of the capabilities must be written
> through the mirror registers prior to initialisation of the SDHCI.
> ```
>
Thanks for the exmaple, I will modify my commit message.
> >
> > Signed-off-by: Steven Lee <[email protected]>
> > ---
> > drivers/mmc/host/sdhci-of-aspeed.c | 57 ++++++++++++++++++++++++++++++
> > 1 file changed, 57 insertions(+)
> >
> > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c
> > b/drivers/mmc/host/sdhci-of-aspeed.c
> > index d001c51074a0..4979f98ffb52 100644
> > --- a/drivers/mmc/host/sdhci-of-aspeed.c
> > +++ b/drivers/mmc/host/sdhci-of-aspeed.c
> > @@ -31,6 +31,11 @@
> > #define ASPEED_SDC_S0_PHASE_OUT_EN GENMASK(1, 0)
> > #define ASPEED_SDC_PHASE_MAX 31
> >
> > +/* SDIO{10,20} */
> > +#define ASPEED_SDC_CAP1_1_8V (0 * 32 + 26)
> > +/* SDIO{14,24} */
> > +#define ASPEED_SDC_CAP2_SDR104 (1 * 32 + 1)
> > +
> > struct aspeed_sdc {
> > struct clk *clk;
> > struct resource *res;
> > @@ -70,8 +75,42 @@ struct aspeed_sdhci {
> > u32 width_mask;
> > struct mmc_clk_phase_map phase_map;
> > const struct aspeed_sdhci_phase_desc *phase_desc;
> > +
> > };
> >
> > +/*
> > + * The function sets the mirror register for updating
> > + * capbilities of the current slot.
> > + *
> > + * slot | capability | caps_reg | mirror_reg
> > + * -----|-------------|----------|------------
> > + * 0 | CAP1_1_8V | SDIO140 | SDIO10
> > + * 0 | CAP2_SDR104 | SDIO144 | SDIO14
> > + * 1 | CAP1_1_8V | SDIO240 | SDIO20
> > + * 1 | CAP2_SDR104 | SDIO244 | SDIO24
>
> It would be nice to align the columns to improve readability.
>
Columns seems are aligned in my mail client(mutt) and my editor(vim).
I paste the above comment in Notepad++, columns are aligned as well.
> > +static void aspeed_sdc_set_slot_capability(struct sdhci_host *host,
> > + struct aspeed_sdc *sdc,
> > + int capability,
> > + bool enable,
> > + u8 slot)
>
> I prefer we don't take up so much vertical space here. I think this
> could be just a couple of lines with multiple variables per line. We
> can go to 100 chars per line.
>
I will change the function as the follows:
static void aspeed_sdc_set_slot_capability(struct sdhci_host *host, struct aspeed_sdc *sdc,
int capability, bool enable, u8 slot)
> > +{
> > + u8 cap_reg;
> > + u32 mirror_reg_offset, cap_val;
>
> The rest of the driver follows "reverse christmas tree" (longest to
> shortest declaration) style, so I prefer we try to maintain consistency
> where we can. Essentially, declare them in this order:
>
> u32 mirror_reg_offset;
> u32 cap_val;
> u8 cap_reg;
>
Will modify it.
> > +
> > + if (slot > 1)
> > + return;
> > +
> > + cap_reg = capability / 32;
> > + cap_val = sdhci_readl(host, 0x40 + (cap_reg * 4));
> > + if (enable)
> > + cap_val |= BIT(capability % 32);
> > + else
> > + cap_val &= ~BIT(capability % 32);
> > + mirror_reg_offset = ((slot + 1) * 0x10) + (cap_reg * 4);
> > + writel(cap_val, sdc->regs + mirror_reg_offset);
> > +}
> > +
> > static void aspeed_sdc_configure_8bit_mode(struct aspeed_sdc *sdc,
> > struct aspeed_sdhci *sdhci,
> > bool bus8)
> > @@ -329,6 +368,7 @@ static int aspeed_sdhci_probe(struct platform_device *pdev)
> > {
> > const struct aspeed_sdhci_pdata *aspeed_pdata;
> > struct sdhci_pltfm_host *pltfm_host;
> > + struct device_node *np = pdev->dev.of_node;
>
> Again here with the reverse-christmas-tree style, so:
>
> const struct aspeed_sdhci_pdata *aspeed_pdata;
> struct device_node *np = pdev->dev.of_node;
> struct sdhci_pltfm_host *pltfm_host;
> ...
>
Will modify it.
> > struct aspeed_sdhci *dev;
> > struct sdhci_host *host;
> > struct resource *res;
> > @@ -372,6 +412,23 @@ static int aspeed_sdhci_probe(struct platform_device *pdev)
> >
> > sdhci_get_of_property(pdev);
> >
> > + if (of_property_read_bool(np, "mmc-hs200-1_8v") ||
> > + of_property_read_bool(np, "sd-uhs-sdr104")) {
> > + aspeed_sdc_set_slot_capability(host,
> > + dev->parent,
> > + ASPEED_SDC_CAP1_1_8V,
> > + true,
> > + slot);
>
> Again, this would be nicer if we compress it to as few lines as possible.
>
Will modify the function as follows:
aspeed_sdc_set_slot_capability(host, dev->parent, ASPEED_SDC_CAP1_1_8V, true, slot);
> > + }
> > +
> > + if (of_property_read_bool(np, "sd-uhs-sdr104")) {
> > + aspeed_sdc_set_slot_capability(host,
> > + dev->parent,
> > + ASPEED_SDC_CAP2_SDR104,
> > + true,
> > + slot);
>
> As above.
>
Will modify the function as follows:
aspeed_sdc_set_slot_capability(host, dev->parent, ASPEED_SDC_CAP2_SDR104,
true, slot);
> Cheers,
>
> Andrew
On Fri, 7 May 2021, at 16:29, Steven Lee wrote:
> The 05/07/2021 10:13, Andrew Jeffery wrote:
> > > +/*
> > > + * The function sets the mirror register for updating
> > > + * capbilities of the current slot.
> > > + *
> > > + * slot | capability | caps_reg | mirror_reg
> > > + * -----|-------------|----------|------------
> > > + * 0 | CAP1_1_8V | SDIO140 | SDIO10
> > > + * 0 | CAP2_SDR104 | SDIO144 | SDIO14
> > > + * 1 | CAP1_1_8V | SDIO240 | SDIO20
> > > + * 1 | CAP2_SDR104 | SDIO244 | SDIO24
> >
> > It would be nice to align the columns to improve readability.
> >
>
> Columns seems are aligned in my mail client(mutt) and my editor(vim).
> I paste the above comment in Notepad++, columns are aligned as well.
>
Ah, it's probably my mail client then. Sorry for the noise!
Andrew
On Thu, May 6, 2021 at 10:14 PM Steven Lee <[email protected]> wrote:
>
> The 05/07/2021 09:13, Rob Herring wrote:
> > On Thu, May 06, 2021 at 06:03:08PM +0800, Steven Lee wrote:
> > > AST2600-A2 EVB has the reference design for enabling SD bus
> > > power and toggling SD bus signal voltage by GPIO pins.
> > >
> > > In the reference design, GPIOV0 of AST2600-A2 EVB is connected to
> > > power load switch that providing 3.3v to SD1 bus vdd. GPIOV1 is
> > > connected to a 1.8v and a 3.3v power load switch that providing
> > > signal voltage to
> > > SD1 bus.
> > >
> > > If GPIOV0 is active high, SD1 bus is enabled. Otherwise, SD1 bus is
> > > disabled.
> > > If GPIOV1 is active high, 3.3v power load switch is enabled, SD1
> > > signal voltage is 3.3v. Otherwise, 1.8v power load switch will be
> > > enabled, SD1 signal voltage becomes 1.8v.
> > >
> > > AST2600-A2 EVB also support toggling signal voltage for SD2 bus.
> > > The design is the same as SD1 bus. It uses GPIOV2 as power-gpio and
> > > GPIOV3 as power-switch-gpio.
> > >
> > > Signed-off-by: Steven Lee <[email protected]>
> > > ---
> > > .../devicetree/bindings/mmc/aspeed,sdhci.yaml | 101 +++++++++++++++++-
> > > 1 file changed, 97 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > index 987b287f3bff..de7e61b3d37a 100644
> > > --- a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > @@ -45,10 +45,16 @@ patternProperties:
> > >
> > > properties:
> > > compatible:
> > > - enum:
> > > - - aspeed,ast2400-sdhci
> > > - - aspeed,ast2500-sdhci
> > > - - aspeed,ast2600-sdhci
> > > + oneOf:
> > > + - items:
> > > + - enum:
> > > + - aspeed,ast2400-sdhci
> > > + - aspeed,ast2500-sdhci
> > > + - aspeed,ast2600-sdhci
> > > + - items:
> > > + - enum:
> > > + - aspeed,ast2600-sdhci
> > > + - const: sdhci
> >
> > Why are you adding 'sdhci'. That's not useful as a compatible given how
> > many quirks different implementations have.
> >
> >
>
> It is for passing the dtbs_check of the second example.
> Without this definition, many device trees have the following
> error:
>
> ['aspeed,ast2600-sdhci', 'sdhci'] is too long
> Additional items are not allowed ('sdhci' was unexpected)
I would probably fix the dts files then. Does anything depend on 'sdhci'?
Rob
The 05/08/2021 01:21, Rob Herring wrote:
> On Thu, May 6, 2021 at 10:14 PM Steven Lee <[email protected]> wrote:
> >
> > The 05/07/2021 09:13, Rob Herring wrote:
> > > On Thu, May 06, 2021 at 06:03:08PM +0800, Steven Lee wrote:
> > > > AST2600-A2 EVB has the reference design for enabling SD bus
> > > > power and toggling SD bus signal voltage by GPIO pins.
> > > >
> > > > In the reference design, GPIOV0 of AST2600-A2 EVB is connected to
> > > > power load switch that providing 3.3v to SD1 bus vdd. GPIOV1 is
> > > > connected to a 1.8v and a 3.3v power load switch that providing
> > > > signal voltage to
> > > > SD1 bus.
> > > >
> > > > If GPIOV0 is active high, SD1 bus is enabled. Otherwise, SD1 bus is
> > > > disabled.
> > > > If GPIOV1 is active high, 3.3v power load switch is enabled, SD1
> > > > signal voltage is 3.3v. Otherwise, 1.8v power load switch will be
> > > > enabled, SD1 signal voltage becomes 1.8v.
> > > >
> > > > AST2600-A2 EVB also support toggling signal voltage for SD2 bus.
> > > > The design is the same as SD1 bus. It uses GPIOV2 as power-gpio and
> > > > GPIOV3 as power-switch-gpio.
> > > >
> > > > Signed-off-by: Steven Lee <[email protected]>
> > > > ---
> > > > .../devicetree/bindings/mmc/aspeed,sdhci.yaml | 101 +++++++++++++++++-
> > > > 1 file changed, 97 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > > index 987b287f3bff..de7e61b3d37a 100644
> > > > --- a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > > +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > > @@ -45,10 +45,16 @@ patternProperties:
> > > >
> > > > properties:
> > > > compatible:
> > > > - enum:
> > > > - - aspeed,ast2400-sdhci
> > > > - - aspeed,ast2500-sdhci
> > > > - - aspeed,ast2600-sdhci
> > > > + oneOf:
> > > > + - items:
> > > > + - enum:
> > > > + - aspeed,ast2400-sdhci
> > > > + - aspeed,ast2500-sdhci
> > > > + - aspeed,ast2600-sdhci
> > > > + - items:
> > > > + - enum:
> > > > + - aspeed,ast2600-sdhci
> > > > + - const: sdhci
> > >
> > > Why are you adding 'sdhci'. That's not useful as a compatible given how
> > > many quirks different implementations have.
> > >
> > >
> >
> > It is for passing the dtbs_check of the second example.
> > Without this definition, many device trees have the following
> > error:
> >
> > ['aspeed,ast2600-sdhci', 'sdhci'] is too long
> > Additional items are not allowed ('sdhci' was unexpected)
>
> I would probably fix the dts files then. Does anything depend on 'sdhci'?
>
The build error is caused by my second example.
My example is refer to the aspeed-g6.dtsi to append "sdhci" after
"aspeed,ast2600-sdhci"
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/aspeed-g6.dtsi?h=v5.12#n561
As long as the second example is removed, the following error won't show in the dtbs_check result.
```
SCHEMA Documentation/devicetree/bindings/processed-schema.json
DTC arch/arm/boot/dts/aspeed-ast2500-evb.dt.yaml
CHECK arch/arm/boot/dts/aspeed-ast2500-evb.dt.yaml
DTC arch/arm/boot/dts/aspeed-ast2600-evb.dt.yaml
CHECK arch/arm/boot/dts/aspeed-ast2600-evb.dt.yaml
/home/slee/aspeed/patch_up/linux/arch/arm/boot/dts/aspeed-ast2600-evb.dt.yaml: sdc@1e740000: sdhci@1e740100:compatible: ['aspeed,ast2600-sdhci', 'sdhci'] is too long
From schema: /home/slee/aspeed/patch_up/linux/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
/home/slee/aspeed/patch_up/linux/arch/arm/boot/dts/aspeed-ast2600-evb.dt.yaml: sdc@1e740000: sdhci@1e740100:compatible: Additional items are not allowed ('sdhci' was unexpected)
From schema: /home/slee/aspeed/patch_up/linux/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
/home/slee/aspeed/patch_up/linux/arch/arm/boot/dts/aspeed-ast2600-evb.dt.yaml: sdc@1e740000: sdhci@1e740200:compatible: ['aspeed,ast2600-sdhci', 'sdhci'] is too long
From schema: /home/slee/aspeed/patch_up/linux/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
/home/slee/aspeed/patch_up/linux/arch/arm/boot/dts/aspeed-ast2600-evb.dt.yaml: sdc@1e740000: sdhci@1e740200:compatible: Additional items are not allowed ('sdhci' was unexpected)
From schema: /home/slee/aspeed/patch_up/linux/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
```
> Rob