[PATCH v4 0/4] Add dw_mmc support for rk3576

[PATCH v4 0/4] Add dw_mmc support for rk3576

[Detlev Casanova]

The SD card controller on the rk3576 SoC stores the phase settings into
the dw_mmc controller, so the code has to be adapted to implement that.

Although the feature can be detected through the USRID register value, the
decision to use it is based on the compatible.

The compatible for rk3576 is added in its own group of compatible to mark
that all devices compatible with rk3576 have internal phase settings and
don't have the ciu-drive and ciu-sample clocks.

Changes since v3:
- Remove internal phase auto detection
- Set compatible in own block, with own dt_parse function
- Add internal_phase variable
- Use function to set clock parameters based on internal_phase variable
  instead of multiple ifs
- Use different commit for skipping phases higher than 270

Changes since v2:
- Drop rockchip,v2-tuning and use compatible-based detection
- Fix coding style

Changes since v1:
- Renamed use-v2-tuning to v2-tuning
- Rewrite v2-tuning description as the hardware feature

Detlev.

Detlev Casanova (2):
  dt-bindings: mmc: Add support for rk3576 dw-mshc
  mmc: dw_mmc-rockchip: Add support for rk3576 SoCs

Shawn Lin (2):
  mmc: dw_mmc-rockchip: Add internal phase support
  mmc: dw_mmc-rockchip: Skip all phases bigger than 270 degrees

 .../bindings/mmc/rockchip-dw-mshc.yaml        |   2 +
 drivers/mmc/host/dw_mmc-rockchip.c            | 220 ++++++++++++++++--
 2 files changed, 207 insertions(+), 15 deletions(-)

-- 
2.46.0

[PATCH v4 1/4] dt-bindings: mmc: Add support for rk3576 dw-mshc

[Detlev Casanova]

Add the compatible string for rockchip,rk3576-dw-mshc in its own new
block, for devices that have internal pahse settings instead of external
clocks.

Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
---
 Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.yaml b/Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.yaml
index 211cd0b0bc5f..06df1269f247 100644
--- a/Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.yaml
+++ b/Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.yaml
@@ -43,6 +43,8 @@ properties:
               - rockchip,rv1108-dw-mshc
               - rockchip,rv1126-dw-mshc
           - const: rockchip,rk3288-dw-mshc
+      # for Rockchip RK3576 with phase tuning inside the controller
+      - const: rockchip,rk3576-dw-mshc
 
   reg:
     maxItems: 1
-- 
2.46.0

[PATCH v4 2/4] mmc: dw_mmc-rockchip: Add internal phase support

[Detlev Casanova]

From: Shawn Lin <shawn.lin@rock-chips.com>

Some Rockchip devices put the phase settings into the dw_mmc controller.

When the feature is present, the ciu-drive and ciu-sample clocks are
not used and the phase configuration is done directly through the mmc
controller.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
Acked-by: Shawn Lin <shawn.lin@rock-chips.com>
---
 drivers/mmc/host/dw_mmc-rockchip.c | 171 +++++++++++++++++++++++++++--
 1 file changed, 160 insertions(+), 11 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c
index b07190ba4b7a..2748f9bf2691 100644
--- a/drivers/mmc/host/dw_mmc-rockchip.c
+++ b/drivers/mmc/host/dw_mmc-rockchip.c
@@ -15,7 +15,17 @@
 #include "dw_mmc.h"
 #include "dw_mmc-pltfm.h"
 
-#define RK3288_CLKGEN_DIV	2
+#define RK3288_CLKGEN_DIV		2
+#define SDMMC_TIMING_CON0		0x130
+#define SDMMC_TIMING_CON1		0x134
+#define ROCKCHIP_MMC_DELAY_SEL		BIT(10)
+#define ROCKCHIP_MMC_DEGREE_MASK	0x3
+#define ROCKCHIP_MMC_DEGREE_OFFSET	1
+#define ROCKCHIP_MMC_DELAYNUM_OFFSET	2
+#define ROCKCHIP_MMC_DELAYNUM_MASK	(0xff << ROCKCHIP_MMC_DELAYNUM_OFFSET)
+#define ROCKCHIP_MMC_DELAY_ELEMENT_PSEC	60
+#define HIWORD_UPDATE(val, mask, shift) \
+		((val) << (shift) | (mask) << ((shift) + 16))
 
 static const unsigned int freqs[] = { 100000, 200000, 300000, 400000 };
 
@@ -24,8 +34,143 @@ struct dw_mci_rockchip_priv_data {
 	struct clk		*sample_clk;
 	int			default_sample_phase;
 	int			num_phases;
+	int			internal_phase;
 };
 
