Re: [PATCH 2/3] mmc: sdhci-brcmstb: clear CFG_OP_DLY when using HS200

[Adrian Hunter <adrian.hunter@intel.com>]

On 03/10/2025 00:04, Kamal Dasu wrote:
> Clear SDIO_1_CFG_OP_DLY register when using HS200 mode to be
> compliant with timing spec.  We only need this for on BCM72116
> SoCs.
> 
> Signed-off-by: Kamal Dasu <kamal.dasu@broadcom.com>

Some minor cosmetic issues.  Otherwise:

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  drivers/mmc/host/sdhci-brcmstb.c | 37 ++++++++++++++++++++++++++++----
>  1 file changed, 33 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-brcmstb.c b/drivers/mmc/host/sdhci-brcmstb.c
> index efc2f3bdc631..0905b316a24b 100644
> --- a/drivers/mmc/host/sdhci-brcmstb.c
> +++ b/drivers/mmc/host/sdhci-brcmstb.c
> @@ -31,13 +31,13 @@
>  
>  #define SDHCI_ARASAN_CQE_BASE_ADDR		0x200
>  
> -#define SDIO_CFG_CQ_CAPABILITY			0x4c
> -#define SDIO_CFG_CQ_CAPABILITY_FMUL		GENMASK(13, 12)

Preferably moving code around would be a separate patch.

> -
>  #define SDIO_CFG_CTRL				0x0
>  #define SDIO_CFG_CTRL_SDCD_N_TEST_EN		BIT(31)
>  #define SDIO_CFG_CTRL_SDCD_N_TEST_LEV		BIT(30)
> -
> +#define SDIO_CFG_OP_DLY				0x34
> +#define SDIO_CFG_OP_DLY_DEFAULT			0x80000003
> +#define SDIO_CFG_CQ_CAPABILITY			0x4c
> +#define SDIO_CFG_CQ_CAPABILITY_FMUL		GENMASK(13, 12)
>  #define SDIO_CFG_MAX_50MHZ_MODE			0x1ac
>  #define SDIO_CFG_MAX_50MHZ_MODE_STRAP_OVERRIDE	BIT(31)
>  #define SDIO_CFG_MAX_50MHZ_MODE_ENABLE		BIT(0)
> @@ -212,6 +212,22 @@ static void sdhci_brcmstb_cfginit_2712(struct sdhci_host *host)
>  	}
>  }
>  
> +static void sdhci_brcmstb_set_72116_uhs_signaling(struct sdhci_host *host,
> +						unsigned int timing)

Prefer to line up function parameters.  Using up to 100 columns is also ok.

> +{
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_brcmstb_priv *priv = sdhci_pltfm_priv(pltfm_host);
> +	u32 reg;
> +
> +	/* no change to SDIO_CFG_OP_DLY_DEFAULT when using preset clk rate */
> +	if (!(host->quirks2 & SDHCI_QUIRK2_PRESET_VALUE_BROKEN))
> +		return;
> +
> +	reg = (timing == MMC_TIMING_MMC_HS200) ? 0 : SDIO_CFG_OP_DLY_DEFAULT;
> +	writel(reg, priv->cfg_regs + SDIO_CFG_OP_DLY);
> +	sdhci_set_uhs_signaling(host, timing);
> +}
> +
>  static void sdhci_brcmstb_dumpregs(struct mmc_host *mmc)
>  {
>  	sdhci_dumpregs(mmc_priv(mmc));
> @@ -252,6 +268,13 @@ static struct sdhci_ops sdhci_brcmstb_ops_2712 = {
>  	.set_uhs_signaling = sdhci_set_uhs_signaling,
>  };
>  
> +static struct sdhci_ops sdhci_brcmstb_ops_72116 = {
> +	.set_clock = sdhci_set_clock,
> +	.set_bus_width = sdhci_set_bus_width,
> +	.reset = sdhci_reset,
> +	.set_uhs_signaling = sdhci_brcmstb_set_72116_uhs_signaling,
> +};
> +
>  static struct sdhci_ops sdhci_brcmstb_ops_7216 = {
>  	.set_clock = sdhci_brcmstb_set_clock,
>  	.set_bus_width = sdhci_set_bus_width,
> @@ -282,6 +305,11 @@ static struct brcmstb_match_priv match_priv_7445 = {
>  	.ops = &sdhci_brcmstb_ops,
>  };
>  
> +static struct brcmstb_match_priv match_priv_72116 = {
> +	.flags = BRCMSTB_MATCH_FLAGS_BROKEN_TIMEOUT,
> +	.ops = &sdhci_brcmstb_ops_72116,
> +};
> +
>  static const struct brcmstb_match_priv match_priv_7216 = {
>  	.flags = BRCMSTB_MATCH_FLAGS_HAS_CLOCK_GATE,
>  	.hs400es = sdhci_brcmstb_hs400es,
> @@ -298,6 +326,7 @@ static const struct of_device_id __maybe_unused sdhci_brcm_of_match[] = {
>  	{ .compatible = "brcm,bcm2712-sdhci", .data = &match_priv_2712 },
>  	{ .compatible = "brcm,bcm7425-sdhci", .data = &match_priv_7425 },
>  	{ .compatible = "brcm,bcm7445-sdhci", .data = &match_priv_7445 },
> +	{ .compatible = "brcm,bcm72116-sdhci", .data = &match_priv_72116 },
>  	{ .compatible = "brcm,bcm7216-sdhci", .data = &match_priv_7216 },
>  	{ .compatible = "brcm,bcm74165b0-sdhci", .data = &match_priv_74165b0 },
>  	{},