Re: [PATCH 4/5] spi: spi-nxp-fspi: add function to select sample clock source for flash reading

[Michael Walle <mwalle@kernel.org>]

Hi,

>> > From: Haibo Chen <haibo.chen@nxp.com>
>> >
>> > fspi define four mode for sample clock source selection.
>> >
>> > Here is the list of modes:
>> > mode 0: Dummy Read strobe generated by FlexSPI Controller and loopback
>> > internally mode 1: Dummy Read strobe generated by FlexSPI Controller
>> > and loopback from DQS pad mode 2: Reserved mode 3: Flash provided Read
>> > strobe and input from DQS pad
>> >
>> > In default, fspi use mode 0 after reset.
>> > For 8-8-8-DTR mode, need to use mode 3, otherwise 8-8-8-DTR read
>> > always get incorrect data.
>> 
>> I'd say this is board dependant, right? If you now hardcode 8d8d8d to 
>> always use
>> mode 3. I'm not sure how a board which doesn't have the DQS connected 
>> to the
>> flash can change this to another mode again. Looks like we'd need a 
>> (DT)
>> property which tells you if there is actually a DQS line connected to 
>> the flash.
> 
> Currently we distinguish through SoC chip, not board. Like patch5.
> If SoC contain the DQS, but the board do not connect it to flash 
> device, then this is a real issue.

See below, there are alternatives to the DQS pin.

But it really is frequency dependent. I'd bet you can use mode 0
with a slow frequency in 8d8d8d mode. I.e. the LS1028ARM will always
refer you to mode 3 if you want to archive "the highest frequency".

> But I think if user use one octal flash device which support dtr mode, 
> they should
> connect this DQS pad if want to work in DTR mode.
> If forget to connect the DQS pad, they can limit the tx/rx buswidth to 
> 4 or 1 in dts.
> 
> Anyway, add a DT property seems better.
> 
> DQS is a must requirement for octal dtr mode, if detect no DQS, we can 
> also disable the DTR mode support.

I don't think this is true in general. I haven't checked with the FSPI
controller. But DQS is used for higher frequencies which isn't 
necessarily
related to DTR.

Also, there are other methods, like manual calibration of the delay 
lines
on the I/Os. There was once a patchset to add that calibration for a TI 
(?)
controller, but it was never merged. I've seen the fspi also supports
some DLL configuration and some kind of data learning feature. As far as
I understand that, these are all alternatives. I.e.
  (1) don't use high frequencies
  (2) use DQS (driven by the flash)
  (3) manually/automatically set the DLL

> Will add in next version.
> 
>> Btw you don't check buswidth, so you'll enable that mode for any DTR 
>> mode.
> 
> Seems current spi-nor code only support one DTR mode, that is 8d-8d-8d.

Yes. But that doesn't mean the spi controller should assume that. If you
don't want to support any other mode, you should probably check that in
your .supports_op() callback.

-michael