Re: [PATCH v2 7/9] spi: spi-s3c64xx: Ensure cur_speed holds actual clock value

[Tomasz Figa]

On Tue, Aug 25, 2020 at 11:02 AM Lukasz Stelmach <l.stelmach@xxxxxxxxxxx> wrote:
>
> It was <2020-08-24 pon 15:21>, when Tomasz Figa wrote:
> > On Mon, Aug 24, 2020 at 3:17 PM Lukasz Stelmach <l.stelmach@xxxxxxxxxxx> wrote:
> >>
> >> It was <2020-08-22 sob 14:43>, when Krzysztof Kozlowski wrote:
> >> > On Fri, Aug 21, 2020 at 06:13:59PM +0200, Łukasz Stelmach wrote:
> >> >> cur_speed is used to calculate transfer timeout and needs to be
> >> >> set to the actual value of (half) the clock speed for precise
> >> >> calculations.
> >> >
> >> > If you need this only for timeout calculation just divide it in
> >> > s3c64xx_wait_for_dma().
> >>
> >> I divide it here to keep the relationship between the value the variable
> >> holds and the one that is inside clk_* (See? It's multiplied 3 lines
> >> above). If you look around every single clk_get_rate() call in the file is
> >> divided by two.
> >>
> >> > Otherwise why only if (cmu) case is updated?
> >>
> >> You are righ I will update that too.
> >>
> >> However, I wonder if it is even possible that the value read from
> >> S3C64XX_SPI_CLK_CFG would be different than the one written to it?
> >>
> >
> > It is not possible for the register itself, but please see my other
> > reply, where I explained the integer rounding error which can happen
> > when calculating the value to write to the register.
>
> I don't have any board to test it and Marek says there is only one that
> doesn't use cmu *and* has an SPI device attached.
>
> Here is what I think should work for the !cmu case.
>
> --8<---------------cut here---------------start------------->8---
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index 18b89e53ceda..5ebb1caade4d 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -655,13 +655,18 @@ static int s3c64xx_spi_config(struct
> s3c64xx_spi_driver_data *sdd)
>                         return ret;
>                 sdd->cur_speed = clk_get_rate(sdd->src_clk) / 2;
>         } else {
> +               int src_clk_rate = clk_get_rate(sdd->src_clk);

The return value of clk_get_rate() is unsigned long.

> +               int clk_val = (src_clk_rate / sdd->cur_speed / 2 - 1);

Perhaps u32, since this is a value to be written to a 32-bit register.
Also if you could add a comment explaining that a negative overflow is
impossible:

/* s3c64xx_spi_setup() ensures that sdd->cur_speed <= src_clk_rate / 2. */

But actually, unless my lack of sleep is badly affecting my brain
processes, the original computation was completely wrong. Let's
consider the scenario below:

src_clk_rate = 8000000
sdd->cur_speed = 2500000

clk_val = 8000000 / 2500000 / 2 - 1 = 3 / 2 - 1 = 1 - 1 = 0
[...]
sdd->cur_speed = 8000000 / (2 * (0 + 1)) = 8000000 / (2 * 1) = 8000000
/ 2 = 4000000

So a request for 2.5 MHz ends up with 4 MHz, which could actually be
above the client device or link spec.

I believe the right thing to do would be DIV_ROUND_UP(src_clk_rate /
2, sdd->cur_speed) - 1. It's safe to divide src_clk_rate directly,
because those are normally high rates divisible by two without much
precision loss.

> +
>                 /* Configure Clock */
>                 val = readl(regs + S3C64XX_SPI_CLK_CFG);
>                 val &= ~S3C64XX_SPI_PSR_MASK;
> -               val |= ((clk_get_rate(sdd->src_clk) / sdd->cur_speed / 2 - 1)
> -                               & S3C64XX_SPI_PSR_MASK);
> +               val |= (clk_val & S3C64XX_SPI_PSR_MASK);
>                 writel(val, regs + S3C64XX_SPI_CLK_CFG);
>
> +               /* Keep the actual value */
> +               sdd->cur_speed = src_clk_rate / (2 * (clk_val + 1));

Also need to consider S3C64XX_SPI_PSR_MASK here, because clk_val could
actually be > S3C64XX_SPI_PSR_MASK.

Best regards,
Tomasz

> +
>                 /* Enable Clock */
>                 val = readl(regs + S3C64XX_SPI_CLK_CFG);
>                 val |= S3C64XX_SPI_ENCLK_ENABLE;
> --8<---------------cut here---------------end--------------->8---
>
>
> >> > You are also affecting here not only timeout but
> >> > s3c64xx_enable_datapath() which is not mentioned in commit log. In other
> >> > words, this looks wrong.
> >>
> >> Indeed, there is a reference too. I've corrected the message.
> >>
> >
> > Thanks!
> >
> > Best regards,
> > Tomasz
> >
> >> >>
> >> >> Cc: Tomasz Figa <tfiga@xxxxxxxxxxxx>
> >> >> Signed-off-by: Łukasz Stelmach <l.stelmach@xxxxxxxxxxx>
> >> >> ---
> >> >>  drivers/spi/spi-s3c64xx.c | 1 +
> >> >>  1 file changed, 1 insertion(+)
> >> >>
> >> >> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> >> >> index 02de734b8ab1..89c162efe355 100644
> >> >> --- a/drivers/spi/spi-s3c64xx.c
> >> >> +++ b/drivers/spi/spi-s3c64xx.c
> >> >> @@ -626,6 +626,7 @@ static int s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd)
> >> >>              ret = clk_set_rate(sdd->src_clk, sdd->cur_speed * 2);
> >> >>              if (ret)
> >> >>                      return ret;
> >> >> +            sdd->cur_speed = clk_get_rate(sdd->src_clk) / 2;
> >> >>      } else {
> >> >>              /* Configure Clock */
> >> >>              val = readl(regs + S3C64XX_SPI_CLK_CFG);
> >> >> --
> >> >> 2.26.2
> >> >>
> >> >
> >> >
> >>
> >> --
> >> Łukasz Stelmach
> >> Samsung R&D Institute Poland
> >> Samsung Electronics
> >
> >
>
> --
> Łukasz Stelmach
> Samsung R&D Institute Poland
> Samsung Electronics