Re: [GIT PULL] arm64: dts: mediatek: changes for v5.13 (second round)

Re: [GIT PULL] arm64: dts: mediatek: changes for v5.13 (second round)

[Arnd Bergmann]

On Wed, Apr 7, 2021 at 1:00 PM Matthias Brugger <matthias.bgg@gmail.com> wrote:
>
> Hi Olof and Arnd,
>
> Here comes the second round of arm64 DT patches. Hope I'm not too late.
> Basically we add several node to MT8167.
> ----------------------------------------------------------------
> Fabien Parent (6):

>       arm64: dts: mediatek: mt8167: add some DRM nodes

I stumbled over this patch adding a lot of aliases:

+       aliases {
+               aal0 = &aal;
+               ccorr0 = &ccorr;
+               color0 = &color;
+               dither0 = &dither;
+               dsi0 = &dsi;
+               ovl0 = &ovl0;
+               pwm0 = &disp_pwm;
+               rdma0 = &rdma0;
+               rdma1 = &rdma1;
+               wdma0 = &wdma;
+       };


Something doesn't quite feel right about this, and I checked with Rob,
who also thinks this looks wrong. I suppose we need to have a set of
well documented alias types rather than just having drivers make up
new ones on the spot.

I also noticed that some of the referenced nodes are missing a DT
binding file, so those need to be added and reviewed as well.

At this point, I'd prefer to drop the entire branch for v5.13 and have
you work it out for the next release.

       Arnd

Re: [GIT PULL] arm64: dts: mediatek: changes for v5.13 (second round)

[Matthias Brugger]

Hi Arnd,

On 07/04/2021 18:06, Arnd Bergmann wrote:
> On Wed, Apr 7, 2021 at 1:00 PM Matthias Brugger <matthias.bgg@gmail.com> wrote:
>>
>> Hi Olof and Arnd,
>>
>> Here comes the second round of arm64 DT patches. Hope I'm not too late.
>> Basically we add several node to MT8167.
>> ----------------------------------------------------------------
>> Fabien Parent (6):
> 
>>       arm64: dts: mediatek: mt8167: add some DRM nodes
> 
> I stumbled over this patch adding a lot of aliases:
> 
> +       aliases {
> +               aal0 = &aal;
> +               ccorr0 = &ccorr;
> +               color0 = &color;
> +               dither0 = &dither;
> +               dsi0 = &dsi;
> +               ovl0 = &ovl0;
> +               pwm0 = &disp_pwm;
> +               rdma0 = &rdma0;
> +               rdma1 = &rdma1;
> +               wdma0 = &wdma;
> +       };
> 
> 
> Something doesn't quite feel right about this, and I checked with Rob,
> who also thinks this looks wrong. I suppose we need to have a set of
> well documented alias types rather than just having drivers make up
> new ones on the spot.

These are needed in the DRM driver, see drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c

I added Chun-Kuang who is the maintainer of the driver. I think it's a good idea
to have this alias described in the binding. Maybe a good excuse to move to yaml
as well :)

> 
> I also noticed that some of the referenced nodes are missing a DT
> binding file, so those need to be added and reviewed as well.

I suppose you are talking about "mediatek,mt8173-vcodec-enc-vp8". The binding
patches are in the media tree [1]. Maybe I supposed wrongly that they will land
in v5.13? Or perhaps I should have mentioned that in the pull request.

If it wasn't about this compatible and you can still remember, please let me
know so that we can fix that :)

I double checked and didn't find any missing binding. Some of them only have the
fallback binding described, maybe that's what you are referring to.

> 
> At this point, I'd prefer to drop the entire branch for v5.13 and have
> you work it out for the next release.
> 

Fair enough, pull request was quite late. I'll queue them for the next round then.

Thanks for having a look,
Matthias

[1]
https://git.linuxtv.org/media_tree.git/commit/?id=dd0008beef0dda915a255691e8b3b0527efaf1d8

Re: [GIT PULL] arm64: dts: mediatek: changes for v5.13 (second round)

