Message-ID: <20181205070046.GD31204@localhost.localdomain>
Date:   Wed, 5 Dec 2018 09:00:46 +0200
From:   Matti Vaittinen <matti.vaittinen@...rohmeurope.com>
To:     Stephen Boyd <sboyd@...nel.org>
Cc:     mazziesaccount@...il.com, mturquette@...libre.com,
        cw00.choi@...sung.com, krzk@...nel.org, b.zolnierkie@...sung.com,
        linux@...linux.org.uk, andy.gross@...aro.org,
        david.brown@...aro.org, pavel@....cz, andrew.smirnov@...il.com,
        pombredanne@...b.com, sjhuang@...vatar.ai, akshu.agrawal@....com,
        djkurtz@...omium.org, rafael.j.wysocki@...el.com,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-clk@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v6 03/10] clk: of-provider: look at parent if registered
 device has no provider info

Hello Stephen,

I copied some parts of the v4 discussion here as well. Let's continue
them under this one email thread. (and yep, this is my bad we now have
multiple email threads - I did these new patches without waiting for
the final conclusion. I should try to be more patient in the future...)

> > > I think we should use parent device's node, not the paren node in DT,        
> > > right? But I agree, we should only look "one level up in the chain".         
                                                                                 
> > Are these two things different? I'm suggesting looking at                      
> > device_node::parent and trying to find a #clock-cells property.                
                                                                                 
> I thought that MFD sub-devices may completely lack the DT node but I
> will verify this tomorrow.

So yep. It appears that the DT node for MFD sub-device is left NULL.
This is quite logical as there really is no clk sub-node in MFD (pmic)
node. The option to "hack around" this would be setting the of_node to
parent node in driver code. But this feels wrong. Drivers should not
mess with the "dt node ownership" - and it also feels a bit odd when
many devices use same DT node. I think we may hit in problems when
obtaining resources or doing reference counting. Hence I think we should
keep the of_node NULL for sub-device if the sub-device does not have own
node inside the main devie node. And I think Rob was not a fan of having
own nodes for sub-devices inside the MFD node (AFAIR my first driver
draft for this device had it but Rob and you thought that was not correct).

On Tue, Dec 04, 2018 at 11:38:17AM -0800, Stephen Boyd wrote:
> Quoting Matti Vaittinen (2018-12-04 03:34:53)
> > It seems to be usual for MFD devices that the created 'clock sub-device'
> > do not have own DT node. The clock provider information is usually in the
> > main device node which is owned by the MFD device. Change the devm variant
> > of clk of-provider registration to check the parent device node if given
> > device has no own node or if the node does not contain the #clock-cells
> > property. In such case use the parent node if it contains the #clock-cells.
> > 
> > Signed-off-by: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>
> > ---
> >  drivers/clk/clk.c | 27 +++++++++++++++++++++++----
> >  1 file changed, 23 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 78c90913f987..66dc7c1483d7 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -3893,6 +3893,11 @@ static void devm_of_clk_release_provider(struct device *dev, void *res)
> >         of_clk_del_provider(*(struct device_node **)res);
> >  }
> >  
> > +static int of_is_clk_provider(struct device_node *np)
> > +{
> > +       return !!of_find_property(np, "#clock-cells", NULL);
> > +}
> > +
> >  /**
> >   * devm_of_clk_add_hw_provider() - Managed clk provider node registration
> >   * @dev: Device acting as the clock provider. Used for DT node and lifetime.
> > @@ -3901,8 +3906,11 @@ static void devm_of_clk_release_provider(struct device *dev, void *res)
> >   *
> >   * Returns 0 on success or an errno on failure.
> >   *
> > - * Registers clock provider for given device's node. Provider is automatically
> > - * released at device exit.
> > + * Registers clock provider for given device's node. If the device has no DT
> > + * node or if the device node lacks of clock provider information (#clock-cells)
> > + * then the parent device's node is scanned for this information. If parent node
> > + * has the #clock-cells then it is used in registration. Provider is
> > + * automatically released at device exit.
> >   */
> >  int devm_of_clk_add_hw_provider(struct device *dev,
> >                         struct clk_hw *(*get)(struct of_phandle_args *clkspec,
> > @@ -3912,12 +3920,17 @@ int devm_of_clk_add_hw_provider(struct device *dev,
> >         struct device_node **ptr, *np;
> >         int ret;
> >  
> > +       np = dev->of_node;
> > +
> > +       if (!of_is_clk_provider(dev->of_node))
> > +               if (of_is_clk_provider(dev->parent->of_node))
> > +                       np = dev->parent->of_node;
> 
> As said on v5, let's just modify of_clk_add_provider() to do the parent
> search.

But that won't solve the issue if we don't do "dirty hacks" in driver.
The devm interface still only gets the device-pointer, not the DT node
as argument. And if DT node for device is NULL (like in MFD cases) -
then there is no parent node, only parent device with a node. For plain
of_clk_add_provider() the driver can just give the parent's node pointer
in cases where it knows it is the parent who has the provider data in
DT. But our original problem is in devm interfaces.

Br,
	Matti Vaittinen

-- 
Matti Vaittinen
ROHM Semiconductors

~~~ "I don't think so," said Rene Descartes.  Just then, he vanished ~~~

Powered by blists - more mailing lists