Message-ID: <4167579.6PsWsQAL7t@diego>
Date: Thu, 22 Aug 2024 09:01:34 +0200
From: Heiko Stübner <heiko@...ech.de>
To: Conor Dooley <conor@...nel.org>,
 Cristian Ciocaltea <cristian.ciocaltea@...labora.com>
Cc: Andrzej Hajda <andrzej.hajda@...el.com>,
 Neil Armstrong <neil.armstrong@...aro.org>, Robert Foss <rfoss@...nel.org>,
 Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
 Jonas Karlman <jonas@...boo.se>, Jernej Skrabec <jernej.skrabec@...il.com>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
 David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>,
 Sandy Huang <hjc@...k-chips.com>, Andy Yan <andy.yan@...k-chips.com>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Mark Yao <markyao0591@...il.com>,
 Sascha Hauer <s.hauer@...gutronix.de>, dri-devel@...ts.freedesktop.org,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-rockchip@...ts.infradead.org, devicetree@...r.kernel.org,
 kernel@...labora.com, Alexandre ARNOUD <aarnoud@...com>,
 Luis de Arquer <ldearquer@...il.com>
Subject:
 Re: [PATCH v4 3/4] dt-bindings: display: rockchip: Add schema for RK3588 HDMI
 TX Controller

Am Donnerstag, 22. August 2024, 01:22:16 CEST schrieb Cristian Ciocaltea:
> On 8/22/24 12:38 AM, Heiko Stuebner wrote:
> > 
> > 
> > Am 21. August 2024 23:28:55 MESZ schrieb Conor Dooley <conor@...nel.org>:
> >> Cristian, Heiko,
> >>
> >> On Wed, Aug 21, 2024 at 11:38:01PM +0300, Cristian Ciocaltea wrote:
> >>> On 8/21/24 6:07 PM, Conor Dooley wrote:
> >>>> On Tue, Aug 20, 2024 at 11:12:45PM +0300, Cristian Ciocaltea wrote:
> >>>>> On 8/20/24 7:14 PM, Conor Dooley wrote:
> >>>>>> On Tue, Aug 20, 2024 at 03:37:44PM +0300, Cristian Ciocaltea wrote:
> >>>>>>> On 8/19/24 7:53 PM, Conor Dooley wrote:
> >>>>>>>> On Mon, Aug 19, 2024 at 01:29:30AM +0300, Cristian Ciocaltea wrote:
> >>>>>>>>> +  rockchip,grf:
> >>>>>>>>> +    $ref: /schemas/types.yaml#/definitions/phandle
> >>>>>>>>> +    description:
> >>>>>>>>> +      Most HDMI QP related data is accessed through SYS GRF regs.
> >>>>>>>>> +
> >>>>>>>>> +  rockchip,vo1-grf:
> >>>>>>>>> +    $ref: /schemas/types.yaml#/definitions/phandle
> >>>>>>>>> +    description:
> >>>>>>>>> +      Additional HDMI QP related data is accessed through VO1 GRF regs.
> >>>>>>>>
> >>>>>>>> Why are these required? What prevents you looking up the syscons by
> >>>>>>>> compatible?
> >>>>>>>
> >>>>>>> That is for getting the proper instance:
> >>>>>>
> >>>>>> Ah, that makes sense. I am, however, curious why these have the same
> >>>>>> compatible when they have different sized regions allocated to them.
> >>>>>
> >>>>> Good question, didn't notice.  I've just checked the TRM and, in both
> >>>>> cases, the maximum register offset is within the 0x100 range.  Presumably
> >>>>> this is nothing but an inconsistency, as the syscons have been added in
> >>>>> separate commits.
> >>>>
> >>>> Is that TRM publicly available? I do find it curious that devices sound
> >>>> like they have different contents have the same compatible. In my view,
> >>>> that is incorrect and they should have unique compatibles if the
> >>>> contents (and therefore the programming model) differs.
> >>>
> >>> Don't know if there's an official location to get it from, but a quick
> >>> search on internet shows a few repos providing them, e.g. [1].
> >>>
> >>> Comparing "6.14 VO0_GRF Register Description" at pg. 777 with "6.15 VO1_GRF
> >>> Register Description" at pg. 786 (from Part1) reveals the layout is mostly
> >>> similar, with a few variations though.
> >>
> >> Page references and everything, thank you very much. I don't think those
> >> two GRFs should have the same compatibles, they're, as you say, similar
> >> but not identical. Seems like a bug to me!
> >>
> >> Heiko, what do you think?
> > 
> > Yes, while the register names sound similar, looking at the bit
> > definitions this evening revealed that they handle vastly different
> > settings.
> > 
> > So I guess we should fix the compatibles. They are all about graphics
> > stuff and HDMI actually is the first output, so right now WE can at least
> > still claim the no-users joker ;-)
> 
> I couldn't find any driver doing a lookup for them by compatible, so I
> think it's fine to fix them - should we go for "rockchip,rk3588-vo0-grf" and
> "rockchip,rk3588-vo1-grf", respectively?

yep. While things like the MIPICDPHY{0,1}_GRF really are identifcal and
serve two separate controllers ... vo0 and vo1 are very different entities,
so fixing the compatible to reflect that makes a lot of sense.


> vo0_grf seems to be used by the usbdp phy nodes:
> 
>     usbdp_phy0: phy@...80000 {
>         compatible = "rockchip,rk3588-usbdp-phy";
>         [...]
>         rockchip,vo-grf = <&vo0_grf>;
>         [...]
> 
> Same for "usbdp_phy1: phy@...90000".
> 
> While vo1_grf is present in:
> 
>     vop: vop@...90000 {
>         compatible = "rockchip,rk3588-vop";
>         [...]
>         rockchip,vo1-grf = <&vo1_grf>;
>         [...]
> 
> I guess it's too late to drop them while updating the related drivers
> accordingly, hence I wonder if we should keep using the phandles for this
> HDMI thing as well, for consistency reasons.

For the property naming, I guess it just tells the driver which "vo"-grf
to use, so the vop is more explicit in naming it vo1-grf even the vo-grf
in the usbdp phy won't hurt too much.

Of course we can still look up the grf by compatible and deprecate the
phandle references.


@Conor: just for me, did some shift happen in our understanding of dt-
best-practices in terms of syscon via phandle vs. syscon via compatible?

Because Rockchip boards are referencing their GRFs via phandes forever
but similar to the soc vs non-soc node thing, I'd like to stay on top of
best-practices ;-)


Heiko


> > Heiko
> > 
> >>
> >>> [1] https://github.com/FanX-Tek/rk3588-TRM-and-Datasheet
> >>>
> >>>>>
> >>>>>>> 	vo0_grf: syscon@...a6000 {
> >>>>>>> 		compatible = "rockchip,rk3588-vo-grf", "syscon";
> >>>>>>> 		reg = <0x0 0xfd5a6000 0x0 0x2000>;
> >>>>>>> 		clocks = <&cru PCLK_VO0GRF>;
> >>>>>>> 	};
> >>>>>>>
> >>>>>>> 	vo1_grf: syscon@...a8000 {
> >>>>>>> 		compatible = "rockchip,rk3588-vo-grf", "syscon";
> >>>>>>> 		reg = <0x0 0xfd5a8000 0x0 0x100>;
> >>>>>>> 		clocks = <&cru PCLK_VO1GRF>;
> >>>>>>> 	};
> >>>>
> > 
> 

Powered by blists - more mailing lists