[Arnd Bergmann]

On Wed, Apr 7, 2021 at 6:34 PM Matthias Brugger <matthias.bgg@gmail.com> wrote:
> On 07/04/2021 18:06, Arnd Bergmann wrote:
> > On Wed, Apr 7, 2021 at 1:00 PM Matthias Brugger <matthias.bgg@gmail.com> wrote:
> >>
> >> Hi Olof and Arnd,
> >>
> >> Here comes the second round of arm64 DT patches. Hope I'm not too late.
> >> Basically we add several node to MT8167.
> >> ----------------------------------------------------------------
> >> Fabien Parent (6):
> >
> >>       arm64: dts: mediatek: mt8167: add some DRM nodes
> >
> > I stumbled over this patch adding a lot of aliases:
> >
> > +       aliases {
> > +               aal0 = &aal;
> > +               ccorr0 = &ccorr;
> > +               color0 = &color;
> > +               dither0 = &dither;
> > +               dsi0 = &dsi;
> > +               ovl0 = &ovl0;
> > +               pwm0 = &disp_pwm;
> > +               rdma0 = &rdma0;
> > +               rdma1 = &rdma1;
> > +               wdma0 = &wdma;
> > +       };
> >
> >
> > Something doesn't quite feel right about this, and I checked with Rob,
> > who also thinks this looks wrong. I suppose we need to have a set of
> > well documented alias types rather than just having drivers make up
> > new ones on the spot.
>
> These are needed in the DRM driver, see drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
>
> I added Chun-Kuang who is the maintainer of the driver. I think it's a good idea
> to have this alias described in the binding. Maybe a good excuse to move to yaml
> as well :)

The aliases certainly need to be described in the binding, I think someone
would likely have complained earlier if that was part of the binding.

Moving it to yaml is also a good idea, and required for any new devices.

> > I also noticed that some of the referenced nodes are missing a DT
> > binding file, so those need to be added and reviewed as well.
>
> I suppose you are talking about "mediatek,mt8173-vcodec-enc-vp8". The binding
> patches are in the media tree [1]. Maybe I supposed wrongly that they will land
> in v5.13? Or perhaps I should have mentioned that in the pull request.
>
> If it wasn't about this compatible and you can still remember, please let me
> know so that we can fix that :)
>
> I double checked and didn't find any missing binding. Some of them only have the
> fallback binding described, maybe that's what you are referring to.

Here is what I see for all compatible strings of the added device nodes in
this patch, as of linux-next-20210407:

$ for i in mediatek,mt8167-disp-mutex mediatek,mt8167-disp-rdma
mediatek,mt2701-disp-rdma mediatek,mt8167-disp-pwm
mediatek,mt8173-disp-pwn mediatek,mt8167-dsi mediatek,mt2701-dsi
mediatek,mt8167-mipi-tx mediatek,mt2701-mipi-tx
mediatek,mt8167-disp-ovl mediatek,mt8173-disp-ovl
mediatek,mt8167-disp-rdma mediatek,mt2701-disp-rdma
mediatek,mt8167-disp-color mediatek,mt8173-disp-color
mediatek,mt8167-disp-ccorr mediatek,mt8183-disp-ccorr
mediatek,mt8167-disp-aal mediatek,mt8167-disp-gamma
mediatek,mt8173-disp-gamma mediatek,mt8167-disp-dither
mediatek,mt8167-disp-wdma ; do echo === $i ;  git grep -wl $i
Documentation/devicetree/ ; done

