| Message ID | 20180604175911.799-1-marek.vasut+renesas@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jun 04, 2018 at 07:59:11PM +0200, Marek Vasut wrote: > Rather than hard-coding the quirk topology, which stopped scaling, > parse the information from DT. The code looks for all compatible > PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied > to the same pin. If so, the code sends a matching sequence to the > PMIC to deassert the IRQ. > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > Cc: Simon Horman <horms+renesas@verge.net.au> > Cc: Wolfram Sang <wsa+renesas@sang-engineering.com> > Cc: linux-renesas-soc@vger.kernel.org > --- > V2: - Replace the DT shared IRQ check loop with memcmp() > - Send the I2C message to deassert the IRQ line to all PMICs > in the list with shared IRQ line instead of just one > - Add comment that this works only in case all the PMICs are > on the same I2C bus > V3: - Drop the addr = 0x00 init > - Drop reinit of argsa in rcar_gen2_regulator_quirk > --- > arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c | 114 ++++++++++++++++----- > 1 file changed, 90 insertions(+), 24 deletions(-) > > diff --git a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c > index 93f628acfd94..b919073aa27e 100644 > --- a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c > +++ b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c > @@ -31,8 +31,10 @@ > #include <linux/i2c.h> > #include <linux/init.h> > #include <linux/io.h> > +#include <linux/list.h> > #include <linux/notifier.h> > #include <linux/of.h> > +#include <linux/of_irq.h> > #include <linux/mfd/da9063/registers.h> > > > @@ -44,34 +46,45 @@ > /* start of DA9210 System Control and Event Registers */ > #define DA9210_REG_MASK_A 0x54 > > +struct regulator_quirk { > + struct list_head list; > + const struct of_device_id *id; > + struct of_phandle_args irq_args; > + struct i2c_msg i2c_msg; > + bool shared; /* IRQ line is shared */ > +}; > + > +static LIST_HEAD(quirk_list); > static void __iomem *irqc; > > /* first byte sets the memory pointer, following are consecutive reg values */ > static u8 da9063_irq_clr[] = { DA9063_REG_IRQ_MASK_A, 0xff, 0xff, 0xff, 0xff }; > static u8 da9210_irq_clr[] = { DA9210_REG_MASK_A, 0xff, 0xff }; > > -static struct i2c_msg da9xxx_msgs[3] = { > - { > - .addr = 0x58, > - .len = ARRAY_SIZE(da9063_irq_clr), > - .buf = da9063_irq_clr, > - }, { > - .addr = 0x68, > - .len = ARRAY_SIZE(da9210_irq_clr), > - .buf = da9210_irq_clr, > - }, { > - .addr = 0x70, > - .len = ARRAY_SIZE(da9210_irq_clr), > - .buf = da9210_irq_clr, > - }, > +static struct i2c_msg da9063_msgs = { > + .len = ARRAY_SIZE(da9063_irq_clr), > + .buf = da9063_irq_clr, > +}; > + > +static struct i2c_msg da9210_msgs = { > + .len = ARRAY_SIZE(da9210_irq_clr), > + .buf = da9210_irq_clr, > +}; > + > +static const struct of_device_id rcar_gen2_quirk_match[] = { > + { .compatible = "dlg,da9063", .data = &da9063_msgs }, > + { .compatible = "dlg,da9210", .data = &da9210_msgs }, > + {}, > }; > > static int regulator_quirk_notify(struct notifier_block *nb, > unsigned long action, void *data) > { > + struct regulator_quirk *pos, *tmp; > struct device *dev = data; > struct i2c_client *client; > static bool done; > + int ret; > u32 mon; > > if (done) > @@ -88,17 +101,20 @@ static int regulator_quirk_notify(struct notifier_block *nb, > client = to_i2c_client(dev); > dev_dbg(dev, "Detected %s\n", client->name); > > - if ((client->addr == 0x58 && !strcmp(client->name, "da9063")) || > - (client->addr == 0x68 && !strcmp(client->name, "da9210")) || > - (client->addr == 0x70 && !strcmp(client->name, "da9210"))) { > - int ret, len; > + /* > + * Send message to all PMICs that share an IRQ line to deassert it. > + * > + * WARNING: This works only if all the PMICs are on the same I2C bus. > + */ > + list_for_each_entry(pos, &quirk_list, list) { > + if (!pos->shared) > + continue; > > - /* There are two DA9210 on Stout, one on the other boards. */ > - len = of_machine_is_compatible("renesas,stout") ? 3 : 2; > + dev_info(&client->dev, "clearing %s@0x%02x interrupts\n", > + pos->id->compatible, pos->i2c_msg.addr); > > - dev_info(&client->dev, "clearing da9063/da9210 interrupts\n"); > - ret = i2c_transfer(client->adapter, da9xxx_msgs, len); > - if (ret != len) > + ret = i2c_transfer(client->adapter, &pos->i2c_msg, 1); > + if (ret != 1) > dev_err(&client->dev, "i2c error %d\n", ret); > } > > @@ -111,6 +127,11 @@ static int regulator_quirk_notify(struct notifier_block *nb, > remove: > dev_info(dev, "IRQ2 is not asserted, removing quirk\n"); > > + list_for_each_entry_safe(pos, tmp, &quirk_list, list) { > + list_del(&pos->list); > + kfree(pos); > + } > + > done = true; > iounmap(irqc); > return 0; > @@ -122,7 +143,13 @@ static struct notifier_block regulator_quirk_nb = { > > static int __init rcar_gen2_regulator_quirk(void) > { > - u32 mon; > + struct device_node *np; > + const struct of_device_id *id; > + struct regulator_quirk *quirk; > + struct regulator_quirk *pos; > + struct of_phandle_args *argsa, *argsb; > + u32 mon, addr; > + int ret; > > if (!of_machine_is_compatible("renesas,koelsch") && > !of_machine_is_compatible("renesas,lager") && > @@ -130,6 +157,45 @@ static int __init rcar_gen2_regulator_quirk(void) > !of_machine_is_compatible("renesas,gose")) > return -ENODEV; > > + for_each_matching_node_and_match(np, rcar_gen2_quirk_match, &id) { > + if (!np || !of_device_is_available(np)) > + break; > + > + quirk = kzalloc(sizeof(*quirk), GFP_KERNEL); > + > + argsa = &quirk->irq_args; > + memcpy(&quirk->i2c_msg, id->data, sizeof(quirk->i2c_msg)); > + > + ret = of_property_read_u32(np, "reg", &addr); > + if (ret) > + return ret; > + > + quirk->id = id; > + quirk->i2c_msg.addr = addr; > + quirk->shared = false; > + > + ret = of_irq_parse_one(np, 0, &quirk->irq_args); As per my comment on v2, &quirk->irq_args is assigned to argsa above and used directly here. > + if (ret) > + return ret; > + > + list_for_each_entry(pos, &quirk_list, list) { > + argsb = &pos->irq_args; > + > + if (argsa->args_count != argsb->args_count) > + continue; > + > + ret = memcmp(argsa->args, argsb->args, > + argsa->args_count * > + sizeof(argsa->args[0])); > + if (!ret) { > + pos->shared = true; > + quirk->shared = true; > + } > + } > + > + list_add_tail(&quirk->list, &quirk_list); > + } > + > irqc = ioremap(IRQC_BASE, PAGE_SIZE); > if (!irqc) > return -ENOMEM; > -- > 2.16.2 >
Hi Marek, On Tue, Jun 05, 2018 at 10:07:28AM +0200, Simon Horman wrote: > On Mon, Jun 04, 2018 at 07:59:11PM +0200, Marek Vasut wrote: > > Rather than hard-coding the quirk topology, which stopped scaling, > > parse the information from DT. The code looks for all compatible > > PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied > > to the same pin. If so, the code sends a matching sequence to the > > PMIC to deassert the IRQ. > > > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > > Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > > Cc: Simon Horman <horms+renesas@verge.net.au> > > Cc: Wolfram Sang <wsa+renesas@sang-engineering.com> From an I2C point of view: Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Minor nits: > > @@ -122,7 +143,13 @@ static struct notifier_block regulator_quirk_nb = { > > > > static int __init rcar_gen2_regulator_quirk(void) > > { > > - u32 mon; > > + struct device_node *np; > > + const struct of_device_id *id; > > + struct regulator_quirk *quirk; > > + struct regulator_quirk *pos; Merge the last two lines into one? > > + struct of_phandle_args *argsa, *argsb; > > + u32 mon, addr; > > + int ret; > > > > if (!of_machine_is_compatible("renesas,koelsch") && > > !of_machine_is_compatible("renesas,lager") && > > @@ -130,6 +157,45 @@ static int __init rcar_gen2_regulator_quirk(void) > > !of_machine_is_compatible("renesas,gose")) > > return -ENODEV; > > > > + for_each_matching_node_and_match(np, rcar_gen2_quirk_match, &id) { > > + if (!np || !of_device_is_available(np)) Can '!np' actually happen? This is the exit condition of the for-loop, or am I overlooking something? Regards, Wolfram
On 06/05/2018 10:07 AM, Simon Horman wrote: > On Mon, Jun 04, 2018 at 07:59:11PM +0200, Marek Vasut wrote: >> Rather than hard-coding the quirk topology, which stopped scaling, >> parse the information from DT. The code looks for all compatible >> PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied >> to the same pin. If so, the code sends a matching sequence to the >> PMIC to deassert the IRQ. >> >> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> >> Cc: Geert Uytterhoeven <geert+renesas@glider.be> >> Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> >> Cc: Simon Horman <horms+renesas@verge.net.au> >> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com> >> Cc: linux-renesas-soc@vger.kernel.org >> --- >> V2: - Replace the DT shared IRQ check loop with memcmp() >> - Send the I2C message to deassert the IRQ line to all PMICs >> in the list with shared IRQ line instead of just one >> - Add comment that this works only in case all the PMICs are >> on the same I2C bus >> V3: - Drop the addr = 0x00 init >> - Drop reinit of argsa in rcar_gen2_regulator_quirk >> --- >> arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c | 114 ++++++++++++++++----- >> 1 file changed, 90 insertions(+), 24 deletions(-) >> >> diff --git a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c >> index 93f628acfd94..b919073aa27e 100644 >> --- a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c >> +++ b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c >> @@ -31,8 +31,10 @@ >> #include <linux/i2c.h> >> #include <linux/init.h> >> #include <linux/io.h> >> +#include <linux/list.h> >> #include <linux/notifier.h> >> #include <linux/of.h> >> +#include <linux/of_irq.h> >> #include <linux/mfd/da9063/registers.h> >> >> >> @@ -44,34 +46,45 @@ >> /* start of DA9210 System Control and Event Registers */ >> #define DA9210_REG_MASK_A 0x54 >> >> +struct regulator_quirk { >> + struct list_head list; >> + const struct of_device_id *id; >> + struct of_phandle_args irq_args; >> + struct i2c_msg i2c_msg; >> + bool shared; /* IRQ line is shared */ >> +}; >> + >> +static LIST_HEAD(quirk_list); >> static void __iomem *irqc; >> >> /* first byte sets the memory pointer, following are consecutive reg values */ >> static u8 da9063_irq_clr[] = { DA9063_REG_IRQ_MASK_A, 0xff, 0xff, 0xff, 0xff }; >> static u8 da9210_irq_clr[] = { DA9210_REG_MASK_A, 0xff, 0xff }; >> >> -static struct i2c_msg da9xxx_msgs[3] = { >> - { >> - .addr = 0x58, >> - .len = ARRAY_SIZE(da9063_irq_clr), >> - .buf = da9063_irq_clr, >> - }, { >> - .addr = 0x68, >> - .len = ARRAY_SIZE(da9210_irq_clr), >> - .buf = da9210_irq_clr, >> - }, { >> - .addr = 0x70, >> - .len = ARRAY_SIZE(da9210_irq_clr), >> - .buf = da9210_irq_clr, >> - }, >> +static struct i2c_msg da9063_msgs = { >> + .len = ARRAY_SIZE(da9063_irq_clr), >> + .buf = da9063_irq_clr, >> +}; >> + >> +static struct i2c_msg da9210_msgs = { >> + .len = ARRAY_SIZE(da9210_irq_clr), >> + .buf = da9210_irq_clr, >> +}; >> + >> +static const struct of_device_id rcar_gen2_quirk_match[] = { >> + { .compatible = "dlg,da9063", .data = &da9063_msgs }, >> + { .compatible = "dlg,da9210", .data = &da9210_msgs }, >> + {}, >> }; >> >> static int regulator_quirk_notify(struct notifier_block *nb, >> unsigned long action, void *data) >> { >> + struct regulator_quirk *pos, *tmp; >> struct device *dev = data; >> struct i2c_client *client; >> static bool done; >> + int ret; >> u32 mon; >> >> if (done) >> @@ -88,17 +101,20 @@ static int regulator_quirk_notify(struct notifier_block *nb, >> client = to_i2c_client(dev); >> dev_dbg(dev, "Detected %s\n", client->name); >> >> - if ((client->addr == 0x58 && !strcmp(client->name, "da9063")) || >> - (client->addr == 0x68 && !strcmp(client->name, "da9210")) || >> - (client->addr == 0x70 && !strcmp(client->name, "da9210"))) { >> - int ret, len; >> + /* >> + * Send message to all PMICs that share an IRQ line to deassert it. >> + * >> + * WARNING: This works only if all the PMICs are on the same I2C bus. >> + */ >> + list_for_each_entry(pos, &quirk_list, list) { >> + if (!pos->shared) >> + continue; >> >> - /* There are two DA9210 on Stout, one on the other boards. */ >> - len = of_machine_is_compatible("renesas,stout") ? 3 : 2; >> + dev_info(&client->dev, "clearing %s@0x%02x interrupts\n", >> + pos->id->compatible, pos->i2c_msg.addr); >> >> - dev_info(&client->dev, "clearing da9063/da9210 interrupts\n"); >> - ret = i2c_transfer(client->adapter, da9xxx_msgs, len); >> - if (ret != len) >> + ret = i2c_transfer(client->adapter, &pos->i2c_msg, 1); >> + if (ret != 1) >> dev_err(&client->dev, "i2c error %d\n", ret); >> } >> >> @@ -111,6 +127,11 @@ static int regulator_quirk_notify(struct notifier_block *nb, >> remove: >> dev_info(dev, "IRQ2 is not asserted, removing quirk\n"); >> >> + list_for_each_entry_safe(pos, tmp, &quirk_list, list) { >> + list_del(&pos->list); >> + kfree(pos); >> + } >> + >> done = true; >> iounmap(irqc); >> return 0; >> @@ -122,7 +143,13 @@ static struct notifier_block regulator_quirk_nb = { >> >> static int __init rcar_gen2_regulator_quirk(void) >> { >> - u32 mon; >> + struct device_node *np; >> + const struct of_device_id *id; >> + struct regulator_quirk *quirk; >> + struct regulator_quirk *pos; >> + struct of_phandle_args *argsa, *argsb; >> + u32 mon, addr; >> + int ret; >> >> if (!of_machine_is_compatible("renesas,koelsch") && >> !of_machine_is_compatible("renesas,lager") && >> @@ -130,6 +157,45 @@ static int __init rcar_gen2_regulator_quirk(void) >> !of_machine_is_compatible("renesas,gose")) >> return -ENODEV; >> >> + for_each_matching_node_and_match(np, rcar_gen2_quirk_match, &id) { >> + if (!np || !of_device_is_available(np)) >> + break; >> + >> + quirk = kzalloc(sizeof(*quirk), GFP_KERNEL); >> + >> + argsa = &quirk->irq_args; >> + memcpy(&quirk->i2c_msg, id->data, sizeof(quirk->i2c_msg)); >> + >> + ret = of_property_read_u32(np, "reg", &addr); >> + if (ret) >> + return ret; >> + >> + quirk->id = id; >> + quirk->i2c_msg.addr = addr; >> + quirk->shared = false; >> + >> + ret = of_irq_parse_one(np, 0, &quirk->irq_args); > > As per my comment on v2, > &quirk->irq_args is assigned to argsa above and used directly here. > Hum, OK
On 06/05/2018 11:21 AM, Wolfram Sang wrote: > Hi Marek, > > On Tue, Jun 05, 2018 at 10:07:28AM +0200, Simon Horman wrote: >> On Mon, Jun 04, 2018 at 07:59:11PM +0200, Marek Vasut wrote: >>> Rather than hard-coding the quirk topology, which stopped scaling, >>> parse the information from DT. The code looks for all compatible >>> PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied >>> to the same pin. If so, the code sends a matching sequence to the >>> PMIC to deassert the IRQ. >>> >>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> >>> Cc: Geert Uytterhoeven <geert+renesas@glider.be> >>> Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> >>> Cc: Simon Horman <horms+renesas@verge.net.au> >>> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com> > > From an I2C point of view: > > Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > > Minor nits: > >>> @@ -122,7 +143,13 @@ static struct notifier_block regulator_quirk_nb = { >>> >>> static int __init rcar_gen2_regulator_quirk(void) >>> { >>> - u32 mon; >>> + struct device_node *np; >>> + const struct of_device_id *id; >>> + struct regulator_quirk *quirk; >>> + struct regulator_quirk *pos; > > Merge the last two lines into one? > >>> + struct of_phandle_args *argsa, *argsb; >>> + u32 mon, addr; >>> + int ret; >>> >>> if (!of_machine_is_compatible("renesas,koelsch") && >>> !of_machine_is_compatible("renesas,lager") && >>> @@ -130,6 +157,45 @@ static int __init rcar_gen2_regulator_quirk(void) >>> !of_machine_is_compatible("renesas,gose")) >>> return -ENODEV; >>> >>> + for_each_matching_node_and_match(np, rcar_gen2_quirk_match, &id) { >>> + if (!np || !of_device_is_available(np)) > > Can '!np' actually happen? This is the exit condition of the for-loop, > or am I overlooking something? I had to take a look again, no, it's not needed.
Hi Marek, On Mon, Jun 4, 2018 at 7:59 PM Marek Vasut <marek.vasut@gmail.com> wrote: > Rather than hard-coding the quirk topology, which stopped scaling, > parse the information from DT. The code looks for all compatible > PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied da9063 > to the same pin. If so, the code sends a matching sequence to the > PMIC to deassert the IRQ. Note that not all R-Car Gen2 boards have all regulators described in DT yet. E.g. gose lacks da9210. So that has to be fixed first. > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > Cc: Simon Horman <horms+renesas@verge.net.au> > Cc: Wolfram Sang <wsa+renesas@sang-engineering.com> > Cc: linux-renesas-soc@vger.kernel.org > --- > V2: - Replace the DT shared IRQ check loop with memcmp() > - Send the I2C message to deassert the IRQ line to all PMICs > in the list with shared IRQ line instead of just one > - Add comment that this works only in case all the PMICs are > on the same I2C bus > V3: - Drop the addr = 0x00 init > - Drop reinit of argsa in rcar_gen2_regulator_quirk Thanks for the update! > --- a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c > +++ b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c > @@ -44,34 +46,45 @@ > /* start of DA9210 System Control and Event Registers */ > #define DA9210_REG_MASK_A 0x54 > > +struct regulator_quirk { > + struct list_head list; > + const struct of_device_id *id; > + struct of_phandle_args irq_args; > + struct i2c_msg i2c_msg; > + bool shared; /* IRQ line is shared */ > +}; > + > +static LIST_HEAD(quirk_list); > static void __iomem *irqc; > > /* first byte sets the memory pointer, following are consecutive reg values */ > static u8 da9063_irq_clr[] = { DA9063_REG_IRQ_MASK_A, 0xff, 0xff, 0xff, 0xff }; > static u8 da9210_irq_clr[] = { DA9210_REG_MASK_A, 0xff, 0xff }; > > -static struct i2c_msg da9xxx_msgs[3] = { > - { > - .addr = 0x58, > - .len = ARRAY_SIZE(da9063_irq_clr), > - .buf = da9063_irq_clr, > - }, { > - .addr = 0x68, > - .len = ARRAY_SIZE(da9210_irq_clr), > - .buf = da9210_irq_clr, > - }, { > - .addr = 0x70, > - .len = ARRAY_SIZE(da9210_irq_clr), > - .buf = da9210_irq_clr, > - }, > +static struct i2c_msg da9063_msgs = { da9063_msg? (it's a single message) > + .len = ARRAY_SIZE(da9063_irq_clr), > + .buf = da9063_irq_clr, > +}; > + > +static struct i2c_msg da9210_msgs = { da9210_msg? > + .len = ARRAY_SIZE(da9210_irq_clr), > + .buf = da9210_irq_clr, > +}; > @@ -122,7 +143,13 @@ static struct notifier_block regulator_quirk_nb = { > > static int __init rcar_gen2_regulator_quirk(void) > { > - u32 mon; > + struct device_node *np; > + const struct of_device_id *id; > + struct regulator_quirk *quirk; > + struct regulator_quirk *pos; > + struct of_phandle_args *argsa, *argsb; > + u32 mon, addr; > + int ret; Some people prefer "Reverse Christmas Tree Ordering", i.e. longest line first. > > if (!of_machine_is_compatible("renesas,koelsch") && > !of_machine_is_compatible("renesas,lager") && > @@ -130,6 +157,45 @@ static int __init rcar_gen2_regulator_quirk(void) > !of_machine_is_compatible("renesas,gose")) > return -ENODEV; I think the board checks above can be removed. That will auto-enable the fix on e.g. Porter (once its regulators have ended up in DTS, of course). > > + for_each_matching_node_and_match(np, rcar_gen2_quirk_match, &id) { > + if (!np || !of_device_is_available(np)) !np cannot happen > + break; > + > + quirk = kzalloc(sizeof(*quirk), GFP_KERNEL); Missing NULL check > + > + argsa = &quirk->irq_args; > + memcpy(&quirk->i2c_msg, id->data, sizeof(quirk->i2c_msg)); > + > + ret = of_property_read_u32(np, "reg", &addr); > + if (ret) > + return ret; I think it's safer to skip this entry and continue, after calling kfree(quirk), of course. > + > + quirk->id = id; > + quirk->i2c_msg.addr = addr; > + quirk->shared = false; No need to clear shared, it was cleared by kzalloc(). > + > + ret = of_irq_parse_one(np, 0, &quirk->irq_args); > + if (ret) > + return ret; kfree(quirk) and continue... Works fine on Koelsch, so Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 06/11/2018 11:56 AM, Geert Uytterhoeven wrote: > Hi Marek, Hi, > On Mon, Jun 4, 2018 at 7:59 PM Marek Vasut <marek.vasut@gmail.com> wrote: >> Rather than hard-coding the quirk topology, which stopped scaling, >> parse the information from DT. The code looks for all compatible >> PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied > > da9063 > >> to the same pin. If so, the code sends a matching sequence to the >> PMIC to deassert the IRQ. > > Note that not all R-Car Gen2 boards have all regulators described in DT yet. > E.g. gose lacks da9210. So that has to be fixed first. [...] > da9210_msg? Fixed > >> + .len = ARRAY_SIZE(da9210_irq_clr), >> + .buf = da9210_irq_clr, >> +}; > >> @@ -122,7 +143,13 @@ static struct notifier_block regulator_quirk_nb = { >> >> static int __init rcar_gen2_regulator_quirk(void) >> { >> - u32 mon; >> + struct device_node *np; >> + const struct of_device_id *id; >> + struct regulator_quirk *quirk; >> + struct regulator_quirk *pos; >> + struct of_phandle_args *argsa, *argsb; >> + u32 mon, addr; >> + int ret; > > Some people prefer "Reverse Christmas Tree Ordering", i.e. longest line first. > >> >> if (!of_machine_is_compatible("renesas,koelsch") && >> !of_machine_is_compatible("renesas,lager") && >> @@ -130,6 +157,45 @@ static int __init rcar_gen2_regulator_quirk(void) >> !of_machine_is_compatible("renesas,gose")) >> return -ENODEV; > > I think the board checks above can be removed. That will auto-enable the > fix on e.g. Porter (once its regulators have ended up in DTS, of course). Removing the check would also enable it on boards where we don't want this enabled, so I'd prefer to keep the check to avoid strange surprises. >> >> + for_each_matching_node_and_match(np, rcar_gen2_quirk_match, &id) { >> + if (!np || !of_device_is_available(np)) > > !np cannot happen Right >> + break; >> + >> + quirk = kzalloc(sizeof(*quirk), GFP_KERNEL); > > Missing NULL check > >> + >> + argsa = &quirk->irq_args; >> + memcpy(&quirk->i2c_msg, id->data, sizeof(quirk->i2c_msg)); >> + >> + ret = of_property_read_u32(np, "reg", &addr); >> + if (ret) >> + return ret; > > I think it's safer to skip this entry and continue, after calling > kfree(quirk), of course. > >> + >> + quirk->id = id; >> + quirk->i2c_msg.addr = addr; >> + quirk->shared = false; > > No need to clear shared, it was cleared by kzalloc(). > >> + >> + ret = of_irq_parse_one(np, 0, &quirk->irq_args); >> + if (ret) >> + return ret; > > kfree(quirk) and continue... > I wonder if it shouldn't rather free the entire list and abort ? > Works fine on Koelsch, so > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds >
Hi Marek, On Mon, Jun 11, 2018 at 2:15 PM Marek Vasut <marek.vasut@gmail.com> wrote: > On 06/11/2018 11:56 AM, Geert Uytterhoeven wrote: > > On Mon, Jun 4, 2018 at 7:59 PM Marek Vasut <marek.vasut@gmail.com> wrote: > >> Rather than hard-coding the quirk topology, which stopped scaling, > >> parse the information from DT. The code looks for all compatible > >> PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied > > > > da9063 > > > >> to the same pin. If so, the code sends a matching sequence to the > >> PMIC to deassert the IRQ. > >> @@ -122,7 +143,13 @@ static struct notifier_block regulator_quirk_nb = { > >> > >> static int __init rcar_gen2_regulator_quirk(void) > >> { > >> - u32 mon; > >> + struct device_node *np; > >> + const struct of_device_id *id; > >> + struct regulator_quirk *quirk; > >> + struct regulator_quirk *pos; > >> + struct of_phandle_args *argsa, *argsb; > >> + u32 mon, addr; > >> + int ret; > > > > Some people prefer "Reverse Christmas Tree Ordering", i.e. longest line first. > > > >> > >> if (!of_machine_is_compatible("renesas,koelsch") && > >> !of_machine_is_compatible("renesas,lager") && > >> @@ -130,6 +157,45 @@ static int __init rcar_gen2_regulator_quirk(void) > >> !of_machine_is_compatible("renesas,gose")) > >> return -ENODEV; > > > > I think the board checks above can be removed. That will auto-enable the > > fix on e.g. Porter (once its regulators have ended up in DTS, of course). > > Removing the check would also enable it on boards where we don't want > this enabled, so I'd prefer to keep the check to avoid strange surprises. Like, Porter? ;-) > >> + ret = of_property_read_u32(np, "reg", &addr); > >> + if (ret) > >> + return ret; > > > > I think it's safer to skip this entry and continue, after calling > > kfree(quirk), of course. > > > >> + > >> + quirk->id = id; > >> + quirk->i2c_msg.addr = addr; > >> + quirk->shared = false; > >> + > >> + ret = of_irq_parse_one(np, 0, &quirk->irq_args); > >> + if (ret) > >> + return ret; > > > > kfree(quirk) and continue... > > I wonder if it shouldn't rather free the entire list and abort ? "Be strict when sending, be liberal when receiving." Gr{oetje,eeting}s, Geert
On 06/11/2018 03:03 PM, Geert Uytterhoeven wrote: > Hi Marek, Hi, > On Mon, Jun 11, 2018 at 2:15 PM Marek Vasut <marek.vasut@gmail.com> wrote: >> On 06/11/2018 11:56 AM, Geert Uytterhoeven wrote: >>> On Mon, Jun 4, 2018 at 7:59 PM Marek Vasut <marek.vasut@gmail.com> wrote: >>>> Rather than hard-coding the quirk topology, which stopped scaling, >>>> parse the information from DT. The code looks for all compatible >>>> PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied >>> >>> da9063 >>> >>>> to the same pin. If so, the code sends a matching sequence to the >>>> PMIC to deassert the IRQ. > >>>> @@ -122,7 +143,13 @@ static struct notifier_block regulator_quirk_nb = { >>>> >>>> static int __init rcar_gen2_regulator_quirk(void) >>>> { >>>> - u32 mon; >>>> + struct device_node *np; >>>> + const struct of_device_id *id; >>>> + struct regulator_quirk *quirk; >>>> + struct regulator_quirk *pos; >>>> + struct of_phandle_args *argsa, *argsb; >>>> + u32 mon, addr; >>>> + int ret; >>> >>> Some people prefer "Reverse Christmas Tree Ordering", i.e. longest line first. >>> >>>> >>>> if (!of_machine_is_compatible("renesas,koelsch") && >>>> !of_machine_is_compatible("renesas,lager") && >>>> @@ -130,6 +157,45 @@ static int __init rcar_gen2_regulator_quirk(void) >>>> !of_machine_is_compatible("renesas,gose")) >>>> return -ENODEV; >>> >>> I think the board checks above can be removed. That will auto-enable the >>> fix on e.g. Porter (once its regulators have ended up in DTS, of course). >> >> Removing the check would also enable it on boards where we don't want >> this enabled, so I'd prefer to keep the check to avoid strange surprises. > > Like, Porter? ;-) I'm adding Porter in a separate patch. >>>> + ret = of_property_read_u32(np, "reg", &addr); >>>> + if (ret) >>>> + return ret; >>> >>> I think it's safer to skip this entry and continue, after calling >>> kfree(quirk), of course. >>> >>>> + >>>> + quirk->id = id; >>>> + quirk->i2c_msg.addr = addr; >>>> + quirk->shared = false; >>>> + >>>> + ret = of_irq_parse_one(np, 0, &quirk->irq_args); >>>> + if (ret) >>>> + return ret; >>> >>> kfree(quirk) and continue... >> >> I wonder if it shouldn't rather free the entire list and abort ? > > "Be strict when sending, be liberal when receiving." Meaning ? I think "the language barrier is protecting me" (TM)
Hi Marek, On Mon, Jun 11, 2018 at 3:39 PM Marek Vasut <marek.vasut@gmail.com> wrote: > On 06/11/2018 03:03 PM, Geert Uytterhoeven wrote: > > On Mon, Jun 11, 2018 at 2:15 PM Marek Vasut <marek.vasut@gmail.com> wrote: > >> On 06/11/2018 11:56 AM, Geert Uytterhoeven wrote: > >>> On Mon, Jun 4, 2018 at 7:59 PM Marek Vasut <marek.vasut@gmail.com> wrote: > >>>> Rather than hard-coding the quirk topology, which stopped scaling, > >>>> parse the information from DT. The code looks for all compatible > >>>> PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied > >>>> to the same pin. If so, the code sends a matching sequence to the > >>>> PMIC to deassert the IRQ. > >>>> + ret = of_property_read_u32(np, "reg", &addr); > >>>> + if (ret) > >>>> + return ret; > >>> > >>> I think it's safer to skip this entry and continue, after calling > >>> kfree(quirk), of course. > >>> > >>>> + > >>>> + quirk->id = id; > >>>> + quirk->i2c_msg.addr = addr; > >>>> + quirk->shared = false; > >>>> + > >>>> + ret = of_irq_parse_one(np, 0, &quirk->irq_args); > >>>> + if (ret) > >>>> + return ret; > >>> > >>> kfree(quirk) and continue... > >> > >> I wonder if it shouldn't rather free the entire list and abort ? > > > > "Be strict when sending, be liberal when receiving." > > Meaning ? I think "the language barrier is protecting me" (TM) Do the best you can, given the buggy DT you received. I.e. don't fail completely, just ignore the bad device node, and continue. Gr{oetje,eeting}s, Geert
On 06/11/2018 03:49 PM, Geert Uytterhoeven wrote: > Hi Marek, Hi, > On Mon, Jun 11, 2018 at 3:39 PM Marek Vasut <marek.vasut@gmail.com> wrote: >> On 06/11/2018 03:03 PM, Geert Uytterhoeven wrote: >>> On Mon, Jun 11, 2018 at 2:15 PM Marek Vasut <marek.vasut@gmail.com> wrote: >>>> On 06/11/2018 11:56 AM, Geert Uytterhoeven wrote: >>>>> On Mon, Jun 4, 2018 at 7:59 PM Marek Vasut <marek.vasut@gmail.com> wrote: >>>>>> Rather than hard-coding the quirk topology, which stopped scaling, >>>>>> parse the information from DT. The code looks for all compatible >>>>>> PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied >>>>>> to the same pin. If so, the code sends a matching sequence to the >>>>>> PMIC to deassert the IRQ. > >>>>>> + ret = of_property_read_u32(np, "reg", &addr); >>>>>> + if (ret) >>>>>> + return ret; >>>>> >>>>> I think it's safer to skip this entry and continue, after calling >>>>> kfree(quirk), of course. >>>>> >>>>>> + >>>>>> + quirk->id = id; >>>>>> + quirk->i2c_msg.addr = addr; >>>>>> + quirk->shared = false; >>>>>> + >>>>>> + ret = of_irq_parse_one(np, 0, &quirk->irq_args); >>>>>> + if (ret) >>>>>> + return ret; >>>>> >>>>> kfree(quirk) and continue... >>>> >>>> I wonder if it shouldn't rather free the entire list and abort ? >>> >>> "Be strict when sending, be liberal when receiving." >> >> Meaning ? I think "the language barrier is protecting me" (TM) > > Do the best you can, given the buggy DT you received. > I.e. don't fail completely, just ignore the bad device node, and continue. But if you ignore node, you might as well ignore one which is shared and then the system crashes due to IRQ storm anyway. So hum, what can we do ?
Hi Marek, On Mon, Jun 11, 2018 at 4:04 PM Marek Vasut <marek.vasut@gmail.com> wrote: > On 06/11/2018 03:49 PM, Geert Uytterhoeven wrote: > > On Mon, Jun 11, 2018 at 3:39 PM Marek Vasut <marek.vasut@gmail.com> wrote: > >> On 06/11/2018 03:03 PM, Geert Uytterhoeven wrote: > >>> On Mon, Jun 11, 2018 at 2:15 PM Marek Vasut <marek.vasut@gmail.com> wrote: > >>>> On 06/11/2018 11:56 AM, Geert Uytterhoeven wrote: > >>>>> On Mon, Jun 4, 2018 at 7:59 PM Marek Vasut <marek.vasut@gmail.com> wrote: > >>>>>> Rather than hard-coding the quirk topology, which stopped scaling, > >>>>>> parse the information from DT. The code looks for all compatible > >>>>>> PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied > >>>>>> to the same pin. If so, the code sends a matching sequence to the > >>>>>> PMIC to deassert the IRQ. > > > >>>>>> + ret = of_property_read_u32(np, "reg", &addr); > >>>>>> + if (ret) > >>>>>> + return ret; > >>>>> > >>>>> I think it's safer to skip this entry and continue, after calling > >>>>> kfree(quirk), of course. > >>>>> > >>>>>> + > >>>>>> + quirk->id = id; > >>>>>> + quirk->i2c_msg.addr = addr; > >>>>>> + quirk->shared = false; > >>>>>> + > >>>>>> + ret = of_irq_parse_one(np, 0, &quirk->irq_args); > >>>>>> + if (ret) > >>>>>> + return ret; > >>>>> > >>>>> kfree(quirk) and continue... > >>>> > >>>> I wonder if it shouldn't rather free the entire list and abort ? > >>> > >>> "Be strict when sending, be liberal when receiving." > >> > >> Meaning ? I think "the language barrier is protecting me" (TM) > > > > Do the best you can, given the buggy DT you received. > > I.e. don't fail completely, just ignore the bad device node, and continue. > > But if you ignore node, you might as well ignore one which is shared and > then the system crashes due to IRQ storm anyway. So hum, what can we do ? Correct. If it's a critical node, it will crash regardless. If it's a non-critical node, you have the choice between aborting and crashing, or ignoring and keeping the system alive. Your call. Gr{oetje,eeting}s, Geert
On 06/11/2018 04:10 PM, Geert Uytterhoeven wrote: > Hi Marek, Hi, > On Mon, Jun 11, 2018 at 4:04 PM Marek Vasut <marek.vasut@gmail.com> wrote: >> On 06/11/2018 03:49 PM, Geert Uytterhoeven wrote: >>> On Mon, Jun 11, 2018 at 3:39 PM Marek Vasut <marek.vasut@gmail.com> wrote: >>>> On 06/11/2018 03:03 PM, Geert Uytterhoeven wrote: >>>>> On Mon, Jun 11, 2018 at 2:15 PM Marek Vasut <marek.vasut@gmail.com> wrote: >>>>>> On 06/11/2018 11:56 AM, Geert Uytterhoeven wrote: >>>>>>> On Mon, Jun 4, 2018 at 7:59 PM Marek Vasut <marek.vasut@gmail.com> wrote: >>>>>>>> Rather than hard-coding the quirk topology, which stopped scaling, >>>>>>>> parse the information from DT. The code looks for all compatible >>>>>>>> PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied >>>>>>>> to the same pin. If so, the code sends a matching sequence to the >>>>>>>> PMIC to deassert the IRQ. >>> >>>>>>>> + ret = of_property_read_u32(np, "reg", &addr); >>>>>>>> + if (ret) >>>>>>>> + return ret; >>>>>>> >>>>>>> I think it's safer to skip this entry and continue, after calling >>>>>>> kfree(quirk), of course. >>>>>>> >>>>>>>> + >>>>>>>> + quirk->id = id; >>>>>>>> + quirk->i2c_msg.addr = addr; >>>>>>>> + quirk->shared = false; >>>>>>>> + >>>>>>>> + ret = of_irq_parse_one(np, 0, &quirk->irq_args); >>>>>>>> + if (ret) >>>>>>>> + return ret; >>>>>>> >>>>>>> kfree(quirk) and continue... >>>>>> >>>>>> I wonder if it shouldn't rather free the entire list and abort ? >>>>> >>>>> "Be strict when sending, be liberal when receiving." >>>> >>>> Meaning ? I think "the language barrier is protecting me" (TM) >>> >>> Do the best you can, given the buggy DT you received. >>> I.e. don't fail completely, just ignore the bad device node, and continue. >> >> But if you ignore node, you might as well ignore one which is shared and >> then the system crashes due to IRQ storm anyway. So hum, what can we do ? > > Correct. If it's a critical node, it will crash regardless. > If it's a non-critical node, you have the choice between aborting and crashing, > or ignoring and keeping the system alive. Your call. But wait, since we control which machines this code runs on , can't we assure they have valid DTs ? This situation with invalid DT starts to look a bit hypothetical to me.
Hi Marek, On Mon, Jun 11, 2018 at 4:19 PM Marek Vasut <marek.vasut@gmail.com> wrote: > On 06/11/2018 04:10 PM, Geert Uytterhoeven wrote: > > On Mon, Jun 11, 2018 at 4:04 PM Marek Vasut <marek.vasut@gmail.com> wrote: > >> On 06/11/2018 03:49 PM, Geert Uytterhoeven wrote: > >>> On Mon, Jun 11, 2018 at 3:39 PM Marek Vasut <marek.vasut@gmail.com> wrote: > >>>> On 06/11/2018 03:03 PM, Geert Uytterhoeven wrote: > >>>>> On Mon, Jun 11, 2018 at 2:15 PM Marek Vasut <marek.vasut@gmail.com> wrote: > >>>>>> On 06/11/2018 11:56 AM, Geert Uytterhoeven wrote: > >>>>>>> On Mon, Jun 4, 2018 at 7:59 PM Marek Vasut <marek.vasut@gmail.com> wrote: > >>>>>>>> Rather than hard-coding the quirk topology, which stopped scaling, > >>>>>>>> parse the information from DT. The code looks for all compatible > >>>>>>>> PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied > >>>>>>>> to the same pin. If so, the code sends a matching sequence to the > >>>>>>>> PMIC to deassert the IRQ. > >>> > >>>>>>>> + ret = of_property_read_u32(np, "reg", &addr); > >>>>>>>> + if (ret) > >>>>>>>> + return ret; > >>>>>>> > >>>>>>> I think it's safer to skip this entry and continue, after calling > >>>>>>> kfree(quirk), of course. > >>>>>>> > >>>>>>>> + > >>>>>>>> + quirk->id = id; > >>>>>>>> + quirk->i2c_msg.addr = addr; > >>>>>>>> + quirk->shared = false; > >>>>>>>> + > >>>>>>>> + ret = of_irq_parse_one(np, 0, &quirk->irq_args); > >>>>>>>> + if (ret) > >>>>>>>> + return ret; > >>>>>>> > >>>>>>> kfree(quirk) and continue... > >>>>>> > >>>>>> I wonder if it shouldn't rather free the entire list and abort ? > >>>>> > >>>>> "Be strict when sending, be liberal when receiving." > >>>> > >>>> Meaning ? I think "the language barrier is protecting me" (TM) > >>> > >>> Do the best you can, given the buggy DT you received. > >>> I.e. don't fail completely, just ignore the bad device node, and continue. > >> > >> But if you ignore node, you might as well ignore one which is shared and > >> then the system crashes due to IRQ storm anyway. So hum, what can we do ? > > > > Correct. If it's a critical node, it will crash regardless. > > If it's a non-critical node, you have the choice between aborting and crashing, > > or ignoring and keeping the system alive. Your call. > > But wait, since we control which machines this code runs on , can't we > assure they have valid DTs ? This situation with invalid DT starts to > look a bit hypothetical to me. That assumes you keep the list of machines to check, and don't want to fix the issue automatically when detected (on any R-Car Gen2 or RZ/G1 platform, so you still need to check for r8a779[0-4] and r8a774[23457]). Anyway, as we care about booting old DTBs on new kernels (for a while), we have a few more release cycles to bikeshed ;-) Gr{oetje,eeting}s, Geert
On 06/11/2018 04:30 PM, Geert Uytterhoeven wrote: > Hi Marek, > > On Mon, Jun 11, 2018 at 4:19 PM Marek Vasut <marek.vasut@gmail.com> wrote: >> On 06/11/2018 04:10 PM, Geert Uytterhoeven wrote: >>> On Mon, Jun 11, 2018 at 4:04 PM Marek Vasut <marek.vasut@gmail.com> wrote: >>>> On 06/11/2018 03:49 PM, Geert Uytterhoeven wrote: >>>>> On Mon, Jun 11, 2018 at 3:39 PM Marek Vasut <marek.vasut@gmail.com> wrote: >>>>>> On 06/11/2018 03:03 PM, Geert Uytterhoeven wrote: >>>>>>> On Mon, Jun 11, 2018 at 2:15 PM Marek Vasut <marek.vasut@gmail.com> wrote: >>>>>>>> On 06/11/2018 11:56 AM, Geert Uytterhoeven wrote: >>>>>>>>> On Mon, Jun 4, 2018 at 7:59 PM Marek Vasut <marek.vasut@gmail.com> wrote: >>>>>>>>>> Rather than hard-coding the quirk topology, which stopped scaling, >>>>>>>>>> parse the information from DT. The code looks for all compatible >>>>>>>>>> PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied >>>>>>>>>> to the same pin. If so, the code sends a matching sequence to the >>>>>>>>>> PMIC to deassert the IRQ. >>>>> >>>>>>>>>> + ret = of_property_read_u32(np, "reg", &addr); >>>>>>>>>> + if (ret) >>>>>>>>>> + return ret; >>>>>>>>> >>>>>>>>> I think it's safer to skip this entry and continue, after calling >>>>>>>>> kfree(quirk), of course. >>>>>>>>> >>>>>>>>>> + >>>>>>>>>> + quirk->id = id; >>>>>>>>>> + quirk->i2c_msg.addr = addr; >>>>>>>>>> + quirk->shared = false; >>>>>>>>>> + >>>>>>>>>> + ret = of_irq_parse_one(np, 0, &quirk->irq_args); >>>>>>>>>> + if (ret) >>>>>>>>>> + return ret; >>>>>>>>> >>>>>>>>> kfree(quirk) and continue... >>>>>>>> >>>>>>>> I wonder if it shouldn't rather free the entire list and abort ? >>>>>>> >>>>>>> "Be strict when sending, be liberal when receiving." >>>>>> >>>>>> Meaning ? I think "the language barrier is protecting me" (TM) >>>>> >>>>> Do the best you can, given the buggy DT you received. >>>>> I.e. don't fail completely, just ignore the bad device node, and continue. >>>> >>>> But if you ignore node, you might as well ignore one which is shared and >>>> then the system crashes due to IRQ storm anyway. So hum, what can we do ? >>> >>> Correct. If it's a critical node, it will crash regardless. >>> If it's a non-critical node, you have the choice between aborting and crashing, >>> or ignoring and keeping the system alive. Your call. >> >> But wait, since we control which machines this code runs on , can't we >> assure they have valid DTs ? This situation with invalid DT starts to >> look a bit hypothetical to me. > > That assumes you keep the list of machines to check, and don't want to fix the > issue automatically when detected (on any R-Car Gen2 or RZ/G1 platform, so > you still need to check for r8a779[0-4] and r8a774[23457]). Yes, I want to keep a list of machines to check, to be _sure_ some machine doesn't randomly blow up. > Anyway, as we care about booting old DTBs on new kernels (for a while), we > have a few more release cycles to bikeshed ;-) I was about to ask if this patch then makes any sense or not.
Hi Marek, On Mon, Jun 11, 2018 at 5:26 PM Marek Vasut <marek.vasut@gmail.com> wrote: > On 06/11/2018 04:30 PM, Geert Uytterhoeven wrote: > > On Mon, Jun 11, 2018 at 4:19 PM Marek Vasut <marek.vasut@gmail.com> wrote: > >> On 06/11/2018 04:10 PM, Geert Uytterhoeven wrote: > >>> On Mon, Jun 11, 2018 at 4:04 PM Marek Vasut <marek.vasut@gmail.com> wrote: > >>>> On 06/11/2018 03:49 PM, Geert Uytterhoeven wrote: > >>>>> On Mon, Jun 11, 2018 at 3:39 PM Marek Vasut <marek.vasut@gmail.com> wrote: > >>>>>> On 06/11/2018 03:03 PM, Geert Uytterhoeven wrote: > >>>>>>> On Mon, Jun 11, 2018 at 2:15 PM Marek Vasut <marek.vasut@gmail.com> wrote: > >>>>>>>> On 06/11/2018 11:56 AM, Geert Uytterhoeven wrote: > >>>>>>>>> On Mon, Jun 4, 2018 at 7:59 PM Marek Vasut <marek.vasut@gmail.com> wrote: > >>>>>>>>>> Rather than hard-coding the quirk topology, which stopped scaling, > >>>>>>>>>> parse the information from DT. The code looks for all compatible > >>>>>>>>>> PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied > >>>>>>>>>> to the same pin. If so, the code sends a matching sequence to the > >>>>>>>>>> PMIC to deassert the IRQ. > >>>>> > >>>>>>>>>> + ret = of_property_read_u32(np, "reg", &addr); > >>>>>>>>>> + if (ret) > >>>>>>>>>> + return ret; > >>>>>>>>> > >>>>>>>>> I think it's safer to skip this entry and continue, after calling > >>>>>>>>> kfree(quirk), of course. > >>>>>>>>> > >>>>>>>>>> + > >>>>>>>>>> + quirk->id = id; > >>>>>>>>>> + quirk->i2c_msg.addr = addr; > >>>>>>>>>> + quirk->shared = false; > >>>>>>>>>> + > >>>>>>>>>> + ret = of_irq_parse_one(np, 0, &quirk->irq_args); > >>>>>>>>>> + if (ret) > >>>>>>>>>> + return ret; > >>>>>>>>> > >>>>>>>>> kfree(quirk) and continue... > >>>>>>>> > >>>>>>>> I wonder if it shouldn't rather free the entire list and abort ? > >>>>>>> > >>>>>>> "Be strict when sending, be liberal when receiving." > >>>>>> > >>>>>> Meaning ? I think "the language barrier is protecting me" (TM) > >>>>> > >>>>> Do the best you can, given the buggy DT you received. > >>>>> I.e. don't fail completely, just ignore the bad device node, and continue. > >>>> > >>>> But if you ignore node, you might as well ignore one which is shared and > >>>> then the system crashes due to IRQ storm anyway. So hum, what can we do ? > >>> > >>> Correct. If it's a critical node, it will crash regardless. > >>> If it's a non-critical node, you have the choice between aborting and crashing, > >>> or ignoring and keeping the system alive. Your call. > >> > >> But wait, since we control which machines this code runs on , can't we > >> assure they have valid DTs ? This situation with invalid DT starts to > >> look a bit hypothetical to me. > > > > That assumes you keep the list of machines to check, and don't want to fix the > > issue automatically when detected (on any R-Car Gen2 or RZ/G1 platform, so > > you still need to check for r8a779[0-4] and r8a774[23457]). > > Yes, I want to keep a list of machines to check, to be _sure_ some > machine doesn't randomly blow up. Just checking for the presence of a "renesas,irqc" node should be sufficient. Using that node would also get rid of the hardcoded IRQC_BASE address. Note that the code assumes IRQ2. If another IRQ is used, that won't harm much though (as in: if it didn't blow up before, it won't blow up now). > > Anyway, as we care about booting old DTBs on new kernels (for a while), we > > have a few more release cycles to bikeshed ;-) > > I was about to ask if this patch then makes any sense or not. Sure. Less hard-coding is always better. Especially if it means we can make it work on more machines automatically :-) Gr{oetje,eeting}s, Geert
On 06/13/2018 01:28 PM, Geert Uytterhoeven wrote: > Hi Marek, Hi, [...] >>>> But wait, since we control which machines this code runs on , can't we >>>> assure they have valid DTs ? This situation with invalid DT starts to >>>> look a bit hypothetical to me. >>> >>> That assumes you keep the list of machines to check, and don't want to fix the >>> issue automatically when detected (on any R-Car Gen2 or RZ/G1 platform, so >>> you still need to check for r8a779[0-4] and r8a774[23457]). >> >> Yes, I want to keep a list of machines to check, to be _sure_ some >> machine doesn't randomly blow up. > > Just checking for the presence of a "renesas,irqc" node should be sufficient. How so? Any other R-Car machine can have the irqc node too. That's fragile at best. > Using that node would also get rid of the hardcoded IRQC_BASE address. > Note that the code assumes IRQ2. If another IRQ is used, that won't harm > much though (as in: if it didn't blow up before, it won't blow up now). We could/should fix up the irqc detection though. >>> Anyway, as we care about booting old DTBs on new kernels (for a while), we >>> have a few more release cycles to bikeshed ;-) >> >> I was about to ask if this patch then makes any sense or not. > > Sure. Less hard-coding is always better. > Especially if it means we can make it work on more machines automatically :-) I prefer to be in control of that.
diff --git a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c index 93f628acfd94..b919073aa27e 100644 --- a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c +++ b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c @@ -31,8 +31,10 @@ #include <linux/i2c.h> #include <linux/init.h> #include <linux/io.h> +#include <linux/list.h> #include <linux/notifier.h> #include <linux/of.h> +#include <linux/of_irq.h> #include <linux/mfd/da9063/registers.h> @@ -44,34 +46,45 @@ /* start of DA9210 System Control and Event Registers */ #define DA9210_REG_MASK_A 0x54 +struct regulator_quirk { + struct list_head list; + const struct of_device_id *id; + struct of_phandle_args irq_args; + struct i2c_msg i2c_msg; + bool shared; /* IRQ line is shared */ +}; + +static LIST_HEAD(quirk_list); static void __iomem *irqc; /* first byte sets the memory pointer, following are consecutive reg values */ static u8 da9063_irq_clr[] = { DA9063_REG_IRQ_MASK_A, 0xff, 0xff, 0xff, 0xff }; static u8 da9210_irq_clr[] = { DA9210_REG_MASK_A, 0xff, 0xff }; -static struct i2c_msg da9xxx_msgs[3] = { - { - .addr = 0x58, - .len = ARRAY_SIZE(da9063_irq_clr), - .buf = da9063_irq_clr, - }, { - .addr = 0x68, - .len = ARRAY_SIZE(da9210_irq_clr), - .buf = da9210_irq_clr, - }, { - .addr = 0x70, - .len = ARRAY_SIZE(da9210_irq_clr), - .buf = da9210_irq_clr, - }, +static struct i2c_msg da9063_msgs = { + .len = ARRAY_SIZE(da9063_irq_clr), + .buf = da9063_irq_clr, +}; + +static struct i2c_msg da9210_msgs = { + .len = ARRAY_SIZE(da9210_irq_clr), + .buf = da9210_irq_clr, +}; + +static const struct of_device_id rcar_gen2_quirk_match[] = { + { .compatible = "dlg,da9063", .data = &da9063_msgs }, + { .compatible = "dlg,da9210", .data = &da9210_msgs }, + {}, }; static int regulator_quirk_notify(struct notifier_block *nb, unsigned long action, void *data) { + struct regulator_quirk *pos, *tmp; struct device *dev = data; struct i2c_client *client; static bool done; + int ret; u32 mon; if (done) @@ -88,17 +101,20 @@ static int regulator_quirk_notify(struct notifier_block *nb, client = to_i2c_client(dev); dev_dbg(dev, "Detected %s\n", client->name); - if ((client->addr == 0x58 && !strcmp(client->name, "da9063")) || - (client->addr == 0x68 && !strcmp(client->name, "da9210")) || - (client->addr == 0x70 && !strcmp(client->name, "da9210"))) { - int ret, len; + /* + * Send message to all PMICs that share an IRQ line to deassert it. + * + * WARNING: This works only if all the PMICs are on the same I2C bus. + */ + list_for_each_entry(pos, &quirk_list, list) { + if (!pos->shared) + continue; - /* There are two DA9210 on Stout, one on the other boards. */ - len = of_machine_is_compatible("renesas,stout") ? 3 : 2; + dev_info(&client->dev, "clearing %s@0x%02x interrupts\n", + pos->id->compatible, pos->i2c_msg.addr); - dev_info(&client->dev, "clearing da9063/da9210 interrupts\n"); - ret = i2c_transfer(client->adapter, da9xxx_msgs, len); - if (ret != len) + ret = i2c_transfer(client->adapter, &pos->i2c_msg, 1); + if (ret != 1) dev_err(&client->dev, "i2c error %d\n", ret); } @@ -111,6 +127,11 @@ static int regulator_quirk_notify(struct notifier_block *nb, remove: dev_info(dev, "IRQ2 is not asserted, removing quirk\n"); + list_for_each_entry_safe(pos, tmp, &quirk_list, list) { + list_del(&pos->list); + kfree(pos); + } + done = true; iounmap(irqc); return 0; @@ -122,7 +143,13 @@ static struct notifier_block regulator_quirk_nb = { static int __init rcar_gen2_regulator_quirk(void) { - u32 mon; + struct device_node *np; + const struct of_device_id *id; + struct regulator_quirk *quirk; + struct regulator_quirk *pos; + struct of_phandle_args *argsa, *argsb; + u32 mon, addr; + int ret; if (!of_machine_is_compatible("renesas,koelsch") && !of_machine_is_compatible("renesas,lager") && @@ -130,6 +157,45 @@ static int __init rcar_gen2_regulator_quirk(void) !of_machine_is_compatible("renesas,gose")) return -ENODEV; + for_each_matching_node_and_match(np, rcar_gen2_quirk_match, &id) { + if (!np || !of_device_is_available(np)) + break; + + quirk = kzalloc(sizeof(*quirk), GFP_KERNEL); + + argsa = &quirk->irq_args; + memcpy(&quirk->i2c_msg, id->data, sizeof(quirk->i2c_msg)); + + ret = of_property_read_u32(np, "reg", &addr); + if (ret) + return ret; + + quirk->id = id; + quirk->i2c_msg.addr = addr; + quirk->shared = false; + + ret = of_irq_parse_one(np, 0, &quirk->irq_args); + if (ret) + return ret; + + list_for_each_entry(pos, &quirk_list, list) { + argsb = &pos->irq_args; + + if (argsa->args_count != argsb->args_count) + continue; + + ret = memcmp(argsa->args, argsb->args, + argsa->args_count * + sizeof(argsa->args[0])); + if (!ret) { + pos->shared = true; + quirk->shared = true; + } + } + + list_add_tail(&quirk->list, &quirk_list); + } + irqc = ioremap(IRQC_BASE, PAGE_SIZE); if (!irqc) return -ENOMEM;
Rather than hard-coding the quirk topology, which stopped scaling, parse the information from DT. The code looks for all compatible PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied to the same pin. If so, the code sends a matching sequence to the PMIC to deassert the IRQ. Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> Cc: Geert Uytterhoeven <geert+renesas@glider.be> Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> Cc: Simon Horman <horms+renesas@verge.net.au> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com> Cc: linux-renesas-soc@vger.kernel.org --- V2: - Replace the DT shared IRQ check loop with memcmp() - Send the I2C message to deassert the IRQ line to all PMICs in the list with shared IRQ line instead of just one - Add comment that this works only in case all the PMICs are on the same I2C bus V3: - Drop the addr = 0x00 init - Drop reinit of argsa in rcar_gen2_regulator_quirk --- arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c | 114 ++++++++++++++++----- 1 file changed, 90 insertions(+), 24 deletions(-)