From: Tomasz Figa <tfiga@chromium.org>
To: Lukasz Stelmach <l.stelmach@samsung.com>
Cc: linux-samsung-soc <linux-samsung-soc@vger.kernel.org>,
Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Krzysztof Kozlowski <krzk@kernel.org>,
linux-spi@vger.kernel.org, Mark Brown <broonie@kernel.org>,
Kukjin Kim <kgene@kernel.org>, Andi Shyti <andi@etezian.org>,
"list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>,
Joerg Roedel <joro@8bytes.org>,
" <linux-arm-kernel@lists.infradead.org>,
Marek Szyprowski <m.szyprowski@samsung.com>
Subject: Re: [PATCH v2 7/9] spi: spi-s3c64xx: Ensure cur_speed holds actual clock value
Date: Tue, 25 Aug 2020 17:11:09 +0200 [thread overview]
Message-ID: <CAAFQd5C7Ysb2wnUhUcFZObuSSn4oW=e-oObO5Abat8rJRvqPqw@mail.gmail.com> (raw)
In-Reply-To: <dleftjk0xnw132.fsf%l.stelmach@samsung.com>
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-08-25 15:13 UTC|newest]
Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CGME20200821161404eucas1p20577160d1bff2e8f5cae7403e93716ab@eucas1p2.samsung.com>
2020-08-21 16:13 ` [PATCH v2 0/9] Some fixes for spi-s3c64xx Łukasz Stelmach
[not found] ` <CGME20200821161405eucas1p1d43a5970c6a26389cd506aab5f986bc8@eucas1p1.samsung.com>
2020-08-21 16:13 ` [PATCH v2 1/9] spi: spi-s3c64xx: swap s3c64xx_spi_set_cs() and s3c64xx_enable_datapath() Łukasz Stelmach
2020-08-22 12:01 ` Krzysztof Kozlowski
[not found] ` <CGME20200821161405eucas1p20aad659cd41bc4f56d5123d3c63394f0@eucas1p2.samsung.com>
2020-08-21 16:13 ` [PATCH v2 2/9] spi: spi-s3s64xx: Add S3C64XX_SPI_QUIRK_CS_AUTO for Exynos3250 Łukasz Stelmach
2020-08-22 12:02 ` Krzysztof Kozlowski
[not found] ` <CGME20200821161405eucas1p19280babcd73926b5c22a48830f5fecd7@eucas1p1.samsung.com>
2020-08-21 16:13 ` [PATCH v2 3/9] spi: spi-s3c64xx: Report more information when errors occur Łukasz Stelmach
2020-08-22 12:03 ` Krzysztof Kozlowski
[not found] ` <CGME20200821161406eucas1p2be3221183a855afd0213f8ce58bd8942@eucas1p2.samsung.com>
2020-08-21 16:13 ` [PATCH v2 4/9] spi: spi-s3c64xx: Rename S3C64XX_SPI_SLAVE_* to S3C64XX_SPI_CS_* Łukasz Stelmach
[not found] ` <CGME20200821161406eucas1p121553719d4e9cc020d2c557a69000f0c@eucas1p1.samsung.com>
2020-08-21 16:13 ` [PATCH v2 5/9] spi: spi-s3c64xx: Fix doc comment for struct s3c64xx_spi_driver_data Łukasz Stelmach
2020-08-22 12:26 ` Krzysztof Kozlowski
[not found] ` <CGME20200821161407eucas1p116af63a668bdbb75fa974589e5f6139f@eucas1p1.samsung.com>
2020-08-21 16:13 ` [PATCH v2 6/9] spi: spi-s3c64xx: Check return values Łukasz Stelmach
2020-08-22 12:37 ` Krzysztof Kozlowski
2020-08-25 19:06 ` Sylwester Nawrocki
[not found] ` <CGME20200901152113eucas1p2977046b7a5b4c5a519f88870d658698a@eucas1p2.samsung.com>
2020-09-01 15:21 ` Lukasz Stelmach
2020-09-02 8:14 ` Sylwester Nawrocki
[not found] ` <CGME20200903084555eucas1p2f40375edb325107b68966fd52198b220@eucas1p2.samsung.com>
2020-09-03 8:45 ` Lukasz Stelmach
2020-09-03 11:18 ` Sylwester Nawrocki
[not found] ` <CGME20200821161407eucas1p249e4833b8839f717cc2a496ab43bb8a2@eucas1p2.samsung.com>
2020-08-21 16:13 ` [PATCH v2 7/9] spi: spi-s3c64xx: Ensure cur_speed holds actual clock value Łukasz Stelmach
2020-08-22 12:43 ` Krzysztof Kozlowski
2020-08-22 14:52 ` Tomasz Figa
2020-08-22 15:18 ` Krzysztof Kozlowski
2020-08-22 15:20 ` Tomasz Figa
[not found] ` <CGME20200824131716eucas1p16a3fde52aa765e7cd6584d4733762047@eucas1p1.samsung.com>
2020-08-24 13:17 ` Lukasz Stelmach
2020-08-24 13:21 ` Tomasz Figa
[not found] ` <CGME20200825090211eucas1p1b63191fa778a775e33169ba2c1d3b74b@eucas1p1.samsung.com>
2020-08-25 9:01 ` Lukasz Stelmach
2020-08-25 15:11 ` Tomasz Figa [this message]
[not found] ` <CGME20200825154611eucas1p284be8779ab484e675af071afef28376b@eucas1p2.samsung.com>
2020-08-25 15:45 ` Lukasz Stelmach
2020-08-22 14:54 ` Tomasz Figa
[not found] ` <CGME20200821161407eucas1p23a283ac117d4381e087e9bacec537665@eucas1p2.samsung.com>
2020-08-21 16:14 ` [PATCH v2 8/9] spi: spi-s3c64xx: Increase transfer timeout Łukasz Stelmach
2020-08-22 12:43 ` Krzysztof Kozlowski
[not found] ` <CGME20200821161408eucas1p196aa4b954b3d19ad1b89a48dbbe41fbc@eucas1p1.samsung.com>
2020-08-21 16:14 ` [PATCH v2 9/9] spi: spi-s3c64xx: Turn on interrupts upon resume Łukasz Stelmach
2020-08-22 12:44 ` Krzysztof Kozlowski
[not found] ` <CGME20201001152246eucas1p1ee289b8a85b707f7496355c081623796@eucas1p1.samsung.com>
2020-10-01 15:21 ` [PATCH v2 RESEND 0/9] Some fixes for spi-s3c64xx Łukasz Stelmach
[not found] ` <CGME20201001152246eucas1p1b4155ab4f06a39cc88f205b4a7cd47f9@eucas1p1.samsung.com>
2020-10-01 15:21 ` [PATCH v2 RESEND 1/9] spi: spi-s3c64xx: swap s3c64xx_spi_set_cs() and s3c64xx_enable_datapath() Łukasz Stelmach
[not found] ` <CGME20201001152246eucas1p2fb22ab55c276d76c4508142842c90ab8@eucas1p2.samsung.com>
2020-10-01 15:21 ` [PATCH v2 RESEND 2/9] spi: spi-s3s64xx: Add S3C64XX_SPI_QUIRK_CS_AUTO for Exynos3250 Łukasz Stelmach
2020-10-01 19:04 ` Krzysztof Kozlowski
[not found] ` <CGME20201002101408eucas1p121c21cde5e644992078978d9bf1c5194@eucas1p1.samsung.com>
2020-10-02 10:13 ` Lukasz Stelmach
[not found] ` <CGME20201001152247eucas1p2b6b1cc61b9b175b0a621609cd58effbd@eucas1p2.samsung.com>
2020-10-01 15:21 ` [PATCH v2 RESEND 3/9] spi: spi-s3c64xx: Check return values Łukasz Stelmach
[not found] ` <CGME20201001152247eucas1p2afff5b5b73f78d7c5111ac1c800e718a@eucas1p2.samsung.com>
2020-10-01 15:21 ` [PATCH v2 RESEND 4/9] spi: spi-s3c64xx: Report more information when errors occur Łukasz Stelmach
[not found] ` <CGME20201001152248eucas1p10219600aaa0df6e030d2493b2aefbe92@eucas1p1.samsung.com>
2020-10-01 15:21 ` [PATCH v2 RESEND 5/9] spi: spi-s3c64xx: Rename S3C64XX_SPI_SLAVE_* to S3C64XX_SPI_CS_* Łukasz Stelmach
[not found] ` <CGME20201001152248eucas1p12f71c21a5873b6360e4b38efebb50271@eucas1p1.samsung.com>
2020-10-01 15:21 ` [PATCH v2 RESEND 6/9] spi: spi-s3c64xx: Fix doc comment for struct s3c64xx_spi_driver_data Łukasz Stelmach
[not found] ` <CGME20201001152248eucas1p132a63f588f62d902879322ebadd67173@eucas1p1.samsung.com>
2020-10-01 15:21 ` [PATCH v2 RESEND 7/9] spi: spi-s3c64xx: Ensure cur_speed holds actual clock value Łukasz Stelmach
2020-10-01 19:12 ` Krzysztof Kozlowski
[not found] ` <CGME20201001152249eucas1p1c3bbe7b2a677affe4e1a1e4d469f94c8@eucas1p1.samsung.com>
2020-10-01 15:21 ` [PATCH v2 RESEND 8/9] spi: spi-s3c64xx: Increase transfer timeout Łukasz Stelmach
[not found] ` <CGME20201001152249eucas1p165b78adf542a48b38b49cb5e00500e26@eucas1p1.samsung.com>
2020-10-01 15:21 ` [PATCH v2 RESEND 9/9] spi: spi-s3c64xx: Turn on interrupts upon resume Łukasz Stelmach
2020-10-01 16:13 ` [PATCH v2 RESEND 0/9] Some fixes for spi-s3c64xx Mark Brown
[not found] ` <CGME20201001182315eucas1p1b1fc9d5d0ea91db6e56e92d5cf2583e5@eucas1p1.samsung.com>
2020-10-01 18:23 ` Lukasz Stelmach
2020-10-01 19:02 ` Krzysztof Kozlowski
2020-10-01 19:43 ` Mark Brown
2020-10-02 6:18 ` Krzysztof Kozlowski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAAFQd5C7Ysb2wnUhUcFZObuSSn4oW=e-oObO5Abat8rJRvqPqw@mail.gmail.com' \
--to=tfiga@chromium.org \
--cc=andi@etezian.org \
--cc=b.zolnierkie@samsung.com \
--cc=broonie@kernel.org \
--cc=kgene@kernel.org \
--cc=krzk@kernel.org \
--cc=l.stelmach@samsung.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-samsung-soc@vger.kernel.org \
--cc=linux-spi@vger.kernel.org \
--cc=m.szyprowski@samsung.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).