Message-ID: <9445203.RYAX2qA09r@archbook>
Date:   Fri, 13 May 2022 16:44:28 +0200
From:   Nicolas Frattaroli <frattaroli.nicolas@...il.com>
To:     Ezequiel Garcia <ezequiel@...guardiasur.com.ar>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Heiko Stuebner <heiko@...ech.de>,
        devicetree <devicetree@...r.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        "open list:ARM/Rockchip SoC..." <linux-rockchip@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 3/3] arm64: dts: rockchip: Add Hantro encoder node to rk356x

On Freitag, 13. Mai 2022 15:07:51 CEST Ezequiel Garcia wrote:
> Hi Nicolas,
> 
> On Fri, May 13, 2022 at 3:23 AM Nicolas Frattaroli
> <frattaroli.nicolas@...il.com> wrote:
> >
> > On Donnerstag, 12. Mai 2022 23:33:03 CEST Ezequiel Garcia wrote:
> > > On Thu, May 12, 2022 at 5:00 PM Nicolas Frattaroli
> > > <frattaroli.nicolas@...il.com> wrote:
> > > >
> > > > On Donnerstag, 12. Mai 2022 16:16:52 CEST Ezequiel Garcia wrote:
> > > > > On Tue, May 10, 2022 at 12:28 PM Nicolas Frattaroli
> > > > > <frattaroli.nicolas@...il.com> wrote:
> > > > > >
> > > > > > Hi Ezequiel,
> > > > > >
> > > > > > On Montag, 9. Mai 2022 16:17:03 CEST Ezequiel Garcia wrote:
> > > > > > > Hi Nicolas,
> > > > > > >
> > > > > > > On Sun, May 8, 2022 at 5:26 PM Nicolas Frattaroli
> > > > > > > <frattaroli.nicolas@...il.com> wrote:
> > > > > > > >
> > > > > > > > The RK3566 and RK3568 come with a dedicated Hantro instance solely for
> > > > > > > > encoding. This patch adds a node for this to the device tree, along with
> > > > > > > > a node for its MMU.
> > > > > > > >
> > > > > > > > Signed-off-by: Nicolas Frattaroli <frattaroli.nicolas@...il.com>
> > > > > > > > ---
> > > > > > > >  arch/arm64/boot/dts/rockchip/rk356x.dtsi | 21 +++++++++++++++++++++
> > > > > > > >  1 file changed, 21 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> > > > > > > > index 7cdef800cb3c..2e3c9e1887e3 100644
> > > > > > > > --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> > > > > > > > +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> > > > > > > > @@ -508,6 +508,27 @@ gpu: gpu@...60000 {
> > > > > > > >                 status = "disabled";
> > > > > > > >         };
> > > > > > > >
> > > > > > > > +       vepu: video-codec@...e0000 {
> > > > > > > > +               compatible = "rockchip,rk3568-vepu";
> > > > > > > > +               reg = <0x0 0xfdee0000 0x0 0x800>;
> > > > > > > > +               interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
> > > > > > > > +               interrupt-names = "vepu";
> > > > > > >
> > > > > > > It this block "encoder only" and if so, maybe we should remove the
> > > > > > > "interrupt-names" [1]?
> > > > > > >
> > > > > > > The driver is able to handle it. See:
> > > > > > >
> > > > > > > https://elixir.bootlin.com/linux/latest/source/drivers/staging/media/hantro/hantro_drv.c#L962
> > > > > > >
> > > > > > > You might have to adjust the dt-bindings for this.
> > > > > > >
> > > > > > > [1] https://lore.kernel.org/linux-media/20210324151715.GA3070006@robh.at.kernel.org/
> > > > > >
> > > > > > What the Linux driver can handle should not matter to the device tree;
> > > > > > device trees are independent of drivers and kernels.
> > > > > >
> > > > >
> > > > > I guess my message wasn't clear, no need to lecture me on Device
> > > > > Trees, although I appreciate
> > > > > your friendly reminder of what a Device Tree is.
> > > > >
> > > > > Having said that, the binding is designed to support both decoders and encoders
> > > > > for instance:
> > > > >
> > > > >         vpu: video-codec@...a0000 {
> > > > >                 compatible = "rockchip,rk3288-vpu";
> > > > >                 reg = <0x0 0xff9a0000 0x0 0x800>;
> > > > >                 interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>,
> > > > >                              <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
> > > > >                 interrupt-names = "vepu", "vdpu";
> > > > >                 clocks = <&cru ACLK_VCODEC>, <&cru HCLK_VCODEC>;
> > > > >                 clock-names = "aclk", "hclk";
> > > > >                 iommus = <&vpu_mmu>;
> > > > >                 power-domains = <&power RK3288_PD_VIDEO>;
> > > > >         };
> > > > >
> > > > > Hence the question is why do you splitted the encoder to its own node?
> > > >
> > > > It has its own IOMMU and is in a different power domain than the decoder.
> > > > I think I have mentioned this multiple times before, including in the
> > > > cover letter.
> > > >
> > > > Assuming you do not believe me, feel free to check the TRM, of which I
> > > > am sure you also have a copy: page 475 of Part 1 shows the VPU being in
> > > > PD_VPU while the JPEG encoder is in PD_RGA. Pages 478 and 479 of Part 2,
> > > > Section 10.5, shows that the JPEG encoder (VEPU121)'s base is not the
> > > > same as the Hantro decoder (VDPU121)'s base, and their IOMMUs which are
> > > > based relative to their base offset are therefore also not at the same
> > > > address. If you think the TRM must be wrong then, consider the fact that
> > > > I have actually run this patch set, presumably being the only person to
> > > > do so, and found that it works, so no, the addresses and power domains
> > > > are correct.
> > > >
> > > > I do not see any way in which it would make sense to put this into the
> > > > same node as the decoder. It would not even be possible to do this in
> > > > your bindings, as they specify a maxItems for power-domains and iommus
> > > > of 1. Even if I modified them the driver wouldn't know which PD and
> > > > IOMMU belongs to decoder and encoder.
> > > >
> > > > I think if we put this encoder in the same node as the decoder, we
> > > > might as well take this to its natural conclusion and put the entire
> > > > device tree into a single very large node. It's not the same hardware,
> > > > it cannot be modelled as being the same hardware, just because the
> > > > bindings lets people model some separate hardware as the same hardware
> > > > doesn't mean this applies to this hardware.
> > > >
> > > > Long story short, why did I split the encoder to its own node? The
> > > > answer is that I didn't. I simply refused to combine it into a node
> > > > that it has nothing to do with.
> > > >
> > >
> > > As I've mentioned:
> > >
> > > """
> > > the current binding models the idea of decoder and encoder
> > > being the same device. This has never been really really accurate,
> > > as the encoder and decoders have always been more or less independent.
> > >
> > > The reason for having them on a single device are mostly historical,
> > > some old devices shared some resource. I don't think this is the case anymore,
> > > but the binding was still modeled to support that.
> > > """
> > >
> > > The PX30 and RK3399 VPUs are probably pretty independent as well,
> > > and in retrospective, we should have done separated Device Tree nodes.
> > > For historical reasons, we didn't, and we introduced those weird "enc_offset"
> > > and "dec_offset" fields:
> > >
> > > const struct hantro_variant px30_vpu_variant = {
> > >         .enc_offset = 0x0,
> > >         .enc_fmts = rockchip_vpu_enc_fmts,
> > >         .num_enc_fmts = ARRAY_SIZE(rockchip_vpu_enc_fmts),
> > >         .dec_offset = 0x400,
> > >         .dec_fmts = rk3399_vpu_dec_fmts,
> > >
> >
> > As I've mentioned: that doesn't work for this hardware. It's not just the
> > memory addresses. You literally quoted the part where I explain this, and
> > then decided to completely ignore it.
> 
> I didn't ignore anything. I was just trying to explain you,
> how the decoder and the encoder could have been separated for almost
> all the other Rockchip devices, just like you are doing here.
> 
> > I will not explain it again, you
> > have the explanation once more right in this e-mail. Read it.
> >
> > Not to mention that you've also ignored that I disagree with rob's
> > assessment about interrupt-names.
> >
> 
> I didn't ignore it, I just didn't reply to it. You think this is about
> changing a dt-binding, but you are actually introducing a new dt-binding
> since you are adding a new compatible string.
> 
> You are doing so by extending an existing dt-binding.
> 
> I am explaining you the _existing_ dt-binding models the (incorrect) idea
> of a combined decoder and encoder. Since your device is encoder-only
> and has a single interrupt line, you should omit the interrupt-names,
> because it doesn't not add anything.
> 
> (About your dislike for the "default" string in /proc,  that is a
> driver thing, which can be changed. It is not related to the
> dt-binding).
> 
> > I'm actually done arguing with you, this is going in circles. v4 will not
> > address any of your concerns, because it's either literally impossible or
> > because I disagree with your concern and you did not actually address my
> > disagreement.
> >
> 
> Let's just wait for a Device Tree maintainer then. If you get a +1
> from a DT maintainer for your dt-binding change, then I'll review and
> consider how the rest of the patches look like.
> 
> However, it is very important that you moderate your communication,
> you have been very pedantic and rude since your first reply.
> 
> Hope you can do that!
> Thanks,
> Ezequiel

I apologise for my rude and pedantic communication, I misread your
replies as you repeatedly suggesting I merge the nodes into one. Had
I understood that this wasn't your intention, and you were trying to
explain to me how the old model worked, I would have been less grumpy
about this. However, I already did know how the old model worked, and
your explanation registered as you trying to make me use it to me as
I did not realise you generally agreed with me separating the encoder
into its own node.

Regards,
Nicolas Frattaroli

> 
> 
> > >
> > > > > If we have good reasons to have separated Device Tree nodes,
> > > > > then having interrupt-names = "vepu" for its only interrupt line
> > > > > doesn't make sense.
> > > >
> > > > How does it not make sense? The bindings allow for a vdpu only
> > > > interrupt-names, which in my understanding makes the same amount
> > > > of sense.
> > > >
> > >
> > > That applies for the binding for the previous existing compatible strings.
> > >
> > > You are adding a new compatible string, so just change the binding
> > > so it no longer requires "interrupt-names", for its single interrupt line.
> > >
> > > Quoting devicetree maintainer [1]:
> > >
> > > """
> > >  *-names are used to distinguish multiple entries
> > > and don't add anything if only a single entry.
> > > """
> > >
> > > [1] https://lore.kernel.org/linux-media/20210324151715.GA3070006@robh.at.kernel.org/
> > >
> > > Thanks!
> > > Ezequiel
> > >
> > > > Regards,
> > > > Nicolas Frattaroli
> > > >
> > > > >
> > > > > > What does matter though is to be consistent in the bindings.
> > > > > > interrupt-names is a required property even if there's only a vdpu
> > > > > > interrupt. I modelled my vepu-only binding after this case.
> > > > > >
> > > > >
> > > > > The current binding models the idea of decoder and encoder
> > > > > being the same device. This has never been really really accurate,
> > > > > as the encoder and decoders have always been more or less independent.
> > > > >
> > > > > The reason for having them on a single device are mostly historical,
> > > > > some old devices shared some resource. I don't think this is the case anymore,
> > > > > but the binding was still modeled to support that.
> > > > >
> > > > > Hopefully this makes sense!
> > > > > Thanks,
> > > > > Ezequiel
> > > > >
> > > > >
> > > > > > If robh thinks there is no value to having the interrupt show up
> > > > > > as anything other than "default" in /proc/interrupts, then I respectfully
> > > > > > disagree with that opinion and point out that this should have been brought
> > > > > > up when the vdpu-only case in the bindings was made to require
> > > > > > interrupt-names also.
> > > > > >
> > > > > > Changing the binding now that there theoretically could be drivers out
> > > > > > in the wild (though I doubt it) that do require interrupt-names, because
> > > > > > the binding told them that this is okay to do, seems unwise to me.
> > > > > >
> > > > > > Regards,
> > > > > > Nicolas Frattaroli
> > > > > >
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Ezequiel
> > > > > > >
> > > > > > > > +               clocks = <&cru ACLK_JENC>, <&cru HCLK_JENC>;
> > > > > > > > +               clock-names = "aclk", "hclk";
> > > > > > > > +               iommus = <&vepu_mmu>;
> > > > > > > > +               power-domains = <&power RK3568_PD_RGA>;
> > > > > > > > +       };
> > > > > > > > +
> > > > > > > > +       vepu_mmu: iommu@...e0800 {
> > > > > > > > +               compatible = "rockchip,rk3568-iommu";
> > > > > > > > +               reg = <0x0 0xfdee0800 0x0 0x40>;
> > > > > > > > +               interrupts = <GIC_SPI 63 IRQ_TYPE_LEVEL_HIGH>;
> > > > > > > > +               clocks = <&cru ACLK_JENC>, <&cru HCLK_JENC>;
> > > > > > > > +               clock-names = "aclk", "iface";
> > > > > > > > +               power-domains = <&power RK3568_PD_RGA>;
> > > > > > > > +               #iommu-cells = <0>;
> > > > > > > > +       };
> > > > > > > > +
> > > > > > > >         sdmmc2: mmc@...00000 {
> > > > > > > >                 compatible = "rockchip,rk3568-dw-mshc", "rockchip,rk3288-dw-mshc";
> > > > > > > >                 reg = <0x0 0xfe000000 0x0 0x4000>;
> > > > > > > > --
> > > > > > > > 2.36.0
> > > > > > > >
> > > > > > > >
> > > > > > > > _______________________________________________
> > > > > > > > Linux-rockchip mailing list
> > > > > > > > Linux-rockchip@...ts.infradead.org
> > > > > > > > http://lists.infradead.org/mailman/listinfo/linux-rockchip
> > > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > >
> > > >
> > > >
> > > >
> > > >
> > >
> >
> >
> >
> >
> 

Powered by blists - more mailing lists