| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <f5f3dd36-0f90-47dd-bc12-1e2f0207fe7d@collabora.com>
Date: Thu, 6 Mar 2025 18:53:48 +0300
From: Dmitry Osipenko <dmitry.osipenko@...labora.com>
To: Heiko Stübner <heiko@...ech.de>,
Shreeya Patel <shreeya.patel@...labora.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>, Hans Verkuil
<hverkuil@...all.nl>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, jose.abreu@...opsys.com, nelson.costa@...opsys.com,
shawn.wen@...k-chips.com, nicolas.dufresne@...labora.com,
Sebastian Reichel <sebastian.reichel@...labora.com>
Cc: kernel@...labora.com, linux-media@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-rockchip@...ts.infradead.org, Tim Surber <me@...surber.de>,
Christophe JAILLET <christophe.jaillet@...adoo.fr>,
Diederik de Haas <didi.debian@...ow.org>
Subject: Re: [PATCH v14 1/3] arm64: dts: rockchip: Add device tree support for
HDMI RX Controller
On 3/6/25 16:06, Heiko Stübner wrote:
> Hi Dmitry,
>
> Am Donnerstag, 6. März 2025, 08:28:40 MEZ schrieb Dmitry Osipenko:
>> From: Shreeya Patel <shreeya.patel@...labora.com>
>>
>> Add device tree support for Synopsys DesignWare HDMI RX
>> Controller.
>>
>> Reviewed-by: Dmitry Osipenko <dmitry.osipenko@...labora.com>
>> Tested-by: Dmitry Osipenko <dmitry.osipenko@...labora.com>
>> Co-developed-by: Dingxian Wen <shawn.wen@...k-chips.com>
>> Signed-off-by: Dingxian Wen <shawn.wen@...k-chips.com>
>> Signed-off-by: Shreeya Patel <shreeya.patel@...labora.com>
>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@...labora.com>
>> ---
>> .../dts/rockchip/rk3588-base-pinctrl.dtsi | 14 +++++
>> .../arm64/boot/dts/rockchip/rk3588-extra.dtsi | 57 +++++++++++++++++++
>> 2 files changed, 71 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-base-pinctrl.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-base-pinctrl.dtsi
>> index 7f874c77410c..2d4b9986a177 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3588-base-pinctrl.dtsi
>> +++ b/arch/arm64/boot/dts/rockchip/rk3588-base-pinctrl.dtsi
>> @@ -594,6 +594,20 @@ hdmim0_tx1_hpd: hdmim0-tx1-hpd {
>> /* hdmim0_tx1_hpd */
>> <1 RK_PA6 5 &pcfg_pull_none>;
>> };
>> +
>> + /omit-if-no-ref/
>> + hdmim1_rx: hdmim1-rx {
>> + rockchip,pins =
>> + /* hdmim1_rx_cec */
>> + <3 RK_PD1 5 &pcfg_pull_none>,
>> + /* hdmim1_rx_scl */
>> + <3 RK_PD2 5 &pcfg_pull_none_smt>,
>> + /* hdmim1_rx_sda */
>> + <3 RK_PD3 5 &pcfg_pull_none_smt>,
>> + /* hdmim1_rx_hpdin */
>> + <3 RK_PD4 5 &pcfg_pull_none>;
>> + };
>> +
>
> what's the reason for duplicating these pinctrl entries?
>
> The base-pinctrl already contains a separate set of pins (and also a
> variant for the m0 set of pins), so why not check and use the already
> existing ones:
>
> hdmim1_rx_cec: hdmim1-rx-cec {
> rockchip,pins =
> /* hdmim1_rx_cec */
> <3 RK_PD1 5 &pcfg_pull_none>;
> };
>
> hdmim1_rx_hpdin: hdmim1-rx-hpdin {
> rockchip,pins =
> /* hdmim1_rx_hpdin */
> <3 RK_PD4 5 &pcfg_pull_none>;
> };
>
> hdmim1_rx_scl: hdmim1-rx-scl {
> rockchip,pins =
> /* hdmim1_rx_scl */
> <3 RK_PD2 5 &pcfg_pull_none>;
> };
>
> hdmim1_rx_sda: hdmim1-rx-sda {
> rockchip,pins =
> /* hdmim1_rx_sda */
> <3 RK_PD3 5 &pcfg_pull_none>;
> };
>
>
> Having multiple sets (even with differeing settings) will cause confusion
> later on.
Will try this variant, thanks!
>> /omit-if-no-ref/
>> hdmim1_rx_cec: hdmim1-rx-cec {
>> rockchip,pins =
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi
>> index 4a950907ea6f..b7d06f93c8ce 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi
>> +++ b/arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi
>> @@ -135,6 +159,39 @@ i2s10_8ch: i2s@...00000 {
>> status = "disabled";
>> };
>>
>> + hdmi_receiver: hdmi_receiver@...e0000 {
>> + compatible = "rockchip,rk3588-hdmirx-ctrler", "snps,dw-hdmi-rx";
>> + reg = <0x0 0xfdee0000 0x0 0x6000>;
>> + power-domains = <&power RK3588_PD_VO1>;
>> + rockchip,grf = <&sys_grf>;
>> + rockchip,vo1-grf = <&vo1_grf>;
>> + interrupts = <GIC_SPI 177 IRQ_TYPE_LEVEL_HIGH 0>,
>> + <GIC_SPI 178 IRQ_TYPE_LEVEL_HIGH 0>,
>> + <GIC_SPI 179 IRQ_TYPE_LEVEL_HIGH 0>;
>> + interrupt-names = "cec", "hdmi", "dma";
>> + clocks = <&cru ACLK_HDMIRX>,
>> + <&cru CLK_HDMIRX_AUD>,
>> + <&cru CLK_CR_PARA>,
>> + <&cru PCLK_HDMIRX>,
>> + <&cru CLK_HDMIRX_REF>,
>> + <&cru PCLK_S_HDMIRX>,
>> + <&cru HCLK_VO1>;
>> + clock-names = "aclk",
>> + "audio",
>> + "cr_para",
>> + "pclk",
>> + "ref",
>> + "hclk_s_hdmirx",
>> + "hclk_vo1";
>> + resets = <&cru SRST_A_HDMIRX>, <&cru SRST_P_HDMIRX>,
>> + <&cru SRST_HDMIRX_REF>, <&cru SRST_A_HDMIRX_BIU>;
>> + reset-names = "axi", "apb", "ref", "biu";
>> + memory-region = <&hdmi_receiver_cma>;
>> + pinctrl-0 = <&hdmim1_rx>;
>> + pinctrl-names = "default";
>
> hmm, this might be better living in the board dts?
HDMI RX controller is a part of the SoC, it's not specific to a
particular board. If you meaning move it into rock5b.dts, then we
definitely shouldn't do it. It's like moving PCIe controller node into a
board DT :)
--
Best regards,
Dmitry
Powered by blists - more mailing lists