Re: [PATCH] of: property: do not create clocks device link for clock controllers

[Dmitry Baryshkov]

Hi Saravana,

On Tue, 7 Dec 2021 at 05:24, Saravana Kannan <saravanak@xxxxxxxxxx> wrote:
>
> On Mon, Dec 6, 2021 at 6:00 PM Dmitry Baryshkov
> <dmitry.baryshkov@xxxxxxxxxx> wrote:
> >
> > Hi Saravana,
> >
> > On Tue, 30 Nov 2021 at 03:24, Dmitry Baryshkov
> > <dmitry.baryshkov@xxxxxxxxxx> wrote:
> > >
> > > On Tue, 30 Nov 2021 at 02:53, Saravana Kannan <saravanak@xxxxxxxxxx> wrote:
> > > >
> > > > On Mon, Nov 29, 2021 at 3:48 PM Saravana Kannan <saravanak@xxxxxxxxxx> wrote:
> > > > >
> > > > > On Thu, Nov 25, 2021 at 10:36 AM Dmitry Baryshkov
> > > > > <dmitry.baryshkov@xxxxxxxxxx> wrote:
> > > > > >
> > > > > > Do not create device link for clock controllers.
> > > > >
> > > > > Nak.
> > > > >
> > > > > > Some of the clocks
> > > > > > provided to the device via OF can be the clocks that are just parents to
> > > > > > the clocks provided by this clock controller. Clock subsystem already
> > > > > > has support for handling missing clock parents correctly (clock
> > > > > > orphans). Later when the parent clock is registered, clocks get
> > > > > > populated properly.
> > > > > >
> > > > > > An example of the system where this matters is the SDM8450 MTP board
> > > > > > (see arch/arm64/boot/dts/qcom/sdm845-mtp.dts). Here the dispcc uses
> > > > > > clocks provided by dsi0_phy and dsi1_phy device tree nodes. However the
> > > > > > dispcc itself provides clocks to both PHYs, to the PHY parent device,
> > > > > > etc. With just dsi0_phy in place devlink is able to break the
> > > > > > dependency,
> > > > >
> > > > > Right, because I wrote code to make sure we handle these clock
> > > > > controller cases properly. If that logic isn't smart enough, let's fix
> > > > > that.
> > >
> > > As I said, devlink was delaying dispcc probing ,waiting for the second
> > > DSI PHY clock provider.
> > > Thus came my proposal to let clock orphans handle the case (which it
> > > does perfectly).
> > >
> > > > >
> > > > > > but with two PHYs, dispcc doesn't get probed at all, thus
> > > > > > breaking display support.
> > > > >
> > > > > Then let's find out why and fix this instead of hiding some
> > > > > dependencies from fw_devlink. You could be breaking other cases/boards
> > > > > with this change you are making.
> > > >
> > > > Btw, forgot to mention. I'll look into this one and try to find the
> > > > reason why it wasn't handled automatically. And then come up with a
> > > > fix.
> > > >
> > > > If you want to find out why fw_devlink didn't notice the cycle
> > > > correctly for the case of 2 PHYs vs 1 PHY, I'd appreciate that too.
> > > >
> > > > Btw, same comment for remote-endpoint. I'll look into what's going on
> > > > in that case. Btw, I'm assuming all the code and DT you are testing
> > > > this on is already upstream. Can you please confirm that?
> > >
> > > All the code and basic DT is upstreamed. The DT part I
> > > referenced/posted was written for the custom extender for the
> > > qrb5165-rb5 board that I use here to test MSM DRM driver, but the
> > > result DT should be more or less the same as smd845-mtp.
>
> Can you point me to some upstream DTS file (not dtsi) that you think
> will definitely have this issue (ideally you've actually hit it), and
> the specific DT nodes in question? That'd make it much easier for me
> to jump in and look as I'm not up to speed on all the MSM boards.

I've been under the load thanks to the NY holidays.
I have verified that ach/arm64/boot/dts/qcom/sdm845-mtp.dts exhibits
the same behaviour. dispcc being deferred forever because of the
dependencies on dsi phy.

>
> > So, is there a way we can assist you in debugging these issues? I
> > still can not get dual DSI setup to work without this patch (or
> > without disabling fw_devlink).
>
> Sorry I've been a bit swamped. I'll try to take a look at this soon.
>
> Another thing you could do is look at the existing code that detects
> these cycles and fixes them up and figure out why it's not noticing a
> cycle for your use case or not fixing the cycle correctly. You'll want
> to look at calls to fw_devlink_relax_cycle() inside
> fw_devlink_create_devlink().

Could you please post patches that can assist you in debugging this?

>
> -Saravana
>
> >
> > >
> > > >
> > > > -Saravana
> > > >
> > > > >
> > > > > -Saravana
> > > > >
> > > > > > Cc: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
> > > > > > Cc: Stephen Boyd <swboyd@xxxxxxxxxxxx>
> > > > > > Cc: Saravana Kannan <saravanak@xxxxxxxxxx>
> > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > > > > > ---
> > > > > >  drivers/of/property.c | 16 +++++++++++++++-
> > > > > >  1 file changed, 15 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > > > > > index a3483484a5a2..f7229e4030e3 100644
> > > > > > --- a/drivers/of/property.c
> > > > > > +++ b/drivers/of/property.c
> > > > > > @@ -1264,7 +1264,6 @@ struct supplier_bindings {
> > > > > >         bool node_not_dev;
> > > > > >  };
> > > > > >
> > > > > > -DEFINE_SIMPLE_PROP(clocks, "clocks", "#clock-cells")
> > > > > >  DEFINE_SIMPLE_PROP(interconnects, "interconnects", "#interconnect-cells")
> > > > > >  DEFINE_SIMPLE_PROP(iommus, "iommus", "#iommu-cells")
> > > > > >  DEFINE_SIMPLE_PROP(mboxes, "mboxes", "#mbox-cells")
> > > > > > @@ -1294,6 +1293,21 @@ DEFINE_SIMPLE_PROP(backlight, "backlight", NULL)
> > > > > >  DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
> > > > > >  DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")
> > > > > >
> > > > > > +static struct device_node *parse_clocks(struct device_node *np,
> > > > > > +                                       const char *prop_name, int index)
> > > > > > +{
> > > > > > +       /*
> > > > > > +        * Do not create clock-related device links for clocks controllers,
> > > > > > +        * clock orphans will handle missing clock parents automatically.
> > > > > > +        */
> > > > > > +       if (!strcmp(prop_name, "clocks") &&
> > > > > > +           of_find_property(np, "#clock-cells", NULL))
> > > > > > +               return NULL;
> > > > > > +
> > > > > > +       return parse_prop_cells(np, prop_name, index, "clocks",
> > > > > > +                                      "#clock-cells");
> > > > > > +}
> > > > > > +
> > > > > >  static struct device_node *parse_gpios(struct device_node *np,
> > > > > >                                        const char *prop_name, int index)
> > > > > >  {


-- 
With best wishes
Dmitry