| Message ID | 20200821161401.11307-8-l.stelmach@samsung.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Some fixes for spi-s3c64xx | expand |
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(). Otherwise why only if (cmu) case is updated? 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. Best regards, Krzysztof > > 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 >
On Sat, Aug 22, 2020 at 2:43 PM Krzysztof Kozlowski <krzk@kernel.org> 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(). Division is not the point of the patch. The point is that clk_set_rate() that was originally there doesn't guarantee that the rate is set exactly. The rate directly determines the SPI transfer speed and thus the driver needs to use the rate that was actually set for further calculations. > Otherwise why only if (cmu) case is updated? Right, the !cmu case actually suffers from the same problem. The code divides the parent clock rate with the requested speed to obtain the divider to program into the register. This is subject to integer rounding, so (parent / (parent / speed)) doesn't always equal (speed). > > 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. Actually this is right and fixes one more problem, which I didn't spot when looking at this code when I suggested the change (I only spotted the effects on timeout calculation). The rounding error might have caused wrong configuration there, because e.g. 30000000 Hz could be requested and rounded to 28000000 Hz. The former is a threshold for setting the S3C64XX_SPI_CH_HS_EN bit, but the real frequency wouldn't actually require setting it. Best regards, Tomasz > > Best regards, > Krzysztof > > > > > 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 > >
On Fri, Aug 21, 2020 at 6:14 PM Łukasz Stelmach <l.stelmach@samsung.com> 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. > > Cc: Tomasz Figa <tfiga@chromium.org> As we talked on IRC, Lukasz forgot to add: Suggested-by: Tomasz Figa <tomasz.figa@gmail.com> Would be nice if it could be added when (and if) applying. > 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 >
On Sat, Aug 22, 2020 at 04:52:40PM +0200, Tomasz Figa wrote: > On Sat, Aug 22, 2020 at 2:43 PM Krzysztof Kozlowski <krzk@kernel.org> 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(). > > Division is not the point of the patch. The point is that > clk_set_rate() that was originally there doesn't guarantee that the > rate is set exactly. Unfortunately onlt that point of timeout is mentioned in commit msg. If the correction of timeout was not the point of the patch, then describe the real point... > The rate directly determines the SPI transfer > speed and thus the driver needs to use the rate that was actually set > for further calculations. Yep, makes sense. > > > Otherwise why only if (cmu) case is updated? > > Right, the !cmu case actually suffers from the same problem. The code > divides the parent clock rate with the requested speed to obtain the > divider to program into the register. This is subject to integer > rounding, so (parent / (parent / speed)) doesn't always equal (speed). It is not only this problem. The meaning of cur_speed is now changed. For !cmu_case this the requested in trasnfer SPI bus clock frequency, for cmu_case this is now real src_clk frequency. It's getting slightly messier. > > > > > 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. > > Actually this is right and fixes one more problem, which I didn't spot > when looking at this code when I suggested the change (I only spotted > the effects on timeout calculation). The rounding error might have > caused wrong configuration there, because e.g. 30000000 Hz could be > requested and rounded to 28000000 Hz. The former is a threshold for > setting the S3C64XX_SPI_CH_HS_EN bit, but the real frequency wouldn't > actually require setting it. Wrong is here describing one thing and doing much more. Best regards, Krzysztof
On Sat, Aug 22, 2020 at 5:18 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On Sat, Aug 22, 2020 at 04:52:40PM +0200, Tomasz Figa wrote: > > On Sat, Aug 22, 2020 at 2:43 PM Krzysztof Kozlowski <krzk@kernel.org> 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(). > > > > Division is not the point of the patch. The point is that > > clk_set_rate() that was originally there doesn't guarantee that the > > rate is set exactly. > > Unfortunately onlt that point of timeout is mentioned in commit msg. If > the correction of timeout was not the point of the patch, then describe > the real point... > > > The rate directly determines the SPI transfer > > speed and thus the driver needs to use the rate that was actually set > > for further calculations. > > Yep, makes sense. > > > > > > Otherwise why only if (cmu) case is updated? > > > > Right, the !cmu case actually suffers from the same problem. The code > > divides the parent clock rate with the requested speed to obtain the > > divider to program into the register. This is subject to integer > > rounding, so (parent / (parent / speed)) doesn't always equal (speed). > > It is not only this problem. The meaning of cur_speed is now changed. > For !cmu_case this the requested in trasnfer SPI bus clock frequency, > for cmu_case this is now real src_clk frequency. It's getting slightly > messier. > > > > > > > > > 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. > > > > Actually this is right and fixes one more problem, which I didn't spot > > when looking at this code when I suggested the change (I only spotted > > the effects on timeout calculation). The rounding error might have > > caused wrong configuration there, because e.g. 30000000 Hz could be > > requested and rounded to 28000000 Hz. The former is a threshold for > > setting the S3C64XX_SPI_CH_HS_EN bit, but the real frequency wouldn't > > actually require setting it. > > Wrong is here describing one thing and doing much more. Yeah, agreed that the description could be improved. :) Best regards, Tomasz
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? > 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. >> >> 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 >> > >
On Mon, Aug 24, 2020 at 3:17 PM Lukasz Stelmach <l.stelmach@samsung.com> 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. > > 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@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
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@samsung.com> 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); + int clk_val = (src_clk_rate / sdd->cur_speed / 2 - 1); + /* 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)); + /* 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@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 > >
On Tue, Aug 25, 2020 at 11:02 AM Lukasz Stelmach <l.stelmach@samsung.com> 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@samsung.com> 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@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
It was <2020-08-25 wto 17:11>, when Tomasz Figa wrote: > On Tue, Aug 25, 2020 at 11:02 AM Lukasz Stelmach <l.stelmach@samsung.com> 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@samsung.com> 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. */ 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. >> + >> /* 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 >> + >> /* 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@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 >
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);
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. 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(+)