Hi Conor, On Wed, Jul 12, 2023 at 8:49 PM Conor Dooley <conor.dooley@xxxxxxxxxxxxx> wrote: > > 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. > Yes, but I think the proper compatible string for hw pl2cache0 and hw pl2cache1 should be as follows: hw pl2cache0 -> compatible = "sifive,pl2cache0","sifive,pl2cache1"; hw pl2cache1 -> compatible = "sifive,pl2cache1"; Since the hw pl2cache0 implements more features than hw pl2cache1, it can be compatible with the pl2cache1 driver. However, hw pl2cache1 only implements a sub-feature of hw pl2cache0, so it cannot be compatible with the pl2cache0 driver. Thus, I'll keep only the compatible = "sifive,pl2cache1". in the driver and dt-binding. Thanks for the suggestions. > 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? > Yes, I think they refer to IP versions. OK, I'll fix it in v2. Thanks for the review. Best regards, Eric Lin > Thanks, > Conor.
