Hi,
i have been experiencing issues writing to NOR-Flash SPI Memories
from two RK3399-based platforms: gru-kevin and sapphire board.
For kevin, this resulted in a bricked device because that memory
is the only boot device.
Fortunately an external programmer is available.
In order to isolate where the issue can be, several tests have been
done, after which it makes me think the issue is related to the
Rockchip SPI driver.
4KB burst reads work fine.
The issue is only observed on the write burst length.
Test user user/kernel kernel RK3399
Num space interface space SoC Status Notes
--------------------------------------------------------
1 flashrom linux_mtd MTD/RKspi RKspi Fail
2 flashrom linux_spi RKspi RKspi Fail
3 flashrom linux_spi spi_gpio GPIO OK
4 custom linux_spi spi_gpio GPIO OK 260-byte burst writes
5 custom linux_spi RKspi RKspi Fail 260-byte burst writes
6 custom linux_spi RKspi RKspi OK 1-byte burst writes
7 custom linux_spi RKspi RKspi OK 47-byte burst writes
8 custom linux_spi RKspi RKspi Fail 48-byte burst writes
3, 4) Unaccetably slow, device tree is
spi_gpio {
compatible = "spi-gpio";
#address-cells = <1>;
#size-cells = <0>;
cs-gpios = <&gpio1 RK_PB2 GPIO_ACTIVE_HIGH>;
sck-gpios = <&gpio1 RK_PB1 GPIO_ACTIVE_HIGH>;
mosi-gpios = <&gpio1 RK_PB0 GPIO_ACTIVE_HIGH>;
miso-gpios = <&gpio1 RK_PA7 GPIO_ACTIVE_HIGH>;
num-chipselects = <1>;
spidev@0 {
compatible = "spidev";
reg = <0>;
spi-max-frequency = <50000000>;
};
};
2, 5, 6, 7, 8) device tree is
&spi1 {
status = "okay";
spidev@0 {
compatible = "spidev";
reg = <0>;
spi-max-frequency = <50000000>;
};
};
5, 6, 7, 8) Burst writes are performed this way
enum { BURST = 48 };
struct spi_ioc_transfer msg[0x105];
unsigned i = 0;
for (unsigned j = 0; j < wcnt; j += BURST) {
msg[i++] = { .tx_buf = warr, .len = BURST<wcnt-j ? BURST : wcnt-j };
warr += BURST;
}
if (rcnt)
msg[i++] = { .rx_buf = rarr, .len = rcnt };
ioctl(fd, SPI_IOC_MESSAGE(i), msg);
1, 2, 5, 8) I have no logic analyzer to see what is happenning on the
SPI bus, but when it fails at 48-byte bursts, the contents of the memory
are like this:
Addr Expected Actual
000 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
FF FF FF FF FF FF FF FF FF
010 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
FF FF FF FF FF FF FF FF FF
020 FF FF FF FF FF FF FF FF FF FF FF FF F1 F2 F3 F4 FF FF FF FF FF FF FF
FF FF F1 F2 F3 F4 FF FF FF
030 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
FF FF FF FF FF FF FF FF FF
040 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
FF FF FF FF FF FF FF FF FF
050 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
FF FF FF FF FF FF FF FF FF
060 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
FF FF FF FF FF FF FF FF FF
070 FF FF FF FF FF FF FF FF FF FF FF FF E1 E2 E3 E4 FF FF FF FF FF FF E1
E2 E3 E4 FF FF FF FF FF FF
080 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
FF FF FF FF FF FF FF FF FF
090 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
FF FF FF FF FF FF FF FF FF
0A0 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
FF FF FF FF FF FF FF FF FF
0B0 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
FF FF FF FF FF FF FF FF FF
0C0 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
FF FF FF FF FF FF FF FF FF
0D0 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
FF FF FF FF FF FF FF FF FF
0E0 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
FF FF FF FF FF FF FF FF FF
0F0 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
FF FF FF FF FF FF FF FF FF
780 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00
790 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00
7A0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00
7B0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00
7C0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00
7D0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00
7E0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00
7F0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 FF FF FF FF FF FF
FF FF FF FF FF FF FF FF FF
800 01 02 03 04 00 00 00 00 00 00 00 00 00 00 00 00 01 02 03 04 00 00 00
00 00 00 00 00 00 00 00 00
810 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00
820 00 00 00 00 00 00 00 00 00 00 00 00 05 06 07 08 00 00 00 00 00 00 00
00 00 05 06 07 08 00 00 00
830 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00
840 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00
850 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00
860 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00
870 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00
Regards,
Vicenç.
Hi again,
just found the first offending commit:
commit eff0275e5253604429aedc42b008c5fcaa6cc597
Date: Wed Oct 31 11:57:06 2018 +0100
spi: rockchip: simplify use_dma logic
We only need to know if we're using dma when setting
up the transfer, so just use a local variable for
that.
It was mainlined in Linux v5.0.
This is the result of a binary search among this list of commits:
04290192f7ebe892828f69ac57c4684e25da378d
65498c6ae2414a1425aa6c4231e79e2998afec05
01b59ce5dac856323a0c13c1d51d99a819f32efe
74b7efa82b11914c21e30d987ed61d3daa57ff21
420b82f842941a32adf309ca1b193adfc77616b0
eff0275e5253604429aedc42b008c5fcaa6cc597
d790c342e689ea77a5cf72d5b993299911ee5276
eee06a9ee2cd5deaddc5f77ce8f6118c8b82b2a0
fc1ad8ee33480bdf0493b54907b74538bf9b75b8
ce386100d99976442093ff57b5b24a9562c6cc27
fab3e4871f623c8f86e8a0e00749f1480ffa08db
2410d6a3c3070e205169a1a741aa78898e30a642
31bcb57be12fd815a9051f07d64334809b8cb472
30688e4e670d21126aa596df4523940e2f8d24de
a3c174021ce780f5d2e9b2105e2cb4903a37226d
d9071b7e9fc474e474e3b865428a8d30d88daaf4
f340b920511a666b02d371e88801d3817ea7a880
058f7c509e84abd36f988d4e16432366bd793d8f
dcfc861d24ec19f0d0d3d55bb016646794571fbb
dd8fd2cbc73f8650f651da71fc61a6e4f30c1566
Regards,
Vicenç.
On Saturday, April 6, 2019 7:34:32 PM CEST, Vicente Bergas wrote:
> Hi,
> i have been experiencing issues writing to NOR-Flash SPI Memories
> from two RK3399-based platforms: gru-kevin and sapphire board.
> For kevin, this resulted in a bricked device because that memory
> is the only boot device.
> Fortunately an external programmer is available.
>
> In order to isolate where the issue can be, several tests have been
> done, after which it makes me think the issue is related to the
> Rockchip SPI driver.
>
> 4KB burst reads work fine.
> The issue is only observed on the write burst length.
>
> Test user user/kernel kernel RK3399 Num space
> interface space SoC Status Notes
> --------------------------------------------------------
> 1 flashrom linux_mtd MTD/RKspi RKspi Fail
> 2 flashrom linux_spi RKspi RKspi Fail
> 3 flashrom linux_spi spi_gpio GPIO OK
> 4 custom linux_spi spi_gpio GPIO OK 260-byte burst writes
> 5 custom linux_spi RKspi RKspi Fail 260-byte burst writes
> 6 custom linux_spi RKspi RKspi OK 1-byte burst writes
> 7 custom linux_spi RKspi RKspi OK 47-byte burst writes
> 8 custom linux_spi RKspi RKspi Fail 48-byte burst writes
>
> 3, 4) Unaccetably slow, device tree is
> spi_gpio {
> compatible = "spi-gpio";
> #address-cells = <1>;
> #size-cells = <0>;
> cs-gpios = <&gpio1 RK_PB2 GPIO_ACTIVE_HIGH>;
> sck-gpios = <&gpio1 RK_PB1 GPIO_ACTIVE_HIGH>;
> mosi-gpios = <&gpio1 RK_PB0 GPIO_ACTIVE_HIGH>;
> miso-gpios = <&gpio1 RK_PA7 GPIO_ACTIVE_HIGH>;
> num-chipselects = <1>;
> spidev@0 {
> compatible = "spidev";
> reg = <0>;
> spi-max-frequency = <50000000>;
> };
> };
> 2, 5, 6, 7, 8) device tree is
> &spi1 {
> status = "okay";
> spidev@0 {
> compatible = "spidev";
> reg = <0>;
> spi-max-frequency = <50000000>;
> };
> };
> 5, 6, 7, 8) Burst writes are performed this way
> enum { BURST = 48 };
> struct spi_ioc_transfer msg[0x105];
> unsigned i = 0;
> for (unsigned j = 0; j < wcnt; j += BURST) {
> msg[i++] = { .tx_buf = warr, .len = BURST<wcnt-j ? BURST : wcnt-j };
> warr += BURST;
> }
> if (rcnt)
> msg[i++] = { .rx_buf = rarr, .len = rcnt };
> ioctl(fd, SPI_IOC_MESSAGE(i), msg);
> 1, 2, 5, 8) I have no logic analyzer to see what is happenning on the
> SPI bus, but when it fails at 48-byte bursts, the contents of the memory
> are like this:
> Addr Expected Actual
> 000 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
> FF FF FF FF FF FF FF FF FF FF FF FF FF
> 010 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
> FF FF FF FF FF FF FF FF FF FF FF FF FF
> 020 FF FF FF FF FF FF FF FF FF FF FF FF F1 F2 F3 F4 FF FF FF
> FF FF FF FF FF FF F1 F2 F3 F4 FF FF FF
> 030 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
> FF FF FF FF FF FF FF FF FF FF FF FF FF
> 040 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
> FF FF FF FF FF FF FF FF FF FF FF FF FF
> 050 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
> FF FF FF FF FF FF FF FF FF FF FF FF FF
> 060 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
> FF FF FF FF FF FF FF FF FF FF FF FF FF
> 070 FF FF FF FF FF FF FF FF FF FF FF FF E1 E2 E3 E4 FF FF FF
> FF FF FF E1 E2 E3 E4 FF FF FF FF FF FF
> 080 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
> FF FF FF FF FF FF FF FF FF FF FF FF FF
> 090 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
> FF FF FF FF FF FF FF FF FF FF FF FF FF
> 0A0 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
> FF FF FF FF FF FF FF FF FF FF FF FF FF
> 0B0 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
> FF FF FF FF FF FF FF FF FF FF FF FF FF
> 0C0 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
> FF FF FF FF FF FF FF FF FF FF FF FF FF
> 0D0 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
> FF FF FF FF FF FF FF FF FF FF FF FF FF
> 0E0 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
> FF FF FF FF FF FF FF FF FF FF FF FF FF
> 0F0 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
> FF FF FF FF FF FF FF FF FF FF FF FF FF
> 780 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00
> 790 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00
> 7A0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00
> 7B0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00
> 7C0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00
> 7D0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00
> 7E0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00
> 7F0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 FF FF
> FF FF FF FF FF FF FF FF FF FF FF FF FF
> 800 01 02 03 04 00 00 00 00 00 00 00 00 00 00 00 00 01 02 03
> 04 00 00 00 00 00 00 00 00 00 00 00 00
> 810 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00
> 820 00 00 00 00 00 00 00 00 00 00 00 00 05 06 07 08 00 00 00
> 00 00 00 00 00 00 05 06 07 08 00 00 00
> 830 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00
> 840 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00
> 850 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00
> 860 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00
> 870 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00
>
> Regards,
> Vicenç.
>
>
>
Hi,
please, forget my previous mail about
eff0275e5253604429aedc42b008c5fcaa6cc597
spi: rockchip: simplify use_dma logic
i messed up with commit IDs, sorry.
The first offending commit is one later than that:
commit 420b82f842941a32adf309ca1b193adfc77616b0
Date: Wed Oct 31 11:57:07 2018 +0100
spi: rockchip: set min/max speed
The driver previously checked each transfer if the
requested speed was higher than possible with the
current spi clock rate and raised the clock rate
accordingly.
However, there is no check to see if the spi clock
was actually set that high and no way to dynamically
lower the spi clock rate again.
So it seems any potiential users of this functionality
are better off just setting the spi clock rate at init
using the assigned-clock-rates devicetree property.
Removing this dynamic spi clock rate raising allows
us let the spi framework handle min/max speeds
for us.
Regards,
Vicenç.
On Saturday, April 6, 2019 7:34:32 PM CEST, Vicente Bergas wrote:
> Hi,
> i have been experiencing issues writing to NOR-Flash SPI Memories
> from two RK3399-based platforms: gru-kevin and sapphire board.
> For kevin, this resulted in a bricked device because that memory
> is the only boot device.
> Fortunately an external programmer is available.
>
> In order to isolate where the issue can be, several tests have been
> done, after which it makes me think the issue is related to the
> Rockchip SPI driver.
>
> 4KB burst reads work fine.
> The issue is only observed on the write burst length.
>
> Test user user/kernel kernel RK3399 Num space
> interface space SoC Status Notes
> --------------------------------------------------------
> 1 flashrom linux_mtd MTD/RKspi RKspi Fail
> 2 flashrom linux_spi RKspi RKspi Fail
> 3 flashrom linux_spi spi_gpio GPIO OK
> 4 custom linux_spi spi_gpio GPIO OK 260-byte burst writes
> 5 custom linux_spi RKspi RKspi Fail 260-byte burst writes
> 6 custom linux_spi RKspi RKspi OK 1-byte burst writes
> 7 custom linux_spi RKspi RKspi OK 47-byte burst writes
> 8 custom linux_spi RKspi RKspi Fail 48-byte burst writes
>
> 3, 4) Unaccetably slow, device tree is
> spi_gpio {
> compatible = "spi-gpio";
> #address-cells = <1>;
> #size-cells = <0>;
> cs-gpios = <&gpio1 RK_PB2 GPIO_ACTIVE_HIGH>;
> sck-gpios = <&gpio1 RK_PB1 GPIO_ACTIVE_HIGH>;
> mosi-gpios = <&gpio1 RK_PB0 GPIO_ACTIVE_HIGH>;
> miso-gpios = <&gpio1 RK_PA7 GPIO_ACTIVE_HIGH>;
> num-chipselects = <1>;
> spidev@0 {
> compatible = "spidev";
> reg = <0>;
> spi-max-frequency = <50000000>;
> };
> };
> 2, 5, 6, 7, 8) device tree is
> &spi1 {
> status = "okay";
> spidev@0 {
> compatible = "spidev";
> reg = <0>;
> spi-max-frequency = <50000000>;
> };
> };
> 5, 6, 7, 8) Burst writes are performed this way
> enum { BURST = 48 };
> struct spi_ioc_transfer msg[0x105];
> unsigned i = 0;
> for (unsigned j = 0; j < wcnt; j += BURST) {
> msg[i++] = { .tx_buf = warr, .len = BURST<wcnt-j ? BURST : wcnt-j };
> warr += BURST;
> }
> if (rcnt)
> msg[i++] = { .rx_buf = rarr, .len = rcnt };
> ioctl(fd, SPI_IOC_MESSAGE(i), msg);
> 1, 2, 5, 8) I have no logic analyzer to see what is happenning on the
> SPI bus, but when it fails at 48-byte bursts, the contents of the memory
> are like this:
> Addr Expected Actual
> 000 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
> FF FF FF FF FF FF FF FF FF FF FF FF FF
> 010 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
> FF FF FF FF FF FF FF FF FF FF FF FF FF
> 020 FF FF FF FF FF FF FF FF FF FF FF FF F1 F2 F3 F4 FF FF FF
> FF FF FF FF FF FF F1 F2 F3 F4 FF FF FF
> 030 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
> FF FF FF FF FF FF FF FF FF FF FF FF FF
> 040 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
> FF FF FF FF FF FF FF FF FF FF FF FF FF
> 050 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
> FF FF FF FF FF FF FF FF FF FF FF FF FF
> 060 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
> FF FF FF FF FF FF FF FF FF FF FF FF FF
> 070 FF FF FF FF FF FF FF FF FF FF FF FF E1 E2 E3 E4 FF FF FF
> FF FF FF E1 E2 E3 E4 FF FF FF FF FF FF
> 080 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
> FF FF FF FF FF FF FF FF FF FF FF FF FF
> 090 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
> FF FF FF FF FF FF FF FF FF FF FF FF FF
> 0A0 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
> FF FF FF FF FF FF FF FF FF FF FF FF FF
> 0B0 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
> FF FF FF FF FF FF FF FF FF FF FF FF FF
> 0C0 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
> FF FF FF FF FF FF FF FF FF FF FF FF FF
> 0D0 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
> FF FF FF FF FF FF FF FF FF FF FF FF FF
> 0E0 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
> FF FF FF FF FF FF FF FF FF FF FF FF FF
> 0F0 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
> FF FF FF FF FF FF FF FF FF FF FF FF FF
> 780 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00
> 790 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00
> 7A0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00
> 7B0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00
> 7C0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00
> 7D0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00
> 7E0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00
> 7F0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 FF FF
> FF FF FF FF FF FF FF FF FF FF FF FF FF
> 800 01 02 03 04 00 00 00 00 00 00 00 00 00 00 00 00 01 02 03
> 04 00 00 00 00 00 00 00 00 00 00 00 00
> 810 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00
> 820 00 00 00 00 00 00 00 00 00 00 00 00 05 06 07 08 00 00 00
> 00 00 00 00 00 00 05 06 07 08 00 00 00
> 830 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00
> 840 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00
> 850 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00
> 860 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00
> 870 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00
>
> Regards,
> Vicenç.
>
>
>
This reverts
commit 420b82f84294 ("spi: rockchip: set min/max speed")
commit 74b7efa82b11 ("spi: rockchip: precompute rx sample delay")
The former breaks bursts of writes of 48 bytes or more.
Both patches touch the same part of the file and it is not trivial to
only revert the first. Reverting both just results in an easy to fix
conflict.
Tested-by: Vicente Bergas <[email protected]>
Signed-off-by: Vicente Bergas <[email protected]>
---
drivers/spi/spi-rockchip.c | 80 ++++++++++++++++++++++----------------
1 file changed, 46 insertions(+), 34 deletions(-)
diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
index 3912526ead66..682f3567a8c8 100644
--- a/drivers/spi/spi-rockchip.c
+++ b/drivers/spi/spi-rockchip.c
@@ -97,7 +97,6 @@
#define CR0_BHT_8BIT 0x1
#define CR0_RSD_OFFSET 14
-#define CR0_RSD_MAX 0x3
#define CR0_FRF_OFFSET 16
#define CR0_FRF_SPI 0x0
@@ -119,10 +118,6 @@
/* Bit fields in SER, 2bit */
#define SER_MASK 0x3
-/* Bit fields in BAUDR */
-#define BAUDR_SCKDV_MIN 2
-#define BAUDR_SCKDV_MAX 65534
-
/* Bit fields in SR, 5bit */
#define SR_MASK 0x1f
#define SR_BUSY (1 << 0)
@@ -155,7 +150,7 @@
#define TXDMA (1 << 1)
/* sclk_out: spi master internal logic in rk3x can support 50Mhz */
-#define MAX_SCLK_OUT 50000000U
+#define MAX_SCLK_OUT 50000000
/*
* SPI_CTRLR1 is 16-bits, so we should support lengths of 0xffff + 1. However,
@@ -184,11 +179,12 @@ struct rockchip_spi {
/*depth of the FIFO buffer */
u32 fifo_len;
- /* frequency of spiclk */
- u32 freq;
+ /* max bus freq supported */
+ u32 max_freq;
u8 n_bytes;
- u8 rsd;
+ u32 rsd_nsecs;
+ u32 speed;
bool cs_asserted[ROCKCHIP_SPI_MAX_CS_NUM];
};
@@ -198,6 +194,11 @@ static inline void spi_enable_chip(struct rockchip_spi *rs, bool enable)
writel_relaxed((enable ? 1U : 0U), rs->regs + ROCKCHIP_SPI_SSIENR);
}
+static inline void spi_set_clk(struct rockchip_spi *rs, u16 div)
+{
+ writel_relaxed(div, rs->regs + ROCKCHIP_SPI_BAUDR);
+}
+
static inline void wait_for_idle(struct rockchip_spi *rs)
{
unsigned long timeout = jiffies + msecs_to_jiffies(5);
@@ -472,9 +473,10 @@ static void rockchip_spi_config(struct rockchip_spi *rs,
| CR0_SSD_ONE << CR0_SSD_OFFSET
| CR0_EM_BIG << CR0_EM_OFFSET;
u32 cr1;
+ u32 div = 0;
u32 dmacr = 0;
+ int rsd = 0;
- cr0 |= rs->rsd << CR0_RSD_OFFSET;
cr0 |= (spi->mode & 0x3U) << CR0_SCPH_OFFSET;
if (spi->mode & SPI_LSB_FIRST)
cr0 |= CR0_FBM_LSB << CR0_FBM_OFFSET;
@@ -514,6 +516,33 @@ static void rockchip_spi_config(struct rockchip_spi *rs,
dmacr |= RF_DMA_EN;
}
+ if (WARN_ON(rs->speed > MAX_SCLK_OUT))
+ rs->speed = MAX_SCLK_OUT;
+
+ /* the minimum divisor is 2 */
+ if (rs->max_freq < 2 * rs->speed) {
+ clk_set_rate(rs->spiclk, 2 * rs->speed);
+ rs->max_freq = clk_get_rate(rs->spiclk);
+ }
+
+ /* div doesn't support odd number */
+ div = DIV_ROUND_UP(rs->max_freq, rs->speed);
+ div = (div + 1) & 0xfffe;
+
+ /* Rx sample delay is expressed in parent clock cycles (max 3) */
+ rsd = DIV_ROUND_CLOSEST(rs->rsd_nsecs * (rs->max_freq >> 8),
+ 1000000000 >> 8);
+ if (!rsd && rs->rsd_nsecs) {
+ pr_warn_once("rockchip-spi: %u Hz are too slow to express %u ns delay\n",
+ rs->max_freq, rs->rsd_nsecs);
+ } else if (rsd > 3) {
+ rsd = 3;
+ pr_warn_once("rockchip-spi: %u Hz are too fast to express %u ns delay, clamping at %u ns\n",
+ rs->max_freq, rs->rsd_nsecs,
+ rsd * 1000000000U / rs->max_freq);
+ }
+ cr0 |= rsd << CR0_RSD_OFFSET;
+
writel_relaxed(cr0, rs->regs + ROCKCHIP_SPI_CTRLR0);
writel_relaxed(cr1, rs->regs + ROCKCHIP_SPI_CTRLR1);
@@ -530,12 +559,9 @@ static void rockchip_spi_config(struct rockchip_spi *rs,
writel_relaxed(0, rs->regs + ROCKCHIP_SPI_DMARDLR);
writel_relaxed(dmacr, rs->regs + ROCKCHIP_SPI_DMACR);
- /* the hardware only supports an even clock divisor, so
- * round divisor = spiclk / speed up to nearest even number
- * so that the resulting speed is <= the requested speed
- */
- writel_relaxed(2 * DIV_ROUND_UP(rs->freq, 2 * xfer->speed_hz),
- rs->regs + ROCKCHIP_SPI_BAUDR);
+ spi_set_clk(rs, div);
+
+ dev_dbg(rs->dev, "cr0 0x%x, div %d\n", cr0, div);
}
static size_t rockchip_spi_max_transfer_size(struct spi_device *spi)
@@ -564,6 +590,7 @@ static int rockchip_spi_transfer_one(
return -EINVAL;
}
+ rs->speed = xfer->speed_hz;
rs->n_bytes = xfer->bits_per_word <= 8 ? 1 : 2;
use_dma = master->can_dma ? master->can_dma(master, spi, xfer) : false;
@@ -652,24 +679,11 @@ static int rockchip_spi_probe(struct platform_device *pdev)
goto err_disable_spiclk;
rs->dev = &pdev->dev;
- rs->freq = clk_get_rate(rs->spiclk);
+ rs->max_freq = clk_get_rate(rs->spiclk);
if (!of_property_read_u32(pdev->dev.of_node, "rx-sample-delay-ns",
- &rsd_nsecs)) {
- /* rx sample delay is expressed in parent clock cycles (max 3) */
- u32 rsd = DIV_ROUND_CLOSEST(rsd_nsecs * (rs->freq >> 8),
- 1000000000 >> 8);
- if (!rsd) {
- dev_warn(rs->dev, "%u Hz are too slow to express %u ns delay\n",
- rs->freq, rsd_nsecs);
- } else if (rsd > CR0_RSD_MAX) {
- rsd = CR0_RSD_MAX;
- dev_warn(rs->dev, "%u Hz are too fast to express %u ns delay, clamping at %u ns\n",
- rs->freq, rsd_nsecs,
- CR0_RSD_MAX * 1000000000U / rs->freq);
- }
- rs->rsd = rsd;
- }
+ &rsd_nsecs))
+ rs->rsd_nsecs = rsd_nsecs;
rs->fifo_len = get_fifo_len(rs);
if (!rs->fifo_len) {
@@ -687,8 +701,6 @@ static int rockchip_spi_probe(struct platform_device *pdev)
master->num_chipselect = ROCKCHIP_SPI_MAX_CS_NUM;
master->dev.of_node = pdev->dev.of_node;
master->bits_per_word_mask = SPI_BPW_MASK(16) | SPI_BPW_MASK(8) | SPI_BPW_MASK(4);
- master->min_speed_hz = rs->freq / BAUDR_SCKDV_MAX;
- master->max_speed_hz = min(rs->freq / BAUDR_SCKDV_MIN, MAX_SCLK_OUT);
master->set_cs = rockchip_spi_set_cs;
master->transfer_one = rockchip_spi_transfer_one;
--
2.21.0
Hi Vicente,
On Sat, 6 Apr 2019 at 19:35, Vicente Bergas <[email protected]> wrote:
>
> Hi,
> i have been experiencing issues writing to NOR-Flash SPI Memories
> from two RK3399-based platforms: gru-kevin and sapphire board.
> For kevin, this resulted in a bricked device because that memory
> is the only boot device.
> Fortunately an external programmer is available.
>
> In order to isolate where the issue can be, several tests have been
> done, after which it makes me think the issue is related to the
> Rockchip SPI driver.
>
> 4KB burst reads work fine.
> The issue is only observed on the write burst length.
>
> Test user user/kernel kernel RK3399
> Num space interface space SoC Status Notes
> --------------------------------------------------------
> 1 flashrom linux_mtd MTD/RKspi RKspi Fail
> 2 flashrom linux_spi RKspi RKspi Fail
> 3 flashrom linux_spi spi_gpio GPIO OK
> 4 custom linux_spi spi_gpio GPIO OK 260-byte burst writes
> 5 custom linux_spi RKspi RKspi Fail 260-byte burst writes
> 6 custom linux_spi RKspi RKspi OK 1-byte burst writes
> 7 custom linux_spi RKspi RKspi OK 47-byte burst writes
> 8 custom linux_spi RKspi RKspi Fail 48-byte burst writes
>
> 3, 4) Unaccetably slow, device tree is
> spi_gpio {
> compatible = "spi-gpio";
> #address-cells = <1>;
> #size-cells = <0>;
> cs-gpios = <&gpio1 RK_PB2 GPIO_ACTIVE_HIGH>;
> sck-gpios = <&gpio1 RK_PB1 GPIO_ACTIVE_HIGH>;
> mosi-gpios = <&gpio1 RK_PB0 GPIO_ACTIVE_HIGH>;
> miso-gpios = <&gpio1 RK_PA7 GPIO_ACTIVE_HIGH>;
> num-chipselects = <1>;
> spidev@0 {
> compatible = "spidev";
> reg = <0>;
> spi-max-frequency = <50000000>;
> };
> };
> 2, 5, 6, 7, 8) device tree is
> &spi1 {
Since you say reverting the "set min/max speed" patch fixes your issues
could you try raising the spi clock like this and see if it works for you?
+ assigned-clocks = <&cru SCLK_SPI1>;
+ assigned-clock-rates = <400000000>;
Of course the driver shouldn't let you configure the spi-controller in a way
that makes it skip bytes, but if this works for you then I still think
you're better off explicitly setting the spi clock speed rather than having
the driver raise it for you. At least while it does it without
checking for errors
or having a way to lower it again as outlined in the commit message.
> status = "okay";
> spidev@0 {
> compatible = "spidev";
> reg = <0>;
> spi-max-frequency = <50000000>;
> };
> };
> ...
/Emil
On Sunday, April 7, 2019 9:55:10 PM CEST, Emil Renner Berthing wrote:
> Hi Vicente,
>
> On Sat, 6 Apr 2019 at 19:35, Vicente Bergas <[email protected]> wrote:
>> Hi,
>> i have been experiencing issues writing to NOR-Flash SPI Memories
>> from two RK3399-based platforms: gru-kevin and sapphire board.
>> For kevin, this resulted in a bricked device because that memory
>> is the only boot device. ...
>
> Since you say reverting the "set min/max speed" patch fixes your issues
> could you try raising the spi clock like this and see if it works for you?
>
> + assigned-clocks = <&cru SCLK_SPI1>;
> + assigned-clock-rates = <400000000>;
>
> Of course the driver shouldn't let you configure the spi-controller in a way
> that makes it skip bytes, but if this works for you then I still think
> you're better off explicitly setting the spi clock speed rather than having
> the driver raise it for you. At least while it does it without
> checking for errors
> or having a way to lower it again as outlined in the commit message.
>
>> status = "okay";
>> spidev@0 {
>> compatible = "spidev";
>> reg = <0>;
>> spi-max-frequency = <50000000>;
>> };
>> };
>> ...
>
> /Emil
>
Hi Emil,
I've added both assigned-clocks properties to the spi1 node and tested
again. Unfortunately it still fails in a similar way:
Before the maximum write burst without errors was 47, now it is 33.
I've also tested it with an SPI bus speed of 100KHz to make sure the
external hardware is not overrun:
ioctl SPI_IOC_WR_MAX_SPEED_HZ 100000
Regards,
Vicenç.
On Sunday, April 7, 2019 2:57:59 AM CEST, Vicente Bergas wrote:
> This reverts
> commit 420b82f84294 ("spi: rockchip: set min/max speed")
> commit 74b7efa82b11 ("spi: rockchip: precompute rx sample delay")
> The former breaks bursts of writes of 48 bytes or more.
> Both patches touch the same part of the file and it is not trivial to
> only revert the first. Reverting both just results in an easy to fix
> conflict.
>
> Tested-by: Vicente Bergas <[email protected]>
> Signed-off-by: Vicente Bergas <[email protected]>
> ---
> drivers/spi/spi-rockchip.c | 80 ++++++++++++++++++++++----------------
> 1 file changed, 46 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
> index 3912526ead66..682f3567a8c8 100644
> --- a/drivers/spi/spi-rockchip.c
> +++ b/drivers/spi/spi-rockchip.c
> ...
Hi Mark,
please, disregard this patch. It is the result of bisecting a bug which
may not be deterministically reproducible, so, the bisection point is
random.
Emil is helping me in the debug process, which is still ongoing.
Hi Emil,
yes, the platform is RK3399. Doing tests on the Sapphire board, but it is
also reproducible on gru-kevin. I prefer not testing on the chromebook
because when it fails it becomes unbootable (a brick) and need to take it
apart to connect an external programmer.
Now testing your suggestion:
--- a/drivers/spi/spi-rockchip.c
+++ b/drivers/spi/spi-rockchip.c
@@ -720,5 +720,4 @@ static int rockchip_spi_probe(struct platform_device *)
if (master->dma_tx && master->dma_rx) {
rs->dma_addr_tx = mem->start + ROCKCHIP_SPI_TXDR;
rs->dma_addr_rx = mem->start + ROCKCHIP_SPI_RXDR;
- master->can_dma = rockchip_spi_can_dma;
}
so far so good, but let me some time to make sure the results are
deterministic.
Now it works without dma and without the assigned-clock* props in the dts.
If that proves to fix the issue, which would be the next debugging step?
Regarding the spidev: if the spi1 dts is
&spi1 { status = "okay"; /* and nothing else */ };
then the /sys/bus/spi/devices directory is empty, so, it cant be bound.
What i am doing wrong?
Regards,
Vicenç.