RE: [PATCH 0/3] spi: spi-cadence-quadspi: Add Rx tuning support for DTR mode

["Potthuri, Sai Krishna" <sai.krishna.potthuri@xxxxxxx>]

Hi Michael,

> -----Original Message-----
> From: Michael Walle <michael@xxxxxxxx>
> Sent: Tuesday, July 18, 2023 3:31 PM
> To: Tudor Ambarus <tudor.ambarus@xxxxxxxxxx>
> Cc: Potthuri, Sai Krishna <sai.krishna.potthuri@xxxxxxx>; Mark Brown
> <broonie@xxxxxxxxxx>; Tudor Ambarus <tudor.ambarus@xxxxxxxxxxxxx>;
> Pratyush Yadav <pratyush@xxxxxxxxxx>; Miquel Raynal
> <miquel.raynal@xxxxxxxxxxx>; Richard Weinberger <richard@xxxxxx>; Vignesh
> Raghavendra <vigneshr@xxxxxx>; linux-mtd@xxxxxxxxxxxxxxxxxxx; linux-
> spi@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; saikrishna12468@xxxxxxxxx;
> git (AMD-Xilinx) <git@xxxxxxx>
> Subject: Re: [PATCH 0/3] spi: spi-cadence-quadspi: Add Rx tuning support for DTR
> mode
> 
> Am 2023-02-07 07:48, schrieb Tudor Ambarus:
> > On 2/7/23 06:09, Sai Krishna Potthuri wrote:
> >> Enable PHY and DQS required for Xilinx Versal Octal SPI to operate in
> >> DTR protocol.
> >> Add and update device_id field in spi_mem structure with flash id
> >> information. Xilinx Versal Octal SPI driver requires the device id
> >> information to perform the Rx tuning operation. Since there is no
> >> common Tuning Data Pattern defined across all vendors, controllers
> >> like Xilinx Versal Octal SPI which requires Rx tuning to find out the
> >> optimal sampling point for data lines, this device id information
> >> will be used as a golden data.
> >
> > Using only 6 bytes as golden pattern seems fragile, but you are aware
> > of that, as I see that you chose to read the ID 10 times to make the
> > decision whether the tap is valid or not. Other option (which is not
> > perfect) is to use SFDP data as golden pattern. If I remember
> > correctly, JESD216 suggests to use the Read SFDP cmd at 50 MHz, so it
> > won't help you much. In practice SPI NOR uses the Read SFDP command at
> > the flash's maximum speed and we haven't seen problems. But better
> > would be to use some flash OTP data maybe? I remember Pratyush has
> > submitted a phy calibration series in the past, I haven't had the
> > chance to read his proposal. Did you? How's your proposal different
> > than his?
> 
> And its not 6 bytes.. it's usually only three. The last three bytes will probably be
> undefined. So the might return ff or just wrap around and return the first three
> bytes again.
> 
> Is there a datasheet where you can read how the calibration is done? Is this the
> same for all i/o pads or individual per i/o pad?
> 
> I cannot see where the op to read the id is coming from. Are you relying on the
> fact that a RDID is the first command which gets executed. If so, please don't.
> 
> Do you calibrate only one pad? RDID (9f) is  single bit i/o, right? And I guess you
> are calibrating with the highest frequency, are we sure that RDID will work with
> any frequency (on any flash).
> 
> 
> >> The reason behind choosing this approach instead of reading the ID
> >> again in the controller driver is to make it generic solution.
> >> - Other controller drivers which want to use similar tuning process,
> >> they will make use of this ID instead of reading the ID again in the
> >> driver.
> 
> Honestly, I'm not sure this is the way to go. Pratyush proposed solution to have a
> dedicated memory area within the flash array with a know pattern seems to
> make more sense, because you are calibrating on the thing you are going to use
> later, that is quad/ocal read with the fastest frequency.
Yes, I am going through his proposal and working on it to adapt that solution.
Agree, this approach makes more sense than RDID also, with this we can avoid the
cons with RDID approach like small pattern, calibrating one line, frequency limitations
with RDID command(if any) etc.,
 
> 
> >> - Also, we can avoid hardcoding the command information and
> >> initiating the transfer in the controller driver as this should
> >> happen from spi-nor.
> 
> So how you know that this is a RDID instruction?
Yes, I am expecting the first command after switching to DDR mode is RDID.
Agree, we cannot rely on this.

Regards
Sai Krishna