| Message ID | 20241008113344.23957-2-didi.debian@cknow.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rockchip: Fix several DT validation errors | expand |
Hi Diederik, On 10/8/24 13:15, Diederik de Haas wrote: > The "rockchip-inno-csi-dphy.yaml" binding requires the power-domains > property. According to RK3568 TRM Part 1 section 7.3 (page 475) the > CSIHOST is placed in the PD_VI power domain. > So set the csi_dphy node power-domains property accordingly. Thanks for the patch. However, I am not sure about this one. The CSI host sure is in this power domain, but we are talking about the CSI PHY here, right? According to the table CSIPHY is part of the power domain "ALIVE", which leads me to believe that the power domain is not necessary here. However, I guess you could put "RK3568_PD_LOGIC_ALIVE" here. It should be noted, though, that I still haven't figured out what the role of this CSI host actually is. I know that the RK3568 ISP has its own MIPI CSI host controller within its register space. But I can only guess right now that this CSI host is somehow linked to the RK3568 VICAP, which is also capable of receiving MIPI CSI. Maybe we can leave this up to however brings up the RK3568 VICAP MIPI CSI feature :-) Best regards, Michael > > Fixes: b6c228401b25 ("arm64: dts: rockchip: add csi dphy node to rk356x") > Signed-off-by: Diederik de Haas <didi.debian@cknow.org> > --- > changes in v2: > - No change > > arch/arm64/boot/dts/rockchip/rk356x.dtsi | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi > index 0ee0ada6f0ab..d581170914f9 100644 > --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi > +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi > @@ -1790,6 +1790,7 @@ csi_dphy: phy@fe870000 { > clocks = <&cru PCLK_MIPICSIPHY>; > clock-names = "pclk"; > #phy-cells = <0>; > + power-domains = <&power RK3568_PD_VI>; > resets = <&cru SRST_P_MIPICSIPHY>; > reset-names = "apb"; > rockchip,grf = <&grf>;
Hi Michael, On Tue Oct 8, 2024 at 2:32 PM CEST, Michael Riesch wrote: > On 10/8/24 13:15, Diederik de Haas wrote: > > The "rockchip-inno-csi-dphy.yaml" binding requires the power-domains > > property. According to RK3568 TRM Part 1 section 7.3 (page 475) the > > CSIHOST is placed in the PD_VI power domain. > > So set the csi_dphy node power-domains property accordingly. > > Thanks for the patch. However, I am not sure about this one. Thanks for your reply. > The CSI host sure is in this power domain, but we are talking about the > CSI PHY here, right? According to the table CSIPHY is part of the power > domain "ALIVE", which leads me to believe that the power domain is not > necessary here. However, I guess you could put "RK3568_PD_LOGIC_ALIVE" here. > > It should be noted, though, that I still haven't figured out what the > role of this CSI host actually is. I know that the RK3568 ISP has its > own MIPI CSI host controller within its register space. But I can only > guess right now that this CSI host is somehow linked to the RK3568 > VICAP, which is also capable of receiving MIPI CSI. Maybe we can leave > this up to however brings up the RK3568 VICAP MIPI CSI feature :-) It indeed isn't as clear cut as I want(ed) it to be. Given that you're also the author of commit 29c99fb085ad ("phy: rockchip: add support for the rk356x variant to rockchip-inno-csidphy"), which added support to the driver part, your opinion should have more weight then mine (IMO). Nonetheless, I collected some extra data points: - The TRM part 1 makes several mentions of 'dphy' in a block describing 'GRF_VI_CON1' (page 381 & 382). The above mentioned commit only added 'PHY_REG' for 'CON0', which I assume was deliberate given your response above. But 'GRF_VI_CON1' does have 'VI' in its name - In rk356x.dtsi there are 'dsi_dphy0' and 'dsi_dphy1' which have 'RK3568_PD_VO' in their 'power-domains' property. Page 475 has 'DSIHOST' in 'PD_VO', while 'DSIPHY' is (also) in the 'ALIVE' power domain. So to be consistent it seems to me that 'csi_dphy' should be in 'PD_VI' or the 'dsi_dphy' nodes should be placed/moved to 'RK3568_PD_LOGIC_ALIVE'. - Of all the compatibles from the binding, I only found 'rockchip,px30-csi-dphy' referenced in DT files (next to 'rockchip,rk3568-csi-dphy' in rk356x.dtsi) and in px30.dtsi the 'csi_dphy' node has 'PX30_PD_VI' as value for its 'power-domains' property. But this is all 'circumstantial'; it would be nice to have a clear answer (from Rockchip?). Cheers, Diederik > > Fixes: b6c228401b25 ("arm64: dts: rockchip: add csi dphy node to rk356x") > > Signed-off-by: Diederik de Haas <didi.debian@cknow.org> > > --- > > changes in v2: > > - No change > > > > arch/arm64/boot/dts/rockchip/rk356x.dtsi | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi > > index 0ee0ada6f0ab..d581170914f9 100644 > > --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi > > +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi > > @@ -1790,6 +1790,7 @@ csi_dphy: phy@fe870000 { > > clocks = <&cru PCLK_MIPICSIPHY>; > > clock-names = "pclk"; > > #phy-cells = <0>; > > + power-domains = <&power RK3568_PD_VI>; > > resets = <&cru SRST_P_MIPICSIPHY>; > > reset-names = "apb"; > > rockchip,grf = <&grf>;
diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi index 0ee0ada6f0ab..d581170914f9 100644 --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi @@ -1790,6 +1790,7 @@ csi_dphy: phy@fe870000 { clocks = <&cru PCLK_MIPICSIPHY>; clock-names = "pclk"; #phy-cells = <0>; + power-domains = <&power RK3568_PD_VI>; resets = <&cru SRST_P_MIPICSIPHY>; reset-names = "apb"; rockchip,grf = <&grf>;
The "rockchip-inno-csi-dphy.yaml" binding requires the power-domains property. According to RK3568 TRM Part 1 section 7.3 (page 475) the CSIHOST is placed in the PD_VI power domain. So set the csi_dphy node power-domains property accordingly. Fixes: b6c228401b25 ("arm64: dts: rockchip: add csi dphy node to rk356x") Signed-off-by: Diederik de Haas <didi.debian@cknow.org> --- changes in v2: - No change arch/arm64/boot/dts/rockchip/rk356x.dtsi | 1 + 1 file changed, 1 insertion(+)