mbox series

[RFC,v6,00/11] hw/ssi: imx_spi: Fix various bugs in the imx_spi model

Message ID 20210112183529.2011863-1-f4bug@amsat.org
Headers show
Series hw/ssi: imx_spi: Fix various bugs in the imx_spi model | expand

Message

Philippe Mathieu-Daudé Jan. 12, 2021, 6:35 p.m. UTC 
Hi,

As it is sometimes harder for me to express myself in plain
English, I found it easier to write the patches I was thinking
about. I know this doesn't scale.

So this is how I understand the ecSPI reset works, after
looking at the IMX6DQRM.pdf datasheet.

This is a respin of Ben's v5 series [*].
Tagged RFC because I have not tested it :)

Sometimes changing device reset to better match hardware gives
trouble when using '-kernel ...' because there is no bootloader
setting the device in the state Linux expects it.

Copy of Ben's v5 cover:

  This series fixes a bunch of bugs in current implementation of the imx
  spi controller, including the following issues:

  - chip select signal was not lower down when spi controller is disabled
  - remove imx_spi_update_irq() in imx_spi_reset()
  - round up the tx burst length to be multiple of 8
  - transfer incorrect data when the burst length is larger than 32 bit
  - spi controller tx and rx fifo endianness is incorrect

[*] https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg02333.html

Diff with Ben's v5:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respective=
ly

001/11:[----] [--] 'hw/ssi: imx_spi: Use a macro for number of chip selects s=
upported'
002/11:[down] 'hw/ssi: imx_spi: Remove pointless variable initialization'
003/11:[down] 'hw/ssi: imx_spi: Convert some debug printf()s to trace events'
004/11:[down] 'hw/ssi: imx_spi: Reduce 'change_mask' variable scope'
005/11:[down] 'hw/ssi: imx_spi: Rework imx_spi_reset() to keep CONREG registe=
r value'
006/11:[down] 'hw/ssi: imx_spi: Rework imx_spi_read() to handle block disable=
d'
007/11:[down] 'hw/ssi: imx_spi: Rework imx_spi_write() to handle block disabl=
ed'
008/11:[0004] [FC] 'hw/ssi: imx_spi: Disable chip selects when controller is =
disabled'
009/11:[----] [--] 'hw/ssi: imx_spi: Round up the burst length to be multiple=
 of 8'
010/11:[----] [--] 'hw/ssi: imx_spi: Correct the burst length > 32 bit transf=
er logic'
011/11:[----] [--] 'hw/ssi: imx_spi: Correct tx and rx fifo endianness'

Bin Meng (4):
  hw/ssi: imx_spi: Use a macro for number of chip selects supported
  hw/ssi: imx_spi: Round up the burst length to be multiple of 8
  hw/ssi: imx_spi: Correct the burst length > 32 bit transfer logic
  hw/ssi: imx_spi: Correct tx and rx fifo endianness

Philippe Mathieu-Daud=C3=A9 (6):
  hw/ssi: imx_spi: Remove pointless variable initialization
  hw/ssi: imx_spi: Convert some debug printf()s to trace events
  hw/ssi: imx_spi: Reduce 'change_mask' variable scope
  hw/ssi: imx_spi: Rework imx_spi_reset() to keep CONREG register value
  hw/ssi: imx_spi: Rework imx_spi_read() to handle block disabled
  hw/ssi: imx_spi: Rework imx_spi_write() to handle block disabled

Xuzhou Cheng (1):
  hw/ssi: imx_spi: Disable chip selects when controller is disabled

 include/hw/ssi/imx_spi.h |   5 +-
 hw/ssi/imx_spi.c         | 147 +++++++++++++++++++++++----------------
 hw/ssi/trace-events      |   7 ++
 3 files changed, 97 insertions(+), 62 deletions(-)

--=20
2.26.2

Comments

Bin Meng Jan. 13, 2021, 3:29 a.m. UTC | #1 
Hi Philippe,

On Wed, Jan 13, 2021 at 2:35 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Hi,
>
> As it is sometimes harder for me to express myself in plain
> English, I found it easier to write the patches I was thinking
> about. I know this doesn't scale.
>
> So this is how I understand the ecSPI reset works, after
> looking at the IMX6DQRM.pdf datasheet.
>
> This is a respin of Ben's v5 series [*].
> Tagged RFC because I have not tested it :)

Unfortunately this series breaks SPI flash testing under both U-Boot
and VxWorks 7.

> Sometimes changing device reset to better match hardware gives
> trouble when using '-kernel ...' because there is no bootloader
> setting the device in the state Linux expects it.
>

