* Re: [PATCH v3] mmc: dw_mmc: Allow lower TMOUT value than maximum
2021-11-10 11:05 ` [PATCH v3] mmc: dw_mmc: Allow lower TMOUT value than maximum Mårten Lindahl
@ 2021-11-10 16:12 ` Doug Anderson
2021-11-10 21:54 ` Jaehoon Chung
2021-11-15 14:54 ` Ulf Hansson
2 siblings, 0 replies; 6+ messages in thread
From: Doug Anderson @ 2021-11-10 16:12 UTC (permalink / raw)
To: Mårten Lindahl; +Cc: Jaehoon Chung, Ulf Hansson, kernel, linux-mmc
Hi,
On Wed, Nov 10, 2021 at 3:05 AM Mårten Lindahl <marten.lindahl@axis.com> wrote:
>
> The TMOUT register is always set with a full value for every transfer,
> which (with a 200MHz clock) will give a full DRTO of ~84 milliseconds.
> Since the software dto_timer acts as a backup in cases when this timeout
> is not long enough, it is normally not a problem.
I'm not 100% sure about what the part about "the software dto_timer
acts as a backup in cases when this timeout is not long enough" means.
At the moment the software fallback should be set to the same as the
hardware value, right? So the software backup doesn't help make the
timeout longer...
> But setting a full
> value makes it impossible to test shorter timeouts, when for example
> testing data read times on different SD cards.
>
> Add a function to set any value smaller than the maximum of 0xFFFFFF.
>
> Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
> ---
>
> v2:
> - Calculate new value before checking boundaries
> - Include CLKDIV register to get proper value
>
> v3:
> - Use 'if-else' instead of 'goto'
> - Don't touch response field when maximize data field
>
> drivers/mmc/host/dw_mmc.c | 28 +++++++++++++++++++++++++++-
> 1 file changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 6578cc64ae9e..637ae2aea9fa 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1283,6 +1283,32 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
> mci_writel(host, CTYPE, (slot->ctype << slot->id));
> }
>
> +static void dw_mci_set_data_timeout(struct dw_mci *host,
> + unsigned int timeout_ns)
> +{
> + unsigned int clk_div, tmp, tmout;
> +
> + clk_div = (mci_readl(host, CLKDIV) & 0xFF) * 2;
> + if (clk_div == 0)
> + clk_div = 1;
> +
> + tmp = DIV_ROUND_UP_ULL((u64)timeout_ns * host->bus_hz,
> + NSEC_PER_SEC * clk_div);
> +
> + /* TMOUT[7:0] (RESPONSE_TIMEOUT) */
> + tmout = 0xFF; /* Set maximum */
> +
> + /* TMOUT[31:8] (DATA_TIMEOUT) */
> + 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%06x",
> + timeout_ns, tmout >> 8);
nit: In theory 0x%06x is slightly better written as %#08x. I wouldn't
spin the patch just for that, though.
In any case, I think this is fine to the best of my knowledge of how
this timeout is supposed to work, so I'm happy with:
Reviewed-by: Douglas Anderson <dianders@chromium.org>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v3] mmc: dw_mmc: Allow lower TMOUT value than maximum
2021-11-10 11:05 ` [PATCH v3] mmc: dw_mmc: Allow lower TMOUT value than maximum Mårten Lindahl
2021-11-10 16:12 ` Doug Anderson
@ 2021-11-10 21:54 ` Jaehoon Chung
2021-11-15 14:54 ` Ulf Hansson
2 siblings, 0 replies; 6+ messages in thread
From: Jaehoon Chung @ 2021-11-10 21:54 UTC (permalink / raw)
To: Mårten Lindahl, Ulf Hansson; +Cc: Doug Anderson, kernel, linux-mmc
Hi Marten,
On 11/10/21 8:05 PM, Mårten Lindahl wrote:
> The TMOUT register is always set with a full value for every transfer,
> which (with a 200MHz clock) will give a full DRTO of ~84 milliseconds.
> Since the software dto_timer acts as a backup in cases when this timeout
> is not long enough, it is normally not a problem. But setting a full
> value makes it impossible to test shorter timeouts, when for example
> testing data read times on different SD cards.
>
> Add a function to set any value smaller than the maximum of 0xFFFFFF.
>
> Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
Best Regards,
Jaehoon Chung
> ---
>
> v2:
> - Calculate new value before checking boundaries
> - Include CLKDIV register to get proper value
>
> v3:
> - Use 'if-else' instead of 'goto'
> - Don't touch response field when maximize data field
>
> drivers/mmc/host/dw_mmc.c | 28 +++++++++++++++++++++++++++-
> 1 file changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 6578cc64ae9e..637ae2aea9fa 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1283,6 +1283,32 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
> mci_writel(host, CTYPE, (slot->ctype << slot->id));
> }
>
> +static void dw_mci_set_data_timeout(struct dw_mci *host,
> + unsigned int timeout_ns)
> +{
> + unsigned int clk_div, tmp, tmout;
> +
> + clk_div = (mci_readl(host, CLKDIV) & 0xFF) * 2;
> + if (clk_div == 0)
> + clk_div = 1;
> +
> + tmp = DIV_ROUND_UP_ULL((u64)timeout_ns * host->bus_hz,
> + NSEC_PER_SEC * clk_div);
> +
> + /* TMOUT[7:0] (RESPONSE_TIMEOUT) */
> + tmout = 0xFF; /* Set maximum */
> +
> + /* TMOUT[31:8] (DATA_TIMEOUT) */
> + 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%06x",
> + timeout_ns, tmout >> 8);
> +}
> +
> static void __dw_mci_start_request(struct dw_mci *host,
> struct dw_mci_slot *slot,
> struct mmc_command *cmd)
> @@ -1303,7 +1329,7 @@ static void __dw_mci_start_request(struct dw_mci *host,
>
> data = cmd->data;
> if (data) {
> - mci_writel(host, TMOUT, 0xFFFFFFFF);
> + dw_mci_set_data_timeout(host, data->timeout_ns);
> mci_writel(host, BYTCNT, data->blksz*data->blocks);
> mci_writel(host, BLKSIZ, data->blksz);
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v3] mmc: dw_mmc: Allow lower TMOUT value than maximum
2021-11-10 11:05 ` [PATCH v3] mmc: dw_mmc: Allow lower TMOUT value than maximum Mårten Lindahl
2021-11-10 16:12 ` Doug Anderson
2021-11-10 21:54 ` Jaehoon Chung
@ 2021-11-15 14:54 ` Ulf Hansson
2021-11-16 16:59 ` Marten Lindahl
2 siblings, 1 reply; 6+ messages in thread
From: Ulf Hansson @ 2021-11-15 14:54 UTC (permalink / raw)
To: Mårten Lindahl; +Cc: Jaehoon Chung, Doug Anderson, kernel, linux-mmc
On Wed, 10 Nov 2021 at 12:05, Mårten Lindahl <marten.lindahl@axis.com> wrote:
>
> The TMOUT register is always set with a full value for every transfer,
> which (with a 200MHz clock) will give a full DRTO of ~84 milliseconds.
> Since the software dto_timer acts as a backup in cases when this timeout
> is not long enough, it is normally not a problem. But setting a full
> value makes it impossible to test shorter timeouts, when for example
> testing data read times on different SD cards.
>
> Add a function to set any value smaller than the maximum of 0xFFFFFF.
>
> Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
Applied for next, thanks!
Kind regards
Uffe
> ---
>
> v2:
> - Calculate new value before checking boundaries
> - Include CLKDIV register to get proper value
>
> v3:
> - Use 'if-else' instead of 'goto'
> - Don't touch response field when maximize data field
>
> drivers/mmc/host/dw_mmc.c | 28 +++++++++++++++++++++++++++-
> 1 file changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 6578cc64ae9e..637ae2aea9fa 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1283,6 +1283,32 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
> mci_writel(host, CTYPE, (slot->ctype << slot->id));
> }
>
> +static void dw_mci_set_data_timeout(struct dw_mci *host,
> + unsigned int timeout_ns)
> +{
> + unsigned int clk_div, tmp, tmout;
> +
> + clk_div = (mci_readl(host, CLKDIV) & 0xFF) * 2;
> + if (clk_div == 0)
> + clk_div = 1;
> +
> + tmp = DIV_ROUND_UP_ULL((u64)timeout_ns * host->bus_hz,
> + NSEC_PER_SEC * clk_div);
> +
> + /* TMOUT[7:0] (RESPONSE_TIMEOUT) */
> + tmout = 0xFF; /* Set maximum */
> +
> + /* TMOUT[31:8] (DATA_TIMEOUT) */
> + 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%06x",
> + timeout_ns, tmout >> 8);
> +}
> +
> static void __dw_mci_start_request(struct dw_mci *host,
> struct dw_mci_slot *slot,
> struct mmc_command *cmd)
> @@ -1303,7 +1329,7 @@ static void __dw_mci_start_request(struct dw_mci *host,
>
> data = cmd->data;
> if (data) {
> - mci_writel(host, TMOUT, 0xFFFFFFFF);
> + dw_mci_set_data_timeout(host, data->timeout_ns);
> mci_writel(host, BYTCNT, data->blksz*data->blocks);
> mci_writel(host, BLKSIZ, data->blksz);
> }
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v3] mmc: dw_mmc: Allow lower TMOUT value than maximum
2021-11-15 14:54 ` Ulf Hansson
@ 2021-11-16 16:59 ` Marten Lindahl
2021-11-17 9:36 ` Ulf Hansson
0 siblings, 1 reply; 6+ messages in thread
From: Marten Lindahl @ 2021-11-16 16:59 UTC (permalink / raw)
To: Ulf Hansson
Cc: Mårten Lindahl, Jaehoon Chung, Doug Anderson, kernel,
linux-mmc@vger.kernel.org
On Mon, Nov 15, 2021 at 03:54:19PM +0100, Ulf Hansson wrote:
> On Wed, 10 Nov 2021 at 12:05, Mårten Lindahl <marten.lindahl@axis.com> wrote:
> >
> > The TMOUT register is always set with a full value for every transfer,
> > which (with a 200MHz clock) will give a full DRTO of ~84 milliseconds.
> > Since the software dto_timer acts as a backup in cases when this timeout
> > is not long enough, it is normally not a problem. But setting a full
> > value makes it impossible to test shorter timeouts, when for example
> > testing data read times on different SD cards.
> >
> > Add a function to set any value smaller than the maximum of 0xFFFFFF.
> >
> > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
>
> Applied for next, thanks!
>
> Kind regards
> Uffe
>
Hi Ulf!
During tests on some units I found that there is a potential overflow
of the 32bit divider in this patch (if clk_div > 4).
Is it possible to stop this patch, until I created a new version of it?
I guess it's better to update this patch, in favour of sending a new
correction patch for it?
Kind regards
Mårten
>
> > ---
> >
> > v2:
> > - Calculate new value before checking boundaries
> > - Include CLKDIV register to get proper value
> >
> > v3:
> > - Use 'if-else' instead of 'goto'
> > - Don't touch response field when maximize data field
> >
> > drivers/mmc/host/dw_mmc.c | 28 +++++++++++++++++++++++++++-
> > 1 file changed, 27 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> > index 6578cc64ae9e..637ae2aea9fa 100644
> > --- a/drivers/mmc/host/dw_mmc.c
> > +++ b/drivers/mmc/host/dw_mmc.c
> > @@ -1283,6 +1283,32 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
> > mci_writel(host, CTYPE, (slot->ctype << slot->id));
> > }
> >
> > +static void dw_mci_set_data_timeout(struct dw_mci *host,
> > + unsigned int timeout_ns)
> > +{
> > + unsigned int clk_div, tmp, tmout;
> > +
> > + clk_div = (mci_readl(host, CLKDIV) & 0xFF) * 2;
> > + if (clk_div == 0)
> > + clk_div = 1;
> > +
> > + tmp = DIV_ROUND_UP_ULL((u64)timeout_ns * host->bus_hz,
> > + NSEC_PER_SEC * clk_div);
> > +
> > + /* TMOUT[7:0] (RESPONSE_TIMEOUT) */
> > + tmout = 0xFF; /* Set maximum */
> > +
> > + /* TMOUT[31:8] (DATA_TIMEOUT) */
> > + 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%06x",
> > + timeout_ns, tmout >> 8);
> > +}
> > +
> > static void __dw_mci_start_request(struct dw_mci *host,
> > struct dw_mci_slot *slot,
> > struct mmc_command *cmd)
> > @@ -1303,7 +1329,7 @@ static void __dw_mci_start_request(struct dw_mci *host,
> >
> > data = cmd->data;
> > if (data) {
> > - mci_writel(host, TMOUT, 0xFFFFFFFF);
> > + dw_mci_set_data_timeout(host, data->timeout_ns);
> > mci_writel(host, BYTCNT, data->blksz*data->blocks);
> > mci_writel(host, BLKSIZ, data->blksz);
> > }
> > --
> > 2.20.1
> >
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v3] mmc: dw_mmc: Allow lower TMOUT value than maximum
2021-11-16 16:59 ` Marten Lindahl
@ 2021-11-17 9:36 ` Ulf Hansson
0 siblings, 0 replies; 6+ messages in thread
From: Ulf Hansson @ 2021-11-17 9:36 UTC (permalink / raw)
To: Marten Lindahl
Cc: Mårten Lindahl, Jaehoon Chung, Doug Anderson, kernel,
linux-mmc@vger.kernel.org
On Tue, 16 Nov 2021 at 17:59, Marten Lindahl <martenli@axis.com> wrote:
>
> On Mon, Nov 15, 2021 at 03:54:19PM +0100, Ulf Hansson wrote:
> > On Wed, 10 Nov 2021 at 12:05, Mårten Lindahl <marten.lindahl@axis.com> wrote:
> > >
> > > The TMOUT register is always set with a full value for every transfer,
> > > which (with a 200MHz clock) will give a full DRTO of ~84 milliseconds.
> > > Since the software dto_timer acts as a backup in cases when this timeout
> > > is not long enough, it is normally not a problem. But setting a full
> > > value makes it impossible to test shorter timeouts, when for example
> > > testing data read times on different SD cards.
> > >
> > > Add a function to set any value smaller than the maximum of 0xFFFFFF.
> > >
> > > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
> >
> > Applied for next, thanks!
> >
> > Kind regards
> > Uffe
> >
>
> Hi Ulf!
>
> During tests on some units I found that there is a potential overflow
> of the 32bit divider in this patch (if clk_div > 4).
>
> Is it possible to stop this patch, until I created a new version of it?
> I guess it's better to update this patch, in favour of sending a new
> correction patch for it?
I have dropped the patch from my next branch. Please send a v4.
[...]
Kind regards
Uffe
^ permalink raw reply [flat|nested] 6+ messages in thread