Re: [PATCH] arm: dts: qcom: Sync pinctrl DT nodes with Linux bindings

[Sumit Garg <sumit.garg@linaro.org>]

On Sat, 16 Jul 2022 at 18:54, Stephan Gerhold <stephan@gerhold.net> wrote:
>
> On Fri, Jul 15, 2022 at 03:21:45PM +0530, Sumit Garg wrote:
> > On Thu, 14 Jul 2022 at 23:45, Stephan Gerhold <stephan@gerhold.net> wrote:
> > > On Thu, Jul 14, 2022 at 01:03:37PM +0530, Sumit Garg wrote:
> > > > This is based on top of mine other patch-set [1]. Although, I have
> > > > tested it on db845c and qcs404-evb but I would like other board
> > > > maintainers to test it as well.
> > > >
> > > > [1] https://patchwork.ozlabs.org/project/uboot/list/?series=309135
> > >
> > > If possible I would prefer to introduce the QCS404 platform with a clean
> > > DT (i.e. qcs404.dtsi imported from the Linux kernel, instead of adding
> > > the custom qcs404-evb.dts and then cleaning it up). This patch would
> > > need to come before the QCS404 addition then.
> > >
> >
> > Sorry but it's unfair to block new SoCs/boards support until all the
> > existing mess around DT is cleaned up in Qcom specific u-boot drivers.
> > This patch is a good example to demonstrate that all corresponding
> > boards DT need to be fixed as well which requires testing. And I don't
> > even have access to starqltechn, ipq4019 based board and db820c.
> >
>
> Sorry, I thought this is the only patch you need to use the Linux QCS404
> DT as-is (maybe I misunderstood). I'm not expecting that you clean up
> all existing boards first of course. :) I just thought it would be nice
> to start clean for QCS404 immediately if this is the only patch you need.
>
> This patch looks simple enough for me if we test it on a couple of
> boards, the pinctrl setup is fairly similar across all of them. However,
> I wrote this before the comments with the additional "reg"s below. If we
> need to add handling for that as well the patch will need to become a
> bit larger of course, maybe too large to prepend it to your QCS404
> series.

Yeah especially the test dependency makes it cumbersome to prepend it
to QCS404 series.

>
> > AFAIK, it's not a requirement yet but a recommendation at this stage
> > to import DT directly from Linux kernel and work with it. But I would
> > be very happy to work in this direction to make Qcom SoCs/boards DT
> > compliant. So I would request the merge of new boards support and then
> > we can follow up with patches like this one.
> >
>
> Thanks! I'm not familiar with the requirements so I'll leave this up to
> Tom to decide. :)
>
> > [...]
> > > > diff --git a/arch/arm/dts/qcs404-evb.dts b/arch/arm/dts/qcs404-evb.dts
> > > > index 4f0ae20bdb..09687e1fd3 100644
> > > > --- a/arch/arm/dts/qcs404-evb.dts
> > > > +++ b/arch/arm/dts/qcs404-evb.dts
> > > > @@ -38,7 +38,7 @@
> > > >               compatible = "simple-bus";
> > > >
> > > >               pinctrl_north@1300000 {
> > > > -                     compatible = "qcom,tlmm-qcs404";
> > > > +                     compatible = "qcom,qcs404-pinctrl";
> > > >                       reg = <0x1300000 0x200000>;
> > >
> > > The Linux node still looks a bit different (from qcs404.dtsi):
> > >
> > >                 tlmm: pinctrl@1000000 {
> > >                         compatible = "qcom,qcs404-pinctrl";
> > >                         reg = <0x01000000 0x200000>,
> > >                               <0x01300000 0x200000>,
> > >                               <0x07b00000 0x200000>;
> > >                         reg-names = "south", "north", "east";
> > >
> > > I guess we'll need to fetch the "north" region from it (if that's what
> > > you need)?
> >
> > This is another example of a shortcut already used by the u-boot
> > pinctrl driver (arch/arm/mach-snapdragon/pinctrl-snapdragon.c). You
> > can find another user here (arch/arm/dts/sdm845.dtsi). So the pinctrl
> > drivers need to be converted to a format supported by Linux kernel.
> > Also, the pinctrl drivers need to be moved to "drivers/pinctrl/qcom/"
> > dir instead.
> >
>
> Right. FYI, there is started work on one possible solution for this
> here: https://github.com/minlexx/u-boot-2nd/commits/660
>
> Basically Alexey (now in Cc) and Michael ported parts of the Linux
> pinctrl-msm driver to U-Boot, in a way that you can mostly just copy the
> platform specific definitions as-is. The additional memory regions are
> handled correctly there AFAICT.
>
> The code size overall is quite a bit higher of course, but I think this
> is not a problem for any of the Qualcomm boards in U-Boot. The ease of
> porting and flexibility should outweigh the cost here, I think.

I had a brief look at this WIP patch-set but in general this should be
the direction we should pursue longer term. Although I think the
platform specific definitions could be limited to what's actually
required for u-boot to function properly rather than adding redundant
code. I think in a similar manner we need to generalize Qcom clock
drivers as well (move from arch/arm/mach-snapdragon/clock-* ->
drivers/clk/qcom/).

I would be happy to review this patch-set once it's posted upstream
and would be able to port platforms which I have access to using a
generic pinctrl and clk framework.

-Sumit

>
> Thanks,
> Stephan