[U-Boot] [RFC PATCH v2 00/11] SF: Migrate to Linux SPI NOR framework

[Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>]

Am 06.12.2018 um 18:39 schrieb Vignesh R:
> 
> 
> On 06/12/18 10:06 PM, Simon Goldschmidt wrote:
>> Am 06.12.2018 um 14:54 schrieb Simon Goldschmidt:
>>> On Thu, Dec 6, 2018 at 2:45 PM Vignesh R <vigneshr@ti.com> wrote:
>>>>
>>>> Hi Simon,
>>>>
>>>> On 06/12/18 2:15 AM, Simon Goldschmidt wrote:
>>>>> Am 05.12.2018 um 07:55 schrieb Simon Goldschmidt:
>>>>>> On Wed, Dec 5, 2018 at 7:51 AM Vignesh R <vigneshr@ti.com> wrote:
>>>> [...]
>>>>>>>> I did some compilation tests first and I'm happy to say that with the
>>>>>>>> SPL_SPI_FLASH_TINY option enabled, my SPL is about 1900 byte smaller
>>>>>>>> than before :-)
>>>>>>>>
>>>>>>>> However, my socfpga socrates board does not boot. I'll have to
>>>>>>>> investigate why. I just applied this series and compiled for
>>>>>>>> socfpga_socrates_defconfig. Is there anything else I should have changed
>>>>>>>> to make it work?
>>>>>>>>
>>>>>>>
>>>>>>> Oops, that's unfortunate.
>>>>>>> Just to be sure, does SPL fail to come up or SPL fails to load U-Boot
>>>>>>> from flash? Is this with SPL_SPI_FLASH_TINY enabled?
>>>>>>
>>>>>> I tried both TINY and standard. Both are failing to load U-Boot (I get
>>>>>> the standard error message that loading from SPI flash failed).
>>>>>>
>>>>>>> Could you enable debug prints at the end of spi_mem_exec_op() in
>>>>>>> drivers/spi/spi-mem.c to see what commands are sent and their responses?
>>>>>>
>>>>>> OK, I'll do that.
>>>>>>
>>>>>>> I have TI EVM with Cadence QSPI(like SoCFPGA) but with a Spansion flash
>>>>>>> and that seems to work fine with both full and tiny stack.
>>>>>
>>>>> OK, so I enabled some debugging and the first issue is that U-Boot has
>>>>> SNOR_MFR_MICRON and SNOR_MFR_ST while Linux only has SNOR_MFR_MICRON but
>>>>> defines it to CFI_MFR_ST (0x20, U-Boot ends up with 0x2c). While this
>>>>> might be correct, it leads to my n25q256a not being handled as
>>>>> "micron-like" and the code falls through to using Spansion-like opcode
>>>>> (0x17) that my chip does not understand.
>>>>>
>>>>
>>>> Oops, sorry, the micron chip I had has CFI MFR id of 0x2c, so I
>>>> overlooked this. I have updated this now.
>>>>
>>>>> However, after changing this (and using opcode 0xb7 to bring the chip
>>>>> into 4 byte mode, including 0x06 for write-enable), it still does not
>>>>> work as it uses the 'READ' opcode 0x03 instead of the 'FAST READ' opcode
>>>>> 0x0b that U-Boot used until now. The problem here might be that 'READ'
>>>>> cannot do Quad-SPI (4-4-4) and I'm not sure the Cadence driver can
>>>>> handle Extended-SPI (1-1-1) that the 'READ' command is limited to...
>>>>>
>>>>
>>>> I had accidentally dropped Fast Read from SPI NOR layer and have sync'd
>>>> it up now.  I see that in socfpga dts, flash does not contain:
>>>> spi-rx-bus-width = <4>;
>>>
>>> We can add that if it helps. I'll try to update the upstream Linux dts
>>> anyway to add "jeded,spi-nor" compatible once Neil's series is
>>> applied.
>>>
>>>> Therefore SPI NOR layer falls back to READ (even though its a QSPI
>>>> flash). But nevertheless with updated branch[1] you should see falling
>>>> back to FAST READ with Cadence QSPI on your board (and 1-1-4, if rx bus
>>>> width is 4).
>>>>
>>>>> Using SFDP would probably help, but that doesn't compile (as I wrote in
>>>>> the other thread).
>>>>>
>>>>
>>>> Sorry for that. I have fixed that up and tested SFDP parsing logic with
>>>> a Spansion flash. Note that Cadence QSPI wont support SFDP parsing as
>>>> driver enables Quad mode for all reads and SFDP is only 1-1-1.
>>>
>>> Oh, I haven't really looked into that yet. That's sad.
>>>
> 
> Yes, thats one of the motivation for this series. I wanted to use same driver
> to support Cadence Octal SPI IP but this simply wasn't possible due disconnect b/w SF and SPI layer.
> I plan to fix Cadence QSPI by moving to spi-mem layer once SF layer is in place
> 
>>>>
>>>>> On the other hand, enabling SFDP will increase the text/rodata size for
>>>>> SPL. We might need to remove the non-SFDP and write code as a counter
>>>>> measure to prevent increasing code size.
>>>>>
>>>>
>>>> That's not an option as Flash devices (even though have same JEDEC ID)
>>>> manufactured before JESD216 std dont have SFDP populated. So, we will
>>>> need to have both non-SFDP and SFDP support with option to disable SFDP
>>>> (for size and boot time optimizations).
>>>
>>> Well, my idea was if you'd know that all the flash chips on your board
>>> supported it, you'd be safe. It would only be a Kconfig option to
>>> reduce binary size (get rid of the flash IDs table).
>>>>> But if SFDP doesn't work for QSPI anyway, that's not the best idea, I guess...
>>>
> 
> Yeah, current U-Boot SPI layer does not allow SPI Flash layer to communicate
> bus width to SPI controller drivers. Either drivers capable of supporting
> Quad mode should move to spi-mem framework or spi_xfer() interface needs to be expanded.
> 
>>>> Thanks for all the debugging! It would be great if you could try below
>>>> branch and let me know if it fixes all the issues.
>>>> If yes, I will remove RFC and post new series.
>>>
>>> Sure, I'll try to find the time soon.
>>
>> Had a quick test with both standard and tiny in SPL and both configs now
>> work on my board. I haven't tested SFDP as you said it does not works on
>> Cadence QSPI. (I tested compiling it though, and that works now.)
>>

