| Message ID | 20230118091122.2205452-1-dmitry.baryshkov@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RESEND] of: property: do not create clocks device link for clock controllers | expand |
On 18.01.2023 10:11, Dmitry Baryshkov wrote: > Do not create device link for clock controllers. 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, but with two PHYs, dispcc doesn't get probed at all, thus > breaking display support. > > Cc: Bjorn Andersson <andersson@kernel.org> > Cc: Stephen Boyd <sboyd@kernel.org> > Cc: Saravana Kannan <saravanak@google.com> > Cc: Abel Vesa <abel.vesa@linaro.org> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- Tested-by: Konrad Dybcio <konrad.dybcio@linaro.org> # SM8350 PDX215 Konrad > > This patch has been posted a year ago in January 2022 ([1]). Since that time > Saravana failed to produce patches to assist in debugging the issue > ([2]) or to fix the issue ([3]). The issue we observe has been described > by Abel at ([4]). As we work on adding support for Dual DSI > configurations, the issue becomes more and more important, since binding > the whole display subsystem fails. > > Currently the qcom/sdm845-mtp board is already broken and I've just > posted a patch adding Dual DSI variant for the qcom/sdm845-db845c board > ([5]). > > [1] https://lore.kernel.org/lkml/20211125183622.597177-1-dmitry.baryshkov@linaro.org/ > [2] https://lore.kernel.org/lkml/CAA8EJpqELXvRMPWJdTLCURjwkcMxyPDPj1tVZPkdOT_JVQb4-w@mail.gmail.com/ > [3] https://lore.kernel.org/all/CAGETcx8F0wP+RA0KpjOJeZfc=DVG-MbM_=SkRHD4UhD2ReL7Kw@mail.gmail.com/ > [4] https://lore.kernel.org/all/YrsdLQrOtg1qdaoE@linaro.org/ > [5] https://lore.kernel.org/all/20230118082048.2198715-1-dmitry.baryshkov@linaro.org/ > > --- > 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 134cfc980b70..d323bf26a613 100644 > --- a/drivers/of/property.c > +++ b/drivers/of/property.c > @@ -1297,7 +1297,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") > @@ -1327,6 +1326,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) > {
On Wed, Jan 18, 2023 at 5:35 AM Rob Herring <robh+dt@kernel.org> wrote: > > On Wed, Jan 18, 2023 at 3:11 AM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: > > > > Do not create device link for clock controllers. 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, but with two PHYs, dispcc doesn't get probed at all, thus > > breaking display support. > > > > Cc: Bjorn Andersson <andersson@kernel.org> > > Cc: Stephen Boyd <sboyd@kernel.org> > > Cc: Saravana Kannan <saravanak@google.com> > > Cc: Abel Vesa <abel.vesa@linaro.org> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > > > This patch has been posted a year ago in January 2022 ([1]). Since that time > > Saravana failed to produce patches to assist in debugging the issue > > ([2]) or to fix the issue ([3]). The issue we observe has been described > > by Abel at ([4]). As we work on adding support for Dual DSI > > configurations, the issue becomes more and more important, since binding > > the whole display subsystem fails. I did send out a patch series[1] to try and fix this. Heck I even talked about this in LPC 2022. So I don't think it's accurate to say I didn't help debug this or fix this. There's some email thread in lore where Abel gave more details and I figured out the issue and we didn't need any more debugging. And then I sent out [1]. Sorry I missed you in the cc lise for [1] -- I try to keep track of everyone to cc but things slip through the cracks sometimes. But at the same time, it's easy to check for emails from me before saying I didn't help or didn't send out fixes :) If you do try to give [1] a shot, there are a bunch of bugs that people pointed out for which I gave fixes on top of [1] in the replies. I was supposed to work on v2 over the holidays, but that didn't happen because of stuff outside my control. > That's ample time to fix this, so I intend to apply this. But I'll > give it a few days for comments. Rob, I'd recommend not applying this because it'll fix it for Dmitry but break someone else's use case. That's the whole reason it takes me a while to send out patches -- it's easy to fix it for a subset of devices, but fixing something without breaking someone else is harder (I still believe it's doable) and it takes a while to test them on all the devices I want to test before sending them out. -Saravana [1] - https://lore.kernel.org/lkml/20220810060040.321697-1-saravanak@google.com/
On Tue, Jan 24, 2023 at 06:12:15PM -0800, Saravana Kannan wrote: > On Wed, Jan 18, 2023 at 5:35 AM Rob Herring <robh+dt@kernel.org> wrote: > > > > On Wed, Jan 18, 2023 at 3:11 AM Dmitry Baryshkov > > <dmitry.baryshkov@linaro.org> wrote: > > > > > > Do not create device link for clock controllers. 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, but with two PHYs, dispcc doesn't get probed at all, thus > > > breaking display support. > > > > > > Cc: Bjorn Andersson <andersson@kernel.org> > > > Cc: Stephen Boyd <sboyd@kernel.org> > > > Cc: Saravana Kannan <saravanak@google.com> > > > Cc: Abel Vesa <abel.vesa@linaro.org> > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > --- > > > > > > This patch has been posted a year ago in January 2022 ([1]). Since that time > > > Saravana failed to produce patches to assist in debugging the issue > > > ([2]) or to fix the issue ([3]). The issue we observe has been described > > > by Abel at ([4]). As we work on adding support for Dual DSI > > > configurations, the issue becomes more and more important, since binding > > > the whole display subsystem fails. > > I did send out a patch series[1] to try and fix this. Heck I even > talked about this in LPC 2022. So I don't think it's accurate to say I > didn't help debug this or fix this. There's some email thread in lore > where Abel gave more details and I figured out the issue and we didn't > need any more debugging. And then I sent out [1]. Sorry I missed you > in the cc lise for [1] -- I try to keep track of everyone to cc but > things slip through the cracks sometimes. But at the same time, it's > easy to check for emails from me before saying I didn't help or didn't > send out fixes :) > > If you do try to give [1] a shot, there are a bunch of bugs that > people pointed out for which I gave fixes on top of [1] in the > replies. I was supposed to work on v2 over the holidays, but that > didn't happen because of stuff outside my control. > > > That's ample time to fix this, so I intend to apply this. But I'll > > give it a few days for comments. > > Rob, I'd recommend not applying this because it'll fix it for Dmitry > but break someone else's use case. That's the whole reason it takes me > a while to send out patches -- it's easy to fix it for a subset of > devices, but fixing something without breaking someone else is harder > (I still believe it's doable) and it takes a while to test them on all > the devices I want to test before sending them out. Okay, will give it a bit longer. Rob
On 25/01/2023 21:09, Rob Herring wrote: > On Tue, Jan 24, 2023 at 06:12:15PM -0800, Saravana Kannan wrote: >> On Wed, Jan 18, 2023 at 5:35 AM Rob Herring <robh+dt@kernel.org> wrote: >>> >>> On Wed, Jan 18, 2023 at 3:11 AM Dmitry Baryshkov >>> <dmitry.baryshkov@linaro.org> wrote: >>>> >>>> Do not create device link for clock controllers. 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, but with two PHYs, dispcc doesn't get probed at all, thus >>>> breaking display support. >>>> >>>> Cc: Bjorn Andersson <andersson@kernel.org> >>>> Cc: Stephen Boyd <sboyd@kernel.org> >>>> Cc: Saravana Kannan <saravanak@google.com> >>>> Cc: Abel Vesa <abel.vesa@linaro.org> >>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>> --- >>>> >>>> This patch has been posted a year ago in January 2022 ([1]). Since that time >>>> Saravana failed to produce patches to assist in debugging the issue >>>> ([2]) or to fix the issue ([3]). The issue we observe has been described >>>> by Abel at ([4]). As we work on adding support for Dual DSI >>>> configurations, the issue becomes more and more important, since binding >>>> the whole display subsystem fails. >> >> I did send out a patch series[1] to try and fix this. Heck I even >> talked about this in LPC 2022. So I don't think it's accurate to say I >> didn't help debug this or fix this. There's some email thread in lore >> where Abel gave more details and I figured out the issue and we didn't >> need any more debugging. And then I sent out [1]. Sorry I missed you >> in the cc lise for [1] -- I try to keep track of everyone to cc but >> things slip through the cracks sometimes. But at the same time, it's >> easy to check for emails from me before saying I didn't help or didn't >> send out fixes :) >> >> If you do try to give [1] a shot, there are a bunch of bugs that >> people pointed out for which I gave fixes on top of [1] in the >> replies. I was supposed to work on v2 over the holidays, but that >> didn't happen because of stuff outside my control. >> >>> That's ample time to fix this, so I intend to apply this. But I'll >>> give it a few days for comments. >> >> Rob, I'd recommend not applying this because it'll fix it for Dmitry >> but break someone else's use case. That's the whole reason it takes me >> a while to send out patches -- it's easy to fix it for a subset of >> devices, but fixing something without breaking someone else is harder >> (I still believe it's doable) and it takes a while to test them on all >> the devices I want to test before sending them out. This case is really simple, I think. Clock controllers (and clock-core-framework) are prepared to handle clock orphans properly. Moreover they have been supposed to work in such way for quite a while. In other words, I don't think we should save them from this -EPROBE_DEFERRED. Thus I think it is better to let them continue doing their job of handling probe deferrals on their own, at least for the time being. And then, when your patches are finished, we can think about reenabling current behaviour. As a reminder, currently, all Qualcomm platforms trying to use double DSI configuration are broken and have to use fw_devlink= kernel params. Stephen, do you have any comments regarding this fw_devlink usage vs CCF? > > Okay, will give it a bit longer. > > Rob
On Thu, Jan 26, 2023 at 2:51 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On 25/01/2023 21:09, Rob Herring wrote: > > On Tue, Jan 24, 2023 at 06:12:15PM -0800, Saravana Kannan wrote: > >> On Wed, Jan 18, 2023 at 5:35 AM Rob Herring <robh+dt@kernel.org> wrote: > >>> > >>> On Wed, Jan 18, 2023 at 3:11 AM Dmitry Baryshkov > >>> <dmitry.baryshkov@linaro.org> wrote: > >>>> > >>>> Do not create device link for clock controllers. 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, but with two PHYs, dispcc doesn't get probed at all, thus > >>>> breaking display support. > >>>> > >>>> Cc: Bjorn Andersson <andersson@kernel.org> > >>>> Cc: Stephen Boyd <sboyd@kernel.org> > >>>> Cc: Saravana Kannan <saravanak@google.com> > >>>> Cc: Abel Vesa <abel.vesa@linaro.org> > >>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > >>>> --- > >>>> > >>>> This patch has been posted a year ago in January 2022 ([1]). Since that time > >>>> Saravana failed to produce patches to assist in debugging the issue > >>>> ([2]) or to fix the issue ([3]). The issue we observe has been described > >>>> by Abel at ([4]). As we work on adding support for Dual DSI > >>>> configurations, the issue becomes more and more important, since binding > >>>> the whole display subsystem fails. > >> > >> I did send out a patch series[1] to try and fix this. Heck I even > >> talked about this in LPC 2022. So I don't think it's accurate to say I > >> didn't help debug this or fix this. There's some email thread in lore > >> where Abel gave more details and I figured out the issue and we didn't > >> need any more debugging. And then I sent out [1]. Sorry I missed you > >> in the cc lise for [1] -- I try to keep track of everyone to cc but > >> things slip through the cracks sometimes. But at the same time, it's > >> easy to check for emails from me before saying I didn't help or didn't > >> send out fixes :) > >> > >> If you do try to give [1] a shot, there are a bunch of bugs that > >> people pointed out for which I gave fixes on top of [1] in the > >> replies. I was supposed to work on v2 over the holidays, but that > >> didn't happen because of stuff outside my control. > >> > >>> That's ample time to fix this, so I intend to apply this. But I'll > >>> give it a few days for comments. > >> > >> Rob, I'd recommend not applying this because it'll fix it for Dmitry > >> but break someone else's use case. That's the whole reason it takes me > >> a while to send out patches -- it's easy to fix it for a subset of > >> devices, but fixing something without breaking someone else is harder > >> (I still believe it's doable) and it takes a while to test them on all > >> the devices I want to test before sending them out. > > This case is really simple, I think. Clock controllers (and > clock-core-framework) are prepared to handle clock orphans properly. > Moreover they have been supposed to work in such way for quite a while. > In other words, I don't think we should save them from this > -EPROBE_DEFERRED. A clock controller can depend on other clock controllers for non clock tree reasons. For example, it might need a clock ON to access its registers. So, while the CCF can handle orphans properly, that's not the only dependency. Also, fw_devlink is not just about probing either. It also has to do with proper sync_state() callbacks. Also, I already fixed the issue you are referring to while not breaking the conditions I'm referring to. So, I don't know why you are so opposed to that. See Abel's Tested-by here: https://lore.kernel.org/lkml/YvonlAwXAoXTUTZe@linaro.org/ > Thus I think it is better to let them continue doing their job of > handling probe deferrals on their own, at least for the time being. I'm pretty sure your patch will break other Qualcomm platforms because they depend on sync_state() callbacks to boot up properly when all/most of their drivers are built as modules. > And > then, when your patches are finished, we can think about reenabling > current behaviour. As a reminder, currently, all Qualcomm platforms > trying to use double DSI configuration are broken and have to use > fw_devlink= kernel params. I'm/was working on sending out the v2 when I got your email. Hold tight please. It shouldn't take too long. -Saravana
On Thu, Jan 26, 2023 at 3:12 PM Saravana Kannan <saravanak@google.com> wrote: > > On Thu, Jan 26, 2023 at 2:51 PM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: > > > > On 25/01/2023 21:09, Rob Herring wrote: > > > On Tue, Jan 24, 2023 at 06:12:15PM -0800, Saravana Kannan wrote: > > >> On Wed, Jan 18, 2023 at 5:35 AM Rob Herring <robh+dt@kernel.org> wrote: > > >>> > > >>> On Wed, Jan 18, 2023 at 3:11 AM Dmitry Baryshkov > > >>> <dmitry.baryshkov@linaro.org> wrote: > > >>>> > > >>>> Do not create device link for clock controllers. 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, but with two PHYs, dispcc doesn't get probed at all, thus > > >>>> breaking display support. > > >>>> > > >>>> Cc: Bjorn Andersson <andersson@kernel.org> > > >>>> Cc: Stephen Boyd <sboyd@kernel.org> > > >>>> Cc: Saravana Kannan <saravanak@google.com> > > >>>> Cc: Abel Vesa <abel.vesa@linaro.org> > > >>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > >>>> --- > > >>>> > > >>>> This patch has been posted a year ago in January 2022 ([1]). Since that time > > >>>> Saravana failed to produce patches to assist in debugging the issue > > >>>> ([2]) or to fix the issue ([3]). The issue we observe has been described > > >>>> by Abel at ([4]). As we work on adding support for Dual DSI > > >>>> configurations, the issue becomes more and more important, since binding > > >>>> the whole display subsystem fails. > > >> > > >> I did send out a patch series[1] to try and fix this. Heck I even > > >> talked about this in LPC 2022. So I don't think it's accurate to say I > > >> didn't help debug this or fix this. There's some email thread in lore > > >> where Abel gave more details and I figured out the issue and we didn't > > >> need any more debugging. And then I sent out [1]. Sorry I missed you > > >> in the cc lise for [1] -- I try to keep track of everyone to cc but > > >> things slip through the cracks sometimes. But at the same time, it's > > >> easy to check for emails from me before saying I didn't help or didn't > > >> send out fixes :) > > >> > > >> If you do try to give [1] a shot, there are a bunch of bugs that > > >> people pointed out for which I gave fixes on top of [1] in the > > >> replies. I was supposed to work on v2 over the holidays, but that > > >> didn't happen because of stuff outside my control. > > >> > > >>> That's ample time to fix this, so I intend to apply this. But I'll > > >>> give it a few days for comments. > > >> > > >> Rob, I'd recommend not applying this because it'll fix it for Dmitry > > >> but break someone else's use case. That's the whole reason it takes me > > >> a while to send out patches -- it's easy to fix it for a subset of > > >> devices, but fixing something without breaking someone else is harder > > >> (I still believe it's doable) and it takes a while to test them on all > > >> the devices I want to test before sending them out. > > > > This case is really simple, I think. Clock controllers (and > > clock-core-framework) are prepared to handle clock orphans properly. > > Moreover they have been supposed to work in such way for quite a while. > > In other words, I don't think we should save them from this > > -EPROBE_DEFERRED. > > A clock controller can depend on other clock controllers for non clock > tree reasons. For example, it might need a clock ON to access its > registers. So, while the CCF can handle orphans properly, that's not > the only dependency. Also, fw_devlink is not just about probing > either. It also has to do with proper sync_state() callbacks. > > Also, I already fixed the issue you are referring to while not > breaking the conditions I'm referring to. So, I don't know why you are > so opposed to that. See Abel's Tested-by here: > https://lore.kernel.org/lkml/YvonlAwXAoXTUTZe@linaro.org/ > > > Thus I think it is better to let them continue doing their job of > > handling probe deferrals on their own, at least for the time being. > > I'm pretty sure your patch will break other Qualcomm platforms because > they depend on sync_state() callbacks to boot up properly when > all/most of their drivers are built as modules. > > > And > > then, when your patches are finished, we can think about reenabling > > current behaviour. As a reminder, currently, all Qualcomm platforms > > trying to use double DSI configuration are broken and have to use > > fw_devlink= kernel params. > > I'm/was working on sending out the v2 when I got your email. Hold > tight please. It shouldn't take too long. There! https://lore.kernel.org/lkml/20230127001141.407071-1-saravanak@google.com/ Happy? :) -Saravana
On Fri, 27 Jan 2023 at 01:12, Saravana Kannan <saravanak@google.com> wrote: > > On Thu, Jan 26, 2023 at 2:51 PM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: > > > > On 25/01/2023 21:09, Rob Herring wrote: > > > On Tue, Jan 24, 2023 at 06:12:15PM -0800, Saravana Kannan wrote: > > >> On Wed, Jan 18, 2023 at 5:35 AM Rob Herring <robh+dt@kernel.org> wrote: > > >>> > > >>> On Wed, Jan 18, 2023 at 3:11 AM Dmitry Baryshkov > > >>> <dmitry.baryshkov@linaro.org> wrote: > > >>>> > > >>>> Do not create device link for clock controllers. 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, but with two PHYs, dispcc doesn't get probed at all, thus > > >>>> breaking display support. > > >>>> > > >>>> Cc: Bjorn Andersson <andersson@kernel.org> > > >>>> Cc: Stephen Boyd <sboyd@kernel.org> > > >>>> Cc: Saravana Kannan <saravanak@google.com> > > >>>> Cc: Abel Vesa <abel.vesa@linaro.org> > > >>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > >>>> --- > > >>>> > > >>>> This patch has been posted a year ago in January 2022 ([1]). Since that time > > >>>> Saravana failed to produce patches to assist in debugging the issue > > >>>> ([2]) or to fix the issue ([3]). The issue we observe has been described > > >>>> by Abel at ([4]). As we work on adding support for Dual DSI > > >>>> configurations, the issue becomes more and more important, since binding > > >>>> the whole display subsystem fails. > > >> > > >> I did send out a patch series[1] to try and fix this. Heck I even > > >> talked about this in LPC 2022. So I don't think it's accurate to say I > > >> didn't help debug this or fix this. There's some email thread in lore > > >> where Abel gave more details and I figured out the issue and we didn't > > >> need any more debugging. And then I sent out [1]. Sorry I missed you > > >> in the cc lise for [1] -- I try to keep track of everyone to cc but > > >> things slip through the cracks sometimes. But at the same time, it's > > >> easy to check for emails from me before saying I didn't help or didn't > > >> send out fixes :) > > >> > > >> If you do try to give [1] a shot, there are a bunch of bugs that > > >> people pointed out for which I gave fixes on top of [1] in the > > >> replies. I was supposed to work on v2 over the holidays, but that > > >> didn't happen because of stuff outside my control. > > >> > > >>> That's ample time to fix this, so I intend to apply this. But I'll > > >>> give it a few days for comments. > > >> > > >> Rob, I'd recommend not applying this because it'll fix it for Dmitry > > >> but break someone else's use case. That's the whole reason it takes me > > >> a while to send out patches -- it's easy to fix it for a subset of > > >> devices, but fixing something without breaking someone else is harder > > >> (I still believe it's doable) and it takes a while to test them on all > > >> the devices I want to test before sending them out. > > > > This case is really simple, I think. Clock controllers (and > > clock-core-framework) are prepared to handle clock orphans properly. > > Moreover they have been supposed to work in such way for quite a while. > > In other words, I don't think we should save them from this > > -EPROBE_DEFERRED. > > A clock controller can depend on other clock controllers for non clock > tree reasons. For example, it might need a clock ON to access its > registers. So, while the CCF can handle orphans properly, that's not > the only dependency. Also, fw_devlink is not just about probing > either. It also has to do with proper sync_state() callbacks. Just a question, please excuse if I'm misunderstanding it. Does fw_devlink created this way also impose any runtime PM dependencies? > > Also, I already fixed the issue you are referring to while not > breaking the conditions I'm referring to. So, I don't know why you are > so opposed to that. See Abel's Tested-by here: > https://lore.kernel.org/lkml/YvonlAwXAoXTUTZe@linaro.org/ > > > Thus I think it is better to let them continue doing their job of > > handling probe deferrals on their own, at least for the time being. > > I'm pretty sure your patch will break other Qualcomm platforms because > they depend on sync_state() callbacks to boot up properly when > all/most of their drivers are built as modules. Qualcomm platforms did not use sync state for clock controllers. Only for the icc drivers. > > > And > > then, when your patches are finished, we can think about reenabling > > current behaviour. As a reminder, currently, all Qualcomm platforms > > trying to use double DSI configuration are broken and have to use > > fw_devlink= kernel params. > > I'm/was working on sending out the v2 when I got your email. Hold > tight please. It shouldn't take too long. I'll give v2 a test next week, thank you!
On Fri, Jan 27, 2023 at 8:41 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Fri, 27 Jan 2023 at 01:12, Saravana Kannan <saravanak@google.com> wrote: > > > > On Thu, Jan 26, 2023 at 2:51 PM Dmitry Baryshkov > > <dmitry.baryshkov@linaro.org> wrote: > > > > > > On 25/01/2023 21:09, Rob Herring wrote: > > > > On Tue, Jan 24, 2023 at 06:12:15PM -0800, Saravana Kannan wrote: > > > >> On Wed, Jan 18, 2023 at 5:35 AM Rob Herring <robh+dt@kernel.org> wrote: > > > >>> > > > >>> On Wed, Jan 18, 2023 at 3:11 AM Dmitry Baryshkov > > > >>> <dmitry.baryshkov@linaro.org> wrote: > > > >>>> > > > >>>> Do not create device link for clock controllers. 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, but with two PHYs, dispcc doesn't get probed at all, thus > > > >>>> breaking display support. > > > >>>> > > > >>>> Cc: Bjorn Andersson <andersson@kernel.org> > > > >>>> Cc: Stephen Boyd <sboyd@kernel.org> > > > >>>> Cc: Saravana Kannan <saravanak@google.com> > > > >>>> Cc: Abel Vesa <abel.vesa@linaro.org> > > > >>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > >>>> --- > > > >>>> > > > >>>> This patch has been posted a year ago in January 2022 ([1]). Since that time > > > >>>> Saravana failed to produce patches to assist in debugging the issue > > > >>>> ([2]) or to fix the issue ([3]). The issue we observe has been described > > > >>>> by Abel at ([4]). As we work on adding support for Dual DSI > > > >>>> configurations, the issue becomes more and more important, since binding > > > >>>> the whole display subsystem fails. > > > >> > > > >> I did send out a patch series[1] to try and fix this. Heck I even > > > >> talked about this in LPC 2022. So I don't think it's accurate to say I > > > >> didn't help debug this or fix this. There's some email thread in lore > > > >> where Abel gave more details and I figured out the issue and we didn't > > > >> need any more debugging. And then I sent out [1]. Sorry I missed you > > > >> in the cc lise for [1] -- I try to keep track of everyone to cc but > > > >> things slip through the cracks sometimes. But at the same time, it's > > > >> easy to check for emails from me before saying I didn't help or didn't > > > >> send out fixes :) > > > >> > > > >> If you do try to give [1] a shot, there are a bunch of bugs that > > > >> people pointed out for which I gave fixes on top of [1] in the > > > >> replies. I was supposed to work on v2 over the holidays, but that > > > >> didn't happen because of stuff outside my control. > > > >> > > > >>> That's ample time to fix this, so I intend to apply this. But I'll > > > >>> give it a few days for comments. > > > >> > > > >> Rob, I'd recommend not applying this because it'll fix it for Dmitry > > > >> but break someone else's use case. That's the whole reason it takes me > > > >> a while to send out patches -- it's easy to fix it for a subset of > > > >> devices, but fixing something without breaking someone else is harder > > > >> (I still believe it's doable) and it takes a while to test them on all > > > >> the devices I want to test before sending them out. > > > > > > This case is really simple, I think. Clock controllers (and > > > clock-core-framework) are prepared to handle clock orphans properly. > > > Moreover they have been supposed to work in such way for quite a while. > > > In other words, I don't think we should save them from this > > > -EPROBE_DEFERRED. > > > > A clock controller can depend on other clock controllers for non clock > > tree reasons. For example, it might need a clock ON to access its > > registers. So, while the CCF can handle orphans properly, that's not > > the only dependency. Also, fw_devlink is not just about probing > > either. It also has to do with proper sync_state() callbacks. > > Just a question, please excuse if I'm misunderstanding it. Does > fw_devlink created this way also impose any runtime PM dependencies? If you set fw_devlink=rpm in the command line. The default is just "on". > > > > Also, I already fixed the issue you are referring to while not > > breaking the conditions I'm referring to. So, I don't know why you are > > so opposed to that. See Abel's Tested-by here: > > https://lore.kernel.org/lkml/YvonlAwXAoXTUTZe@linaro.org/ > > > > > Thus I think it is better to let them continue doing their job of > > > handling probe deferrals on their own, at least for the time being. > > > > I'm pretty sure your patch will break other Qualcomm platforms because > > they depend on sync_state() callbacks to boot up properly when > > all/most of their drivers are built as modules. > > Qualcomm platforms did not use sync state for clock controllers. Only > for the icc drivers. > > > > > > And > > > then, when your patches are finished, we can think about reenabling > > > current behaviour. As a reminder, currently, all Qualcomm platforms > > > trying to use double DSI configuration are broken and have to use > > > fw_devlink= kernel params. > > > > I'm/was working on sending out the v2 when I got your email. Hold > > tight please. It shouldn't take too long. > > I'll give v2 a test next week, thank you! Thanks. -Saravana
28 января 2023 г. 07:54:14 GMT+03:00, Saravana Kannan <saravanak@google.com> пишет: >On Fri, Jan 27, 2023 at 8:41 PM Dmitry Baryshkov ><dmitry.baryshkov@linaro.org> wrote: >> >> On Fri, 27 Jan 2023 at 01:12, Saravana Kannan <saravanak@google.com> wrote: >> > >> > On Thu, Jan 26, 2023 at 2:51 PM Dmitry Baryshkov >> > <dmitry.baryshkov@linaro.org> wrote: >> > > >> > > On 25/01/2023 21:09, Rob Herring wrote: >> > > > On Tue, Jan 24, 2023 at 06:12:15PM -0800, Saravana Kannan wrote: >> > > >> On Wed, Jan 18, 2023 at 5:35 AM Rob Herring <robh+dt@kernel.org> wrote: >> > > >>> >> > > >>> On Wed, Jan 18, 2023 at 3:11 AM Dmitry Baryshkov >> > > >>> <dmitry.baryshkov@linaro.org> wrote: >> > > >>>> >> > > >>>> Do not create device link for clock controllers. 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, but with two PHYs, dispcc doesn't get probed at all, thus >> > > >>>> breaking display support. >> > > >>>> >> > > >>>> Cc: Bjorn Andersson <andersson@kernel.org> >> > > >>>> Cc: Stephen Boyd <sboyd@kernel.org> >> > > >>>> Cc: Saravana Kannan <saravanak@google.com> >> > > >>>> Cc: Abel Vesa <abel.vesa@linaro.org> >> > > >>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> > > >>>> --- >> > > >>>> >> > > >>>> This patch has been posted a year ago in January 2022 ([1]). Since that time >> > > >>>> Saravana failed to produce patches to assist in debugging the issue >> > > >>>> ([2]) or to fix the issue ([3]). The issue we observe has been described >> > > >>>> by Abel at ([4]). As we work on adding support for Dual DSI >> > > >>>> configurations, the issue becomes more and more important, since binding >> > > >>>> the whole display subsystem fails. >> > > >> >> > > >> I did send out a patch series[1] to try and fix this. Heck I even >> > > >> talked about this in LPC 2022. So I don't think it's accurate to say I >> > > >> didn't help debug this or fix this. There's some email thread in lore >> > > >> where Abel gave more details and I figured out the issue and we didn't >> > > >> need any more debugging. And then I sent out [1]. Sorry I missed you >> > > >> in the cc lise for [1] -- I try to keep track of everyone to cc but >> > > >> things slip through the cracks sometimes. But at the same time, it's >> > > >> easy to check for emails from me before saying I didn't help or didn't >> > > >> send out fixes :) >> > > >> >> > > >> If you do try to give [1] a shot, there are a bunch of bugs that >> > > >> people pointed out for which I gave fixes on top of [1] in the >> > > >> replies. I was supposed to work on v2 over the holidays, but that >> > > >> didn't happen because of stuff outside my control. >> > > >> >> > > >>> That's ample time to fix this, so I intend to apply this. But I'll >> > > >>> give it a few days for comments. >> > > >> >> > > >> Rob, I'd recommend not applying this because it'll fix it for Dmitry >> > > >> but break someone else's use case. That's the whole reason it takes me >> > > >> a while to send out patches -- it's easy to fix it for a subset of >> > > >> devices, but fixing something without breaking someone else is harder >> > > >> (I still believe it's doable) and it takes a while to test them on all >> > > >> the devices I want to test before sending them out. >> > > >> > > This case is really simple, I think. Clock controllers (and >> > > clock-core-framework) are prepared to handle clock orphans properly. >> > > Moreover they have been supposed to work in such way for quite a while. >> > > In other words, I don't think we should save them from this >> > > -EPROBE_DEFERRED. >> > >> > A clock controller can depend on other clock controllers for non clock >> > tree reasons. For example, it might need a clock ON to access its >> > registers. So, while the CCF can handle orphans properly, that's not >> > the only dependency. Also, fw_devlink is not just about probing >> > either. It also has to do with proper sync_state() callbacks. >> >> Just a question, please excuse if I'm misunderstanding it. Does >> fw_devlink created this way also impose any runtime PM dependencies? > >If you set fw_devlink=rpm in the command line. The default is just "on". So you plan to switch to rpm at some point?
On Fri, Jan 27, 2023 at 9:34 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > 28 января 2023 г. 07:54:14 GMT+03:00, Saravana Kannan <saravanak@google.com> пишет: > >On Fri, Jan 27, 2023 at 8:41 PM Dmitry Baryshkov > ><dmitry.baryshkov@linaro.org> wrote: > >> > >> On Fri, 27 Jan 2023 at 01:12, Saravana Kannan <saravanak@google.com> wrote: > >> > > >> > On Thu, Jan 26, 2023 at 2:51 PM Dmitry Baryshkov > >> > <dmitry.baryshkov@linaro.org> wrote: > >> > > > >> > > On 25/01/2023 21:09, Rob Herring wrote: > >> > > > On Tue, Jan 24, 2023 at 06:12:15PM -0800, Saravana Kannan wrote: > >> > > >> On Wed, Jan 18, 2023 at 5:35 AM Rob Herring <robh+dt@kernel.org> wrote: > >> > > >>> > >> > > >>> On Wed, Jan 18, 2023 at 3:11 AM Dmitry Baryshkov > >> > > >>> <dmitry.baryshkov@linaro.org> wrote: > >> > > >>>> > >> > > >>>> Do not create device link for clock controllers. 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, but with two PHYs, dispcc doesn't get probed at all, thus > >> > > >>>> breaking display support. > >> > > >>>> > >> > > >>>> Cc: Bjorn Andersson <andersson@kernel.org> > >> > > >>>> Cc: Stephen Boyd <sboyd@kernel.org> > >> > > >>>> Cc: Saravana Kannan <saravanak@google.com> > >> > > >>>> Cc: Abel Vesa <abel.vesa@linaro.org> > >> > > >>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > >> > > >>>> --- > >> > > >>>> > >> > > >>>> This patch has been posted a year ago in January 2022 ([1]). Since that time > >> > > >>>> Saravana failed to produce patches to assist in debugging the issue > >> > > >>>> ([2]) or to fix the issue ([3]). The issue we observe has been described > >> > > >>>> by Abel at ([4]). As we work on adding support for Dual DSI > >> > > >>>> configurations, the issue becomes more and more important, since binding > >> > > >>>> the whole display subsystem fails. > >> > > >> > >> > > >> I did send out a patch series[1] to try and fix this. Heck I even > >> > > >> talked about this in LPC 2022. So I don't think it's accurate to say I > >> > > >> didn't help debug this or fix this. There's some email thread in lore > >> > > >> where Abel gave more details and I figured out the issue and we didn't > >> > > >> need any more debugging. And then I sent out [1]. Sorry I missed you > >> > > >> in the cc lise for [1] -- I try to keep track of everyone to cc but > >> > > >> things slip through the cracks sometimes. But at the same time, it's > >> > > >> easy to check for emails from me before saying I didn't help or didn't > >> > > >> send out fixes :) > >> > > >> > >> > > >> If you do try to give [1] a shot, there are a bunch of bugs that > >> > > >> people pointed out for which I gave fixes on top of [1] in the > >> > > >> replies. I was supposed to work on v2 over the holidays, but that > >> > > >> didn't happen because of stuff outside my control. > >> > > >> > >> > > >>> That's ample time to fix this, so I intend to apply this. But I'll > >> > > >>> give it a few days for comments. > >> > > >> > >> > > >> Rob, I'd recommend not applying this because it'll fix it for Dmitry > >> > > >> but break someone else's use case. That's the whole reason it takes me > >> > > >> a while to send out patches -- it's easy to fix it for a subset of > >> > > >> devices, but fixing something without breaking someone else is harder > >> > > >> (I still believe it's doable) and it takes a while to test them on all > >> > > >> the devices I want to test before sending them out. > >> > > > >> > > This case is really simple, I think. Clock controllers (and > >> > > clock-core-framework) are prepared to handle clock orphans properly. > >> > > Moreover they have been supposed to work in such way for quite a while. > >> > > In other words, I don't think we should save them from this > >> > > -EPROBE_DEFERRED. > >> > > >> > A clock controller can depend on other clock controllers for non clock > >> > tree reasons. For example, it might need a clock ON to access its > >> > registers. So, while the CCF can handle orphans properly, that's not > >> > the only dependency. Also, fw_devlink is not just about probing > >> > either. It also has to do with proper sync_state() callbacks. > >> > >> Just a question, please excuse if I'm misunderstanding it. Does > >> fw_devlink created this way also impose any runtime PM dependencies? > > > >If you set fw_devlink=rpm in the command line. The default is just "on". > > So you plan to switch to rpm at some point? Ideally, but it's a loooong way off. I need to fix all the issues people are pointing out right now before I try to go for that being the default. -Saravana
On Fri, Jan 27, 2023 at 8:41 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Fri, 27 Jan 2023 at 01:12, Saravana Kannan <saravanak@google.com> wrote: > > > > On Thu, Jan 26, 2023 at 2:51 PM Dmitry Baryshkov > > <dmitry.baryshkov@linaro.org> wrote: > > > > > > On 25/01/2023 21:09, Rob Herring wrote: > > > > On Tue, Jan 24, 2023 at 06:12:15PM -0800, Saravana Kannan wrote: > > > >> On Wed, Jan 18, 2023 at 5:35 AM Rob Herring <robh+dt@kernel.org> wrote: > > > >>> > > > >>> On Wed, Jan 18, 2023 at 3:11 AM Dmitry Baryshkov > > > >>> <dmitry.baryshkov@linaro.org> wrote: > > > >>>> > > > >>>> Do not create device link for clock controllers. 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, but with two PHYs, dispcc doesn't get probed at all, thus > > > >>>> breaking display support. > > > >>>> > > > >>>> Cc: Bjorn Andersson <andersson@kernel.org> > > > >>>> Cc: Stephen Boyd <sboyd@kernel.org> > > > >>>> Cc: Saravana Kannan <saravanak@google.com> > > > >>>> Cc: Abel Vesa <abel.vesa@linaro.org> > > > >>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > >>>> --- > > > >>>> > > > >>>> This patch has been posted a year ago in January 2022 ([1]). Since that time > > > >>>> Saravana failed to produce patches to assist in debugging the issue > > > >>>> ([2]) or to fix the issue ([3]). The issue we observe has been described > > > >>>> by Abel at ([4]). As we work on adding support for Dual DSI > > > >>>> configurations, the issue becomes more and more important, since binding > > > >>>> the whole display subsystem fails. > > > >> > > > >> I did send out a patch series[1] to try and fix this. Heck I even > > > >> talked about this in LPC 2022. So I don't think it's accurate to say I > > > >> didn't help debug this or fix this. There's some email thread in lore > > > >> where Abel gave more details and I figured out the issue and we didn't > > > >> need any more debugging. And then I sent out [1]. Sorry I missed you > > > >> in the cc lise for [1] -- I try to keep track of everyone to cc but > > > >> things slip through the cracks sometimes. But at the same time, it's > > > >> easy to check for emails from me before saying I didn't help or didn't > > > >> send out fixes :) > > > >> > > > >> If you do try to give [1] a shot, there are a bunch of bugs that > > > >> people pointed out for which I gave fixes on top of [1] in the > > > >> replies. I was supposed to work on v2 over the holidays, but that > > > >> didn't happen because of stuff outside my control. > > > >> > > > >>> That's ample time to fix this, so I intend to apply this. But I'll > > > >>> give it a few days for comments. > > > >> > > > >> Rob, I'd recommend not applying this because it'll fix it for Dmitry > > > >> but break someone else's use case. That's the whole reason it takes me > > > >> a while to send out patches -- it's easy to fix it for a subset of > > > >> devices, but fixing something without breaking someone else is harder > > > >> (I still believe it's doable) and it takes a while to test them on all > > > >> the devices I want to test before sending them out. > > > > > > This case is really simple, I think. Clock controllers (and > > > clock-core-framework) are prepared to handle clock orphans properly. > > > Moreover they have been supposed to work in such way for quite a while. > > > In other words, I don't think we should save them from this > > > -EPROBE_DEFERRED. > > > > A clock controller can depend on other clock controllers for non clock > > tree reasons. For example, it might need a clock ON to access its > > registers. So, while the CCF can handle orphans properly, that's not > > the only dependency. Also, fw_devlink is not just about probing > > either. It also has to do with proper sync_state() callbacks. > > Just a question, please excuse if I'm misunderstanding it. Does > fw_devlink created this way also impose any runtime PM dependencies? > > > > > Also, I already fixed the issue you are referring to while not > > breaking the conditions I'm referring to. So, I don't know why you are > > so opposed to that. See Abel's Tested-by here: > > https://lore.kernel.org/lkml/YvonlAwXAoXTUTZe@linaro.org/ > > > > > Thus I think it is better to let them continue doing their job of > > > handling probe deferrals on their own, at least for the time being. > > > > I'm pretty sure your patch will break other Qualcomm platforms because > > they depend on sync_state() callbacks to boot up properly when > > all/most of their drivers are built as modules. > > Qualcomm platforms did not use sync state for clock controllers. Only > for the icc drivers. > > > > > > And > > > then, when your patches are finished, we can think about reenabling > > > current behaviour. As a reminder, currently, all Qualcomm platforms > > > trying to use double DSI configuration are broken and have to use > > > fw_devlink= kernel params. > > > > I'm/was working on sending out the v2 when I got your email. Hold > > tight please. It shouldn't take too long. > > I'll give v2 a test next week, thank you! Nudge... I rushed out the series for you. -Saravana
diff --git a/drivers/of/property.c b/drivers/of/property.c index 134cfc980b70..d323bf26a613 100644 --- a/drivers/of/property.c +++ b/drivers/of/property.c @@ -1297,7 +1297,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") @@ -1327,6 +1326,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) {
Do not create device link for clock controllers. 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, but with two PHYs, dispcc doesn't get probed at all, thus breaking display support. Cc: Bjorn Andersson <andersson@kernel.org> Cc: Stephen Boyd <sboyd@kernel.org> Cc: Saravana Kannan <saravanak@google.com> Cc: Abel Vesa <abel.vesa@linaro.org> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- This patch has been posted a year ago in January 2022 ([1]). Since that time Saravana failed to produce patches to assist in debugging the issue ([2]) or to fix the issue ([3]). The issue we observe has been described by Abel at ([4]). As we work on adding support for Dual DSI configurations, the issue becomes more and more important, since binding the whole display subsystem fails. Currently the qcom/sdm845-mtp board is already broken and I've just posted a patch adding Dual DSI variant for the qcom/sdm845-db845c board ([5]). [1] https://lore.kernel.org/lkml/20211125183622.597177-1-dmitry.baryshkov@linaro.org/ [2] https://lore.kernel.org/lkml/CAA8EJpqELXvRMPWJdTLCURjwkcMxyPDPj1tVZPkdOT_JVQb4-w@mail.gmail.com/ [3] https://lore.kernel.org/all/CAGETcx8F0wP+RA0KpjOJeZfc=DVG-MbM_=SkRHD4UhD2ReL7Kw@mail.gmail.com/ [4] https://lore.kernel.org/all/YrsdLQrOtg1qdaoE@linaro.org/ [5] https://lore.kernel.org/all/20230118082048.2198715-1-dmitry.baryshkov@linaro.org/ --- drivers/of/property.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)