+/*
+ * Each fine delay is between 44ps-77ps. Assume each fine delay is 60ps to
+ * simplify calculations. So 45degs could be anywhere between 33deg and 57.8deg.
+ */
+static int rockchip_mmc_get_internal_phase(struct dw_mci *host, bool sample)
+{
+	unsigned long rate = clk_get_rate(host->ciu_clk);
+	u32 raw_value;
+	u16 degrees;
+	u32 delay_num = 0;
+
+	/* Constant signal, no measurable phase shift */
+	if (!rate)
+		return 0;
+
+	if (sample)
+		raw_value = mci_readl(host, TIMING_CON1);
+	else
+		raw_value = mci_readl(host, TIMING_CON0);
+
+	raw_value >>= ROCKCHIP_MMC_DEGREE_OFFSET;
+	degrees = (raw_value & ROCKCHIP_MMC_DEGREE_MASK) * 90;
+
+	if (raw_value & ROCKCHIP_MMC_DELAY_SEL) {
+		/* degrees/delaynum * 1000000 */
+		unsigned long factor = (ROCKCHIP_MMC_DELAY_ELEMENT_PSEC / 10) *
+					36 * (rate / 10000);
+
+		delay_num = (raw_value & ROCKCHIP_MMC_DELAYNUM_MASK);
+		delay_num >>= ROCKCHIP_MMC_DELAYNUM_OFFSET;
+		degrees += DIV_ROUND_CLOSEST(delay_num * factor, 1000000);
+	}
+
+	return degrees % 360;
+}
+
+static int rockchip_mmc_get_phase(struct dw_mci *host, bool sample)
+{
+	struct dw_mci_rockchip_priv_data *priv = host->priv;
+	struct clk *clock = sample ? priv->sample_clk : priv->drv_clk;
+
+	if (priv->internal_phase)
+		return rockchip_mmc_get_internal_phase(host, sample);
+	else
+		return clk_get_phase(clock);
+}
+
+static int rockchip_mmc_set_internal_phase(struct dw_mci *host, bool sample, int degrees)
+{
+	unsigned long rate = clk_get_rate(host->ciu_clk);
+	u8 nineties, remainder;
+	u8 delay_num;
+	u32 raw_value;
+	u32 delay;
+
+	/*
+	 * The below calculation is based on the output clock from
+	 * MMC host to the card, which expects the phase clock inherits
+	 * the clock rate from its parent, namely the output clock
+	 * provider of MMC host. However, things may go wrong if
+	 * (1) It is orphan.
+	 * (2) It is assigned to the wrong parent.
+	 *
+	 * This check help debug the case (1), which seems to be the
+	 * most likely problem we often face and which makes it difficult
+	 * for people to debug unstable mmc tuning results.
+	 */
+	if (!rate) {
+		dev_err(host->dev, "%s: invalid clk rate\n", __func__);
+		return -EINVAL;
+	}
+
+	nineties = degrees / 90;
+	remainder = (degrees % 90);
+
+	/*
+	 * Due to the inexact nature of the "fine" delay, we might
+	 * actually go non-monotonic.  We don't go _too_ monotonic
+	 * though, so we should be OK.  Here are options of how we may
+	 * work:
+	 *
+	 * Ideally we end up with:
+	 *   1.0, 2.0, ..., 69.0, 70.0, ...,  89.0, 90.0
+	 *
+	 * On one extreme (if delay is actually 44ps):
+	 *   .73, 1.5, ..., 50.6, 51.3, ...,  65.3, 90.0
+	 * The other (if delay is actually 77ps):
+	 *   1.3, 2.6, ..., 88.6. 89.8, ..., 114.0, 90
+	 *
+	 * It's possible we might make a delay that is up to 25
+	 * degrees off from what we think we're making.  That's OK
+	 * though because we should be REALLY far from any bad range.
+	 */
+
+	/*
+	 * Convert to delay; do a little extra work to make sure we
+	 * don't overflow 32-bit / 64-bit numbers.
+	 */
+	delay = 10000000; /* PSECS_PER_SEC / 10000 / 10 */
+	delay *= remainder;
+	delay = DIV_ROUND_CLOSEST(delay,
+			(rate / 1000) * 36 *
+				(ROCKCHIP_MMC_DELAY_ELEMENT_PSEC / 10));
+
+	delay_num = (u8) min_t(u32, delay, 255);
+
+	raw_value = delay_num ? ROCKCHIP_MMC_DELAY_SEL : 0;
+	raw_value |= delay_num << ROCKCHIP_MMC_DELAYNUM_OFFSET;
+	raw_value |= nineties;
+
+	if (sample)
+		mci_writel(host, TIMING_CON1, HIWORD_UPDATE(raw_value, 0x07ff, 1));
+	else
+		mci_writel(host, TIMING_CON0, HIWORD_UPDATE(raw_value, 0x07ff, 1));
+
+	dev_dbg(host->dev, "set %s_phase(%d) delay_nums=%u actual_degrees=%d\n",
+		sample ? "sample" : "drv", degrees, delay_num,
+		rockchip_mmc_get_phase(host, sample)
+	);
+
+	return 0;
+}
+
+static int rockchip_mmc_set_phase(struct dw_mci *host, bool sample, int degrees)
+{
+	struct dw_mci_rockchip_priv_data *priv = host->priv;
+	struct clk *clock = sample ? priv->sample_clk : priv->drv_clk;
+
+	if (priv->internal_phase)
+		return rockchip_mmc_set_internal_phase(host, sample, degrees);
+	else
+		return clk_set_phase(clock, degrees);
+}
+
 static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
 {
 	struct dw_mci_rockchip_priv_data *priv = host->priv;
@@ -64,7 +209,7 @@ static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
 
 	/* Make sure we use phases which we can enumerate with */
 	if (!IS_ERR(priv->sample_clk) && ios->timing <= MMC_TIMING_SD_HS)
-		clk_set_phase(priv->sample_clk, priv->default_sample_phase);
+		rockchip_mmc_set_phase(host, true, priv->default_sample_phase);
 
 	/*
 	 * Set the drive phase offset based on speed mode to achieve hold times.
@@ -127,7 +272,7 @@ static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
 			break;
 		}
 
-		clk_set_phase(priv->drv_clk, phase);
+		rockchip_mmc_set_phase(host, false, phase);
 	}
 }
 
@@ -151,6 +296,7 @@ static int dw_mci_rk3288_execute_tuning(struct dw_mci_slot *slot, u32 opcode)
 	int longest_range_len = -1;
 	int longest_range = -1;
 	int middle_phase;
+	int phase;
 
 	if (IS_ERR(priv->sample_clk)) {
 		dev_err(host->dev, "Tuning clock (sample_clk) not defined.\n");
@@ -164,8 +310,10 @@ static int dw_mci_rk3288_execute_tuning(struct dw_mci_slot *slot, u32 opcode)
 
 	/* Try each phase and extract good ranges */
 	for (i = 0; i < priv->num_phases; ) {
-		clk_set_phase(priv->sample_clk,
-			      TUNING_ITERATION_TO_PHASE(i, priv->num_phases));
+		rockchip_mmc_set_phase(host, true,
+				       TUNING_ITERATION_TO_PHASE(
+						i,
+						priv->num_phases));
 
 		v = !mmc_send_tuning(mmc, opcode, NULL);
 
@@ -211,7 +359,8 @@ static int dw_mci_rk3288_execute_tuning(struct dw_mci_slot *slot, u32 opcode)
 	}
 
 	if (ranges[0].start == 0 && ranges[0].end == priv->num_phases - 1) {
-		clk_set_phase(priv->sample_clk, priv->default_sample_phase);
+		rockchip_mmc_set_phase(host, true, priv->default_sample_phase);
+
 		dev_info(host->dev, "All phases work, using default phase %d.",
 			 priv->default_sample_phase);
 		goto free;
@@ -248,12 +397,10 @@ static int dw_mci_rk3288_execute_tuning(struct dw_mci_slot *slot, u32 opcode)
 
 	middle_phase = ranges[longest_range].start + longest_range_len / 2;
 	middle_phase %= priv->num_phases;
-	dev_info(host->dev, "Successfully tuned phase to %d\n",
-		 TUNING_ITERATION_TO_PHASE(middle_phase, priv->num_phases));
+	phase = TUNING_ITERATION_TO_PHASE(middle_phase, priv->num_phases);
+	dev_info(host->dev, "Successfully tuned phase to %d\n", phase);
 
-	clk_set_phase(priv->sample_clk,
-		      TUNING_ITERATION_TO_PHASE(middle_phase,
-						priv->num_phases));
+	rockchip_mmc_set_phase(host, true, phase);
 
 free:
 	kfree(ranges);
@@ -287,6 +434,8 @@ static int dw_mci_rk3288_parse_dt(struct dw_mci *host)
 
 	host->priv = priv;
 
+	priv->internal_phase = false;
+
 	return 0;
 }
 
-- 
2.46.0

[PATCH v4 3/4] mmc: dw_mmc-rockchip: Skip all phases bigger than 270 degrees

[Detlev Casanova]

From: Shawn Lin <shawn.lin@rock-chips.com>

Per design recommendation, it'd better not try to use any phase
which is bigger than 270. Let's officially follow this.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
(cherry picked from commit 2a53aab5cfa43065b2e979959d727332a8a03c03)
Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
---
 drivers/mmc/host/dw_mmc-rockchip.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c
index 2748f9bf2691..1458cb5fd5c7 100644
--- a/drivers/mmc/host/dw_mmc-rockchip.c
+++ b/drivers/mmc/host/dw_mmc-rockchip.c
@@ -310,6 +310,9 @@ static int dw_mci_rk3288_execute_tuning(struct dw_mci_slot *slot, u32 opcode)
 
 	/* Try each phase and extract good ranges */
 	for (i = 0; i < priv->num_phases; ) {
+		/* Cannot guarantee any phases larger than 270 would work well */
+		if (TUNING_ITERATION_TO_PHASE(i, priv->num_phases) > 270)
+			break;
 		rockchip_mmc_set_phase(host, true,
 				       TUNING_ITERATION_TO_PHASE(
 						i,
-- 
2.46.0

[PATCH v4 4/4] mmc: dw_mmc-rockchip: Add support for rk3576 SoCs

[Detlev Casanova]

On rk3576 the tunable clocks are inside the controller itself, removing
the need for the "ciu-drive" and "ciu-sample" clocks.

That makes it a new type of controller that has its own dt_parse function.

Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
---
 drivers/mmc/host/dw_mmc-rockchip.c | 48 ++++++++++++++++++++++++++----
 1 file changed, 43 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c
index 1458cb5fd5c7..7c8ccf5e71bc 100644
--- a/drivers/mmc/host/dw_mmc-rockchip.c
+++ b/drivers/mmc/host/dw_mmc-rockchip.c
@@ -410,7 +410,7 @@ static int dw_mci_rk3288_execute_tuning(struct dw_mci_slot *slot, u32 opcode)
 	return ret;
 }
 
-static int dw_mci_rk3288_parse_dt(struct dw_mci *host)
+static int dw_mci_common_parse_dt(struct dw_mci *host)
 {
 	struct device_node *np = host->dev->of_node;
 	struct dw_mci_rockchip_priv_data *priv;
@@ -420,13 +420,29 @@ static int dw_mci_rk3288_parse_dt(struct dw_mci *host)
 		return -ENOMEM;
 
 	if (of_property_read_u32(np, "rockchip,desired-num-phases",
-					&priv->num_phases))
+				 &priv->num_phases))
 		priv->num_phases = 360;
 
 	if (of_property_read_u32(np, "rockchip,default-sample-phase",
-					&priv->default_sample_phase))
+				 &priv->default_sample_phase))
 		priv->default_sample_phase = 0;
 
