[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 at intel.com> wrote:
>
> From: Chee Hong Ang <chee.hong.ang at intel.com>
>
> This commit (82de42fa14682d408da935adfb0f935354c5008f) calls child's
> ofdata_to_platdata() method before the parent is probed in dm core.
> This has caused the driver no longer able to get the correct parent
> clock's register base in the ofdata_to_platdata() method because the
> parent clocks will only be probed after the child's ofdata_to_platdata().
> To resolve this, the clock parent's register base will only be retrieved
> by the child in probe() method instead of ofdata_to_platdata().

I think one thing that is going on here is that DM allows ofdata to be
read for a device before its parent devices have been read, but it
requires that parent devices be probed before their children.

The idea is that it should be possible to read the ofdata for a node
without needing to have done so for parents. But perhaps this
assumption is too brave?

I suspect we could change this, so that device_ofdata_to_platdata()
first calls itself on its parent.

I can think of various reasons why this change might be desirable.

>
> Signed-off-by: Chee Hong Ang <chee.hong.ang at intel.com>
> ---
>  drivers/clk/altera/clk-arria10.c | 40 ++++++++++++++++++----------------------
>  1 file changed, 18 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/clk/altera/clk-arria10.c b/drivers/clk/altera/clk-arria10.c
> index affeb31..b7eed94 100644
> --- a/drivers/clk/altera/clk-arria10.c
> +++ b/drivers/clk/altera/clk-arria10.c
> @@ -274,6 +274,8 @@ static int socfpga_a10_clk_bind(struct udevice *dev)
>  static int socfpga_a10_clk_probe(struct udevice *dev)
>  {
>         struct socfpga_a10_clk_platdata *plat = dev_get_platdata(dev);
> +       struct socfpga_a10_clk_platdata *pplat;
> +       struct udevice *pdev;
>         const void *fdt = gd->fdt_blob;
>         int offset = dev_of_offset(dev);
>
> @@ -281,6 +283,21 @@ static int socfpga_a10_clk_probe(struct udevice *dev)
>
>         socfpga_a10_handoff_workaround(dev);
>
> +       if (!fdt_node_check_compatible(fdt, offset, "altr,clk-mgr")) {

This is strange to me. I think the idea here is to detect whether this
is the root clk node or one of the ones created in the bind() function
above. Is that right?

If so, it should be enough to say:

   if (dev_ofvalid(dev))

If you actually need to distinguish between different compatible
strings, you can use the .data member in your udevice_id table, with
dev_get_driver_data().

> +               plat->regs = devfdt_get_addr(dev);
> +       } else {
> +               pdev = dev_get_parent(dev);
> +               if (!pdev)
> +                       return -ENODEV;
> +
> +               pplat = dev_get_platdata(pdev);
> +               if (!pplat)
> +                       return -EINVAL;
> +
> +               plat->ctl_reg = dev_read_u32_default(dev, "reg", 0x0);
> +               plat->regs = pplat->regs;
> +       }
> +
>         if (!fdt_node_check_compatible(fdt, offset,
>                                        "altr,socfpga-a10-pll-clock")) {
>                 /* Main PLL has 3 upstream clock */
> @@ -304,29 +321,8 @@ static int socfpga_a10_clk_probe(struct udevice *dev)
>  static int socfpga_a10_ofdata_to_platdata(struct udevice *dev)
>  {
>         struct socfpga_a10_clk_platdata *plat = dev_get_platdata(dev);
> -       struct socfpga_a10_clk_platdata *pplat;
> -       struct udevice *pdev;
> -       const void *fdt = gd->fdt_blob;
>         unsigned int divreg[3], gatereg[2];
> -       int ret, offset = dev_of_offset(dev);
> -       u32 regs;
> -
> -       regs = dev_read_u32_default(dev, "reg", 0x0);
> -
> -       if (!fdt_node_check_compatible(fdt, offset, "altr,clk-mgr")) {
> -               plat->regs = devfdt_get_addr(dev);
> -       } else {
> -               pdev = dev_get_parent(dev);
> -               if (!pdev)
> -                       return -ENODEV;
> -
> -               pplat = dev_get_platdata(pdev);
> -               if (!pplat)
> -                       return -EINVAL;
> -
> -               plat->ctl_reg = regs;
> -               plat->regs = pplat->regs;
> -       }
> +       int ret;
>
>         plat->type = SOCFPGA_A10_CLK_UNKNOWN_CLK;
>
> --
> 2.7.4
>

Regards,
Simon