=== mediatek,mt8167-disp-mutex
=== mediatek,mt8167-disp-rdma
=== mediatek,mt2701-disp-rdma
=== mediatek,mt8167-disp-pwm
Documentation/devicetree/bindings/pwm/pwm-mtk-disp.txt
=== mediatek,mt8173-disp-pwn
=== mediatek,mt8167-dsi
=== mediatek,mt2701-dsi
=== mediatek,mt8167-mipi-tx
=== mediatek,mt2701-mipi-tx
Documentation/devicetree/bindings/phy/mediatek,dsi-phy.yaml
=== mediatek,mt8167-disp-ovl
=== mediatek,mt8173-disp-ovl
Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
=== mediatek,mt8167-disp-rdma
=== mediatek,mt2701-disp-rdma
=== mediatek,mt8167-disp-color
=== mediatek,mt8173-disp-color
Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
=== mediatek,mt8167-disp-ccorr
=== mediatek,mt8183-disp-ccorr
=== mediatek,mt8167-disp-aal
=== mediatek,mt8167-disp-gamma
=== mediatek,mt8173-disp-gamma
Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
=== mediatek,mt8167-disp-dither
=== mediatek,mt8167-disp-wdma

So five of the strings are documented, the others are missing. I did not check
the other patches in your branch.

         Arnd

Re: [GIT PULL] arm64: dts: mediatek: changes for v5.13 (second round)

[Fabien Parent]

Hi Arnd,

On Wed, Apr 7, 2021 at 7:55 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Wed, Apr 7, 2021 at 6:34 PM Matthias Brugger <matthias.bgg@gmail.com> wrote:
> > On 07/04/2021 18:06, Arnd Bergmann wrote:
> > > On Wed, Apr 7, 2021 at 1:00 PM Matthias Brugger <matthias.bgg@gmail.com> wrote:
> > >>
> > >> Hi Olof and Arnd,
> > >>
> > >> Here comes the second round of arm64 DT patches. Hope I'm not too late.
> > >> Basically we add several node to MT8167.
> > >> ----------------------------------------------------------------
> > >> Fabien Parent (6):
> > >
> > >>       arm64: dts: mediatek: mt8167: add some DRM nodes
> > >
> > > I stumbled over this patch adding a lot of aliases:
> > >
> > > +       aliases {
> > > +               aal0 = &aal;
> > > +               ccorr0 = &ccorr;
> > > +               color0 = &color;
> > > +               dither0 = &dither;
> > > +               dsi0 = &dsi;
> > > +               ovl0 = &ovl0;
> > > +               pwm0 = &disp_pwm;
> > > +               rdma0 = &rdma0;
> > > +               rdma1 = &rdma1;
> > > +               wdma0 = &wdma;
> > > +       };
> > >
> > >
> > > Something doesn't quite feel right about this, and I checked with Rob,
> > > who also thinks this looks wrong. I suppose we need to have a set of
> > > well documented alias types rather than just having drivers make up
> > > new ones on the spot.
> >
> > These are needed in the DRM driver, see drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> >
> > I added Chun-Kuang who is the maintainer of the driver. I think it's a good idea
> > to have this alias described in the binding. Maybe a good excuse to move to yaml
> > as well :)
>
> The aliases certainly need to be described in the binding, I think someone
> would likely have complained earlier if that was part of the binding.
>
> Moving it to yaml is also a good idea, and required for any new devices.
>
> > > I also noticed that some of the referenced nodes are missing a DT
> > > binding file, so those need to be added and reviewed as well.
> >
> > I suppose you are talking about "mediatek,mt8173-vcodec-enc-vp8". The binding
> > patches are in the media tree [1]. Maybe I supposed wrongly that they will land
> > in v5.13? Or perhaps I should have mentioned that in the pull request.
> >
> > If it wasn't about this compatible and you can still remember, please let me
> > know so that we can fix that :)
> >
> > I double checked and didn't find any missing binding. Some of them only have the
> > fallback binding described, maybe that's what you are referring to.
>
> Here is what I see for all compatible strings of the added device nodes in
> this patch, as of linux-next-20210407:
>
> $ for i in mediatek,mt8167-disp-mutex mediatek,mt8167-disp-rdma
> mediatek,mt2701-disp-rdma mediatek,mt8167-disp-pwm
> mediatek,mt8173-disp-pwn mediatek,mt8167-dsi mediatek,mt2701-dsi
> mediatek,mt8167-mipi-tx mediatek,mt2701-mipi-tx
> mediatek,mt8167-disp-ovl mediatek,mt8173-disp-ovl
> mediatek,mt8167-disp-rdma mediatek,mt2701-disp-rdma
> mediatek,mt8167-disp-color mediatek,mt8173-disp-color
> mediatek,mt8167-disp-ccorr mediatek,mt8183-disp-ccorr
> mediatek,mt8167-disp-aal mediatek,mt8167-disp-gamma
> mediatek,mt8173-disp-gamma mediatek,mt8167-disp-dither
> mediatek,mt8167-disp-wdma ; do echo === $i ;  git grep -wl $i
> Documentation/devicetree/ ; done
>
> === mediatek,mt8167-disp-mutex
> === mediatek,mt8167-disp-rdma
> === mediatek,mt2701-disp-rdma
> === mediatek,mt8167-disp-pwm
> Documentation/devicetree/bindings/pwm/pwm-mtk-disp.txt
> === mediatek,mt8173-disp-pwn
> === mediatek,mt8167-dsi
> === mediatek,mt2701-dsi
> === mediatek,mt8167-mipi-tx
> === mediatek,mt2701-mipi-tx
> Documentation/devicetree/bindings/phy/mediatek,dsi-phy.yaml
> === mediatek,mt8167-disp-ovl
> === mediatek,mt8173-disp-ovl
> Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> === mediatek,mt8167-disp-rdma
> === mediatek,mt2701-disp-rdma
> === mediatek,mt8167-disp-color
> === mediatek,mt8173-disp-color
> Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> === mediatek,mt8167-disp-ccorr
> === mediatek,mt8183-disp-ccorr
> === mediatek,mt8167-disp-aal
> === mediatek,mt8167-disp-gamma
> === mediatek,mt8173-disp-gamma
> Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> === mediatek,mt8167-disp-dither
> === mediatek,mt8167-disp-wdma
>
> So five of the strings are documented, the others are missing. I did not check
> the other patches in your branch.

