Re: [PATCH] of/platform: Support dynamic device tree on AMBA bus

[Rob Herring]

On Tue, Nov 06, 2018 at 01:49:05AM +0900, Jaewon Kim wrote:
> Hi Rob,
> 
> On 11/1/18 3:31 AM, Rob Herring wrote:
> > On Thu, Oct 25, 2018 at 11:40 AM Jaewon Kim <jaewon02.kim@xxxxxxxxx> wrote:
> > > This patch supports dynamic device-tree for AMBA device.
> > > The AMBA device must be registered on the AMBA bus, not the platform bus.
> > I'm not convinced we should even support this. There's a limited
> > number of AMBA devices. They would almost certainly be on-chip and
> > static. I suppose in theory you could have them in an FPGA, but
> > generally the FPGA vendors have their own IP blocks and don't use ARM
> > Primecell IP. So what is the usecase?
> 
> In my case, our target has GPIO output pin, like RPI board. And I want to
> use dt-overlay to change GPIO alternative function. But, I found that AMBA
> device not work with dt-overlay. Most AMBA devices do not require dynamic
> device-tree. But, pl022(serial) and pl011(SPI) needs dynamic device-tree.

The bootloader should apply the overlay.

> 
> > 
> > > Signed-off-by: Jaewon Kim <jaewon02.kim@xxxxxxxxxxx>
> > Your author and S-o-b emails should match.
> Okay, I will change it my gmail.
> > 
> > > ---
> > >   drivers/of/platform.c | 93 ++++++++++++++++++++++++++++++++++++++++-----------
> > >   1 file changed, 73 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > > index 04ad312..b9ac105 100644
> > > --- a/drivers/of/platform.c
> > > +++ b/drivers/of/platform.c
> > > @@ -286,6 +286,22 @@ static struct amba_device *of_amba_device_create(struct device_node *node,
> > >          of_node_clear_flag(node, OF_POPULATED);
> > >          return NULL;
> > >   }
> > > +
> > > +/**
> > > + * of_find_amba_device_by_node - Find the amba_device associated with a node
> > > + * @np: Pointer to device tree node
> > > + *
> > > + * Returns amba_device pointer, or NULL if not found
> > > + */
> > > +struct amba_device *of_find_amba_device_by_node(struct device_node *np)
> > > +{
> > > +       struct device *dev;
> > > +
> > > +       dev = bus_find_device(&amba_bustype, NULL, np, of_dev_node_match);
> > > +       return dev ? to_amba_device(dev) : NULL;
> > > +}
> > > +EXPORT_SYMBOL(of_find_amba_device_by_node);
> > I would prefer we add (or change the platform device version) an
> > of_find_device_by_node which can be extended to different bus types.
> 
> The returned type is different. of_find_device_by_node () returns 'struct
> platform_device *', and of_find_amba_device_by_node() returns 'struct
> amba_device *'. To make this the same, It need to modify return value of
> of_find_device_by_node() function or merge amba_device to
> platform_device.of_find_device_by_node() function is a widely used kernel
> source and it requires a lot of modifications.I think it would be simpler to
> make of_find_amba_device_by_node().

You don't have to make treewide changes. Define a new function returning 
struct device. Make of_find_device_by_node() a wrapper that gets the 
platform_device from the struct device.

> 
> > 
> > > +
> > >   #else /* CONFIG_ARM_AMBA */
> > >   static struct amba_device *of_amba_device_create(struct device_node *node,
> > >                                                   const char *bus_id,
> > > @@ -294,6 +310,12 @@ static struct amba_device *of_amba_device_create(struct device_node *node,
> > >   {
> > >          return NULL;
> > >   }
> > > +
> > > +struct amba_device *of_find_amba_device_by_node(struct device_node *np)
> > > +{
> > > +       return NULL;
> > > +}
> > > +EXPORT_SYMBOL(of_find_amba_device_by_node);
> > >   #endif /* CONFIG_ARM_AMBA */
> > > 
> > >   /**
> > > @@ -665,6 +687,7 @@ static int of_platform_notify(struct notifier_block *nb,
> > >   {
> > >          struct of_reconfig_data *rd = arg;
> > >          struct platform_device *pdev_parent, *pdev;
> > > +       struct amba_device *adev_p, *adev;
> > >          bool children_left;
> > > 
> > >          switch (of_reconfig_get_state_change(action, rd)) {
> > > @@ -677,17 +700,34 @@ static int of_platform_notify(struct notifier_block *nb,
> > >                  if (of_node_check_flag(rd->dn, OF_POPULATED))
> > >                          return NOTIFY_OK;
> > > 
> > > -               /* pdev_parent may be NULL when no bus platform device */
> > > -               pdev_parent = of_find_device_by_node(rd->dn->parent);
> > > -               pdev = of_platform_device_create(rd->dn, NULL,
> > > -                               pdev_parent ? &pdev_parent->dev : NULL);
> > > -               of_dev_put(pdev_parent);
> > > -
> > > -               if (pdev == NULL) {
> > > -                       pr_err("%s: failed to create for '%pOF'\n",
> > > -                                       __func__, rd->dn);
> > > -                       /* of_platform_device_create tosses the error code */
> > > -                       return notifier_from_errno(-EINVAL);
> > > +               if (of_device_is_compatible(rd->dn, "arm,primecell")) {
> > > +                       /* adev_p may be NULL when no bus amba device */
> > > +                       adev_p = of_find_amba_device_by_node(rd->dn->parent);
> > An amba_device can't ever have a parent that's an amba_device. The
> > parent of an amba_device is typically a platform_device for a
> > 'simple-bus'.
> 
> You are right. there is no parent device .
> I will remove it in v2.
> 
> > 
> > > +                       adev = of_amba_device_create(rd->dn, NULL, NULL,
> > > +                                       adev_p ? &adev_p->dev : NULL);
> > > +
> > > +                       if (adev_p)
> > > +                               put_device(&adev_p->dev);
> > > +
> > > +                       if (adev == NULL) {
> > > +                               pr_err("%s: failed to create for '%s'\n",
> > > +                                               __func__, rd->dn->full_name);
> > > +                               /* of_amba_device_create tosses the error */
> > > +                               return notifier_from_errno(-EINVAL);
> > > +                       }
> > > +               } else {
> > > +                       /* pdev_parent may be NULL when no bus platform device*/
> > > +                       pdev_parent = of_find_device_by_node(rd->dn->parent);
> > > +                       pdev = of_platform_device_create(rd->dn, NULL,
> > > +                                       pdev_parent ? &pdev_parent->dev : NULL);
> > > +                       of_dev_put(pdev_parent);
> > > +
> > > +                       if (pdev == NULL) {
> > > +                               pr_err("%s: failed to create for '%pOF'\n",
> > > +                                               __func__, rd->dn);
> > > +                               /* of_platform_device_create tosses the error */
> > > +                               return notifier_from_errno(-EINVAL);
> > > +                       }
> > This all pretty much duplicates what of_platform_bus_create() does and
> > we should use it here rather than have another copy. Plus what about
> > handling of any child devices (in the platform device case).
> 
> The code looks duplicated, but processed type of variable is
> different.(struct amba_device, struct platform_device)

of_platform_bus_create() works on both. Also, it handles some conditions 
not handled here.

Rob