Mailing List Archive

[PATCH v4 4/5] mmc: sdhci: move SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN frequency limit
Push handling of clock frequency dependence for
SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN quirk to the drivers that use it.

Signed-off-by: Micha? Miros?aw <mirq-linux@rere.qmqm.pl>
---
v4: fix build issue reported by kernel test robot
v3: rebased on v5.14-rc2 and reworded commitmsg
v2: reworded commitmsg
---
drivers/mmc/host/sdhci-of-arasan.c | 11 ++++-------
drivers/mmc/host/sdhci-of-dwcmshc.c | 8 +++++---
drivers/mmc/host/sdhci.c | 3 +--
3 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
index 737e2bfdedc2..f2a6441ab540 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -452,8 +452,7 @@ static const struct sdhci_ops sdhci_arasan_cqe_ops = {
static const struct sdhci_pltfm_data sdhci_arasan_cqe_pdata = {
.ops = &sdhci_arasan_cqe_ops,
.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
- .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
- SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN,
+ .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
};

#ifdef CONFIG_PM_SLEEP
@@ -1118,7 +1117,6 @@ static const struct sdhci_pltfm_data sdhci_arasan_pdata = {
.ops = &sdhci_arasan_ops,
.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
- SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN |
SDHCI_QUIRK2_STOP_WITH_TC,
};

@@ -1141,7 +1139,6 @@ static const struct sdhci_pltfm_data sdhci_keembay_emmc_pdata = {
SDHCI_QUIRK_32BIT_DMA_SIZE |
SDHCI_QUIRK_32BIT_ADMA_SIZE,
.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
- SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN |
SDHCI_QUIRK2_CAPS_BIT63_FOR_HS400 |
SDHCI_QUIRK2_STOP_WITH_TC |
SDHCI_QUIRK2_BROKEN_64_BIT_DMA,
@@ -1156,7 +1153,6 @@ static const struct sdhci_pltfm_data sdhci_keembay_sd_pdata = {
SDHCI_QUIRK_32BIT_DMA_SIZE |
SDHCI_QUIRK_32BIT_ADMA_SIZE,
.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
- SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN |
SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON |
SDHCI_QUIRK2_STOP_WITH_TC |
SDHCI_QUIRK2_BROKEN_64_BIT_DMA,
@@ -1171,7 +1167,6 @@ static const struct sdhci_pltfm_data sdhci_keembay_sdio_pdata = {
SDHCI_QUIRK_32BIT_DMA_SIZE |
SDHCI_QUIRK_32BIT_ADMA_SIZE,
.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
- SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN |
SDHCI_QUIRK2_HOST_OFF_CARD_ON |
SDHCI_QUIRK2_BROKEN_64_BIT_DMA,
};
@@ -1197,7 +1192,6 @@ static struct sdhci_arasan_of_data intel_lgm_sdxc_data = {
static const struct sdhci_pltfm_data sdhci_arasan_zynqmp_pdata = {
.ops = &sdhci_arasan_ops,
.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
- SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN |
SDHCI_QUIRK2_STOP_WITH_TC,
};

@@ -1502,6 +1496,9 @@ static int sdhci_arasan_add_host(struct sdhci_arasan_data *sdhci_arasan)
bool dma64;
int ret;

+ if (sdhci_pltfm_clk_get_max_clock(host) <= 25000000)
+ host->quirks2 |= SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN;
+
if (!sdhci_arasan->has_cqe)
return sdhci_add_host(host);

diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
index bac874ab0b33..b6b7c4068e90 100644
--- a/drivers/mmc/host/sdhci-of-dwcmshc.c
+++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
@@ -283,14 +283,13 @@ static const struct sdhci_pltfm_data sdhci_dwcmshc_rk3568_pdata = {
.ops = &sdhci_dwcmshc_rk3568_ops,
.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN |
SDHCI_QUIRK_BROKEN_TIMEOUT_VAL,
- .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
- SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN,
+ .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
};

static int dwcmshc_rk3568_init(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv)
{
- int err;
struct rk3568_priv *priv = dwc_priv->priv;
+ int err;

priv->rockchip_clks[0].id = "axi";
priv->rockchip_clks[1].id = "block";
@@ -318,6 +317,9 @@ static int dwcmshc_rk3568_init(struct sdhci_host *host, struct dwcmshc_priv *dwc
sdhci_writel(host, 0, DWCMSHC_EMMC_DLL_TXCLK);
sdhci_writel(host, 0, DWCMSHC_EMMC_DLL_STRBIN);

+ if (sdhci_pltfm_clk_get_max_clock(host) <= 25000000)
+ host->quirks2 |= SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN;
+
return 0;
}

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 0993f7d0ce8e..cfa314e659bc 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1905,8 +1905,7 @@ u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
/* Version 3.00 divisors must be a multiple of 2. */
if (host->max_clk <= clock) {
div = 1;
- if ((host->quirks2 & SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN)
- && host->max_clk <= 25000000)
+ if (host->quirks2 & SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN)
div = 2;
} else {
for (div = 2; div < SDHCI_MAX_DIV_SPEC_300;
--
2.30.2
Re: [PATCH v4 4/5] mmc: sdhci: move SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN frequency limit [ In reply to ]
On 25/07/21 12:20 pm, Micha? Miros?aw wrote:
> Push handling of clock frequency dependence for
> SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN quirk to the drivers that use it.

What is the max_clk dependency for, and why push it down?

>
> Signed-off-by: Micha? Miros?aw <mirq-linux@rere.qmqm.pl>
> ---
> v4: fix build issue reported by kernel test robot
> v3: rebased on v5.14-rc2 and reworded commitmsg
> v2: reworded commitmsg
> ---
> drivers/mmc/host/sdhci-of-arasan.c | 11 ++++-------
> drivers/mmc/host/sdhci-of-dwcmshc.c | 8 +++++---
> drivers/mmc/host/sdhci.c | 3 +--
> 3 files changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
> index 737e2bfdedc2..f2a6441ab540 100644
> --- a/drivers/mmc/host/sdhci-of-arasan.c
> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> @@ -452,8 +452,7 @@ static const struct sdhci_ops sdhci_arasan_cqe_ops = {
> static const struct sdhci_pltfm_data sdhci_arasan_cqe_pdata = {
> .ops = &sdhci_arasan_cqe_ops,
> .quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
> - .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
> - SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN,
> + .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
> };
>
> #ifdef CONFIG_PM_SLEEP
> @@ -1118,7 +1117,6 @@ static const struct sdhci_pltfm_data sdhci_arasan_pdata = {
> .ops = &sdhci_arasan_ops,
> .quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
> .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
> - SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN |
> SDHCI_QUIRK2_STOP_WITH_TC,
> };
>
> @@ -1141,7 +1139,6 @@ static const struct sdhci_pltfm_data sdhci_keembay_emmc_pdata = {
> SDHCI_QUIRK_32BIT_DMA_SIZE |
> SDHCI_QUIRK_32BIT_ADMA_SIZE,
> .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
> - SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN |
> SDHCI_QUIRK2_CAPS_BIT63_FOR_HS400 |
> SDHCI_QUIRK2_STOP_WITH_TC |
> SDHCI_QUIRK2_BROKEN_64_BIT_DMA,
> @@ -1156,7 +1153,6 @@ static const struct sdhci_pltfm_data sdhci_keembay_sd_pdata = {
> SDHCI_QUIRK_32BIT_DMA_SIZE |
> SDHCI_QUIRK_32BIT_ADMA_SIZE,
> .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
> - SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN |
> SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON |
> SDHCI_QUIRK2_STOP_WITH_TC |
> SDHCI_QUIRK2_BROKEN_64_BIT_DMA,
> @@ -1171,7 +1167,6 @@ static const struct sdhci_pltfm_data sdhci_keembay_sdio_pdata = {
> SDHCI_QUIRK_32BIT_DMA_SIZE |
> SDHCI_QUIRK_32BIT_ADMA_SIZE,
> .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
> - SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN |
> SDHCI_QUIRK2_HOST_OFF_CARD_ON |
> SDHCI_QUIRK2_BROKEN_64_BIT_DMA,
> };
> @@ -1197,7 +1192,6 @@ static struct sdhci_arasan_of_data intel_lgm_sdxc_data = {
> static const struct sdhci_pltfm_data sdhci_arasan_zynqmp_pdata = {
> .ops = &sdhci_arasan_ops,
> .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
> - SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN |
> SDHCI_QUIRK2_STOP_WITH_TC,
> };
>
> @@ -1502,6 +1496,9 @@ static int sdhci_arasan_add_host(struct sdhci_arasan_data *sdhci_arasan)
> bool dma64;
> int ret;
>
> + if (sdhci_pltfm_clk_get_max_clock(host) <= 25000000)
> + host->quirks2 |= SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN;
> +
> if (!sdhci_arasan->has_cqe)
> return sdhci_add_host(host);
>
> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> index bac874ab0b33..b6b7c4068e90 100644
> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> @@ -283,14 +283,13 @@ static const struct sdhci_pltfm_data sdhci_dwcmshc_rk3568_pdata = {
> .ops = &sdhci_dwcmshc_rk3568_ops,
> .quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN |
> SDHCI_QUIRK_BROKEN_TIMEOUT_VAL,
> - .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
> - SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN,
> + .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
> };
>
> static int dwcmshc_rk3568_init(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv)
> {
> - int err;
> struct rk3568_priv *priv = dwc_priv->priv;
> + int err;
>
> priv->rockchip_clks[0].id = "axi";
> priv->rockchip_clks[1].id = "block";
> @@ -318,6 +317,9 @@ static int dwcmshc_rk3568_init(struct sdhci_host *host, struct dwcmshc_priv *dwc
> sdhci_writel(host, 0, DWCMSHC_EMMC_DLL_TXCLK);
> sdhci_writel(host, 0, DWCMSHC_EMMC_DLL_STRBIN);
>
> + if (sdhci_pltfm_clk_get_max_clock(host) <= 25000000)
> + host->quirks2 |= SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN;
> +
> return 0;
> }
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 0993f7d0ce8e..cfa314e659bc 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1905,8 +1905,7 @@ u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
> /* Version 3.00 divisors must be a multiple of 2. */
> if (host->max_clk <= clock) {
> div = 1;
> - if ((host->quirks2 & SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN)
> - && host->max_clk <= 25000000)
> + if (host->quirks2 & SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN)
> div = 2;
> } else {
> for (div = 2; div < SDHCI_MAX_DIV_SPEC_300;
>
Re: [PATCH v4 4/5] mmc: sdhci: move SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN frequency limit [ In reply to ]
On Wed, Aug 04, 2021 at 03:33:56PM +0300, Adrian Hunter wrote:
> On 25/07/21 12:20 pm, Micha? Miros?aw wrote:
> > Push handling of clock frequency dependence for
> > SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN quirk to the drivers that use it.
> What is the max_clk dependency for, and why push it down?

I guess this is a workaround for a hardware issue. When I wrote this,
there was only a single user. Now I don't know if the second user got
the limit by accident or just uses the flag not knowing it doesn't work
as the quirk name suggests. IOW this makes it easier to fix in drivers
if the limit is wrong or irrelevant. The dependency doesn't feel like
it belongs to the generic driver anyway.

[...]
> > @@ -318,6 +317,9 @@ static int dwcmshc_rk3568_init(struct sdhci_host *host, struct dwcmshc_priv *dwc
> > sdhci_writel(host, 0, DWCMSHC_EMMC_DLL_TXCLK);
> > sdhci_writel(host, 0, DWCMSHC_EMMC_DLL_STRBIN);
> >
> > + if (sdhci_pltfm_clk_get_max_clock(host) <= 25000000)
> > + host->quirks2 |= SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN;
> > +
> > return 0;
> > }
> >
> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > index 0993f7d0ce8e..cfa314e659bc 100644
> > --- a/drivers/mmc/host/sdhci.c
> > +++ b/drivers/mmc/host/sdhci.c
> > @@ -1905,8 +1905,7 @@ u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
> > /* Version 3.00 divisors must be a multiple of 2. */
> > if (host->max_clk <= clock) {
> > div = 1;
> > - if ((host->quirks2 & SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN)
> > - && host->max_clk <= 25000000)
> > + if (host->quirks2 & SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN)
> > div = 2;
> > } else {
> > for (div = 2; div < SDHCI_MAX_DIV_SPEC_300;
Re: [PATCH v4 4/5] mmc: sdhci: move SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN frequency limit [ In reply to ]
On 7/08/21 5:12 pm, Micha? Miros?aw wrote:
> On Wed, Aug 04, 2021 at 03:33:56PM +0300, Adrian Hunter wrote:
>> On 25/07/21 12:20 pm, Micha? Miros?aw wrote:
>>> Push handling of clock frequency dependence for
>>> SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN quirk to the drivers that use it.
>> What is the max_clk dependency for, and why push it down?
>
> I guess this is a workaround for a hardware issue. When I wrote this,
> there was only a single user. Now I don't know if the second user got
> the limit by accident or just uses the flag not knowing it doesn't work
> as the quirk name suggests. IOW this makes it easier to fix in drivers
> if the limit is wrong or irrelevant. The dependency doesn't feel like
> it belongs to the generic driver anyway.

Would you mind reaching out to the authors of the relevant patches
and drivers to try to find out the purpose of the max_clk dependency,
before we make any changes?


>
> [...]
>>> @@ -318,6 +317,9 @@ static int dwcmshc_rk3568_init(struct sdhci_host *host, struct dwcmshc_priv *dwc
>>> sdhci_writel(host, 0, DWCMSHC_EMMC_DLL_TXCLK);
>>> sdhci_writel(host, 0, DWCMSHC_EMMC_DLL_STRBIN);
>>>
>>> + if (sdhci_pltfm_clk_get_max_clock(host) <= 25000000)
>>> + host->quirks2 |= SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN;
>>> +
>>> return 0;
>>> }
>>>
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index 0993f7d0ce8e..cfa314e659bc 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -1905,8 +1905,7 @@ u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
>>> /* Version 3.00 divisors must be a multiple of 2. */
>>> if (host->max_clk <= clock) {
>>> div = 1;
>>> - if ((host->quirks2 & SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN)
>>> - && host->max_clk <= 25000000)
>>> + if (host->quirks2 & SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN)
>>> div = 2;
>>> } else {
>>> for (div = 2; div < SDHCI_MAX_DIV_SPEC_300;