Re: [PATCH v1 2/7] clk: bcm: rpi: Add a function to retrieve the maximum

From: Maxime Ripard <maxime@cerno.tech>
To: Stefan Wahren <stefan.wahren@i2se.com>
Cc: Stephen Boyd <sboyd@kernel.org>,
	Broadcom internal kernel review list 
	<bcm-kernel-feedback-list@broadcom.com>,
	Daniel Vetter <daniel@ffwll.ch>, David Airlie <airlied@linux.ie>,
	Emma Anholt <emma@anholt.net>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Michael Turquette <mturquette@baylibre.com>,
	Ray Jui <rjui@broadcom.com>,
	Scott Branden <sbranden@broadcom.com>,
	linux-arm-kernel@lists.infradead.org,
	linux-rpi-kernel@lists.infradead.org,
	dri-devel@lists.freedesktop.org,
	Dom Cobley <popcornmix@gmail.com>,
	linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1 2/7] clk: bcm: rpi: Add a function to retrieve the maximum
Date: Thu, 15 Sep 2022 12:38:01 +0100	[thread overview]
Message-ID: <20220915113801.hexlaer3sp725co5@penduick> (raw)
In-Reply-To: <ebb86dfa-2f89-dddc-0864-42fc4d2e9386@i2se.com>

[-- Attachment #1: Type: text/plain, Size: 3578 bytes --]

Hi Stefan,

On Thu, Sep 15, 2022 at 01:30:02PM +0200, Stefan Wahren wrote:
> Am 15.09.22 um 09:54 schrieb Maxime Ripard:
> > On Wed, Sep 14, 2022 at 08:26:55PM +0200, Stefan Wahren wrote:
> > > Am 14.09.22 um 20:14 schrieb Stephen Boyd:
> > > > Quoting Stefan Wahren (2022-09-14 11:09:04)
> > > > > Am 14.09.22 um 20:05 schrieb Stephen Boyd:
> > > > > > Quoting Stefan Wahren (2022-09-14 10:45:48)
> > > > > > > Am 14.09.22 um 17:50 schrieb Stephen Boyd:
> > > > > > > > Furthermore, I wonder if even that part needs to be implemented.  Why
> > > > > > > > not make a direct call to rpi_firmware_property() and get the max rate?
> > > > > > > > All of that can live in the drm driver. Making it a generic API that
> > > > > > > > takes a 'struct clk' means that it looks like any clk can be passed,
> > > > > > > > when that isn't true. It would be better to restrict it to the one use
> > > > > > > > case so that the scope of the problem doesn't grow. I understand that it
> > > > > > > > duplicates a few lines of code, but that looks like a fair tradeoff vs.
> > > > > > > > exposing an API that can be used for other clks in the future.
> > > > > > > it would be nice to keep all the Rpi specific stuff out of the DRM
> > > > > > > driver, since there more users of it.
> > > > > > Instead of 'all' did you mean 'any'?
> > > > > yes
> > > > Why?
> > > This firmware is written specific for the Raspberry Pi and not stable from
> > > interface point of view. So i'm afraid that the DRM driver is only usable
> > > for the Raspberry Pi at the end with all these board specific dependencies.
> > I'm open for suggestions there, but is there any other bcm2711 device
> > that we support upstream?
>
> I meant the driver as a whole. According to the vc4 binding there are three
> compatibles bcm2835-vc4, cygnus-vc4 and bcm2711-vc5. Unfortunately i don't
> have access to any of these Cygnus boards, so i cannot do any regression
> tests or provide more information to your question.

I don't have access to these boards either, and none of them are
upstream, so I'm not sure what we can do to improve their support by then.

> > If not, I'm not sure what the big deal is at this point. Chances are the
> > DRM driver won't work as is on a different board.
> > 
> > Plus, such a board wouldn't be using config.txt at all, so this whole
> > dance to find what was enabled or not wouldn't be used at all.
>
> My concern is that we reach some point that we need to say this kernel
> version requires this firmware version. In the Raspberry Pi OS world this is
> not a problem, but not all distributions has this specific knowledge.

The recent mess with the Intel GPU firmware
(https://lore.kernel.org/dri-devel/CAPM=9txdca1VnRpp-oNLXpBc2UWq3=ceeim5+Gw4N9tAriRY6A@mail.gmail.com/)
makes it fairly clear that such a situation should be considered a
regression and fixed. So it might be a situation that the downstream
tree will end up in, but it's not something we will allow to happen
upstream.

> > > Emma invested a lot of time to make this open source and now it looks that
> > > like that more and more functionality moves back to firmware.
> > What functionality has been moved back to firmware?
>
> This wasn't a offense against your great work. Just a slight warning that
> some functions of clock or power management moved back into firmware. We
> should watch out, but maybe i emote here.

Yeah, I guess we'll want to consider it on a case per case basis but
it's not like we merged fkms either :)

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]