Re: [U-Boot] [PATCH] arm: timer: sunxi: fix a64 spurious timeout issues
On Fri, Mar 22, 2019 at 1:55 AM Oskari Lemmelä wrote:
>
> On 3/17/19 11:14 PM, André Przywara wrote:
> > On 17/03/2019 18:41, Oskari Lemmelä wrote:
> >> On 3/17/19 6:04 PM, Peter Robinson wrote:
> >>> On Sun, Mar 17, 2019 at 2:56 PM Oskari Lemmela
> >>> wrote:
> Fixes spurious timeouts which have been seen during testing
> SPI_SUNXI driver. The false timeouts disappear when number of
> bits reduced to 10 in workaround.
> > So in general I am fine with this patch, if it fixes things for you, but
> > still scratching my head about this.
> > AFAIK Samuel has never seen less than 11 identical bits in his testing,
> > and I am using the new SPI driver for some weeks now (without the patch)
> > and never had any issues. I am on the Pine64 LTS (with that "R18" SoC).
> >
> > So Oskari, can you share how exactly you triggered this problem?
> > I'd rather know what's going on before papering over the issue,
> > especially if that leaves the much more important Linux kernel still at
> > risk.
>
> Actually now when I'm trying to retrigger it seems to be hard to catch.
> I was running following loop couple of days without any issues
>
> setenv read_loop 'setenv i 0; while sf read ${kernel_addr_r} 0; do
> setexpr i ${i} + 1; echo run ${i}; done'
>
> Then I changed wait loop code in spi driver to original one.
W/o this timer patch change?
>
> diff --git a/drivers/spi/spi-sunxi.c b/drivers/spi/spi-sunxi.c
> index dbfeac77ee..6aaa79ddd9 100644
> --- a/drivers/spi/spi-sunxi.c
> +++ b/drivers/spi/spi-sunxi.c
> @@ -377,14 +377,11 @@ static int sun4i_spi_xfer(struct udevice *dev,
> unsigned int bitlen,
> setbits_le32(SPI_REG(priv, SPI_TCR),
> SPI_BIT(priv, SPI_TCR_XCH));
>
> - /* Wait till RX FIFO to be empty */
> - ret = readl_poll_timeout(SPI_REG(priv, SPI_FSR),
> -rx_fifocnt,
> -(((rx_fifocnt &
> -SPI_BIT(priv,
> SPI_FSR_RF_CNT_MASK)) >>
> - SUN4I_FIFO_STA_RF_CNT_BITS) >= nbytes),
> - SUN4I_SPI_TIMEOUT_US);
> - if (ret < 0) {
> + /* Wait transfer to complete */
> + u32 *tcr = SPI_REG(priv, SPI_TCR);
> + ret = wait_for_bit_le32(tcr, 0x8000,
This look strange, to me since we are relying on poll transfer where
the existing code is checking fifo count which seems valid.
> + false, SUN4I_SPI_TIMEOUT_US, false);
> + if (ret) {
> printf("ERROR: sun4i_spi: Timeout transferring
> data\n");
> sun4i_spi_set_cs(bus, slave_plat->cs, false);
> return ret;
>
> Don't know it that was difference but then only one of my boards trigger
> it two times in row.
>
> => setenv read_loop 'setenv i 0; while sf read ${kernel_addr_r} 0; do
> setexpr i ${i} + 1; echo run ${i}; done'
let me test it.
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] arm: timer: sunxi: fix a64 spurious timeout issues
On Sun, Mar 17, 2019 at 2:56 PM Oskari Lemmela wrote:
>
> Fixes spurious timeouts which have been seen during testing
> SPI_SUNXI driver. The false timeouts disappear when number of
> bits reduced to 10 in workaround.
>
> The false timeouts are caused by timer backward jumps.
Wouldn't it be best to apply the same fix as the kernel uses here [1]
as a more permanent fix?
[1] https://www.spinics.net/lists/arm-kernel/msg699622.html
> Signed-off-by: Oskari Lemmela
> ---
> arch/arm/cpu/armv8/generic_timer.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/cpu/armv8/generic_timer.c
> b/arch/arm/cpu/armv8/generic_timer.c
> index c1706dcec1..2e06ee4ed2 100644
> --- a/arch/arm/cpu/armv8/generic_timer.c
> +++ b/arch/arm/cpu/armv8/generic_timer.c
> @@ -66,7 +66,7 @@ unsigned long timer_read_counter(void)
> isb();
> do {
> asm volatile("mrs %0, cntpct_el0" : "=r" (cntpct));
> - } while (((cntpct + 1) & GENMASK(10, 0)) <= 1);
> + } while (((cntpct + 1) & GENMASK(9, 0)) <= 1);
>
> return cntpct;
> }
> --
> 2.17.1
>
> ___
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] arm: timer: sunxi: fix a64 spurious timeout issues
On 3/17/19 6:04 PM, Peter Robinson wrote:
On Sun, Mar 17, 2019 at 2:56 PM Oskari Lemmela wrote:
Fixes spurious timeouts which have been seen during testing
SPI_SUNXI driver. The false timeouts disappear when number of
bits reduced to 10 in workaround.
The false timeouts are caused by timer backward jumps.
Wouldn't it be best to apply the same fix as the kernel uses here [1]
as a more permanent fix?
[1] https://www.spinics.net/lists/arm-kernel/msg699622.html
Sure. Difference is that retry counter was added to while loop in kernel
side.
Only CNTPCT is used in u-boot.
So CNTVCT part of kernel patch is currently not needed?
Signed-off-by: Oskari Lemmela
---
arch/arm/cpu/armv8/generic_timer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/cpu/armv8/generic_timer.c
b/arch/arm/cpu/armv8/generic_timer.c
index c1706dcec1..2e06ee4ed2 100644
--- a/arch/arm/cpu/armv8/generic_timer.c
+++ b/arch/arm/cpu/armv8/generic_timer.c
@@ -66,7 +66,7 @@ unsigned long timer_read_counter(void)
isb();
do {
asm volatile("mrs %0, cntpct_el0" : "=r" (cntpct));
- } while (((cntpct + 1) & GENMASK(10, 0)) <= 1);
+ } while (((cntpct + 1) & GENMASK(9, 0)) <= 1);
return cntpct;
}
--
2.17.1
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Oskari
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] arm: timer: sunxi: fix a64 spurious timeout issues
On 03/17/19 13:41, Oskari Lemmelä wrote:
> On 3/17/19 6:04 PM, Peter Robinson wrote:
>> On Sun, Mar 17, 2019 at 2:56 PM Oskari Lemmela wrote:
>>> Fixes spurious timeouts which have been seen during testing
>>> SPI_SUNXI driver. The false timeouts disappear when number of
>>> bits reduced to 10 in workaround.
>>>
>>> The false timeouts are caused by timer backward jumps.
>> Wouldn't it be best to apply the same fix as the kernel uses here [1]
>> as a more permanent fix?
>>
>> [1] https://www.spinics.net/lists/arm-kernel/msg699622.html
This _is_ the same fix as the kernel uses.
> Sure. Difference is that retry counter was added to while loop in kernel side.
And the retry counter isn't needed here, since there is no preemption in u-boot.
> Only CNTPCT is used in u-boot.
> So CNTVCT part of kernel patch is currently not needed?
Correct.
>>
>>> Signed-off-by: Oskari Lemmela
>>> ---
>>> arch/arm/cpu/armv8/generic_timer.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/cpu/armv8/generic_timer.c
>>> b/arch/arm/cpu/armv8/generic_timer.c
>>> index c1706dcec1..2e06ee4ed2 100644
>>> --- a/arch/arm/cpu/armv8/generic_timer.c
>>> +++ b/arch/arm/cpu/armv8/generic_timer.c
>>> @@ -66,7 +66,7 @@ unsigned long timer_read_counter(void)
>>> isb();
>>> do {
>>> asm volatile("mrs %0, cntpct_el0" : "=r" (cntpct));
>>> - } while (((cntpct + 1) & GENMASK(10, 0)) <= 1);
>>> + } while (((cntpct + 1) & GENMASK(9, 0)) <= 1);
Please update the 11 to 10 in the comment above as well.
>>> return cntpct;
>>> }
>>> --
>>> 2.17.1
Thanks,
Samuel
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] arm: timer: sunxi: fix a64 spurious timeout issues
On 17/03/2019 18:41, Oskari Lemmelä wrote:
> On 3/17/19 6:04 PM, Peter Robinson wrote:
>> On Sun, Mar 17, 2019 at 2:56 PM Oskari Lemmela
>> wrote:
>>> Fixes spurious timeouts which have been seen during testing
>>> SPI_SUNXI driver. The false timeouts disappear when number of
>>> bits reduced to 10 in workaround.
So in general I am fine with this patch, if it fixes things for you, but
still scratching my head about this.
AFAIK Samuel has never seen less than 11 identical bits in his testing,
and I am using the new SPI driver for some weeks now (without the patch)
and never had any issues. I am on the Pine64 LTS (with that "R18" SoC).
So Oskari, can you share how exactly you triggered this problem?
I'd rather know what's going on before papering over the issue,
especially if that leaves the much more important Linux kernel still at
risk.
Cheers,
Andre.
>>>
>>> The false timeouts are caused by timer backward jumps.
>> Wouldn't it be best to apply the same fix as the kernel uses here [1]
>> as a more permanent fix?
>>
>> [1] https://www.spinics.net/lists/arm-kernel/msg699622.html
>
> Sure. Difference is that retry counter was added to while loop in kernel
> side.
>
> Only CNTPCT is used in u-boot.
> So CNTVCT part of kernel patch is currently not needed?
>
>>
>>> Signed-off-by: Oskari Lemmela
>>> ---
>>> arch/arm/cpu/armv8/generic_timer.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/cpu/armv8/generic_timer.c
>>> b/arch/arm/cpu/armv8/generic_timer.c
>>> index c1706dcec1..2e06ee4ed2 100644
>>> --- a/arch/arm/cpu/armv8/generic_timer.c
>>> +++ b/arch/arm/cpu/armv8/generic_timer.c
>>> @@ -66,7 +66,7 @@ unsigned long timer_read_counter(void)
>>> isb();
>>> do {
>>> asm volatile("mrs %0, cntpct_el0" : "=r" (cntpct));
>>> - } while (((cntpct + 1) & GENMASK(10, 0)) <= 1);
>>> + } while (((cntpct + 1) & GENMASK(9, 0)) <= 1);
>>>
>>> return cntpct;
>>> }
>>> --
>>> 2.17.1
>>>
>>> ___
>>> U-Boot mailing list
>>> U-Boot@lists.denx.de
>>> https://lists.denx.de/listinfo/u-boot
>
> Oskari
>
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] arm: timer: sunxi: fix a64 spurious timeout issues
On 3/17/19 11:14 PM, André Przywara wrote:
On 17/03/2019 18:41, Oskari Lemmelä wrote:
On 3/17/19 6:04 PM, Peter Robinson wrote:
On Sun, Mar 17, 2019 at 2:56 PM Oskari Lemmela
wrote:
Fixes spurious timeouts which have been seen during testing
SPI_SUNXI driver. The false timeouts disappear when number of
bits reduced to 10 in workaround.
So in general I am fine with this patch, if it fixes things for you, but
still scratching my head about this.
AFAIK Samuel has never seen less than 11 identical bits in his testing,
and I am using the new SPI driver for some weeks now (without the patch)
and never had any issues. I am on the Pine64 LTS (with that "R18" SoC).
So Oskari, can you share how exactly you triggered this problem?
I'd rather know what's going on before papering over the issue,
especially if that leaves the much more important Linux kernel still at
risk.
Actually now when I'm trying to retrigger it seems to be hard to catch.
I was running following loop couple of days without any issues
setenv read_loop 'setenv i 0; while sf read ${kernel_addr_r} 0; do
setexpr i ${i} + 1; echo run ${i}; done'
Then I changed wait loop code in spi driver to original one.
diff --git a/drivers/spi/spi-sunxi.c b/drivers/spi/spi-sunxi.c
index dbfeac77ee..6aaa79ddd9 100644
--- a/drivers/spi/spi-sunxi.c
+++ b/drivers/spi/spi-sunxi.c
@@ -377,14 +377,11 @@ static int sun4i_spi_xfer(struct udevice *dev,
unsigned int bitlen,
setbits_le32(SPI_REG(priv, SPI_TCR),
SPI_BIT(priv, SPI_TCR_XCH));
- /* Wait till RX FIFO to be empty */
- ret = readl_poll_timeout(SPI_REG(priv, SPI_FSR),
- rx_fifocnt,
- (((rx_fifocnt &
- SPI_BIT(priv,
SPI_FSR_RF_CNT_MASK)) >>
- SUN4I_FIFO_STA_RF_CNT_BITS) >= nbytes),
- SUN4I_SPI_TIMEOUT_US);
- if (ret < 0) {
+ /* Wait transfer to complete */
+ u32 *tcr = SPI_REG(priv, SPI_TCR);
+ ret = wait_for_bit_le32(tcr, 0x8000,
+ false, SUN4I_SPI_TIMEOUT_US, false);
+ if (ret) {
printf("ERROR: sun4i_spi: Timeout transferring
data\n");
sun4i_spi_set_cs(bus, slave_plat->cs, false);
return ret;
Don't know it that was difference but then only one of my boards trigger
it two times in row.
=> setenv read_loop 'setenv i 0; while sf read ${kernel_addr_r} 0; do
setexpr i ${i} + 1; echo run ${i}; done'
=> sf probe
SF: Detected w25q128 with page size 256 Bytes, erase size 4 KiB, total
16 MiB
=> run read_loop
device 0 whole chip
ERROR: sun4i_spi: Timeout transferring data
SF: 16777216 bytes @ 0x0 Read: ERROR -110
=> run read_loop
device 0 whole chip
ERROR: sun4i_spi: Timeout transferring data
SF: 16777216 bytes @ 0x0 Read: ERROR -110
=>
And after that also couple of times.
I'll leave all those three Pine64-LTS boards to run same code to see if
only one of them
have this issue.
Thanks,
Oskari
Cheers,
Andre.
The false timeouts are caused by timer backward jumps.
Wouldn't it be best to apply the same fix as the kernel uses here [1]
as a more permanent fix?
[1] https://www.spinics.net/lists/arm-kernel/msg699622.html
Sure. Difference is that retry counter was added to while loop in kernel
side.
Only CNTPCT is used in u-boot.
So CNTVCT part of kernel patch is currently not needed?
Signed-off-by: Oskari Lemmela
---
arch/arm/cpu/armv8/generic_timer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/cpu/armv8/generic_timer.c
b/arch/arm/cpu/armv8/generic_timer.c
index c1706dcec1..2e06ee4ed2 100644
--- a/arch/arm/cpu/armv8/generic_timer.c
+++ b/arch/arm/cpu/armv8/generic_timer.c
@@ -66,7 +66,7 @@ unsigned long timer_read_counter(void)
isb();
do {
asm volatile("mrs %0, cntpct_el0" : "=r" (cntpct));
- } while (((cntpct + 1) & GENMASK(10, 0)) <= 1);
+ } while (((cntpct + 1) & GENMASK(9, 0)) <= 1);
return cntpct;
}
--
2.17.1
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Oskari
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
