[PATCH 0/8] Support SD/SDIO controllers on RK3528

[PATCH 0/8] Support SD/SDIO controllers on RK3528

[Yao Zi]

RK3528 features two SDIO controllers and one SD/MMC controller. This
series adds essential support for their tuning clocks, document the
controller in dt-bindings and bring the SD/MMC one up on Radxa E20C
board with pinctrl set up by the previous bootloader. Both HS and SDR104
mode are verified.

Yao Zi (8):
  dt-bindings: soc: rockchip: Add RK3528 VO GRF syscon
  dt-bindings: soc: rockchip: Add RK3528 VPU GRF syscon
  dt-bindings: mmc: rockchip-dw-mshc: Add compatible string for RK3528
  dt-bindings: clock: Add GRF clock definition for RK3528
  clk: rockchip: Support MMC clocks in GRF region
  clk: rockchip: rk3528: Add SD/SDIO tuning clocks in GRF region
  arm64: dts: rockchip: Add SDMMC/SDIO controllers for RK3528
  arm64: dts: rockchip: Enable SD-card interface on Radxa E20C

 .../bindings/mmc/rockchip-dw-mshc.yaml        |  1 +
 .../devicetree/bindings/soc/rockchip/grf.yaml |  2 +
 .../boot/dts/rockchip/rk3528-radxa-e20c.dts   | 14 +++++
 arch/arm64/boot/dts/rockchip/rk3528.dtsi      | 62 +++++++++++++++++++
 drivers/clk/rockchip/clk-mmc-phase.c          | 24 +++++--
 drivers/clk/rockchip/clk-rk3528.c             | 56 +++++++++++++++--
 drivers/clk/rockchip/clk.c                    | 42 +++++++++++++
 drivers/clk/rockchip/clk.h                    | 23 ++++++-
 .../dt-bindings/clock/rockchip,rk3528-cru.h   |  6 ++
 9 files changed, 220 insertions(+), 10 deletions(-)

-- 
2.48.1

[PATCH 1/8] dt-bindings: soc: rockchip: Add RK3528 VO GRF syscon

[Yao Zi]

Add compatible string for VO GRF found on RK3528 SoC.

Signed-off-by: Yao Zi <ziyao@disroot.org>
---
 Documentation/devicetree/bindings/soc/rockchip/grf.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/soc/rockchip/grf.yaml b/Documentation/devicetree/bindings/soc/rockchip/grf.yaml
index 61f38b68a4a3..7936be79159e 100644
--- a/Documentation/devicetree/bindings/soc/rockchip/grf.yaml
+++ b/Documentation/devicetree/bindings/soc/rockchip/grf.yaml
@@ -15,6 +15,7 @@ properties:
       - items:
           - enum:
               - rockchip,rk3288-sgrf
+              - rockchip,rk3528-vo-grf
               - rockchip,rk3566-pipe-grf
               - rockchip,rk3568-pcie3-phy-grf
               - rockchip,rk3568-pipe-grf
-- 
2.48.1

Re: [PATCH 1/8] dt-bindings: soc: rockchip: Add RK3528 VO GRF syscon

[Rob Herring (Arm)]

On Sat, 01 Mar 2025 10:42:43 +0000, Yao Zi wrote:
> Add compatible string for VO GRF found on RK3528 SoC.
> 
> Signed-off-by: Yao Zi <ziyao@disroot.org>
> ---
>  Documentation/devicetree/bindings/soc/rockchip/grf.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 

Acked-by: Rob Herring (Arm) <robh@kernel.org>

[PATCH 2/8] dt-bindings: soc: rockchip: Add RK3528 VPU GRF syscon

[Yao Zi]

Add compatible string for VPU GRF found on RK3528 SoC.

Signed-off-by: Yao Zi <ziyao@disroot.org>
---
 Documentation/devicetree/bindings/soc/rockchip/grf.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/soc/rockchip/grf.yaml b/Documentation/devicetree/bindings/soc/rockchip/grf.yaml
index 7936be79159e..d43c3f7e74fa 100644
--- a/Documentation/devicetree/bindings/soc/rockchip/grf.yaml
+++ b/Documentation/devicetree/bindings/soc/rockchip/grf.yaml
@@ -16,6 +16,7 @@ properties:
           - enum:
               - rockchip,rk3288-sgrf
               - rockchip,rk3528-vo-grf
+              - rockchip,rk3528-vpu-grf
               - rockchip,rk3566-pipe-grf
               - rockchip,rk3568-pcie3-phy-grf
               - rockchip,rk3568-pipe-grf
-- 
2.48.1

Re: [PATCH 2/8] dt-bindings: soc: rockchip: Add RK3528 VPU GRF syscon

[Rob Herring (Arm)]

On Sat, 01 Mar 2025 10:42:44 +0000, Yao Zi wrote:
> Add compatible string for VPU GRF found on RK3528 SoC.
> 
> Signed-off-by: Yao Zi <ziyao@disroot.org>
> ---
>  Documentation/devicetree/bindings/soc/rockchip/grf.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 

Acked-by: Rob Herring (Arm) <robh@kernel.org>

[PATCH 3/8] dt-bindings: mmc: rockchip-dw-mshc: Add compatible string for RK3528

[Yao Zi]

Add RK3528 compatible string for SD/SDIO interface.

Signed-off-by: Yao Zi <ziyao@disroot.org>
---
 Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.yaml b/Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.yaml
index 06df1269f247..ea0feb733e32 100644
--- a/Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.yaml
+++ b/Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.yaml
@@ -38,6 +38,7 @@ properties:
               - rockchip,rk3328-dw-mshc
               - rockchip,rk3368-dw-mshc
               - rockchip,rk3399-dw-mshc
+              - rockchip,rk3528-dw-mshc
               - rockchip,rk3568-dw-mshc
               - rockchip,rk3588-dw-mshc
               - rockchip,rv1108-dw-mshc
-- 
2.48.1

Re: [PATCH 3/8] dt-bindings: mmc: rockchip-dw-mshc: Add compatible string for RK3528

[Rob Herring (Arm)]

On Sat, 01 Mar 2025 10:42:45 +0000, Yao Zi wrote:
> Add RK3528 compatible string for SD/SDIO interface.
> 
> Signed-off-by: Yao Zi <ziyao@disroot.org>
> ---
>  Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 

Acked-by: Rob Herring (Arm) <robh@kernel.org>

[PATCH 4/8] dt-bindings: clock: Add GRF clock definition for RK3528

[Yao Zi]

These clocks are for SD/SDIO tuning purpose and come with registers
in GRF syscon.

Signed-off-by: Yao Zi <ziyao@disroot.org>
---
 include/dt-bindings/clock/rockchip,rk3528-cru.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/dt-bindings/clock/rockchip,rk3528-cru.h b/include/dt-bindings/clock/rockchip,rk3528-cru.h
index 55a448f5ed6d..0245a53fc334 100644
--- a/include/dt-bindings/clock/rockchip,rk3528-cru.h
+++ b/include/dt-bindings/clock/rockchip,rk3528-cru.h
@@ -414,6 +414,12 @@
 #define MCLK_I2S2_2CH_SAI_SRC_PRE	402
 #define MCLK_I2S3_8CH_SAI_SRC_PRE	403
 #define MCLK_SDPDIF_SRC_PRE		404
+#define SCLK_SDMMC_DRV			405
+#define SCLK_SDMMC_SAMPLE		406
+#define SCLK_SDIO0_DRV			407
+#define SCLK_SDIO0_SAMPLE		408
+#define SCLK_SDIO1_DRV			409
+#define SCLK_SDIO1_SAMPLE		410
 
 /* scmi-clocks indices */
 #define SCMI_PCLK_KEYREADER		0
-- 
2.48.1

[PATCH 5/8] clk: rockchip: Support MMC clocks in GRF region

[Yao Zi]

Registers of MMC drive/sample clocks in Rockchip RV1106 and RK3528
locate in GRF regions. Adjust MMC clock code to support register
operations through regmap. Also add a helper to ease registration of GRF
clocks.

Signed-off-by: Yao Zi <ziyao@disroot.org>
---
 drivers/clk/rockchip/clk-mmc-phase.c | 24 +++++++++++++---
 drivers/clk/rockchip/clk.c           | 42 ++++++++++++++++++++++++++++
 drivers/clk/rockchip/clk.h           | 20 ++++++++++++-
 3 files changed, 81 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/rockchip/clk-mmc-phase.c b/drivers/clk/rockchip/clk-mmc-phase.c
index 91012078681b..b3ed8e7523e5 100644
--- a/drivers/clk/rockchip/clk-mmc-phase.c
+++ b/drivers/clk/rockchip/clk-mmc-phase.c
@@ -9,11 +9,14 @@
 #include <linux/clk-provider.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
