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

[Michal Simek]

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.

Thanks,
Michal