> -----Original Message-----
> From: Schrempf Frieder <frieder.schre...@kontron.de>
> Sent: Monday, January 13, 2020 2:07 PM
> To: Kuldeep Singh <kuldeep.si...@nxp.com>; u-boot@lists.denx.de
> Cc: Joe Hershberger <joe.hershber...@ni.com>; Thomas Hebb
> <tommyh...@gmail.com>; Patrick Delaunay <patrick.delau...@st.com>
> Subject: Re: [EXT] Re: [Patch v3] net: pfe_eth: Use spi_flash_read API to
> access
> flash memory
>
> Caution: EXT Email
>
> On 10.01.20 11:58, Kuldeep Singh wrote:
> > Hi Frieder,
> >
> >> -----Original Message-----
> >> From: Schrempf Frieder <frieder.schre...@kontron.de>
> >> Sent: Wednesday, January 8, 2020 2:52 PM
> >> To: Kuldeep Singh <kuldeep.si...@nxp.com>; u-boot@lists.denx.de
> >> Cc: Joe Hershberger <joe.hershber...@ni.com>; Thomas Hebb
> >> <tommyh...@gmail.com>; Patrick Delaunay <patrick.delau...@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.si...@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@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.
>
> No, I don't think it's a good idea to return an error code here. You already
> get
> an error code back from spi_flash_read() if it fails and you pass this error
> to
> the caller at the end of the function, which should be sufficient.
Okay. I will not return error code here.
Thanks
Kuldeep
>
> Also returning here would require to add a call to spi_flash_free() first.
>
> >
> > 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
> >>>
