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

[Schrempf Frieder]

On 10.01.20 11:58, Kuldeep Singh wrote:
> 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.

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
>>>