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

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 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-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 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

[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 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 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