+#include <linux/regmap.h>
 #include "clk.h"
 
 struct rockchip_mmc_clock {
 	struct clk_hw	hw;
 	void __iomem	*reg;
+	struct regmap	*grf;
+	int		grf_reg;
 	int		shift;
 	int		cached_phase;
 	struct notifier_block clk_rate_change_nb;
@@ -54,7 +57,12 @@ static int rockchip_mmc_get_phase(struct clk_hw *hw)
 	if (!rate)
 		return 0;
 
-	raw_value = readl(mmc_clock->reg) >> (mmc_clock->shift);
+	if (mmc_clock->grf)
+		regmap_read(mmc_clock->grf, mmc_clock->grf_reg, &raw_value);
+	else
+		raw_value = readl(mmc_clock->reg);
+
+	raw_value >>= mmc_clock->shift;
 
 	degrees = (raw_value & ROCKCHIP_MMC_DEGREE_MASK) * 90;
 
@@ -134,8 +142,12 @@ static int rockchip_mmc_set_phase(struct clk_hw *hw, int degrees)
 	raw_value = delay_num ? ROCKCHIP_MMC_DELAY_SEL : 0;
 	raw_value |= delay_num << ROCKCHIP_MMC_DELAYNUM_OFFSET;
 	raw_value |= nineties;
-	writel(HIWORD_UPDATE(raw_value, 0x07ff, mmc_clock->shift),
-	       mmc_clock->reg);
+	raw_value = HIWORD_UPDATE(raw_value, 0x07ff, mmc_clock->shift);
+
+	if (mmc_clock->grf)
+		regmap_write(mmc_clock->grf, mmc_clock->grf_reg, raw_value);
+	else
+		writel(raw_value, mmc_clock->reg);
 
 	pr_debug("%s->set_phase(%d) delay_nums=%u reg[0x%p]=0x%03x actual_degrees=%d\n",
 		clk_hw_get_name(hw), degrees, delay_num,
@@ -189,7 +201,9 @@ static int rockchip_mmc_clk_rate_notify(struct notifier_block *nb,
 
 struct clk *rockchip_clk_register_mmc(const char *name,
 				const char *const *parent_names, u8 num_parents,
-				void __iomem *reg, int shift)
+				void __iomem *reg,
+				struct regmap *grf, int grf_reg,
+				int shift)
 {
 	struct clk_init_data init;
 	struct rockchip_mmc_clock *mmc_clock;
@@ -208,6 +222,8 @@ struct clk *rockchip_clk_register_mmc(const char *name,
 
 	mmc_clock->hw.init = &init;
 	mmc_clock->reg = reg;
+	mmc_clock->grf = grf;
+	mmc_clock->grf_reg = grf_reg;
 	mmc_clock->shift = shift;
 
 	clk = clk_register(NULL, &mmc_clock->hw);
diff --git a/drivers/clk/rockchip/clk.c b/drivers/clk/rockchip/clk.c
index cbf93ea119a9..ce2f3323d84e 100644
--- a/drivers/clk/rockchip/clk.c
+++ b/drivers/clk/rockchip/clk.c
@@ -590,6 +590,7 @@ void rockchip_clk_register_branches(struct rockchip_clk_provider *ctx,
 				list->name,
 				list->parent_names, list->num_parents,
 				ctx->reg_base + list->muxdiv_offset,
+				NULL, 0,
 				list->div_shift
 			);
 			break;
@@ -619,6 +620,11 @@ void rockchip_clk_register_branches(struct rockchip_clk_provider *ctx,
 			break;
 		case branch_linked_gate:
 			/* must be registered late, fall-through for error message */
+		case branch_mmc_grf:
+			/*
+			 * must be registered through rockchip_clk_register_grf_branches,
+			 * fall-through for error message
+			 */
 			break;
 		}
 
@@ -665,6 +671,42 @@ void rockchip_clk_register_late_branches(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(rockchip_clk_register_late_branches);
 
+void rockchip_clk_register_grf_branches(struct rockchip_clk_provider *ctx,
+					struct rockchip_clk_branch *list,
+					struct regmap *grf,
+					unsigned int nr_clk)
+{
+	unsigned int idx;
+	struct clk *clk;
+
+	for (idx = 0; idx < nr_clk; idx++, list++) {
+		clk = NULL;
+
+		switch (list->branch_type) {
+		case branch_mmc_grf:
+			clk = rockchip_clk_register_mmc(
+				list->name,
+				list->parent_names, list->num_parents,
+				NULL,
+				grf, list->muxdiv_offset,
+				list->div_shift
+			);
+			break;
+		default:
+			pr_err("%s: unknown clock type %d\n",
+			       __func__, list->branch_type);
+			break;
+		}
+
+		if (!clk)
+			pr_err("%s: failed to register clock %s: %ld\n",
+			       __func__, list->name, PTR_ERR(clk));
+		else
+			rockchip_clk_set_lookup(ctx, clk, list->id);
+	}
+}
+EXPORT_SYMBOL_GPL(rockchip_clk_register_grf_branches);
+
 void rockchip_clk_register_armclk(struct rockchip_clk_provider *ctx,
 				  unsigned int lookup_id,
 				  const char *name, const char *const *parent_names,
diff --git a/drivers/clk/rockchip/clk.h b/drivers/clk/rockchip/clk.h
index b322d42dc879..5d82ec5facfa 100644
--- a/drivers/clk/rockchip/clk.h
+++ b/drivers/clk/rockchip/clk.h
@@ -555,7 +555,9 @@ struct clk *rockchip_clk_register_cpuclk(const char *name,
 
 struct clk *rockchip_clk_register_mmc(const char *name,
 				const char *const *parent_names, u8 num_parents,
-				void __iomem *reg, int shift);
+				void __iomem *reg,
+				struct regmap *grf, int grf_reg,
+				int shift);
 
 /*
  * DDRCLK flags, including method of setting the rate
@@ -594,6 +596,7 @@ enum rockchip_clk_branch_type {
 	branch_gate,
 	branch_linked_gate,
 	branch_mmc,
+	branch_mmc_grf,
 	branch_inverter,
 	branch_factor,
 	branch_ddrclk,
@@ -944,6 +947,17 @@ struct rockchip_clk_branch {
 		.div_shift	= shift,			\
 	}
 
+#define MMC_GRF(_id, cname, pname, offset, shift)		\
+	{							\
+		.id		= _id,				\
+		.branch_type	= branch_mmc_grf,		\
+		.name		= cname,			\
+		.parent_names	= (const char *[]){ pname },	\
+		.num_parents	= 1,				\
+		.muxdiv_offset	= offset,			\
+		.div_shift	= shift,			\
+	}
+
 #define INVERTER(_id, cname, pname, io, is, if)			\
 	{							\
 		.id		= _id,				\
@@ -1093,6 +1107,10 @@ void rockchip_clk_register_late_branches(struct device *dev,
 					 struct rockchip_clk_provider *ctx,
 					 struct rockchip_clk_branch *list,
 					 unsigned int nr_clk);
+void rockchip_clk_register_grf_branches(struct rockchip_clk_provider *ctx,
+					struct rockchip_clk_branch *list,
+					struct regmap *grf,
+					unsigned int nr_clk);
 void rockchip_clk_register_plls(struct rockchip_clk_provider *ctx,
 				struct rockchip_pll_clock *pll_list,
 				unsigned int nr_pll, int grf_lock_offset);
-- 
2.48.1

[PATCH 6/8] clk: rockchip: rk3528: Add SD/SDIO tuning clocks in GRF region

[Yao Zi]

These clocks locate in VO and VPU GRF, serving for SD/SDIO controller
tuning purpose. Add their definitions and register them in driver if
corresponding GRF is available.

GRFs are looked up by compatible to simplify devicetree binding.

Signed-off-by: Yao Zi <ziyao@disroot.org>
---
 drivers/clk/rockchip/clk-rk3528.c | 56 ++++++++++++++++++++++++++++---
 drivers/clk/rockchip/clk.h        |  3 ++
 2 files changed, 54 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/rockchip/clk-rk3528.c b/drivers/clk/rockchip/clk-rk3528.c
index b8b577b902a0..b89440dd7448 100644
--- a/drivers/clk/rockchip/clk-rk3528.c
+++ b/drivers/clk/rockchip/clk-rk3528.c
@@ -10,6 +10,7 @@
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
+#include <linux/mfd/syscon.h>
 
 #include <dt-bindings/clock/rockchip,rk3528-cru.h>
 
@@ -1061,23 +1062,62 @@ static struct rockchip_clk_branch rk3528_clk_branches[] __initdata = {
 			0, 1, 1),
 };
 
+static struct rockchip_clk_branch rk3528_vo_clk_branches[] __initdata = {
+	MMC_GRF(SCLK_SDMMC_DRV, "sdmmc_drv", "cclk_src_sdmmc0",
+			RK3528_SDMMC_CON(0), 1),
+	MMC_GRF(SCLK_SDMMC_SAMPLE, "sdmmc_sample", "cclk_src_sdmmc0",
+			RK3528_SDMMC_CON(1), 1),
+};
+
+static struct rockchip_clk_branch rk3528_vpu_clk_branches[] __initdata = {
+	MMC_GRF(SCLK_SDIO0_DRV, "sdio0_drv", "cclk_src_sdio0",
+			RK3528_SDIO0_CON(0), 1),
+	MMC_GRF(SCLK_SDIO0_SAMPLE, "sdio0_sample", "cclk_src_sdio0",
+			RK3528_SDIO0_CON(1), 1),
+	MMC_GRF(SCLK_SDIO1_DRV, "sdio1_drv", "cclk_src_sdio1",
+			RK3528_SDIO1_CON(0), 1),
+	MMC_GRF(SCLK_SDIO1_SAMPLE, "sdio1_sample", "cclk_src_sdio1",
+			RK3528_SDIO1_CON(1), 1),
+};
+
 static int __init clk_rk3528_probe(struct platform_device *pdev)
 {
+	unsigned long nr_vpu_branches = ARRAY_SIZE(rk3528_vpu_clk_branches);
+	unsigned long nr_vo_branches = ARRAY_SIZE(rk3528_vo_clk_branches);
+	unsigned long nr_branches = ARRAY_SIZE(rk3528_clk_branches);
 	struct rockchip_clk_provider *ctx;
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
-	unsigned long nr_branches = ARRAY_SIZE(rk3528_clk_branches);
-	unsigned long nr_clks;
+	struct regmap *vo_grf, *vpu_grf;
 	void __iomem *reg_base;
-
-	nr_clks = rockchip_clk_find_max_clk_id(rk3528_clk_branches,
-					       nr_branches) + 1;
+	unsigned long nr_clks;
 
 	reg_base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(reg_base))
 		return dev_err_probe(dev, PTR_ERR(reg_base),
 				     "could not map cru region");
 
+	nr_clks = rockchip_clk_find_max_clk_id(rk3528_clk_branches,
+					       nr_branches) + 1;
+
+	vo_grf = syscon_regmap_lookup_by_compatible("rockchip,rk3528-vo-grf");
+	if (!IS_ERR(vo_grf))
+		nr_clks = MAX(rockchip_clk_find_max_clk_id(rk3528_vo_clk_branches,
+							   nr_vo_branches) + 1,
+			      nr_clks);
+	else if (PTR_ERR(vo_grf) != ENODEV)
+		return dev_err_probe(dev, PTR_ERR(vo_grf),
+				     "failed to look up VO GRF\n");
+
+	vpu_grf = syscon_regmap_lookup_by_compatible("rockchip,rk3528-vpu-grf");
+	if (!IS_ERR(vpu_grf))
+		nr_clks = MAX(rockchip_clk_find_max_clk_id(rk3528_vpu_clk_branches,
+							   nr_vpu_branches) + 1,
+			      nr_clks);
+	else if (PTR_ERR(vpu_grf) != ENODEV)
+		return dev_err_probe(dev, PTR_ERR(vpu_grf),
+				     "failed to look up VPU GRF\n");
+
 	ctx = rockchip_clk_init(np, reg_base, nr_clks);
 	if (IS_ERR(ctx))
 		return dev_err_probe(dev, PTR_ERR(ctx),
@@ -1091,6 +1131,12 @@ static int __init clk_rk3528_probe(struct platform_device *pdev)
 				     &rk3528_cpuclk_data, rk3528_cpuclk_rates,
 				     ARRAY_SIZE(rk3528_cpuclk_rates));
 	rockchip_clk_register_branches(ctx, rk3528_clk_branches, nr_branches);
+	if (!IS_ERR(vo_grf))
+		rockchip_clk_register_grf_branches(ctx, rk3528_vo_clk_branches,
+						   vo_grf, nr_vo_branches);
+	if (!IS_ERR(vpu_grf))
+		rockchip_clk_register_grf_branches(ctx, rk3528_vpu_clk_branches,
+						   vpu_grf, nr_vpu_branches);
 
 	rk3528_rst_init(np, reg_base);
 
diff --git a/drivers/clk/rockchip/clk.h b/drivers/clk/rockchip/clk.h
index 5d82ec5facfa..02d4ca0012d7 100644
--- a/drivers/clk/rockchip/clk.h
+++ b/drivers/clk/rockchip/clk.h
@@ -217,6 +217,9 @@ struct clk;
 #define RK3528_CLKSEL_CON(x)		((x) * 0x4 + 0x300)
 #define RK3528_CLKGATE_CON(x)		((x) * 0x4 + 0x800)
 #define RK3528_SOFTRST_CON(x)		((x) * 0x4 + 0xa00)
+#define RK3528_SDMMC_CON(x)		((x) * 0x4 + 0x24)
+#define RK3528_SDIO0_CON(x)		((x) * 0x4 + 0x4)
+#define RK3528_SDIO1_CON(x)		((x) * 0x4 + 0xc)
 #define RK3528_PMU_CLKSEL_CON(x)	((x) * 0x4 + 0x300 + RK3528_PMU_CRU_BASE)
 #define RK3528_PMU_CLKGATE_CON(x)	((x) * 0x4 + 0x800 + RK3528_PMU_CRU_BASE)
 #define RK3528_PCIE_CLKSEL_CON(x)	((x) * 0x4 + 0x300 + RK3528_PCIE_CRU_BASE)
-- 
2.48.1

Re: [PATCH 6/8] clk: rockchip: rk3528: Add SD/SDIO tuning clocks in GRF region

[Chukun Pan]

Hi,

> +	nr_clks = rockchip_clk_find_max_clk_id(rk3528_clk_branches,
> +					       nr_branches) + 1;
> +
> +	vo_grf = syscon_regmap_lookup_by_compatible("rockchip,rk3528-vo-grf");
> +	if (!IS_ERR(vo_grf))
> +		nr_clks = MAX(rockchip_clk_find_max_clk_id(rk3528_vo_clk_branches,
> +							   nr_vo_branches) + 1,

drivers/clk/rockchip/clk-rk3528.c: In function 'clk_rk3528_probe':
drivers/clk/rockchip/clk-rk3528.c:1105:27: error: implicit declaration of function 'MAX'; did you mean 'MUX'?
 1105 |                 nr_clks = MAX(rockchip_clk_find_max_clk_id(rk3528_vo_clk_branches,
      |                           ^~~
      |                           MUX

It seems that missing definition with older kernels.

Thanks,
Chukun

-- 
2.25.1

Re: [PATCH 6/8] clk: rockchip: rk3528: Add SD/SDIO tuning clocks in GRF region

[Heiko Stübner]

Hi,

Am Samstag, 1. März 2025, 11:47:24 MEZ schrieb Yao Zi:
> These clocks locate in VO and VPU GRF, serving for SD/SDIO controller
> tuning purpose. Add their definitions and register them in driver if
> corresponding GRF is available.

(no critique, just an observation :-) )

this puts a completely new meaning on the "general register files"
as dumping ground ;-) .

Whoever got the idea of making sdmm/sdio tuning controls part
of GRFs that are supposed display and/or video encoder parts :-D


> GRFs are looked up by compatible to simplify devicetree binding.
> 
> Signed-off-by: Yao Zi <ziyao@disroot.org>
> ---

>  static int __init clk_rk3528_probe(struct platform_device *pdev)
>  {
> +	unsigned long nr_vpu_branches = ARRAY_SIZE(rk3528_vpu_clk_branches);
> +	unsigned long nr_vo_branches = ARRAY_SIZE(rk3528_vo_clk_branches);
> +	unsigned long nr_branches = ARRAY_SIZE(rk3528_clk_branches);
>  	struct rockchip_clk_provider *ctx;
>  	struct device *dev = &pdev->dev;
>  	struct device_node *np = dev->of_node;
> -	unsigned long nr_branches = ARRAY_SIZE(rk3528_clk_branches);
> -	unsigned long nr_clks;
> +	struct regmap *vo_grf, *vpu_grf;
>  	void __iomem *reg_base;
> -
> -	nr_clks = rockchip_clk_find_max_clk_id(rk3528_clk_branches,
> -					       nr_branches) + 1;
> +	unsigned long nr_clks;
>  
>  	reg_base = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(reg_base))
>  		return dev_err_probe(dev, PTR_ERR(reg_base),
>  				     "could not map cru region");
>  
> +	nr_clks = rockchip_clk_find_max_clk_id(rk3528_clk_branches,
> +					       nr_branches) + 1;
> +
> +	vo_grf = syscon_regmap_lookup_by_compatible("rockchip,rk3528-vo-grf");
> +	if (!IS_ERR(vo_grf))

for readability, please make this into something like
	if (!IS_ERR(vo_grf)) {
		nr_vo_clks = rockchip_clk_find_max_clk_id(rk3528_vo_clk_branches,
							   nr_vo_branches) + 1;
		nr_clks = max(nr_vo_clks, nr_clks);
	}

> +	else if (PTR_ERR(vo_grf) != ENODEV)
> +		return dev_err_probe(dev, PTR_ERR(vo_grf),
> +				     "failed to look up VO GRF\n");
> +
> +	vpu_grf = syscon_regmap_lookup_by_compatible("rockchip,rk3528-vpu-grf");
> +	if (!IS_ERR(vpu_grf))
> +		nr_clks = MAX(rockchip_clk_find_max_clk_id(rk3528_vpu_clk_branches,
> +							   nr_vpu_branches) + 1,
> +			      nr_clks);

same here please

> +	else if (PTR_ERR(vpu_grf) != ENODEV)
> +		return dev_err_probe(dev, PTR_ERR(vpu_grf),
> +				     "failed to look up VPU GRF\n");
> +
>  	ctx = rockchip_clk_init(np, reg_base, nr_clks);
>  	if (IS_ERR(ctx))
>  		return dev_err_probe(dev, PTR_ERR(ctx),

Thanks
Heiko

Re: [PATCH 6/8] clk: rockchip: rk3528: Add SD/SDIO tuning clocks in GRF region

[Yao Zi]

On Wed, Mar 05, 2025 at 11:21:48AM +0100, Heiko Stübner wrote:
> Hi,
> 
> Am Samstag, 1. März 2025, 11:47:24 MEZ schrieb Yao Zi:
> > These clocks locate in VO and VPU GRF, serving for SD/SDIO controller
> > tuning purpose. Add their definitions and register them in driver if
> > corresponding GRF is available.
> 
> (no critique, just an observation :-) )
> 
> this puts a completely new meaning on the "general register files"
> as dumping ground ;-) .
> 
> Whoever got the idea of making sdmm/sdio tuning controls part
> of GRFs that are supposed display and/or video encoder parts :-D

Yes, the register layout is quite weird. Additionally some USB2 phy
registers locate in VO GRF as well...

> 
> > GRFs are looked up by compatible to simplify devicetree binding.
> > 
> > Signed-off-by: Yao Zi <ziyao@disroot.org>
> > ---
> 
> >  static int __init clk_rk3528_probe(struct platform_device *pdev)
> >  {
> > +	unsigned long nr_vpu_branches = ARRAY_SIZE(rk3528_vpu_clk_branches);
> > +	unsigned long nr_vo_branches = ARRAY_SIZE(rk3528_vo_clk_branches);
> > +	unsigned long nr_branches = ARRAY_SIZE(rk3528_clk_branches);
> >  	struct rockchip_clk_provider *ctx;
> >  	struct device *dev = &pdev->dev;
> >  	struct device_node *np = dev->of_node;
> > -	unsigned long nr_branches = ARRAY_SIZE(rk3528_clk_branches);
> > -	unsigned long nr_clks;
> > +	struct regmap *vo_grf, *vpu_grf;
> >  	void __iomem *reg_base;
> > -
> > -	nr_clks = rockchip_clk_find_max_clk_id(rk3528_clk_branches,
> > -					       nr_branches) + 1;
> > +	unsigned long nr_clks;
> >  
> >  	reg_base = devm_platform_ioremap_resource(pdev, 0);
> >  	if (IS_ERR(reg_base))
> >  		return dev_err_probe(dev, PTR_ERR(reg_base),
> >  				     "could not map cru region");
> >  
> > +	nr_clks = rockchip_clk_find_max_clk_id(rk3528_clk_branches,
> > +					       nr_branches) + 1;
> > +
> > +	vo_grf = syscon_regmap_lookup_by_compatible("rockchip,rk3528-vo-grf");
> > +	if (!IS_ERR(vo_grf))
> 
> for readability, please make this into something like
> 	if (!IS_ERR(vo_grf)) {
> 		nr_vo_clks = rockchip_clk_find_max_clk_id(rk3528_vo_clk_branches,
> 							   nr_vo_branches) + 1;
> 		nr_clks = max(nr_vo_clks, nr_clks);
> 	}

Thanks for the suggestion, will take it.

> > +	else if (PTR_ERR(vo_grf) != ENODEV)
> > +		return dev_err_probe(dev, PTR_ERR(vo_grf),
> > +				     "failed to look up VO GRF\n");
> > +
> > +	vpu_grf = syscon_regmap_lookup_by_compatible("rockchip,rk3528-vpu-grf");
> > +	if (!IS_ERR(vpu_grf))
> > +		nr_clks = MAX(rockchip_clk_find_max_clk_id(rk3528_vpu_clk_branches,
> > +							   nr_vpu_branches) + 1,
> > +			      nr_clks);
> 
> same here please
> 
> > +	else if (PTR_ERR(vpu_grf) != ENODEV)
> > +		return dev_err_probe(dev, PTR_ERR(vpu_grf),
> > +				     "failed to look up VPU GRF\n");
> > +
> >  	ctx = rockchip_clk_init(np, reg_base, nr_clks);
> >  	if (IS_ERR(ctx))
> >  		return dev_err_probe(dev, PTR_ERR(ctx),
> 
> Thanks
> Heiko
> 

Cheers,
Yao Zi

[PATCH 7/8] arm64: dts: rockchip: Add SDMMC/SDIO controllers for RK3528

[Yao Zi]

RK3528 features two SDIO controllers and one SD/MMC controller, describe
them in devicetree. Since their sample and drive clocks are located in
the VO and VPU GRFs, corresponding syscons are added to make these
clocks available.

Signed-off-by: Yao Zi <ziyao@disroot.org>
---
 arch/arm64/boot/dts/rockchip/rk3528.dtsi | 62 ++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3528.dtsi b/arch/arm64/boot/dts/rockchip/rk3528.dtsi
index 5b334690356a..078c97fa1d9f 100644
--- a/arch/arm64/boot/dts/rockchip/rk3528.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3528.dtsi
@@ -7,6 +7,7 @@
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/interrupt-controller/irq.h>
 #include <dt-bindings/clock/rockchip,rk3528-cru.h>
+#include <dt-bindings/reset/rockchip,rk3528-cru.h>
 
 / {
 	compatible = "rockchip,rk3528";
@@ -122,6 +123,16 @@ gic: interrupt-controller@fed01000 {
 			#interrupt-cells = <3>;
 		};
 
+		vpu_grf: syscon@ff340000 {
+			compatible = "rockchip,rk3528-vpu-grf", "syscon";
+			reg = <0x0 0xff340000 0x0 0x8000>;
+		};
+
+		vo_grf: syscon@ff360000 {
+			compatible = "rockchip,rk3528-vo-grf", "syscon";
+			reg = <0x0 0xff360000 0x0 0x10000>;
+		};
+
 		cru: clock-controller@ff4a0000 {
 			compatible = "rockchip,rk3528-cru";
 			reg = <0x0 0xff4a0000 0x0 0x30000>;
@@ -251,5 +262,56 @@ uart7: serial@ffa28000 {
 			reg-shift = <2>;
 			status = "disabled";
 		};
+
+		sdio0: mmc@ffc10000 {
+			compatible = "rockchip,rk3528-dw-mshc",
+				     "rockchip,rk3288-dw-mshc";
+			reg = <0x0 0xffc10000 0x0 0x4000>;
+			clocks = <&cru HCLK_SDIO0>,
+				 <&cru CCLK_SRC_SDIO0>,
+				 <&cru SCLK_SDIO0_DRV>,
+				 <&cru SCLK_SDIO0_SAMPLE>;
+			clock-names = "biu", "ciu", "ciu-drive", "ciu-sample";
+			fifo-depth = <0x100>;
+			interrupts = <GIC_SPI 137 IRQ_TYPE_LEVEL_HIGH>;
+			max-frequency = <150000000>;
+			resets = <&cru SRST_H_SDIO0>;
+			reset-names = "reset";
+			status = "disabled";
+		};
+
+		sdio1: mmc@ffc20000 {
+			compatible = "rockchip,rk3528-dw-mshc",
+				     "rockchip,rk3288-dw-mshc";
+			reg = <0x0 0xffc20000 0x0 0x4000>;
+			clocks = <&cru HCLK_SDIO1>,
+				 <&cru CCLK_SRC_SDIO1>,
+				 <&cru SCLK_SDIO1_DRV>,
+				 <&cru SCLK_SDIO1_SAMPLE>;
+			clock-names = "biu", "ciu", "ciu-drive", "ciu-sample";
+			fifo-depth = <0x100>;
+			interrupts = <GIC_SPI 138 IRQ_TYPE_LEVEL_HIGH>;
+			max-frequency = <150000000>;
+			resets = <&cru SRST_H_SDIO1>;
+			reset-names = "reset";
+			status = "disabled";
+		};
+
+		sdmmc: mmc@ffc30000 {
+			compatible = "rockchip,rk3528-dw-mshc",
+				     "rockchip,rk3288-dw-mshc";
+			reg = <0x0 0xffc30000 0x0 0x4000>;
+			clocks = <&cru HCLK_SDMMC0>,
+				 <&cru CCLK_SRC_SDMMC0>,
+				 <&cru SCLK_SDMMC_DRV>,
+				 <&cru SCLK_SDMMC_SAMPLE>;
+			clock-names = "biu", "ciu", "ciu-drive", "ciu-sample";
+			fifo-depth = <0x100>;
+			interrupts = <GIC_SPI 133 IRQ_TYPE_LEVEL_HIGH>;
+			max-frequency = <150000000>;
+			resets = <&cru SRST_H_SDMMC0>;
+			reset-names = "reset";
+			status = "disabled";
+		};
 	};
 };
-- 
2.48.1

Re: [PATCH 7/8] arm64: dts: rockchip: Add SDMMC/SDIO controllers for RK3528

[Jonas Karlman]

Hi,

On 2025-03-01 11:47, Yao Zi wrote:
> RK3528 features two SDIO controllers and one SD/MMC controller, describe
> them in devicetree. Since their sample and drive clocks are located in
> the VO and VPU GRFs, corresponding syscons are added to make these
> clocks available.
> 
> Signed-off-by: Yao Zi <ziyao@disroot.org>
> ---
>  arch/arm64/boot/dts/rockchip/rk3528.dtsi | 62 ++++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3528.dtsi b/arch/arm64/boot/dts/rockchip/rk3528.dtsi
> index 5b334690356a..078c97fa1d9f 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3528.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3528.dtsi
> @@ -7,6 +7,7 @@
>  #include <dt-bindings/interrupt-controller/arm-gic.h>
>  #include <dt-bindings/interrupt-controller/irq.h>
>  #include <dt-bindings/clock/rockchip,rk3528-cru.h>
> +#include <dt-bindings/reset/rockchip,rk3528-cru.h>
>  
>  / {
>  	compatible = "rockchip,rk3528";
> @@ -122,6 +123,16 @@ gic: interrupt-controller@fed01000 {
>  			#interrupt-cells = <3>;
>  		};
>  
> +		vpu_grf: syscon@ff340000 {
> +			compatible = "rockchip,rk3528-vpu-grf", "syscon";

vpu_grf is also used for gmac1, so should possible be a "syscon",
"simple-mfd", or have I misunderstood when to use simple-mfd ?

> +			reg = <0x0 0xff340000 0x0 0x8000>;
> +		};
> +
> +		vo_grf: syscon@ff360000 {
> +			compatible = "rockchip,rk3528-vo-grf", "syscon";

similar here, vo_grf is also used for gmac0.

> +			reg = <0x0 0xff360000 0x0 0x10000>;
> +		};
> +
>  		cru: clock-controller@ff4a0000 {
>  			compatible = "rockchip,rk3528-cru";
>  			reg = <0x0 0xff4a0000 0x0 0x30000>;
> @@ -251,5 +262,56 @@ uart7: serial@ffa28000 {
>  			reg-shift = <2>;
>  			status = "disabled";
>  		};
> +
> +		sdio0: mmc@ffc10000 {
> +			compatible = "rockchip,rk3528-dw-mshc",
> +				     "rockchip,rk3288-dw-mshc";
> +			reg = <0x0 0xffc10000 0x0 0x4000>;
> +			clocks = <&cru HCLK_SDIO0>,
> +				 <&cru CCLK_SRC_SDIO0>,
> +				 <&cru SCLK_SDIO0_DRV>,
> +				 <&cru SCLK_SDIO0_SAMPLE>;
> +			clock-names = "biu", "ciu", "ciu-drive", "ciu-sample";
> +			fifo-depth = <0x100>;
> +			interrupts = <GIC_SPI 137 IRQ_TYPE_LEVEL_HIGH>;
> +			max-frequency = <150000000>;
> +			resets = <&cru SRST_H_SDIO0>;
> +			reset-names = "reset";
> +			status = "disabled";
> +		};
> +
> +		sdio1: mmc@ffc20000 {
> +			compatible = "rockchip,rk3528-dw-mshc",
> +				     "rockchip,rk3288-dw-mshc";
> +			reg = <0x0 0xffc20000 0x0 0x4000>;
> +			clocks = <&cru HCLK_SDIO1>,
> +				 <&cru CCLK_SRC_SDIO1>,
> +				 <&cru SCLK_SDIO1_DRV>,
> +				 <&cru SCLK_SDIO1_SAMPLE>;
> +			clock-names = "biu", "ciu", "ciu-drive", "ciu-sample";
> +			fifo-depth = <0x100>;
> +			interrupts = <GIC_SPI 138 IRQ_TYPE_LEVEL_HIGH>;
> +			max-frequency = <150000000>;
> +			resets = <&cru SRST_H_SDIO1>;
> +			reset-names = "reset";
> +			status = "disabled";
> +		};
> +
> +		sdmmc: mmc@ffc30000 {
> +			compatible = "rockchip,rk3528-dw-mshc",
> +				     "rockchip,rk3288-dw-mshc";
> +			reg = <0x0 0xffc30000 0x0 0x4000>;
> +			clocks = <&cru HCLK_SDMMC0>,
> +				 <&cru CCLK_SRC_SDMMC0>,
> +				 <&cru SCLK_SDMMC_DRV>,
> +				 <&cru SCLK_SDMMC_SAMPLE>;
> +			clock-names = "biu", "ciu", "ciu-drive", "ciu-sample";
> +			fifo-depth = <0x100>;
> +			interrupts = <GIC_SPI 133 IRQ_TYPE_LEVEL_HIGH>;
> +			max-frequency = <150000000>;
> +			resets = <&cru SRST_H_SDMMC0>;
> +			reset-names = "reset";

Suggest adding default pinctrl props here:

  pinctrl-names = "default";
  pinctrl-0 = <&sdmmc_bus4>, <&sdmmc_clk>, <&sdmmc_cmd>, <&sdmmc_det>;

And possible also for sdio0 and sdio1.

Regards,
Jonas

> +			status = "disabled";
> +		};
>  	};
>  };

Re: [PATCH 7/8] arm64: dts: rockchip: Add SDMMC/SDIO controllers for RK3528

[Heiko Stübner]

Hey Joas,

Am Samstag, 1. März 2025, 13:47:47 MEZ schrieb Jonas Karlman:
> On 2025-03-01 11:47, Yao Zi wrote:
> > RK3528 features two SDIO controllers and one SD/MMC controller, describe
> > them in devicetree. Since their sample and drive clocks are located in
> > the VO and VPU GRFs, corresponding syscons are added to make these
> > clocks available.
> > 
> > Signed-off-by: Yao Zi <ziyao@disroot.org>
> > ---
> >  arch/arm64/boot/dts/rockchip/rk3528.dtsi | 62 ++++++++++++++++++++++++
> >  1 file changed, 62 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3528.dtsi b/arch/arm64/boot/dts/rockchip/rk3528.dtsi
> > index 5b334690356a..078c97fa1d9f 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3528.dtsi
> > +++ b/arch/arm64/boot/dts/rockchip/rk3528.dtsi
> > @@ -7,6 +7,7 @@
> >  #include <dt-bindings/interrupt-controller/arm-gic.h>
> >  #include <dt-bindings/interrupt-controller/irq.h>
> >  #include <dt-bindings/clock/rockchip,rk3528-cru.h>
> > +#include <dt-bindings/reset/rockchip,rk3528-cru.h>
> >  
> >  / {
> >  	compatible = "rockchip,rk3528";
> > @@ -122,6 +123,16 @@ gic: interrupt-controller@fed01000 {
> >  			#interrupt-cells = <3>;
> >  		};
> >  
> > +		vpu_grf: syscon@ff340000 {
> > +			compatible = "rockchip,rk3528-vpu-grf", "syscon";
> 
> vpu_grf is also used for gmac1, so should possible be a "syscon",
> "simple-mfd", or have I misunderstood when to use simple-mfd ?

simple-mfd is needed when the additional device is completely contained
inside the particular syscon.

For example, the usb2phy0 on rk3588 is completely living inside the
usb2phy0-grf.

Similarly the power-domains are living inside the rk3588 pmugrf.
But the pmugrf also contains more stuff, so the power-domains are a
subset of the pmugrf.

Both of these above are a case for a simple-mfd.


Similarly, gmac1 on rk3588 is ethernet@fe1c0000 , so a completely separate
io-memory area, but references both the sysgrf as well as the php-grf
as syscons for additional settings.

So here the syscon does not need to be a simple-mfd.


Hope that helps a bit
Heiko

Re: [PATCH 7/8] arm64: dts: rockchip: Add SDMMC/SDIO controllers for RK3528

[Jonas Karlman]

Hi Heiko,

On 2025-03-01 13:55, Heiko Stübner wrote:
> Hey Joas,
> 
> Am Samstag, 1. März 2025, 13:47:47 MEZ schrieb Jonas Karlman:
>> On 2025-03-01 11:47, Yao Zi wrote:
>>> RK3528 features two SDIO controllers and one SD/MMC controller, describe
>>> them in devicetree. Since their sample and drive clocks are located in
>>> the VO and VPU GRFs, corresponding syscons are added to make these
>>> clocks available.
>>>
>>> Signed-off-by: Yao Zi <ziyao@disroot.org>
>>> ---
>>>  arch/arm64/boot/dts/rockchip/rk3528.dtsi | 62 ++++++++++++++++++++++++
>>>  1 file changed, 62 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3528.dtsi b/arch/arm64/boot/dts/rockchip/rk3528.dtsi
>>> index 5b334690356a..078c97fa1d9f 100644
>>> --- a/arch/arm64/boot/dts/rockchip/rk3528.dtsi
>>> +++ b/arch/arm64/boot/dts/rockchip/rk3528.dtsi
>>> @@ -7,6 +7,7 @@
>>>  #include <dt-bindings/interrupt-controller/arm-gic.h>
>>>  #include <dt-bindings/interrupt-controller/irq.h>
>>>  #include <dt-bindings/clock/rockchip,rk3528-cru.h>
>>> +#include <dt-bindings/reset/rockchip,rk3528-cru.h>
>>>  
>>>  / {
>>>  	compatible = "rockchip,rk3528";
>>> @@ -122,6 +123,16 @@ gic: interrupt-controller@fed01000 {
>>>  			#interrupt-cells = <3>;
>>>  		};
>>>  
>>> +		vpu_grf: syscon@ff340000 {
>>> +			compatible = "rockchip,rk3528-vpu-grf", "syscon";
>>
>> vpu_grf is also used for gmac1, so should possible be a "syscon",
>> "simple-mfd", or have I misunderstood when to use simple-mfd ?
> 
> simple-mfd is needed when the additional device is completely contained
> inside the particular syscon.
> 
> For example, the usb2phy0 on rk3588 is completely living inside the
> usb2phy0-grf.
> 
> Similarly the power-domains are living inside the rk3588 pmugrf.
> But the pmugrf also contains more stuff, so the power-domains are a
> subset of the pmugrf.
> 
> Both of these above are a case for a simple-mfd.
> 
> 
> Similarly, gmac1 on rk3588 is ethernet@fe1c0000 , so a completely separate
> io-memory area, but references both the sysgrf as well as the php-grf
> as syscons for additional settings.
> 
> So here the syscon does not need to be a simple-mfd.
> 
> 
> Hope that helps a bit

Thanks for this explanation, it helped me better understand the meaning
of simple-mfd :-)

Regards,
Jonas

> Heiko
> 
> 

Re: [PATCH 7/8] arm64: dts: rockchip: Add SDMMC/SDIO controllers for RK3528

[Yao Zi]

On Sat, Mar 01, 2025 at 01:47:47PM +0100, Jonas Karlman wrote:
> Hi,
> 
> On 2025-03-01 11:47, Yao Zi wrote:
> > RK3528 features two SDIO controllers and one SD/MMC controller, describe
> > them in devicetree. Since their sample and drive clocks are located in
> > the VO and VPU GRFs, corresponding syscons are added to make these
> > clocks available.
> > 
> > Signed-off-by: Yao Zi <ziyao@disroot.org>
> > ---
> >  arch/arm64/boot/dts/rockchip/rk3528.dtsi | 62 ++++++++++++++++++++++++
> >  1 file changed, 62 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3528.dtsi b/arch/arm64/boot/dts/rockchip/rk3528.dtsi
> > index 5b334690356a..078c97fa1d9f 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3528.dtsi
> > +++ b/arch/arm64/boot/dts/rockchip/rk3528.dtsi
> > @@ -7,6 +7,7 @@
> >  #include <dt-bindings/interrupt-controller/arm-gic.h>
> >  #include <dt-bindings/interrupt-controller/irq.h>
> >  #include <dt-bindings/clock/rockchip,rk3528-cru.h>
> > +#include <dt-bindings/reset/rockchip,rk3528-cru.h>
> >  
> >  / {
> >  	compatible = "rockchip,rk3528";
> > @@ -122,6 +123,16 @@ gic: interrupt-controller@fed01000 {
> >  			#interrupt-cells = <3>;
> >  		};
> >  
> > +		vpu_grf: syscon@ff340000 {
> > +			compatible = "rockchip,rk3528-vpu-grf", "syscon";
> 
> vpu_grf is also used for gmac1, so should possible be a "syscon",
> "simple-mfd", or have I misunderstood when to use simple-mfd ?

Just as Heiko explained, "simple-mfd" is only required when the child
nodes should be populated automatically. Here these two GRFs are only
referenced and have no child, thus "simple-mfd" compatible isn't useful.

> > +			reg = <0x0 0xff340000 0x0 0x8000>;
> > +		};
> > +
> > +		vo_grf: syscon@ff360000 {
> > +			compatible = "rockchip,rk3528-vo-grf", "syscon";
> 
> similar here, vo_grf is also used for gmac0.
> 
> > +			reg = <0x0 0xff360000 0x0 0x10000>;
> > +		};
> > +
> >  		cru: clock-controller@ff4a0000 {
> >  			compatible = "rockchip,rk3528-cru";
> >  			reg = <0x0 0xff4a0000 0x0 0x30000>;
> > @@ -251,5 +262,56 @@ uart7: serial@ffa28000 {
> >  			reg-shift = <2>;
> >  			status = "disabled";
> >  		};
> > +
> > +		sdio0: mmc@ffc10000 {
> > +			compatible = "rockchip,rk3528-dw-mshc",
> > +				     "rockchip,rk3288-dw-mshc";
> > +			reg = <0x0 0xffc10000 0x0 0x4000>;
> > +			clocks = <&cru HCLK_SDIO0>,
> > +				 <&cru CCLK_SRC_SDIO0>,
> > +				 <&cru SCLK_SDIO0_DRV>,
> > +				 <&cru SCLK_SDIO0_SAMPLE>;
> > +			clock-names = "biu", "ciu", "ciu-drive", "ciu-sample";
> > +			fifo-depth = <0x100>;
> > +			interrupts = <GIC_SPI 137 IRQ_TYPE_LEVEL_HIGH>;
> > +			max-frequency = <150000000>;
> > +			resets = <&cru SRST_H_SDIO0>;
> > +			reset-names = "reset";
> > +			status = "disabled";
> > +		};
> > +
> > +		sdio1: mmc@ffc20000 {
> > +			compatible = "rockchip,rk3528-dw-mshc",
> > +				     "rockchip,rk3288-dw-mshc";
> > +			reg = <0x0 0xffc20000 0x0 0x4000>;
> > +			clocks = <&cru HCLK_SDIO1>,
> > +				 <&cru CCLK_SRC_SDIO1>,
> > +				 <&cru SCLK_SDIO1_DRV>,
> > +				 <&cru SCLK_SDIO1_SAMPLE>;
> > +			clock-names = "biu", "ciu", "ciu-drive", "ciu-sample";
> > +			fifo-depth = <0x100>;
> > +			interrupts = <GIC_SPI 138 IRQ_TYPE_LEVEL_HIGH>;
> > +			max-frequency = <150000000>;
> > +			resets = <&cru SRST_H_SDIO1>;
> > +			reset-names = "reset";
> > +			status = "disabled";
> > +		};
> > +
> > +		sdmmc: mmc@ffc30000 {
> > +			compatible = "rockchip,rk3528-dw-mshc",
> > +				     "rockchip,rk3288-dw-mshc";
> > +			reg = <0x0 0xffc30000 0x0 0x4000>;
> > +			clocks = <&cru HCLK_SDMMC0>,
> > +				 <&cru CCLK_SRC_SDMMC0>,
> > +				 <&cru SCLK_SDMMC_DRV>,
> > +				 <&cru SCLK_SDMMC_SAMPLE>;
> > +			clock-names = "biu", "ciu", "ciu-drive", "ciu-sample";
> > +			fifo-depth = <0x100>;
> > +			interrupts = <GIC_SPI 133 IRQ_TYPE_LEVEL_HIGH>;
> > +			max-frequency = <150000000>;
> > +			resets = <&cru SRST_H_SDMMC0>;
> > +			reset-names = "reset";
> 
> Suggest adding default pinctrl props here:
> 
>   pinctrl-names = "default";
>   pinctrl-0 = <&sdmmc_bus4>, <&sdmmc_clk>, <&sdmmc_cmd>, <&sdmmc_det>;
> 
> And possible also for sdio0 and sdio1.
> 
> Regards,
> Jonas

It makes sense. As mentioned in the cover letter, I depended on the
bootloader to setup pinctrl, to minimize dependency of the series.

Will complete the pinctrl properties in next version.

> > +			status = "disabled";
> > +		};
> >  	};
> >  };
> 

Best regards,
Yao Zi

Re: [PATCH 7/8] arm64: dts: rockchip: Add SDMMC/SDIO controllers for RK3528

[Jonas Karlman]

Hi Yao Zi,

On 2025-03-01 14:33, Yao Zi wrote:
> On Sat, Mar 01, 2025 at 01:47:47PM +0100, Jonas Karlman wrote:
>> Hi,
>>
>> On 2025-03-01 11:47, Yao Zi wrote:
>>> RK3528 features two SDIO controllers and one SD/MMC controller, describe
>>> them in devicetree. Since their sample and drive clocks are located in
>>> the VO and VPU GRFs, corresponding syscons are added to make these
>>> clocks available.
>>>
>>> Signed-off-by: Yao Zi <ziyao@disroot.org>
>>> ---
>>>  arch/arm64/boot/dts/rockchip/rk3528.dtsi | 62 ++++++++++++++++++++++++
>>>  1 file changed, 62 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3528.dtsi b/arch/arm64/boot/dts/rockchip/rk3528.dtsi
>>> index 5b334690356a..078c97fa1d9f 100644
>>> --- a/arch/arm64/boot/dts/rockchip/rk3528.dtsi
>>> +++ b/arch/arm64/boot/dts/rockchip/rk3528.dtsi
>>> @@ -7,6 +7,7 @@
>>>  #include <dt-bindings/interrupt-controller/arm-gic.h>
>>>  #include <dt-bindings/interrupt-controller/irq.h>
>>>  #include <dt-bindings/clock/rockchip,rk3528-cru.h>
>>> +#include <dt-bindings/reset/rockchip,rk3528-cru.h>
>>>  
>>>  / {
>>>  	compatible = "rockchip,rk3528";
>>> @@ -122,6 +123,16 @@ gic: interrupt-controller@fed01000 {
>>>  			#interrupt-cells = <3>;
>>>  		};
>>>  
>>> +		vpu_grf: syscon@ff340000 {
>>> +			compatible = "rockchip,rk3528-vpu-grf", "syscon";
>>
>> vpu_grf is also used for gmac1, so should possible be a "syscon",
>> "simple-mfd", or have I misunderstood when to use simple-mfd ?
> 
> Just as Heiko explained, "simple-mfd" is only required when the child
> nodes should be populated automatically. Here these two GRFs are only
> referenced and have no child, thus "simple-mfd" compatible isn't useful.

Thanks for the explanations.

> 
>>> +			reg = <0x0 0xff340000 0x0 0x8000>;
>>> +		};
>>> +
>>> +		vo_grf: syscon@ff360000 {
>>> +			compatible = "rockchip,rk3528-vo-grf", "syscon";
>>
>> similar here, vo_grf is also used for gmac0.
>>
>>> +			reg = <0x0 0xff360000 0x0 0x10000>;
>>> +		};
>>> +
>>>  		cru: clock-controller@ff4a0000 {
>>>  			compatible = "rockchip,rk3528-cru";
>>>  			reg = <0x0 0xff4a0000 0x0 0x30000>;
>>> @@ -251,5 +262,56 @@ uart7: serial@ffa28000 {
>>>  			reg-shift = <2>;
>>>  			status = "disabled";
>>>  		};
>>> +
>>> +		sdio0: mmc@ffc10000 {
>>> +			compatible = "rockchip,rk3528-dw-mshc",
>>> +				     "rockchip,rk3288-dw-mshc";
>>> +			reg = <0x0 0xffc10000 0x0 0x4000>;
>>> +			clocks = <&cru HCLK_SDIO0>,
>>> +				 <&cru CCLK_SRC_SDIO0>,
>>> +				 <&cru SCLK_SDIO0_DRV>,
>>> +				 <&cru SCLK_SDIO0_SAMPLE>;
>>> +			clock-names = "biu", "ciu", "ciu-drive", "ciu-sample";
>>> +			fifo-depth = <0x100>;
>>> +			interrupts = <GIC_SPI 137 IRQ_TYPE_LEVEL_HIGH>;
>>> +			max-frequency = <150000000>;
>>> +			resets = <&cru SRST_H_SDIO0>;
>>> +			reset-names = "reset";
>>> +			status = "disabled";
>>> +		};
>>> +
>>> +		sdio1: mmc@ffc20000 {
>>> +			compatible = "rockchip,rk3528-dw-mshc",
>>> +				     "rockchip,rk3288-dw-mshc";
>>> +			reg = <0x0 0xffc20000 0x0 0x4000>;
>>> +			clocks = <&cru HCLK_SDIO1>,
>>> +				 <&cru CCLK_SRC_SDIO1>,
>>> +				 <&cru SCLK_SDIO1_DRV>,
>>> +				 <&cru SCLK_SDIO1_SAMPLE>;
>>> +			clock-names = "biu", "ciu", "ciu-drive", "ciu-sample";
>>> +			fifo-depth = <0x100>;
>>> +			interrupts = <GIC_SPI 138 IRQ_TYPE_LEVEL_HIGH>;
>>> +			max-frequency = <150000000>;
>>> +			resets = <&cru SRST_H_SDIO1>;
>>> +			reset-names = "reset";
>>> +			status = "disabled";
>>> +		};
>>> +
>>> +		sdmmc: mmc@ffc30000 {
>>> +			compatible = "rockchip,rk3528-dw-mshc",
>>> +				     "rockchip,rk3288-dw-mshc";
>>> +			reg = <0x0 0xffc30000 0x0 0x4000>;
>>> +			clocks = <&cru HCLK_SDMMC0>,
>>> +				 <&cru CCLK_SRC_SDMMC0>,
>>> +				 <&cru SCLK_SDMMC_DRV>,
>>> +				 <&cru SCLK_SDMMC_SAMPLE>;
>>> +			clock-names = "biu", "ciu", "ciu-drive", "ciu-sample";
>>> +			fifo-depth = <0x100>;
>>> +			interrupts = <GIC_SPI 133 IRQ_TYPE_LEVEL_HIGH>;
>>> +			max-frequency = <150000000>;
>>> +			resets = <&cru SRST_H_SDMMC0>;
>>> +			reset-names = "reset";
>>
>> Suggest adding default pinctrl props here:
>>
>>   pinctrl-names = "default";
>>   pinctrl-0 = <&sdmmc_bus4>, <&sdmmc_clk>, <&sdmmc_cmd>, <&sdmmc_det>;
>>
>> And possible also for sdio0 and sdio1.
>>
>> Regards,
>> Jonas
> 
> It makes sense. As mentioned in the cover letter, I depended on the
> bootloader to setup pinctrl, to minimize dependency of the series.

BootROM typically setup pinctrl for the storage media when probing for
idblock and mainline U-Boot will setup pinctrl based on the board device
tree synced from Linux. Adding pinctrl early in Linux will help avoid a
need for using workarounds in U-Boot.

For RK3528 there only seem to be one option for sdmmc/sdio pins, adding
a default to soc dtsi should help reduce duplication in future board
device trees.

> 
> Will complete the pinctrl properties in next version.

Thanks :-)

Regards,
Jonas

> 
>>> +			status = "disabled";
>>> +		};
>>>  	};
>>>  };
>>
> 
> Best regards,
> Yao Zi

[PATCH 8/8] arm64: dts: rockchip: Enable SD-card interface on Radxa E20C

[Yao Zi]

SD-card is available on Radxa E20C board.

Signed-off-by: Yao Zi <ziyao@disroot.org>
---
 arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts b/arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts
index d2cdb63d4a9d..473065aa4228 100644
--- a/arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts
@@ -12,6 +12,10 @@ / {
 	model = "Radxa E20C";
 	compatible = "radxa,e20c", "rockchip,rk3528";
 
+	aliases {
+		mmc0 = &sdmmc;
+	};
+
 	chosen {
 		stdout-path = "serial0:1500000n8";
 	};
@@ -20,3 +24,13 @@ chosen {
 &uart0 {
 	status = "okay";
 };
+
+&sdmmc {
+	bus-width = <4>;
+	cap-mmc-highspeed;
+	cap-sd-highspeed;
+	disable-wp;
+	rockchip,default-sample-phase = <90>;
+	sd-uhs-sdr104;
+	status = "okay";
+};
-- 
2.48.1

Re: [PATCH 8/8] arm64: dts: rockchip: Enable SD-card interface on Radxa E20C

[Jonas Karlman]

Hi,

On 2025-03-01 11:48, Yao Zi wrote:
> SD-card is available on Radxa E20C board.
> 
> Signed-off-by: Yao Zi <ziyao@disroot.org>
> ---
>  arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts b/arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts
> index d2cdb63d4a9d..473065aa4228 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts
> @@ -12,6 +12,10 @@ / {
>  	model = "Radxa E20C";
>  	compatible = "radxa,e20c", "rockchip,rk3528";
>  
> +	aliases {
> +		mmc0 = &sdmmc;

Suggest using mmc1 for sd-card because the e20c typically have onboard
emmc, compared to removable sd-card.

> +	};
> +
>  	chosen {
>  		stdout-path = "serial0:1500000n8";
>  	};
> @@ -20,3 +24,13 @@ chosen {
>  &uart0 {
>  	status = "okay";
>  };
> +
> +&sdmmc {
> +	bus-width = <4>;
> +	cap-mmc-highspeed;
> +	cap-sd-highspeed;
> +	disable-wp;
> +	rockchip,default-sample-phase = <90>;
> +	sd-uhs-sdr104;

Are you sure uhs-sdr104 works as is should? Vendor kernel use a
different "v2" tuning and this is also missing the vccio_sd vqmmc-supply
to switch between 3v3 and 1v8.

You could add following regulator for sdmmc:

	vccio_sd: regulator-vccio-sd {
		compatible = "regulator-gpio";
		gpios = <&gpio4 RK_PB6 GPIO_ACTIVE_HIGH>;
		pinctrl-names = "default";
		pinctrl-0 = <&sdmmc_vol_ctrl_h>;
		regulator-name = "vccio_sd";
		regulator-min-microvolt = <1800000>;
		regulator-max-microvolt = <3300000>;
		states = <1800000 0x0>, <3300000 0x1>;
	};

and following pinctrl:

	sdmmc {
		sdmmc_vol_ctrl_h: sdmmc-vol-ctrl-h {
			rockchip,pins = <4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_none>;
		};
	};

add then the power supplies to the sdmmc node:

	vmmc-supply = <&vcc_3v3>;
	vqmmc-supply = <&vccio_sd>;

That matches the schematics for e20c, and works when testing non-uhs modes.

Regards,
Jonas

> +	status = "okay";
> +};

Re: [PATCH 8/8] arm64: dts: rockchip: Enable SD-card interface on Radxa E20C

[Yao Zi]

On Sat, Mar 01, 2025 at 02:01:05PM +0100, Jonas Karlman wrote:
> Hi,
> 
> On 2025-03-01 11:48, Yao Zi wrote:
> > SD-card is available on Radxa E20C board.
> > 
> > Signed-off-by: Yao Zi <ziyao@disroot.org>
> > ---
> >  arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts b/arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts
> > index d2cdb63d4a9d..473065aa4228 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts
> > +++ b/arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts
> > @@ -12,6 +12,10 @@ / {
> >  	model = "Radxa E20C";
> >  	compatible = "radxa,e20c", "rockchip,rk3528";
> >  
> > +	aliases {
> > +		mmc0 = &sdmmc;
> 
> Suggest using mmc1 for sd-card because the e20c typically have onboard
> emmc, compared to removable sd-card.

My board doesn't have an eMMC: it's optional as well, but all variants
of Radxa E20C come with an SD-card interface. The vendor devicetree sets
sdmmc as mmc0 as well[1].

I won't insist on it and am willing to take the change if you still
consider mmc0 is better.

> > +	};
> > +
> >  	chosen {
> >  		stdout-path = "serial0:1500000n8";
> >  	};
> > @@ -20,3 +24,13 @@ chosen {
> >  &uart0 {
> >  	status = "okay";
> >  };
> > +
> > +&sdmmc {
> > +	bus-width = <4>;
> > +	cap-mmc-highspeed;
> > +	cap-sd-highspeed;
> > +	disable-wp;
> > +	rockchip,default-sample-phase = <90>;
> > +	sd-uhs-sdr104;
> 
> Are you sure uhs-sdr104 works as is should?

In fact yes, tuning succeeds at 148.5MHz and results in 66MB/s reading
speed.

> Vendor kernel use a different "v2" tuning

This isn't a problem. IMHO V2 tuning is more like a quick path, which
tries inheritting the phase from firmware and then re-tunes roughly.
Fine tunning is still a fallback here in case of failure, see the commit
message in the downstream kernel[2]. And testing proves it's okay for
RK3528 to issue fine-tuning always.

> and this is also missing the vccio_sd vqmmc-supply to switch between
> 3v3 and 1v8.

But this is a problem, thanks for catching it! Somehow my card managed
to run at 148.5MHz with 3v3 voltage level, but it's definitely a
compatiblity issue. I'm surprised that the driver doesn't complain when
switching to SDR modes without a regulator configured.

> You could add following regulator for sdmmc:
> 
> 	vccio_sd: regulator-vccio-sd {
> 		compatible = "regulator-gpio";
> 		gpios = <&gpio4 RK_PB6 GPIO_ACTIVE_HIGH>;
> 		pinctrl-names = "default";
> 		pinctrl-0 = <&sdmmc_vol_ctrl_h>;
> 		regulator-name = "vccio_sd";
> 		regulator-min-microvolt = <1800000>;
> 		regulator-max-microvolt = <3300000>;
> 		states = <1800000 0x0>, <3300000 0x1>;
> 	};
>
> and following pinctrl:
> 
> 	sdmmc {
> 		sdmmc_vol_ctrl_h: sdmmc-vol-ctrl-h {
> 			rockchip,pins = <4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_none>;
> 		};
> 	};
> 
> add then the power supplies to the sdmmc node:
> 
> 	vmmc-supply = <&vcc_3v3>;
> 	vqmmc-supply = <&vccio_sd>;
> 
> That matches the schematics for e20c, and works when testing non-uhs modes.

Thanks for the hints. Will rebase on your pinctrl series and get
regulators and pinctrl settings applied in the next version.

> Regards,
> Jonas
> 
> > +	status = "okay";
> > +};
> 

Cheers,
Yao Zi

[1]: https://github.com/radxa/kernel/blob/2b0c8de7dc4c68947cda206dcc2e457e9677e426/arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts#L22-L26
[2]: https://github.com/rockchip-linux/kernel/commit/795e052cc8610aa59a64b104f975cc4a45493d5d

Re: [PATCH 8/8] arm64: dts: rockchip: Enable SD-card interface on Radxa E20C

[Jonas Karlman]

Hi Yao Zi,

On 2025-03-01 16:15, Yao Zi wrote:
> On Sat, Mar 01, 2025 at 02:01:05PM +0100, Jonas Karlman wrote:
>> Hi,
>>
>> On 2025-03-01 11:48, Yao Zi wrote:
>>> SD-card is available on Radxa E20C board.
>>>
>>> Signed-off-by: Yao Zi <ziyao@disroot.org>
>>> ---
>>>  arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts | 14 ++++++++++++++
>>>  1 file changed, 14 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts b/arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts
>>> index d2cdb63d4a9d..473065aa4228 100644
>>> --- a/arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts
>>> +++ b/arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts
>>> @@ -12,6 +12,10 @@ / {
>>>  	model = "Radxa E20C";
>>>  	compatible = "radxa,e20c", "rockchip,rk3528";
>>>  
>>> +	aliases {
>>> +		mmc0 = &sdmmc;
>>
>> Suggest using mmc1 for sd-card because the e20c typically have onboard
>> emmc, compared to removable sd-card.
> 
> My board doesn't have an eMMC: it's optional as well, but all variants
> of Radxa E20C come with an SD-card interface. The vendor devicetree sets
> sdmmc as mmc0 as well[1].

This is strange as Radxa typically want to align with mmc0=emmc and
mmc1=sd-card, as seen in [3] and [4].

  Align with other Radxa products.
  - mmc0 is eMMC
  - mmc1 is microSD

Also mainline U-Boot for Rockchip SoCs typically always treat mmc0 as
emmc and mmc1 as sd-card, and for most SoCs it will even override the
board aliases to have some predictability across boards.

> 
> I won't insist on it and am willing to take the change if you still
> consider mmc0 is better.

Yes, my position is that we should use following:

  mmc0 = &sdhci;
  mmc1 = &sdmmc;

I will send out a short sdhci series based on top of v2 of this series.
Driver changes was not needed to get basic sdhci working on RK3528 and
is only required to get HS400 modes working.

[3] https://lore.kernel.org/r/20240620224435.2752-1-naoki@radxa.com
[4] https://lore.kernel.org/r/20240619050047.1217-2-naoki@radxa.com

> 
>>> +	};
>>> +
>>>  	chosen {
>>>  		stdout-path = "serial0:1500000n8";
>>>  	};
>>> @@ -20,3 +24,13 @@ chosen {
>>>  &uart0 {
>>>  	status = "okay";
>>>  };
>>> +
>>> +&sdmmc {

This node should be placed above &uart0 to be in alphabetical order.

>>> +	bus-width = <4>;
>>> +	cap-mmc-highspeed;
>>> +	cap-sd-highspeed;
>>> +	disable-wp;
>>> +	rockchip,default-sample-phase = <90>;
>>> +	sd-uhs-sdr104;
>>
>> Are you sure uhs-sdr104 works as is should?
> 
> In fact yes, tuning succeeds at 148.5MHz and results in 66MB/s reading
> speed.
> 
>> Vendor kernel use a different "v2" tuning
> 
> This isn't a problem. IMHO V2 tuning is more like a quick path, which
> tries inheritting the phase from firmware and then re-tunes roughly.
> Fine tunning is still a fallback here in case of failure, see the commit
> message in the downstream kernel[2]. And testing proves it's okay for
> RK3528 to issue fine-tuning always.

Thanks for this information, I did not inspect exactly what the v2
tuning meant, only observed that vendor kernel (incorrectly) used a DT
prop to indicate when v2 tuning should be used.

> 
>> and this is also missing the vccio_sd vqmmc-supply to switch between
>> 3v3 and 1v8.
> 
> But this is a problem, thanks for catching it! Somehow my card managed
> to run at 148.5MHz with 3v3 voltage level, but it's definitely a
> compatiblity issue. I'm surprised that the driver doesn't complain when
> switching to SDR modes without a regulator configured.
> 
>> You could add following regulator for sdmmc:
>>
>> 	vccio_sd: regulator-vccio-sd {
>> 		compatible = "regulator-gpio";
>> 		gpios = <&gpio4 RK_PB6 GPIO_ACTIVE_HIGH>;
>> 		pinctrl-names = "default";
>> 		pinctrl-0 = <&sdmmc_vol_ctrl_h>;
>> 		regulator-name = "vccio_sd";
>> 		regulator-min-microvolt = <1800000>;
>> 		regulator-max-microvolt = <3300000>;
>> 		states = <1800000 0x0>, <3300000 0x1>;

This should also have something like:

  vin-supply = <&vcc5v0_sys>;

>> 	};
>>
>> and following pinctrl:
>>
>> 	sdmmc {
>> 		sdmmc_vol_ctrl_h: sdmmc-vol-ctrl-h {
>> 			rockchip,pins = <4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_none>;
>> 		};
>> 	};
>>
>> add then the power supplies to the sdmmc node:
>>
>> 	vmmc-supply = <&vcc_3v3>;
>> 	vqmmc-supply = <&vccio_sd>;
>>
>> That matches the schematics for e20c, and works when testing non-uhs modes.
> 
> Thanks for the hints. Will rebase on your pinctrl series and get
> regulators and pinctrl settings applied in the next version.

Thanks :-)

Regards,
Jonas

> 
>> Regards,
>> Jonas
>>
>>> +	status = "okay";
>>> +};
>>
> 
> Cheers,
> Yao Zi
> 
> [1]: https://github.com/radxa/kernel/blob/2b0c8de7dc4c68947cda206dcc2e457e9677e426/arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts#L22-L26
> [2]: https://github.com/rockchip-linux/kernel/commit/795e052cc8610aa59a64b104f975cc4a45493d5d

Re: [PATCH 8/8] arm64: dts: rockchip: Enable SD-card interface on Radxa E20C

[Yao Zi]

On Sun, Mar 02, 2025 at 12:56:42PM +0100, Jonas Karlman wrote:
> Hi Yao Zi,
> 
> On 2025-03-01 16:15, Yao Zi wrote:
> > On Sat, Mar 01, 2025 at 02:01:05PM +0100, Jonas Karlman wrote:
> >> Hi,
> >>
> >> On 2025-03-01 11:48, Yao Zi wrote:
> >>> SD-card is available on Radxa E20C board.
> >>>
> >>> Signed-off-by: Yao Zi <ziyao@disroot.org>
> >>> ---
> >>>  arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts | 14 ++++++++++++++
> >>>  1 file changed, 14 insertions(+)
> >>>
> >>> diff --git a/arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts b/arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts
> >>> index d2cdb63d4a9d..473065aa4228 100644
> >>> --- a/arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts
> >>> +++ b/arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts
> >>> @@ -12,6 +12,10 @@ / {
> >>>  	model = "Radxa E20C";
> >>>  	compatible = "radxa,e20c", "rockchip,rk3528";
> >>>  
> >>> +	aliases {
> >>> +		mmc0 = &sdmmc;
> >>
> >> Suggest using mmc1 for sd-card because the e20c typically have onboard
> >> emmc, compared to removable sd-card.
> > 
> > My board doesn't have an eMMC: it's optional as well, but all variants
> > of Radxa E20C come with an SD-card interface. The vendor devicetree sets
> > sdmmc as mmc0 as well[1].
> 
> This is strange as Radxa typically want to align with mmc0=emmc and
> mmc1=sd-card, as seen in [3] and [4].
> 
>   Align with other Radxa products.
>   - mmc0 is eMMC
>   - mmc1 is microSD
> 
> Also mainline U-Boot for Rockchip SoCs typically always treat mmc0 as
> emmc and mmc1 as sd-card, and for most SoCs it will even override the
> board aliases to have some predictability across boards.
> 
> > 
> > I won't insist on it and am willing to take the change if you still
> > consider mmc0 is better.
> 
> Yes, my position is that we should use following:

Ack. I got your point but there's a typo (s/mmc0/mmc1) in my reply.

>   mmc0 = &sdhci;
>   mmc1 = &sdmmc;
> 
> I will send out a short sdhci series based on top of v2 of this series.
> Driver changes was not needed to get basic sdhci working on RK3528 and
> is only required to get HS400 modes working.
> 
> [3] https://lore.kernel.org/r/20240620224435.2752-1-naoki@radxa.com
> [4] https://lore.kernel.org/r/20240619050047.1217-2-naoki@radxa.com
> 
> > 
> >>> +	};
> >>> +
> >>>  	chosen {
> >>>  		stdout-path = "serial0:1500000n8";
> >>>  	};
> >>> @@ -20,3 +24,13 @@ chosen {
> >>>  &uart0 {
> >>>  	status = "okay";
> >>>  };
> >>> +
> >>> +&sdmmc {
> 
> This node should be placed above &uart0 to be in alphabetical order.
> 

The original patch keeps the order of nodes in the SoC devicetree
(sorted by MMIO address), but alphabetical order seems more common. Will
fix in v2, thanks.

> >>> +	bus-width = <4>;
> >>> +	cap-mmc-highspeed;
> >>> +	cap-sd-highspeed;
> >>> +	disable-wp;
> >>> +	rockchip,default-sample-phase = <90>;
> >>> +	sd-uhs-sdr104;
 
Thanks,
Yao Zi

Re: [PATCH 8/8] arm64: dts: rockchip: Enable SD-card interface on Radxa E20C

[Chukun Pan]

Hi,

> +	aliases {
> +		mmc0 = &sdmmc;

s/mmc0/mmc1

> +&sdmmc {
> +	bus-width = <4>;
> +	cap-mmc-highspeed;
> +	cap-sd-highspeed;

I think for sdcard, only cap-sd-highspeed
is needed, not cap-mmc-highspeed?

> +	disable-wp;

Missing pinctrl.

> +	rockchip,default-sample-phase = <90>;

It seems that all rk3528 devices need to set this
default phase, so maybe this can be placed in dtsi?

> +	sd-uhs-sdr104;

The rk3528 devices uses gpio to switch IO voltage, maybe
more modes should be added here like vendor kernel?
And these devices use 3.3V IO voltage by default.

	sd-uhs-sdr12;
	sd-uhs-sdr25;
	sd-uhs-sdr50;
	sd-uhs-sdr104;

Thanks,
Chukun

-- 
2.25.1

Re: [PATCH 8/8] arm64: dts: rockchip: Enable SD-card interface on Radxa E20C

[Yao Zi]

On Tue, Mar 04, 2025 at 08:10:36PM +0800, Chukun Pan wrote:
> Hi,
> 
> > +	aliases {
> > +		mmc0 = &sdmmc;
> 
> s/mmc0/mmc1

Will take it and add the missing pinctrl, as Jonas already pointed out.

> > +&sdmmc {
> > +	bus-width = <4>;
> > +	cap-mmc-highspeed;
> > +	cap-sd-highspeed;
> 
> I think for sdcard, only cap-sd-highspeed
> is needed, not cap-mmc-highspeed?

This makes sense, will remove it in the next version.

> > +	disable-wp;
> 
> Missing pinctrl.
>
> > +	rockchip,default-sample-phase = <90>;
> 
> It seems that all rk3528 devices need to set this
> default phase, so maybe this can be placed in dtsi?

Yes, since the tuned phase offset is a SoC-specific value, as pointed
out by comment in the driver,

	this is _not_ a value that is dynamically tuned and is also
	_not_ a value that will vary from board to board.  It is a value
	that could vary between different SoC models.

Will take it in the next version, thanks for finding it!

> > +	sd-uhs-sdr104;
> 
> The rk3528 devices uses gpio to switch IO voltage, maybe
> more modes should be added here like vendor kernel?

I cannot get the relationship between things you mentioned. For the
regulator, yes, here vqmmc-supply is missing, as already pointed out by
Jonas.

> And these devices use 3.3V IO voltage by default.
> 
> 	sd-uhs-sdr12;
> 	sd-uhs-sdr25;
> 	sd-uhs-sdr50;
> 	sd-uhs-sdr104;

But I don't think it's necessary to lay out these slower modes
explicitly, since SDR104 seems to imply them, see
sd_update_bus_speed_mode() in drivers/mmc/core/sd.c[1],

        if ((card->host->caps & MMC_CAP_UHS_SDR104) &&
            (card->sw_caps.sd3_bus_mode & SD_MODE_UHS_SDR104)) {
                        card->sd_bus_speed = UHS_SDR104_BUS_SPEED;
        } else if ((card->host->caps & MMC_CAP_UHS_DDR50) &&
                   (card->sw_caps.sd3_bus_mode & SD_MODE_UHS_DDR50)) {
                        card->sd_bus_speed = UHS_DDR50_BUS_SPEED;
        } else if ((card->host->caps & (MMC_CAP_UHS_SDR104 |
                    MMC_CAP_UHS_SDR50)) && (card->sw_caps.sd3_bus_mode &
                    SD_MODE_UHS_SDR50)) {
                        card->sd_bus_speed = UHS_SDR50_BUS_SPEED;
        } else if ((card->host->caps & (MMC_CAP_UHS_SDR104 |
                    MMC_CAP_UHS_SDR50 | MMC_CAP_UHS_SDR25)) &&
                   (card->sw_caps.sd3_bus_mode & SD_MODE_UHS_SDR25)) {
                        card->sd_bus_speed = UHS_SDR25_BUS_SPEED;
        } else if ((card->host->caps & (MMC_CAP_UHS_SDR104 |
                    MMC_CAP_UHS_SDR50 | MMC_CAP_UHS_SDR25 |
                    MMC_CAP_UHS_SDR12)) && (card->sw_caps.sd3_bus_mode &
                    SD_MODE_UHS_SDR12)) {
                        card->sd_bus_speed = UHS_SDR12_BUS_SPEED;
        }

> Thanks,
> Chukun
> 
> -- 
> 2.25.1
> 

Regards,
Yao Zi

[1]: https://elixir.bootlin.com/linux/v6.13.5/source/drivers/mmc/core/sd.c#L448-L479

Re: [PATCH 8/8] arm64: dts: rockchip: Enable SD-card interface on Radxa E20C

[Jonas Karlman]

Hi Yao Zi,

On 2025-03-04 20:49, Yao Zi wrote:
> On Tue, Mar 04, 2025 at 08:10:36PM +0800, Chukun Pan wrote:
>> Hi,
>>
>>> +	aliases {
>>> +		mmc0 = &sdmmc;
>>
>> s/mmc0/mmc1
> 
> Will take it and add the missing pinctrl, as Jonas already pointed out.
> 
>>> +&sdmmc {
>>> +	bus-width = <4>;
>>> +	cap-mmc-highspeed;
>>> +	cap-sd-highspeed;
>>
>> I think for sdcard, only cap-sd-highspeed
>> is needed, not cap-mmc-highspeed?
> 
> This makes sense, will remove it in the next version.

Please do not remove the cap-mmc-highspeed prop, I tested the controller
with a microSD to eMMC adapter and MMC HS speed is supported:

  mmc1: card 59b4 removed
  mmc_host mmc1: Bus speed (slot 0) = 400000Hz (slot req 400000Hz, actual 400000HZ div = 0)
  mmc_host mmc1: Bus speed (slot 0) = 49800000Hz (slot req 52000000Hz, actual 49800000HZ div = 0)
  mmc1: new high speed MMC card at address 0001
  mmcblk1: mmc1:0001 DG4008 7.28 GiB
   mmcblk1: p1 p2
  mmcblk1boot0: mmc1:0001 DG4008 4.00 MiB
  mmcblk1boot1: mmc1:0001 DG4008 4.00 MiB
  mmcblk1rpmb: mmc1:0001 DG4008 4.00 MiB, chardev (499:0)

  ~ # cat /sys/kernel/debug/mmc1/ios
  clock:          52000000 Hz
  vdd:            21 (3.3 ~ 3.4 V)
  bus mode:       2 (push-pull)
  chip select:    0 (don't care)
  power mode:     2 (on)
  bus width:      2 (4 bits)
  timing spec:    1 (mmc high-speed)
  signal voltage: 0 (3.30 V)
  driver type:    0 (driver type B)

Regards,
Jonas

> 
>>> +	disable-wp;
>>
>> Missing pinctrl.
>>
>>> +	rockchip,default-sample-phase = <90>;
>>
>> It seems that all rk3528 devices need to set this
>> default phase, so maybe this can be placed in dtsi?
> 
> Yes, since the tuned phase offset is a SoC-specific value, as pointed
> out by comment in the driver,
> 
> 	this is _not_ a value that is dynamically tuned and is also
> 	_not_ a value that will vary from board to board.  It is a value
> 	that could vary between different SoC models.
> 
> Will take it in the next version, thanks for finding it!
> 
>>> +	sd-uhs-sdr104;
>>
>> The rk3528 devices uses gpio to switch IO voltage, maybe
>> more modes should be added here like vendor kernel?
> 
> I cannot get the relationship between things you mentioned. For the
> regulator, yes, here vqmmc-supply is missing, as already pointed out by
> Jonas.
> 
>> And these devices use 3.3V IO voltage by default.
>>
>> 	sd-uhs-sdr12;
>> 	sd-uhs-sdr25;
>> 	sd-uhs-sdr50;
>> 	sd-uhs-sdr104;
> 
> But I don't think it's necessary to lay out these slower modes
> explicitly, since SDR104 seems to imply them, see
> sd_update_bus_speed_mode() in drivers/mmc/core/sd.c[1],
> 
>         if ((card->host->caps & MMC_CAP_UHS_SDR104) &&
>             (card->sw_caps.sd3_bus_mode & SD_MODE_UHS_SDR104)) {
>                         card->sd_bus_speed = UHS_SDR104_BUS_SPEED;
>         } else if ((card->host->caps & MMC_CAP_UHS_DDR50) &&
>                    (card->sw_caps.sd3_bus_mode & SD_MODE_UHS_DDR50)) {
>                         card->sd_bus_speed = UHS_DDR50_BUS_SPEED;
>         } else if ((card->host->caps & (MMC_CAP_UHS_SDR104 |
>                     MMC_CAP_UHS_SDR50)) && (card->sw_caps.sd3_bus_mode &
>                     SD_MODE_UHS_SDR50)) {
>                         card->sd_bus_speed = UHS_SDR50_BUS_SPEED;
>         } else if ((card->host->caps & (MMC_CAP_UHS_SDR104 |
>                     MMC_CAP_UHS_SDR50 | MMC_CAP_UHS_SDR25)) &&
>                    (card->sw_caps.sd3_bus_mode & SD_MODE_UHS_SDR25)) {
>                         card->sd_bus_speed = UHS_SDR25_BUS_SPEED;
>         } else if ((card->host->caps & (MMC_CAP_UHS_SDR104 |
>                     MMC_CAP_UHS_SDR50 | MMC_CAP_UHS_SDR25 |
>                     MMC_CAP_UHS_SDR12)) && (card->sw_caps.sd3_bus_mode &
>                     SD_MODE_UHS_SDR12)) {
>                         card->sd_bus_speed = UHS_SDR12_BUS_SPEED;
>         }
> 
>> Thanks,
>> Chukun
>>
>> -- 
>> 2.25.1
>>
> 
> Regards,
> Yao Zi
> 
> [1]: https://elixir.bootlin.com/linux/v6.13.5/source/drivers/mmc/core/sd.c#L448-L479

Re: [PATCH 8/8] arm64: dts: rockchip: Enable SD-card interface on Radxa E20C

[Yao Zi]

On Tue, Mar 04, 2025 at 08:55:36PM +0100, Jonas Karlman wrote:
> Hi Yao Zi,
> 
> On 2025-03-04 20:49, Yao Zi wrote:
> > On Tue, Mar 04, 2025 at 08:10:36PM +0800, Chukun Pan wrote:
> >> Hi,
> >>
> >>> +	aliases {
> >>> +		mmc0 = &sdmmc;
> >>
> >> s/mmc0/mmc1
> > 
> > Will take it and add the missing pinctrl, as Jonas already pointed out.
> > 
> >>> +&sdmmc {
> >>> +	bus-width = <4>;
> >>> +	cap-mmc-highspeed;
> >>> +	cap-sd-highspeed;
> >>
> >> I think for sdcard, only cap-sd-highspeed
> >> is needed, not cap-mmc-highspeed?
> > 
> > This makes sense, will remove it in the next version.
> 
> Please do not remove the cap-mmc-highspeed prop, I tested the controller
> with a microSD to eMMC adapter and MMC HS speed is supported:
> 
>   mmc1: card 59b4 removed
>   mmc_host mmc1: Bus speed (slot 0) = 400000Hz (slot req 400000Hz, actual 400000HZ div = 0)
>   mmc_host mmc1: Bus speed (slot 0) = 49800000Hz (slot req 52000000Hz, actual 49800000HZ div = 0)
>   mmc1: new high speed MMC card at address 0001
>   mmcblk1: mmc1:0001 DG4008 7.28 GiB
>    mmcblk1: p1 p2
>   mmcblk1boot0: mmc1:0001 DG4008 4.00 MiB
>   mmcblk1boot1: mmc1:0001 DG4008 4.00 MiB
>   mmcblk1rpmb: mmc1:0001 DG4008 4.00 MiB, chardev (499:0)
> 
>   ~ # cat /sys/kernel/debug/mmc1/ios
>   clock:          52000000 Hz
>   vdd:            21 (3.3 ~ 3.4 V)
>   bus mode:       2 (push-pull)
>   chip select:    0 (don't care)
>   power mode:     2 (on)
>   bus width:      2 (4 bits)
>   timing spec:    1 (mmc high-speed)
>   signal voltage: 0 (3.30 V)
>   driver type:    0 (driver type B)

Oops, indeed, I didn't expect the adapted usecase and thought only
sdcards could be connected through the interface.

> Regards,
> Jonas

Thanks for the correction,
Yao Zi