+	host->priv = priv;
+
+	return 0;
+}
+
+static int dw_mci_rk3288_parse_dt(struct dw_mci *host)
+{
+	struct dw_mci_rockchip_priv_data *priv;
+	int err;
+
+	err = dw_mci_common_parse_dt(host);
+	if (err)
+		return err;
+
+	priv = host->priv;
+
 	priv->drv_clk = devm_clk_get(host->dev, "ciu-drive");
 	if (IS_ERR(priv->drv_clk))
 		dev_dbg(host->dev, "ciu-drive not available\n");
@@ -435,13 +451,25 @@ static int dw_mci_rk3288_parse_dt(struct dw_mci *host)
 	if (IS_ERR(priv->sample_clk))
 		dev_dbg(host->dev, "ciu-sample not available\n");
 
-	host->priv = priv;
-
 	priv->internal_phase = false;
 
 	return 0;
 }
 
+static int dw_mci_rk3576_parse_dt(struct dw_mci *host)
+{
+	struct dw_mci_rockchip_priv_data *priv;
+	int err = dw_mci_common_parse_dt(host);
+	if (err)
+		return err;
+
+	priv = host->priv;
+
+	priv->internal_phase = true;
+
+	return 0;
+}
+
 static int dw_mci_rockchip_init(struct dw_mci *host)
 {
 	int ret, i;
@@ -483,11 +511,21 @@ static const struct dw_mci_drv_data rk3288_drv_data = {
 	.init			= dw_mci_rockchip_init,
 };
 
+static const struct dw_mci_drv_data rk3576_drv_data = {
+	.common_caps		= MMC_CAP_CMD23,
+	.set_ios		= dw_mci_rk3288_set_ios,
+	.execute_tuning		= dw_mci_rk3288_execute_tuning,
+	.parse_dt		= dw_mci_rk3576_parse_dt,
+	.init			= dw_mci_rockchip_init,
+};
+
 static const struct of_device_id dw_mci_rockchip_match[] = {
 	{ .compatible = "rockchip,rk2928-dw-mshc",
 		.data = &rk2928_drv_data },
 	{ .compatible = "rockchip,rk3288-dw-mshc",
 		.data = &rk3288_drv_data },
+	{ .compatible = "rockchip,rk3576-dw-mshc",
+		.data = &rk3576_drv_data },
 	{},
 };
 MODULE_DEVICE_TABLE(of, dw_mci_rockchip_match);
-- 
2.46.0

Re: [PATCH v4 2/4] mmc: dw_mmc-rockchip: Add internal phase support

[Dragan Simic]

Hello Detlev,

Please see a comment below.

On 2024-08-22 23:15, Detlev Casanova wrote:
> From: Shawn Lin <shawn.lin@rock-chips.com>
> 
> Some Rockchip devices put the phase settings into the dw_mmc 
> controller.
> 
> When the feature is present, the ciu-drive and ciu-sample clocks are
> not used and the phase configuration is done directly through the mmc
> controller.
> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> Acked-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
>  drivers/mmc/host/dw_mmc-rockchip.c | 171 +++++++++++++++++++++++++++--
>  1 file changed, 160 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/mmc/host/dw_mmc-rockchip.c
> b/drivers/mmc/host/dw_mmc-rockchip.c
> index b07190ba4b7a..2748f9bf2691 100644
> --- a/drivers/mmc/host/dw_mmc-rockchip.c
> +++ b/drivers/mmc/host/dw_mmc-rockchip.c
> @@ -15,7 +15,17 @@
>  #include "dw_mmc.h"
>  #include "dw_mmc-pltfm.h"
> 
> -#define RK3288_CLKGEN_DIV	2
> +#define RK3288_CLKGEN_DIV		2
> +#define SDMMC_TIMING_CON0		0x130
> +#define SDMMC_TIMING_CON1		0x134
> +#define ROCKCHIP_MMC_DELAY_SEL		BIT(10)
> +#define ROCKCHIP_MMC_DEGREE_MASK	0x3
> +#define ROCKCHIP_MMC_DEGREE_OFFSET	1
> +#define ROCKCHIP_MMC_DELAYNUM_OFFSET	2
> +#define ROCKCHIP_MMC_DELAYNUM_MASK	(0xff << 
> ROCKCHIP_MMC_DELAYNUM_OFFSET)
> +#define ROCKCHIP_MMC_DELAY_ELEMENT_PSEC	60
> +#define HIWORD_UPDATE(val, mask, shift) \
> +		((val) << (shift) | (mask) << ((shift) + 16))
> 
>  static const unsigned int freqs[] = { 100000, 200000, 300000, 400000 
> };
> 
> @@ -24,8 +34,143 @@ struct dw_mci_rockchip_priv_data {
>  	struct clk		*sample_clk;
>  	int			default_sample_phase;
>  	int			num_phases;
> +	int			internal_phase;
>  };

