| Message ID | 22814673-e9fe-f65b-cc0f-b02be4f90d1a@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [GIT,PULL] arm64: dts: mediatek: changes for v5.13 (second round) | expand |
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
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
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
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
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
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. Please have a look. Regards, Matthias --- The following changes since commit a7dceafed43a4a610d340da3703653cca2c50c1d: arm64: dts: mediatek: fix reset GPIO level on pumpkin (2021-04-01 11:39:10 +0200) are available in the Git repository at: https://git.kernel.org/pub/scm/linux/kernel/git/matthias.bgg/linux.git/ tags/v5.12-next-dts64.2 for you to fetch changes up to 3a8c657a3b4a70405582bf8e1cc39ff3b55c53f6: arm64: dts: mediatek: mt8167: add some DRM nodes (2021-04-06 15:12:40 +0200) ---------------------------------------------------------------- - separate enconder blocks for MT8173 MT8167: - add power domain node - add nodes for graphical output (iommu, larb, smi, mmsys) ---------------------------------------------------------------- Fabien Parent (6): arm64: dts: mediatek: mt8167: add power domains arm64: dts: mediatek: mt8167: add mmsys node arm64: dts: mediatek: mt8167: add smi_common node arm64: dts: mediatek: mt8167: add larb nodes arm64: dts: mediatek: mt8167: add iommu node arm64: dts: mediatek: mt8167: add some DRM nodes Irui Wang (1): arm64: dts: mt8173: Separating mtk-vcodec-enc device node arch/arm64/boot/dts/mediatek/mt8167.dtsi | 270 +++++++++++++++++++++++++++++++ arch/arm64/boot/dts/mediatek/mt8173.dtsi | 60 +++---- 2 files changed, 301 insertions(+), 29 deletions(-)