| Message ID | 20211206142929.26729-4-marten.lindahl@axis.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add ARTPEC-8 support to DWMMC controller | expand |
On 06/12/2021 15:29, Mårten Lindahl wrote: > Current dw_mci driver supports a TMOUT register which consists of a 24 > bit field (TMOUT[31:8]) for the DATA_TIMEOUT. The maximum value of this > field is 0xFFFFFF, which with a 200MHz clock will give a full DRTO of: > > 0xFFFFFF / 200000000 => ~84 ms > > However, the ARTPEC-8 SoC DWMMC IP version has a TMOUT register with an > extended DATA_TIMEOUT field, which supports longer timers for the DRTO. > In this version the DATA_TIMEOUT field is split into two, which with the > same 200MHz clock as above will allow a maximum timeout of: > > ((TMOUT[10:8] -1) * 0xFFFFFF + TMOUT[31:11] * 8) / 200000000 => ~587 ms > > Add a quirk to support this. The quirk is enabled for ARTPEC-8 SoCs. > > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com> > --- > > v2: > - Removed unnecessary comment > - Change 1<<0 to BIT(0) > > drivers/mmc/host/dw_mmc-exynos.c | 5 +++++ > drivers/mmc/host/dw_mmc.c | 33 ++++++++++++++++++++++++++++---- > drivers/mmc/host/dw_mmc.h | 6 ++++++ > 3 files changed, 40 insertions(+), 4 deletions(-) > Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> Best regards, Krzysztof
On Mon, 6 Dec 2021 at 15:29, Mårten Lindahl <marten.lindahl@axis.com> wrote: > > Current dw_mci driver supports a TMOUT register which consists of a 24 > bit field (TMOUT[31:8]) for the DATA_TIMEOUT. The maximum value of this > field is 0xFFFFFF, which with a 200MHz clock will give a full DRTO of: > > 0xFFFFFF / 200000000 => ~84 ms > > However, the ARTPEC-8 SoC DWMMC IP version has a TMOUT register with an > extended DATA_TIMEOUT field, which supports longer timers for the DRTO. > In this version the DATA_TIMEOUT field is split into two, which with the > same 200MHz clock as above will allow a maximum timeout of: > > ((TMOUT[10:8] -1) * 0xFFFFFF + TMOUT[31:11] * 8) / 200000000 => ~587 ms > > Add a quirk to support this. The quirk is enabled for ARTPEC-8 SoCs. > > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com> > --- > > v2: > - Removed unnecessary comment > - Change 1<<0 to BIT(0) > > drivers/mmc/host/dw_mmc-exynos.c | 5 +++++ > drivers/mmc/host/dw_mmc.c | 33 ++++++++++++++++++++++++++++---- > drivers/mmc/host/dw_mmc.h | 6 ++++++ > 3 files changed, 40 insertions(+), 4 deletions(-) > > diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c > index 86486e6659de..1b625642c5b4 100644 > --- a/drivers/mmc/host/dw_mmc-exynos.c > +++ b/drivers/mmc/host/dw_mmc-exynos.c > @@ -127,6 +127,11 @@ static int dw_mci_exynos_priv_init(struct dw_mci *host) > DQS_CTRL_GET_RD_DELAY(priv->saved_strobe_ctrl); > } > > + if (priv->ctrl_type == DW_MCI_TYPE_ARTPEC8) { > + /* Quirk needed for ARTPEC-8 SoCs */ > + host->quirks |= DW_MMC_QUIRK_EXTENDED_TMOUT; > + } > + > host->bus_hz /= (priv->ciu_div + 1); > > return 0; > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > index f2a14a434bef..45ea9fd97a6a 100644 > --- a/drivers/mmc/host/dw_mmc.c > +++ b/drivers/mmc/host/dw_mmc.c > @@ -1289,6 +1289,7 @@ static void dw_mci_set_data_timeout(struct dw_mci *host, > { > u32 clk_div, tmout; > u64 tmp; > + unsigned int tmp2; > > clk_div = (mci_readl(host, CLKDIV) & 0xFF) * 2; > if (clk_div == 0) > @@ -1301,10 +1302,28 @@ static void dw_mci_set_data_timeout(struct dw_mci *host, > tmout = 0xFF; /* Set maximum */ > > /* TMOUT[31:8] (DATA_TIMEOUT) */ > - if (!tmp || tmp > 0xFFFFFF) > - tmout |= (0xFFFFFF << 8); > - else > - tmout |= (tmp & 0xFFFFFF) << 8; > + if (host->quirks & DW_MMC_QUIRK_EXTENDED_TMOUT) { Adding an option for dealing with quirks, should be avoided if possible. That's because we want to avoid sprinkling common dw_mmc code with variant specific code, as it will sooner or later turn into a nightmare to maintain. In this case, I suggest you look into extending the struct dw_mci_drv_data with some new callback (perhaps ->set_data_timeout()) and then implement it for your variant. If that callback is present, it should take precedence over the generic dw_mci_set_data_timeout(), if you get what I mean. Moreover, if some common dw_mmc code needs to be called from your callback, I suggest we make that code available through exported dw_mmc library functions instead. > + /* > + * Extended HW timer (max = 0x6FFFFF2): > + * ((TMOUT[10:8] - 1) * 0xFFFFFF + TMOUT[31:11] * 8) > + */ > + if (!tmp || tmp > 0x6FFFFF2) > + tmout |= (0xFFFFFF << 8); > + else { > + /* TMOUT[10:8] */ > + tmp2 = (((unsigned int)tmp / 0xFFFFFF) + 1) & 0x7; > + tmout |= tmp2 << 8; > + > + /* TMOUT[31:11] */ > + tmp = tmp - ((tmp2 - 1) * 0xFFFFFF); > + tmout |= (tmp & 0xFFFFF8) << 8; > + } > + } else { > + if (!tmp || tmp > 0xFFFFFF) > + tmout |= (0xFFFFFF << 8); > + else > + tmout |= (tmp & 0xFFFFFF) << 8; > + } > > mci_writel(host, TMOUT, tmout); > dev_dbg(host->dev, "timeout_ns: %u => TMOUT[31:8]: 0x%#08x", > @@ -2005,9 +2024,15 @@ static void dw_mci_set_drto(struct dw_mci *host) > if (drto_div == 0) > drto_div = 1; > > + if (host->quirks & DW_MMC_QUIRK_EXTENDED_TMOUT) > + drto_clks = (((drto_clks & 0x7) - 1) * 0xFFFFFF) + > + ((drto_clks & 0xFFFFF8)); > + > drto_ms = DIV_ROUND_UP_ULL((u64)MSEC_PER_SEC * drto_clks * drto_div, > host->bus_hz); > > + dev_dbg(host->dev, "drto_ms: %u\n", drto_ms); > + > /* add a bit spare time */ > drto_ms += 10; > > diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h > index 771d5afa3136..3b6510d4a684 100644 > --- a/drivers/mmc/host/dw_mmc.h > +++ b/drivers/mmc/host/dw_mmc.h > @@ -118,6 +118,7 @@ struct dw_mci_dma_slave { > * @part_buf: Simple buffer for partial fifo reads/writes. > * @push_data: Pointer to FIFO push function. > * @pull_data: Pointer to FIFO pull function. > + * @quirks: Set of quirks that apply to specific versions of the IP. > * @vqmmc_enabled: Status of vqmmc, should be true or false. > * @irq_flags: The flags to be passed to request_irq. > * @irq: The irq value to be passed to request_irq. > @@ -223,6 +224,8 @@ struct dw_mci { > void (*push_data)(struct dw_mci *host, void *buf, int cnt); > void (*pull_data)(struct dw_mci *host, void *buf, int cnt); > > + u32 quirks; > + > bool vqmmc_enabled; > unsigned long irq_flags; /* IRQ flags */ > int irq; > @@ -274,6 +277,9 @@ struct dw_mci_board { > struct dma_pdata *data; > }; > > +/* Support for longer data read timeout */ > +#define DW_MMC_QUIRK_EXTENDED_TMOUT BIT(0) > + > #define DW_MMC_240A 0x240a > #define DW_MMC_280A 0x280a > Kind regards Uffe
On Wed, Dec 08, 2021 at 03:41:24PM +0100, Ulf Hansson wrote: > On Mon, 6 Dec 2021 at 15:29, Mårten Lindahl <marten.lindahl@axis.com> wrote: > > > > Current dw_mci driver supports a TMOUT register which consists of a 24 > > bit field (TMOUT[31:8]) for the DATA_TIMEOUT. The maximum value of this > > field is 0xFFFFFF, which with a 200MHz clock will give a full DRTO of: > > > > 0xFFFFFF / 200000000 => ~84 ms > > > > However, the ARTPEC-8 SoC DWMMC IP version has a TMOUT register with an > > extended DATA_TIMEOUT field, which supports longer timers for the DRTO. > > In this version the DATA_TIMEOUT field is split into two, which with the > > same 200MHz clock as above will allow a maximum timeout of: > > > > ((TMOUT[10:8] -1) * 0xFFFFFF + TMOUT[31:11] * 8) / 200000000 => ~587 ms > > > > Add a quirk to support this. The quirk is enabled for ARTPEC-8 SoCs. > > > > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com> > > --- > > > > v2: > > - Removed unnecessary comment > > - Change 1<<0 to BIT(0) > > > > drivers/mmc/host/dw_mmc-exynos.c | 5 +++++ > > drivers/mmc/host/dw_mmc.c | 33 ++++++++++++++++++++++++++++---- > > drivers/mmc/host/dw_mmc.h | 6 ++++++ > > 3 files changed, 40 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c > > index 86486e6659de..1b625642c5b4 100644 > > --- a/drivers/mmc/host/dw_mmc-exynos.c > > +++ b/drivers/mmc/host/dw_mmc-exynos.c > > @@ -127,6 +127,11 @@ static int dw_mci_exynos_priv_init(struct dw_mci *host) > > DQS_CTRL_GET_RD_DELAY(priv->saved_strobe_ctrl); > > } > > > > + if (priv->ctrl_type == DW_MCI_TYPE_ARTPEC8) { > > + /* Quirk needed for ARTPEC-8 SoCs */ > > + host->quirks |= DW_MMC_QUIRK_EXTENDED_TMOUT; > > + } > > + > > host->bus_hz /= (priv->ciu_div + 1); > > > > return 0; > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > > index f2a14a434bef..45ea9fd97a6a 100644 > > --- a/drivers/mmc/host/dw_mmc.c > > +++ b/drivers/mmc/host/dw_mmc.c > > @@ -1289,6 +1289,7 @@ static void dw_mci_set_data_timeout(struct dw_mci *host, > > { > > u32 clk_div, tmout; > > u64 tmp; > > + unsigned int tmp2; > > > > clk_div = (mci_readl(host, CLKDIV) & 0xFF) * 2; > > if (clk_div == 0) > > @@ -1301,10 +1302,28 @@ static void dw_mci_set_data_timeout(struct dw_mci *host, > > tmout = 0xFF; /* Set maximum */ > > > > /* TMOUT[31:8] (DATA_TIMEOUT) */ > > - if (!tmp || tmp > 0xFFFFFF) > > - tmout |= (0xFFFFFF << 8); > > - else > > - tmout |= (tmp & 0xFFFFFF) << 8; > > + if (host->quirks & DW_MMC_QUIRK_EXTENDED_TMOUT) { > > Adding an option for dealing with quirks, should be avoided if > possible. That's because we want to avoid sprinkling common dw_mmc > code with variant specific code, as it will sooner or later turn into > a nightmare to maintain. > > In this case, I suggest you look into extending the struct > dw_mci_drv_data with some new callback (perhaps ->set_data_timeout()) > and then implement it for your variant. > If that callback is present, it should take precedence over the > generic dw_mci_set_data_timeout(), if you get what I mean. Hi Ulf! Thanks! That's a good idea. I will implement the callback approach instead of a quirk for this patch. It will be two callbacks; one for the set_data_timeout(), and one for the set_drto(). > > Moreover, if some common dw_mmc code needs to be called from your > callback, I suggest we make that code available through exported > dw_mmc library functions instead. > I don't think I need to export anything for this patch. Kind regards Mårten > > + /* > > + * Extended HW timer (max = 0x6FFFFF2): > > + * ((TMOUT[10:8] - 1) * 0xFFFFFF + TMOUT[31:11] * 8) > > + */ > > + if (!tmp || tmp > 0x6FFFFF2) > > + tmout |= (0xFFFFFF << 8); > > + else { > > + /* TMOUT[10:8] */ > > + tmp2 = (((unsigned int)tmp / 0xFFFFFF) + 1) & 0x7; > > + tmout |= tmp2 << 8; > > + > > + /* TMOUT[31:11] */ > > + tmp = tmp - ((tmp2 - 1) * 0xFFFFFF); > > + tmout |= (tmp & 0xFFFFF8) << 8; > > + } > > + } else { > > + if (!tmp || tmp > 0xFFFFFF) > > + tmout |= (0xFFFFFF << 8); > > + else > > + tmout |= (tmp & 0xFFFFFF) << 8; > > + } > > > > mci_writel(host, TMOUT, tmout); > > dev_dbg(host->dev, "timeout_ns: %u => TMOUT[31:8]: 0x%#08x", > > @@ -2005,9 +2024,15 @@ static void dw_mci_set_drto(struct dw_mci *host) > > if (drto_div == 0) > > drto_div = 1; > > > > + if (host->quirks & DW_MMC_QUIRK_EXTENDED_TMOUT) > > + drto_clks = (((drto_clks & 0x7) - 1) * 0xFFFFFF) + > > + ((drto_clks & 0xFFFFF8)); > > + > > drto_ms = DIV_ROUND_UP_ULL((u64)MSEC_PER_SEC * drto_clks * drto_div, > > host->bus_hz); > > > > + dev_dbg(host->dev, "drto_ms: %u\n", drto_ms); > > + > > /* add a bit spare time */ > > drto_ms += 10; > > > > diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h > > index 771d5afa3136..3b6510d4a684 100644 > > --- a/drivers/mmc/host/dw_mmc.h > > +++ b/drivers/mmc/host/dw_mmc.h > > @@ -118,6 +118,7 @@ struct dw_mci_dma_slave { > > * @part_buf: Simple buffer for partial fifo reads/writes. > > * @push_data: Pointer to FIFO push function. > > * @pull_data: Pointer to FIFO pull function. > > + * @quirks: Set of quirks that apply to specific versions of the IP. > > * @vqmmc_enabled: Status of vqmmc, should be true or false. > > * @irq_flags: The flags to be passed to request_irq. > > * @irq: The irq value to be passed to request_irq. > > @@ -223,6 +224,8 @@ struct dw_mci { > > void (*push_data)(struct dw_mci *host, void *buf, int cnt); > > void (*pull_data)(struct dw_mci *host, void *buf, int cnt); > > > > + u32 quirks; > > + > > bool vqmmc_enabled; > > unsigned long irq_flags; /* IRQ flags */ > > int irq; > > @@ -274,6 +277,9 @@ struct dw_mci_board { > > struct dma_pdata *data; > > }; > > > > +/* Support for longer data read timeout */ > > +#define DW_MMC_QUIRK_EXTENDED_TMOUT BIT(0) > > + > > #define DW_MMC_240A 0x240a > > #define DW_MMC_280A 0x280a > > > > Kind regards > Uffe
diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c index 86486e6659de..1b625642c5b4 100644 --- a/drivers/mmc/host/dw_mmc-exynos.c +++ b/drivers/mmc/host/dw_mmc-exynos.c @@ -127,6 +127,11 @@ static int dw_mci_exynos_priv_init(struct dw_mci *host) DQS_CTRL_GET_RD_DELAY(priv->saved_strobe_ctrl); } + if (priv->ctrl_type == DW_MCI_TYPE_ARTPEC8) { + /* Quirk needed for ARTPEC-8 SoCs */ + host->quirks |= DW_MMC_QUIRK_EXTENDED_TMOUT; + } + host->bus_hz /= (priv->ciu_div + 1); return 0; diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c index f2a14a434bef..45ea9fd97a6a 100644 --- a/drivers/mmc/host/dw_mmc.c +++ b/drivers/mmc/host/dw_mmc.c @@ -1289,6 +1289,7 @@ static void dw_mci_set_data_timeout(struct dw_mci *host, { u32 clk_div, tmout; u64 tmp; + unsigned int tmp2; clk_div = (mci_readl(host, CLKDIV) & 0xFF) * 2; if (clk_div == 0) @@ -1301,10 +1302,28 @@ static void dw_mci_set_data_timeout(struct dw_mci *host, tmout = 0xFF; /* Set maximum */ /* TMOUT[31:8] (DATA_TIMEOUT) */ - if (!tmp || tmp > 0xFFFFFF) - tmout |= (0xFFFFFF << 8); - else - tmout |= (tmp & 0xFFFFFF) << 8; + if (host->quirks & DW_MMC_QUIRK_EXTENDED_TMOUT) { + /* + * Extended HW timer (max = 0x6FFFFF2): + * ((TMOUT[10:8] - 1) * 0xFFFFFF + TMOUT[31:11] * 8) + */ + if (!tmp || tmp > 0x6FFFFF2) + tmout |= (0xFFFFFF << 8); + else { + /* TMOUT[10:8] */ + tmp2 = (((unsigned int)tmp / 0xFFFFFF) + 1) & 0x7; + tmout |= tmp2 << 8; + + /* TMOUT[31:11] */ + tmp = tmp - ((tmp2 - 1) * 0xFFFFFF); + tmout |= (tmp & 0xFFFFF8) << 8; + } + } else { + if (!tmp || tmp > 0xFFFFFF) + tmout |= (0xFFFFFF << 8); + else + tmout |= (tmp & 0xFFFFFF) << 8; + } mci_writel(host, TMOUT, tmout); dev_dbg(host->dev, "timeout_ns: %u => TMOUT[31:8]: 0x%#08x", @@ -2005,9 +2024,15 @@ static void dw_mci_set_drto(struct dw_mci *host) if (drto_div == 0) drto_div = 1; + if (host->quirks & DW_MMC_QUIRK_EXTENDED_TMOUT) + drto_clks = (((drto_clks & 0x7) - 1) * 0xFFFFFF) + + ((drto_clks & 0xFFFFF8)); + drto_ms = DIV_ROUND_UP_ULL((u64)MSEC_PER_SEC * drto_clks * drto_div, host->bus_hz); + dev_dbg(host->dev, "drto_ms: %u\n", drto_ms); + /* add a bit spare time */ drto_ms += 10; diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h index 771d5afa3136..3b6510d4a684 100644 --- a/drivers/mmc/host/dw_mmc.h +++ b/drivers/mmc/host/dw_mmc.h @@ -118,6 +118,7 @@ struct dw_mci_dma_slave { * @part_buf: Simple buffer for partial fifo reads/writes. * @push_data: Pointer to FIFO push function. * @pull_data: Pointer to FIFO pull function. + * @quirks: Set of quirks that apply to specific versions of the IP. * @vqmmc_enabled: Status of vqmmc, should be true or false. * @irq_flags: The flags to be passed to request_irq. * @irq: The irq value to be passed to request_irq. @@ -223,6 +224,8 @@ struct dw_mci { void (*push_data)(struct dw_mci *host, void *buf, int cnt); void (*pull_data)(struct dw_mci *host, void *buf, int cnt); + u32 quirks; + bool vqmmc_enabled; unsigned long irq_flags; /* IRQ flags */ int irq; @@ -274,6 +277,9 @@ struct dw_mci_board { struct dma_pdata *data; }; +/* Support for longer data read timeout */ +#define DW_MMC_QUIRK_EXTENDED_TMOUT BIT(0) + #define DW_MMC_240A 0x240a #define DW_MMC_280A 0x280a
Current dw_mci driver supports a TMOUT register which consists of a 24 bit field (TMOUT[31:8]) for the DATA_TIMEOUT. The maximum value of this field is 0xFFFFFF, which with a 200MHz clock will give a full DRTO of: 0xFFFFFF / 200000000 => ~84 ms However, the ARTPEC-8 SoC DWMMC IP version has a TMOUT register with an extended DATA_TIMEOUT field, which supports longer timers for the DRTO. In this version the DATA_TIMEOUT field is split into two, which with the same 200MHz clock as above will allow a maximum timeout of: ((TMOUT[10:8] -1) * 0xFFFFFF + TMOUT[31:11] * 8) / 200000000 => ~587 ms Add a quirk to support this. The quirk is enabled for ARTPEC-8 SoCs. Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com> --- v2: - Removed unnecessary comment - Change 1<<0 to BIT(0) drivers/mmc/host/dw_mmc-exynos.c | 5 +++++ drivers/mmc/host/dw_mmc.c | 33 ++++++++++++++++++++++++++++---- drivers/mmc/host/dw_mmc.h | 6 ++++++ 3 files changed, 40 insertions(+), 4 deletions(-)