Given most of the new changes in this RFC series are clean-ups, I
suggest we apply the v5 series unless there is anything seriously
wrong in v5, IOW, don't fix it unless it's broken.

Thoughts?

Regards,
Bin
Philippe Mathieu-Daudé Jan. 13, 2021, 7:53 a.m. UTC | #2 
Hi Ben,

On 1/13/21 4:29 AM, Bin Meng wrote:
> On Wed, Jan 13, 2021 at 2:35 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> Hi,
>>
>> As it is sometimes harder for me to express myself in plain
>> English, I found it easier to write the patches I was thinking
>> about. I know this doesn't scale.
>>
>> So this is how I understand the ecSPI reset works, after
>> looking at the IMX6DQRM.pdf datasheet.
>>
>> This is a respin of Ben's v5 series [*].
>> Tagged RFC because I have not tested it :)
> 
> Unfortunately this series breaks SPI flash testing under both U-Boot
> and VxWorks 7.

Thanks for testing :) Can you provide the binary tested and the command
line used? At least one, so I can have a look.

>> Sometimes changing device reset to better match hardware gives
>> trouble when using '-kernel ...' because there is no bootloader
>> setting the device in the state Linux expects it.
>>
> 
> Given most of the new changes in this RFC series are clean-ups, I
> suggest we apply the v5 series unless there is anything seriously
> wrong in v5, IOW, don't fix it unless it's broken.
> 
> Thoughts?

Up to the maintainer :)

The IMX6DQRM datasheet is available here:
https://community.nxp.com/t5/i-MX-Processors-Knowledge-Base/i-MX-6DQ-Reference-Manual-IMX6DQRM-R2-Part-1/ta-p/1115983
https://community.nxp.com/t5/i-MX-Processors-Knowledge-Base/i-MX-6DQ-Reference-Manual-IMX6DQRM-R2-Part-2/ta-p/1118510

Regards,

Phil.
Bin Meng Jan. 13, 2021, 1:27 p.m. UTC | #3 
Hi Philippe,

On Wed, Jan 13, 2021 at 3:53 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Hi Ben,
>
> On 1/13/21 4:29 AM, Bin Meng wrote:
> > On Wed, Jan 13, 2021 at 2:35 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >>
> >> Hi,
> >>
> >> As it is sometimes harder for me to express myself in plain
> >> English, I found it easier to write the patches I was thinking
> >> about. I know this doesn't scale.
> >>
> >> So this is how I understand the ecSPI reset works, after
> >> looking at the IMX6DQRM.pdf datasheet.
> >>
> >> This is a respin of Ben's v5 series [*].
> >> Tagged RFC because I have not tested it :)
> >
> > Unfortunately this series breaks SPI flash testing under both U-Boot
> > and VxWorks 7.
>
> Thanks for testing :) Can you provide the binary tested and the command
> line used? At least one, so I can have a look.

Sure, will send you offline.

>
> >> Sometimes changing device reset to better match hardware gives
> >> trouble when using '-kernel ...' because there is no bootloader
> >> setting the device in the state Linux expects it.
> >>
> >
> > Given most of the new changes in this RFC series are clean-ups, I
> > suggest we apply the v5 series unless there is anything seriously
> > wrong in v5, IOW, don't fix it unless it's broken.
> >
> > Thoughts?
>
> Up to the maintainer :)
>
> The IMX6DQRM datasheet is available here:
> https://community.nxp.com/t5/i-MX-Processors-Knowledge-Base/i-MX-6DQ-Reference-Manual-IMX6DQRM-R2-Part-1/ta-p/1115983
> https://community.nxp.com/t5/i-MX-Processors-Knowledge-Base/i-MX-6DQ-Reference-Manual-IMX6DQRM-R2-Part-2/ta-p/1118510

Regards,
Bin
Bin Meng Jan. 13, 2021, 2:16 p.m. UTC | #4 
Hi Philippe,

On Wed, Jan 13, 2021 at 9:27 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Philippe,
>
> On Wed, Jan 13, 2021 at 3:53 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >
> > Hi Ben,
> >
> > On 1/13/21 4:29 AM, Bin Meng wrote:
> > > On Wed, Jan 13, 2021 at 2:35 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> > >>
> > >> Hi,
> > >>
> > >> As it is sometimes harder for me to express myself in plain
> > >> English, I found it easier to write the patches I was thinking
> > >> about. I know this doesn't scale.
> > >>
> > >> So this is how I understand the ecSPI reset works, after
> > >> looking at the IMX6DQRM.pdf datasheet.
> > >>
> > >> This is a respin of Ben's v5 series [*].
> > >> Tagged RFC because I have not tested it :)
> > >
> > > Unfortunately this series breaks SPI flash testing under both U-Boot
> > > and VxWorks 7.
> >
> > Thanks for testing :) Can you provide the binary tested and the command
> > line used? At least one, so I can have a look.
>
> Sure, will send you offline.

