| Message ID | 1583742120-6661-2-git-send-email-chee.hong.ang@intel.com |
|---|---|
| State | Accepted |
| Commit | 32d630fc1d2eb935027f4a760e4830dc3f15040c |
| Headers | show |
| Series | Fix A10 clock driver crash after changes in DM core | expand |
> -----Original Message----- > From: Ang, Chee Hong <chee.hong.ang at intel.com> > Sent: Monday, March 9, 2020 4:22 PM > To: u-boot at lists.denx.de > Cc: Marek Vasut <marex at denx.de>; Simon Goldschmidt > <simon.k.r.goldschmidt at gmail.com>; See, Chin Liang > <chin.liang.see at intel.com>; Tan, Ley Foon <ley.foon.tan at intel.com>; > Westergreen, Dalon <dalon.westergreen at intel.com>; Ang, Chee Hong > <chee.hong.ang at intel.com> > Subject: [PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in > probe function > > From: Chee Hong Ang <chee.hong.ang at intel.com> > > This commit (82de42fa14682d408da935adfb0f935354c5008f) calls child's > ofdata_to_platdata() method before the parent is probed in dm core. > This has caused the driver no longer able to get the correct parent clock's > register base in the ofdata_to_platdata() method because the parent clocks > will only be probed after the child's ofdata_to_platdata(). > To resolve this, the clock parent's register base will only be retrieved by the > child in probe() method instead of ofdata_to_platdata(). > > Signed-off-by: Chee Hong Ang <chee.hong.ang at intel.com> Reviewed-by: Ley Foon Tan <ley.foon.tan at intel.com>
On 3/9/20 9:21 AM, chee.hong.ang at intel.com wrote: > From: Chee Hong Ang <chee.hong.ang at intel.com> > > This commit (82de42fa14682d408da935adfb0f935354c5008f) calls child's > ofdata_to_platdata() method before the parent is probed in dm core. > This has caused the driver no longer able to get the correct parent > clock's register base in the ofdata_to_platdata() method because the > parent clocks will only be probed after the child's ofdata_to_platdata(). > To resolve this, the clock parent's register base will only be retrieved > by the child in probe() method instead of ofdata_to_platdata(). You should be able to bind the drivers and resolve their register offsets without probing them, so this look more like a bug in the driver core ? > Signed-off-by: Chee Hong Ang <chee.hong.ang at intel.com> > --- > drivers/clk/altera/clk-arria10.c | 40 ++++++++++++++++++---------------------- > 1 file changed, 18 insertions(+), 22 deletions(-) > > diff --git a/drivers/clk/altera/clk-arria10.c b/drivers/clk/altera/clk-arria10.c > index affeb31..b7eed94 100644 > --- a/drivers/clk/altera/clk-arria10.c > +++ b/drivers/clk/altera/clk-arria10.c > @@ -274,6 +274,8 @@ static int socfpga_a10_clk_bind(struct udevice *dev) > static int socfpga_a10_clk_probe(struct udevice *dev) > { > struct socfpga_a10_clk_platdata *plat = dev_get_platdata(dev); > + struct socfpga_a10_clk_platdata *pplat; > + struct udevice *pdev; > const void *fdt = gd->fdt_blob; > int offset = dev_of_offset(dev); > > @@ -281,6 +283,21 @@ static int socfpga_a10_clk_probe(struct udevice *dev) > > socfpga_a10_handoff_workaround(dev); > > + if (!fdt_node_check_compatible(fdt, offset, "altr,clk-mgr")) { > + plat->regs = devfdt_get_addr(dev); > + } else { > + pdev = dev_get_parent(dev); > + if (!pdev) > + return -ENODEV; > + > + pplat = dev_get_platdata(pdev); > + if (!pplat) > + return -EINVAL; > + > + plat->ctl_reg = dev_read_u32_default(dev, "reg", 0x0); > + plat->regs = pplat->regs; > + } > + > if (!fdt_node_check_compatible(fdt, offset, > "altr,socfpga-a10-pll-clock")) { > /* Main PLL has 3 upstream clock */ > @@ -304,29 +321,8 @@ static int socfpga_a10_clk_probe(struct udevice *dev) > static int socfpga_a10_ofdata_to_platdata(struct udevice *dev) > { > struct socfpga_a10_clk_platdata *plat = dev_get_platdata(dev); > - struct socfpga_a10_clk_platdata *pplat; > - struct udevice *pdev; > - const void *fdt = gd->fdt_blob; > unsigned int divreg[3], gatereg[2]; > - int ret, offset = dev_of_offset(dev); > - u32 regs; > - > - regs = dev_read_u32_default(dev, "reg", 0x0); > - > - if (!fdt_node_check_compatible(fdt, offset, "altr,clk-mgr")) { > - plat->regs = devfdt_get_addr(dev); > - } else { > - pdev = dev_get_parent(dev); > - if (!pdev) > - return -ENODEV; > - > - pplat = dev_get_platdata(pdev); > - if (!pplat) > - return -EINVAL; > - > - plat->ctl_reg = regs; > - plat->regs = pplat->regs; > - } > + int ret; > > plat->type = SOCFPGA_A10_CLK_UNKNOWN_CLK; > >
> On 3/9/20 9:21 AM, chee.hong.ang at intel.com wrote: > > From: Chee Hong Ang <chee.hong.ang at intel.com> > > > > This commit (82de42fa14682d408da935adfb0f935354c5008f) calls child's > > ofdata_to_platdata() method before the parent is probed in dm core. > > This has caused the driver no longer able to get the correct parent > > clock's register base in the ofdata_to_platdata() method because the > > parent clocks will only be probed after the child's ofdata_to_platdata(). > > To resolve this, the clock parent's register base will only be > > retrieved by the child in probe() method instead of ofdata_to_platdata(). > > You should be able to bind the drivers and resolve their register offsets without > probing them, so this look more like a bug in the driver core ? The problem is the children clock driver need to resolve/derive their register base from their clock parents. With this new change, clock parents are still not yet being initialized when the children clock drivers need to resolve their register base from their parent. A10 is not booting in mainline due to this issue. I can't think of a better way to fix this. Should we fix the clock driver itself or the DM core ? > > > Signed-off-by: Chee Hong Ang <chee.hong.ang at intel.com> > > --- > > drivers/clk/altera/clk-arria10.c | 40 > > ++++++++++++++++++---------------------- > > 1 file changed, 18 insertions(+), 22 deletions(-) > > > > diff --git a/drivers/clk/altera/clk-arria10.c > > b/drivers/clk/altera/clk-arria10.c > > index affeb31..b7eed94 100644 > > --- a/drivers/clk/altera/clk-arria10.c > > +++ b/drivers/clk/altera/clk-arria10.c > > @@ -274,6 +274,8 @@ static int socfpga_a10_clk_bind(struct udevice > > *dev) static int socfpga_a10_clk_probe(struct udevice *dev) { > > struct socfpga_a10_clk_platdata *plat = dev_get_platdata(dev); > > + struct socfpga_a10_clk_platdata *pplat; > > + struct udevice *pdev; > > const void *fdt = gd->fdt_blob; > > int offset = dev_of_offset(dev); > > > > @@ -281,6 +283,21 @@ static int socfpga_a10_clk_probe(struct udevice > > *dev) > > > > socfpga_a10_handoff_workaround(dev); > > > > + if (!fdt_node_check_compatible(fdt, offset, "altr,clk-mgr")) { > > + plat->regs = devfdt_get_addr(dev); > > + } else { > > + pdev = dev_get_parent(dev); > > + if (!pdev) > > + return -ENODEV; > > + > > + pplat = dev_get_platdata(pdev); > > + if (!pplat) > > + return -EINVAL; > > + > > + plat->ctl_reg = dev_read_u32_default(dev, "reg", 0x0); > > + plat->regs = pplat->regs; > > + } > > + > > if (!fdt_node_check_compatible(fdt, offset, > > "altr,socfpga-a10-pll-clock")) { > > /* Main PLL has 3 upstream clock */ @@ -304,29 +321,8 @@ > static int > > socfpga_a10_clk_probe(struct udevice *dev) static int > > socfpga_a10_ofdata_to_platdata(struct udevice *dev) { > > struct socfpga_a10_clk_platdata *plat = dev_get_platdata(dev); > > - struct socfpga_a10_clk_platdata *pplat; > > - struct udevice *pdev; > > - const void *fdt = gd->fdt_blob; > > unsigned int divreg[3], gatereg[2]; > > - int ret, offset = dev_of_offset(dev); > > - u32 regs; > > - > > - regs = dev_read_u32_default(dev, "reg", 0x0); > > - > > - if (!fdt_node_check_compatible(fdt, offset, "altr,clk-mgr")) { > > - plat->regs = devfdt_get_addr(dev); > > - } else { > > - pdev = dev_get_parent(dev); > > - if (!pdev) > > - return -ENODEV; > > - > > - pplat = dev_get_platdata(pdev); > > - if (!pplat) > > - return -EINVAL; > > - > > - plat->ctl_reg = regs; > > - plat->regs = pplat->regs; > > - } > > + int ret; > > > > plat->type = SOCFPGA_A10_CLK_UNKNOWN_CLK; > > > > > > > -- > Best regards, > Marek Vasut
On 3/9/20 1:52 PM, Ang, Chee Hong wrote: >> On 3/9/20 9:21 AM, chee.hong.ang at intel.com wrote: >>> From: Chee Hong Ang <chee.hong.ang at intel.com> >>> >>> This commit (82de42fa14682d408da935adfb0f935354c5008f) calls child's >>> ofdata_to_platdata() method before the parent is probed in dm core. >>> This has caused the driver no longer able to get the correct parent >>> clock's register base in the ofdata_to_platdata() method because the >>> parent clocks will only be probed after the child's ofdata_to_platdata(). >>> To resolve this, the clock parent's register base will only be >>> retrieved by the child in probe() method instead of ofdata_to_platdata(). >> >> You should be able to bind the drivers and resolve their register offsets without >> probing them, so this look more like a bug in the driver core ? > The problem is the children clock driver need to resolve/derive their register base > from their clock parents. With this new change, clock parents are still not yet > being initialized when the children clock drivers need to resolve their register base > from their parent. > A10 is not booting in mainline due to this issue. > I can't think of a better way to fix this. Should we fix the clock driver itself or the > DM core ? It seems more like a bug in the later, since the register offsets are in the DT and reading out the DT information should be possible before .probe() is called for any of the clock.
Hi, On Mon, 9 Mar 2020 at 02:22, <chee.hong.ang at intel.com> wrote: > > From: Chee Hong Ang <chee.hong.ang at intel.com> > > This commit (82de42fa14682d408da935adfb0f935354c5008f) calls child's > ofdata_to_platdata() method before the parent is probed in dm core. > This has caused the driver no longer able to get the correct parent > clock's register base in the ofdata_to_platdata() method because the > parent clocks will only be probed after the child's ofdata_to_platdata(). > To resolve this, the clock parent's register base will only be retrieved > by the child in probe() method instead of ofdata_to_platdata(). I think one thing that is going on here is that DM allows ofdata to be read for a device before its parent devices have been read, but it requires that parent devices be probed before their children. The idea is that it should be possible to read the ofdata for a node without needing to have done so for parents. But perhaps this assumption is too brave? I suspect we could change this, so that device_ofdata_to_platdata() first calls itself on its parent. I can think of various reasons why this change might be desirable. > > Signed-off-by: Chee Hong Ang <chee.hong.ang at intel.com> > --- > drivers/clk/altera/clk-arria10.c | 40 ++++++++++++++++++---------------------- > 1 file changed, 18 insertions(+), 22 deletions(-) > > diff --git a/drivers/clk/altera/clk-arria10.c b/drivers/clk/altera/clk-arria10.c > index affeb31..b7eed94 100644 > --- a/drivers/clk/altera/clk-arria10.c > +++ b/drivers/clk/altera/clk-arria10.c > @@ -274,6 +274,8 @@ static int socfpga_a10_clk_bind(struct udevice *dev) > static int socfpga_a10_clk_probe(struct udevice *dev) > { > struct socfpga_a10_clk_platdata *plat = dev_get_platdata(dev); > + struct socfpga_a10_clk_platdata *pplat; > + struct udevice *pdev; > const void *fdt = gd->fdt_blob; > int offset = dev_of_offset(dev); > > @@ -281,6 +283,21 @@ static int socfpga_a10_clk_probe(struct udevice *dev) > > socfpga_a10_handoff_workaround(dev); > > + if (!fdt_node_check_compatible(fdt, offset, "altr,clk-mgr")) { This is strange to me. I think the idea here is to detect whether this is the root clk node or one of the ones created in the bind() function above. Is that right? If so, it should be enough to say: if (dev_ofvalid(dev)) If you actually need to distinguish between different compatible strings, you can use the .data member in your udevice_id table, with dev_get_driver_data(). > + plat->regs = devfdt_get_addr(dev); > + } else { > + pdev = dev_get_parent(dev); > + if (!pdev) > + return -ENODEV; > + > + pplat = dev_get_platdata(pdev); > + if (!pplat) > + return -EINVAL; > + > + plat->ctl_reg = dev_read_u32_default(dev, "reg", 0x0); > + plat->regs = pplat->regs; > + } > + > if (!fdt_node_check_compatible(fdt, offset, > "altr,socfpga-a10-pll-clock")) { > /* Main PLL has 3 upstream clock */ > @@ -304,29 +321,8 @@ static int socfpga_a10_clk_probe(struct udevice *dev) > static int socfpga_a10_ofdata_to_platdata(struct udevice *dev) > { > struct socfpga_a10_clk_platdata *plat = dev_get_platdata(dev); > - struct socfpga_a10_clk_platdata *pplat; > - struct udevice *pdev; > - const void *fdt = gd->fdt_blob; > unsigned int divreg[3], gatereg[2]; > - int ret, offset = dev_of_offset(dev); > - u32 regs; > - > - regs = dev_read_u32_default(dev, "reg", 0x0); > - > - if (!fdt_node_check_compatible(fdt, offset, "altr,clk-mgr")) { > - plat->regs = devfdt_get_addr(dev); > - } else { > - pdev = dev_get_parent(dev); > - if (!pdev) > - return -ENODEV; > - > - pplat = dev_get_platdata(pdev); > - if (!pplat) > - return -EINVAL; > - > - plat->ctl_reg = regs; > - plat->regs = pplat->regs; > - } > + int ret; > > plat->type = SOCFPGA_A10_CLK_UNKNOWN_CLK; > > -- > 2.7.4 > Regards, Simon
On 3/11/20 12:50 PM, Simon Glass wrote: > Hi, Hi, > On Mon, 9 Mar 2020 at 02:22, <chee.hong.ang at intel.com> wrote: >> >> From: Chee Hong Ang <chee.hong.ang at intel.com> >> >> This commit (82de42fa14682d408da935adfb0f935354c5008f) calls child's >> ofdata_to_platdata() method before the parent is probed in dm core. >> This has caused the driver no longer able to get the correct parent >> clock's register base in the ofdata_to_platdata() method because the >> parent clocks will only be probed after the child's ofdata_to_platdata(). >> To resolve this, the clock parent's register base will only be retrieved >> by the child in probe() method instead of ofdata_to_platdata(). > > I think one thing that is going on here is that DM allows ofdata to be > read for a device before its parent devices have been read, but it > requires that parent devices be probed before their children. This seems wrong. The clock driver should be able to instantiate devices and read their ofdata without probing them. That is one of the core design principles of the DM. > The idea is that it should be possible to read the ofdata for a node > without needing to have done so for parents. But perhaps this > assumption is too brave? Why is it brave ? That's how it always was, the DT is already there, so why wouldn't you be able to read it. > I suspect we could change this, so that device_ofdata_to_platdata() > first calls itself on its parent. > > I can think of various reasons why this change might be desirable. I think this is how it worked before already.
Hi Marek, On Wed, 11 Mar 2020 at 05:55, Marek Vasut <marex at denx.de> wrote: > > On 3/11/20 12:50 PM, Simon Glass wrote: > > Hi, > > Hi, > > > On Mon, 9 Mar 2020 at 02:22, <chee.hong.ang at intel.com> wrote: > >> > >> From: Chee Hong Ang <chee.hong.ang at intel.com> > >> > >> This commit (82de42fa14682d408da935adfb0f935354c5008f) calls child's > >> ofdata_to_platdata() method before the parent is probed in dm core. > >> This has caused the driver no longer able to get the correct parent > >> clock's register base in the ofdata_to_platdata() method because the > >> parent clocks will only be probed after the child's ofdata_to_platdata(). > >> To resolve this, the clock parent's register base will only be retrieved > >> by the child in probe() method instead of ofdata_to_platdata(). > > > > I think one thing that is going on here is that DM allows ofdata to be > > read for a device before its parent devices have been read, but it > > requires that parent devices be probed before their children. > > This seems wrong. The clock driver should be able to instantiate devices > and read their ofdata without probing them. That is one of the core > design principles of the DM. That's a different question. Yes you can read ofdata without probing a device. That's why we have two methods. The point I am making is that at present there is no requirement that a parent's ofdata be read before a child's ofdata is read. But there is a requirement that a parent be probed before a child is probed. > > > The idea is that it should be possible to read the ofdata for a node > > without needing to have done so for parents. But perhaps this > > assumption is too brave? > > Why is it brave ? That's how it always was, the DT is already there, so > why wouldn't you be able to read it. That was my thinking too. But we are finding in a few situations that the child's ofdata depends on the parent's. For example, the parent may have a base address, or a range mapping, or something else that is needed for the child to correctly get its base address, etc. > > > I suspect we could change this, so that device_ofdata_to_platdata() > > first calls itself on its parent. > > > > I can think of various reasons why this change might be desirable. > > I think this is how it worked before already. Well effectively, yes, because ofdata and probe were joined together. Regards, Simon
diff --git a/drivers/clk/altera/clk-arria10.c b/drivers/clk/altera/clk-arria10.c index affeb31..b7eed94 100644 --- a/drivers/clk/altera/clk-arria10.c +++ b/drivers/clk/altera/clk-arria10.c @@ -274,6 +274,8 @@ static int socfpga_a10_clk_bind(struct udevice *dev) static int socfpga_a10_clk_probe(struct udevice *dev) { struct socfpga_a10_clk_platdata *plat = dev_get_platdata(dev); + struct socfpga_a10_clk_platdata *pplat; + struct udevice *pdev; const void *fdt = gd->fdt_blob; int offset = dev_of_offset(dev); @@ -281,6 +283,21 @@ static int socfpga_a10_clk_probe(struct udevice *dev) socfpga_a10_handoff_workaround(dev); + if (!fdt_node_check_compatible(fdt, offset, "altr,clk-mgr")) { + plat->regs = devfdt_get_addr(dev); + } else { + pdev = dev_get_parent(dev); + if (!pdev) + return -ENODEV; + + pplat = dev_get_platdata(pdev); + if (!pplat) + return -EINVAL; + + plat->ctl_reg = dev_read_u32_default(dev, "reg", 0x0); + plat->regs = pplat->regs; + } + if (!fdt_node_check_compatible(fdt, offset, "altr,socfpga-a10-pll-clock")) { /* Main PLL has 3 upstream clock */ @@ -304,29 +321,8 @@ static int socfpga_a10_clk_probe(struct udevice *dev) static int socfpga_a10_ofdata_to_platdata(struct udevice *dev) { struct socfpga_a10_clk_platdata *plat = dev_get_platdata(dev); - struct socfpga_a10_clk_platdata *pplat; - struct udevice *pdev; - const void *fdt = gd->fdt_blob; unsigned int divreg[3], gatereg[2]; - int ret, offset = dev_of_offset(dev); - u32 regs; - - regs = dev_read_u32_default(dev, "reg", 0x0); - - if (!fdt_node_check_compatible(fdt, offset, "altr,clk-mgr")) { - plat->regs = devfdt_get_addr(dev); - } else { - pdev = dev_get_parent(dev); - if (!pdev) - return -ENODEV; - - pplat = dev_get_platdata(pdev); - if (!pplat) - return -EINVAL; - - plat->ctl_reg = regs; - plat->regs = pplat->regs; - } + int ret; plat->type = SOCFPGA_A10_CLK_UNKNOWN_CLK;