It might be good to declare internal_phase as "unsigned int 
internal_phase:1",
i.e. as a bit field, which isn't going to save some memory in this 
particular
case, but it would show additional attention to detail.

Re: [PATCH v4 3/4] mmc: dw_mmc-rockchip: Skip all phases bigger than 270 degrees

[Dragan Simic]

Hello Detlev,

On 2024-08-22 23:15, Detlev Casanova wrote:
> From: Shawn Lin <shawn.lin@rock-chips.com>
> 
> Per design recommendation, it'd better not try to use any phase
> which is bigger than 270. Let's officially follow this.

Would it be possible to provide a reference to the actual design
specification?  This change affects all users of the dw_mmc-rockchip
driver, so in case any regressions are found later, having as much
detail as possible can only be beneficial.

> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> (cherry picked from commit 2a53aab5cfa43065b2e979959d727332a8a03c03)
> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> ---
>  drivers/mmc/host/dw_mmc-rockchip.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/mmc/host/dw_mmc-rockchip.c
> b/drivers/mmc/host/dw_mmc-rockchip.c
> index 2748f9bf2691..1458cb5fd5c7 100644
> --- a/drivers/mmc/host/dw_mmc-rockchip.c
> +++ b/drivers/mmc/host/dw_mmc-rockchip.c
> @@ -310,6 +310,9 @@ static int dw_mci_rk3288_execute_tuning(struct
> dw_mci_slot *slot, u32 opcode)
> 
>  	/* Try each phase and extract good ranges */
>  	for (i = 0; i < priv->num_phases; ) {
> +		/* Cannot guarantee any phases larger than 270 would work well */
> +		if (TUNING_ITERATION_TO_PHASE(i, priv->num_phases) > 270)
> +			break;
>  		rockchip_mmc_set_phase(host, true,
>  				       TUNING_ITERATION_TO_PHASE(
>  						i,

Re: [PATCH v4 4/4] mmc: dw_mmc-rockchip: Add support for rk3576 SoCs

[Dragan Simic]

Hello Detlev,

Please see a comment below.

On 2024-08-22 23:15, Detlev Casanova wrote:
> On rk3576 the tunable clocks are inside the controller itself, removing
> the need for the "ciu-drive" and "ciu-sample" clocks.
> 
> That makes it a new type of controller that has its own dt_parse 
> function.
> 
> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> ---
>  drivers/mmc/host/dw_mmc-rockchip.c | 48 ++++++++++++++++++++++++++----
>  1 file changed, 43 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mmc/host/dw_mmc-rockchip.c
> b/drivers/mmc/host/dw_mmc-rockchip.c
> index 1458cb5fd5c7..7c8ccf5e71bc 100644
> --- a/drivers/mmc/host/dw_mmc-rockchip.c
> +++ b/drivers/mmc/host/dw_mmc-rockchip.c
> @@ -410,7 +410,7 @@ static int dw_mci_rk3288_execute_tuning(struct
> dw_mci_slot *slot, u32 opcode)
>  	return ret;
>  }
> 
> -static int dw_mci_rk3288_parse_dt(struct dw_mci *host)
> +static int dw_mci_common_parse_dt(struct dw_mci *host)
>  {
>  	struct device_node *np = host->dev->of_node;
>  	struct dw_mci_rockchip_priv_data *priv;
> @@ -420,13 +420,29 @@ static int dw_mci_rk3288_parse_dt(struct dw_mci 
> *host)
>  		return -ENOMEM;
> 
>  	if (of_property_read_u32(np, "rockchip,desired-num-phases",
> -					&priv->num_phases))
> +				 &priv->num_phases))
>  		priv->num_phases = 360;
> 
>  	if (of_property_read_u32(np, "rockchip,default-sample-phase",
> -					&priv->default_sample_phase))
> +				 &priv->default_sample_phase))
>  		priv->default_sample_phase = 0;
> 
> +	host->priv = priv;
> +
> +	return 0;
> +}
> +
> +static int dw_mci_rk3288_parse_dt(struct dw_mci *host)
> +{
> +	struct dw_mci_rockchip_priv_data *priv;
> +	int err;
> +
> +	err = dw_mci_common_parse_dt(host);
> +	if (err)
> +		return err;
> +
> +	priv = host->priv;
> +
>  	priv->drv_clk = devm_clk_get(host->dev, "ciu-drive");
>  	if (IS_ERR(priv->drv_clk))
>  		dev_dbg(host->dev, "ciu-drive not available\n");
> @@ -435,13 +451,25 @@ static int dw_mci_rk3288_parse_dt(struct dw_mci 
> *host)
>  	if (IS_ERR(priv->sample_clk))
>  		dev_dbg(host->dev, "ciu-sample not available\n");
> 
> -	host->priv = priv;
> -
>  	priv->internal_phase = false;
> 
>  	return 0;
>  }
> 
> +static int dw_mci_rk3576_parse_dt(struct dw_mci *host)
> +{
> +	struct dw_mci_rockchip_priv_data *priv;
> +	int err = dw_mci_common_parse_dt(host);
> +	if (err)
> +		return err;
> +
> +	priv = host->priv;
> +
> +	priv->internal_phase = true;

Defining priv, assigning it and using it seems rather redundant,
when all that's needed is simple "host->priv->internal_phase = true"
assignment instead.

> +
> +	return 0;
> +}
> +
>  static int dw_mci_rockchip_init(struct dw_mci *host)
>  {
>  	int ret, i;
> @@ -483,11 +511,21 @@ static const struct dw_mci_drv_data 
> rk3288_drv_data = {
>  	.init			= dw_mci_rockchip_init,
>  };
> 
> +static const struct dw_mci_drv_data rk3576_drv_data = {
> +	.common_caps		= MMC_CAP_CMD23,
> +	.set_ios		= dw_mci_rk3288_set_ios,
> +	.execute_tuning		= dw_mci_rk3288_execute_tuning,
> +	.parse_dt		= dw_mci_rk3576_parse_dt,
> +	.init			= dw_mci_rockchip_init,
> +};
> +
>  static const struct of_device_id dw_mci_rockchip_match[] = {
>  	{ .compatible = "rockchip,rk2928-dw-mshc",
>  		.data = &rk2928_drv_data },
>  	{ .compatible = "rockchip,rk3288-dw-mshc",
>  		.data = &rk3288_drv_data },
> +	{ .compatible = "rockchip,rk3576-dw-mshc",
> +		.data = &rk3576_drv_data },
>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, dw_mci_rockchip_match);

Re: [PATCH v4 1/4] dt-bindings: mmc: Add support for rk3576 dw-mshc

[Krzysztof Kozlowski]

On Thu, Aug 22, 2024 at 05:15:31PM -0400, Detlev Casanova wrote:
> Add the compatible string for rockchip,rk3576-dw-mshc in its own new
> block, for devices that have internal pahse settings instead of external

typo: phase

> clocks.
> 
> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> ---
>  Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.yaml | 2 ++

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v4 4/4] mmc: dw_mmc-rockchip: Add support for rk3576 SoCs

