Re: [PATCH 3/3] dt-bindings: riscv: sifive: Add SiFive Private L2 cache controller

[Conor Dooley <conor.dooley@microchip.com>]

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

On Wed, Jul 12, 2023 at 02:30:06PM +0200, Krzysztof Kozlowski wrote:
> On 12/07/2023 13:09, Eric Lin wrote:
> > On Sat, Jul 01, 2023 at 10:22:25AM +0200, Krzysztof Kozlowski wrote:
> >> On 28/06/2023 18:31, Eric Lin wrote:
> >>
> >>>>>>
> >>>>>>> +      - enum:
> >>>>>>> +          - sifive,pL2Cache0
> >>>>>>> +          - sifive,pL2Cache1
> >>>>>>
> >>>>>> What is "0" and "1" here? What do these compatibles represent? Why they
> >>>>>> do not have any SoC related part?
> >>>>>
> >>>>> The pL2Cache1 has minor changes in hardware, but it can use the same
> >>>>> pl2 cache driver.
> >>>>
> >>>> Then why aren't they compatible?
> >>>>
> >>>
> >>> The pL2Cache1 has removed some unused bits in the register compared to
> >>> pl2Cache0.
> >>> From the hardware perspective, they are not compatible but they can
> >>> share the same pl2 cache driver in software.
> >>
> >> So they are compatible... If they were not compatible, you wouldn't be
> >> able to use the same match in the driver.
> >>
> >>> Thus, we would like to keep both. It would be great if you can provide
> >>> some suggestions. Thanks.
> >>
> >> I propose to make them compatible, like every other piece of SoC. I
> >> don't see any benefit of having them separate.
> >>
> > Sorry for the late reply.
> > The pl2 cache is our internal platform IP and is not part of any SoC. 
> > 
> > The reason why this driver is compatible with the hardware "pl2cache0" and hardware "pl2cache1"
> > is that it doesn't program the different parts of the config register
> > However, our internal software (e.g., bare-metal software) will program these different parts,
> > so it needs to rely on the different compatible string to identify the hardware.
> >   
> > Additionally, we would like the compatible strings to reflect which hardware is being used Thanks.
> 
> I don't understand how does it contradicts anything I said. So you do
> agree with me? Or what?

I probably should've been keeping a closer eye here, sorry.

I assume what Krzysztof means is why do you permit both
"sifive,pL2Cache0" and "sifive,pL2Cache1" appearing in isolation. IOW,
both of
compatible = "sifive,pl2cache0";
and
compatible = "sifive,pl2cache1";
are valid in your binding.

The hardware for both might be different, and their full featuresets may
be incompatible, but they implement a compatible subset of features. I
would expect that the following would be the permitted compatible setups:
compatible = "sifive,pl2cache0";
and
compatible = "sifive,pl2cache1", "sifive,pl2cache0";

A consumer of the DT that does care for the differences should be
looking for the specific compatible, and OS code that does not care can
always bind to the "0" version.

Do the "0" & "1" here refer to the IP version, as in
sifive-blocks-ip-versioning.txt? I didn't think the compatibles
containing those IP versions were supposed to appear in isolation,
without a soc-specific one?

Thanks,
Conor.

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