On 10.01.20 11:58, Kuldeep Singh wrote: > Hi Frieder, > >> -----Original Message----- >> From: Schrempf Frieder <[email protected]> >> Sent: Wednesday, January 8, 2020 2:52 PM >> To: Kuldeep Singh <[email protected]>; [email protected] >> Cc: Joe Hershberger <[email protected]>; Thomas Hebb >> <[email protected]>; Patrick Delaunay <[email protected]> >> 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 <[email protected]> >>> --- >>> 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. 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 >>>
