| Message ID | 20200330033158.26751-2-walter.lozano@collabora.com |
|---|---|
| State | RFC |
| Delegated to: | Stefano Babic |
| Headers | show |
| Series | mx6cuboxi: enable OF_PLATDATA with MMC support | expand |
Hi Walter, On Sun, 29 Mar 2020 at 21:32, Walter Lozano <walter.lozano@collabora.com> wrote: > > Signed-off-by: Walter Lozano <walter.lozano@collabora.com> > --- > drivers/mmc/fsl_esdhc_imx.c | 46 +++++++++++++++++++++++++++++++++---- > 1 file changed, 42 insertions(+), 4 deletions(-) > > diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c > index 4900498e9b..761a4b46e9 100644 > --- a/drivers/mmc/fsl_esdhc_imx.c > +++ b/drivers/mmc/fsl_esdhc_imx.c > @@ -29,6 +29,8 @@ > #include <dm.h> > #include <asm-generic/gpio.h> > #include <dm/pinctrl.h> > +#include <dt-structs.h> > +#include <mapmem.h> > > #if !CONFIG_IS_ENABLED(BLK) > #include "mmc_private.h" > @@ -98,6 +100,11 @@ struct fsl_esdhc { > }; > > struct fsl_esdhc_plat { > +#if CONFIG_IS_ENABLED(OF_PLATDATA) > + /* Put this first since driver model will copy the data here */ > + struct dtd_fsl_imx6q_usdhc dtplat; > +#endif > + > struct mmc_config cfg; > struct mmc mmc; > }; > @@ -1377,14 +1384,18 @@ static int fsl_esdhc_probe(struct udevice *dev) > struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev); > struct fsl_esdhc_plat *plat = dev_get_platdata(dev); > struct fsl_esdhc_priv *priv = dev_get_priv(dev); > +#if !CONFIG_IS_ENABLED(OF_PLATDATA) > const void *fdt = gd->fdt_blob; > int node = dev_of_offset(dev); > + fdt_addr_t addr; > +#else > + struct dtd_fsl_imx6q_usdhc *dtplat = &plat->dtplat; > +#endif > struct esdhc_soc_data *data = > (struct esdhc_soc_data *)dev_get_driver_data(dev); > #if CONFIG_IS_ENABLED(DM_REGULATOR) > struct udevice *vqmmc_dev; > #endif > - fdt_addr_t addr; > unsigned int val; > struct mmc *mmc; > #if !CONFIG_IS_ENABLED(BLK) > @@ -1392,14 +1403,23 @@ static int fsl_esdhc_probe(struct udevice *dev) > #endif > int ret; > > +#if CONFIG_IS_ENABLED(OF_PLATDATA) > + priv->esdhc_regs = map_sysmem(dtplat->reg[0], dtplat->reg[1]); > + val = plat->dtplat.bus_width; > + if (val == 8) > + priv->bus_width = 8; > + else if (val == 4) > + priv->bus_width = 4; > + else > + priv->bus_width = 1; > + priv->non_removable = 1; > +#else > addr = dev_read_addr(dev); > if (addr == FDT_ADDR_T_NONE) > return -EINVAL; > priv->esdhc_regs = (struct fsl_esdhc *)addr; > priv->dev = dev; > priv->mode = -1; > - if (data) > - priv->flags = data->flags; > > val = dev_read_u32_default(dev, "bus-width", -1); > if (val == 8) > @@ -1462,7 +1482,9 @@ static int fsl_esdhc_probe(struct udevice *dev) > priv->vs18_enable = 1; > } > #endif > - > +#endif > + if (data) > + priv->flags = data->flags; > /* > * TODO: > * Because lack of clk driver, if SDHC clk is not enabled, > @@ -1513,9 +1535,11 @@ static int fsl_esdhc_probe(struct udevice *dev) > return ret; > } > > +#if !CONFIG_IS_ENABLED(OF_PLATDATA) Maybe can use if() for this one? > ret = mmc_of_parse(dev, &plat->cfg); > if (ret) > return ret; > +#endif > > mmc = &plat->mmc; > mmc->cfg = &plat->cfg; > @@ -1648,4 +1672,18 @@ U_BOOT_DRIVER(fsl_esdhc) = { > .platdata_auto_alloc_size = sizeof(struct fsl_esdhc_plat), > .priv_auto_alloc_size = sizeof(struct fsl_esdhc_priv), > }; > + > +#if CONFIG_IS_ENABLED(OF_PLATDATA) Don't you already have a U_BOOT_DRIVER declaration? You may need to change the name of your existing driver though (see of-platdata docs). So if it is because of that, please add a comment. > +U_BOOT_DRIVER(fsl_usdhc) = { > + .name = "fsl_imx6q_usdhc", > + .id = UCLASS_MMC, > + .ops = &fsl_esdhc_ops, > +#if CONFIG_IS_ENABLED(BLK) > + .bind = fsl_esdhc_bind, > +#endif > + .probe = fsl_esdhc_probe, > + .platdata_auto_alloc_size = sizeof(struct fsl_esdhc_plat), > + .priv_auto_alloc_size = sizeof(struct fsl_esdhc_priv), > +}; > +#endif > #endif > -- > 2.20.1 > Regards, Simon
Hi Simon, Thank you for taking the time to review this series. On 6/4/20 00:42, Simon Glass wrote: > Hi Walter, > > On Sun, 29 Mar 2020 at 21:32, Walter Lozano<walter.lozano@collabora.com> wrote: >> Signed-off-by: Walter Lozano<walter.lozano@collabora.com> >> --- >> drivers/mmc/fsl_esdhc_imx.c | 46 +++++++++++++++++++++++++++++++++---- >> 1 file changed, 42 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c >> index 4900498e9b..761a4b46e9 100644 >> --- a/drivers/mmc/fsl_esdhc_imx.c >> +++ b/drivers/mmc/fsl_esdhc_imx.c >> @@ -29,6 +29,8 @@ >> #include <dm.h> >> #include <asm-generic/gpio.h> >> #include <dm/pinctrl.h> >> +#include <dt-structs.h> >> +#include <mapmem.h> >> >> #if !CONFIG_IS_ENABLED(BLK) >> #include "mmc_private.h" >> @@ -98,6 +100,11 @@ struct fsl_esdhc { >> }; >> >> struct fsl_esdhc_plat { >> +#if CONFIG_IS_ENABLED(OF_PLATDATA) >> + /* Put this first since driver model will copy the data here */ >> + struct dtd_fsl_imx6q_usdhc dtplat; >> +#endif >> + >> struct mmc_config cfg; >> struct mmc mmc; >> }; >> @@ -1377,14 +1384,18 @@ static int fsl_esdhc_probe(struct udevice *dev) >> struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev); >> struct fsl_esdhc_plat *plat = dev_get_platdata(dev); >> struct fsl_esdhc_priv *priv = dev_get_priv(dev); >> +#if !CONFIG_IS_ENABLED(OF_PLATDATA) >> const void *fdt = gd->fdt_blob; >> int node = dev_of_offset(dev); >> + fdt_addr_t addr; >> +#else >> + struct dtd_fsl_imx6q_usdhc *dtplat = &plat->dtplat; >> +#endif >> struct esdhc_soc_data *data = >> (struct esdhc_soc_data *)dev_get_driver_data(dev); >> #if CONFIG_IS_ENABLED(DM_REGULATOR) >> struct udevice *vqmmc_dev; >> #endif >> - fdt_addr_t addr; >> unsigned int val; >> struct mmc *mmc; >> #if !CONFIG_IS_ENABLED(BLK) >> @@ -1392,14 +1403,23 @@ static int fsl_esdhc_probe(struct udevice *dev) >> #endif >> int ret; >> >> +#if CONFIG_IS_ENABLED(OF_PLATDATA) >> + priv->esdhc_regs = map_sysmem(dtplat->reg[0], dtplat->reg[1]); >> + val = plat->dtplat.bus_width; >> + if (val == 8) >> + priv->bus_width = 8; >> + else if (val == 4) >> + priv->bus_width = 4; >> + else >> + priv->bus_width = 1; >> + priv->non_removable = 1; >> +#else >> addr = dev_read_addr(dev); >> if (addr == FDT_ADDR_T_NONE) >> return -EINVAL; >> priv->esdhc_regs = (struct fsl_esdhc *)addr; >> priv->dev = dev; >> priv->mode = -1; >> - if (data) >> - priv->flags = data->flags; >> >> val = dev_read_u32_default(dev, "bus-width", -1); >> if (val == 8) >> @@ -1462,7 +1482,9 @@ static int fsl_esdhc_probe(struct udevice *dev) >> priv->vs18_enable = 1; >> } >> #endif >> - >> +#endif >> + if (data) >> + priv->flags = data->flags; >> /* >> * TODO: >> * Because lack of clk driver, if SDHC clk is not enabled, >> @@ -1513,9 +1535,11 @@ static int fsl_esdhc_probe(struct udevice *dev) >> return ret; >> } >> >> +#if !CONFIG_IS_ENABLED(OF_PLATDATA) > Maybe can use if() for this one? Thank you for the suggestion >> ret = mmc_of_parse(dev, &plat->cfg); >> if (ret) >> return ret; >> +#endif >> >> mmc = &plat->mmc; >> mmc->cfg = &plat->cfg; >> @@ -1648,4 +1672,18 @@ U_BOOT_DRIVER(fsl_esdhc) = { >> .platdata_auto_alloc_size = sizeof(struct fsl_esdhc_plat), >> .priv_auto_alloc_size = sizeof(struct fsl_esdhc_priv), >> }; >> + >> +#if CONFIG_IS_ENABLED(OF_PLATDATA) > Don't you already have a U_BOOT_DRIVER declaration? > > You may need to change the name of your existing driver though (see > of-platdata docs). > > So if it is because of that, please add a comment. I have my doubts regarding this issue. As I see, this driver is used by many different DT with different compatible strings, so I'm thinking in trying to find a more generic approach. Would it be useful to have a more elaborated solution searching for the compatible string when matching drivers with device? >> +U_BOOT_DRIVER(fsl_usdhc) = { >> + .name = "fsl_imx6q_usdhc", >> + .id = UCLASS_MMC, >> + .ops = &fsl_esdhc_ops, >> +#if CONFIG_IS_ENABLED(BLK) >> + .bind = fsl_esdhc_bind, >> +#endif >> + .probe = fsl_esdhc_probe, >> + .platdata_auto_alloc_size = sizeof(struct fsl_esdhc_plat), >> + .priv_auto_alloc_size = sizeof(struct fsl_esdhc_priv), >> +}; >> +#endif >> #endif >> -- >> 2.20.1 >> > Regards, > Simon
Hi Walter, On Tue, 7 Apr 2020 at 14:05, Walter Lozano <walter.lozano@collabora.com> wrote: > > Hi Simon, > > Thank you for taking the time to review this series. > > On 6/4/20 00:42, Simon Glass wrote: > > Hi Walter, > > > > On Sun, 29 Mar 2020 at 21:32, Walter Lozano<walter.lozano@collabora.com> wrote: > >> Signed-off-by: Walter Lozano<walter.lozano@collabora.com> > >> --- > >> drivers/mmc/fsl_esdhc_imx.c | 46 +++++++++++++++++++++++++++++++++---- > >> 1 file changed, 42 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c > >> index 4900498e9b..761a4b46e9 100644 > >> --- a/drivers/mmc/fsl_esdhc_imx.c > >> +++ b/drivers/mmc/fsl_esdhc_imx.c > >> @@ -29,6 +29,8 @@ > >> #include <dm.h> > >> #include <asm-generic/gpio.h> > >> #include <dm/pinctrl.h> > >> +#include <dt-structs.h> > >> +#include <mapmem.h> > >> > >> #if !CONFIG_IS_ENABLED(BLK) > >> #include "mmc_private.h" > >> @@ -98,6 +100,11 @@ struct fsl_esdhc { > >> }; > >> > >> struct fsl_esdhc_plat { > >> +#if CONFIG_IS_ENABLED(OF_PLATDATA) > >> + /* Put this first since driver model will copy the data here */ > >> + struct dtd_fsl_imx6q_usdhc dtplat; > >> +#endif > >> + > >> struct mmc_config cfg; > >> struct mmc mmc; > >> }; > >> @@ -1377,14 +1384,18 @@ static int fsl_esdhc_probe(struct udevice *dev) > >> struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev); > >> struct fsl_esdhc_plat *plat = dev_get_platdata(dev); > >> struct fsl_esdhc_priv *priv = dev_get_priv(dev); > >> +#if !CONFIG_IS_ENABLED(OF_PLATDATA) > >> const void *fdt = gd->fdt_blob; > >> int node = dev_of_offset(dev); > >> + fdt_addr_t addr; > >> +#else > >> + struct dtd_fsl_imx6q_usdhc *dtplat = &plat->dtplat; > >> +#endif > >> struct esdhc_soc_data *data = > >> (struct esdhc_soc_data *)dev_get_driver_data(dev); > >> #if CONFIG_IS_ENABLED(DM_REGULATOR) > >> struct udevice *vqmmc_dev; > >> #endif > >> - fdt_addr_t addr; > >> unsigned int val; > >> struct mmc *mmc; > >> #if !CONFIG_IS_ENABLED(BLK) > >> @@ -1392,14 +1403,23 @@ static int fsl_esdhc_probe(struct udevice *dev) > >> #endif > >> int ret; > >> > >> +#if CONFIG_IS_ENABLED(OF_PLATDATA) > >> + priv->esdhc_regs = map_sysmem(dtplat->reg[0], dtplat->reg[1]); > >> + val = plat->dtplat.bus_width; > >> + if (val == 8) > >> + priv->bus_width = 8; > >> + else if (val == 4) > >> + priv->bus_width = 4; > >> + else > >> + priv->bus_width = 1; > >> + priv->non_removable = 1; > >> +#else > >> addr = dev_read_addr(dev); > >> if (addr == FDT_ADDR_T_NONE) > >> return -EINVAL; > >> priv->esdhc_regs = (struct fsl_esdhc *)addr; > >> priv->dev = dev; > >> priv->mode = -1; > >> - if (data) > >> - priv->flags = data->flags; > >> > >> val = dev_read_u32_default(dev, "bus-width", -1); > >> if (val == 8) > >> @@ -1462,7 +1482,9 @@ static int fsl_esdhc_probe(struct udevice *dev) > >> priv->vs18_enable = 1; > >> } > >> #endif > >> - > >> +#endif > >> + if (data) > >> + priv->flags = data->flags; > >> /* > >> * TODO: > >> * Because lack of clk driver, if SDHC clk is not enabled, > >> @@ -1513,9 +1535,11 @@ static int fsl_esdhc_probe(struct udevice *dev) > >> return ret; > >> } > >> > >> +#if !CONFIG_IS_ENABLED(OF_PLATDATA) > > Maybe can use if() for this one? > > Thank you for the suggestion > > >> ret = mmc_of_parse(dev, &plat->cfg); > >> if (ret) > >> return ret; > >> +#endif > >> > >> mmc = &plat->mmc; > >> mmc->cfg = &plat->cfg; > >> @@ -1648,4 +1672,18 @@ U_BOOT_DRIVER(fsl_esdhc) = { > >> .platdata_auto_alloc_size = sizeof(struct fsl_esdhc_plat), > >> .priv_auto_alloc_size = sizeof(struct fsl_esdhc_priv), > >> }; > >> + > >> +#if CONFIG_IS_ENABLED(OF_PLATDATA) > > Don't you already have a U_BOOT_DRIVER declaration? > > > > You may need to change the name of your existing driver though (see > > of-platdata docs). > > > > So if it is because of that, please add a comment. > > I have my doubts regarding this issue. As I see, this driver is used by > many different DT with different compatible strings, so I'm thinking in > trying to find a more generic approach. Would it be useful to have a > more elaborated solution searching for the compatible string when > matching drivers with device? Yes I think so. Actually searching for a string is not great anyway. I wonder if we can use the linker-list idea in the previous email somehow? Regards, SImon
Hi Simon, On 8/4/20 00:14, Simon Glass wrote: > Hi Walter, > > On Tue, 7 Apr 2020 at 14:05, Walter Lozano <walter.lozano@collabora.com> wrote: >> Hi Simon, >> >> Thank you for taking the time to review this series. >> >> On 6/4/20 00:42, Simon Glass wrote: >>> Hi Walter, >>> >>> On Sun, 29 Mar 2020 at 21:32, Walter Lozano<walter.lozano@collabora.com> wrote: >>>> Signed-off-by: Walter Lozano<walter.lozano@collabora.com> >>>> --- >>>> drivers/mmc/fsl_esdhc_imx.c | 46 +++++++++++++++++++++++++++++++++---- >>>> 1 file changed, 42 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c >>>> index 4900498e9b..761a4b46e9 100644 >>>> --- a/drivers/mmc/fsl_esdhc_imx.c >>>> +++ b/drivers/mmc/fsl_esdhc_imx.c >>>> @@ -29,6 +29,8 @@ >>>> #include <dm.h> >>>> #include <asm-generic/gpio.h> >>>> #include <dm/pinctrl.h> >>>> +#include <dt-structs.h> >>>> +#include <mapmem.h> >>>> >>>> #if !CONFIG_IS_ENABLED(BLK) >>>> #include "mmc_private.h" >>>> @@ -98,6 +100,11 @@ struct fsl_esdhc { >>>> }; >>>> >>>> struct fsl_esdhc_plat { >>>> +#if CONFIG_IS_ENABLED(OF_PLATDATA) >>>> + /* Put this first since driver model will copy the data here */ >>>> + struct dtd_fsl_imx6q_usdhc dtplat; >>>> +#endif >>>> + >>>> struct mmc_config cfg; >>>> struct mmc mmc; >>>> }; >>>> @@ -1377,14 +1384,18 @@ static int fsl_esdhc_probe(struct udevice *dev) >>>> struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev); >>>> struct fsl_esdhc_plat *plat = dev_get_platdata(dev); >>>> struct fsl_esdhc_priv *priv = dev_get_priv(dev); >>>> +#if !CONFIG_IS_ENABLED(OF_PLATDATA) >>>> const void *fdt = gd->fdt_blob; >>>> int node = dev_of_offset(dev); >>>> + fdt_addr_t addr; >>>> +#else >>>> + struct dtd_fsl_imx6q_usdhc *dtplat = &plat->dtplat; >>>> +#endif >>>> struct esdhc_soc_data *data = >>>> (struct esdhc_soc_data *)dev_get_driver_data(dev); >>>> #if CONFIG_IS_ENABLED(DM_REGULATOR) >>>> struct udevice *vqmmc_dev; >>>> #endif >>>> - fdt_addr_t addr; >>>> unsigned int val; >>>> struct mmc *mmc; >>>> #if !CONFIG_IS_ENABLED(BLK) >>>> @@ -1392,14 +1403,23 @@ static int fsl_esdhc_probe(struct udevice *dev) >>>> #endif >>>> int ret; >>>> >>>> +#if CONFIG_IS_ENABLED(OF_PLATDATA) >>>> + priv->esdhc_regs = map_sysmem(dtplat->reg[0], dtplat->reg[1]); >>>> + val = plat->dtplat.bus_width; >>>> + if (val == 8) >>>> + priv->bus_width = 8; >>>> + else if (val == 4) >>>> + priv->bus_width = 4; >>>> + else >>>> + priv->bus_width = 1; >>>> + priv->non_removable = 1; >>>> +#else >>>> addr = dev_read_addr(dev); >>>> if (addr == FDT_ADDR_T_NONE) >>>> return -EINVAL; >>>> priv->esdhc_regs = (struct fsl_esdhc *)addr; >>>> priv->dev = dev; >>>> priv->mode = -1; >>>> - if (data) >>>> - priv->flags = data->flags; >>>> >>>> val = dev_read_u32_default(dev, "bus-width", -1); >>>> if (val == 8) >>>> @@ -1462,7 +1482,9 @@ static int fsl_esdhc_probe(struct udevice *dev) >>>> priv->vs18_enable = 1; >>>> } >>>> #endif >>>> - >>>> +#endif >>>> + if (data) >>>> + priv->flags = data->flags; >>>> /* >>>> * TODO: >>>> * Because lack of clk driver, if SDHC clk is not enabled, >>>> @@ -1513,9 +1535,11 @@ static int fsl_esdhc_probe(struct udevice *dev) >>>> return ret; >>>> } >>>> >>>> +#if !CONFIG_IS_ENABLED(OF_PLATDATA) >>> Maybe can use if() for this one? >> Thank you for the suggestion >> >>>> ret = mmc_of_parse(dev, &plat->cfg); >>>> if (ret) >>>> return ret; >>>> +#endif >>>> >>>> mmc = &plat->mmc; >>>> mmc->cfg = &plat->cfg; >>>> @@ -1648,4 +1672,18 @@ U_BOOT_DRIVER(fsl_esdhc) = { >>>> .platdata_auto_alloc_size = sizeof(struct fsl_esdhc_plat), >>>> .priv_auto_alloc_size = sizeof(struct fsl_esdhc_priv), >>>> }; >>>> + >>>> +#if CONFIG_IS_ENABLED(OF_PLATDATA) >>> Don't you already have a U_BOOT_DRIVER declaration? >>> >>> You may need to change the name of your existing driver though (see >>> of-platdata docs). >>> >>> So if it is because of that, please add a comment. >> I have my doubts regarding this issue. As I see, this driver is used by >> many different DT with different compatible strings, so I'm thinking in >> trying to find a more generic approach. Would it be useful to have a >> more elaborated solution searching for the compatible string when >> matching drivers with device? > Yes I think so. > > Actually searching for a string is not great anyway. I wonder if we > can use the linker-list idea in the previous email somehow? I'm sure I' understand the idea you try to share with me. Sorry, I understand that one limitation of the current implementation of OF_PLATDATA is the fact that the U_BOOT_DRIVER name should match the one used as first entry in DT. As in particular this driver has several compatible { .compatible = "fsl,imx53-esdhc", }, { .compatible = "fsl,imx6ul-usdhc", }, { .compatible = "fsl,imx6sx-usdhc", }, { .compatible = "fsl,imx6sl-usdhc", }, { .compatible = "fsl,imx6q-usdhc", }, { .compatible = "fsl,imx7d-usdhc", .data = (ulong)&usdhc_imx7d_data,}, { .compatible = "fsl,imx7ulp-usdhc", }, { .compatible = "fsl,imx8qm-usdhc", .data = (ulong)&usdhc_imx8qm_data,}, { .compatible = "fsl,imx8mm-usdhc", .data = (ulong)&usdhc_imx8qm_data,}, { .compatible = "fsl,imx8mn-usdhc", .data = (ulong)&usdhc_imx8qm_data,}, { .compatible = "fsl,imx8mq-usdhc", .data = (ulong)&usdhc_imx8qm_data,}, { .compatible = "fsl,imxrt-usdhc", }, and in order to create a general solution, we need to search for the compatible string instead of matching for driver name. Could you please elaborate a bit more your suggestion in order to understand your approach. Thanks in advance. > Regards, > SImon Regards, Walter
Hi Walter, On Thu, 9 Apr 2020 at 13:07, Walter Lozano <walter.lozano@collabora.com> wrote: > > Hi Simon, > > On 8/4/20 00:14, Simon Glass wrote: > > Hi Walter, > > > > On Tue, 7 Apr 2020 at 14:05, Walter Lozano <walter.lozano@collabora.com> wrote: > >> Hi Simon, > >> > >> Thank you for taking the time to review this series. > >> > >> On 6/4/20 00:42, Simon Glass wrote: > >>> Hi Walter, > >>> > >>> On Sun, 29 Mar 2020 at 21:32, Walter Lozano<walter.lozano@collabora.com> wrote: > >>>> Signed-off-by: Walter Lozano<walter.lozano@collabora.com> > >>>> --- > >>>> drivers/mmc/fsl_esdhc_imx.c | 46 +++++++++++++++++++++++++++++++++---- > >>>> 1 file changed, 42 insertions(+), 4 deletions(-) > >>>> > >>>> diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c > >>>> index 4900498e9b..761a4b46e9 100644 > >>>> --- a/drivers/mmc/fsl_esdhc_imx.c > >>>> +++ b/drivers/mmc/fsl_esdhc_imx.c > >>>> @@ -29,6 +29,8 @@ > >>>> #include <dm.h> > >>>> #include <asm-generic/gpio.h> > >>>> #include <dm/pinctrl.h> > >>>> +#include <dt-structs.h> > >>>> +#include <mapmem.h> > >>>> > >>>> #if !CONFIG_IS_ENABLED(BLK) > >>>> #include "mmc_private.h" > >>>> @@ -98,6 +100,11 @@ struct fsl_esdhc { > >>>> }; > >>>> > >>>> struct fsl_esdhc_plat { > >>>> +#if CONFIG_IS_ENABLED(OF_PLATDATA) > >>>> + /* Put this first since driver model will copy the data here */ > >>>> + struct dtd_fsl_imx6q_usdhc dtplat; > >>>> +#endif > >>>> + > >>>> struct mmc_config cfg; > >>>> struct mmc mmc; > >>>> }; > >>>> @@ -1377,14 +1384,18 @@ static int fsl_esdhc_probe(struct udevice *dev) > >>>> struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev); > >>>> struct fsl_esdhc_plat *plat = dev_get_platdata(dev); > >>>> struct fsl_esdhc_priv *priv = dev_get_priv(dev); > >>>> +#if !CONFIG_IS_ENABLED(OF_PLATDATA) > >>>> const void *fdt = gd->fdt_blob; > >>>> int node = dev_of_offset(dev); > >>>> + fdt_addr_t addr; > >>>> +#else > >>>> + struct dtd_fsl_imx6q_usdhc *dtplat = &plat->dtplat; > >>>> +#endif > >>>> struct esdhc_soc_data *data = > >>>> (struct esdhc_soc_data *)dev_get_driver_data(dev); > >>>> #if CONFIG_IS_ENABLED(DM_REGULATOR) > >>>> struct udevice *vqmmc_dev; > >>>> #endif > >>>> - fdt_addr_t addr; > >>>> unsigned int val; > >>>> struct mmc *mmc; > >>>> #if !CONFIG_IS_ENABLED(BLK) > >>>> @@ -1392,14 +1403,23 @@ static int fsl_esdhc_probe(struct udevice *dev) > >>>> #endif > >>>> int ret; > >>>> > >>>> +#if CONFIG_IS_ENABLED(OF_PLATDATA) > >>>> + priv->esdhc_regs = map_sysmem(dtplat->reg[0], dtplat->reg[1]); > >>>> + val = plat->dtplat.bus_width; > >>>> + if (val == 8) > >>>> + priv->bus_width = 8; > >>>> + else if (val == 4) > >>>> + priv->bus_width = 4; > >>>> + else > >>>> + priv->bus_width = 1; > >>>> + priv->non_removable = 1; > >>>> +#else > >>>> addr = dev_read_addr(dev); > >>>> if (addr == FDT_ADDR_T_NONE) > >>>> return -EINVAL; > >>>> priv->esdhc_regs = (struct fsl_esdhc *)addr; > >>>> priv->dev = dev; > >>>> priv->mode = -1; > >>>> - if (data) > >>>> - priv->flags = data->flags; > >>>> > >>>> val = dev_read_u32_default(dev, "bus-width", -1); > >>>> if (val == 8) > >>>> @@ -1462,7 +1482,9 @@ static int fsl_esdhc_probe(struct udevice *dev) > >>>> priv->vs18_enable = 1; > >>>> } > >>>> #endif > >>>> - > >>>> +#endif > >>>> + if (data) > >>>> + priv->flags = data->flags; > >>>> /* > >>>> * TODO: > >>>> * Because lack of clk driver, if SDHC clk is not enabled, > >>>> @@ -1513,9 +1535,11 @@ static int fsl_esdhc_probe(struct udevice *dev) > >>>> return ret; > >>>> } > >>>> > >>>> +#if !CONFIG_IS_ENABLED(OF_PLATDATA) > >>> Maybe can use if() for this one? > >> Thank you for the suggestion > >> > >>>> ret = mmc_of_parse(dev, &plat->cfg); > >>>> if (ret) > >>>> return ret; > >>>> +#endif > >>>> > >>>> mmc = &plat->mmc; > >>>> mmc->cfg = &plat->cfg; > >>>> @@ -1648,4 +1672,18 @@ U_BOOT_DRIVER(fsl_esdhc) = { > >>>> .platdata_auto_alloc_size = sizeof(struct fsl_esdhc_plat), > >>>> .priv_auto_alloc_size = sizeof(struct fsl_esdhc_priv), > >>>> }; > >>>> + > >>>> +#if CONFIG_IS_ENABLED(OF_PLATDATA) > >>> Don't you already have a U_BOOT_DRIVER declaration? > >>> > >>> You may need to change the name of your existing driver though (see > >>> of-platdata docs). > >>> > >>> So if it is because of that, please add a comment. > >> I have my doubts regarding this issue. As I see, this driver is used by > >> many different DT with different compatible strings, so I'm thinking in > >> trying to find a more generic approach. Would it be useful to have a > >> more elaborated solution searching for the compatible string when > >> matching drivers with device? > > Yes I think so. > > > > Actually searching for a string is not great anyway. I wonder if we > > can use the linker-list idea in the previous email somehow? > > > I'm sure I' understand the idea you try to share with me. Sorry, I > understand that one limitation of the current implementation of > OF_PLATDATA is the fact that the U_BOOT_DRIVER name should match the one > used as first entry in DT. As in particular this driver has several > compatible > > { .compatible = "fsl,imx53-esdhc", }, > { .compatible = "fsl,imx6ul-usdhc", }, > { .compatible = "fsl,imx6sx-usdhc", }, > { .compatible = "fsl,imx6sl-usdhc", }, > { .compatible = "fsl,imx6q-usdhc", }, > { .compatible = "fsl,imx7d-usdhc", .data = > (ulong)&usdhc_imx7d_data,}, > { .compatible = "fsl,imx7ulp-usdhc", }, > { .compatible = "fsl,imx8qm-usdhc", .data = > (ulong)&usdhc_imx8qm_data,}, > { .compatible = "fsl,imx8mm-usdhc", .data = > (ulong)&usdhc_imx8qm_data,}, > { .compatible = "fsl,imx8mn-usdhc", .data = > (ulong)&usdhc_imx8qm_data,}, > { .compatible = "fsl,imx8mq-usdhc", .data = > (ulong)&usdhc_imx8qm_data,}, > { .compatible = "fsl,imxrt-usdhc", }, > > and in order to create a general solution, we need to search for the > compatible string instead of matching for driver name. > > Could you please elaborate a bit more your suggestion in order to > understand your approach. > > Thanks in advance. I am wondering if we can use the DM_GET_DRIVER() macro somehow in dt_platdata.c. At present we emit a string, and that string matches the driver name, so we should be able to. That will give a compile error if something is wrong, much better than the current behaviour of not being able to bind the driver at runtime. This is just to improve the current impl, not to do what you are asking here. For what you want, our current approach is to use the first compatible string to find the driver name. Since all compatible strings point to the same driver, perhaps that is good enough? We should at least understand the limitations before going further. The main one I am aware of is that you need to duplicate the U_BOOT_DRIVER()xxx for each compatible string. To fix that we could add: U_BOOT_DRIVER_ALIAS(xxx, new_name) which creates a linker list symbol that points to the original driver, perhaps using ll_entry_get(). That should be easy enough I think. Then whichever symbol you use you will get the same driver since all the symbols point to it. Unfortunately the .data field is not available with of_platdata. That could be added to the dtd_... struct automatically by dtoc, I suspect. However that requires some clever parsing of the C code... As you can tell I would really like to avoid string comparisons and tables of compatible strings in the image itself. It adds overheade. Regards, Simon
Hi Simon, On 9/4/20 16:36, Simon Glass wrote: > Hi Walter, > > On Thu, 9 Apr 2020 at 13:07, Walter Lozano <walter.lozano@collabora.com> wrote: >> Hi Simon, >> >> On 8/4/20 00:14, Simon Glass wrote: >>> Hi Walter, >>> >>> On Tue, 7 Apr 2020 at 14:05, Walter Lozano <walter.lozano@collabora.com> wrote: >>>> Hi Simon, >>>> >>>> Thank you for taking the time to review this series. >>>> >>>> On 6/4/20 00:42, Simon Glass wrote: >>>>> Hi Walter, >>>>> >>>>> On Sun, 29 Mar 2020 at 21:32, Walter Lozano<walter.lozano@collabora.com> wrote: >>>>>> Signed-off-by: Walter Lozano<walter.lozano@collabora.com> >>>>>> --- >>>>>> drivers/mmc/fsl_esdhc_imx.c | 46 +++++++++++++++++++++++++++++++++---- >>>>>> 1 file changed, 42 insertions(+), 4 deletions(-) >>>>>> >>>>>> diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c >>>>>> index 4900498e9b..761a4b46e9 100644 >>>>>> --- a/drivers/mmc/fsl_esdhc_imx.c >>>>>> +++ b/drivers/mmc/fsl_esdhc_imx.c >>>>>> @@ -29,6 +29,8 @@ >>>>>> #include <dm.h> >>>>>> #include <asm-generic/gpio.h> >>>>>> #include <dm/pinctrl.h> >>>>>> +#include <dt-structs.h> >>>>>> +#include <mapmem.h> >>>>>> >>>>>> #if !CONFIG_IS_ENABLED(BLK) >>>>>> #include "mmc_private.h" >>>>>> @@ -98,6 +100,11 @@ struct fsl_esdhc { >>>>>> }; >>>>>> >>>>>> struct fsl_esdhc_plat { >>>>>> +#if CONFIG_IS_ENABLED(OF_PLATDATA) >>>>>> + /* Put this first since driver model will copy the data here */ >>>>>> + struct dtd_fsl_imx6q_usdhc dtplat; >>>>>> +#endif >>>>>> + >>>>>> struct mmc_config cfg; >>>>>> struct mmc mmc; >>>>>> }; >>>>>> @@ -1377,14 +1384,18 @@ static int fsl_esdhc_probe(struct udevice *dev) >>>>>> struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev); >>>>>> struct fsl_esdhc_plat *plat = dev_get_platdata(dev); >>>>>> struct fsl_esdhc_priv *priv = dev_get_priv(dev); >>>>>> +#if !CONFIG_IS_ENABLED(OF_PLATDATA) >>>>>> const void *fdt = gd->fdt_blob; >>>>>> int node = dev_of_offset(dev); >>>>>> + fdt_addr_t addr; >>>>>> +#else >>>>>> + struct dtd_fsl_imx6q_usdhc *dtplat = &plat->dtplat; >>>>>> +#endif >>>>>> struct esdhc_soc_data *data = >>>>>> (struct esdhc_soc_data *)dev_get_driver_data(dev); >>>>>> #if CONFIG_IS_ENABLED(DM_REGULATOR) >>>>>> struct udevice *vqmmc_dev; >>>>>> #endif >>>>>> - fdt_addr_t addr; >>>>>> unsigned int val; >>>>>> struct mmc *mmc; >>>>>> #if !CONFIG_IS_ENABLED(BLK) >>>>>> @@ -1392,14 +1403,23 @@ static int fsl_esdhc_probe(struct udevice *dev) >>>>>> #endif >>>>>> int ret; >>>>>> >>>>>> +#if CONFIG_IS_ENABLED(OF_PLATDATA) >>>>>> + priv->esdhc_regs = map_sysmem(dtplat->reg[0], dtplat->reg[1]); >>>>>> + val = plat->dtplat.bus_width; >>>>>> + if (val == 8) >>>>>> + priv->bus_width = 8; >>>>>> + else if (val == 4) >>>>>> + priv->bus_width = 4; >>>>>> + else >>>>>> + priv->bus_width = 1; >>>>>> + priv->non_removable = 1; >>>>>> +#else >>>>>> addr = dev_read_addr(dev); >>>>>> if (addr == FDT_ADDR_T_NONE) >>>>>> return -EINVAL; >>>>>> priv->esdhc_regs = (struct fsl_esdhc *)addr; >>>>>> priv->dev = dev; >>>>>> priv->mode = -1; >>>>>> - if (data) >>>>>> - priv->flags = data->flags; >>>>>> >>>>>> val = dev_read_u32_default(dev, "bus-width", -1); >>>>>> if (val == 8) >>>>>> @@ -1462,7 +1482,9 @@ static int fsl_esdhc_probe(struct udevice *dev) >>>>>> priv->vs18_enable = 1; >>>>>> } >>>>>> #endif >>>>>> - >>>>>> +#endif >>>>>> + if (data) >>>>>> + priv->flags = data->flags; >>>>>> /* >>>>>> * TODO: >>>>>> * Because lack of clk driver, if SDHC clk is not enabled, >>>>>> @@ -1513,9 +1535,11 @@ static int fsl_esdhc_probe(struct udevice *dev) >>>>>> return ret; >>>>>> } >>>>>> >>>>>> +#if !CONFIG_IS_ENABLED(OF_PLATDATA) >>>>> Maybe can use if() for this one? >>>> Thank you for the suggestion >>>> >>>>>> ret = mmc_of_parse(dev, &plat->cfg); >>>>>> if (ret) >>>>>> return ret; >>>>>> +#endif >>>>>> >>>>>> mmc = &plat->mmc; >>>>>> mmc->cfg = &plat->cfg; >>>>>> @@ -1648,4 +1672,18 @@ U_BOOT_DRIVER(fsl_esdhc) = { >>>>>> .platdata_auto_alloc_size = sizeof(struct fsl_esdhc_plat), >>>>>> .priv_auto_alloc_size = sizeof(struct fsl_esdhc_priv), >>>>>> }; >>>>>> + >>>>>> +#if CONFIG_IS_ENABLED(OF_PLATDATA) >>>>> Don't you already have a U_BOOT_DRIVER declaration? >>>>> >>>>> You may need to change the name of your existing driver though (see >>>>> of-platdata docs). >>>>> >>>>> So if it is because of that, please add a comment. >>>> I have my doubts regarding this issue. As I see, this driver is used by >>>> many different DT with different compatible strings, so I'm thinking in >>>> trying to find a more generic approach. Would it be useful to have a >>>> more elaborated solution searching for the compatible string when >>>> matching drivers with device? >>> Yes I think so. >>> >>> Actually searching for a string is not great anyway. I wonder if we >>> can use the linker-list idea in the previous email somehow? >> >> I'm sure I' understand the idea you try to share with me. Sorry, I >> understand that one limitation of the current implementation of >> OF_PLATDATA is the fact that the U_BOOT_DRIVER name should match the one >> used as first entry in DT. As in particular this driver has several >> compatible >> >> { .compatible = "fsl,imx53-esdhc", }, >> { .compatible = "fsl,imx6ul-usdhc", }, >> { .compatible = "fsl,imx6sx-usdhc", }, >> { .compatible = "fsl,imx6sl-usdhc", }, >> { .compatible = "fsl,imx6q-usdhc", }, >> { .compatible = "fsl,imx7d-usdhc", .data = >> (ulong)&usdhc_imx7d_data,}, >> { .compatible = "fsl,imx7ulp-usdhc", }, >> { .compatible = "fsl,imx8qm-usdhc", .data = >> (ulong)&usdhc_imx8qm_data,}, >> { .compatible = "fsl,imx8mm-usdhc", .data = >> (ulong)&usdhc_imx8qm_data,}, >> { .compatible = "fsl,imx8mn-usdhc", .data = >> (ulong)&usdhc_imx8qm_data,}, >> { .compatible = "fsl,imx8mq-usdhc", .data = >> (ulong)&usdhc_imx8qm_data,}, >> { .compatible = "fsl,imxrt-usdhc", }, >> >> and in order to create a general solution, we need to search for the >> compatible string instead of matching for driver name. >> >> Could you please elaborate a bit more your suggestion in order to >> understand your approach. >> >> Thanks in advance. > I am wondering if we can use the DM_GET_DRIVER() macro somehow in > dt_platdata.c. At present we emit a string, and that string matches > the driver name, so we should be able to. That will give a compile > error if something is wrong, much better than the current behaviour of > not being able to bind the driver at runtime. > > This is just to improve the current impl, not to do what you are asking here. > > For what you want, our current approach is to use the first compatible > string to find the driver name. Since all compatible strings point to > the same driver, perhaps that is good enough? We should at least > understand the limitations before going further. > > The main one I am aware of is that you need to duplicate the > U_BOOT_DRIVER()xxx for each compatible string. To fix that we could > add: > > U_BOOT_DRIVER_ALIAS(xxx, new_name) > > which creates a linker list symbol that points to the original driver, > perhaps using ll_entry_get(). That should be easy enough I think. Then > whichever symbol you use you will get the same driver since all the > symbols point to it. > > Unfortunately the .data field is not available with of_platdata. That > could be added to the dtd_... struct automatically by dtoc, I suspect. > However that requires some clever parsing of the C code... > > As you can tell I would really like to avoid string comparisons and > tables of compatible strings in the image itself. It adds overheade. Thanks for taking the time to elaborate your idea, now is clear. I totally agree with you, the whole idea behind it to reduce the image size, so we need to work to avoid any kind of table of strings. I will investigate you approach and come back to you. Regards, Walter
Hi Walter, On Thu, 9 Apr 2020 at 13:44, Walter Lozano <walter.lozano@collabora.com> wrote: > > Hi Simon, > > On 9/4/20 16:36, Simon Glass wrote: > > Hi Walter, > > > > On Thu, 9 Apr 2020 at 13:07, Walter Lozano <walter.lozano@collabora.com> wrote: > >> Hi Simon, > >> > >> On 8/4/20 00:14, Simon Glass wrote: > >>> Hi Walter, > >>> > >>> On Tue, 7 Apr 2020 at 14:05, Walter Lozano <walter.lozano@collabora.com> wrote: > >>>> Hi Simon, > >>>> > >>>> Thank you for taking the time to review this series. > >>>> > >>>> On 6/4/20 00:42, Simon Glass wrote: > >>>>> Hi Walter, > >>>>> > >>>>> On Sun, 29 Mar 2020 at 21:32, Walter Lozano<walter.lozano@collabora.com> wrote: > >>>>>> Signed-off-by: Walter Lozano<walter.lozano@collabora.com> > >>>>>> --- > >>>>>> drivers/mmc/fsl_esdhc_imx.c | 46 +++++++++++++++++++++++++++++++++---- > >>>>>> 1 file changed, 42 insertions(+), 4 deletions(-) > >>>>>> > >>>>>> diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c > >>>>>> index 4900498e9b..761a4b46e9 100644 > >>>>>> --- a/drivers/mmc/fsl_esdhc_imx.c > >>>>>> +++ b/drivers/mmc/fsl_esdhc_imx.c > >>>>>> @@ -29,6 +29,8 @@ > >>>>>> #include <dm.h> > >>>>>> #include <asm-generic/gpio.h> > >>>>>> #include <dm/pinctrl.h> > >>>>>> +#include <dt-structs.h> > >>>>>> +#include <mapmem.h> > >>>>>> > >>>>>> #if !CONFIG_IS_ENABLED(BLK) > >>>>>> #include "mmc_private.h" > >>>>>> @@ -98,6 +100,11 @@ struct fsl_esdhc { > >>>>>> }; > >>>>>> > >>>>>> struct fsl_esdhc_plat { > >>>>>> +#if CONFIG_IS_ENABLED(OF_PLATDATA) > >>>>>> + /* Put this first since driver model will copy the data here */ > >>>>>> + struct dtd_fsl_imx6q_usdhc dtplat; > >>>>>> +#endif > >>>>>> + > >>>>>> struct mmc_config cfg; > >>>>>> struct mmc mmc; > >>>>>> }; > >>>>>> @@ -1377,14 +1384,18 @@ static int fsl_esdhc_probe(struct udevice *dev) > >>>>>> struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev); > >>>>>> struct fsl_esdhc_plat *plat = dev_get_platdata(dev); > >>>>>> struct fsl_esdhc_priv *priv = dev_get_priv(dev); > >>>>>> +#if !CONFIG_IS_ENABLED(OF_PLATDATA) > >>>>>> const void *fdt = gd->fdt_blob; > >>>>>> int node = dev_of_offset(dev); > >>>>>> + fdt_addr_t addr; > >>>>>> +#else > >>>>>> + struct dtd_fsl_imx6q_usdhc *dtplat = &plat->dtplat; > >>>>>> +#endif > >>>>>> struct esdhc_soc_data *data = > >>>>>> (struct esdhc_soc_data *)dev_get_driver_data(dev); > >>>>>> #if CONFIG_IS_ENABLED(DM_REGULATOR) > >>>>>> struct udevice *vqmmc_dev; > >>>>>> #endif > >>>>>> - fdt_addr_t addr; > >>>>>> unsigned int val; > >>>>>> struct mmc *mmc; > >>>>>> #if !CONFIG_IS_ENABLED(BLK) > >>>>>> @@ -1392,14 +1403,23 @@ static int fsl_esdhc_probe(struct udevice *dev) > >>>>>> #endif > >>>>>> int ret; > >>>>>> > >>>>>> +#if CONFIG_IS_ENABLED(OF_PLATDATA) > >>>>>> + priv->esdhc_regs = map_sysmem(dtplat->reg[0], dtplat->reg[1]); > >>>>>> + val = plat->dtplat.bus_width; > >>>>>> + if (val == 8) > >>>>>> + priv->bus_width = 8; > >>>>>> + else if (val == 4) > >>>>>> + priv->bus_width = 4; > >>>>>> + else > >>>>>> + priv->bus_width = 1; > >>>>>> + priv->non_removable = 1; > >>>>>> +#else > >>>>>> addr = dev_read_addr(dev); > >>>>>> if (addr == FDT_ADDR_T_NONE) > >>>>>> return -EINVAL; > >>>>>> priv->esdhc_regs = (struct fsl_esdhc *)addr; > >>>>>> priv->dev = dev; > >>>>>> priv->mode = -1; > >>>>>> - if (data) > >>>>>> - priv->flags = data->flags; > >>>>>> > >>>>>> val = dev_read_u32_default(dev, "bus-width", -1); > >>>>>> if (val == 8) > >>>>>> @@ -1462,7 +1482,9 @@ static int fsl_esdhc_probe(struct udevice *dev) > >>>>>> priv->vs18_enable = 1; > >>>>>> } > >>>>>> #endif > >>>>>> - > >>>>>> +#endif > >>>>>> + if (data) > >>>>>> + priv->flags = data->flags; > >>>>>> /* > >>>>>> * TODO: > >>>>>> * Because lack of clk driver, if SDHC clk is not enabled, > >>>>>> @@ -1513,9 +1535,11 @@ static int fsl_esdhc_probe(struct udevice *dev) > >>>>>> return ret; > >>>>>> } > >>>>>> > >>>>>> +#if !CONFIG_IS_ENABLED(OF_PLATDATA) > >>>>> Maybe can use if() for this one? > >>>> Thank you for the suggestion > >>>> > >>>>>> ret = mmc_of_parse(dev, &plat->cfg); > >>>>>> if (ret) > >>>>>> return ret; > >>>>>> +#endif > >>>>>> > >>>>>> mmc = &plat->mmc; > >>>>>> mmc->cfg = &plat->cfg; > >>>>>> @@ -1648,4 +1672,18 @@ U_BOOT_DRIVER(fsl_esdhc) = { > >>>>>> .platdata_auto_alloc_size = sizeof(struct fsl_esdhc_plat), > >>>>>> .priv_auto_alloc_size = sizeof(struct fsl_esdhc_priv), > >>>>>> }; > >>>>>> + > >>>>>> +#if CONFIG_IS_ENABLED(OF_PLATDATA) > >>>>> Don't you already have a U_BOOT_DRIVER declaration? > >>>>> > >>>>> You may need to change the name of your existing driver though (see > >>>>> of-platdata docs). > >>>>> > >>>>> So if it is because of that, please add a comment. > >>>> I have my doubts regarding this issue. As I see, this driver is used by > >>>> many different DT with different compatible strings, so I'm thinking in > >>>> trying to find a more generic approach. Would it be useful to have a > >>>> more elaborated solution searching for the compatible string when > >>>> matching drivers with device? > >>> Yes I think so. > >>> > >>> Actually searching for a string is not great anyway. I wonder if we > >>> can use the linker-list idea in the previous email somehow? > >> > >> I'm sure I' understand the idea you try to share with me. Sorry, I > >> understand that one limitation of the current implementation of > >> OF_PLATDATA is the fact that the U_BOOT_DRIVER name should match the one > >> used as first entry in DT. As in particular this driver has several > >> compatible > >> > >> { .compatible = "fsl,imx53-esdhc", }, > >> { .compatible = "fsl,imx6ul-usdhc", }, > >> { .compatible = "fsl,imx6sx-usdhc", }, > >> { .compatible = "fsl,imx6sl-usdhc", }, > >> { .compatible = "fsl,imx6q-usdhc", }, > >> { .compatible = "fsl,imx7d-usdhc", .data = > >> (ulong)&usdhc_imx7d_data,}, > >> { .compatible = "fsl,imx7ulp-usdhc", }, > >> { .compatible = "fsl,imx8qm-usdhc", .data = > >> (ulong)&usdhc_imx8qm_data,}, > >> { .compatible = "fsl,imx8mm-usdhc", .data = > >> (ulong)&usdhc_imx8qm_data,}, > >> { .compatible = "fsl,imx8mn-usdhc", .data = > >> (ulong)&usdhc_imx8qm_data,}, > >> { .compatible = "fsl,imx8mq-usdhc", .data = > >> (ulong)&usdhc_imx8qm_data,}, > >> { .compatible = "fsl,imxrt-usdhc", }, > >> > >> and in order to create a general solution, we need to search for the > >> compatible string instead of matching for driver name. > >> > >> Could you please elaborate a bit more your suggestion in order to > >> understand your approach. > >> > >> Thanks in advance. > > I am wondering if we can use the DM_GET_DRIVER() macro somehow in > > dt_platdata.c. At present we emit a string, and that string matches > > the driver name, so we should be able to. That will give a compile > > error if something is wrong, much better than the current behaviour of > > not being able to bind the driver at runtime. > > > > This is just to improve the current impl, not to do what you are asking here. > > > > For what you want, our current approach is to use the first compatible > > string to find the driver name. Since all compatible strings point to > > the same driver, perhaps that is good enough? We should at least > > understand the limitations before going further. > > > > The main one I am aware of is that you need to duplicate the > > U_BOOT_DRIVER()xxx for each compatible string. To fix that we could > > add: > > > > U_BOOT_DRIVER_ALIAS(xxx, new_name) > > > > which creates a linker list symbol that points to the original driver, > > perhaps using ll_entry_get(). That should be easy enough I think. Then > > whichever symbol you use you will get the same driver since all the > > symbols point to it. > > > > Unfortunately the .data field is not available with of_platdata. That > > could be added to the dtd_... struct automatically by dtoc, I suspect. > > However that requires some clever parsing of the C code... > > > > As you can tell I would really like to avoid string comparisons and > > tables of compatible strings in the image itself. It adds overheade. > > > Thanks for taking the time to elaborate your idea, now is clear. I > totally agree with you, the whole idea behind it to reduce the image > size, so we need to work to avoid any kind of table of strings. > > > I will investigate you approach and come back to you. OK sounds good. I should mention that the dtoc tool should be upstreamed to dtc. I was thinking of sending something very simple to start. Regards, SImon
On 9/4/20 16:50, Simon Glass wrote: > Hi Walter, > > On Thu, 9 Apr 2020 at 13:44, Walter Lozano <walter.lozano@collabora.com> wrote: >> Hi Simon, >> >> On 9/4/20 16:36, Simon Glass wrote: >>> Hi Walter, >>> >>> On Thu, 9 Apr 2020 at 13:07, Walter Lozano <walter.lozano@collabora.com> wrote: >>>> Hi Simon, >>>> >>>> On 8/4/20 00:14, Simon Glass wrote: >>>>> Hi Walter, >>>>> >>>>> On Tue, 7 Apr 2020 at 14:05, Walter Lozano <walter.lozano@collabora.com> wrote: >>>>>> Hi Simon, >>>>>> >>>>>> Thank you for taking the time to review this series. >>>>>> >>>>>> On 6/4/20 00:42, Simon Glass wrote: >>>>>>> Hi Walter, >>>>>>> >>>>>>> On Sun, 29 Mar 2020 at 21:32, Walter Lozano<walter.lozano@collabora.com> wrote: >>>>>>>> Signed-off-by: Walter Lozano<walter.lozano@collabora.com> >>>>>>>> --- >>>>>>>> drivers/mmc/fsl_esdhc_imx.c | 46 +++++++++++++++++++++++++++++++++---- >>>>>>>> 1 file changed, 42 insertions(+), 4 deletions(-) >>>>>>>> >>>>>>>> diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c >>>>>>>> index 4900498e9b..761a4b46e9 100644 >>>>>>>> --- a/drivers/mmc/fsl_esdhc_imx.c >>>>>>>> +++ b/drivers/mmc/fsl_esdhc_imx.c >>>>>>>> @@ -29,6 +29,8 @@ >>>>>>>> #include <dm.h> >>>>>>>> #include <asm-generic/gpio.h> >>>>>>>> #include <dm/pinctrl.h> >>>>>>>> +#include <dt-structs.h> >>>>>>>> +#include <mapmem.h> >>>>>>>> >>>>>>>> #if !CONFIG_IS_ENABLED(BLK) >>>>>>>> #include "mmc_private.h" >>>>>>>> @@ -98,6 +100,11 @@ struct fsl_esdhc { >>>>>>>> }; >>>>>>>> >>>>>>>> struct fsl_esdhc_plat { >>>>>>>> +#if CONFIG_IS_ENABLED(OF_PLATDATA) >>>>>>>> + /* Put this first since driver model will copy the data here */ >>>>>>>> + struct dtd_fsl_imx6q_usdhc dtplat; >>>>>>>> +#endif >>>>>>>> + >>>>>>>> struct mmc_config cfg; >>>>>>>> struct mmc mmc; >>>>>>>> }; >>>>>>>> @@ -1377,14 +1384,18 @@ static int fsl_esdhc_probe(struct udevice *dev) >>>>>>>> struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev); >>>>>>>> struct fsl_esdhc_plat *plat = dev_get_platdata(dev); >>>>>>>> struct fsl_esdhc_priv *priv = dev_get_priv(dev); >>>>>>>> +#if !CONFIG_IS_ENABLED(OF_PLATDATA) >>>>>>>> const void *fdt = gd->fdt_blob; >>>>>>>> int node = dev_of_offset(dev); >>>>>>>> + fdt_addr_t addr; >>>>>>>> +#else >>>>>>>> + struct dtd_fsl_imx6q_usdhc *dtplat = &plat->dtplat; >>>>>>>> +#endif >>>>>>>> struct esdhc_soc_data *data = >>>>>>>> (struct esdhc_soc_data *)dev_get_driver_data(dev); >>>>>>>> #if CONFIG_IS_ENABLED(DM_REGULATOR) >>>>>>>> struct udevice *vqmmc_dev; >>>>>>>> #endif >>>>>>>> - fdt_addr_t addr; >>>>>>>> unsigned int val; >>>>>>>> struct mmc *mmc; >>>>>>>> #if !CONFIG_IS_ENABLED(BLK) >>>>>>>> @@ -1392,14 +1403,23 @@ static int fsl_esdhc_probe(struct udevice *dev) >>>>>>>> #endif >>>>>>>> int ret; >>>>>>>> >>>>>>>> +#if CONFIG_IS_ENABLED(OF_PLATDATA) >>>>>>>> + priv->esdhc_regs = map_sysmem(dtplat->reg[0], dtplat->reg[1]); >>>>>>>> + val = plat->dtplat.bus_width; >>>>>>>> + if (val == 8) >>>>>>>> + priv->bus_width = 8; >>>>>>>> + else if (val == 4) >>>>>>>> + priv->bus_width = 4; >>>>>>>> + else >>>>>>>> + priv->bus_width = 1; >>>>>>>> + priv->non_removable = 1; >>>>>>>> +#else >>>>>>>> addr = dev_read_addr(dev); >>>>>>>> if (addr == FDT_ADDR_T_NONE) >>>>>>>> return -EINVAL; >>>>>>>> priv->esdhc_regs = (struct fsl_esdhc *)addr; >>>>>>>> priv->dev = dev; >>>>>>>> priv->mode = -1; >>>>>>>> - if (data) >>>>>>>> - priv->flags = data->flags; >>>>>>>> >>>>>>>> val = dev_read_u32_default(dev, "bus-width", -1); >>>>>>>> if (val == 8) >>>>>>>> @@ -1462,7 +1482,9 @@ static int fsl_esdhc_probe(struct udevice *dev) >>>>>>>> priv->vs18_enable = 1; >>>>>>>> } >>>>>>>> #endif >>>>>>>> - >>>>>>>> +#endif >>>>>>>> + if (data) >>>>>>>> + priv->flags = data->flags; >>>>>>>> /* >>>>>>>> * TODO: >>>>>>>> * Because lack of clk driver, if SDHC clk is not enabled, >>>>>>>> @@ -1513,9 +1535,11 @@ static int fsl_esdhc_probe(struct udevice *dev) >>>>>>>> return ret; >>>>>>>> } >>>>>>>> >>>>>>>> +#if !CONFIG_IS_ENABLED(OF_PLATDATA) >>>>>>> Maybe can use if() for this one? >>>>>> Thank you for the suggestion >>>>>> >>>>>>>> ret = mmc_of_parse(dev, &plat->cfg); >>>>>>>> if (ret) >>>>>>>> return ret; >>>>>>>> +#endif >>>>>>>> >>>>>>>> mmc = &plat->mmc; >>>>>>>> mmc->cfg = &plat->cfg; >>>>>>>> @@ -1648,4 +1672,18 @@ U_BOOT_DRIVER(fsl_esdhc) = { >>>>>>>> .platdata_auto_alloc_size = sizeof(struct fsl_esdhc_plat), >>>>>>>> .priv_auto_alloc_size = sizeof(struct fsl_esdhc_priv), >>>>>>>> }; >>>>>>>> + >>>>>>>> +#if CONFIG_IS_ENABLED(OF_PLATDATA) >>>>>>> Don't you already have a U_BOOT_DRIVER declaration? >>>>>>> >>>>>>> You may need to change the name of your existing driver though (see >>>>>>> of-platdata docs). >>>>>>> >>>>>>> So if it is because of that, please add a comment. >>>>>> I have my doubts regarding this issue. As I see, this driver is used by >>>>>> many different DT with different compatible strings, so I'm thinking in >>>>>> trying to find a more generic approach. Would it be useful to have a >>>>>> more elaborated solution searching for the compatible string when >>>>>> matching drivers with device? >>>>> Yes I think so. >>>>> >>>>> Actually searching for a string is not great anyway. I wonder if we >>>>> can use the linker-list idea in the previous email somehow? >>>> I'm sure I' understand the idea you try to share with me. Sorry, I >>>> understand that one limitation of the current implementation of >>>> OF_PLATDATA is the fact that the U_BOOT_DRIVER name should match the one >>>> used as first entry in DT. As in particular this driver has several >>>> compatible >>>> >>>> { .compatible = "fsl,imx53-esdhc", }, >>>> { .compatible = "fsl,imx6ul-usdhc", }, >>>> { .compatible = "fsl,imx6sx-usdhc", }, >>>> { .compatible = "fsl,imx6sl-usdhc", }, >>>> { .compatible = "fsl,imx6q-usdhc", }, >>>> { .compatible = "fsl,imx7d-usdhc", .data = >>>> (ulong)&usdhc_imx7d_data,}, >>>> { .compatible = "fsl,imx7ulp-usdhc", }, >>>> { .compatible = "fsl,imx8qm-usdhc", .data = >>>> (ulong)&usdhc_imx8qm_data,}, >>>> { .compatible = "fsl,imx8mm-usdhc", .data = >>>> (ulong)&usdhc_imx8qm_data,}, >>>> { .compatible = "fsl,imx8mn-usdhc", .data = >>>> (ulong)&usdhc_imx8qm_data,}, >>>> { .compatible = "fsl,imx8mq-usdhc", .data = >>>> (ulong)&usdhc_imx8qm_data,}, >>>> { .compatible = "fsl,imxrt-usdhc", }, >>>> >>>> and in order to create a general solution, we need to search for the >>>> compatible string instead of matching for driver name. >>>> >>>> Could you please elaborate a bit more your suggestion in order to >>>> understand your approach. >>>> >>>> Thanks in advance. >>> I am wondering if we can use the DM_GET_DRIVER() macro somehow in >>> dt_platdata.c. At present we emit a string, and that string matches >>> the driver name, so we should be able to. That will give a compile >>> error if something is wrong, much better than the current behaviour of >>> not being able to bind the driver at runtime. >>> >>> This is just to improve the current impl, not to do what you are asking here. >>> >>> For what you want, our current approach is to use the first compatible >>> string to find the driver name. Since all compatible strings point to >>> the same driver, perhaps that is good enough? We should at least >>> understand the limitations before going further. >>> >>> The main one I am aware of is that you need to duplicate the >>> U_BOOT_DRIVER()xxx for each compatible string. To fix that we could >>> add: >>> >>> U_BOOT_DRIVER_ALIAS(xxx, new_name) >>> >>> which creates a linker list symbol that points to the original driver, >>> perhaps using ll_entry_get(). That should be easy enough I think. Then >>> whichever symbol you use you will get the same driver since all the >>> symbols point to it. >>> >>> Unfortunately the .data field is not available with of_platdata. That >>> could be added to the dtd_... struct automatically by dtoc, I suspect. >>> However that requires some clever parsing of the C code... >>> >>> As you can tell I would really like to avoid string comparisons and >>> tables of compatible strings in the image itself. It adds overheade. >> >> Thanks for taking the time to elaborate your idea, now is clear. I >> totally agree with you, the whole idea behind it to reduce the image >> size, so we need to work to avoid any kind of table of strings. >> >> >> I will investigate you approach and come back to you. > OK sounds good. I should mention that the dtoc tool should be > upstreamed to dtc. I was thinking of sending something very simple to > start. OK, Iet's do it step by step, I hope to schedule some time for this series next week. > Regards, > SImon Regards, Walter
Hi Simon, On 9/4/20 16:50, Simon Glass wrote: > Hi Walter, > > On Thu, 9 Apr 2020 at 13:44, Walter Lozano <walter.lozano@collabora.com> wrote: >> Hi Simon, >> >> On 9/4/20 16:36, Simon Glass wrote: >>> Hi Walter, >>> >>> On Thu, 9 Apr 2020 at 13:07, Walter Lozano <walter.lozano@collabora.com> wrote: >>>> Hi Simon, >>>> >>>> On 8/4/20 00:14, Simon Glass wrote: >>>>> Hi Walter, >>>>> >>>>> On Tue, 7 Apr 2020 at 14:05, Walter Lozano <walter.lozano@collabora.com> wrote: >>>>>> Hi Simon, >>>>>> >>>>>> Thank you for taking the time to review this series. >>>>>> >>>>>> On 6/4/20 00:42, Simon Glass wrote: >>>>>>> Hi Walter, >>>>>>> >>>>>>> On Sun, 29 Mar 2020 at 21:32, Walter Lozano<walter.lozano@collabora.com> wrote: >>>>>>>> Signed-off-by: Walter Lozano<walter.lozano@collabora.com> >>>>>>>> --- >>>>>>>> drivers/mmc/fsl_esdhc_imx.c | 46 +++++++++++++++++++++++++++++++++---- >>>>>>>> 1 file changed, 42 insertions(+), 4 deletions(-) >>>>>>>> >>>>>>>> diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c >>>>>>>> index 4900498e9b..761a4b46e9 100644 >>>>>>>> --- a/drivers/mmc/fsl_esdhc_imx.c >>>>>>>> +++ b/drivers/mmc/fsl_esdhc_imx.c >>>>>>>> @@ -29,6 +29,8 @@ >>>>>>>> #include <dm.h> >>>>>>>> #include <asm-generic/gpio.h> >>>>>>>> #include <dm/pinctrl.h> >>>>>>>> +#include <dt-structs.h> >>>>>>>> +#include <mapmem.h> >>>>>>>> >>>>>>>> #if !CONFIG_IS_ENABLED(BLK) >>>>>>>> #include "mmc_private.h" >>>>>>>> @@ -98,6 +100,11 @@ struct fsl_esdhc { >>>>>>>> }; >>>>>>>> >>>>>>>> struct fsl_esdhc_plat { >>>>>>>> +#if CONFIG_IS_ENABLED(OF_PLATDATA) >>>>>>>> + /* Put this first since driver model will copy the data here */ >>>>>>>> + struct dtd_fsl_imx6q_usdhc dtplat; >>>>>>>> +#endif >>>>>>>> + >>>>>>>> struct mmc_config cfg; >>>>>>>> struct mmc mmc; >>>>>>>> }; >>>>>>>> @@ -1377,14 +1384,18 @@ static int fsl_esdhc_probe(struct udevice *dev) >>>>>>>> struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev); >>>>>>>> struct fsl_esdhc_plat *plat = dev_get_platdata(dev); >>>>>>>> struct fsl_esdhc_priv *priv = dev_get_priv(dev); >>>>>>>> +#if !CONFIG_IS_ENABLED(OF_PLATDATA) >>>>>>>> const void *fdt = gd->fdt_blob; >>>>>>>> int node = dev_of_offset(dev); >>>>>>>> + fdt_addr_t addr; >>>>>>>> +#else >>>>>>>> + struct dtd_fsl_imx6q_usdhc *dtplat = &plat->dtplat; >>>>>>>> +#endif >>>>>>>> struct esdhc_soc_data *data = >>>>>>>> (struct esdhc_soc_data *)dev_get_driver_data(dev); >>>>>>>> #if CONFIG_IS_ENABLED(DM_REGULATOR) >>>>>>>> struct udevice *vqmmc_dev; >>>>>>>> #endif >>>>>>>> - fdt_addr_t addr; >>>>>>>> unsigned int val; >>>>>>>> struct mmc *mmc; >>>>>>>> #if !CONFIG_IS_ENABLED(BLK) >>>>>>>> @@ -1392,14 +1403,23 @@ static int fsl_esdhc_probe(struct udevice *dev) >>>>>>>> #endif >>>>>>>> int ret; >>>>>>>> >>>>>>>> +#if CONFIG_IS_ENABLED(OF_PLATDATA) >>>>>>>> + priv->esdhc_regs = map_sysmem(dtplat->reg[0], dtplat->reg[1]); >>>>>>>> + val = plat->dtplat.bus_width; >>>>>>>> + if (val == 8) >>>>>>>> + priv->bus_width = 8; >>>>>>>> + else if (val == 4) >>>>>>>> + priv->bus_width = 4; >>>>>>>> + else >>>>>>>> + priv->bus_width = 1; >>>>>>>> + priv->non_removable = 1; >>>>>>>> +#else >>>>>>>> addr = dev_read_addr(dev); >>>>>>>> if (addr == FDT_ADDR_T_NONE) >>>>>>>> return -EINVAL; >>>>>>>> priv->esdhc_regs = (struct fsl_esdhc *)addr; >>>>>>>> priv->dev = dev; >>>>>>>> priv->mode = -1; >>>>>>>> - if (data) >>>>>>>> - priv->flags = data->flags; >>>>>>>> >>>>>>>> val = dev_read_u32_default(dev, "bus-width", -1); >>>>>>>> if (val == 8) >>>>>>>> @@ -1462,7 +1482,9 @@ static int fsl_esdhc_probe(struct udevice *dev) >>>>>>>> priv->vs18_enable = 1; >>>>>>>> } >>>>>>>> #endif >>>>>>>> - >>>>>>>> +#endif >>>>>>>> + if (data) >>>>>>>> + priv->flags = data->flags; >>>>>>>> /* >>>>>>>> * TODO: >>>>>>>> * Because lack of clk driver, if SDHC clk is not enabled, >>>>>>>> @@ -1513,9 +1535,11 @@ static int fsl_esdhc_probe(struct udevice *dev) >>>>>>>> return ret; >>>>>>>> } >>>>>>>> >>>>>>>> +#if !CONFIG_IS_ENABLED(OF_PLATDATA) >>>>>>> Maybe can use if() for this one? >>>>>> Thank you for the suggestion >>>>>> >>>>>>>> ret = mmc_of_parse(dev, &plat->cfg); >>>>>>>> if (ret) >>>>>>>> return ret; >>>>>>>> +#endif >>>>>>>> >>>>>>>> mmc = &plat->mmc; >>>>>>>> mmc->cfg = &plat->cfg; >>>>>>>> @@ -1648,4 +1672,18 @@ U_BOOT_DRIVER(fsl_esdhc) = { >>>>>>>> .platdata_auto_alloc_size = sizeof(struct fsl_esdhc_plat), >>>>>>>> .priv_auto_alloc_size = sizeof(struct fsl_esdhc_priv), >>>>>>>> }; >>>>>>>> + >>>>>>>> +#if CONFIG_IS_ENABLED(OF_PLATDATA) >>>>>>> Don't you already have a U_BOOT_DRIVER declaration? >>>>>>> >>>>>>> You may need to change the name of your existing driver though (see >>>>>>> of-platdata docs). >>>>>>> >>>>>>> So if it is because of that, please add a comment. >>>>>> I have my doubts regarding this issue. As I see, this driver is used by >>>>>> many different DT with different compatible strings, so I'm thinking in >>>>>> trying to find a more generic approach. Would it be useful to have a >>>>>> more elaborated solution searching for the compatible string when >>>>>> matching drivers with device? >>>>> Yes I think so. >>>>> >>>>> Actually searching for a string is not great anyway. I wonder if we >>>>> can use the linker-list idea in the previous email somehow? >>>> I'm sure I' understand the idea you try to share with me. Sorry, I >>>> understand that one limitation of the current implementation of >>>> OF_PLATDATA is the fact that the U_BOOT_DRIVER name should match the one >>>> used as first entry in DT. As in particular this driver has several >>>> compatible >>>> >>>> { .compatible = "fsl,imx53-esdhc", }, >>>> { .compatible = "fsl,imx6ul-usdhc", }, >>>> { .compatible = "fsl,imx6sx-usdhc", }, >>>> { .compatible = "fsl,imx6sl-usdhc", }, >>>> { .compatible = "fsl,imx6q-usdhc", }, >>>> { .compatible = "fsl,imx7d-usdhc", .data = >>>> (ulong)&usdhc_imx7d_data,}, >>>> { .compatible = "fsl,imx7ulp-usdhc", }, >>>> { .compatible = "fsl,imx8qm-usdhc", .data = >>>> (ulong)&usdhc_imx8qm_data,}, >>>> { .compatible = "fsl,imx8mm-usdhc", .data = >>>> (ulong)&usdhc_imx8qm_data,}, >>>> { .compatible = "fsl,imx8mn-usdhc", .data = >>>> (ulong)&usdhc_imx8qm_data,}, >>>> { .compatible = "fsl,imx8mq-usdhc", .data = >>>> (ulong)&usdhc_imx8qm_data,}, >>>> { .compatible = "fsl,imxrt-usdhc", }, >>>> >>>> and in order to create a general solution, we need to search for the >>>> compatible string instead of matching for driver name. >>>> >>>> Could you please elaborate a bit more your suggestion in order to >>>> understand your approach. >>>> >>>> Thanks in advance. >>> I am wondering if we can use the DM_GET_DRIVER() macro somehow in >>> dt_platdata.c. At present we emit a string, and that string matches >>> the driver name, so we should be able to. That will give a compile >>> error if something is wrong, much better than the current behaviour of >>> not being able to bind the driver at runtime. >>> >>> This is just to improve the current impl, not to do what you are asking here. >>> >>> For what you want, our current approach is to use the first compatible >>> string to find the driver name. Since all compatible strings point to >>> the same driver, perhaps that is good enough? We should at least >>> understand the limitations before going further. >>> >>> The main one I am aware of is that you need to duplicate the >>> U_BOOT_DRIVER()xxx for each compatible string. To fix that we could >>> add: >>> >>> U_BOOT_DRIVER_ALIAS(xxx, new_name) >>> >>> which creates a linker list symbol that points to the original driver, >>> perhaps using ll_entry_get(). That should be easy enough I think. Then >>> whichever symbol you use you will get the same driver since all the >>> symbols point to it. >>> >>> Unfortunately the .data field is not available with of_platdata. That >>> could be added to the dtd_... struct automatically by dtoc, I suspect. >>> However that requires some clever parsing of the C code... >>> >>> As you can tell I would really like to avoid string comparisons and >>> tables of compatible strings in the image itself. It adds overheade. >> >> Thanks for taking the time to elaborate your idea, now is clear. I >> totally agree with you, the whole idea behind it to reduce the image >> size, so we need to work to avoid any kind of table of strings. >> >> >> I will investigate you approach and come back to you. > OK sounds good. I should mention that the dtoc tool should be > upstreamed to dtc. I was thinking of sending something very simple to > start. I have been thinking in your suggestions and the main goal behind all these. With that in mind I think that the best approach we can use is to move as much complexity as we can to dtoc, which will give us - Reduction in TPL/SPL image - Warnings at compile time So I was thinking in add the support for aliases to doc as - Add U_BOOT_DRIVER_ALIAS(name, new_name) in drivers which generate no code for U-Boot - Parse U_BOOT_DRIVER(name) to build a list of drivers in dtoc - Parse U_BOOT_DRIVER_ALIAS(name, new_name) to populate the list of drivers with aliases in dtoc - When parsing dts file, look for the proper driver name based on the list created, is no one is found raise an error I think the parser could be simple, something like (this is only a draft to show the idea) #!/usr/bin/python import os import re class UBootDriverList: def parse_u_boot_driver(self, fn): f = open(fn) b = f.read() drivers = re.findall('U_BOOT_DRIVER\((.*)\)', b) for d in drivers: self.driver_list.append(d) def parse_u_boot_drivers(self): self.driver_list = [] for (dirpath, dirnames, filenames) in os.walk('./'): for fn in filenames: if not fn.endswith('.c'): continue self.parse_u_boot_driver(dirpath + '/' + fn) def print_list(self): for d in self.driver_list: print d driver_list = UBootDriverList() driver_list.parse_u_boot_drivers() driver_list.print_list() What do you think? If this make sense we can try to go also in this way for other improvements. Regards, Walter
Hi Walter, On Fri, 17 Apr 2020 at 14:05, Walter Lozano <walter.lozano@collabora.com> wrote: > > Hi Simon, > > On 9/4/20 16:50, Simon Glass wrote: > > Hi Walter, > > > > On Thu, 9 Apr 2020 at 13:44, Walter Lozano <walter.lozano@collabora.com> wrote: > >> Hi Simon, > >> > >> On 9/4/20 16:36, Simon Glass wrote: > >>> Hi Walter, > >>> > >>> On Thu, 9 Apr 2020 at 13:07, Walter Lozano <walter.lozano@collabora.com> wrote: > >>>> Hi Simon, > >>>> > >>>> On 8/4/20 00:14, Simon Glass wrote: > >>>>> Hi Walter, > >>>>> > >>>>> On Tue, 7 Apr 2020 at 14:05, Walter Lozano <walter.lozano@collabora.com> wrote: > >>>>>> Hi Simon, > >>>>>> > >>>>>> Thank you for taking the time to review this series. > >>>>>> > >>>>>> On 6/4/20 00:42, Simon Glass wrote: > >>>>>>> Hi Walter, > >>>>>>> > >>>>>>> On Sun, 29 Mar 2020 at 21:32, Walter Lozano<walter.lozano@collabora.com> wrote: > >>>>>>>> Signed-off-by: Walter Lozano<walter.lozano@collabora.com> > >>>>>>>> --- > >>>>>>>> drivers/mmc/fsl_esdhc_imx.c | 46 +++++++++++++++++++++++++++++++++---- > >>>>>>>> 1 file changed, 42 insertions(+), 4 deletions(-) > >>>>>>>> > >>>>>>>> diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c > >>>>>>>> index 4900498e9b..761a4b46e9 100644 > >>>>>>>> --- a/drivers/mmc/fsl_esdhc_imx.c > >>>>>>>> +++ b/drivers/mmc/fsl_esdhc_imx.c > >>>>>>>> @@ -29,6 +29,8 @@ > >>>>>>>> #include <dm.h> > >>>>>>>> #include <asm-generic/gpio.h> > >>>>>>>> #include <dm/pinctrl.h> > >>>>>>>> +#include <dt-structs.h> > >>>>>>>> +#include <mapmem.h> > >>>>>>>> > >>>>>>>> #if !CONFIG_IS_ENABLED(BLK) > >>>>>>>> #include "mmc_private.h" > >>>>>>>> @@ -98,6 +100,11 @@ struct fsl_esdhc { > >>>>>>>> }; > >>>>>>>> > >>>>>>>> struct fsl_esdhc_plat { > >>>>>>>> +#if CONFIG_IS_ENABLED(OF_PLATDATA) > >>>>>>>> + /* Put this first since driver model will copy the data here */ > >>>>>>>> + struct dtd_fsl_imx6q_usdhc dtplat; > >>>>>>>> +#endif > >>>>>>>> + > >>>>>>>> struct mmc_config cfg; > >>>>>>>> struct mmc mmc; > >>>>>>>> }; > >>>>>>>> @@ -1377,14 +1384,18 @@ static int fsl_esdhc_probe(struct udevice *dev) > >>>>>>>> struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev); > >>>>>>>> struct fsl_esdhc_plat *plat = dev_get_platdata(dev); > >>>>>>>> struct fsl_esdhc_priv *priv = dev_get_priv(dev); > >>>>>>>> +#if !CONFIG_IS_ENABLED(OF_PLATDATA) > >>>>>>>> const void *fdt = gd->fdt_blob; > >>>>>>>> int node = dev_of_offset(dev); > >>>>>>>> + fdt_addr_t addr; > >>>>>>>> +#else > >>>>>>>> + struct dtd_fsl_imx6q_usdhc *dtplat = &plat->dtplat; > >>>>>>>> +#endif > >>>>>>>> struct esdhc_soc_data *data = > >>>>>>>> (struct esdhc_soc_data *)dev_get_driver_data(dev); > >>>>>>>> #if CONFIG_IS_ENABLED(DM_REGULATOR) > >>>>>>>> struct udevice *vqmmc_dev; > >>>>>>>> #endif > >>>>>>>> - fdt_addr_t addr; > >>>>>>>> unsigned int val; > >>>>>>>> struct mmc *mmc; > >>>>>>>> #if !CONFIG_IS_ENABLED(BLK) > >>>>>>>> @@ -1392,14 +1403,23 @@ static int fsl_esdhc_probe(struct udevice *dev) > >>>>>>>> #endif > >>>>>>>> int ret; > >>>>>>>> > >>>>>>>> +#if CONFIG_IS_ENABLED(OF_PLATDATA) > >>>>>>>> + priv->esdhc_regs = map_sysmem(dtplat->reg[0], dtplat->reg[1]); > >>>>>>>> + val = plat->dtplat.bus_width; > >>>>>>>> + if (val == 8) > >>>>>>>> + priv->bus_width = 8; > >>>>>>>> + else if (val == 4) > >>>>>>>> + priv->bus_width = 4; > >>>>>>>> + else > >>>>>>>> + priv->bus_width = 1; > >>>>>>>> + priv->non_removable = 1; > >>>>>>>> +#else > >>>>>>>> addr = dev_read_addr(dev); > >>>>>>>> if (addr == FDT_ADDR_T_NONE) > >>>>>>>> return -EINVAL; > >>>>>>>> priv->esdhc_regs = (struct fsl_esdhc *)addr; > >>>>>>>> priv->dev = dev; > >>>>>>>> priv->mode = -1; > >>>>>>>> - if (data) > >>>>>>>> - priv->flags = data->flags; > >>>>>>>> > >>>>>>>> val = dev_read_u32_default(dev, "bus-width", -1); > >>>>>>>> if (val == 8) > >>>>>>>> @@ -1462,7 +1482,9 @@ static int fsl_esdhc_probe(struct udevice *dev) > >>>>>>>> priv->vs18_enable = 1; > >>>>>>>> } > >>>>>>>> #endif > >>>>>>>> - > >>>>>>>> +#endif > >>>>>>>> + if (data) > >>>>>>>> + priv->flags = data->flags; > >>>>>>>> /* > >>>>>>>> * TODO: > >>>>>>>> * Because lack of clk driver, if SDHC clk is not enabled, > >>>>>>>> @@ -1513,9 +1535,11 @@ static int fsl_esdhc_probe(struct udevice *dev) > >>>>>>>> return ret; > >>>>>>>> } > >>>>>>>> > >>>>>>>> +#if !CONFIG_IS_ENABLED(OF_PLATDATA) > >>>>>>> Maybe can use if() for this one? > >>>>>> Thank you for the suggestion > >>>>>> > >>>>>>>> ret = mmc_of_parse(dev, &plat->cfg); > >>>>>>>> if (ret) > >>>>>>>> return ret; > >>>>>>>> +#endif > >>>>>>>> > >>>>>>>> mmc = &plat->mmc; > >>>>>>>> mmc->cfg = &plat->cfg; > >>>>>>>> @@ -1648,4 +1672,18 @@ U_BOOT_DRIVER(fsl_esdhc) = { > >>>>>>>> .platdata_auto_alloc_size = sizeof(struct fsl_esdhc_plat), > >>>>>>>> .priv_auto_alloc_size = sizeof(struct fsl_esdhc_priv), > >>>>>>>> }; > >>>>>>>> + > >>>>>>>> +#if CONFIG_IS_ENABLED(OF_PLATDATA) > >>>>>>> Don't you already have a U_BOOT_DRIVER declaration? > >>>>>>> > >>>>>>> You may need to change the name of your existing driver though (see > >>>>>>> of-platdata docs). > >>>>>>> > >>>>>>> So if it is because of that, please add a comment. > >>>>>> I have my doubts regarding this issue. As I see, this driver is used by > >>>>>> many different DT with different compatible strings, so I'm thinking in > >>>>>> trying to find a more generic approach. Would it be useful to have a > >>>>>> more elaborated solution searching for the compatible string when > >>>>>> matching drivers with device? > >>>>> Yes I think so. > >>>>> > >>>>> Actually searching for a string is not great anyway. I wonder if we > >>>>> can use the linker-list idea in the previous email somehow? > >>>> I'm sure I' understand the idea you try to share with me. Sorry, I > >>>> understand that one limitation of the current implementation of > >>>> OF_PLATDATA is the fact that the U_BOOT_DRIVER name should match the one > >>>> used as first entry in DT. As in particular this driver has several > >>>> compatible > >>>> > >>>> { .compatible = "fsl,imx53-esdhc", }, > >>>> { .compatible = "fsl,imx6ul-usdhc", }, > >>>> { .compatible = "fsl,imx6sx-usdhc", }, > >>>> { .compatible = "fsl,imx6sl-usdhc", }, > >>>> { .compatible = "fsl,imx6q-usdhc", }, > >>>> { .compatible = "fsl,imx7d-usdhc", .data = > >>>> (ulong)&usdhc_imx7d_data,}, > >>>> { .compatible = "fsl,imx7ulp-usdhc", }, > >>>> { .compatible = "fsl,imx8qm-usdhc", .data = > >>>> (ulong)&usdhc_imx8qm_data,}, > >>>> { .compatible = "fsl,imx8mm-usdhc", .data = > >>>> (ulong)&usdhc_imx8qm_data,}, > >>>> { .compatible = "fsl,imx8mn-usdhc", .data = > >>>> (ulong)&usdhc_imx8qm_data,}, > >>>> { .compatible = "fsl,imx8mq-usdhc", .data = > >>>> (ulong)&usdhc_imx8qm_data,}, > >>>> { .compatible = "fsl,imxrt-usdhc", }, > >>>> > >>>> and in order to create a general solution, we need to search for the > >>>> compatible string instead of matching for driver name. > >>>> > >>>> Could you please elaborate a bit more your suggestion in order to > >>>> understand your approach. > >>>> > >>>> Thanks in advance. > >>> I am wondering if we can use the DM_GET_DRIVER() macro somehow in > >>> dt_platdata.c. At present we emit a string, and that string matches > >>> the driver name, so we should be able to. That will give a compile > >>> error if something is wrong, much better than the current behaviour of > >>> not being able to bind the driver at runtime. > >>> > >>> This is just to improve the current impl, not to do what you are asking here. > >>> > >>> For what you want, our current approach is to use the first compatible > >>> string to find the driver name. Since all compatible strings point to > >>> the same driver, perhaps that is good enough? We should at least > >>> understand the limitations before going further. > >>> > >>> The main one I am aware of is that you need to duplicate the > >>> U_BOOT_DRIVER()xxx for each compatible string. To fix that we could > >>> add: > >>> > >>> U_BOOT_DRIVER_ALIAS(xxx, new_name) > >>> > >>> which creates a linker list symbol that points to the original driver, > >>> perhaps using ll_entry_get(). That should be easy enough I think. Then > >>> whichever symbol you use you will get the same driver since all the > >>> symbols point to it. > >>> > >>> Unfortunately the .data field is not available with of_platdata. That > >>> could be added to the dtd_... struct automatically by dtoc, I suspect. > >>> However that requires some clever parsing of the C code... > >>> > >>> As you can tell I would really like to avoid string comparisons and > >>> tables of compatible strings in the image itself. It adds overheade. > >> > >> Thanks for taking the time to elaborate your idea, now is clear. I > >> totally agree with you, the whole idea behind it to reduce the image > >> size, so we need to work to avoid any kind of table of strings. > >> > >> > >> I will investigate you approach and come back to you. > > OK sounds good. I should mention that the dtoc tool should be > > upstreamed to dtc. I was thinking of sending something very simple to > > start. > > I have been thinking in your suggestions and the main goal behind all > these. With that in mind I think that the best approach we can use is to > move as much complexity as we can to dtoc, which will give us > > - Reduction in TPL/SPL image > > - Warnings at compile time Good ideas. > > > So I was thinking in add the support for aliases to doc as > > - Add U_BOOT_DRIVER_ALIAS(name, new_name) in drivers which generate no > code for U-Boot > > - Parse U_BOOT_DRIVER(name) to build a list of drivers in dtoc > > - Parse U_BOOT_DRIVER_ALIAS(name, new_name) to populate the list of > drivers with aliases in dtoc > > - When parsing dts file, look for the proper driver name based on the > list created, is no one is found raise an error > > > I think the parser could be simple, something like (this is only a draft > to show the idea) > > #!/usr/bin/python > import os > import re > > class UBootDriverList: > > def parse_u_boot_driver(self, fn): > f = open(fn) > > b = f.read() > > drivers = re.findall('U_BOOT_DRIVER\((.*)\)', b) > > for d in drivers: > self.driver_list.append(d) > > def parse_u_boot_drivers(self): > self.driver_list = [] > for (dirpath, dirnames, filenames) in os.walk('./'): > for fn in filenames: > if not fn.endswith('.c'): > continue > self.parse_u_boot_driver(dirpath + '/' + fn) > > def print_list(self): > for d in self.driver_list: > print d > > driver_list = UBootDriverList() > driver_list.parse_u_boot_drivers() > driver_list.print_list() > > What do you think? > > If this make sense we can try to go also in this way for other improvements. LGTM sounds nice. Regards, Simon
diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c index 4900498e9b..761a4b46e9 100644 --- a/drivers/mmc/fsl_esdhc_imx.c +++ b/drivers/mmc/fsl_esdhc_imx.c @@ -29,6 +29,8 @@ #include <dm.h> #include <asm-generic/gpio.h> #include <dm/pinctrl.h> +#include <dt-structs.h> +#include <mapmem.h> #if !CONFIG_IS_ENABLED(BLK) #include "mmc_private.h" @@ -98,6 +100,11 @@ struct fsl_esdhc { }; struct fsl_esdhc_plat { +#if CONFIG_IS_ENABLED(OF_PLATDATA) + /* Put this first since driver model will copy the data here */ + struct dtd_fsl_imx6q_usdhc dtplat; +#endif + struct mmc_config cfg; struct mmc mmc; }; @@ -1377,14 +1384,18 @@ static int fsl_esdhc_probe(struct udevice *dev) struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev); struct fsl_esdhc_plat *plat = dev_get_platdata(dev); struct fsl_esdhc_priv *priv = dev_get_priv(dev); +#if !CONFIG_IS_ENABLED(OF_PLATDATA) const void *fdt = gd->fdt_blob; int node = dev_of_offset(dev); + fdt_addr_t addr; +#else + struct dtd_fsl_imx6q_usdhc *dtplat = &plat->dtplat; +#endif struct esdhc_soc_data *data = (struct esdhc_soc_data *)dev_get_driver_data(dev); #if CONFIG_IS_ENABLED(DM_REGULATOR) struct udevice *vqmmc_dev; #endif - fdt_addr_t addr; unsigned int val; struct mmc *mmc; #if !CONFIG_IS_ENABLED(BLK) @@ -1392,14 +1403,23 @@ static int fsl_esdhc_probe(struct udevice *dev) #endif int ret; +#if CONFIG_IS_ENABLED(OF_PLATDATA) + priv->esdhc_regs = map_sysmem(dtplat->reg[0], dtplat->reg[1]); + val = plat->dtplat.bus_width; + if (val == 8) + priv->bus_width = 8; + else if (val == 4) + priv->bus_width = 4; + else + priv->bus_width = 1; + priv->non_removable = 1; +#else addr = dev_read_addr(dev); if (addr == FDT_ADDR_T_NONE) return -EINVAL; priv->esdhc_regs = (struct fsl_esdhc *)addr; priv->dev = dev; priv->mode = -1; - if (data) - priv->flags = data->flags; val = dev_read_u32_default(dev, "bus-width", -1); if (val == 8) @@ -1462,7 +1482,9 @@ static int fsl_esdhc_probe(struct udevice *dev) priv->vs18_enable = 1; } #endif - +#endif + if (data) + priv->flags = data->flags; /* * TODO: * Because lack of clk driver, if SDHC clk is not enabled, @@ -1513,9 +1535,11 @@ static int fsl_esdhc_probe(struct udevice *dev) return ret; } +#if !CONFIG_IS_ENABLED(OF_PLATDATA) ret = mmc_of_parse(dev, &plat->cfg); if (ret) return ret; +#endif mmc = &plat->mmc; mmc->cfg = &plat->cfg; @@ -1648,4 +1672,18 @@ U_BOOT_DRIVER(fsl_esdhc) = { .platdata_auto_alloc_size = sizeof(struct fsl_esdhc_plat), .priv_auto_alloc_size = sizeof(struct fsl_esdhc_priv), }; + +#if CONFIG_IS_ENABLED(OF_PLATDATA) +U_BOOT_DRIVER(fsl_usdhc) = { + .name = "fsl_imx6q_usdhc", + .id = UCLASS_MMC, + .ops = &fsl_esdhc_ops, +#if CONFIG_IS_ENABLED(BLK) + .bind = fsl_esdhc_bind, +#endif + .probe = fsl_esdhc_probe, + .platdata_auto_alloc_size = sizeof(struct fsl_esdhc_plat), + .priv_auto_alloc_size = sizeof(struct fsl_esdhc_priv), +}; +#endif #endif
Signed-off-by: Walter Lozano <walter.lozano@collabora.com> --- drivers/mmc/fsl_esdhc_imx.c | 46 +++++++++++++++++++++++++++++++++---- 1 file changed, 42 insertions(+), 4 deletions(-)