[U-Boot] [Patch V2 4/4] dm: env_sf: fix saveenv() to use driver model

[Qianyu Gong]

> -----Original Message-----
> From: sjg at google.com [mailto:sjg at google.com] On Behalf Of Simon Glass
> Sent: Friday, January 08, 2016 11:34 AM
> To: Gong Qianyu <Qianyu.Gong at freescale.com>
> Cc: U-Boot Mailing List <u-boot at lists.denx.de>; Mingkai Hu
> <Mingkai.Hu at freescale.com>; R58495 at freescale.com; yao.yuan at freescale.com;
> Jagan Teki <jteki at openedev.com>
> Subject: Re: [Patch V2 4/4] dm: env_sf: fix saveenv() to use driver model
> 
> Hi,
> 
> On 15 December 2015 at 03:32, Gong Qianyu <Qianyu.Gong at freescale.com>
> wrote:
> > It might be missed when converting spi_flash_probe() in cmd_sf.c.
> >
> > This commit refers to fbb099183e3a53f77a975964cdf2e73d11e565af.
> >
> > Signed-off-by: Gong Qianyu <Qianyu.Gong at freescale.com>
> > ---
> > V2:
> >  - New Patch.
> >
> >  common/env_sf.c | 49
> > +++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 49 insertions(+)
> >
> > diff --git a/common/env_sf.c b/common/env_sf.c index 9409831..31d96a7
> > 100644
> > --- a/common/env_sf.c
> > +++ b/common/env_sf.c
> > @@ -16,6 +16,7 @@
> >  #include <spi_flash.h>
> >  #include <search.h>
> >  #include <errno.h>
> > +#include <dm/device-internal.h>
> >
> >  #ifndef CONFIG_ENV_SPI_BUS
> >  # define CONFIG_ENV_SPI_BUS    0
> > @@ -51,6 +52,29 @@ int saveenv(void)
> >         char    *saved_buffer = NULL, flag = OBSOLETE_FLAG;
> >         u32     saved_size, saved_offset, sector = 1;
> >         int     ret;
> > +#ifdef CONFIG_DM_SPI_FLASH
> > +       struct udevice *new, *bus_dev;
> > +       unsigned int bus = CONFIG_SF_DEFAULT_BUS;
> > +       unsigned int cs = CONFIG_SF_DEFAULT_CS;
> > +       unsigned int speed = CONFIG_SF_DEFAULT_SPEED;
> > +       unsigned int mode = CONFIG_SF_DEFAULT_MODE;
> > +
> > +       /* Remove the old device, otherwise probe will just be a nop */
> > +       ret = spi_find_bus_and_cs(bus, cs, &bus_dev, &new);
> > +       if (!ret) {
> > +               device_remove(new);
> > +               device_unbind(new);
> > +       }
> > +       env_flash = NULL;
> 
> You should be abve to avoid the above code and just have the code below:
> 
> > +       ret = spi_flash_probe_bus_cs(bus, cs, speed, mode, &new);
> > +       if (ret) {
> > +               printf("Failed to initialize SPI flash at %u:%u (error %d)\n",
> > +                      bus, cs, ret);
> > +               return 1;
> > +       }
> > +
> > +       env_flash = dev_get_uclass_priv(new);
> 
> The reason why 'sf probe' removes the old device is to ensure that it is probed
> again, in case something has changed. Even that is suspect, but in the env case it
> seems plain wrong.
> 
> However, you must always do this with driver model. Even if env_flash is non-
> NULL, you must get it again (with driver model).
> 
Hi Simon,

Ok, I see. Thanks for the explanation. 
I'll send a new version of it then.

Regards,
Qianyu

> > +#else
> >
> >         if (!env_flash) {
> >                 env_flash = spi_flash_probe(CONFIG_ENV_SPI_BUS,
> > @@ -61,6 +85,7 @@ int saveenv(void)
> >                         return 1;
> >                 }
> >         }
> > +#endif
> >
> >         ret = env_export(&env_new);
> >         if (ret)
> > @@ -227,6 +252,29 @@ int saveenv(void)
> >         char    *saved_buffer = NULL;
> >         int     ret = 1;
> >         env_t   env_new;
> > +#ifdef CONFIG_DM_SPI_FLASH
> > +       struct udevice *new, *bus_dev;
> > +       unsigned int bus = CONFIG_SF_DEFAULT_BUS;
> > +       unsigned int cs = CONFIG_SF_DEFAULT_CS;
> > +       unsigned int speed = CONFIG_SF_DEFAULT_SPEED;
> > +       unsigned int mode = CONFIG_SF_DEFAULT_MODE;
> > +
> > +       /* Remove the old device, otherwise probe will just be a nop */
> > +       ret = spi_find_bus_and_cs(bus, cs, &bus_dev, &new);
> > +       if (!ret) {
> > +               device_remove(new);
> > +               device_unbind(new);
> > +       }
> > +       env_flash = NULL;
> > +       ret = spi_flash_probe_bus_cs(bus, cs, speed, mode, &new);
> > +       if (ret) {
> > +               printf("Failed to initialize SPI flash at %u:%u (error %d)\n",
> > +                      bus, cs, ret);
> > +               return 1;
> > +       }
> > +
> > +       env_flash = dev_get_uclass_priv(new); #else
> >
> >         if (!env_flash) {
> >                 env_flash = spi_flash_probe(CONFIG_ENV_SPI_BUS,
> > @@ -237,6 +285,7 @@ int saveenv(void)
> >                         return 1;
> >                 }
> >         }
> > +#endif
> >
> >         /* Is the sector larger than the env (i.e. embedded) */
> >         if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
> > --
> > 2.1.0.27.g96db324
> >
> 
> Regards,
> Simon