After enabling DEBUG in spi-mem.c, I see that we're changing to 4-byte 
mode (via b7h) instead of using 4-byte opcodes. Unfortunately, this 
conflicts with the boot rom using the default read opcose in 3-byte mode 
on warm reboot.

Do you plan to change this or would this be fixed by parsing SFDP?

> 
> I used below hack to test SFDP with Cadence QSPI:

Just by accident, I saw that enabling the SFDP config does not seem to 
break booting. But it still switches to 4-byte mode, so it probably just 
fails to parse and continues the non-SFDP way?

> 
> diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c
> index a8af35203035..9dbf9aa7d20c 100644
> --- a/drivers/spi/cadence_qspi_apb.c
> +++ b/drivers/spi/cadence_qspi_apb.c
> @@ -559,7 +559,7 @@ int cadence_qspi_apb_indirect_read_setup(struct cadence_spi_platdata *plat,
>          /* Configure the opcode */
>          rd_reg = cmdbuf[0] << CQSPI_REG_RD_INSTR_OPCODE_LSB;
>   
> -       if (rx_width & SPI_RX_QUAD)
> +       if (rx_width & SPI_RX_QUAD && (cmdbuf[0] != 0x5a))

But this is Spansion-specific, right?

Regards,
Simon

>                  /* Instruction and address at DQ0, data at DQ0-3. */
>                  rd_reg |= CQSPI_INST_TYPE_QUAD << CQSPI_REG_RD_INSTR_TYPE_DATA_LSB;
>   
> 
> 
> 
>> Oh, and I think it's important to say that with your spi-nor-tiny code,
>> SPL code size is reduced by 1980 bytes for my config
>> (socfpga_socrates_defconfig)!
>>
> 
> Thats good to know.
> 
>> Regards,
>> Simon
>>
>>>
>>> Regards,
>>> Simon
>>>
>>>>
>>>> [1] https://github.com/r-vignesh/u-boot.git spi-nor-mig-patch-v1
>>>>
>>>> --
>>>> Regards
>>>> Vignesh
>>
>