| Message ID | 20190404215344.6330-4-sboyd@kernel.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Rewrite clk parent handling | expand |
On 4/4/2019 3:53 PM, Stephen Boyd wrote: > In some circumstances drivers register clks early and don't have access > to a struct device because the device model isn't initialized yet. Add > an API to let drivers register clks associated with a struct device_node > so that these drivers can participate in getting parent clks through DT. NACK. This patch broke boot for me. I had to pull the below from JTAG. What do you need to debug this? [ 0.000000] Unable to handle kernel read from unreadable memory at virtual address 0000000000000280 [ 0.000000] Mem abort info: [ 0.000000] ESR = 0x96000004 [ 0.000000] Exception class = DABT (current EL), IL = 32 bits [ 0.000000] SET = 0, FnV = 0 [ 0.000000] EA = 0, S1PTW = 0 [ 0.000000] Data abort info: [ 0.000000] ISV = 0, ISS = 0x00000004 [ 0.000000] CM = 0, WnR = 0 [ 0.000000] [0000000000000280] user address but active_mm is swapper [ 0.000000] Internal error: Oops: 96000004 [#1] SMP [ 0.000000] Modules linked in: [ 0.000000] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.1.0-rc1+ #453 [ 0.000000] Hardware name: Qualcomm Technologies, Inc. MSM8998 v1 MTP (DT) [ 0.000000] pstate: 60400085 (nZCv daIf +PAN -UAO) [ 0.000000] pc : clk_hw_register+0x24/0x40 [ 0.000000] lr : clk_hw_register_fixed_rate_with_accuracy+0xac/0x100 [ 0.000000] sp : ffff0000117d3df0 [ 0.000000] x29: ffff0000117d3df0 x28: ffff8000fdc7cd28 [ 0.000000] x27: dead000000000100 x26: dead000000000200 [ 0.000000] x25: 0000000000000001 x24: ffff7dfffe80047c [ 0.000000] x23: 000000000124f800 x22: 0000000000000000 [ 0.000000] x21: 0000000000000000 x20: ffff8000f9524c00 [ 0.000000] x19: 0000000000000000 x18: ffffffffffffffff [ 0.000000] x17: 0000000000000000 x16: 0000000000000000 [ 0.000000] x15: ffff0000117dc708 x14: ffffffffffffffff [ 0.000000] x13: 0000000000000020 x12: 0101010101010101 [ 0.000000] x11: 7f7f7f7f7f7f7f7f x10: 02fefeff01fefeff [ 0.000000] x9 : 0000000000000000 x8 : ffff8000f9524c80 [ 0.000000] x7 : 0000000000000000 x6 : 000000000000003f [ 0.000000] x5 : 0000000000000040 x4 : 0000000000000000 [ 0.000000] x3 : 0000000000000000 x2 : ffff8000f9524c00 [ 0.000000] x1 : ffff8000f9524c00 x0 : 0000000000000000 [ 0.000000] Process swapper/0 (pid: 0, stack limit = 0x(____ptrval____)) [ 0.000000] Call trace: [ 0.000000] clk_hw_register+0x24/0x40 [ 0.000000] clk_hw_register_fixed_rate_with_accuracy+0xac/0x100 [ 0.000000] _of_fixed_clk_setup+0xa8/0x120 [ 0.000000] of_fixed_clk_setup+0x20/0x2c [ 0.000000] of_clk_init+0x19c/0x254 [ 0.000000] time_init+0x18/0x4c [ 0.000000] start_kernel+0x394/0x524 [ 0.000000] Code: aa1e03e0 d503201f aa1403e2 aa1303e0 (f9414261) [ 0.000000] ---[ end trace 5d02af5e3cbab7bf ]--- [ 0.000000] Kernel panic - not syncing: Attempted to kill the idle task! [ 0.000000] ---[ end Kernel panic - not syncing: Attempted to kill the idle task! ]--- > > Cc: Miquel Raynal <miquel.raynal@bootlin.com> > Cc: Jerome Brunet <jbrunet@baylibre.com> > Cc: Russell King <linux@armlinux.org.uk> > Cc: Michael Turquette <mturquette@baylibre.com> > Cc: Jeffrey Hugo <jhugo@codeaurora.org> > Cc: Chen-Yu Tsai <wens@csie.org> > Cc: Rob Herring <robh@kernel.org> > Signed-off-by: Stephen Boyd <sboyd@kernel.org> > --- > drivers/clk/clk.c | 26 +++++++++++++++++++++++--- > include/linux/clk-provider.h | 1 + > 2 files changed, 24 insertions(+), 3 deletions(-) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index d27775a73e67..4971e9651ca5 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -45,6 +45,7 @@ struct clk_core { > struct clk_hw *hw; > struct module *owner; > struct device *dev; > + struct device_node *of_node; > struct clk_core *parent; > const char **parent_names; > struct clk_core **parents; > @@ -3313,7 +3314,8 @@ struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw, > return clk; > } > > -static struct clk *__clk_register(struct device *dev, struct clk_hw *hw) > +static struct clk * > +__clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw) > { > int i, ret; > struct clk_core *core; > @@ -3339,6 +3341,7 @@ static struct clk *__clk_register(struct device *dev, struct clk_hw *hw) > if (dev && pm_runtime_enabled(dev)) > core->rpm_enabled = true; > core->dev = dev; > + core->of_node = np; > if (dev && dev->driver) > core->owner = dev->driver->owner; > core->hw = hw; > @@ -3429,7 +3432,7 @@ static struct clk *__clk_register(struct device *dev, struct clk_hw *hw) > */ > struct clk *clk_register(struct device *dev, struct clk_hw *hw) > { > - return __clk_register(dev, hw); > + return __clk_register(dev, dev->of_node, hw); > } > EXPORT_SYMBOL_GPL(clk_register); > > @@ -3445,10 +3448,27 @@ EXPORT_SYMBOL_GPL(clk_register); > */ > int clk_hw_register(struct device *dev, struct clk_hw *hw) > { > - return PTR_ERR_OR_ZERO(__clk_register(dev, hw)); > + return PTR_ERR_OR_ZERO(__clk_register(dev, dev->of_node, hw)); > } > EXPORT_SYMBOL_GPL(clk_hw_register); > > +/* > + * of_clk_hw_register - register a clk_hw and return an error code > + * @node: device_node of device that is registering this clock > + * @hw: link to hardware-specific clock data > + * > + * of_clk_hw_register() is the primary interface for populating the clock tree > + * with new clock nodes when a struct device is not available, but a struct > + * device_node is. It returns an integer equal to zero indicating success or > + * less than zero indicating failure. Drivers must test for an error code after > + * calling of_clk_hw_register(). > + */ > +int of_clk_hw_register(struct device_node *node, struct clk_hw *hw) > +{ > + return PTR_ERR_OR_ZERO(__clk_register(NULL, node, hw)); > +} > +EXPORT_SYMBOL_GPL(of_clk_hw_register); > + > /* Free memory allocated for a clock. */ > static void __clk_release(struct kref *ref) > { > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h > index b7cf80a71293..7d2d97e15b76 100644 > --- a/include/linux/clk-provider.h > +++ b/include/linux/clk-provider.h > @@ -773,6 +773,7 @@ struct clk *devm_clk_register(struct device *dev, struct clk_hw *hw); > > int __must_check clk_hw_register(struct device *dev, struct clk_hw *hw); > int __must_check devm_clk_hw_register(struct device *dev, struct clk_hw *hw); > +int __must_check of_clk_hw_register(struct device_node *node, struct clk_hw *hw); > > void clk_unregister(struct clk *clk); > void devm_clk_unregister(struct device *dev, struct clk *clk); >
Quoting Jeffrey Hugo (2019-04-08 14:46:11) > On 4/4/2019 3:53 PM, Stephen Boyd wrote: > > In some circumstances drivers register clks early and don't have access > > to a struct device because the device model isn't initialized yet. Add > > an API to let drivers register clks associated with a struct device_node > > so that these drivers can participate in getting parent clks through DT. > > NACK. This patch broke boot for me. I had to pull the below from JTAG. No early printk support on this board? Sadness. > What do you need to debug this? > Hopefully nothing. > [ 0.000000] Unable to handle kernel read from unreadable memory at > virtual address 0000000000000280 I guess you pass NULL as dev to clk_hw_register(). Will fix! Thanks for testing. > [ 0.000000] Mem abort info:
Quoting Jeffrey Hugo (2019-04-08 14:46:11) > On 4/4/2019 3:53 PM, Stephen Boyd wrote: > > In some circumstances drivers register clks early and don't have access > > to a struct device because the device model isn't initialized yet. Add > > an API to let drivers register clks associated with a struct device_node > > so that these drivers can participate in getting parent clks through DT. > > NACK. This patch broke boot for me. I had to pull the below from JTAG. > What do you need to debug this? > Here's a patch to try to squash in: ---8<---- diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 709492d901a1..040ce083c89e 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -3662,7 +3662,7 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw) */ struct clk *clk_register(struct device *dev, struct clk_hw *hw) { - return __clk_register(dev, dev->of_node, hw); + return __clk_register(dev, dev_of_node(dev), hw); } EXPORT_SYMBOL_GPL(clk_register); @@ -3678,7 +3678,7 @@ EXPORT_SYMBOL_GPL(clk_register); */ int clk_hw_register(struct device *dev, struct clk_hw *hw) { - return PTR_ERR_OR_ZERO(__clk_register(dev, dev->of_node, hw)); + return PTR_ERR_OR_ZERO(__clk_register(dev, dev_of_node(dev), hw)); } EXPORT_SYMBOL_GPL(clk_hw_register);
On 4/10/2019 10:53 AM, Stephen Boyd wrote: > Quoting Jeffrey Hugo (2019-04-08 14:46:11) >> On 4/4/2019 3:53 PM, Stephen Boyd wrote: >>> In some circumstances drivers register clks early and don't have access >>> to a struct device because the device model isn't initialized yet. Add >>> an API to let drivers register clks associated with a struct device_node >>> so that these drivers can participate in getting parent clks through DT. >> >> NACK. This patch broke boot for me. I had to pull the below from JTAG. >> What do you need to debug this? >> > > Here's a patch to try to squash in: No dice. Same issue. > > ---8<---- > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index 709492d901a1..040ce083c89e 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -3662,7 +3662,7 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw) > */ > struct clk *clk_register(struct device *dev, struct clk_hw *hw) > { > - return __clk_register(dev, dev->of_node, hw); > + return __clk_register(dev, dev_of_node(dev), hw); > } > EXPORT_SYMBOL_GPL(clk_register); > > @@ -3678,7 +3678,7 @@ EXPORT_SYMBOL_GPL(clk_register); > */ > int clk_hw_register(struct device *dev, struct clk_hw *hw) > { > - return PTR_ERR_OR_ZERO(__clk_register(dev, dev->of_node, hw)); > + return PTR_ERR_OR_ZERO(__clk_register(dev, dev_of_node(dev), hw)); > } > EXPORT_SYMBOL_GPL(clk_hw_register); > >
Quoting Jeffrey Hugo (2019-04-10 12:39:16) > On 4/10/2019 10:53 AM, Stephen Boyd wrote: > > Quoting Jeffrey Hugo (2019-04-08 14:46:11) > >> On 4/4/2019 3:53 PM, Stephen Boyd wrote: > >>> In some circumstances drivers register clks early and don't have access > >>> to a struct device because the device model isn't initialized yet. Add > >>> an API to let drivers register clks associated with a struct device_node > >>> so that these drivers can participate in getting parent clks through DT. > >> > >> NACK. This patch broke boot for me. I had to pull the below from JTAG. > >> What do you need to debug this? > >> > > > > Here's a patch to try to squash in: > > No dice. Same issue. Argh! dev_of_node() doesn't check for NULL device. :-/ I want to be extremely lazy! Let's get this merged too. Thanks for the testing. ---8<--- diff --git a/include/linux/device.h b/include/linux/device.h index b425a7ee04ce..0370dd0b3ae7 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -1231,7 +1231,7 @@ static inline void device_lock_assert(struct device *dev) static inline struct device_node *dev_of_node(struct device *dev) { - if (!IS_ENABLED(CONFIG_OF)) + if (!IS_ENABLED(CONFIG_OF) || !dev) return NULL; return dev->of_node; }
On 4/10/2019 3:45 PM, Stephen Boyd wrote: > Quoting Jeffrey Hugo (2019-04-10 12:39:16) >> On 4/10/2019 10:53 AM, Stephen Boyd wrote: >>> Quoting Jeffrey Hugo (2019-04-08 14:46:11) >>>> On 4/4/2019 3:53 PM, Stephen Boyd wrote: >>>>> In some circumstances drivers register clks early and don't have access >>>>> to a struct device because the device model isn't initialized yet. Add >>>>> an API to let drivers register clks associated with a struct device_node >>>>> so that these drivers can participate in getting parent clks through DT. >>>> >>>> NACK. This patch broke boot for me. I had to pull the below from JTAG. >>>> What do you need to debug this? >>>> >>> >>> Here's a patch to try to squash in: >> >> No dice. Same issue. > > Argh! dev_of_node() doesn't check for NULL device. :-/ I want to be > extremely lazy! Let's get this merged too. Thanks for the testing. Fixed. Will continue to test the rest of the series and let you know.
On 4/10/2019 4:18 PM, Jeffrey Hugo wrote: > On 4/10/2019 3:45 PM, Stephen Boyd wrote: >> Quoting Jeffrey Hugo (2019-04-10 12:39:16) >>> On 4/10/2019 10:53 AM, Stephen Boyd wrote: >>>> Quoting Jeffrey Hugo (2019-04-08 14:46:11) >>>>> On 4/4/2019 3:53 PM, Stephen Boyd wrote: >>>>>> In some circumstances drivers register clks early and don't have >>>>>> access >>>>>> to a struct device because the device model isn't initialized yet. >>>>>> Add >>>>>> an API to let drivers register clks associated with a struct >>>>>> device_node >>>>>> so that these drivers can participate in getting parent clks >>>>>> through DT. >>>>> >>>>> NACK. This patch broke boot for me. I had to pull the below from >>>>> JTAG. >>>>> What do you need to debug this? >>>>> >>>> >>>> Here's a patch to try to squash in: >>> >>> No dice. Same issue. >> >> Argh! dev_of_node() doesn't check for NULL device. :-/ I want to be >> extremely lazy! Let's get this merged too. Thanks for the testing. > > Fixed. Will continue to test the rest of the series and let you know. > With the two changes we discussed, for the entire series: Tested-by: Jeffrey Hugo <jhugo@codeaurora.org>
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index d27775a73e67..4971e9651ca5 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -45,6 +45,7 @@ struct clk_core { struct clk_hw *hw; struct module *owner; struct device *dev; + struct device_node *of_node; struct clk_core *parent; const char **parent_names; struct clk_core **parents; @@ -3313,7 +3314,8 @@ struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw, return clk; } -static struct clk *__clk_register(struct device *dev, struct clk_hw *hw) +static struct clk * +__clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw) { int i, ret; struct clk_core *core; @@ -3339,6 +3341,7 @@ static struct clk *__clk_register(struct device *dev, struct clk_hw *hw) if (dev && pm_runtime_enabled(dev)) core->rpm_enabled = true; core->dev = dev; + core->of_node = np; if (dev && dev->driver) core->owner = dev->driver->owner; core->hw = hw; @@ -3429,7 +3432,7 @@ static struct clk *__clk_register(struct device *dev, struct clk_hw *hw) */ struct clk *clk_register(struct device *dev, struct clk_hw *hw) { - return __clk_register(dev, hw); + return __clk_register(dev, dev->of_node, hw); } EXPORT_SYMBOL_GPL(clk_register); @@ -3445,10 +3448,27 @@ EXPORT_SYMBOL_GPL(clk_register); */ int clk_hw_register(struct device *dev, struct clk_hw *hw) { - return PTR_ERR_OR_ZERO(__clk_register(dev, hw)); + return PTR_ERR_OR_ZERO(__clk_register(dev, dev->of_node, hw)); } EXPORT_SYMBOL_GPL(clk_hw_register); +/* + * of_clk_hw_register - register a clk_hw and return an error code + * @node: device_node of device that is registering this clock + * @hw: link to hardware-specific clock data + * + * of_clk_hw_register() is the primary interface for populating the clock tree + * with new clock nodes when a struct device is not available, but a struct + * device_node is. It returns an integer equal to zero indicating success or + * less than zero indicating failure. Drivers must test for an error code after + * calling of_clk_hw_register(). + */ +int of_clk_hw_register(struct device_node *node, struct clk_hw *hw) +{ + return PTR_ERR_OR_ZERO(__clk_register(NULL, node, hw)); +} +EXPORT_SYMBOL_GPL(of_clk_hw_register); + /* Free memory allocated for a clock. */ static void __clk_release(struct kref *ref) { diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h index b7cf80a71293..7d2d97e15b76 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -773,6 +773,7 @@ struct clk *devm_clk_register(struct device *dev, struct clk_hw *hw); int __must_check clk_hw_register(struct device *dev, struct clk_hw *hw); int __must_check devm_clk_hw_register(struct device *dev, struct clk_hw *hw); +int __must_check of_clk_hw_register(struct device_node *node, struct clk_hw *hw); void clk_unregister(struct clk *clk); void devm_clk_unregister(struct device *dev, struct clk *clk);
In some circumstances drivers register clks early and don't have access to a struct device because the device model isn't initialized yet. Add an API to let drivers register clks associated with a struct device_node so that these drivers can participate in getting parent clks through DT. Cc: Miquel Raynal <miquel.raynal@bootlin.com> Cc: Jerome Brunet <jbrunet@baylibre.com> Cc: Russell King <linux@armlinux.org.uk> Cc: Michael Turquette <mturquette@baylibre.com> Cc: Jeffrey Hugo <jhugo@codeaurora.org> Cc: Chen-Yu Tsai <wens@csie.org> Cc: Rob Herring <robh@kernel.org> Signed-off-by: Stephen Boyd <sboyd@kernel.org> --- drivers/clk/clk.c | 26 +++++++++++++++++++++++--- include/linux/clk-provider.h | 1 + 2 files changed, 24 insertions(+), 3 deletions(-)