Please use attached u-boot image to test.

You will also need the following additional QEMU patches:

http://patchwork.ozlabs.org/project/qemu-devel/patch/1606704602-59435-1-git-send-email-bmeng.cn@gmail.com/
http://patchwork.ozlabs.org/project/qemu-devel/list/?series=221754

$ qemu-system-arm -display none -serial null -serial stdio -M
sabrelite -m 1G -kernel u-boot

=> sf probe
SF: Detected sst25vf016b with page size 256 Bytes, erase size 4 KiB, total 2 MiB
=> sf test 1ff000 1000
SPI flash test:
0 erase: 0 ticks, 4096000 KiB/s 32768.000 Mbps
1 check: 2 ticks, 2000 KiB/s 16.000 Mbps
2 write: 187 ticks, 21 KiB/s 0.168 Mbps
3 read: 2 ticks, 2000 KiB/s 16.000 Mbps
Test passed
0 erase: 0 ticks, 4096000 KiB/s 32768.000 Mbps
1 check: 2 ticks, 2000 KiB/s 16.000 Mbps
2 write: 187 ticks, 21 KiB/s 0.168 Mbps
3 read: 2 ticks, 2000 KiB/s 16.000 Mbps

Regards,
Bin
Philippe Mathieu-Daudé Jan. 13, 2021, 5:56 p.m. UTC | #5 
On 1/13/21 2:27 PM, Bin Meng wrote:
> Hi Philippe,
> 
> On Wed, Jan 13, 2021 at 3:53 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> Hi Ben,
>>
>> On 1/13/21 4:29 AM, Bin Meng wrote:
>>> On Wed, Jan 13, 2021 at 2:35 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>>
>>>> Hi,
>>>>
>>>> As it is sometimes harder for me to express myself in plain
>>>> English, I found it easier to write the patches I was thinking
>>>> about. I know this doesn't scale.
>>>>
>>>> So this is how I understand the ecSPI reset works, after
>>>> looking at the IMX6DQRM.pdf datasheet.
>>>>
>>>> This is a respin of Ben's v5 series [*].
>>>> Tagged RFC because I have not tested it :)
>>>
>>> Unfortunately this series breaks SPI flash testing under both U-Boot
>>> and VxWorks 7.
>>
>> Thanks for testing :) Can you provide the binary tested and the command
>> line used? At least one, so I can have a look.
> 
> Sure, will send you offline.

Arf, stupid mistake in patch 7 :) With this diff I can run your
test:

-- >8 --
--- a/hw/ssi/imx_spi.c
+++ b/hw/ssi/imx_spi.c
@@ -343,7 +343,7 @@ static void imx_spi_write(void *opaque, hwaddr
offset, uint64_t value,
             return;
         }
         s->regs[ECSPI_CONREG] = value;
-        if (value & ECSPI_CONREG_EN) {
+        if (!(value & ECSPI_CONREG_EN)) {
             /* Keep disabled */
             return;
         }
---

Regards,

Phil.
Philippe Mathieu-Daudé Jan. 14, 2021, 4 p.m. UTC | #6 
Hi Ben,

On 1/13/21 6:56 PM, Philippe Mathieu-Daudé wrote:
> On 1/13/21 2:27 PM, Bin Meng wrote:
>> Hi Philippe,
>>
>>>> Unfortunately this series breaks SPI flash testing under both U-Boot
>>>> and VxWorks 7.
>>>
>>> Thanks for testing :) Can you provide the binary tested and the command
>>> line used? At least one, so I can have a look.
>>
>> Sure, will send you offline.
> 
> Arf, stupid mistake in patch 7 :) With this diff I can run your
> test:
> 
> -- >8 --
> --- a/hw/ssi/imx_spi.c
> +++ b/hw/ssi/imx_spi.c
> @@ -343,7 +343,7 @@ static void imx_spi_write(void *opaque, hwaddr
> offset, uint64_t value,
>              return;
>          }
>          s->regs[ECSPI_CONREG] = value;
> -        if (value & ECSPI_CONREG_EN) {
> +        if (!(value & ECSPI_CONREG_EN)) {
>              /* Keep disabled */
>              return;
>          }
> ---

Could you have a try at this? Do you prefer I resubmit
the whole series?

Thanks,

Phil.