[PATCH v1 0/2] Fix A10 clock driver crash after changes in DM core
From: "Ang, Chee Hong" chee.hong.ang@intel.com
Changes in DM core from following patch crashed the A10 clock driver:
commit: 82de42fa14682d408da935adfb0f935354c5008f Subject: dm: core: Allocate parent data separate from probing parent
At present the parent is probed before the child's ofdata_to_platdata() method is called. Adjust the logic slightly so that probing parents is not done until afterwards.
Signed-off-by: Simon Glass sjg@chromium.org
These patchsets fix the A10 driver issue and replce the FDT API with ofnode API.
Chee Hong Ang (2): clk: socfpga: Read the clock parent's register base in probe function clk: socfpga: Switch to use ofnode API
drivers/clk/altera/clk-arria10.c | 88 +++++++++++++++++++--------------------- 1 file changed, 41 insertions(+), 47 deletions(-)
From: Chee Hong Ang chee.hong.ang@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@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;
-----Original Message----- From: Ang, Chee Hong chee.hong.ang@intel.com Sent: Monday, March 9, 2020 4:22 PM To: u-boot@lists.denx.de Cc: Marek Vasut marex@denx.de; Simon Goldschmidt simon.k.r.goldschmidt@gmail.com; See, Chin Liang chin.liang.see@intel.com; Tan, Ley Foon ley.foon.tan@intel.com; Westergreen, Dalon dalon.westergreen@intel.com; Ang, Chee Hong chee.hong.ang@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@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@intel.com
Reviewed-by: Ley Foon Tan ley.foon.tan@intel.com
On 3/9/20 9:21 AM, chee.hong.ang@intel.com wrote:
From: Chee Hong Ang chee.hong.ang@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@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@intel.com wrote:
From: Chee Hong Ang chee.hong.ang@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@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@intel.com wrote:
From: Chee Hong Ang chee.hong.ang@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@intel.com wrote:
From: Chee Hong Ang chee.hong.ang@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@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@intel.com wrote:
From: Chee Hong Ang chee.hong.ang@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@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@intel.com wrote:
From: Chee Hong Ang chee.hong.ang@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
Hi Marek,
On Wed, 11 Mar 2020 at 05:55, Marek Vasut marex@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@intel.com wrote:
From: Chee Hong Ang chee.hong.ang@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
Simon, do you have plan to fix this DM core issue ?
Hi,
On Tue, 31 Mar 2020 at 20:33, Ang, Chee Hong chee.hong.ang@intel.com wrote:
Hi Marek,
On Wed, 11 Mar 2020 at 05:55, Marek Vasut marex@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@intel.com wrote:
From: Chee Hong Ang chee.hong.ang@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.
Simon, do you have plan to fix this DM core issue ?
I'm not sure it definitely should be changed. But I'll do a patch and see how it looks.
Regards, Simon
On 4/2/20 4:34 AM, Simon Glass wrote:
Hi,
On Tue, 31 Mar 2020 at 20:33, Ang, Chee Hong chee.hong.ang@intel.com wrote:
Hi Marek,
On Wed, 11 Mar 2020 at 05:55, Marek Vasut marex@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@intel.com wrote:
From: Chee Hong Ang chee.hong.ang@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.
Simon, do you have plan to fix this DM core issue ?
I'm not sure it definitely should be changed. But I'll do a patch and see how it looks.
Do I understand it correctly that the patch 82de42fa14682d408da935adfb0f935354c5008f actually completely breaks SoCFPGA ? Then I would say this is a release blocker ?
On 4/2/20 4:34 AM, Simon Glass wrote:
Hi,
On Tue, 31 Mar 2020 at 20:33, Ang, Chee Hong chee.hong.ang@intel.com
wrote:
Hi Marek,
On Wed, 11 Mar 2020 at 05:55, Marek Vasut marex@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@intel.com wrote: > > From: Chee Hong Ang chee.hong.ang@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.
Simon, do you have plan to fix this DM core issue ?
I'm not sure it definitely should be changed. But I'll do a patch and see how it looks.
Do I understand it correctly that the patch 82de42fa14682d408da935adfb0f935354c5008f actually completely breaks SoCFPGA ? Then I would say this is a release blocker ?
Yes. A10 SPL won't boot at all. It crashes during the clock manager setup.
Hi.
On Wed, 1 Apr 2020 at 20:44, Ang, Chee Hong chee.hong.ang@intel.com wrote:
On 4/2/20 4:34 AM, Simon Glass wrote:
Hi,
On Tue, 31 Mar 2020 at 20:33, Ang, Chee Hong chee.hong.ang@intel.com
wrote:
Hi Marek,
On Wed, 11 Mar 2020 at 05:55, Marek Vasut marex@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@intel.com wrote: >> >> From: Chee Hong Ang chee.hong.ang@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.
Simon, do you have plan to fix this DM core issue ?
I'm not sure it definitely should be changed. But I'll do a patch and see how it looks.
Do I understand it correctly that the patch 82de42fa14682d408da935adfb0f935354c5008f actually completely breaks SoCFPGA ? Then I would say this is a release blocker ?
Yes. A10 SPL won't boot at all. It crashes during the clock manager setup.
This came in right at the beginning of the cycle. I thought the purpose of the 3-month cycle was to allow time to test?
I do plan to try out changing the behaviour to read a parent's ofdata before the child, but I am not comfortable adding such a major change just before a release. It could have any number of ill effects.
Can you update the clock driver? E.g. you could move some of the code from socfpga_a10_ofdata_to_platdata() to a probe() method?
Regards, Simon
On 4/2/20 8:50 PM, Simon Glass wrote:
Hi.
Hi,
[...]
>> 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.
Simon, do you have plan to fix this DM core issue ?
I'm not sure it definitely should be changed. But I'll do a patch and see how it looks.
Do I understand it correctly that the patch 82de42fa14682d408da935adfb0f935354c5008f actually completely breaks SoCFPGA ? Then I would say this is a release blocker ?
Yes. A10 SPL won't boot at all. It crashes during the clock manager setup.
This came in right at the beginning of the cycle. I thought the purpose of the 3-month cycle was to allow time to test?
It was ... altera ?
I do plan to try out changing the behaviour to read a parent's ofdata before the child, but I am not comfortable adding such a major change just before a release. It could have any number of ill effects.
Can you update the clock driver? E.g. you could move some of the code from socfpga_a10_ofdata_to_platdata() to a probe() method?
Can we revert the patch which broke arria10 instead ? It did work before, so who knows how many other ill side effects there are ...
Hi Marek,
On Thu, 2 Apr 2020 at 13:45, Marek Vasut marex@denx.de wrote:
On 4/2/20 8:50 PM, Simon Glass wrote:
Hi.
Hi,
[...]
>>> 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.
Simon, do you have plan to fix this DM core issue ?
I'm not sure it definitely should be changed. But I'll do a patch and see how it looks.
Do I understand it correctly that the patch 82de42fa14682d408da935adfb0f935354c5008f actually completely breaks SoCFPGA ? Then I would say this is a release blocker ?
Yes. A10 SPL won't boot at all. It crashes during the clock manager setup.
This came in right at the beginning of the cycle. I thought the purpose of the 3-month cycle was to allow time to test?
It was ... altera ?
I do plan to try out changing the behaviour to read a parent's ofdata before the child, but I am not comfortable adding such a major change just before a release. It could have any number of ill effects.
Can you update the clock driver? E.g. you could move some of the code from socfpga_a10_ofdata_to_platdata() to a probe() method?
Can we revert the patch which broke arria10 instead ? It did work before, so who knows how many other ill side effects there are ...
No, sorry, we need to fix Altera. Other boards have fixed driver bugs exposed by the patch.
BTW what is a good Altera board to get that doesn't cost too much?
Regards, Simon
On 4/2/20 9:49 PM, Simon Glass wrote:
Hi Marek,
On Thu, 2 Apr 2020 at 13:45, Marek Vasut marex@denx.de wrote:
On 4/2/20 8:50 PM, Simon Glass wrote:
Hi.
Hi,
[...]
>>>> 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.
> Simon, do you have plan to fix this DM core issue ?
I'm not sure it definitely should be changed. But I'll do a patch and see how it looks.
Do I understand it correctly that the patch 82de42fa14682d408da935adfb0f935354c5008f actually completely breaks SoCFPGA ? Then I would say this is a release blocker ?
Yes. A10 SPL won't boot at all. It crashes during the clock manager setup.
This came in right at the beginning of the cycle. I thought the purpose of the 3-month cycle was to allow time to test?
It was ... altera ?
I do plan to try out changing the behaviour to read a parent's ofdata before the child, but I am not comfortable adding such a major change just before a release. It could have any number of ill effects.
Can you update the clock driver? E.g. you could move some of the code from socfpga_a10_ofdata_to_platdata() to a probe() method?
Can we revert the patch which broke arria10 instead ? It did work before, so who knows how many other ill side effects there are ...
No, sorry, we need to fix Altera. Other boards have fixed driver bugs exposed by the patch.
How is altera broken again ? It used to work fine.
BTW what is a good Altera board to get that doesn't cost too much?
With arria10 ? I think there's only one, the a10 socdk ...
On 02.04.2020 21:49, Simon Glass wrote:
Hi Marek,
On Thu, 2 Apr 2020 at 13:45, Marek Vasut marex@denx.de wrote:
On 4/2/20 8:50 PM, Simon Glass wrote:
Hi.
Hi,
[...]
>>>> 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.
> Simon, do you have plan to fix this DM core issue ?
I'm not sure it definitely should be changed. But I'll do a patch and see how it looks.
Do I understand it correctly that the patch 82de42fa14682d408da935adfb0f935354c5008f actually completely breaks SoCFPGA ? Then I would say this is a release blocker ?
Yes. A10 SPL won't boot at all. It crashes during the clock manager setup.
This came in right at the beginning of the cycle. I thought the purpose of the 3-month cycle was to allow time to test?
It was ... altera ?
I do plan to try out changing the behaviour to read a parent's ofdata before the child, but I am not comfortable adding such a major change just before a release. It could have any number of ill effects.
Can you update the clock driver? E.g. you could move some of the code from socfpga_a10_ofdata_to_platdata() to a probe() method?
Can we revert the patch which broke arria10 instead ? It did work before, so who knows how many other ill side effects there are ...
No, sorry, we need to fix Altera. Other boards have fixed driver bugs exposed by the patch.
BTW what is a good Altera board to get that doesn't cost too much?
A problem here might be that you'd need to have a gen5, an A10 and a stratix board to find all problems...
Regards, Simon
Regards, Simon
On 4/2/20 9:53 PM, Simon Goldschmidt wrote:
On 02.04.2020 21:49, Simon Glass wrote:
Hi Marek,
On Thu, 2 Apr 2020 at 13:45, Marek Vasut marex@denx.de wrote:
On 4/2/20 8:50 PM, Simon Glass wrote:
Hi.
Hi,
[...]
>>>>> 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. > >> Simon, do you have plan to fix this DM core issue ? > > I'm not sure it definitely should be changed. But I'll do a patch > and > see how it looks.
Do I understand it correctly that the patch 82de42fa14682d408da935adfb0f935354c5008f actually completely breaks SoCFPGA ? Then I would say this is a release blocker ?
Yes. A10 SPL won't boot at all. It crashes during the clock manager setup.
This came in right at the beginning of the cycle. I thought the purpose of the 3-month cycle was to allow time to test?
It was ... altera ?
I do plan to try out changing the behaviour to read a parent's ofdata before the child, but I am not comfortable adding such a major change just before a release. It could have any number of ill effects.
Can you update the clock driver? E.g. you could move some of the code from socfpga_a10_ofdata_to_platdata() to a probe() method?
Can we revert the patch which broke arria10 instead ? It did work before, so who knows how many other ill side effects there are ...
No, sorry, we need to fix Altera. Other boards have fixed driver bugs exposed by the patch.
BTW what is a good Altera board to get that doesn't cost too much?
A problem here might be that you'd need to have a gen5, an A10 and a stratix board to find all problems...
And agilex too ...
Any links to one?
- Simon
On Thu, 2 Apr 2020 at 13:54, Marek Vasut marex@denx.de wrote:
On 4/2/20 9:53 PM, Simon Goldschmidt wrote:
On 02.04.2020 21:49, Simon Glass wrote:
Hi Marek,
On Thu, 2 Apr 2020 at 13:45, Marek Vasut marex@denx.de wrote:
On 4/2/20 8:50 PM, Simon Glass wrote:
Hi.
Hi,
[...]
>>>>>> 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. >> >>> Simon, do you have plan to fix this DM core issue ? >> >> I'm not sure it definitely should be changed. But I'll do a patch >> and >> see how it looks. > > Do I understand it correctly that the patch > 82de42fa14682d408da935adfb0f935354c5008f actually completely breaks > SoCFPGA ? Then I would say this is a release blocker ? Yes. A10 SPL won't boot at all. It crashes during the clock manager setup.
This came in right at the beginning of the cycle. I thought the purpose of the 3-month cycle was to allow time to test?
It was ... altera ?
I do plan to try out changing the behaviour to read a parent's ofdata before the child, but I am not comfortable adding such a major change just before a release. It could have any number of ill effects.
Can you update the clock driver? E.g. you could move some of the code from socfpga_a10_ofdata_to_platdata() to a probe() method?
Can we revert the patch which broke arria10 instead ? It did work before, so who knows how many other ill side effects there are ...
No, sorry, we need to fix Altera. Other boards have fixed driver bugs exposed by the patch.
BTW what is a good Altera board to get that doesn't cost too much?
A problem here might be that you'd need to have a gen5, an A10 and a stratix board to find all problems...
And agilex too ...
On 02.04.2020 21:54, Marek Vasut wrote:
On 4/2/20 9:53 PM, Simon Goldschmidt wrote:
On 02.04.2020 21:49, Simon Glass wrote:
Hi Marek,
On Thu, 2 Apr 2020 at 13:45, Marek Vasut marex@denx.de wrote:
On 4/2/20 8:50 PM, Simon Glass wrote:
Hi.
Hi,
[...]
>>>>>> 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. >> >>> Simon, do you have plan to fix this DM core issue ? >> >> I'm not sure it definitely should be changed. But I'll do a patch >> and >> see how it looks. > > Do I understand it correctly that the patch > 82de42fa14682d408da935adfb0f935354c5008f actually completely breaks > SoCFPGA ? Then I would say this is a release blocker ? Yes. A10 SPL won't boot at all. It crashes during the clock manager setup.
This came in right at the beginning of the cycle. I thought the purpose of the 3-month cycle was to allow time to test?
It was ... altera ?
I do plan to try out changing the behaviour to read a parent's ofdata before the child, but I am not comfortable adding such a major change just before a release. It could have any number of ill effects.
Can you update the clock driver? E.g. you could move some of the code from socfpga_a10_ofdata_to_platdata() to a probe() method?
Can we revert the patch which broke arria10 instead ? It did work before, so who knows how many other ill side effects there are ...
No, sorry, we need to fix Altera. Other boards have fixed driver bugs exposed by the patch.
BTW what is a good Altera board to get that doesn't cost too much?
A problem here might be that you'd need to have a gen5, an A10 and a stratix board to find all problems...
And agilex too ...
Hmm, I thought we would try and keep stratix and agilex more close than gen5/A10...
I even don't have any non-gen5 boards myself here, so I cannot even test those, either.
But in the end, yes, it would be a good thing to have all those boards execute basic tests at least after rc1 has been tagged. I wonder if OSADL could help out here, given they already have a broad range of boards testing linux-rt already. Or do we have a separate hardware area for this?
Regards, Simon
On Thu, Apr 02, 2020 at 09:45:25PM +0200, Marek Vasut wrote:
On 4/2/20 8:50 PM, Simon Glass wrote:
Hi.
Hi,
[...]
>>> 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.
Simon, do you have plan to fix this DM core issue ?
I'm not sure it definitely should be changed. But I'll do a patch and see how it looks.
Do I understand it correctly that the patch 82de42fa14682d408da935adfb0f935354c5008f actually completely breaks SoCFPGA ? Then I would say this is a release blocker ?
Yes. A10 SPL won't boot at all. It crashes during the clock manager setup.
This came in right at the beginning of the cycle. I thought the purpose of the 3-month cycle was to allow time to test?
It was ... altera ?
Sorry, I'm missing how that's an answer to the question. This came in basically right at the start of the merge window.
I do plan to try out changing the behaviour to read a parent's ofdata before the child, but I am not comfortable adding such a major change just before a release. It could have any number of ill effects.
Can you update the clock driver? E.g. you could move some of the code from socfpga_a10_ofdata_to_platdata() to a probe() method?
Can we revert the patch which broke arria10 instead ? It did work before, so who knows how many other ill side effects there are ...
I'm not in favor of reverting 82de42fa1468 unless the answer is "that commit was wrong" and not "that commit caused other problems to surface, that we can fix". I am quite willing to say "we need to delay the release to test changes for problems that've surfaced and verify more hardware is fine".
On 4/2/20 10:54 PM, Tom Rini wrote: [...]
I'm not sure it definitely should be changed. But I'll do a patch and see how it looks.
Do I understand it correctly that the patch 82de42fa14682d408da935adfb0f935354c5008f actually completely breaks SoCFPGA ? Then I would say this is a release blocker ?
Yes. A10 SPL won't boot at all. It crashes during the clock manager setup.
This came in right at the beginning of the cycle. I thought the purpose of the 3-month cycle was to allow time to test?
It was ... altera ?
Sorry, I'm missing how that's an answer to the question. This came in basically right at the start of the merge window.
I don't have an A10 available right now, so what can I do ?
I do plan to try out changing the behaviour to read a parent's ofdata before the child, but I am not comfortable adding such a major change just before a release. It could have any number of ill effects.
Can you update the clock driver? E.g. you could move some of the code from socfpga_a10_ofdata_to_platdata() to a probe() method?
Can we revert the patch which broke arria10 instead ? It did work before, so who knows how many other ill side effects there are ...
I'm not in favor of reverting 82de42fa1468 unless the answer is "that commit was wrong" and not "that commit caused other problems to surface, that we can fix". I am quite willing to say "we need to delay the release to test changes for problems that've surfaced and verify more hardware is fine".
If I understand this correctly, the clock code did scan the DT and bound all the clocks, so that drivers can use them. That's how bind function should work, it creates instances of devices, but without probing them. This doesn't work anymore if I understand it correctly, but that means the basic premise of DT is broken ? And the solution is to do more magic in probe function , which I don't think even works here ?
Hi Marek,
On Thu, 2 Apr 2020 at 15:07, Marek Vasut marex@denx.de wrote:
On 4/2/20 10:54 PM, Tom Rini wrote: [...]
> I'm not sure it definitely should be changed. But I'll do a patch and > see how it looks.
Do I understand it correctly that the patch 82de42fa14682d408da935adfb0f935354c5008f actually completely breaks SoCFPGA ? Then I would say this is a release blocker ?
Yes. A10 SPL won't boot at all. It crashes during the clock manager setup.
This came in right at the beginning of the cycle. I thought the purpose of the 3-month cycle was to allow time to test?
It was ... altera ?
Sorry, I'm missing how that's an answer to the question. This came in basically right at the start of the merge window.
I don't have an A10 available right now, so what can I do ?
I do plan to try out changing the behaviour to read a parent's ofdata before the child, but I am not comfortable adding such a major change just before a release. It could have any number of ill effects.
Can you update the clock driver? E.g. you could move some of the code from socfpga_a10_ofdata_to_platdata() to a probe() method?
Can we revert the patch which broke arria10 instead ? It did work before, so who knows how many other ill side effects there are ...
I'm not in favor of reverting 82de42fa1468 unless the answer is "that commit was wrong" and not "that commit caused other problems to surface, that we can fix". I am quite willing to say "we need to delay the release to test changes for problems that've surfaced and verify more hardware is fine".
If I understand this correctly, the clock code did scan the DT and bound all the clocks, so that drivers can use them. That's how bind function should work, it creates instances of devices, but without probing them. This doesn't work anymore if I understand it correctly, but that means the basic premise of DT is broken ? And the solution is to do more magic in probe function , which I don't think even works here ?
No...please see here from the cover letter:
Cover-letter: dm: core: Fully separate ofdata_to_platdata() from probe() At present ofdata_to_platdata() is done as part of device_probe(). Drivers are supposed to read their platdata in the ofdata_to_platdata method() but this requirement is not always followed.
Having these methods separate helps deal with of-platdata, where the probe() method is common to both DT/of-platdata operation, but the ofdata_to_platdata() method is implemented differently.
Another case has come up where this separate is useful. Generation of ACPI tables uses the of-platdata but does not want to probe the device. Probing would cause U-Boot to violate one of its design principles, viz that it should only probe devices that are used. For ACPI we want to generate a table for each device, even if U-Boot does not use it. In fact it may not even be possible to probe the device - e.g. an SD card which is not present will cause an error on probe, yet we still must tell Linux about the SD card connector in case it is used while Linux is running.
This series splits out the ofdata_to_platdata() part of device_probe() so that it can be used separately from device_probe(). Thus devices now go through two distinct states when probing: reading platform data and actually touching the hardware to bring the device up.
This should not break existing boards since the operations still happen in mostly the same order. The main change is that parents and uclasses are probed after ofdata_to_platdata() is called.
HOWEVER it is quite possible that some boards break the rules and due to a series of unfortunate events, something will break. Two boards were found in this category already. SO this series may require some tidying up from board maintainers, if problems arise.
Note that there are cases where devices must be probed in the ofdata_to_platdata() method. An example is where a GPIO is selected - this obviously requires that the GPIO device is probed.
One board was found to burst its size limit with this series, despite the very small size increase. The patches to remove use of BUG_ON() are to ensure that this series passes tests. END
Regards, Simon
On Thu, Apr 02, 2020 at 11:07:31PM +0200, Marek Vasut wrote:
On 4/2/20 10:54 PM, Tom Rini wrote: [...]
> I'm not sure it definitely should be changed. But I'll do a patch and > see how it looks.
Do I understand it correctly that the patch 82de42fa14682d408da935adfb0f935354c5008f actually completely breaks SoCFPGA ? Then I would say this is a release blocker ?
Yes. A10 SPL won't boot at all. It crashes during the clock manager setup.
This came in right at the beginning of the cycle. I thought the purpose of the 3-month cycle was to allow time to test?
It was ... altera ?
Sorry, I'm missing how that's an answer to the question. This came in basically right at the start of the merge window.
I don't have an A10 available right now, so what can I do ?
Ah, so the answer is "I can't test this platform myself". That's what then, thanks.
On 4/3/20 12:10 AM, Tom Rini wrote:
On Thu, Apr 02, 2020 at 11:07:31PM +0200, Marek Vasut wrote:
On 4/2/20 10:54 PM, Tom Rini wrote: [...]
>> I'm not sure it definitely should be changed. But I'll do a patch and >> see how it looks. > > Do I understand it correctly that the patch > 82de42fa14682d408da935adfb0f935354c5008f actually completely breaks > SoCFPGA ? Then I would say this is a release blocker ? Yes. A10 SPL won't boot at all. It crashes during the clock manager setup.
This came in right at the beginning of the cycle. I thought the purpose of the 3-month cycle was to allow time to test?
It was ... altera ?
Sorry, I'm missing how that's an answer to the question. This came in basically right at the start of the merge window.
I don't have an A10 available right now, so what can I do ?
Ah, so the answer is "I can't test this platform myself". That's what then, thanks.
Clearly Altera can , since they reported this issue.
-----Original Message----- From: Marek Vasut marex@denx.de Sent: Friday, April 3, 2020 6:47 AM To: Tom Rini trini@konsulko.com Cc: Simon Glass sjg@chromium.org; Ang, Chee Hong chee.hong.ang@intel.com; U-Boot Mailing List u-boot@lists.denx.de; Simon Goldschmidt simon.k.r.goldschmidt@gmail.com; See, Chin Liang chin.liang.see@intel.com; Tan, Ley Foon ley.foon.tan@intel.com; Westergreen, Dalon dalon.westergreen@intel.com Subject: Re: [PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function
On 4/3/20 12:10 AM, Tom Rini wrote:
On Thu, Apr 02, 2020 at 11:07:31PM +0200, Marek Vasut wrote:
On 4/2/20 10:54 PM, Tom Rini wrote: [...]
>>> I'm not sure it definitely should be changed. But I'll do a >>> patch and see how it looks. >> >> Do I understand it correctly that the patch >> 82de42fa14682d408da935adfb0f935354c5008f actually completely >> breaks SoCFPGA ? Then I would say this is a release blocker ? > Yes. A10 SPL won't boot at all. It crashes during the clock manager
setup.
This came in right at the beginning of the cycle. I thought the purpose of the 3-month cycle was to allow time to test?
It was ... altera ?
Sorry, I'm missing how that's an answer to the question. This came in basically right at the start of the merge window.
I don't have an A10 available right now, so what can I do ?
Ah, so the answer is "I can't test this platform myself". That's what then, thanks.
Clearly Altera can , since they reported this issue.
Yes, we have Arria10 devkit here and we see the issue when testing latest uboot. It looks like other platform [1] also encounter issue with patch 82de42fa14682d40. Like Marek said, we don't know any other driver is broken after this patch.
[1] https://patchwork.ozlabs.org/patch/1265188/
Regards Ley Foon
On Fri, Apr 03, 2020 at 03:52:54AM +0000, Tan, Ley Foon wrote:
-----Original Message----- From: Marek Vasut marex@denx.de Sent: Friday, April 3, 2020 6:47 AM To: Tom Rini trini@konsulko.com Cc: Simon Glass sjg@chromium.org; Ang, Chee Hong chee.hong.ang@intel.com; U-Boot Mailing List u-boot@lists.denx.de; Simon Goldschmidt simon.k.r.goldschmidt@gmail.com; See, Chin Liang chin.liang.see@intel.com; Tan, Ley Foon ley.foon.tan@intel.com; Westergreen, Dalon dalon.westergreen@intel.com Subject: Re: [PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function
On 4/3/20 12:10 AM, Tom Rini wrote:
On Thu, Apr 02, 2020 at 11:07:31PM +0200, Marek Vasut wrote:
On 4/2/20 10:54 PM, Tom Rini wrote: [...]
>>>> I'm not sure it definitely should be changed. But I'll do a >>>> patch and see how it looks. >>> >>> Do I understand it correctly that the patch >>> 82de42fa14682d408da935adfb0f935354c5008f actually completely >>> breaks SoCFPGA ? Then I would say this is a release blocker ? >> Yes. A10 SPL won't boot at all. It crashes during the clock manager
setup.
> > This came in right at the beginning of the cycle. I thought the > purpose of the 3-month cycle was to allow time to test?
It was ... altera ?
Sorry, I'm missing how that's an answer to the question. This came in basically right at the start of the merge window.
I don't have an A10 available right now, so what can I do ?
Ah, so the answer is "I can't test this platform myself". That's what then, thanks.
Clearly Altera can , since they reported this issue.
Yes, we have Arria10 devkit here and we see the issue when testing latest uboot. It looks like other platform [1] also encounter issue with patch 82de42fa14682d40. Like Marek said, we don't know any other driver is broken after this patch.
Yes, my concern here is that the platform in question wasn't tested after -rc1 or -rc2 or -rc3 only right now just before release. So we're in a bit of a scramble to fix the driver. Thanks!
-----Original Message----- From: Tom Rini trini@konsulko.com Sent: Friday, April 3, 2020 8:21 PM To: Tan, Ley Foon ley.foon.tan@intel.com Cc: Marek Vasut marex@denx.de; Simon Glass sjg@chromium.org; Ang, Chee Hong chee.hong.ang@intel.com; U-Boot Mailing List <u- boot@lists.denx.de>; Simon Goldschmidt simon.k.r.goldschmidt@gmail.com; See, Chin Liang chin.liang.see@intel.com; Westergreen, Dalon dalon.westergreen@intel.com Subject: Re: [PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function
On Fri, Apr 03, 2020 at 03:52:54AM +0000, Tan, Ley Foon wrote:
-----Original Message----- From: Marek Vasut marex@denx.de Sent: Friday, April 3, 2020 6:47 AM To: Tom Rini trini@konsulko.com Cc: Simon Glass sjg@chromium.org; Ang, Chee Hong chee.hong.ang@intel.com; U-Boot Mailing List u-boot@lists.denx.de; Simon Goldschmidt simon.k.r.goldschmidt@gmail.com; See, Chin Liang chin.liang.see@intel.com; Tan, Ley Foon ley.foon.tan@intel.com; Westergreen, Dalon dalon.westergreen@intel.com Subject: Re: [PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function
On 4/3/20 12:10 AM, Tom Rini wrote:
On Thu, Apr 02, 2020 at 11:07:31PM +0200, Marek Vasut wrote:
On 4/2/20 10:54 PM, Tom Rini wrote: [...]
>>>>> I'm not sure it definitely should be changed. But I'll do a >>>>> patch and see how it looks. >>>> >>>> Do I understand it correctly that the patch >>>> 82de42fa14682d408da935adfb0f935354c5008f actually
completely
>>>> breaks SoCFPGA ? Then I would say this is a release blocker ? >>> Yes. A10 SPL won't boot at all. It crashes during the clock >>> manager
setup.
>> >> This came in right at the beginning of the cycle. I thought >> the purpose of the 3-month cycle was to allow time to test? > > It was ... altera ?
Sorry, I'm missing how that's an answer to the question. This came in basically right at the start of the merge window.
I don't have an A10 available right now, so what can I do ?
Ah, so the answer is "I can't test this platform myself". That's what then, thanks.
Clearly Altera can , since they reported this issue.
Yes, we have Arria10 devkit here and we see the issue when testing latest
uboot.
It looks like other platform [1] also encounter issue with patch
82de42fa14682d40. Like Marek said, we don't know any other driver is broken after this patch.
Yes, my concern here is that the platform in question wasn't tested after -rc1 or -rc2 or -rc3 only right now just before release. So we're in a bit of a scramble to fix the driver. Thanks!
We will improve our regression test going forward. Sorry about that.
Thanks.
Regards Ley Foon
On Mon, Mar 09, 2020 at 01:21:59AM -0700, chee.hong.ang@intel.com wrote:
From: Chee Hong Ang chee.hong.ang@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@intel.com Reviewed-by: Ley Foon Tan ley.foon.tan@intel.com
Applied to u-boot/master, thanks!
On 4/6/20 1:28 PM, Tom Rini wrote:
On Mon, Mar 09, 2020 at 01:21:59AM -0700, chee.hong.ang@intel.com wrote:
From: Chee Hong Ang chee.hong.ang@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@intel.com Reviewed-by: Ley Foon Tan ley.foon.tan@intel.com
Applied to u-boot/master, thanks!
You probably want to replace this one with [PATCH] dm: core: Read parent ofdata before children
From: Chee Hong Ang chee.hong.ang@intel.com
Replace FDT API with more generic ofnode API.
Signed-off-by: Chee Hong Ang chee.hong.ang@intel.com --- drivers/clk/altera/clk-arria10.c | 52 +++++++++++++++++++--------------------- 1 file changed, 25 insertions(+), 27 deletions(-)
diff --git a/drivers/clk/altera/clk-arria10.c b/drivers/clk/altera/clk-arria10.c index b7eed94..01bf5ec 100644 --- a/drivers/clk/altera/clk-arria10.c +++ b/drivers/clk/altera/clk-arria10.c @@ -190,16 +190,16 @@ static struct clk_ops socfpga_a10_clk_ops = { static void socfpga_a10_handoff_workaround(struct udevice *dev) { struct socfpga_a10_clk_platdata *plat = dev_get_platdata(dev); - const void *fdt = gd->fdt_blob; struct clk_bulk *bulk = &plat->clks; - int i, ret, offset = dev_of_offset(dev); + ofnode node = dev_ofnode(dev); + int i, ret; static const char * const socfpga_a10_fixedclk_map[] = { "osc1", "altera_arria10_hps_eosc1", "cb_intosc_ls_clk", "altera_arria10_hps_cb_intosc_ls", "f2s_free_clk", "altera_arria10_hps_f2h_free", };
- if (fdt_node_check_compatible(fdt, offset, "fixed-clock")) + if (!ofnode_device_is_compatible(node, "fixed-clock")) return;
for (i = 0; i < ARRAY_SIZE(socfpga_a10_fixedclk_map); i += 2) @@ -227,42 +227,41 @@ static void socfpga_a10_handoff_workaround(struct udevice *dev)
static int socfpga_a10_clk_bind(struct udevice *dev) { - const void *fdt = gd->fdt_blob; - int offset = dev_of_offset(dev); bool pre_reloc_only = !(gd->flags & GD_FLG_RELOC); const char *name; + ofnode node; int ret;
- for (offset = fdt_first_subnode(fdt, offset); - offset > 0; - offset = fdt_next_subnode(fdt, offset)) { - name = fdt_get_name(fdt, offset, NULL); + for (node = dev_read_first_subnode(dev); + ofnode_valid(node); + node = ofnode_next_subnode(node)) { + name = ofnode_get_name(node); if (!name) return -EINVAL;
if (!strcmp(name, "clocks")) { - offset = fdt_first_subnode(fdt, offset); - name = fdt_get_name(fdt, offset, NULL); + node = ofnode_first_subnode(node); + name = ofnode_get_name(node); if (!name) return -EINVAL; }
/* Filter out supported sub-clock */ - if (fdt_node_check_compatible(fdt, offset, + if (!ofnode_device_is_compatible(node, "altr,socfpga-a10-pll-clock") && - fdt_node_check_compatible(fdt, offset, + !ofnode_device_is_compatible(node, "altr,socfpga-a10-perip-clk") && - fdt_node_check_compatible(fdt, offset, + !ofnode_device_is_compatible(node, "altr,socfpga-a10-gate-clk") && - fdt_node_check_compatible(fdt, offset, "fixed-clock")) + !ofnode_device_is_compatible(node, "fixed-clock")) continue;
if (pre_reloc_only && - !dm_ofnode_pre_reloc(offset_to_ofnode(offset))) + !dm_ofnode_pre_reloc(node)) continue;
ret = device_bind_driver_to_node(dev, "clk-a10", name, - offset_to_ofnode(offset), + node, NULL); if (ret) return ret; @@ -273,18 +272,17 @@ static int socfpga_a10_clk_bind(struct udevice *dev)
static int socfpga_a10_clk_probe(struct udevice *dev) { + ofnode node = dev_ofnode(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);
clk_get_bulk(dev, &plat->clks);
socfpga_a10_handoff_workaround(dev);
- if (!fdt_node_check_compatible(fdt, offset, "altr,clk-mgr")) { - plat->regs = devfdt_get_addr(dev); + if (ofnode_device_is_compatible(node, "altr,clk-mgr")) { + plat->regs = ofnode_get_addr(node); } else { pdev = dev_get_parent(dev); if (!pdev) @@ -298,18 +296,18 @@ static int socfpga_a10_clk_probe(struct udevice *dev) plat->regs = pplat->regs; }
- if (!fdt_node_check_compatible(fdt, offset, - "altr,socfpga-a10-pll-clock")) { + if (ofnode_device_is_compatible(node, + "altr,socfpga-a10-pll-clock")) { /* Main PLL has 3 upstream clock */ if (plat->clks.count == 3) plat->type = SOCFPGA_A10_CLK_MAIN_PLL; else plat->type = SOCFPGA_A10_CLK_PER_PLL; - } else if (!fdt_node_check_compatible(fdt, offset, - "altr,socfpga-a10-perip-clk")) { + } else if (ofnode_device_is_compatible(node, + "altr,socfpga-a10-perip-clk")) { plat->type = SOCFPGA_A10_CLK_PERIP_CLK; - } else if (!fdt_node_check_compatible(fdt, offset, - "altr,socfpga-a10-gate-clk")) { + } else if (ofnode_device_is_compatible(node, + "altr,socfpga-a10-gate-clk")) { plat->type = SOCFPGA_A10_CLK_GATE_CLK; } else { plat->type = SOCFPGA_A10_CLK_UNKNOWN_CLK;
-----Original Message----- From: Ang, Chee Hong chee.hong.ang@intel.com Sent: Monday, March 9, 2020 4:22 PM To: u-boot@lists.denx.de Cc: Marek Vasut marex@denx.de; Simon Goldschmidt simon.k.r.goldschmidt@gmail.com; See, Chin Liang chin.liang.see@intel.com; Tan, Ley Foon ley.foon.tan@intel.com; Westergreen, Dalon dalon.westergreen@intel.com; Ang, Chee Hong chee.hong.ang@intel.com Subject: [PATCH v1 2/2] clk: socfpga: Switch to use ofnode API
From: Chee Hong Ang chee.hong.ang@intel.com
Replace FDT API with more generic ofnode API.
Signed-off-by: Chee Hong Ang chee.hong.ang@intel.com
Reviewed-by: Ley Foon Tan ley.foon.tan@intel.com
participants (7)
-
Ang, Chee Hong -
chee.hong.ang＠intel.com
-
Marek Vasut -
Simon Glass -
Simon Goldschmidt -
Tan, Ley Foon -
Tom Rini