Received-SPF: pass (google.com: domain of linux-kernel+bounces-108648-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240318-rp1-cfe-v1-0-ac6d960ff22d@ideasonboard.com>
 <20240318-rp1-cfe-v1-2-ac6d960ff22d@ideasonboard.com> <eb854c43-1e92-4c19-bfd3-1bde94924319@linaro.org>
 <f97faeb9-8a6b-47c6-9317-daca88257802@ideasonboard.com> <30430e0e-70de-4831-97ad-974e350a2e54@ideasonboard.com>
 <5ca1d005-1beb-47ec-943a-9358ae3c6704@linaro.org> <CAEmqJPp7uGYe993L+ujth2mfRy66s8-S9FNxPY7vwkrboDq9yg@mail.gmail.com>
 <89d459dd-cc8c-4780-a56a-809e24343e69@linaro.org> <CAEmqJPrLP3j37Kcj0mX23x00p=gWuxZPNSUTRGNkcEqsUJ2MjQ@mail.gmail.com>
 <9d238cd6-0e11-4775-bc00-7df50f0a6638@linaro.org> <CAEmqJPoVFRUBRnuvRaeWg6vxDaNMzdFzgj2_Gi5bxh5nacdmDw@mail.gmail.com>
 <0401eb0f-0172-4371-9a16-f51b6b885b55@ideasonboard.com> <CAEmqJPohq1Y11uwBWdGGX3B1vPLEK9_A7OQC=-k+bHcdk3n=mQ@mail.gmail.com>
 <b95fb19d-1c22-45b2-8b87-78e56d17ae8e@ideasonboard.com>
In-Reply-To: <b95fb19d-1c22-45b2-8b87-78e56d17ae8e@ideasonboard.com>
From: Naushir Patuck <naush@raspberrypi.com>
Date: Wed, 20 Mar 2024 08:50:56 +0000
Message-ID: <CAEmqJPoBA4PrbvoGHtiah1B=C7WO+MODUUY0qr0ZdF2eWEB3TA@mail.gmail.com>
Subject: Re: [PATCH 2/4] dt-bindings: media: Add bindings for raspberrypi,rp1-cfe
To: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>, linux-media@vger.kernel.org, 
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, 
	linux-rpi-kernel@lists.infradead.org, linux-arm-kernel@lists.infradead.org, 
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>, 
	Sakari Ailus <sakari.ailus@linux.intel.com>, Jacopo Mondi <jacopo.mondi@ideasonboard.com>, 
	Kieran Bingham <kieran.bingham@ideasonboard.com>, 
	Mauro Carvalho Chehab <mchehab@kernel.org>, 
	Raspberry Pi Kernel Maintenance <kernel-list@raspberrypi.com>, Rob Herring <robh+dt@kernel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, 
	Florian Fainelli <florian.fainelli@broadcom.com>, 
	Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com>
Content-Type: text/plain; charset="UTF-8"

On Tue, 19 Mar 2024 at 17:05, Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:
>
> On 19/03/2024 17:32, Naushir Patuck wrote:
> > On Tue, 19 Mar 2024 at 14:03, Tomi Valkeinen
> > <tomi.valkeinen@ideasonboard.com> wrote:
> >>
> >> On 19/03/2024 15:05, Naushir Patuck wrote:
> >>> On Tue, 19 Mar 2024 at 13:02, Krzysztof Kozlowski
> >>> <krzysztof.kozlowski@linaro.org> wrote:
> >>>>
> >>>> On 19/03/2024 13:57, Naushir Patuck wrote:
> >>>>>>>>
> >>>>>>>> See writing bindings. Compatibles should be SoC specific. In some cases
> >>>>>>>> generic fallbacks make sense, in some note. But don't just choose
> >>>>>>>> "generic fallback" because you want. Provide rationale.
> >>>>>>>
> >>>>>>> If the compatible is SoC specific, I suppose "raspberrypi,rp1-cfe"
> >>>>>>> would be the correct string.
> >>>>>>
> >>>>>> Sure, but then please think what if rp1 is on Rpi6, called exactly the
> >>>>>> same (rp1), with some minor differences? Could it be?
> >>>>>
> >>>>> Yes, this is definitely possible.  In such cases, I would expect the
> >>>>> hardware to have a version register that would be queried by the
> >>>>> driver to adjust for minor differences, and the compatible string
> >>>>> remains the same.  Does that seem reasonable?
> >>>>
> >>>> The "would expect" is concerning. The register(s) must be there already,
> >>>> with proper value.
> >>>>
> >>>
> >>> A version register already exists in the current hardware, so we will
> >>> update it to identify future hardware revisions.
> >>
> >> But that's a version register for the FE block, not for the whole
> >> module, right? Are you suggesting that you'll make sure the FE version
> >> will be changed every time anything in the bigger CFE block is changed,
> >> and thus the FE version would also reflect the whole CFE version?
> >
> > Yes, we will update the FE versioning when either CSI2 / FE blocks are updated.
> >
> >>
> >> Can there be versions without the FE block, with just the CSI-2 parts?
> >
> > There is no version register just in the CSI2 block in isolation, so
> > this is not possible.
>
> I meant could there be a future RPx with only the CSI-2 parts on it, no
> FE? In which case we would not have any register for the version. But
> then, that would be a rather big change, so a different compatible
> string would be in order.
>
> So, while it's not exactly a perfect version register, I think it will
> work fine, assuming the HW people will actually increase the version
> also for changes outside FE.
>
> >>
> >> Also, I'm still wondering about the RP1 part there in the compatible
> >> string. Is it necessary? The CFE is located in the RP1 co-processor, but
> >> is that relevant?
> >>
> >> Is there a versioning for the whole RP1 chip? Maybe it's going to the
> >> wrong direction if we use the board/SoC for this compatible name, as
> >> it's actually the RP1 where the CFE is located in, not the SoC.
> >>
> >
> > I don't really know the conversion required to answer this one.
> > Logically CFE is on RP1, so it makes sense to me to have "rp1" in the
> > string, but I will follow the judgment of the maintainers.
>
> Well, my thinking here was that if we have a register from which to read
> the version, and Raspberry Pi would create a new co-processor, RP2, with
> the same CFE. Would we then have "raspberrypi,rp1-cfe" and
> "raspberrypi,rp2-cfe", even if there are no changes? Or would a plain
> "raspberrypi,cfe" do for both?
>
> In other words, if we don't need the "rp1" for versioning purposes,
> should it then be dropped?

I agree with the above, you've convinced me that "raspberrypi,cfe"
might be the more appropriate string, or a convincing argument for
that to be a fallback string.

Naush

>
> On the other hand, maybe it is safer to just keep the "rp1" there anyway...
>
>   Tomi
>