[PATCH v1 0/2] Fix A10 clock driver crash after changes in DM core

[PATCH v1 0/2] Fix A10 clock driver crash after changes in DM core

[chee.hong.ang at intel.com]

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(-)

-- 
2.7.4

[PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function

[chee.hong.ang at intel.com]

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;
 
-- 
2.7.4

[PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function

[Tan, Ley Foon]

> -----Original Message-----
> From: Ang, Chee Hong <chee.hong.ang@intel.com>
> Sent: Monday, March 9, 2020 4:22 PM
> To: u-boot at 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>

[PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function

[Marek Vasut]

On 3/9/20 9:21 AM, chee.hong.ang at 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;
>  
> 


-- 
Best regards,
Marek Vasut

[PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function

[Ang, Chee Hong]

> On 3/9/20 9:21 AM, chee.hong.ang at 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

[PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function

[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@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.

[PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function

[Simon Glass]

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

[PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function

[Marek Vasut]

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.

-- 
Best regards,
Marek Vasut

[PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function

[Simon Glass]

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

[PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function

[Ang, Chee Hong]

> 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 ?

[PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function

[Simon Glass]

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

[PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function

[Marek Vasut]

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 ?

[PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function

[Ang, Chee Hong]

> 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.

[PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function

[Simon Glass]

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

[PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function

[Marek Vasut]

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 ...

-- 
Best regards,
Marek Vasut

[PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function

[Simon Glass]

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

[PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function

[Marek Vasut]

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 ...

[PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function

[Simon Goldschmidt]

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
> 

[PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function

[Marek Vasut]

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 ...

[PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function

[Simon Glass]

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 ...

[PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function

[Simon Goldschmidt]

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

[PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function

[Tom Rini]

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".

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200402/e30fda06/attachment.sig>

[PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function

[Marek Vasut]

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 ?

[PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function

[Simon Glass]

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

[PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function

[Tom Rini]

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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200402/ea1c8fa7/attachment.sig>

[PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function

[Marek Vasut]

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.

[PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function

[Tan, Ley Foon]

> -----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

[PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function

[Tom Rini]

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.
> 
> [1] https://patchwork.ozlabs.org/patch/1265188/ 

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200403/105d2ec2/attachment.sig>

[PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function

[Tan, Ley Foon]

> -----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 at 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.
> >
> > [1] https://patchwork.ozlabs.org/patch/1265188/
> 
> 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 

[PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function

[Tom Rini]

On Mon, Mar 09, 2020 at 01:21:59AM -0700, chee.hong.ang at 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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200406/d63d249c/attachment.sig>

[PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function

[Marek Vasut]

On 4/6/20 1:28 PM, Tom Rini wrote:
> On Mon, Mar 09, 2020 at 01:21:59AM -0700, chee.hong.ang at 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

[PATCH v1 2/2] clk: socfpga: Switch to use ofnode API

[chee.hong.ang at intel.com]

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;
-- 
2.7.4

[PATCH v1 2/2] clk: socfpga: Switch to use ofnode API

[Tan, Ley Foon]

> -----Original Message-----
> From: Ang, Chee Hong <chee.hong.ang@intel.com>
> Sent: Monday, March 9, 2020 4:22 PM
> To: u-boot at 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>