The binding documentation for these drivers are here [0]. The display
bindings are documented as:
- compatible: "mediatek,<chip>-disp-<function>", one of

The <chip> placeholder is never expanded for all the supported chips.
The 5 existings matches from your grep command comes from the example.
I guess these will be fixed whenever someone converts [0] to yaml.

[0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt?h=v5.12-rc6#n29

>
>          Arnd

Re: [GIT PULL] arm64: dts: mediatek: changes for v5.13 (second round)

[Arnd Bergmann]

On Thu, Apr 8, 2021 at 12:14 AM Fabien Parent <fparent@baylibre.com> wrote:
> On Wed, Apr 7, 2021 at 7:55 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > On Wed, Apr 7, 2021 at 6:34 PM Matthias Brugger <matthias.bgg@gmail.com> wrote:
> > > On 07/04/2021 18:06, Arnd Bergmann wrote:
> >
> > So five of the strings are documented, the others are missing. I did not check
> > the other patches in your branch.
>
> The binding documentation for these drivers are here [0]. The display
> bindings are documented as:
> - compatible: "mediatek,<chip>-disp-<function>", one of
>
> The <chip> placeholder is never expanded for all the supported chips.
> The 5 existings matches from your grep command comes from the example.
> I guess these will be fixed whenever someone converts [0] to yaml.

Ok. I suppose the wildcards just didn't get caught in the initial review of the
binding. The way the binding is defined is not all that helpful since the entire
point of having chip specific strings is to allow having different bindings for
future chips that have different requirements.

There is still an open question on what to do to replace the aliases. At least
since they are not part of the documented binding, it is fairly easy to argue
that the drivers should not rely on them, and we can still change them.
I also see that as late as last november, there were still incompatible
code changes to the ad-hoc binding in
drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c, so I'm not too worried
about breaking existing dts files that relied on it.

I don't claim to understand how the various blocks all fit together here,
but I would expect that this can all be replaced with just having
references to phandles for the other nodes in one place.

Are the aliases in this case actually board specific, or do they just
document how the SoC is wired up?

       Arnd