| Message ID | 20181122063349.7526-1-jagan@amarulasolutions.com |
|---|---|
| State | Superseded |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
| Series | [U-Boot,v3,1/3] spi: pl022: Simplify platdata code | expand |
Hi Jagan, On Thu, Nov 22, 2018 at 12:03:47PM +0530, Jagan Teki wrote: > pl022 spi driver support both OF_CONTROL and PLATDATA, this > patch is trying to simplify the code that differentiating > platdata vs of_control. > - Move OF_CONTROL code at one place > - Handle clock setup code directly in pl022_spi_ofdata_to_platdata > > Cc: Quentin Schulz <quentin.schulz@bootlin.com> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > --- > Changes for v3: > - none > Changes for v2: > - Update commit message > - Use struct clk for clkdev > > drivers/spi/pl022_spi.c | 48 ++++++++++++---------------- > include/dm/platform_data/pl022_spi.h | 9 ------ > 2 files changed, 20 insertions(+), 37 deletions(-) > > diff --git a/drivers/spi/pl022_spi.c b/drivers/spi/pl022_spi.c > index 86b71d2e21..05f4f6f481 100644 > --- a/drivers/spi/pl022_spi.c > +++ b/drivers/spi/pl022_spi.c > @@ -72,11 +72,7 @@ > > struct pl022_spi_slave { > void *base; > -#if !CONFIG_IS_ENABLED(OF_PLATDATA) > - struct clk clk; > -#else > unsigned int freq; > -#endif > }; > > /* > @@ -96,30 +92,13 @@ static int pl022_is_supported(struct pl022_spi_slave *ps) > return 0; > } > > -#if !CONFIG_IS_ENABLED(OF_PLATDATA) > -static int pl022_spi_ofdata_to_platdata(struct udevice *bus) > -{ > - struct pl022_spi_pdata *plat = bus->platdata; > - const void *fdt = gd->fdt_blob; > - int node = dev_of_offset(bus); > - > - plat->addr = fdtdec_get_addr_size(fdt, node, "reg", &plat->size); > - > - return clk_get_by_index(bus, 0, &plat->clk); > -} > -#endif > - > static int pl022_spi_probe(struct udevice *bus) > { > struct pl022_spi_pdata *plat = dev_get_platdata(bus); > struct pl022_spi_slave *ps = dev_get_priv(bus); > > ps->base = ioremap(plat->addr, plat->size); > -#if !CONFIG_IS_ENABLED(OF_PLATDATA) > - ps->clk = plat->clk; > -#else > ps->freq = plat->freq; > -#endif > > /* Check the PL022 version */ > if (!pl022_is_supported(ps)) > @@ -240,11 +219,7 @@ static int pl022_spi_set_speed(struct udevice *bus, uint speed) > u16 scr = SSP_SCR_MIN, cr0 = 0, cpsr = SSP_CPSR_MIN, best_scr = scr, > best_cpsr = cpsr; > u32 min, max, best_freq = 0, tmp; > -#if !CONFIG_IS_ENABLED(OF_PLATDATA) > - u32 rate = clk_get_rate(&ps->clk); > -#else > u32 rate = ps->freq; > -#endif > bool found = false; > > max = spi_rate(rate, SSP_CPSR_MIN, SSP_SCR_MIN); > @@ -316,6 +291,25 @@ static const struct dm_spi_ops pl022_spi_ops = { > }; > > #if !CONFIG_IS_ENABLED(OF_PLATDATA) > +static int pl022_spi_ofdata_to_platdata(struct udevice *bus) > +{ > + struct pl022_spi_pdata *plat = bus->platdata; > + const void *fdt = gd->fdt_blob; > + int node = dev_of_offset(bus); > + struct clk clkdev; > + int ret; > + > + plat->addr = fdtdec_get_addr_size(fdt, node, "reg", &plat->size); > + > + ret = clk_get_by_index(bus, 0, &clkdev); > + if (ret) > + return ret; > + > + plat->freq = clk_get_rate(&clkdev); > + > + return 0; > +} > + > static const struct udevice_id pl022_spi_ids[] = { > { .compatible = "arm,pl022-spi" }, > { } > @@ -327,11 +321,9 @@ U_BOOT_DRIVER(pl022_spi) = { > .id = UCLASS_SPI, > #if !CONFIG_IS_ENABLED(OF_PLATDATA) > .of_match = pl022_spi_ids, > -#endif > - .ops = &pl022_spi_ops, > -#if !CONFIG_IS_ENABLED(OF_PLATDATA) > .ofdata_to_platdata = pl022_spi_ofdata_to_platdata, > #endif > + .ops = &pl022_spi_ops, > .platdata_auto_alloc_size = sizeof(struct pl022_spi_pdata), > .priv_auto_alloc_size = sizeof(struct pl022_spi_slave), > .probe = pl022_spi_probe, > diff --git a/include/dm/platform_data/pl022_spi.h b/include/dm/platform_data/pl022_spi.h > index 77fe6da3cb..57d12ac912 100644 > --- a/include/dm/platform_data/pl022_spi.h > +++ b/include/dm/platform_data/pl022_spi.h > @@ -10,19 +10,10 @@ > #ifndef __PL022_SPI_H__ > #define __PL022_SPI_H__ > > -#if !CONFIG_IS_ENABLED(OF_PLATDATA) > -#include <clk.h> > -#endif > -#include <fdtdec.h> > - As stated in your first version of the patch[1][2], I need fdtdec.h to be in this header file so that I can successfuly compile. [1] https://lists.denx.de/pipermail/u-boot/2018-November/346470.html [2] https://lists.denx.de/pipermail/u-boot/2018-November/347371.html Quentin
On Thu, Nov 22, 2018 at 1:21 PM Quentin Schulz <quentin.schulz@bootlin.com> wrote: > > Hi Jagan, > > On Thu, Nov 22, 2018 at 12:03:47PM +0530, Jagan Teki wrote: > > pl022 spi driver support both OF_CONTROL and PLATDATA, this > > patch is trying to simplify the code that differentiating > > platdata vs of_control. > > - Move OF_CONTROL code at one place > > - Handle clock setup code directly in pl022_spi_ofdata_to_platdata > > > > Cc: Quentin Schulz <quentin.schulz@bootlin.com> > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > > --- > > Changes for v3: > > - none > > Changes for v2: > > - Update commit message > > - Use struct clk for clkdev > > > > drivers/spi/pl022_spi.c | 48 ++++++++++++---------------- > > include/dm/platform_data/pl022_spi.h | 9 ------ > > 2 files changed, 20 insertions(+), 37 deletions(-) > > > > diff --git a/drivers/spi/pl022_spi.c b/drivers/spi/pl022_spi.c > > index 86b71d2e21..05f4f6f481 100644 > > --- a/drivers/spi/pl022_spi.c > > +++ b/drivers/spi/pl022_spi.c > > @@ -72,11 +72,7 @@ > > > > struct pl022_spi_slave { > > void *base; > > -#if !CONFIG_IS_ENABLED(OF_PLATDATA) > > - struct clk clk; > > -#else > > unsigned int freq; > > -#endif > > }; > > > > /* > > @@ -96,30 +92,13 @@ static int pl022_is_supported(struct pl022_spi_slave *ps) > > return 0; > > } > > > > -#if !CONFIG_IS_ENABLED(OF_PLATDATA) > > -static int pl022_spi_ofdata_to_platdata(struct udevice *bus) > > -{ > > - struct pl022_spi_pdata *plat = bus->platdata; > > - const void *fdt = gd->fdt_blob; > > - int node = dev_of_offset(bus); > > - > > - plat->addr = fdtdec_get_addr_size(fdt, node, "reg", &plat->size); > > - > > - return clk_get_by_index(bus, 0, &plat->clk); > > -} > > -#endif > > - > > static int pl022_spi_probe(struct udevice *bus) > > { > > struct pl022_spi_pdata *plat = dev_get_platdata(bus); > > struct pl022_spi_slave *ps = dev_get_priv(bus); > > > > ps->base = ioremap(plat->addr, plat->size); > > -#if !CONFIG_IS_ENABLED(OF_PLATDATA) > > - ps->clk = plat->clk; > > -#else > > ps->freq = plat->freq; > > -#endif > > > > /* Check the PL022 version */ > > if (!pl022_is_supported(ps)) > > @@ -240,11 +219,7 @@ static int pl022_spi_set_speed(struct udevice *bus, uint speed) > > u16 scr = SSP_SCR_MIN, cr0 = 0, cpsr = SSP_CPSR_MIN, best_scr = scr, > > best_cpsr = cpsr; > > u32 min, max, best_freq = 0, tmp; > > -#if !CONFIG_IS_ENABLED(OF_PLATDATA) > > - u32 rate = clk_get_rate(&ps->clk); > > -#else > > u32 rate = ps->freq; > > -#endif > > bool found = false; > > > > max = spi_rate(rate, SSP_CPSR_MIN, SSP_SCR_MIN); > > @@ -316,6 +291,25 @@ static const struct dm_spi_ops pl022_spi_ops = { > > }; > > > > #if !CONFIG_IS_ENABLED(OF_PLATDATA) > > +static int pl022_spi_ofdata_to_platdata(struct udevice *bus) > > +{ > > + struct pl022_spi_pdata *plat = bus->platdata; > > + const void *fdt = gd->fdt_blob; > > + int node = dev_of_offset(bus); > > + struct clk clkdev; > > + int ret; > > + > > + plat->addr = fdtdec_get_addr_size(fdt, node, "reg", &plat->size); > > + > > + ret = clk_get_by_index(bus, 0, &clkdev); > > + if (ret) > > + return ret; > > + > > + plat->freq = clk_get_rate(&clkdev); > > + > > + return 0; > > +} > > + > > static const struct udevice_id pl022_spi_ids[] = { > > { .compatible = "arm,pl022-spi" }, > > { } > > @@ -327,11 +321,9 @@ U_BOOT_DRIVER(pl022_spi) = { > > .id = UCLASS_SPI, > > #if !CONFIG_IS_ENABLED(OF_PLATDATA) > > .of_match = pl022_spi_ids, > > -#endif > > - .ops = &pl022_spi_ops, > > -#if !CONFIG_IS_ENABLED(OF_PLATDATA) > > .ofdata_to_platdata = pl022_spi_ofdata_to_platdata, > > #endif > > + .ops = &pl022_spi_ops, > > .platdata_auto_alloc_size = sizeof(struct pl022_spi_pdata), > > .priv_auto_alloc_size = sizeof(struct pl022_spi_slave), > > .probe = pl022_spi_probe, > > diff --git a/include/dm/platform_data/pl022_spi.h b/include/dm/platform_data/pl022_spi.h > > index 77fe6da3cb..57d12ac912 100644 > > --- a/include/dm/platform_data/pl022_spi.h > > +++ b/include/dm/platform_data/pl022_spi.h > > @@ -10,19 +10,10 @@ > > #ifndef __PL022_SPI_H__ > > #define __PL022_SPI_H__ > > > > -#if !CONFIG_IS_ENABLED(OF_PLATDATA) > > -#include <clk.h> > > -#endif > > -#include <fdtdec.h> > > - > > As stated in your first version of the patch[1][2], I need fdtdec.h to > be in this header file so that I can successfuly compile. which board config I need to enable?
Hi Jagan, On Thu, Nov 22, 2018 at 01:31:25PM +0530, Jagan Teki wrote: > On Thu, Nov 22, 2018 at 1:21 PM Quentin Schulz > <quentin.schulz@bootlin.com> wrote: > > > > Hi Jagan, > > > > On Thu, Nov 22, 2018 at 12:03:47PM +0530, Jagan Teki wrote: > > > pl022 spi driver support both OF_CONTROL and PLATDATA, this > > > patch is trying to simplify the code that differentiating > > > platdata vs of_control. > > > - Move OF_CONTROL code at one place > > > - Handle clock setup code directly in pl022_spi_ofdata_to_platdata > > > > > > Cc: Quentin Schulz <quentin.schulz@bootlin.com> > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > > > --- > > > Changes for v3: > > > - none > > > Changes for v2: > > > - Update commit message > > > - Use struct clk for clkdev > > > > > > drivers/spi/pl022_spi.c | 48 ++++++++++++---------------- > > > include/dm/platform_data/pl022_spi.h | 9 ------ > > > 2 files changed, 20 insertions(+), 37 deletions(-) > > > > > > diff --git a/drivers/spi/pl022_spi.c b/drivers/spi/pl022_spi.c > > > index 86b71d2e21..05f4f6f481 100644 > > > --- a/drivers/spi/pl022_spi.c > > > +++ b/drivers/spi/pl022_spi.c > > > @@ -72,11 +72,7 @@ > > > > > > struct pl022_spi_slave { > > > void *base; > > > -#if !CONFIG_IS_ENABLED(OF_PLATDATA) > > > - struct clk clk; > > > -#else > > > unsigned int freq; > > > -#endif > > > }; > > > > > > /* > > > @@ -96,30 +92,13 @@ static int pl022_is_supported(struct pl022_spi_slave *ps) > > > return 0; > > > } > > > > > > -#if !CONFIG_IS_ENABLED(OF_PLATDATA) > > > -static int pl022_spi_ofdata_to_platdata(struct udevice *bus) > > > -{ > > > - struct pl022_spi_pdata *plat = bus->platdata; > > > - const void *fdt = gd->fdt_blob; > > > - int node = dev_of_offset(bus); > > > - > > > - plat->addr = fdtdec_get_addr_size(fdt, node, "reg", &plat->size); > > > - > > > - return clk_get_by_index(bus, 0, &plat->clk); > > > -} > > > -#endif > > > - > > > static int pl022_spi_probe(struct udevice *bus) > > > { > > > struct pl022_spi_pdata *plat = dev_get_platdata(bus); > > > struct pl022_spi_slave *ps = dev_get_priv(bus); > > > > > > ps->base = ioremap(plat->addr, plat->size); > > > -#if !CONFIG_IS_ENABLED(OF_PLATDATA) > > > - ps->clk = plat->clk; > > > -#else > > > ps->freq = plat->freq; > > > -#endif > > > > > > /* Check the PL022 version */ > > > if (!pl022_is_supported(ps)) > > > @@ -240,11 +219,7 @@ static int pl022_spi_set_speed(struct udevice *bus, uint speed) > > > u16 scr = SSP_SCR_MIN, cr0 = 0, cpsr = SSP_CPSR_MIN, best_scr = scr, > > > best_cpsr = cpsr; > > > u32 min, max, best_freq = 0, tmp; > > > -#if !CONFIG_IS_ENABLED(OF_PLATDATA) > > > - u32 rate = clk_get_rate(&ps->clk); > > > -#else > > > u32 rate = ps->freq; > > > -#endif > > > bool found = false; > > > > > > max = spi_rate(rate, SSP_CPSR_MIN, SSP_SCR_MIN); > > > @@ -316,6 +291,25 @@ static const struct dm_spi_ops pl022_spi_ops = { > > > }; > > > > > > #if !CONFIG_IS_ENABLED(OF_PLATDATA) > > > +static int pl022_spi_ofdata_to_platdata(struct udevice *bus) > > > +{ > > > + struct pl022_spi_pdata *plat = bus->platdata; > > > + const void *fdt = gd->fdt_blob; > > > + int node = dev_of_offset(bus); > > > + struct clk clkdev; > > > + int ret; > > > + > > > + plat->addr = fdtdec_get_addr_size(fdt, node, "reg", &plat->size); > > > + > > > + ret = clk_get_by_index(bus, 0, &clkdev); > > > + if (ret) > > > + return ret; > > > + > > > + plat->freq = clk_get_rate(&clkdev); > > > + > > > + return 0; > > > +} > > > + > > > static const struct udevice_id pl022_spi_ids[] = { > > > { .compatible = "arm,pl022-spi" }, > > > { } > > > @@ -327,11 +321,9 @@ U_BOOT_DRIVER(pl022_spi) = { > > > .id = UCLASS_SPI, > > > #if !CONFIG_IS_ENABLED(OF_PLATDATA) > > > .of_match = pl022_spi_ids, > > > -#endif > > > - .ops = &pl022_spi_ops, > > > -#if !CONFIG_IS_ENABLED(OF_PLATDATA) > > > .ofdata_to_platdata = pl022_spi_ofdata_to_platdata, > > > #endif > > > + .ops = &pl022_spi_ops, > > > .platdata_auto_alloc_size = sizeof(struct pl022_spi_pdata), > > > .priv_auto_alloc_size = sizeof(struct pl022_spi_slave), > > > .probe = pl022_spi_probe, > > > diff --git a/include/dm/platform_data/pl022_spi.h b/include/dm/platform_data/pl022_spi.h > > > index 77fe6da3cb..57d12ac912 100644 > > > --- a/include/dm/platform_data/pl022_spi.h > > > +++ b/include/dm/platform_data/pl022_spi.h > > > @@ -10,19 +10,10 @@ > > > #ifndef __PL022_SPI_H__ > > > #define __PL022_SPI_H__ > > > > > > -#if !CONFIG_IS_ENABLED(OF_PLATDATA) > > > -#include <clk.h> > > > -#endif > > > -#include <fdtdec.h> > > > - > > > > As stated in your first version of the patch[1][2], I need fdtdec.h to > > be in this header file so that I can successfuly compile. > > which board config I need to enable? Not mainline. The point I'm trying to make[2], is that ANY board defining platdata in a board file will need the `include/dm/platform_data/pl022_spi.h` header, this is the reason it's there, to be reused outside of the driver. With your patch, each and every board file that will need to define a U_BOOT_DEVICE entry with .platdata being of type `struct pl022_spi_pdata` will need to include the fdtdec header because in this structure, we have both fdt_addr_t and fdt_size_t that are used which are only defined in include/fdtdec.h[3]. Your patch is wrong, because: 1) It breaks the compilation on my side. While I could hear the argument of "it's not mainline we don't care", there is 2) 2) It's absolutely horrendous design to rely on each header file or C file including this header to include also fdtdec.h. With this mindset, let's not include any header file except in the final C file. You include the header file where you use parts of it. Here we use fdt_addr_t and fdt_size_t in include/dm/platform_data/pl022_spi.h which are both defined in include/fdtdec.h. [2] https://lists.denx.de/pipermail/u-boot/2018-November/347371.html [3] https://elixir.bootlin.com/u-boot/latest/source/include/fdtdec.h#L25 Quentin
On Thu, Nov 22, 2018 at 1:48 PM Quentin Schulz <quentin.schulz@bootlin.com> wrote: > > Hi Jagan, > > On Thu, Nov 22, 2018 at 01:31:25PM +0530, Jagan Teki wrote: > > On Thu, Nov 22, 2018 at 1:21 PM Quentin Schulz > > <quentin.schulz@bootlin.com> wrote: > > > > > > Hi Jagan, > > > > > > On Thu, Nov 22, 2018 at 12:03:47PM +0530, Jagan Teki wrote: > > > > pl022 spi driver support both OF_CONTROL and PLATDATA, this > > > > patch is trying to simplify the code that differentiating > > > > platdata vs of_control. > > > > - Move OF_CONTROL code at one place > > > > - Handle clock setup code directly in pl022_spi_ofdata_to_platdata > > > > > > > > Cc: Quentin Schulz <quentin.schulz@bootlin.com> > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > > > > --- > > > > Changes for v3: > > > > - none > > > > Changes for v2: > > > > - Update commit message > > > > - Use struct clk for clkdev > > > > > > > > drivers/spi/pl022_spi.c | 48 ++++++++++++---------------- > > > > include/dm/platform_data/pl022_spi.h | 9 ------ > > > > 2 files changed, 20 insertions(+), 37 deletions(-) > > > > > > > > diff --git a/drivers/spi/pl022_spi.c b/drivers/spi/pl022_spi.c > > > > index 86b71d2e21..05f4f6f481 100644 > > > > --- a/drivers/spi/pl022_spi.c > > > > +++ b/drivers/spi/pl022_spi.c > > > > @@ -72,11 +72,7 @@ > > > > > > > > struct pl022_spi_slave { > > > > void *base; > > > > -#if !CONFIG_IS_ENABLED(OF_PLATDATA) > > > > - struct clk clk; > > > > -#else > > > > unsigned int freq; > > > > -#endif > > > > }; > > > > > > > > /* > > > > @@ -96,30 +92,13 @@ static int pl022_is_supported(struct pl022_spi_slave *ps) > > > > return 0; > > > > } > > > > > > > > -#if !CONFIG_IS_ENABLED(OF_PLATDATA) > > > > -static int pl022_spi_ofdata_to_platdata(struct udevice *bus) > > > > -{ > > > > - struct pl022_spi_pdata *plat = bus->platdata; > > > > - const void *fdt = gd->fdt_blob; > > > > - int node = dev_of_offset(bus); > > > > - > > > > - plat->addr = fdtdec_get_addr_size(fdt, node, "reg", &plat->size); > > > > - > > > > - return clk_get_by_index(bus, 0, &plat->clk); > > > > -} > > > > -#endif > > > > - > > > > static int pl022_spi_probe(struct udevice *bus) > > > > { > > > > struct pl022_spi_pdata *plat = dev_get_platdata(bus); > > > > struct pl022_spi_slave *ps = dev_get_priv(bus); > > > > > > > > ps->base = ioremap(plat->addr, plat->size); > > > > -#if !CONFIG_IS_ENABLED(OF_PLATDATA) > > > > - ps->clk = plat->clk; > > > > -#else > > > > ps->freq = plat->freq; > > > > -#endif > > > > > > > > /* Check the PL022 version */ > > > > if (!pl022_is_supported(ps)) > > > > @@ -240,11 +219,7 @@ static int pl022_spi_set_speed(struct udevice *bus, uint speed) > > > > u16 scr = SSP_SCR_MIN, cr0 = 0, cpsr = SSP_CPSR_MIN, best_scr = scr, > > > > best_cpsr = cpsr; > > > > u32 min, max, best_freq = 0, tmp; > > > > -#if !CONFIG_IS_ENABLED(OF_PLATDATA) > > > > - u32 rate = clk_get_rate(&ps->clk); > > > > -#else > > > > u32 rate = ps->freq; > > > > -#endif > > > > bool found = false; > > > > > > > > max = spi_rate(rate, SSP_CPSR_MIN, SSP_SCR_MIN); > > > > @@ -316,6 +291,25 @@ static const struct dm_spi_ops pl022_spi_ops = { > > > > }; > > > > > > > > #if !CONFIG_IS_ENABLED(OF_PLATDATA) > > > > +static int pl022_spi_ofdata_to_platdata(struct udevice *bus) > > > > +{ > > > > + struct pl022_spi_pdata *plat = bus->platdata; > > > > + const void *fdt = gd->fdt_blob; > > > > + int node = dev_of_offset(bus); > > > > + struct clk clkdev; > > > > + int ret; > > > > + > > > > + plat->addr = fdtdec_get_addr_size(fdt, node, "reg", &plat->size); > > > > + > > > > + ret = clk_get_by_index(bus, 0, &clkdev); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + plat->freq = clk_get_rate(&clkdev); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > static const struct udevice_id pl022_spi_ids[] = { > > > > { .compatible = "arm,pl022-spi" }, > > > > { } > > > > @@ -327,11 +321,9 @@ U_BOOT_DRIVER(pl022_spi) = { > > > > .id = UCLASS_SPI, > > > > #if !CONFIG_IS_ENABLED(OF_PLATDATA) > > > > .of_match = pl022_spi_ids, > > > > -#endif > > > > - .ops = &pl022_spi_ops, > > > > -#if !CONFIG_IS_ENABLED(OF_PLATDATA) > > > > .ofdata_to_platdata = pl022_spi_ofdata_to_platdata, > > > > #endif > > > > + .ops = &pl022_spi_ops, > > > > .platdata_auto_alloc_size = sizeof(struct pl022_spi_pdata), > > > > .priv_auto_alloc_size = sizeof(struct pl022_spi_slave), > > > > .probe = pl022_spi_probe, > > > > diff --git a/include/dm/platform_data/pl022_spi.h b/include/dm/platform_data/pl022_spi.h > > > > index 77fe6da3cb..57d12ac912 100644 > > > > --- a/include/dm/platform_data/pl022_spi.h > > > > +++ b/include/dm/platform_data/pl022_spi.h > > > > @@ -10,19 +10,10 @@ > > > > #ifndef __PL022_SPI_H__ > > > > #define __PL022_SPI_H__ > > > > > > > > -#if !CONFIG_IS_ENABLED(OF_PLATDATA) > > > > -#include <clk.h> > > > > -#endif > > > > -#include <fdtdec.h> > > > > - > > > > > > As stated in your first version of the patch[1][2], I need fdtdec.h to > > > be in this header file so that I can successfuly compile. > > > > which board config I need to enable? > > Not mainline. > > The point I'm trying to make[2], is that ANY board defining platdata in > a board file will need the `include/dm/platform_data/pl022_spi.h` > header, this is the reason it's there, to be reused outside of the > driver. > > With your patch, each and every board file that will need to define a > U_BOOT_DEVICE entry with .platdata being of type `struct > pl022_spi_pdata` will need to include the fdtdec header because in this > structure, we have both fdt_addr_t and fdt_size_t that are used which > are only defined in include/fdtdec.h[3]. > > Your patch is wrong, because: > 1) It breaks the compilation on my side. While I could hear the argument > of "it's not mainline we don't care", there is 2) > > 2) It's absolutely horrendous design to rely on each header file or C > file including this header to include also fdtdec.h. With this mindset, > let's not include any header file except in the final C file. You > include the header file where you use parts of it. Here we use > fdt_addr_t and fdt_size_t in include/dm/platform_data/pl022_spi.h which > are both defined in include/fdtdec.h. Got your point, didn't notice that the driver is using devfdt_get_addr. I think we can switch to recent devfdt function to skip the fdtdec.h. like using devfdt_get_addr and devm_ioremap
Hi Jagan, On Sat, Nov 24, 2018 at 05:41:03PM +0530, Jagan Teki wrote: > On Thu, Nov 22, 2018 at 1:48 PM Quentin Schulz > <quentin.schulz@bootlin.com> wrote: > > > > Hi Jagan, > > > > On Thu, Nov 22, 2018 at 01:31:25PM +0530, Jagan Teki wrote: > > > On Thu, Nov 22, 2018 at 1:21 PM Quentin Schulz > > > <quentin.schulz@bootlin.com> wrote: > > > > > > > > Hi Jagan, > > > > > > > > On Thu, Nov 22, 2018 at 12:03:47PM +0530, Jagan Teki wrote: > > > > > pl022 spi driver support both OF_CONTROL and PLATDATA, this > > > > > patch is trying to simplify the code that differentiating > > > > > platdata vs of_control. > > > > > - Move OF_CONTROL code at one place > > > > > - Handle clock setup code directly in pl022_spi_ofdata_to_platdata > > > > > > > > > > Cc: Quentin Schulz <quentin.schulz@bootlin.com> > > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > > > > > --- > > > > > Changes for v3: > > > > > - none > > > > > Changes for v2: > > > > > - Update commit message > > > > > - Use struct clk for clkdev > > > > > > > > > > drivers/spi/pl022_spi.c | 48 ++++++++++++---------------- > > > > > include/dm/platform_data/pl022_spi.h | 9 ------ > > > > > 2 files changed, 20 insertions(+), 37 deletions(-) > > > > > > > > > > diff --git a/drivers/spi/pl022_spi.c b/drivers/spi/pl022_spi.c > > > > > index 86b71d2e21..05f4f6f481 100644 > > > > > --- a/drivers/spi/pl022_spi.c > > > > > +++ b/drivers/spi/pl022_spi.c > > > > > @@ -72,11 +72,7 @@ > > > > > > > > > > struct pl022_spi_slave { > > > > > void *base; > > > > > -#if !CONFIG_IS_ENABLED(OF_PLATDATA) > > > > > - struct clk clk; > > > > > -#else > > > > > unsigned int freq; > > > > > -#endif > > > > > }; > > > > > > > > > > /* > > > > > @@ -96,30 +92,13 @@ static int pl022_is_supported(struct pl022_spi_slave *ps) > > > > > return 0; > > > > > } > > > > > > > > > > -#if !CONFIG_IS_ENABLED(OF_PLATDATA) > > > > > -static int pl022_spi_ofdata_to_platdata(struct udevice *bus) > > > > > -{ > > > > > - struct pl022_spi_pdata *plat = bus->platdata; > > > > > - const void *fdt = gd->fdt_blob; > > > > > - int node = dev_of_offset(bus); > > > > > - > > > > > - plat->addr = fdtdec_get_addr_size(fdt, node, "reg", &plat->size); > > > > > - > > > > > - return clk_get_by_index(bus, 0, &plat->clk); > > > > > -} > > > > > -#endif > > > > > - > > > > > static int pl022_spi_probe(struct udevice *bus) > > > > > { > > > > > struct pl022_spi_pdata *plat = dev_get_platdata(bus); > > > > > struct pl022_spi_slave *ps = dev_get_priv(bus); > > > > > > > > > > ps->base = ioremap(plat->addr, plat->size); > > > > > -#if !CONFIG_IS_ENABLED(OF_PLATDATA) > > > > > - ps->clk = plat->clk; > > > > > -#else > > > > > ps->freq = plat->freq; > > > > > -#endif > > > > > > > > > > /* Check the PL022 version */ > > > > > if (!pl022_is_supported(ps)) > > > > > @@ -240,11 +219,7 @@ static int pl022_spi_set_speed(struct udevice *bus, uint speed) > > > > > u16 scr = SSP_SCR_MIN, cr0 = 0, cpsr = SSP_CPSR_MIN, best_scr = scr, > > > > > best_cpsr = cpsr; > > > > > u32 min, max, best_freq = 0, tmp; > > > > > -#if !CONFIG_IS_ENABLED(OF_PLATDATA) > > > > > - u32 rate = clk_get_rate(&ps->clk); > > > > > -#else > > > > > u32 rate = ps->freq; > > > > > -#endif > > > > > bool found = false; > > > > > > > > > > max = spi_rate(rate, SSP_CPSR_MIN, SSP_SCR_MIN); > > > > > @@ -316,6 +291,25 @@ static const struct dm_spi_ops pl022_spi_ops = { > > > > > }; > > > > > > > > > > #if !CONFIG_IS_ENABLED(OF_PLATDATA) > > > > > +static int pl022_spi_ofdata_to_platdata(struct udevice *bus) > > > > > +{ > > > > > + struct pl022_spi_pdata *plat = bus->platdata; > > > > > + const void *fdt = gd->fdt_blob; > > > > > + int node = dev_of_offset(bus); > > > > > + struct clk clkdev; > > > > > + int ret; > > > > > + > > > > > + plat->addr = fdtdec_get_addr_size(fdt, node, "reg", &plat->size); > > > > > + > > > > > + ret = clk_get_by_index(bus, 0, &clkdev); > > > > > + if (ret) > > > > > + return ret; > > > > > + > > > > > + plat->freq = clk_get_rate(&clkdev); > > > > > + > > > > > + return 0; > > > > > +} > > > > > + > > > > > static const struct udevice_id pl022_spi_ids[] = { > > > > > { .compatible = "arm,pl022-spi" }, > > > > > { } > > > > > @@ -327,11 +321,9 @@ U_BOOT_DRIVER(pl022_spi) = { > > > > > .id = UCLASS_SPI, > > > > > #if !CONFIG_IS_ENABLED(OF_PLATDATA) > > > > > .of_match = pl022_spi_ids, > > > > > -#endif > > > > > - .ops = &pl022_spi_ops, > > > > > -#if !CONFIG_IS_ENABLED(OF_PLATDATA) > > > > > .ofdata_to_platdata = pl022_spi_ofdata_to_platdata, > > > > > #endif > > > > > + .ops = &pl022_spi_ops, > > > > > .platdata_auto_alloc_size = sizeof(struct pl022_spi_pdata), > > > > > .priv_auto_alloc_size = sizeof(struct pl022_spi_slave), > > > > > .probe = pl022_spi_probe, > > > > > diff --git a/include/dm/platform_data/pl022_spi.h b/include/dm/platform_data/pl022_spi.h > > > > > index 77fe6da3cb..57d12ac912 100644 > > > > > --- a/include/dm/platform_data/pl022_spi.h > > > > > +++ b/include/dm/platform_data/pl022_spi.h > > > > > @@ -10,19 +10,10 @@ > > > > > #ifndef __PL022_SPI_H__ > > > > > #define __PL022_SPI_H__ > > > > > > > > > > -#if !CONFIG_IS_ENABLED(OF_PLATDATA) > > > > > -#include <clk.h> > > > > > -#endif > > > > > -#include <fdtdec.h> > > > > > - > > > > > > > > As stated in your first version of the patch[1][2], I need fdtdec.h to > > > > be in this header file so that I can successfuly compile. > > > > > > which board config I need to enable? > > > > Not mainline. > > > > The point I'm trying to make[2], is that ANY board defining platdata in > > a board file will need the `include/dm/platform_data/pl022_spi.h` > > header, this is the reason it's there, to be reused outside of the > > driver. > > > > With your patch, each and every board file that will need to define a > > U_BOOT_DEVICE entry with .platdata being of type `struct > > pl022_spi_pdata` will need to include the fdtdec header because in this > > structure, we have both fdt_addr_t and fdt_size_t that are used which > > are only defined in include/fdtdec.h[3]. > > > > Your patch is wrong, because: > > 1) It breaks the compilation on my side. While I could hear the argument > > of "it's not mainline we don't care", there is 2) > > > > 2) It's absolutely horrendous design to rely on each header file or C > > file including this header to include also fdtdec.h. With this mindset, > > let's not include any header file except in the final C file. You > > include the header file where you use parts of it. Here we use > > fdt_addr_t and fdt_size_t in include/dm/platform_data/pl022_spi.h which > > are both defined in include/fdtdec.h. > > Got your point, didn't notice that the driver is using > devfdt_get_addr. I think we can switch to recent devfdt function to > skip the fdtdec.h. like using devfdt_get_addr and devm_ioremap You will NOT be able to get rid of fdtdec.h. In the include/dm/platform_data/pl022_spi.h header file you have the pl022_spi_pdata structure which have two variables, fdt_addr_t addr and fdt_size_t size. fdt_addr_t and fdt_size_t are only defined in fdtdec.h header file[1][2]. So the only way to get rid of fdtdec.h is to get rid of those two variables in the pl022_spi_pdata structure. Quentin [1] https://elixir.bootlin.com/u-boot/latest/ident/fdt_addr_t [2] https://elixir.bootlin.com/u-boot/latest/ident/fdt_size_t
On Sun, Nov 25, 2018 at 6:02 PM Quentin Schulz <quentin.schulz@bootlin.com> wrote: > > Hi Jagan, > > On Sat, Nov 24, 2018 at 05:41:03PM +0530, Jagan Teki wrote: > > On Thu, Nov 22, 2018 at 1:48 PM Quentin Schulz > > <quentin.schulz@bootlin.com> wrote: > > > > > > Hi Jagan, > > > > > > On Thu, Nov 22, 2018 at 01:31:25PM +0530, Jagan Teki wrote: > > > > On Thu, Nov 22, 2018 at 1:21 PM Quentin Schulz > > > > <quentin.schulz@bootlin.com> wrote: > > > > > > > > > > Hi Jagan, > > > > > > > > > > On Thu, Nov 22, 2018 at 12:03:47PM +0530, Jagan Teki wrote: > > > > > > pl022 spi driver support both OF_CONTROL and PLATDATA, this > > > > > > patch is trying to simplify the code that differentiating > > > > > > platdata vs of_control. > > > > > > - Move OF_CONTROL code at one place > > > > > > - Handle clock setup code directly in pl022_spi_ofdata_to_platdata > > > > > > > > > > > > Cc: Quentin Schulz <quentin.schulz@bootlin.com> > > > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > > > > > > --- > > > > > > Changes for v3: > > > > > > - none > > > > > > Changes for v2: > > > > > > - Update commit message > > > > > > - Use struct clk for clkdev > > > > > > > > > > > > drivers/spi/pl022_spi.c | 48 ++++++++++++---------------- > > > > > > include/dm/platform_data/pl022_spi.h | 9 ------ > > > > > > 2 files changed, 20 insertions(+), 37 deletions(-) > > > > > > > > > > > > diff --git a/drivers/spi/pl022_spi.c b/drivers/spi/pl022_spi.c > > > > > > index 86b71d2e21..05f4f6f481 100644 > > > > > > --- a/drivers/spi/pl022_spi.c > > > > > > +++ b/drivers/spi/pl022_spi.c > > > > > > @@ -72,11 +72,7 @@ > > > > > > > > > > > > struct pl022_spi_slave { > > > > > > void *base; > > > > > > -#if !CONFIG_IS_ENABLED(OF_PLATDATA) > > > > > > - struct clk clk; > > > > > > -#else > > > > > > unsigned int freq; > > > > > > -#endif > > > > > > }; > > > > > > > > > > > > /* > > > > > > @@ -96,30 +92,13 @@ static int pl022_is_supported(struct pl022_spi_slave *ps) > > > > > > return 0; > > > > > > } > > > > > > > > > > > > -#if !CONFIG_IS_ENABLED(OF_PLATDATA) > > > > > > -static int pl022_spi_ofdata_to_platdata(struct udevice *bus) > > > > > > -{ > > > > > > - struct pl022_spi_pdata *plat = bus->platdata; > > > > > > - const void *fdt = gd->fdt_blob; > > > > > > - int node = dev_of_offset(bus); > > > > > > - > > > > > > - plat->addr = fdtdec_get_addr_size(fdt, node, "reg", &plat->size); > > > > > > - > > > > > > - return clk_get_by_index(bus, 0, &plat->clk); > > > > > > -} > > > > > > -#endif > > > > > > - > > > > > > static int pl022_spi_probe(struct udevice *bus) > > > > > > { > > > > > > struct pl022_spi_pdata *plat = dev_get_platdata(bus); > > > > > > struct pl022_spi_slave *ps = dev_get_priv(bus); > > > > > > > > > > > > ps->base = ioremap(plat->addr, plat->size); > > > > > > -#if !CONFIG_IS_ENABLED(OF_PLATDATA) > > > > > > - ps->clk = plat->clk; > > > > > > -#else > > > > > > ps->freq = plat->freq; > > > > > > -#endif > > > > > > > > > > > > /* Check the PL022 version */ > > > > > > if (!pl022_is_supported(ps)) > > > > > > @@ -240,11 +219,7 @@ static int pl022_spi_set_speed(struct udevice *bus, uint speed) > > > > > > u16 scr = SSP_SCR_MIN, cr0 = 0, cpsr = SSP_CPSR_MIN, best_scr = scr, > > > > > > best_cpsr = cpsr; > > > > > > u32 min, max, best_freq = 0, tmp; > > > > > > -#if !CONFIG_IS_ENABLED(OF_PLATDATA) > > > > > > - u32 rate = clk_get_rate(&ps->clk); > > > > > > -#else > > > > > > u32 rate = ps->freq; > > > > > > -#endif > > > > > > bool found = false; > > > > > > > > > > > > max = spi_rate(rate, SSP_CPSR_MIN, SSP_SCR_MIN); > > > > > > @@ -316,6 +291,25 @@ static const struct dm_spi_ops pl022_spi_ops = { > > > > > > }; > > > > > > > > > > > > #if !CONFIG_IS_ENABLED(OF_PLATDATA) > > > > > > +static int pl022_spi_ofdata_to_platdata(struct udevice *bus) > > > > > > +{ > > > > > > + struct pl022_spi_pdata *plat = bus->platdata; > > > > > > + const void *fdt = gd->fdt_blob; > > > > > > + int node = dev_of_offset(bus); > > > > > > + struct clk clkdev; > > > > > > + int ret; > > > > > > + > > > > > > + plat->addr = fdtdec_get_addr_size(fdt, node, "reg", &plat->size); > > > > > > + > > > > > > + ret = clk_get_by_index(bus, 0, &clkdev); > > > > > > + if (ret) > > > > > > + return ret; > > > > > > + > > > > > > + plat->freq = clk_get_rate(&clkdev); > > > > > > + > > > > > > + return 0; > > > > > > +} > > > > > > + > > > > > > static const struct udevice_id pl022_spi_ids[] = { > > > > > > { .compatible = "arm,pl022-spi" }, > > > > > > { } > > > > > > @@ -327,11 +321,9 @@ U_BOOT_DRIVER(pl022_spi) = { > > > > > > .id = UCLASS_SPI, > > > > > > #if !CONFIG_IS_ENABLED(OF_PLATDATA) > > > > > > .of_match = pl022_spi_ids, > > > > > > -#endif > > > > > > - .ops = &pl022_spi_ops, > > > > > > -#if !CONFIG_IS_ENABLED(OF_PLATDATA) > > > > > > .ofdata_to_platdata = pl022_spi_ofdata_to_platdata, > > > > > > #endif > > > > > > + .ops = &pl022_spi_ops, > > > > > > .platdata_auto_alloc_size = sizeof(struct pl022_spi_pdata), > > > > > > .priv_auto_alloc_size = sizeof(struct pl022_spi_slave), > > > > > > .probe = pl022_spi_probe, > > > > > > diff --git a/include/dm/platform_data/pl022_spi.h b/include/dm/platform_data/pl022_spi.h > > > > > > index 77fe6da3cb..57d12ac912 100644 > > > > > > --- a/include/dm/platform_data/pl022_spi.h > > > > > > +++ b/include/dm/platform_data/pl022_spi.h > > > > > > @@ -10,19 +10,10 @@ > > > > > > #ifndef __PL022_SPI_H__ > > > > > > #define __PL022_SPI_H__ > > > > > > > > > > > > -#if !CONFIG_IS_ENABLED(OF_PLATDATA) > > > > > > -#include <clk.h> > > > > > > -#endif > > > > > > -#include <fdtdec.h> > > > > > > - > > > > > > > > > > As stated in your first version of the patch[1][2], I need fdtdec.h to > > > > > be in this header file so that I can successfuly compile. > > > > > > > > which board config I need to enable? > > > > > > Not mainline. > > > > > > The point I'm trying to make[2], is that ANY board defining platdata in > > > a board file will need the `include/dm/platform_data/pl022_spi.h` > > > header, this is the reason it's there, to be reused outside of the > > > driver. > > > > > > With your patch, each and every board file that will need to define a > > > U_BOOT_DEVICE entry with .platdata being of type `struct > > > pl022_spi_pdata` will need to include the fdtdec header because in this > > > structure, we have both fdt_addr_t and fdt_size_t that are used which > > > are only defined in include/fdtdec.h[3]. > > > > > > Your patch is wrong, because: > > > 1) It breaks the compilation on my side. While I could hear the argument > > > of "it's not mainline we don't care", there is 2) > > > > > > 2) It's absolutely horrendous design to rely on each header file or C > > > file including this header to include also fdtdec.h. With this mindset, > > > let's not include any header file except in the final C file. You > > > include the header file where you use parts of it. Here we use > > > fdt_addr_t and fdt_size_t in include/dm/platform_data/pl022_spi.h which > > > are both defined in include/fdtdec.h. > > > > Got your point, didn't notice that the driver is using > > devfdt_get_addr. I think we can switch to recent devfdt function to > > skip the fdtdec.h. like using devfdt_get_addr and devm_ioremap > > You will NOT be able to get rid of fdtdec.h. > > In the include/dm/platform_data/pl022_spi.h header file you have the > pl022_spi_pdata structure which have two variables, fdt_addr_t addr and > fdt_size_t size. fdt_addr_t and fdt_size_t are only defined in fdtdec.h > header file[1][2]. > > So the only way to get rid of fdtdec.h is to get rid of those two > variables in the pl022_spi_pdata structure. with adding structure pointer of reg space. anyway we can look it later. are you fine with v4?
diff --git a/drivers/spi/pl022_spi.c b/drivers/spi/pl022_spi.c index 86b71d2e21..05f4f6f481 100644 --- a/drivers/spi/pl022_spi.c +++ b/drivers/spi/pl022_spi.c @@ -72,11 +72,7 @@ struct pl022_spi_slave { void *base; -#if !CONFIG_IS_ENABLED(OF_PLATDATA) - struct clk clk; -#else unsigned int freq; -#endif }; /* @@ -96,30 +92,13 @@ static int pl022_is_supported(struct pl022_spi_slave *ps) return 0; } -#if !CONFIG_IS_ENABLED(OF_PLATDATA) -static int pl022_spi_ofdata_to_platdata(struct udevice *bus) -{ - struct pl022_spi_pdata *plat = bus->platdata; - const void *fdt = gd->fdt_blob; - int node = dev_of_offset(bus); - - plat->addr = fdtdec_get_addr_size(fdt, node, "reg", &plat->size); - - return clk_get_by_index(bus, 0, &plat->clk); -} -#endif - static int pl022_spi_probe(struct udevice *bus) { struct pl022_spi_pdata *plat = dev_get_platdata(bus); struct pl022_spi_slave *ps = dev_get_priv(bus); ps->base = ioremap(plat->addr, plat->size); -#if !CONFIG_IS_ENABLED(OF_PLATDATA) - ps->clk = plat->clk; -#else ps->freq = plat->freq; -#endif /* Check the PL022 version */ if (!pl022_is_supported(ps)) @@ -240,11 +219,7 @@ static int pl022_spi_set_speed(struct udevice *bus, uint speed) u16 scr = SSP_SCR_MIN, cr0 = 0, cpsr = SSP_CPSR_MIN, best_scr = scr, best_cpsr = cpsr; u32 min, max, best_freq = 0, tmp; -#if !CONFIG_IS_ENABLED(OF_PLATDATA) - u32 rate = clk_get_rate(&ps->clk); -#else u32 rate = ps->freq; -#endif bool found = false; max = spi_rate(rate, SSP_CPSR_MIN, SSP_SCR_MIN); @@ -316,6 +291,25 @@ static const struct dm_spi_ops pl022_spi_ops = { }; #if !CONFIG_IS_ENABLED(OF_PLATDATA) +static int pl022_spi_ofdata_to_platdata(struct udevice *bus) +{ + struct pl022_spi_pdata *plat = bus->platdata; + const void *fdt = gd->fdt_blob; + int node = dev_of_offset(bus); + struct clk clkdev; + int ret; + + plat->addr = fdtdec_get_addr_size(fdt, node, "reg", &plat->size); + + ret = clk_get_by_index(bus, 0, &clkdev); + if (ret) + return ret; + + plat->freq = clk_get_rate(&clkdev); + + return 0; +} + static const struct udevice_id pl022_spi_ids[] = { { .compatible = "arm,pl022-spi" }, { } @@ -327,11 +321,9 @@ U_BOOT_DRIVER(pl022_spi) = { .id = UCLASS_SPI, #if !CONFIG_IS_ENABLED(OF_PLATDATA) .of_match = pl022_spi_ids, -#endif - .ops = &pl022_spi_ops, -#if !CONFIG_IS_ENABLED(OF_PLATDATA) .ofdata_to_platdata = pl022_spi_ofdata_to_platdata, #endif + .ops = &pl022_spi_ops, .platdata_auto_alloc_size = sizeof(struct pl022_spi_pdata), .priv_auto_alloc_size = sizeof(struct pl022_spi_slave), .probe = pl022_spi_probe, diff --git a/include/dm/platform_data/pl022_spi.h b/include/dm/platform_data/pl022_spi.h index 77fe6da3cb..57d12ac912 100644 --- a/include/dm/platform_data/pl022_spi.h +++ b/include/dm/platform_data/pl022_spi.h @@ -10,19 +10,10 @@ #ifndef __PL022_SPI_H__ #define __PL022_SPI_H__ -#if !CONFIG_IS_ENABLED(OF_PLATDATA) -#include <clk.h> -#endif -#include <fdtdec.h> - struct pl022_spi_pdata { fdt_addr_t addr; fdt_size_t size; -#if !CONFIG_IS_ENABLED(OF_PLATDATA) - struct clk clk; -#else unsigned int freq; -#endif }; #endif
pl022 spi driver support both OF_CONTROL and PLATDATA, this patch is trying to simplify the code that differentiating platdata vs of_control. - Move OF_CONTROL code at one place - Handle clock setup code directly in pl022_spi_ofdata_to_platdata Cc: Quentin Schulz <quentin.schulz@bootlin.com> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> --- Changes for v3: - none Changes for v2: - Update commit message - Use struct clk for clkdev drivers/spi/pl022_spi.c | 48 ++++++++++++---------------- include/dm/platform_data/pl022_spi.h | 9 ------ 2 files changed, 20 insertions(+), 37 deletions(-)