* Re: [GIT PULL] arm64: dts: mediatek: changes for v5.13 (second round) [not found] <22814673-e9fe-f65b-cc0f-b02be4f90d1a@gmail.com> @ 2021-04-07 16:06 ` Arnd Bergmann 2021-04-07 16:34 ` Matthias Brugger 0 siblings, 1 reply; 5+ messages in thread From: Arnd Bergmann @ 2021-04-07 16:06 UTC (permalink / raw) To: Matthias Brugger Cc: SoC Team, arm-soc, linux-arm-kernel@lists.infradead.org, moderated list:ARM/Mediatek SoC support, Fabien Parent, Irui Wang, Rob Herring, DTML 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [GIT PULL] arm64: dts: mediatek: changes for v5.13 (second round) 2021-04-07 16:06 ` [GIT PULL] arm64: dts: mediatek: changes for v5.13 (second round) Arnd Bergmann @ 2021-04-07 16:34 ` Matthias Brugger 2021-04-07 17:55 ` Arnd Bergmann 0 siblings, 1 reply; 5+ messages in thread From: Matthias Brugger @ 2021-04-07 16:34 UTC (permalink / raw) To: Arnd Bergmann Cc: SoC Team, arm-soc, linux-arm-kernel@lists.infradead.org, moderated list:ARM/Mediatek SoC support, Fabien Parent, Irui Wang, Rob Herring, DTML, Chun-Kuang Hu 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [GIT PULL] arm64: dts: mediatek: changes for v5.13 (second round) 2021-04-07 16:34 ` Matthias Brugger @ 2021-04-07 17:55 ` Arnd Bergmann 2021-04-07 22:14 ` Fabien Parent 0 siblings, 1 reply; 5+ messages in thread From: Arnd Bergmann @ 2021-04-07 17:55 UTC (permalink / raw) To: Matthias Brugger Cc: SoC Team, arm-soc, linux-arm-kernel@lists.infradead.org, moderated list:ARM/Mediatek SoC support, Fabien Parent, Irui Wang, Rob Herring, DTML, Chun-Kuang Hu 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [GIT PULL] arm64: dts: mediatek: changes for v5.13 (second round) 2021-04-07 17:55 ` Arnd Bergmann @ 2021-04-07 22:14 ` Fabien Parent 2021-04-08 7:26 ` Arnd Bergmann 0 siblings, 1 reply; 5+ messages in thread From: Fabien Parent @ 2021-04-07 22:14 UTC (permalink / raw) To: Arnd Bergmann Cc: Matthias Brugger, SoC Team, arm-soc, linux-arm-kernel@lists.infradead.org, moderated list:ARM/Mediatek SoC support, Irui Wang, Rob Herring, DTML, Chun-Kuang Hu 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [GIT PULL] arm64: dts: mediatek: changes for v5.13 (second round) 2021-04-07 22:14 ` Fabien Parent @ 2021-04-08 7:26 ` Arnd Bergmann 0 siblings, 0 replies; 5+ messages in thread From: Arnd Bergmann @ 2021-04-08 7:26 UTC (permalink / raw) To: Fabien Parent Cc: Matthias Brugger, SoC Team, arm-soc, linux-arm-kernel@lists.infradead.org, moderated list:ARM/Mediatek SoC support, Irui Wang, Rob Herring, DTML, Chun-Kuang Hu 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-04-08 7:26 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <22814673-e9fe-f65b-cc0f-b02be4f90d1a@gmail.com>
2021-04-07 16:06 ` [GIT PULL] arm64: dts: mediatek: changes for v5.13 (second round) Arnd Bergmann
2021-04-07 16:34 ` Matthias Brugger
2021-04-07 17:55 ` Arnd Bergmann
2021-04-07 22:14 ` Fabien Parent
2021-04-08 7:26 ` Arnd Bergmann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).