[Detlev Casanova]

Hi Dragan,

On Friday, 23 August 2024 03:00:57 EDT Dragan Simic wrote:
> Hello Detlev,
> 
> Please see a comment below.
> 
> On 2024-08-22 23:15, Detlev Casanova wrote:
> > On rk3576 the tunable clocks are inside the controller itself, removing
> > the need for the "ciu-drive" and "ciu-sample" clocks.
> > 
> > That makes it a new type of controller that has its own dt_parse
> > function.
> > 
> > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> > ---
> > 
> >  drivers/mmc/host/dw_mmc-rockchip.c | 48 ++++++++++++++++++++++++++----
> >  1 file changed, 43 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/dw_mmc-rockchip.c
> > b/drivers/mmc/host/dw_mmc-rockchip.c
> > index 1458cb5fd5c7..7c8ccf5e71bc 100644
> > --- a/drivers/mmc/host/dw_mmc-rockchip.c
> > +++ b/drivers/mmc/host/dw_mmc-rockchip.c
[...]
> > @@ -435,13 +451,25 @@ static int dw_mci_rk3288_parse_dt(struct dw_mci
> > *host)
> > 
> >  	if (IS_ERR(priv->sample_clk))
> >  	
> >  		dev_dbg(host->dev, "ciu-sample not available\n");
> > 
> > -	host->priv = priv;
> > -
> > 
> >  	priv->internal_phase = false;
> >  	
> >  	return 0;
> >  
> >  }
> > 
> > +static int dw_mci_rk3576_parse_dt(struct dw_mci *host)
> > +{
> > +	struct dw_mci_rockchip_priv_data *priv;
> > +	int err = dw_mci_common_parse_dt(host);
> > +	if (err)
> > +		return err;
> > +
> > +	priv = host->priv;
> > +
> > +	priv->internal_phase = true;
> 
> Defining priv, assigning it and using it seems rather redundant,
> when all that's needed is simple "host->priv->internal_phase = true"
> assignment instead.

Yes, that's what I did at first, but host->priv is declared as void*, which 
means it needs to be cast to struct dw_mci_rockchip_priv_data * and I felt 
that 

((struct dw_mci_rockchip_priv_data *)host->priv)->internal_phase = true;

is not very pretty and harder to read.

> > +
> > +	return 0;
> > +}
> > +
> > 
> >  static int dw_mci_rockchip_init(struct dw_mci *host)
> >  {
> >  
> >  	int ret, i;
> > 
> > @@ -483,11 +511,21 @@ static const struct dw_mci_drv_data
> > rk3288_drv_data = {
> > 
> >  	.init			= dw_mci_rockchip_init,
> >  
> >  };
> > 
> > +static const struct dw_mci_drv_data rk3576_drv_data = {
> > +	.common_caps		= MMC_CAP_CMD23,
> > +	.set_ios		= dw_mci_rk3288_set_ios,
> > +	.execute_tuning		= dw_mci_rk3288_execute_tuning,
> > +	.parse_dt		= dw_mci_rk3576_parse_dt,
> > +	.init			= dw_mci_rockchip_init,
> > +};
> > +
> > 
> >  static const struct of_device_id dw_mci_rockchip_match[] = {
> >  
> >  	{ .compatible = "rockchip,rk2928-dw-mshc",
> >  	
> >  		.data = &rk2928_drv_data },
> >  	
> >  	{ .compatible = "rockchip,rk3288-dw-mshc",
> >  	
> >  		.data = &rk3288_drv_data },
> > 
> > +	{ .compatible = "rockchip,rk3576-dw-mshc",
> > +		.data = &rk3576_drv_data },
> > 
> >  	{},
> >  
> >  };
> >  MODULE_DEVICE_TABLE(of, dw_mci_rockchip_match);

Re: [PATCH v4 2/4] mmc: dw_mmc-rockchip: Add internal phase support

[Detlev Casanova]

Hi Dragan,

On Friday, 23 August 2024 01:41:44 EDT Dragan Simic wrote:
> Hello Detlev,
> 
> Please see a comment below.
> 
> On 2024-08-22 23:15, Detlev Casanova wrote:
> > From: Shawn Lin <shawn.lin@rock-chips.com>
> > 
> > Some Rockchip devices put the phase settings into the dw_mmc
> > controller.
> > 
> > When the feature is present, the ciu-drive and ciu-sample clocks are
> > not used and the phase configuration is done directly through the mmc
> > controller.
> > 
> > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> > Acked-by: Shawn Lin <shawn.lin@rock-chips.com>
> > ---
> > 
> >  drivers/mmc/host/dw_mmc-rockchip.c | 171 +++++++++++++++++++++++++++--
> >  1 file changed, 160 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/dw_mmc-rockchip.c
> > b/drivers/mmc/host/dw_mmc-rockchip.c
> > index b07190ba4b7a..2748f9bf2691 100644
> > --- a/drivers/mmc/host/dw_mmc-rockchip.c
> > +++ b/drivers/mmc/host/dw_mmc-rockchip.c
> > @@ -15,7 +15,17 @@
> > 
> >  #include "dw_mmc.h"
> >  #include "dw_mmc-pltfm.h"
> > 
> > -#define RK3288_CLKGEN_DIV	2
> > +#define RK3288_CLKGEN_DIV		2
> > +#define SDMMC_TIMING_CON0		0x130
> > +#define SDMMC_TIMING_CON1		0x134
> > +#define ROCKCHIP_MMC_DELAY_SEL		BIT(10)
> > +#define ROCKCHIP_MMC_DEGREE_MASK	0x3
> > +#define ROCKCHIP_MMC_DEGREE_OFFSET	1
> > +#define ROCKCHIP_MMC_DELAYNUM_OFFSET	2
> > +#define ROCKCHIP_MMC_DELAYNUM_MASK	(0xff <<
> > ROCKCHIP_MMC_DELAYNUM_OFFSET)
> > +#define ROCKCHIP_MMC_DELAY_ELEMENT_PSEC	60
> > +#define HIWORD_UPDATE(val, mask, shift) \
> > +		((val) << (shift) | (mask) << ((shift) + 16))
> > 
> >  static const unsigned int freqs[] = { 100000, 200000, 300000, 400000
> > 
> > };
> > 
> > @@ -24,8 +34,143 @@ struct dw_mci_rockchip_priv_data {
> > 
> >  	struct clk		*sample_clk;
> >  	int			default_sample_phase;
> >  	int			num_phases;
> > 
> > +	int			internal_phase;
> > 
> >  };
> 
> It might be good to declare internal_phase as "unsigned int
> internal_phase:1",
> i.e. as a bit field, which isn't going to save some memory in this
> particular
> case, but it would show additional attention to detail.

