It was <2020-08-25 wto 17:11>, when Tomasz Figa wrote:

On Tue, Aug 25, 2020 at 11:02 AM Lukasz Stelmach [off-list ref] wrote:

quoted

It was <2020-08-24 pon 15:21>, when Tomasz Figa wrote:

quoted

On Mon, Aug 24, 2020 at 3:17 PM Lukasz Stelmach [off-list ref] wrote:

quoted

It was <2020-08-22 sob 14:43>, when Krzysztof Kozlowski wrote:

quoted

On Fri, Aug 21, 2020 at 06:13:59PM +0200, Łukasz Stelmach wrote:

quoted

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.

quoted

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.

quoted

+               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. */

OK.

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.

This makes sense.

quoted

+
                /* 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.

Good point, but this

    clk_val = clk_val < 127 ? clk_val : 127;

seems more appropriate since masking may give very bogus value. Eg.

    src_clk_rate = 80000000 // 80 MHz
    cur_speed = 300000 // 300 kHz

    clk_val = 80000000/300000/2-1        => 132
    clk_val &= S3C64XX_SPI_PSR_MASK      => 4
    cur_speed = 80000000 / (2 * (4 + 1)) => 8000000 // 8 MHz

quoted

+
                /* Enable Clock */
                val = readl(regs + S3C64XX_SPI_CLK_CFG);
                val |= S3C64XX_SPI_ENCLK_ENABLE;
--8<---------------cut here---------------end--------------->8---

quoted

quoted

quoted

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

quoted

quoted

quoted

Cc: Tomasz Figa <tfiga@chromium.org>
Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
---
 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

-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics