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?

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

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?