Re: [PATCH v2 1/4] arm64: dts: rockchip: Add PD to csi dphy node on rk356x

[Diederik de Haas]

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@xxxxxxxxx>
> > ---
> > 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>;

Attachment: signature.asc
Description: PGP signature