[EXT] Re: [Patch v3] net: pfe_eth: Use spi_flash_read API to access flash memory

[Kuldeep Singh]

Hi Frieder,

> -----Original Message-----
> From: Schrempf Frieder <frieder.schrempf at kontron.de>
> Sent: Wednesday, January 8, 2020 2:52 PM
> To: Kuldeep Singh <kuldeep.singh at nxp.com>; u-boot at lists.denx.de
> Cc: Joe Hershberger <joe.hershberger at ni.com>; Thomas Hebb
> <tommyhebb at gmail.com>; Patrick Delaunay <patrick.delaunay at st.com>
> Subject: [EXT] Re: [Patch v3] net: pfe_eth: Use spi_flash_read API to access
> flash memory
> 
> Caution: EXT Email
> 
> On 08.01.20 10:08, Kuldeep Singh wrote:
> > Current PFE firmware access spi-nor memory directly. New spi-mem
> > framework does not support direct memory access. So, let's use
> > spi_flash_read API to access memory instead of directly using it.
> >
> > Signed-off-by: Kuldeep Singh <kuldeep.singh at nxp.com>
> > ---
> > v3:
> > -Replace ret with 0 if flash probe fails
> > v2:
> > -Add return error code
> > -Some changes in displayed log
> >
> >   drivers/net/pfe_eth/pfe_firmware.c | 45
> +++++++++++++++++++++++++++++++++++++-
> >   include/configs/ls1012a_common.h   |  5 ++++-
> >   2 files changed, 48 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/pfe_eth/pfe_firmware.c
> > b/drivers/net/pfe_eth/pfe_firmware.c
> > index e4563f1..6145f4e 100644
> > --- a/drivers/net/pfe_eth/pfe_firmware.c
> > +++ b/drivers/net/pfe_eth/pfe_firmware.c
> > @@ -12,13 +12,14 @@
> >
> >   #include <net/pfe_eth/pfe_eth.h>
> >   #include <net/pfe_eth/pfe_firmware.h>
> > +#include <spi_flash.h>
> >   #ifdef CONFIG_CHAIN_OF_TRUST
> >   #include <fsl_validate.h>
> >   #endif
> >
> >   #define PFE_FIRMWARE_FIT_CNF_NAME   "config at 1"
> >
> > -static const void *pfe_fit_addr = (void *)CONFIG_SYS_LS_PFE_FW_ADDR;
> > +static const void *pfe_fit_addr;
> >
> >   /*
> >    * PFE elf firmware loader.
> > @@ -159,6 +160,44 @@ static int pfe_fit_check(void)
> >       return ret;
> >   }
> >
> > +int pfe_spi_flash_init(void)
> > +{
> > +     struct spi_flash *pfe_flash;
> > +     int ret = 0;
> > +     void *addr = malloc(CONFIG_SYS_QE_FMAN_FW_LENGTH);
> > +
> > +#ifdef CONFIG_DM_SPI_FLASH
> > +     struct udevice *new;
> > +
> > +     /* speed and mode will be read from DT */
> > +     ret = spi_flash_probe_bus_cs(CONFIG_ENV_SPI_BUS,
> > +                                  CONFIG_ENV_SPI_CS, 0, 0, &new);
> > +
> > +     pfe_flash = dev_get_uclass_priv(new); #else
> > +     pfe_flash = spi_flash_probe(CONFIG_ENV_SPI_BUS,
> > +                                 CONFIG_ENV_SPI_CS,
> > +                                 CONFIG_ENV_SPI_MAX_HZ,
> > +                                 CONFIG_ENV_SPI_MODE); #endif
> > +     if (!pfe_flash) {
> > +             printf("SF: probe for pfe failed\n");
> > +             return 0;
> 
> No again. It seems like you're not getting what I'm trying to tell you, so it
> would be better to just ask back instead of posting new versions.
> 
> pfe_spi_flash_init() should return an error code in case the probe of the SPI
> flash failed. An error code is a negative value like for example -ENODEV to
> report that the device is not available to the calling function.

Thanks Frieder for providing the info.
I will return -ENODEV here instead of 0.

> 
> > +     }
> > +
> > +     ret = spi_flash_read(pfe_flash,
> > +                          CONFIG_SYS_LS_PFE_FW_ADDR,
> > +                          CONFIG_SYS_QE_FMAN_FW_LENGTH,
> > +                          addr);
> > +     if (ret)
> > +             printf("SF: read for pfe failed\n");

I think -EIO should also be returned here as pfe functionality will fail if data is not read.
Please confirm the same if error code is correct/required. I will update this in next version.

Thanks
Kuldeep

> > +
> > +     pfe_fit_addr = addr;
> > +     spi_flash_free(pfe_flash);
> > +
> > +     return ret;
> > +}
> > +
> >   /*
> >    * PFE firmware initialization.
> >    * Loads different firmware files from FIT image.
> > @@ -183,6 +222,10 @@ int pfe_firmware_init(void)
> >       int ret = 0;
> >       int fw_count;
> >
> > +     ret = pfe_spi_flash_init();
> > +     if (ret)
> > +             goto err;
> > +
> >       ret = pfe_fit_check();
> >       if (ret)
> >               goto err;
> > diff --git a/include/configs/ls1012a_common.h
> > b/include/configs/ls1012a_common.h
> > index 2579e2f..cbc5bff 100644
> > --- a/include/configs/ls1012a_common.h
> > +++ b/include/configs/ls1012a_common.h
> > @@ -36,9 +36,12 @@
> >   /* Size of malloc() pool */
> >   #define CONFIG_SYS_MALLOC_LEN               (CONFIG_ENV_SIZE + 128 *
> 1024)
> >
> > +/* PFE */
> > +#define CONFIG_SYS_FMAN_FW_ADDR              0x400d0000
> > +#define CONFIG_SYS_QE_FMAN_FW_LENGTH 0x10000
> > +
> >   /*SPI device */
> >   #if defined(CONFIG_QSPI_BOOT) || defined(CONFIG_TFABOOT)
> > -#define CONFIG_SYS_FMAN_FW_ADDR              0x400d0000
> >   #define CONFIG_SPI_FLASH_SPANSION
> >   #define CONFIG_FSL_SPI_INTERFACE
> >   #define CONFIG_SF_DATAFLASH
> >