This patch-set straightens out some serious errors incurred during
the v3.10 merge window, including:
Patch 1: Possible over-write of DB8500-PRCMU platform data
Patch 2: Fail to register AB8500 GPIO driver
Patch 3: Fail to start Ethernet when using ATAGS - Clocks
Patch 4: Fail to probe DB8500 Thermal driver - No pdata
Patch 5: Fail to use Early Printk - Incorrect virtual addresses
Patch 6: Fail to start Ethernet with using Device Tree - Clocks
Patch 7: Oops kernel during driver fail - Null pointer
Patch 8: Fail to probe AB8500 GPIO driver - Incorrect platform ID
Patch 8: Fail to probe AB8500 GPADC driver - Regulators
arch/arm/include/debug/ux500.S | 6 +++---
arch/arm/mach-ux500/board-mop500.c | 6 +++---
arch/arm/mach-ux500/cpu-db8500.c | 6 ++----
arch/arm/mach-ux500/setup.h | 2 +-
drivers/clk/ux500/u8500_clk.c | 2 +-
drivers/mfd/ab8500-core.c | 9 ++++++---
drivers/mfd/db8500-prcmu.c | 1 +
drivers/pinctrl/pinctrl-abx500.c | 30 ++++++++++++++----------------
8 files changed, 31 insertions(+), 31 deletions(-)
A recent move to rid header files which were hindering multiplatform
support forced address allocations out of the headers and into the
files which were using them. We also lost some useful macros such as
IO_ADDRESS(), so physical -> virtual addressing has been carried out
manually in this case. Unfortunately the incorrect value was converted.
This patch rectifies the error and ensures earlyprintk works again.
Signed-off-by: Lee Jones <[email protected]>
---
arch/arm/include/debug/ux500.S | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/arm/include/debug/ux500.S b/arch/arm/include/debug/ux500.S
index 2848857..fbd24be 100644
--- a/arch/arm/include/debug/ux500.S
+++ b/arch/arm/include/debug/ux500.S
@@ -24,9 +24,9 @@
#define U8500_UART0_PHYS_BASE (0x80120000)
#define U8500_UART1_PHYS_BASE (0x80121000)
#define U8500_UART2_PHYS_BASE (0x80007000)
-#define U8500_UART0_VIRT_BASE (0xa8120000)
-#define U8500_UART1_VIRT_BASE (0xa8121000)
-#define U8500_UART2_VIRT_BASE (0xa8007000)
+#define U8500_UART0_VIRT_BASE (0xf8120000)
+#define U8500_UART1_VIRT_BASE (0xf8121000)
+#define U8500_UART2_VIRT_BASE (0xf8007000)
#define __UX500_PHYS_UART(n) U8500_UART##n##_PHYS_BASE
#define __UX500_VIRT_UART(n) U8500_UART##n##_VIRT_BASE
#endif
--
1.7.10.4
If a sub-driver has not been specified correctly, there is a good chance
that plat_id is NULL, hence using an attribute of plat_id in the error
message is likely to not only fail the driver but Oops the kernel. Use
the failed ID instead.
Signed-off-by: Lee Jones <[email protected]>
---
drivers/pinctrl/pinctrl-abx500.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-abx500.c b/drivers/pinctrl/pinctrl-abx500.c
index aa17f75..2a2a9df 100644
--- a/drivers/pinctrl/pinctrl-abx500.c
+++ b/drivers/pinctrl/pinctrl-abx500.c
@@ -900,8 +900,7 @@ static int abx500_gpio_probe(struct platform_device *pdev)
abx500_pinctrl_ab8505_init(&pct->soc);
break;
default:
- dev_err(&pdev->dev, "Unsupported pinctrl sub driver (%d)\n",
- (int) platid->driver_data);
+ dev_err(&pdev->dev, "Unsupported pinctrl sub driver (%d)\n", id);
mutex_destroy(&pct->lock);
return -EINVAL;
}
--
1.7.10.4
When booting with Device Tree enabled the MFD core uses each device's
compatible string to find and allocate its associated of_node pointer,
which in turn is passed to the driver via the platform_device struct.
Without it, the driver won't be able to interrogate the Device Tree or
locate suitable regulators and will most likely fail to probe.
Cc: Samuel Ortiz <[email protected]>
Signed-off-by: Lee Jones <[email protected]>
---
drivers/mfd/ab8500-core.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/mfd/ab8500-core.c b/drivers/mfd/ab8500-core.c
index 2047485..983eb1e 100644
--- a/drivers/mfd/ab8500-core.c
+++ b/drivers/mfd/ab8500-core.c
@@ -1060,6 +1060,7 @@ static struct mfd_cell ab8500_devs[] = {
},
{
.name = "ab8500-gpadc",
+ .of_compatible = "stericsson,ab8500-gpadc",
.num_resources = ARRAY_SIZE(ab8500_gpadc_resources),
.resources = ab8500_gpadc_resources,
},
@@ -1217,6 +1218,7 @@ static struct mfd_cell ab8505_devs[] = {
},
{
.name = "ab8500-gpadc",
+ .of_compatible = "stericsson,ab8500-gpadc",
.num_resources = ARRAY_SIZE(ab8505_gpadc_resources),
.resources = ab8505_gpadc_resources,
},
@@ -1280,6 +1282,7 @@ static struct mfd_cell ab8540_devs[] = {
},
{
.name = "ab8500-gpadc",
+ .of_compatible = "stericsson,ab8500-gpadc",
.num_resources = ARRAY_SIZE(ab8505_gpadc_resources),
.resources = ab8505_gpadc_resources,
},
--
1.7.10.4
A pointer to GPIO platform data is always passed to the driver now, so
there's little point in checking for 'pdata' and executing the DT case if
it's not there. The difference between booting with DT and !DT is when
booting with DT, plat_id is not populated. Thus, in the DT case we have
to use a DT match table in order to find out which platform we're
executing on. So, we're changing the semantics here to only use the
match table if no plat_id is supplied though platform data.
Signed-off-by: Lee Jones <[email protected]>
---
drivers/pinctrl/pinctrl-abx500.c | 27 +++++++++++++--------------
1 file changed, 13 insertions(+), 14 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-abx500.c b/drivers/pinctrl/pinctrl-abx500.c
index 2a2a9df..6d45327 100644
--- a/drivers/pinctrl/pinctrl-abx500.c
+++ b/drivers/pinctrl/pinctrl-abx500.c
@@ -851,23 +851,12 @@ static int abx500_gpio_probe(struct platform_device *pdev)
if (abx500_pdata)
pdata = abx500_pdata->gpio;
- if (!pdata) {
- if (np) {
- const struct of_device_id *match;
- match = of_match_device(abx500_gpio_match, &pdev->dev);
- if (!match)
- return -ENODEV;
- id = (unsigned long)match->data;
- } else {
- dev_err(&pdev->dev, "gpio dt and platform data missing\n");
- return -ENODEV;
- }
+ if (!(pdata || np)) {
+ dev_err(&pdev->dev, "gpio dt and platform data missing\n");
+ return -ENODEV;
}
- if (platid)
- id = platid->driver_data;
-
pct = devm_kzalloc(&pdev->dev, sizeof(struct abx500_pinctrl),
GFP_KERNEL);
if (pct == NULL) {
@@ -882,6 +871,16 @@ static int abx500_gpio_probe(struct platform_device *pdev)
pct->chip.dev = &pdev->dev;
pct->chip.base = (np) ? -1 : pdata->gpio_base;
+ if (platid)
+ id = platid->driver_data;
+ else if (np) {
+ const struct of_device_id *match;
+
+ match = of_match_device(abx500_gpio_match, &pdev->dev);
+ if (match)
+ id = (unsigned long)match->data;
+ }
+
/* initialize the lock */
mutex_init(&pct->lock);
--
1.7.10.4
First Ethernet device has a ".0" appended onto the device name. It
appears that we need this in order to obtain the correct clock.
Signed-off-by: Lee Jones <[email protected]>
---
arch/arm/mach-ux500/cpu-db8500.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/mach-ux500/cpu-db8500.c b/arch/arm/mach-ux500/cpu-db8500.c
index 8c2590c..718e707 100644
--- a/arch/arm/mach-ux500/cpu-db8500.c
+++ b/arch/arm/mach-ux500/cpu-db8500.c
@@ -276,7 +276,7 @@ static struct of_dev_auxdata u8500_auxdata_lookup[] __initdata = {
OF_DEV_AUXDATA("st,nomadik-i2c", 0x8012a000, "nmk-i2c.4", NULL),
OF_DEV_AUXDATA("stericsson,db8500-prcmu", 0x80157000, "db8500-prcmu",
&db8500_prcmu_pdata),
- OF_DEV_AUXDATA("smsc,lan9115", 0x50000000, "smsc911x", NULL),
+ OF_DEV_AUXDATA("smsc,lan9115", 0x50000000, "smsc911x.0", NULL),
/* Requires device name bindings. */
OF_DEV_AUXDATA("stericsson,nmk-pinctrl", U8500_PRCMU_BASE,
"pinctrl-db8500", NULL),
--
1.7.10.4
When we're using Device Tree to enable GPIO drivers we're forced to be
OS agnostic, thus we are forbidden use names like pinctrl as they are
specific only to Linux. However, when we are registering devices using
internal systems such as MFD or platform registration, we can use such
terminology. In this case we can and should use the platform device ID
mechanism to specify which device we wish to utilise by detailing
pinctrl-<device_name>.
Cc: Samuel Ortiz <[email protected]>
Signed-off-by: Lee Jones <[email protected]>
---
drivers/mfd/ab8500-core.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/mfd/ab8500-core.c b/drivers/mfd/ab8500-core.c
index 42abd3a..2047485 100644
--- a/drivers/mfd/ab8500-core.c
+++ b/drivers/mfd/ab8500-core.c
@@ -1106,7 +1106,7 @@ static struct mfd_cell ab8500_devs[] = {
.of_compatible = "stericsson,ab8500-denc",
},
{
- .name = "ab8500-gpio",
+ .name = "pinctrl-ab8500",
.of_compatible = "stericsson,ab8500-gpio",
},
{
@@ -1243,7 +1243,7 @@ static struct mfd_cell ab8505_devs[] = {
.name = "ab8500-leds",
},
{
- .name = "ab8500-gpio",
+ .name = "pinctrl-ab8505",
},
{
.name = "ab8500-usb",
@@ -1311,7 +1311,7 @@ static struct mfd_cell ab8540_devs[] = {
.resources = ab8500_temp_resources,
},
{
- .name = "ab8500-gpio",
+ .name = "pinctrl-ab8540",
},
{
.name = "ab8540-usb",
--
1.7.10.4
First Ethernet device has a ".0" appended onto the device name. It
appears that we need this in order to obtain the correct clock.
Cc: Ulf Hansson <[email protected]>
Cc: Mike Turquette <[email protected]>
Signed-off-by: Lee Jones <[email protected]>
---
drivers/clk/ux500/u8500_clk.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/clk/ux500/u8500_clk.c b/drivers/clk/ux500/u8500_clk.c
index 0b4f35a..80069c3 100644
--- a/drivers/clk/ux500/u8500_clk.c
+++ b/drivers/clk/ux500/u8500_clk.c
@@ -325,7 +325,7 @@ void u8500_clk_init(u32 clkrst1_base, u32 clkrst2_base, u32 clkrst3_base,
clk = clk_reg_prcc_pclk("p3_pclk0", "per3clk", clkrst3_base,
BIT(0), 0);
clk_register_clkdev(clk, "fsmc", NULL);
- clk_register_clkdev(clk, NULL, "smsc911x");
+ clk_register_clkdev(clk, NULL, "smsc911x.0");
clk = clk_reg_prcc_pclk("p3_pclk1", "per3clk", clkrst3_base,
BIT(1), 0);
--
1.7.10.4
The MFD subsystem requires drivers to state the size of any platform
data passed, or it will fail to assign it to the device. This will
culminate in a NULL platform_data attribute and normally a failure to
probe() or a kernel Oops.
Cc: Samuel Ortiz <[email protected]>
Signed-off-by: Lee Jones <[email protected]>
---
drivers/mfd/db8500-prcmu.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/mfd/db8500-prcmu.c b/drivers/mfd/db8500-prcmu.c
index 319b8ab..5389368 100644
--- a/drivers/mfd/db8500-prcmu.c
+++ b/drivers/mfd/db8500-prcmu.c
@@ -3095,6 +3095,7 @@ static struct mfd_cell db8500_prcmu_devs[] = {
.num_resources = ARRAY_SIZE(db8500_thsens_resources),
.resources = db8500_thsens_resources,
.platform_data = &db8500_thsens_data,
+ .pdata_size = sizeof(db8500_thsens_data),
},
};
--
1.7.10.4
Since: "05ec260 mfd: db8500-prcmu: update resource passing", the AB8500's
platform data 'ab8500_platdata' is passed directly as an attribute to
'db8500_prcmu_pdata', so there's no requirement to assign it a second
time. In fact, it's only due to an ordering issue that the entire
'db8500_prcmu_pdata' data structure isn't completely over-written by the
assignment in u8500_init_devices().
Signed-off-by: Lee Jones <[email protected]>
---
arch/arm/mach-ux500/board-mop500.c | 6 +++---
arch/arm/mach-ux500/cpu-db8500.c | 4 +---
arch/arm/mach-ux500/setup.h | 2 +-
3 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c
index 9bc762a..14a3598 100644
--- a/arch/arm/mach-ux500/board-mop500.c
+++ b/arch/arm/mach-ux500/board-mop500.c
@@ -623,7 +623,7 @@ static void __init mop500_init_machine(void)
sdi0_reg_info.gpios[0].gpio = GPIO_SDMMC_1V8_3V_SEL;
mop500_pinmaps_init();
- parent = u8500_init_devices(&ab8500_platdata);
+ parent = u8500_init_devices();
for (i = 0; i < ARRAY_SIZE(mop500_platform_devs); i++)
mop500_platform_devs[i]->dev.parent = parent;
@@ -660,7 +660,7 @@ static void __init snowball_init_machine(void)
sdi0_reg_info.gpios[0].gpio = SNOWBALL_SDMMC_1V8_3V_GPIO;
snowball_pinmaps_init();
- parent = u8500_init_devices(&ab8500_platdata);
+ parent = u8500_init_devices();
for (i = 0; i < ARRAY_SIZE(snowball_platform_devs); i++)
snowball_platform_devs[i]->dev.parent = parent;
@@ -698,7 +698,7 @@ static void __init hrefv60_init_machine(void)
sdi0_reg_info.gpios[0].gpio = HREFV60_SDMMC_1V8_3V_GPIO;
hrefv60_pinmaps_init();
- parent = u8500_init_devices(&ab8500_platdata);
+ parent = u8500_init_devices();
for (i = 0; i < ARRAY_SIZE(mop500_platform_devs); i++)
mop500_platform_devs[i]->dev.parent = parent;
diff --git a/arch/arm/mach-ux500/cpu-db8500.c b/arch/arm/mach-ux500/cpu-db8500.c
index 995928b..8c2590c 100644
--- a/arch/arm/mach-ux500/cpu-db8500.c
+++ b/arch/arm/mach-ux500/cpu-db8500.c
@@ -206,7 +206,7 @@ static struct device * __init db8500_soc_device_init(void)
/*
* This function is called from the board init
*/
-struct device * __init u8500_init_devices(struct ab8500_platform_data *ab8500)
+struct device * __init u8500_init_devices(void)
{
struct device *parent;
int i;
@@ -220,8 +220,6 @@ struct device * __init u8500_init_devices(struct ab8500_platform_data *ab8500)
for (i = 0; i < ARRAY_SIZE(platform_devs); i++)
platform_devs[i]->dev.parent = parent;
- db8500_prcmu_device.dev.platform_data = ab8500;
-
platform_add_devices(platform_devs, ARRAY_SIZE(platform_devs));
return parent;
diff --git a/arch/arm/mach-ux500/setup.h b/arch/arm/mach-ux500/setup.h
index bddce2b..cad3ca8 100644
--- a/arch/arm/mach-ux500/setup.h
+++ b/arch/arm/mach-ux500/setup.h
@@ -18,7 +18,7 @@
void __init ux500_map_io(void);
extern void __init u8500_map_io(void);
-extern struct device * __init u8500_init_devices(struct ab8500_platform_data *ab8500);
+extern struct device * __init u8500_init_devices(void);
extern void __init ux500_init_irq(void);
extern void __init ux500_init_late(void);
--
1.7.10.4
Hi, Sam, Mike & Ulf
I see that you weren't on the list for the 0/9 patch, so here FYI:
> This patch-set straightens out some serious errors incurred during
> the v3.10 merge window, including:
>
> Patch 1: Possible over-write of DB8500-PRCMU platform data
> Patch 2: Fail to register AB8500 GPIO driver
> Patch 3: Fail to start Ethernet when using ATAGS - Clocks
> Patch 4: Fail to probe DB8500 Thermal driver - No pdata
> Patch 5: Fail to use Early Printk - Incorrect virtual addresses
> Patch 6: Fail to start Ethernet with using Device Tree - Clocks
> Patch 7: Oops kernel during driver fail - Null pointer
> Patch 8: Fail to probe AB8500 GPIO driver - Incorrect platform ID
> Patch 9: Fail to probe AB8500 GPADC driver - Regulators
>
> arch/arm/include/debug/ux500.S | 6 +++---
> arch/arm/mach-ux500/board-mop500.c | 6 +++---
> arch/arm/mach-ux500/cpu-db8500.c | 6 ++----
> arch/arm/mach-ux500/setup.h | 2 +-
> drivers/clk/ux500/u8500_clk.c | 2 +-
> drivers/mfd/ab8500-core.c | 9 ++++++---
> drivers/mfd/db8500-prcmu.c | 1 +
> drivers/pinctrl/pinctrl-abx500.c | 30 ++++++++++++++----------------
> 8 files changed, 31 insertions(+), 31 deletions(-)
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On 8 May 2013 15:29, Lee Jones <[email protected]> wrote:
> First Ethernet device has a ".0" appended onto the device name. It
> appears that we need this in order to obtain the correct clock.
>
> Cc: Ulf Hansson <[email protected]>
> Cc: Mike Turquette <[email protected]>
> Signed-off-by: Lee Jones <[email protected]>
> ---
> drivers/clk/ux500/u8500_clk.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/clk/ux500/u8500_clk.c b/drivers/clk/ux500/u8500_clk.c
> index 0b4f35a..80069c3 100644
> --- a/drivers/clk/ux500/u8500_clk.c
> +++ b/drivers/clk/ux500/u8500_clk.c
> @@ -325,7 +325,7 @@ void u8500_clk_init(u32 clkrst1_base, u32 clkrst2_base, u32 clkrst3_base,
> clk = clk_reg_prcc_pclk("p3_pclk0", "per3clk", clkrst3_base,
> BIT(0), 0);
> clk_register_clkdev(clk, "fsmc", NULL);
> - clk_register_clkdev(clk, NULL, "smsc911x");
> + clk_register_clkdev(clk, NULL, "smsc911x.0");
>
> clk = clk_reg_prcc_pclk("p3_pclk1", "per3clk", clkrst3_base,
> BIT(1), 0);
> --
> 1.7.10.4
>
Acked-by: Ulf Hansson <[email protected]>
On Wed, May 8, 2013 at 3:29 PM, Lee Jones <[email protected]> wrote:
> Since: "05ec260 mfd: db8500-prcmu: update resource passing", the AB8500's
> platform data 'ab8500_platdata' is passed directly as an attribute to
> 'db8500_prcmu_pdata', so there's no requirement to assign it a second
> time. In fact, it's only due to an ordering issue that the entire
> 'db8500_prcmu_pdata' data structure isn't completely over-written by the
> assignment in u8500_init_devices().
>
> Signed-off-by: Lee Jones <[email protected]>
Good catch! Patch applied.
Yours,
Linus Walleij
On Wed, May 8, 2013 at 3:29 PM, Lee Jones <[email protected]> wrote:
> When we're using Device Tree to enable GPIO drivers we're forced to be
> OS agnostic, thus we are forbidden use names like pinctrl as they are
> specific only to Linux.
Oh that depends. I have hade lectures on pin control at the embedded
systems conference and there I treat it as a neutral term referring to this
kind of electronic constructions.
However how something is established in OF ontology I don't know so
you might be right there, but you're making it sound like
Documentation/devicetree/bindings/pinctrl/
should not exist and all those bindings be moved to gpio or
something if the assumption that "pin control" or the abbreviation
pinctrl is a Linux-specific term. (Which is not my interpretation.)
> However, when we are registering devices using
> internal systems such as MFD or platform registration, we can use such
> terminology. In this case we can and should use the platform device ID
> mechanism to specify which device we wish to utilise by detailing
> pinctrl-<device_name>.
>
> Cc: Samuel Ortiz <[email protected]>
> Signed-off-by: Lee Jones <[email protected]>
The commit message should state what regression it is actually
solving. Which is that when using a non-DT boot, the ABx500
pinctrl devices are not probed, right?
Apart from the commit message:
Acked-by: Linus Walleij <[email protected]>
On the patch as such.
Yours,
Linus Walleij
On Wed, May 8, 2013 at 3:29 PM, Lee Jones <[email protected]> wrote:
> First Ethernet device has a ".0" appended onto the device name. It
> appears that we need this in order to obtain the correct clock.
>
> Cc: Ulf Hansson <[email protected]>
> Cc: Mike Turquette <[email protected]>
> Signed-off-by: Lee Jones <[email protected]>
Acked-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Wed, May 8, 2013 at 3:29 PM, Lee Jones <[email protected]> wrote:
> The MFD subsystem requires drivers to state the size of any platform
> data passed, or it will fail to assign it to the device. This will
> culminate in a NULL platform_data attribute and normally a failure to
> probe() or a kernel Oops.
>
> Cc: Samuel Ortiz <[email protected]>
> Signed-off-by: Lee Jones <[email protected]>
Acked-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Wed, May 8, 2013 at 3:29 PM, Lee Jones <[email protected]> wrote:
> A recent move to rid header files which were hindering multiplatform
> support forced address allocations out of the headers and into the
> files which were using them. We also lost some useful macros such as
> IO_ADDRESS(), so physical -> virtual addressing has been carried out
> manually in this case. Unfortunately the incorrect value was converted.
> This patch rectifies the error and ensures earlyprintk works again.
>
> Signed-off-by: Lee Jones <[email protected]>
Thanks for fixing this!
And sorry for screwing it up :-(
Patch applied!
Yours,
Linus Walleij
On Wed, May 8, 2013 at 3:29 PM, Lee Jones <[email protected]> wrote:
> First Ethernet device has a ".0" appended onto the device name. It
> appears that we need this in order to obtain the correct clock.
>
> Signed-off-by: Lee Jones <[email protected]>
(...)
> - OF_DEV_AUXDATA("smsc,lan9115", 0x50000000, "smsc911x", NULL),
> + OF_DEV_AUXDATA("smsc,lan9115", 0x50000000, "smsc911x.0", NULL),
I guess what you want to say in the commit message is that
on a non-DT boot the ethernet will be named "smsc911x.0"
and since the clocks are not converted to device tree these
names need to be matched when providing the name from
auxdata.
Edited the commit message and applied!
Yours,
Linus Walleij
On Wed, May 8, 2013 at 3:29 PM, Lee Jones <[email protected]> wrote:
> If a sub-driver has not been specified correctly, there is a good chance
> that plat_id is NULL, hence using an attribute of plat_id in the error
> message is likely to not only fail the driver but Oops the kernel. Use
> the failed ID instead.
>
> Signed-off-by: Lee Jones <[email protected]>
Good patch! Applied to the pinctrl fixes.
Yours,
Linus Walleij
On Wed, May 8, 2013 at 3:29 PM, Lee Jones <[email protected]> wrote:
> A pointer to GPIO platform data is always passed to the driver now, so
> there's little point in checking for 'pdata' and executing the DT case if
> it's not there. The difference between booting with DT and !DT is when
> booting with DT, plat_id is not populated. Thus, in the DT case we have
> to use a DT match table in order to find out which platform we're
> executing on. So, we're changing the semantics here to only use the
> match table if no plat_id is supplied though platform data.
>
> Signed-off-by: Lee Jones <[email protected]>
Is this really included in the [0/9] fire alarm wrapper statement
"Important ux500 fixups due for the v3.10 -rc:s"?
It seems more like a random refactoring to me.
The commit message fails to specify which regression this
is fixing, like if it's causing an oops or so.
So I've tentatively applied it to the pinctrl devel branch for
v3.11 unless something comes up...
Yours,
Linus Walleij
> > First Ethernet device has a ".0" appended onto the device name. It
> > appears that we need this in order to obtain the correct clock.
> >
> > Cc: Ulf Hansson <[email protected]>
> > Cc: Mike Turquette <[email protected]>
> > Signed-off-by: Lee Jones <[email protected]>
> Acked-by: Linus Walleij <[email protected]>
> Acked-by: Ulf Hansson <[email protected]>
Post -rc1 poke for Mike.
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Wed, May 8, 2013 at 3:29 PM, Lee Jones <[email protected]> wrote:
> When booting with Device Tree enabled the MFD core uses each device's
> compatible string to find and allocate its associated of_node pointer,
> which in turn is passed to the driver via the platform_device struct.
> Without it, the driver won't be able to interrogate the Device Tree or
> locate suitable regulators and will most likely fail to probe.
>
> Cc: Samuel Ortiz <[email protected]>
> Signed-off-by: Lee Jones <[email protected]>
Can you explain what regression this is causing since the GPADC
driver (drivers/mfd/ab8500-gpadc.c) does not support device tree
probing and does not have an .of_match_table defined in it's
driver struct? I mean, what could possibly match that
compatible string? The .name field will take care of naming
the device does it not?
For non-emergency merging though:
Acked-by: Linus Walleij <[email protected]>
Since it matches the example in
Documentation/devicetree/bindings/mfd/ab8500.txt
But for this to make sense the AB8500 ADC driver needs
to be augmented for DT probing and preferrably also moved
to drivers/adc and made to utilize that subsystem.
Yours,
Linus Walleij
On Tue, 14 May 2013, Linus Walleij wrote:
> On Wed, May 8, 2013 at 3:29 PM, Lee Jones <[email protected]> wrote:
>
> > A pointer to GPIO platform data is always passed to the driver now, so
> > there's little point in checking for 'pdata' and executing the DT case if
> > it's not there. The difference between booting with DT and !DT is when
> > booting with DT, plat_id is not populated. Thus, in the DT case we have
> > to use a DT match table in order to find out which platform we're
> > executing on. So, we're changing the semantics here to only use the
> > match table if no plat_id is supplied though platform data.
> >
> > Signed-off-by: Lee Jones <[email protected]>
>
> Is this really included in the [0/9] fire alarm wrapper statement
> "Important ux500 fixups due for the v3.10 -rc:s"?
>
> It seems more like a random refactoring to me.
>
> The commit message fails to specify which regression this
> is fixing, like if it's causing an oops or so.
>
> So I've tentatively applied it to the pinctrl devel branch for
> v3.11 unless something comes up...
Perhaps the commit message is a bit weak, but yes, it _needs_ to go
into v3.10, or this driver will be _broken_ when we boot with DT.
Please apply this to your -fixes branch.
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Tue, 14 May 2013, Linus Walleij wrote:
> On Wed, May 8, 2013 at 3:29 PM, Lee Jones <[email protected]> wrote:
>
> > First Ethernet device has a ".0" appended onto the device name. It
> > appears that we need this in order to obtain the correct clock.
> >
> > Signed-off-by: Lee Jones <[email protected]>
> (...)
> > - OF_DEV_AUXDATA("smsc,lan9115", 0x50000000, "smsc911x", NULL),
> > + OF_DEV_AUXDATA("smsc,lan9115", 0x50000000, "smsc911x.0", NULL),
>
> I guess what you want to say in the commit message is that
> on a non-DT boot the ethernet will be named "smsc911x.0"
> and since the clocks are not converted to device tree these
> names need to be matched when providing the name from
> auxdata.
>
> Edited the commit message and applied!
Thanks for the effort. :)
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Tue, May 14, 2013 at 10:47 AM, Lee Jones <[email protected]> wrote:
> On Tue, 14 May 2013, Linus Walleij wrote:
>
>> On Wed, May 8, 2013 at 3:29 PM, Lee Jones <[email protected]> wrote:
>>
>> > A pointer to GPIO platform data is always passed to the driver now, so
>> > there's little point in checking for 'pdata' and executing the DT case if
>> > it's not there. The difference between booting with DT and !DT is when
>> > booting with DT, plat_id is not populated. Thus, in the DT case we have
>> > to use a DT match table in order to find out which platform we're
>> > executing on. So, we're changing the semantics here to only use the
>> > match table if no plat_id is supplied though platform data.
>> >
>> > Signed-off-by: Lee Jones <[email protected]>
>>
>> Is this really included in the [0/9] fire alarm wrapper statement
>> "Important ux500 fixups due for the v3.10 -rc:s"?
>>
>> It seems more like a random refactoring to me.
>>
>> The commit message fails to specify which regression this
>> is fixing, like if it's causing an oops or so.
>>
>> So I've tentatively applied it to the pinctrl devel branch for
>> v3.11 unless something comes up...
>
> Perhaps the commit message is a bit weak, but yes, it _needs_ to go
> into v3.10, or this driver will be _broken_ when we boot with DT.
>
> Please apply this to your -fixes branch.
Quting myself from above:
"The commit message fails to specify which regression this
is fixing, like if it's causing an oops or so."
I'm happy to apply it but what do I write in the commit message?
Yours,
Linus Walleij
On Tue, 14 May 2013, Linus Walleij wrote:
> On Wed, May 8, 2013 at 3:29 PM, Lee Jones <[email protected]> wrote:
>
> > When booting with Device Tree enabled the MFD core uses each device's
> > compatible string to find and allocate its associated of_node pointer,
> > which in turn is passed to the driver via the platform_device struct.
> > Without it, the driver won't be able to interrogate the Device Tree or
> > locate suitable regulators and will most likely fail to probe.
> >
> > Cc: Samuel Ortiz <[email protected]>
> > Signed-off-by: Lee Jones <[email protected]>
>
> Can you explain what regression this is causing since the GPADC
> driver (drivers/mfd/ab8500-gpadc.c) does not support device tree
Yes it does. :)
> probing and does not have an .of_match_table defined in it's
> driver struct? I mean, what could possibly match that
> compatible string? The .name field will take care of naming
> the device does it not?
You only need that stuff if you require _extra_ bindings. Things like
regulators and interrupt numbers are configured behind the scenes.
> For non-emergency merging though:
> Acked-by: Linus Walleij <[email protected]>
If we don't put this into v3.10-rcX, then the GPADC driver will be
broken in v3.10 when booting with DT.
> Since it matches the example in
> Documentation/devicetree/bindings/mfd/ab8500.txt
>
> But for this to make sense the AB8500 ADC driver needs
> to be augmented for DT probing and preferrably also moved
> to drivers/adc and made to utilize that subsystem.
Not sure I understand the reasoning for this. It works well as a plain
MFD device.
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Tue, 14 May 2013, Linus Walleij wrote:
> On Tue, May 14, 2013 at 10:47 AM, Lee Jones <[email protected]> wrote:
> > On Tue, 14 May 2013, Linus Walleij wrote:
> >
> >> On Wed, May 8, 2013 at 3:29 PM, Lee Jones <[email protected]> wrote:
> >>
> >> > A pointer to GPIO platform data is always passed to the driver now, so
> >> > there's little point in checking for 'pdata' and executing the DT case if
> >> > it's not there. The difference between booting with DT and !DT is when
> >> > booting with DT, plat_id is not populated. Thus, in the DT case we have
> >> > to use a DT match table in order to find out which platform we're
> >> > executing on. So, we're changing the semantics here to only use the
> >> > match table if no plat_id is supplied though platform data.
> >> >
> >> > Signed-off-by: Lee Jones <[email protected]>
> >>
> >> Is this really included in the [0/9] fire alarm wrapper statement
> >> "Important ux500 fixups due for the v3.10 -rc:s"?
> >>
> >> It seems more like a random refactoring to me.
> >>
> >> The commit message fails to specify which regression this
> >> is fixing, like if it's causing an oops or so.
> >>
> >> So I've tentatively applied it to the pinctrl devel branch for
> >> v3.11 unless something comes up...
> >
> > Perhaps the commit message is a bit weak, but yes, it _needs_ to go
> > into v3.10, or this driver will be _broken_ when we boot with DT.
> >
> > Please apply this to your -fixes branch.
>
> Quting myself from above:
> "The commit message fails to specify which regression this
> is fixing, like if it's causing an oops or so."
>
> I'm happy to apply it but what do I write in the commit message?
Platform Data is invariably populated for this driver, even when
booting with Device Tree. Thus the Device Tree probing code encased
within the first check for Platform Data will never executed, causing
the driver to fail when DT is enabled.
This patch fixes the aforementioned regression by rejigging the
probe() semantics to attempt to extract a platform ID from Device Tree
if one can not be sourced from platform data.
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Tue, May 14, 2013 at 11:26 AM, Lee Jones <[email protected]> wrote:
>> probing and does not have an .of_match_table defined in it's
>> driver struct? I mean, what could possibly match that
>> compatible string? The .name field will take care of naming
>> the device does it not?
>
> You only need that stuff if you require _extra_ bindings. Things like
> regulators and interrupt numbers are configured behind the scenes.
Sure, but isn't the idea with of_match_table that this should be
used in drivers when probing from device tree?
Surely it *works* with other schemes but I always thought about
them as some kind of rough fallback hacks. I would be way more
convenient with the patch if it added an of_match_table to
the ADC driver as well, because then I understand what is
going on.
>> For non-emergency merging though:
>> Acked-by: Linus Walleij <[email protected]>
>
> If we don't put this into v3.10-rcX, then the GPADC driver will be
> broken in v3.10 when booting with DT.
OK I think I understand why now...
>> But for this to make sense the AB8500 ADC driver needs
>> to be augmented for DT probing and preferrably also moved
>> to drivers/adc and made to utilize that subsystem.
>
> Not sure I understand the reasoning for this. It works well as a plain
> MFD device.
The commit message says:
"Without it, the driver won't be able to interrogate the Device Tree or
locate suitable regulators and will most likely fail to probe."
That commit message makes it sound like the driver is
interrogating (hm is that the right term...) the device tree, but
now you say it isn't (as you say it is a plain MFD device).
Of course the MFD core and drivers/of/platform.c is doing
all sorts of magic based on the device tree, and that is
what the patch fixes, right? Nothing to do with the driver
really?
I suspect the missing regulators due to how the OF core or
MFD core uses the device tree is the actual problem
solved by the patch?
Anyway I think I was after this:
Isn't the idea that all devices that get probed from a device tree
shall have corresponding bindings documented under
Documentation/devicetree/bindings/*?
And this driver is definately in the wrong subsystem (not your
fault) so I suspect the binding doc should be in
Documentation/devicetree/bindings/adc/ab8500.txt or
something. I'm not pointing that out as a problem in this
patch, I'm just discussing the mess we're in ...
Yours,
Linus Walleij
On Tue, May 14, 2013 at 11:40 AM, Lee Jones <[email protected]> wrote:
>> I'm happy to apply it but what do I write in the commit message?
>
> Platform Data is invariably populated for this driver, even when
> booting with Device Tree. Thus the Device Tree probing code encased
> within the first check for Platform Data will never executed, causing
> the driver to fail when DT is enabled.
>
> This patch fixes the aforementioned regression by rejigging the
> probe() semantics to attempt to extract a platform ID from Device Tree
> if one can not be sourced from platform data.
Thanks!
Copy/pasted into commit message and applied to fixes.
Yours,
Linus Walleij
> >> probing and does not have an .of_match_table defined in it's
> >> driver struct? I mean, what could possibly match that
> >> compatible string? The .name field will take care of naming
> >> the device does it not?
> >
> > You only need that stuff if you require _extra_ bindings. Things like
> > regulators and interrupt numbers are configured behind the scenes.
>
> Sure, but isn't the idea with of_match_table that this should be
> used in drivers when probing from device tree?
We're not probing from Device Tree. We're registering from MFD.
> Surely it *works* with other schemes but I always thought about
> them as some kind of rough fallback hacks. I would be way more
> convenient with the patch if it added an of_match_table to
> the ADC driver as well, because then I understand what is
> going on.
They're not full-back hacks. All drivers obtain their interrupts this
way when using Device Tree.
> >> For non-emergency merging though:
> >> Acked-by: Linus Walleij <[email protected]>
> >
> > If we don't put this into v3.10-rcX, then the GPADC driver will be
> > broken in v3.10 when booting with DT.
>
> OK I think I understand why now...
>
> >> But for this to make sense the AB8500 ADC driver needs
> >> to be augmented for DT probing and preferrably also moved
> >> to drivers/adc and made to utilize that subsystem.
> >
> > Not sure I understand the reasoning for this. It works well as a plain
> > MFD device.
>
> The commit message says:
> "Without it, the driver won't be able to interrogate the Device Tree or
> locate suitable regulators and will most likely fail to probe."
>
> That commit message makes it sound like the driver is
> interrogating (hm is that the right term...) the device tree, but
> now you say it isn't (as you say it is a plain MFD device).
> Of course the MFD core and drivers/of/platform.c is doing
> all sorts of magic based on the device tree, and that is
> what the patch fixes, right? Nothing to do with the driver
> really?
I guess you're kind of correct. Yes, it's not the actual driver that's
interrogating (yes it's the right term) the Device Tree, other
subsystems are doing it on the driver's behalf. But the driver's node
is being used, which is why we need the compatible string.
> I suspect the missing regulators due to how the OF core or
> MFD core uses the device tree is the actual problem
> solved by the patch?
>
> Anyway I think I was after this:
>
> Isn't the idea that all devices that get probed from a device tree
> shall have corresponding bindings documented under
> Documentation/devicetree/bindings/*?
It has an entry in Documentation/devicetree/bindings/mfd/ab8500.txt.
> And this driver is definately in the wrong subsystem (not your
> fault) so I suspect the binding doc should be in
> Documentation/devicetree/bindings/adc/ab8500.txt or
> something.
All of the AB8500 devices are covered in the aforementioned file and
to my knowledge none of them are represented in their respective
'real' driver type's binding doc folders (except Power).
> I'm not pointing that out as a problem in this
> patch, I'm just discussing the mess we're in ...
Sure.
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Quoting Lee Jones (2013-05-08 06:29:03)
> First Ethernet device has a ".0" appended onto the device name. It
> appears that we need this in order to obtain the correct clock.
>
> Cc: Ulf Hansson <[email protected]>
> Cc: Mike Turquette <[email protected]>
> Signed-off-by: Lee Jones <[email protected]>
Looks good to me. Can I take this into clk-next or are you looking for
an Ack?
Regards,
Mike
> ---
> drivers/clk/ux500/u8500_clk.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/clk/ux500/u8500_clk.c b/drivers/clk/ux500/u8500_clk.c
> index 0b4f35a..80069c3 100644
> --- a/drivers/clk/ux500/u8500_clk.c
> +++ b/drivers/clk/ux500/u8500_clk.c
> @@ -325,7 +325,7 @@ void u8500_clk_init(u32 clkrst1_base, u32 clkrst2_base, u32 clkrst3_base,
> clk = clk_reg_prcc_pclk("p3_pclk0", "per3clk", clkrst3_base,
> BIT(0), 0);
> clk_register_clkdev(clk, "fsmc", NULL);
> - clk_register_clkdev(clk, NULL, "smsc911x");
> + clk_register_clkdev(clk, NULL, "smsc911x.0");
>
> clk = clk_reg_prcc_pclk("p3_pclk1", "per3clk", clkrst3_base,
> BIT(1), 0);
> --
> 1.7.10.4
On Tue, May 14, 2013 at 5:58 PM, Mike Turquette <[email protected]> wrote:
> Quoting Lee Jones (2013-05-08 06:29:03)
>> First Ethernet device has a ".0" appended onto the device name. It
>> appears that we need this in order to obtain the correct clock.
>>
>> Cc: Ulf Hansson <[email protected]>
>> Cc: Mike Turquette <[email protected]>
>> Signed-off-by: Lee Jones <[email protected]>
>
> Looks good to me. Can I take this into clk-next or are you looking for
> an Ack?
You can probably take it through the clk tree, but this series
deals with regression fixes so it should go into the -rc series
I think.
Yours,
Linus Walleij
On Wed, 15 May 2013, Linus Walleij wrote:
> On Tue, May 14, 2013 at 5:58 PM, Mike Turquette <[email protected]> wrote:
> > Quoting Lee Jones (2013-05-08 06:29:03)
> >> First Ethernet device has a ".0" appended onto the device name. It
> >> appears that we need this in order to obtain the correct clock.
> >>
> >> Cc: Ulf Hansson <[email protected]>
> >> Cc: Mike Turquette <[email protected]>
> >> Signed-off-by: Lee Jones <[email protected]>
> >
> > Looks good to me. Can I take this into clk-next or are you looking for
> > an Ack?
>
> You can probably take it through the clk tree, but this series
> deals with regression fixes so it should go into the -rc series
> I think.
That's what Mike has done.
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Hi Mike,
> > First Ethernet device has a ".0" appended onto the device name. It
> > appears that we need this in order to obtain the correct clock.
> >
> > Cc: Ulf Hansson <[email protected]>
> > Cc: Mike Turquette <[email protected]>
> > Signed-off-by: Lee Jones <[email protected]>
>
> Acked-by: Ulf Hansson <[email protected]>
> Acked-by: Linus Walleij <[email protected]>
It appears this one slipped through the gaps.
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Mon, 10 Jun 2013, Lee Jones wrote:
> Hi Mike,
>
> > > First Ethernet device has a ".0" appended onto the device name. It
> > > appears that we need this in order to obtain the correct clock.
> > >
> > > Cc: Ulf Hansson <[email protected]>
> > > Cc: Mike Turquette <[email protected]>
> > > Signed-off-by: Lee Jones <[email protected]>
> >
> > Acked-by: Ulf Hansson <[email protected]>
> > Acked-by: Linus Walleij <[email protected]>
>
> It appears this one slipped through the gaps.
Ah, I'm telling lies of course.
It _is_ in -next. Thanks Mike.
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog