[U-Boot] [UBOOT PATCH v3 3/3] spi: xilinx_spi: Added support to read JEDEC-id twice at the boot time

[Jagan Teki]

On Wed, Jun 27, 2018 at 12:59 PM, Michal Simek <michal.simek at xilinx.com> wrote:
> Hi Jagan,
>
> On 27.6.2018 08:43, Jagan Teki wrote:
>> On Mon, Jun 25, 2018 at 4:06 PM, Vipul Kumar <vipulk at xilinx.com> wrote:
>>> Hi Jagan,
>>>
>>>> -----Original Message-----
>>>> From: Jagan Teki [mailto:jagannadh.teki at gmail.com]
>>>> Sent: Monday, June 25, 2018 3:47 PM
>>>> To: Vipul Kumar <vipulk at xilinx.com>
>>>> Cc: U-Boot Mailing List <u-boot at lists.denx.de>; Michal Simek
>>>> <michals at xilinx.com>
>>>> Subject: Re: [U-Boot] [UBOOT PATCH v3 3/3] spi: xilinx_spi: Added support to
>>>> read JEDEC-id twice at the boot time
>>>>
>>>> On Thu, Jun 21, 2018 at 2:53 PM, Vipul Kumar <vipul.kumar at xilinx.com>
>>>> wrote:
>>>>> This patch is for the startup block issue in the spi controller.
>>>>> SPI clock is passing through STARTUP block to FLASH. STARTUP block
>>>>> don't provide clock as soon as QSPI provides command. So, first
>>>>> command fails.
>>>>
>>>> Does this a controller issue?
>>>
>>> Yes, this is a controller issue.
>>>
>>>>
>>>>>
>>>>> This patch added support to read JEDEC id in xilinx_spi_xfer ().
>>>>>
>>>>> Signed-off-by: Vipul Kumar <vipul.kumar at xilinx.com>
>>>>> ---
>>>>> Changes in v3:
>>>>> - No change
>>>>> ---
>>>>>  drivers/spi/xilinx_spi.c | 41
>>>>> +++++++++++++++++++++++++++++++++++++++++
>>>>>  1 file changed, 41 insertions(+)
>>>>>
>>>>> diff --git a/drivers/spi/xilinx_spi.c b/drivers/spi/xilinx_spi.c index
>>>>> 4026540..61301c2 100644
>>>>> --- a/drivers/spi/xilinx_spi.c
>>>>> +++ b/drivers/spi/xilinx_spi.c
>>>>> @@ -204,6 +204,40 @@ static u32 xilinx_spi_read_rxfifo(struct udevice
>>>> *bus, u8 *rxp, u32 rxbytes)
>>>>>         return i;
>>>>>  }
>>>>>
>>>>> +static void xilinx_startup_block(struct udevice *dev, unsigned int bytes,
>>>>> +                                const void *dout, void *din) {
>>>>> +       struct udevice *bus = dev_get_parent(dev);
>>>>> +       struct xilinx_spi_priv *priv = dev_get_priv(bus);
>>>>> +       struct xilinx_spi_regs *regs = priv->regs;
>>>>> +       struct dm_spi_slave_platdata *slave_plat =
>>>> dev_get_parent_platdata(dev);
>>>>> +       const unsigned char *txp = dout;
>>>>> +       unsigned char *rxp = din;
>>>>> +       static int startup;
>>>>> +       u32 reg, count;
>>>>> +       u32 txbytes = bytes;
>>>>> +       u32 rxbytes = bytes;
>>>>> +
>>>>> +       /*
>>>>> +        * This loop runs two times. First time to send the command.
>>>>> +        * Second time to transfer data. After transferring data,
>>>>> +        * it sets txp to the initial value for the normal operation.
>>>>> +        */
>>>>> +       for ( ; startup < 2; startup++) {
>>>>> +               count = xilinx_spi_fill_txfifo(bus, txp, txbytes);
>>>>> +               reg = readl(&regs->spicr) & ~SPICR_MASTER_INHIBIT;
>>>>> +               writel(reg, &regs->spicr);
>>>>> +               count = xilinx_spi_read_rxfifo(bus, rxp, rxbytes);
>>>>> +               txp = din;
>>>>> +
>>>>> +               if (startup) {
>>>>> +                       spi_cs_deactivate(dev);
>>>>> +                       spi_cs_activate(dev, slave_plat->cs);
>>>>> +                       txp = dout;
>>>>> +               }
>>>>> +       }
>>>>> +}
>>>>> +
>>>>>  static int xilinx_spi_xfer(struct udevice *dev, unsigned int bitlen,
>>>>>                             const void *dout, void *din, unsigned long
>>>>> flags)  { @@ -236,6 +270,13 @@ static int xilinx_spi_xfer(struct
>>>>> udevice *dev, unsigned int bitlen,
>>>>>         if (flags & SPI_XFER_BEGIN)
>>>>>                 spi_cs_activate(dev, slave_plat->cs);
>>>>>
>>>>> +       /*
>>>>> +        * This is the work around for the startup block issue in
>>>>> +        * the spi controller. SPI clock is passing through STARTUP
>>>>> +        * block to FLASH. STARTUP block don't provide clock as soon
>>>>> +        * as QSPI provides command. So first command fails.
>>>>> +        */
>>>>> +       xilinx_startup_block(dev, bytes, dout, din);
>>>>
>>>> But not every spi_xfer from flash side is to read JEDEC ID? during probe of sf,
>>>> the relevant spi_xfer is to read JEDEC ID
>>>
>>> Inside xilinx_startup_block() function, we are using static variable, so that code will execute only first time.
>>
>> Sorry, IMHO mainline won't accept this kind of hack (particularly
>> avoiding function execution through static variables) by for the sake
>> of bug fix. Having a proper condition to check and fix the errata look
>> fine to me, any idea how Linux handling this?
>
> ok - static startup variable should be move to private data structure to
> be private for every instance.
>
> Also please put a link to errata for a record.
>
> Jagan: It will be much easier if you can simply write snippet which you
> are suggesting.

thought of, I'm still thinking how we can do this. I didn't see the
Linux code yet, any idea how Linux does?

Jagan.