In that case, I would go with a bool instead of int, that makes things even 
clearer.

Re: [PATCH v4 3/4] mmc: dw_mmc-rockchip: Skip all phases bigger than 270 degrees

[Detlev Casanova]

Hi Dragan,

On Friday, 23 August 2024 01:45:07 EDT Dragan Simic wrote:
> Hello Detlev,
> 
> On 2024-08-22 23:15, Detlev Casanova wrote:
> > From: Shawn Lin <shawn.lin@rock-chips.com>
> > 
> > Per design recommendation, it'd better not try to use any phase
> > which is bigger than 270. Let's officially follow this.
> 
> Would it be possible to provide a reference to the actual design
> specification?  This change affects all users of the dw_mmc-rockchip
> driver, so in case any regressions are found later, having as much
> detail as possible can only be beneficial.

I don't have the reference and only trusting rockchip on this. This could be 
specific to rockchip hardware.
Anyway, the drivers works well on my side on my rk3576 armsom sige5 without 
this patch, so I'm willing to drop it completely.

> > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> > (cherry picked from commit 2a53aab5cfa43065b2e979959d727332a8a03c03)
> > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> > ---
> > 
> >  drivers/mmc/host/dw_mmc-rockchip.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/mmc/host/dw_mmc-rockchip.c
> > b/drivers/mmc/host/dw_mmc-rockchip.c
> > index 2748f9bf2691..1458cb5fd5c7 100644
> > --- a/drivers/mmc/host/dw_mmc-rockchip.c
> > +++ b/drivers/mmc/host/dw_mmc-rockchip.c
> > @@ -310,6 +310,9 @@ static int dw_mci_rk3288_execute_tuning(struct
> > dw_mci_slot *slot, u32 opcode)
> > 
> >  	/* Try each phase and extract good ranges */
> >  	for (i = 0; i < priv->num_phases; ) {
> > 
> > +		/* Cannot guarantee any phases larger than 270 would 
work well */
> > +		if (TUNING_ITERATION_TO_PHASE(i, priv->num_phases) > 
270)
> > +			break;
> > 
> >  		rockchip_mmc_set_phase(host, true,
> >  		
> >  				       TUNING_ITERATION_TO_PHASE(
> >  						
> >  						i,

Re: [PATCH v4 4/4] mmc: dw_mmc-rockchip: Add support for rk3576 SoCs

[Dragan Simic]

Hello Detlev,

On 2024-08-23 15:20, Detlev Casanova wrote:
> On Friday, 23 August 2024 03:00:57 EDT Dragan Simic wrote:
>> Hello Detlev,
>> 
>> Please see a comment below.
>> 
>> On 2024-08-22 23:15, Detlev Casanova wrote:
>> > On rk3576 the tunable clocks are inside the controller itself, removing
>> > the need for the "ciu-drive" and "ciu-sample" clocks.
>> >
>> > That makes it a new type of controller that has its own dt_parse
>> > function.
>> >
>> > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
>> > ---
>> >
>> >  drivers/mmc/host/dw_mmc-rockchip.c | 48 ++++++++++++++++++++++++++----
>> >  1 file changed, 43 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/drivers/mmc/host/dw_mmc-rockchip.c
>> > b/drivers/mmc/host/dw_mmc-rockchip.c
>> > index 1458cb5fd5c7..7c8ccf5e71bc 100644
>> > --- a/drivers/mmc/host/dw_mmc-rockchip.c
>> > +++ b/drivers/mmc/host/dw_mmc-rockchip.c
> [...]
>> > @@ -435,13 +451,25 @@ static int dw_mci_rk3288_parse_dt(struct dw_mci
>> > *host)
>> >
>> >  	if (IS_ERR(priv->sample_clk))
>> >
>> >  		dev_dbg(host->dev, "ciu-sample not available\n");
>> >
>> > -	host->priv = priv;
>> > -
>> >
>> >  	priv->internal_phase = false;
>> >
>> >  	return 0;
>> >
>> >  }
>> >
>> > +static int dw_mci_rk3576_parse_dt(struct dw_mci *host)
>> > +{
>> > +	struct dw_mci_rockchip_priv_data *priv;
>> > +	int err = dw_mci_common_parse_dt(host);
>> > +	if (err)
>> > +		return err;
>> > +
>> > +	priv = host->priv;
>> > +
>> > +	priv->internal_phase = true;
>> 
>> Defining priv, assigning it and using it seems rather redundant,
>> when all that's needed is simple "host->priv->internal_phase = true"
>> assignment instead.
> 
> Yes, that's what I did at first, but host->priv is declared as void*, 
> which
> means it needs to be cast to struct dw_mci_rockchip_priv_data * and I 
> felt
> that
> 
> ((struct dw_mci_rockchip_priv_data *)host->priv)->internal_phase = 
> true;
> 
> is not very pretty and harder to read.

Ah, you're right, and I somehow managed to ignore that.

I agree with your conclusions, although I'd suggest something like
this, for slightly improved readability:

  +static int dw_mci_rk3576_parse_dt(struct dw_mci *host)
  +{
  +	struct dw_mci_rockchip_priv_data *priv = host->priv;
  +	int err;
  +
  +	err = dw_mci_common_parse_dt(host);
  +	if (err)
  +		return err;
  +
  +	priv->internal_phase = true;
  +
  +	return 0;
  +}

Re: [PATCH v4 2/4] mmc: dw_mmc-rockchip: Add internal phase support

[Dragan Simic]

Hello Detlev,

On 2024-08-23 15:34, Detlev Casanova wrote:
> On Friday, 23 August 2024 01:41:44 EDT Dragan Simic wrote:
>> Hello Detlev,
>> 
>> Please see a comment below.
>> 
>> On 2024-08-22 23:15, Detlev Casanova wrote:
>> > From: Shawn Lin <shawn.lin@rock-chips.com>
>> >
>> > Some Rockchip devices put the phase settings into the dw_mmc
>> > controller.
>> >
>> > When the feature is present, the ciu-drive and ciu-sample clocks are
>> > not used and the phase configuration is done directly through the mmc
>> > controller.
>> >
>> > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>> > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
>> > Acked-by: Shawn Lin <shawn.lin@rock-chips.com>
>> > ---
>> >
>> >  drivers/mmc/host/dw_mmc-rockchip.c | 171 +++++++++++++++++++++++++++--
>> >  1 file changed, 160 insertions(+), 11 deletions(-)
>> >
>> > diff --git a/drivers/mmc/host/dw_mmc-rockchip.c
>> > b/drivers/mmc/host/dw_mmc-rockchip.c
>> > index b07190ba4b7a..2748f9bf2691 100644
>> > --- a/drivers/mmc/host/dw_mmc-rockchip.c
>> > +++ b/drivers/mmc/host/dw_mmc-rockchip.c
>> > @@ -15,7 +15,17 @@
>> >
>> >  #include "dw_mmc.h"
>> >  #include "dw_mmc-pltfm.h"
>> >
>> > -#define RK3288_CLKGEN_DIV	2
>> > +#define RK3288_CLKGEN_DIV		2
>> > +#define SDMMC_TIMING_CON0		0x130
>> > +#define SDMMC_TIMING_CON1		0x134
>> > +#define ROCKCHIP_MMC_DELAY_SEL		BIT(10)
>> > +#define ROCKCHIP_MMC_DEGREE_MASK	0x3
>> > +#define ROCKCHIP_MMC_DEGREE_OFFSET	1
>> > +#define ROCKCHIP_MMC_DELAYNUM_OFFSET	2
>> > +#define ROCKCHIP_MMC_DELAYNUM_MASK	(0xff <<
>> > ROCKCHIP_MMC_DELAYNUM_OFFSET)
>> > +#define ROCKCHIP_MMC_DELAY_ELEMENT_PSEC	60
>> > +#define HIWORD_UPDATE(val, mask, shift) \
>> > +		((val) << (shift) | (mask) << ((shift) + 16))
>> >
>> >  static const unsigned int freqs[] = { 100000, 200000, 300000, 400000
>> >
>> > };
>> >
>> > @@ -24,8 +34,143 @@ struct dw_mci_rockchip_priv_data {
>> >
>> >  	struct clk		*sample_clk;
>> >  	int			default_sample_phase;
>> >  	int			num_phases;
>> >
>> > +	int			internal_phase;
>> >
>> >  };
>> 
>> It might be good to declare internal_phase as "unsigned int
>> internal_phase:1",
>> i.e. as a bit field, which isn't going to save some memory in this
>> particular
>> case, but it would show additional attention to detail.
> 
> In that case, I would go with a bool instead of int, that makes things
> even clearer.

My suggestion to use "unsigned int internal_phase:1" actually takes
inspiration from the ASoC code, in which such bit fields are used
quite a lot, even when using them actually doesn't save space.

In this particular case, using plain bool would make sense, but I
still think that using an "unsigned int internal_phase:1" bit field
would fit better, because it would show the intention to possibly
save a bit of RAM at some point.  OTOH, I don't think that using
bool with such bit fields would actually work cleanly, because bool
actually resolves to int that's a signed type.

Re: [PATCH v4 3/4] mmc: dw_mmc-rockchip: Skip all phases bigger than 270 degrees

[Dragan Simic]

Hello Detlev,

On 2024-08-23 15:59, Detlev Casanova wrote:
> On Friday, 23 August 2024 01:45:07 EDT Dragan Simic wrote:
>> Hello Detlev,
>> 
>> On 2024-08-22 23:15, Detlev Casanova wrote:
>> > From: Shawn Lin <shawn.lin@rock-chips.com>
>> >
>> > Per design recommendation, it'd better not try to use any phase
>> > which is bigger than 270. Let's officially follow this.
>> 
>> Would it be possible to provide a reference to the actual design
>> specification?  This change affects all users of the dw_mmc-rockchip
>> driver, so in case any regressions are found later, having as much
>> detail as possible can only be beneficial.
> 
> I don't have the reference and only trusting rockchip on this. This 
> could be
> specific to rockchip hardware.
> Anyway, the drivers works well on my side on my rk3576 armsom sige5 
> without
> this patch, so I'm willing to drop it completely.

I think it would be better if you'd drop it in this series, and submit
it later separately, as a follow-up patch, to reduce the chances for any
possible regressions.  Maybe we'll also have more background information
available by then, who knows.

>> > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>> > (cherry picked from commit 2a53aab5cfa43065b2e979959d727332a8a03c03)
>> > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
>> > ---
>> >
>> >  drivers/mmc/host/dw_mmc-rockchip.c | 3 +++
>> >  1 file changed, 3 insertions(+)
>> >
>> > diff --git a/drivers/mmc/host/dw_mmc-rockchip.c
>> > b/drivers/mmc/host/dw_mmc-rockchip.c
>> > index 2748f9bf2691..1458cb5fd5c7 100644
>> > --- a/drivers/mmc/host/dw_mmc-rockchip.c
>> > +++ b/drivers/mmc/host/dw_mmc-rockchip.c
>> > @@ -310,6 +310,9 @@ static int dw_mci_rk3288_execute_tuning(struct
>> > dw_mci_slot *slot, u32 opcode)
>> >
>> >  	/* Try each phase and extract good ranges */
>> >  	for (i = 0; i < priv->num_phases; ) {
>> >
>> > +		/* Cannot guarantee any phases larger than 270 would
> work well */
>> > +		if (TUNING_ITERATION_TO_PHASE(i, priv->num_phases) >
> 270)
>> > +			break;
>> >
>> >  		rockchip_mmc_set_phase(host, true,
>> >
>> >  				       TUNING_ITERATION_TO_PHASE(
>> >
>> >  						i,

Re: [PATCH v4 4/4] mmc: dw_mmc-rockchip: Add support for rk3576 SoCs

[Detlev Casanova]

Hi Dragan,

On Monday, 26 August 2024 10:07:49 EDT Dragan Simic wrote:
> Hello Detlev,
> 
> On 2024-08-23 15:20, Detlev Casanova wrote:
> > On Friday, 23 August 2024 03:00:57 EDT Dragan Simic wrote:
> >> Hello Detlev,
> >> 
> >> Please see a comment below.
> >> 
> >> On 2024-08-22 23:15, Detlev Casanova wrote:
> >> > On rk3576 the tunable clocks are inside the controller itself, removing
> >> > the need for the "ciu-drive" and "ciu-sample" clocks.
> >> > 
> >> > That makes it a new type of controller that has its own dt_parse
> >> > function.
> >> > 
> >> > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> >> > ---
> >> > 
> >> >  drivers/mmc/host/dw_mmc-rockchip.c | 48 ++++++++++++++++++++++++++----
> >> >  1 file changed, 43 insertions(+), 5 deletions(-)
> >> > 
> >> > diff --git a/drivers/mmc/host/dw_mmc-rockchip.c
> >> > b/drivers/mmc/host/dw_mmc-rockchip.c
> >> > index 1458cb5fd5c7..7c8ccf5e71bc 100644
> >> > --- a/drivers/mmc/host/dw_mmc-rockchip.c
> >> > +++ b/drivers/mmc/host/dw_mmc-rockchip.c
> > 
> > [...]
> > 
> >> > @@ -435,13 +451,25 @@ static int dw_mci_rk3288_parse_dt(struct dw_mci
> >> > *host)
> >> > 
> >> >  	if (IS_ERR(priv->sample_clk))
> >> >  	
> >> >  		dev_dbg(host->dev, "ciu-sample not available\n");
> >> > 
> >> > -	host->priv = priv;
> >> > -
> >> > 
> >> >  	priv->internal_phase = false;
> >> >  	
> >> >  	return 0;
> >> >  
> >> >  }
> >> > 
> >> > +static int dw_mci_rk3576_parse_dt(struct dw_mci *host)
> >> > +{
> >> > +	struct dw_mci_rockchip_priv_data *priv;
> >> > +	int err = dw_mci_common_parse_dt(host);
> >> > +	if (err)
> >> > +		return err;
> >> > +
> >> > +	priv = host->priv;
> >> > +
> >> > +	priv->internal_phase = true;
> >> 
> >> Defining priv, assigning it and using it seems rather redundant,
> >> when all that's needed is simple "host->priv->internal_phase = true"
> >> assignment instead.
> > 
> > Yes, that's what I did at first, but host->priv is declared as void*,
> > which
> > means it needs to be cast to struct dw_mci_rockchip_priv_data * and I
> > felt
> > that
> > 
> > ((struct dw_mci_rockchip_priv_data *)host->priv)->internal_phase =
> > true;
> > 
> > is not very pretty and harder to read.
> 
> Ah, you're right, and I somehow managed to ignore that.
> 
> I agree with your conclusions, although I'd suggest something like
> this, for slightly improved readability:
> 
>   +static int dw_mci_rk3576_parse_dt(struct dw_mci *host)
>   +{
>   +	struct dw_mci_rockchip_priv_data *priv = host->priv;
>   +	int err;
>   +
>   +	err = dw_mci_common_parse_dt(host);
>   +	if (err)
>   +		return err;
>   +
>   +	priv->internal_phase = true;
>   +
>   +	return 0;
>   +}

That won't work either, because host->priv is initialized in 
dw_mci_common_parse_dt.

Re: [PATCH v4 2/4] mmc: dw_mmc-rockchip: Add internal phase support

[Detlev Casanova]

On Monday, 26 August 2024 10:39:58 EDT Dragan Simic wrote:
> Hello Detlev,
> 
> On 2024-08-23 15:34, Detlev Casanova wrote:
> > On Friday, 23 August 2024 01:41:44 EDT Dragan Simic wrote:
> >> Hello Detlev,
> >> 
> >> Please see a comment below.
> >> 
> >> On 2024-08-22 23:15, Detlev Casanova wrote:
> >> > From: Shawn Lin <shawn.lin@rock-chips.com>
> >> > 
> >> > Some Rockchip devices put the phase settings into the dw_mmc
> >> > controller.
> >> > 
> >> > When the feature is present, the ciu-drive and ciu-sample clocks are
> >> > not used and the phase configuration is done directly through the mmc
> >> > controller.
> >> > 
> >> > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> >> > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> >> > Acked-by: Shawn Lin <shawn.lin@rock-chips.com>
> >> > ---
> >> > 
> >> >  drivers/mmc/host/dw_mmc-rockchip.c | 171 +++++++++++++++++++++++++++--
> >> >  1 file changed, 160 insertions(+), 11 deletions(-)
> >> > 
> >> > diff --git a/drivers/mmc/host/dw_mmc-rockchip.c
> >> > b/drivers/mmc/host/dw_mmc-rockchip.c
> >> > index b07190ba4b7a..2748f9bf2691 100644
> >> > --- a/drivers/mmc/host/dw_mmc-rockchip.c
> >> > +++ b/drivers/mmc/host/dw_mmc-rockchip.c
> >> > @@ -15,7 +15,17 @@
> >> > 
> >> >  #include "dw_mmc.h"
> >> >  #include "dw_mmc-pltfm.h"
> >> > 
> >> > -#define RK3288_CLKGEN_DIV	2
> >> > +#define RK3288_CLKGEN_DIV		2
> >> > +#define SDMMC_TIMING_CON0		0x130
> >> > +#define SDMMC_TIMING_CON1		0x134
> >> > +#define ROCKCHIP_MMC_DELAY_SEL		BIT(10)
> >> > +#define ROCKCHIP_MMC_DEGREE_MASK	0x3
> >> > +#define ROCKCHIP_MMC_DEGREE_OFFSET	1
> >> > +#define ROCKCHIP_MMC_DELAYNUM_OFFSET	2
> >> > +#define ROCKCHIP_MMC_DELAYNUM_MASK	(0xff <<
> >> > ROCKCHIP_MMC_DELAYNUM_OFFSET)
> >> > +#define ROCKCHIP_MMC_DELAY_ELEMENT_PSEC	60
> >> > +#define HIWORD_UPDATE(val, mask, shift) \
> >> > +		((val) << (shift) | (mask) << ((shift) + 16))
> >> > 
> >> >  static const unsigned int freqs[] = { 100000, 200000, 300000, 400000
> >> > 
> >> > };
> >> > 
> >> > @@ -24,8 +34,143 @@ struct dw_mci_rockchip_priv_data {
> >> > 
> >> >  	struct clk		*sample_clk;
> >> >  	int			default_sample_phase;
> >> >  	int			num_phases;
> >> > 
> >> > +	int			internal_phase;
> >> > 
> >> >  };
> >> 
> >> It might be good to declare internal_phase as "unsigned int
> >> internal_phase:1",
> >> i.e. as a bit field, which isn't going to save some memory in this
> >> particular
> >> case, but it would show additional attention to detail.
> > 
> > In that case, I would go with a bool instead of int, that makes things
> > even clearer.
> 
> My suggestion to use "unsigned int internal_phase:1" actually takes
> inspiration from the ASoC code, in which such bit fields are used
> quite a lot, even when using them actually doesn't save space.
> 
> In this particular case, using plain bool would make sense, but I
> still think that using an "unsigned int internal_phase:1" bit field
> would fit better, because it would show the intention to possibly
> save a bit of RAM at some point.  OTOH, I don't think that using
> bool with such bit fields would actually work cleanly, because bool
> actually resolves to int that's a signed type.

I wouldn't use bool with a bit field of course. I've always considered using 
bit fileds only for structs that must have a certain format, like a header 
format definition.

For me, it is better to use "bool internal_phase" so that it is obvious that 
the feature can be on or off when reading the code.

When using bit fields with a struct that is not marked as "__packed", I 
immediately think that there could be a bug there and wonder why the bit field 
is used, not really thinking "the dev wanted to show they cared about memory 
usage".
But I guess that is all about preferences. In the end, it won't change how it 
works.

Detlev.