Received: by 2002:a6b:500f:0:0:0:0:0 with SMTP id e15csp1021620iob; Fri, 13 May 2022 19:58:28 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwNHhrovwHLj9+UXxLeSdjuWEFbyxUR0mbMUzZs4E/vSaMTS5CfxzpwC5zK6wH11cIKEqun X-Received: by 2002:a05:600c:3583:b0:394:77fc:d6a2 with SMTP id p3-20020a05600c358300b0039477fcd6a2mr7105514wmq.63.1652497107939; Fri, 13 May 2022 19:58:27 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1652497107; cv=none; d=google.com; s=arc-20160816; b=dV8KUaQmrrI5fBYRPKVSvAglP2is9MnaqUHK0RsXH3qI5VC4VM6N4FGW2QvtB+ebgx JSC32ibyn5opIZg/qQxKrTluqBcPD4CL72qsk1eAYc4NS8huNXNLrhlAEqV8C42GtuW0 HJvIiO4iT8bNYO1fvchJsBtrYHGNszkCXBVBAvydlxSyJAIpLanLGtsW+Gq7qm1Y8fzS 32g5C8/WAHAeoQgbGX4Wn6CoRg10HLrc+M+a1NJjD/EK15FWJnrsCBzaRcIGf95wu+LZ RJq2qLnWmjwbMqU/xk0g/CvAbQ/gGw3zvZlufv0L/fz4REGJ5yAsvpe8TUqfvBl9ysdC vmAQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:subject:message-id:date:from:in-reply-to :references:mime-version:dkim-signature; bh=J3wpuO3G1zUNPiMfqaAtLGBtGTOMe9/8kb2GcX2nNqk=; b=yp0l2I9NeT9V6hUxwS11NggQafs5beXHsTVu3CC7K6xG3qXk+5b78oxayM0q4VY1kB 4eLyH3YadLweZp3BLl3uPQa5lyG/mzzpkWXHuzyjT7fWYVZNDTmqLyqIN5giVSOU4DqD uC1EMCVkrIER7GVw+mcvl50hpx+SfBae1265LcPjZG7aTbJSgnhVmsLUiW6lwLKoSy0v 8sl9uLT5gD2GHq7sCW++itXMJssDmJ/udZdtQpC0I/hvh5nLVu2iSn2+/gjSnrs0VEgK rmmcPwqGKPrYOPzYoxKWl0AsHGv4VXoIFWhMquULZS7lQF5fLxDlBTl0TlBVBTviT/Db IyEA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@vanguardiasur-com-ar.20210112.gappssmtp.com header.s=20210112 header.b=G7rpee1n; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from lindbergh.monkeyblade.net (lindbergh.monkeyblade.net. [2620:137:e000::1:18]) by mx.google.com with ESMTPS id i2-20020a05600c354200b00394800c4c5bsi2497081wmq.178.2022.05.13.19.58.27 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 13 May 2022 19:58:27 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18; Authentication-Results: mx.google.com; dkim=pass header.i=@vanguardiasur-com-ar.20210112.gappssmtp.com header.s=20210112 header.b=G7rpee1n; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 51511397341; Fri, 13 May 2022 16:42:43 -0700 (PDT) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1358739AbiELVdW (ORCPT + 99 others); Thu, 12 May 2022 17:33:22 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:59278 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1352990AbiELVdU (ORCPT ); Thu, 12 May 2022 17:33:20 -0400 Received: from mail-ej1-x632.google.com (mail-ej1-x632.google.com [IPv6:2a00:1450:4864:20::632]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id BAB2D20F778 for ; Thu, 12 May 2022 14:33:15 -0700 (PDT) Received: by mail-ej1-x632.google.com with SMTP id ch13so12635496ejb.12 for ; Thu, 12 May 2022 14:33:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=vanguardiasur-com-ar.20210112.gappssmtp.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=J3wpuO3G1zUNPiMfqaAtLGBtGTOMe9/8kb2GcX2nNqk=; b=G7rpee1nr777LSnFhf4C2bktAOoeKKFMBKA9XRG2w1I5bfy5/neDr9JntXRStjs7IL WHS2I40fa/aqJ4yC60jSyCYSXojutLte9EfUiLuC9xChkPkGJGKCMZ7CB/cWENp9+Qo1 rFFoHQHH+3VZwrgF95lGQxLKBmhR6bVcjcmt2llKIXEoGEs2qUh1C95MKAf2bLlxNkPx WSUjZn4jvXIk2zRS64x9GabWz6mZVzpW08I4nV7CRyfOyCpjKiPWKvHyU7a1a+nqi5Bp bY2HCV+o5KLlPaeQ8V/Z+4RWHo9EsLeiwFuZhd8+4GzvUWs5Orktof6Tm+6W5RTR1o0Y D3OQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=J3wpuO3G1zUNPiMfqaAtLGBtGTOMe9/8kb2GcX2nNqk=; b=fumtoI0K/Lpcuf1GhqB7v8tnuaND8h3lGB8TWme4YHrRR0Qe9ouVyO5BfNO2ebkkAh g6G6fMw1u38jIfJci6yS6rDOTN+bMzStQ3Kev5JesIbwQMQt0gYbJPGqJh2KpMUo09Mq Eg/3R/BCPAmHdhu8gRKQ1Vvpz7/Q37s02ElIgxebf1Er7BqQF8t2JsZwRO5D+dEXAO6d 0jOARIgk0XgkxXda72ggmiwddiettB4ZwkZT6F3REZXNuxeNsjpnMNoFOX5ssGojjVNJ gJUHw8nSreEUw8pf3n74A5cpnv1pY23bFrezK0+8vsg2l5qAhOvtcA21AhnGz2PZx1Ek tvnA== X-Gm-Message-State: AOAM531mQPlAdOwkzIvNCpD1FG1ztGHJHdA5oXUqwh5rba2PWU7NRpsB 0sBUVkoBDxWT5Gisy70QXVY/n3UASZJrt6b5iHeXmA== X-Received: by 2002:a17:906:656:b0:6f4:ebc2:da81 with SMTP id t22-20020a170906065600b006f4ebc2da81mr1583629ejb.757.1652391194218; Thu, 12 May 2022 14:33:14 -0700 (PDT) MIME-Version: 1.0 References: <20220508202544.501981-1-frattaroli.nicolas@gmail.com> <1959188.DQhRDO7MrQ@archbook> <14624530.8YKtBhKLIE@archbook> In-Reply-To: <14624530.8YKtBhKLIE@archbook> From: Ezequiel Garcia Date: Thu, 12 May 2022 18:33:03 -0300 Message-ID: Subject: Re: [PATCH v2 3/3] arm64: dts: rockchip: Add Hantro encoder node to rk356x To: Nicolas Frattaroli Cc: Rob Herring , Krzysztof Kozlowski , Heiko Stuebner , devicetree , linux-arm-kernel , "open list:ARM/Rockchip SoC..." , Linux Kernel Mailing List Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RDNS_NONE, SPF_HELO_NONE,T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, May 12, 2022 at 5:00 PM Nicolas Frattaroli wrote: > > On Donnerstag, 12. Mai 2022 16:16:52 CEST Ezequiel Garcia wrote: > > On Tue, May 10, 2022 at 12:28 PM Nicolas Frattaroli > > 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 > > > > 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 > > > > > --- > > > > > 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@fde60000 { > > > > > status = "disabled"; > > > > > }; > > > > > > > > > > + vepu: video-codec@fdee0000 { > > > > > + compatible = "rockchip,rk3568-vepu"; > > > > > + reg = <0x0 0xfdee0000 0x0 0x800>; > > > > > + interrupts = ; > > > > > + 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@ff9a0000 { > > compatible = "rockchip,rk3288-vpu"; > > reg = <0x0 0xff9a0000 0x0 0x800>; > > interrupts = , > > ; > > 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, > > 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@fdee0800 { > > > > > + compatible = "rockchip,rk3568-iommu"; > > > > > + reg = <0x0 0xfdee0800 0x0 0x40>; > > > > > + interrupts = ; > > > > > + clocks = <&cru ACLK_JENC>, <&cru HCLK_JENC>; > > > > > + clock-names = "aclk", "iface"; > > > > > + power-domains = <&power RK3568_PD_RGA>; > > > > > + #iommu-cells = <0>; > > > > > + }; > > > > > + > > > > > sdmmc2: mmc@fe000000 { > > > > > compatible = "rockchip,rk3568-dw-mshc", "rockchip,rk3288-dw-mshc"; > > > > > reg = <0x0 0xfe000000 0x0 0x4000>; > > > > > -- > > > > > 2.36.0 > > > > > > > > > > > > > > > _______________________________________________ > > > > > Linux-rockchip mailing list > > > > > Linux-rockchip@lists.infradead.org > > > > > http://lists.infradead.org/mailman/listinfo/linux-rockchip > > > > > > > > > > > > > > > > > > > > > >