| Message ID | 5667267E.2080601@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/12/15 18:50, Dirk Behme wrote: > On 07.12.2015 20:03, Mark Rutland wrote: >> On Mon, Dec 07, 2015 at 06:49:43PM +0000, Sudeep Holla wrote: >>> >>> On 07/12/15 18:24, Geert Uytterhoeven wrote: >>>> + L2_CA57: cache-controller@0 { >>>> + compatible = "cache"; >>>> + arm,data-latency = <4 4 1>; >>>> + arm,tag-latency = <3 3 3>; >>> >>> Interesting, only PL2xx/3xx cache controller driver reads this from the >>> DT and configures the controller. The integrated L2 found in >>> A15/A7/A57/A53 needs doesn't make use of these values from the DT. >> >> These properties seem to be from l2cc.txt, which really only corresponds >> to PL210/PL220/PL310 (and variants) and isn't as generic as it sounds. >> >> I don't see that these are necessary at all. > > > What's about a documentation patch like [1], then? > > > For what is the arm64 dts entry > > cpu@0 { > ... > next-level-cache = <&L2_0>; > }; > > L2_0: l2-cache0 { > compatible = "cache"; > }; > > good for at all, then? > > > Best regards > > Dirk > > > [1] > > diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt > b/Documentation/devicetree/bindings/arm/l2cc.txt > index 06c88a4..f687aed 100644 > --- a/Documentation/devicetree/bindings/arm/l2cc.txt > +++ b/Documentation/devicetree/bindings/arm/l2cc.txt > @@ -1,12 +1,18 @@ > * ARM L2 Cache Controller > > -ARM cores often have a separate level 2 cache controller. There are > various > +ARM 32-bit cores often have a separate level 2 cache controller. There > are various > implementations of the L2 cache controller with compatible programming > models. > Some of the properties that are just prefixed "cache-*" are taken from > section > 3.7.3 of the ePAPR v1.1 specification which can be found at: > > https://www.power.org/wp-content/uploads/2012/06/Power_ePAPR_APPROVED_v1.1.pdf > > > -The ARM L2 cache representation in the device tree should be done as > follows: > +ARM 64-bit cores (e.g. A15/A7/A57/A53) have an integrated level 2 cache which A15 and A7 are 32-bit cores, so you can't generalize it. IMO you can just specify that this binding is applicable for separate L2 cache controllers.
On Tue, Dec 08, 2015 at 07:50:38PM +0100, Dirk Behme wrote: > On 07.12.2015 20:03, Mark Rutland wrote: > >On Mon, Dec 07, 2015 at 06:49:43PM +0000, Sudeep Holla wrote: > >> > >>On 07/12/15 18:24, Geert Uytterhoeven wrote: > >>>+ L2_CA57: cache-controller@0 { > >>>+ compatible = "cache"; > >>>+ arm,data-latency = <4 4 1>; > >>>+ arm,tag-latency = <3 3 3>; > >> > >>Interesting, only PL2xx/3xx cache controller driver reads this from the > >>DT and configures the controller. The integrated L2 found in > >>A15/A7/A57/A53 needs doesn't make use of these values from the DT. > > > >These properties seem to be from l2cc.txt, which really only corresponds > >to PL210/PL220/PL310 (and variants) and isn't as generic as it sounds. > > > >I don't see that these are necessary at all. > > > What's about a documentation patch like [1], then? I think it would be better to s/l2cc/l2x0/, and to make it clear that the document only applies to the variants listed above. If ePAPR doesn't cover the other cases, we should document those separately. > For what is the arm64 dts entry > > cpu@0 { > ... > next-level-cache = <&L2_0>; > }; > > L2_0: l2-cache0 { > compatible = "cache"; > }; > > good for at all, then? With the other properties from ePAPR you can acquire information on the geometry of the cache, which cannot be acquired from architected registers. Thanks, Mark. > [1] > > diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt > b/Documentation/devicetree/bindings/arm/l2cc.txt > index 06c88a4..f687aed 100644 > --- a/Documentation/devicetree/bindings/arm/l2cc.txt > +++ b/Documentation/devicetree/bindings/arm/l2cc.txt > @@ -1,12 +1,18 @@ > * ARM L2 Cache Controller > > -ARM cores often have a separate level 2 cache controller. There are > various > +ARM 32-bit cores often have a separate level 2 cache controller. > There are various > implementations of the L2 cache controller with compatible > programming models. > Some of the properties that are just prefixed "cache-*" are taken > from section > 3.7.3 of the ePAPR v1.1 specification which can be found at: > > https://www.power.org/wp-content/uploads/2012/06/Power_ePAPR_APPROVED_v1.1.pdf > > -The ARM L2 cache representation in the device tree should be done > as follows: > +ARM 64-bit cores (e.g. A15/A7/A57/A53) have an integrated level 2 > cache which > +doesn't make use of any values from the kernel device tree. There is no > +L2 cache configuration done in the kernel. The L2 cache is assumed to be > +preconfigured by early secure boot code. > + > +The ARM L2 cache representation for 32-bit cores in the device tree > should be done > +as follows: > > Required properties: > >
On 08.12.2015 20:16, Mark Rutland wrote: > On Tue, Dec 08, 2015 at 07:50:38PM +0100, Dirk Behme wrote: >> On 07.12.2015 20:03, Mark Rutland wrote: >>> On Mon, Dec 07, 2015 at 06:49:43PM +0000, Sudeep Holla wrote: >>>> >>>> On 07/12/15 18:24, Geert Uytterhoeven wrote: >>>>> + L2_CA57: cache-controller@0 { >>>>> + compatible = "cache"; >>>>> + arm,data-latency = <4 4 1>; >>>>> + arm,tag-latency = <3 3 3>; >>>> >>>> Interesting, only PL2xx/3xx cache controller driver reads this from the >>>> DT and configures the controller. The integrated L2 found in >>>> A15/A7/A57/A53 needs doesn't make use of these values from the DT. >>> >>> These properties seem to be from l2cc.txt, which really only corresponds >>> to PL210/PL220/PL310 (and variants) and isn't as generic as it sounds. >>> >>> I don't see that these are necessary at all. >> >> >> What's about a documentation patch like [1], then? > > I think it would be better to s/l2cc/l2x0/, and to make it clear that > the document only applies to the variants listed above. > > If ePAPR doesn't cover the other cases, we should document those > separately. Ok, thanks, I'll have a look to it. >> For what is the arm64 dts entry >> >> cpu@0 { >> ... >> next-level-cache = <&L2_0>; >> }; >> >> L2_0: l2-cache0 { >> compatible = "cache"; >> }; >> >> good for at all, then? > > With the other properties from ePAPR you can acquire information on the > geometry of the cache, which cannot be acquired from architected > registers. Just for my understanding: Yes, if other properties from ePAPR like geometry of the cache are added to the device tree l2 cache entries then it makes sense to have them. But an "empty" entry like the one given in the example above doesn't make much sense and could be removed without loosing any functionality? It looks to me that most of the L2 entries we have in arch/arm64/boot/dts are such "empty" entries. Is this understanding correct? Best regards Dirk
On 09/12/15 16:58, Dirk Behme wrote: > On 08.12.2015 20:16, Mark Rutland wrote: [...] >> >> With the other properties from ePAPR you can acquire information on the >> geometry of the cache, which cannot be acquired from architected >> registers. > > > Just for my understanding: Yes, if other properties from ePAPR like > geometry of the cache are added to the device tree l2 cache entries then > it makes sense to have them. > > But an "empty" entry like the one given in the example above doesn't > make much sense and could be removed without loosing any functionality? > No they are required to detect the cache hierarchy as there's no architectural way of detecting the same. > It looks to me that most of the L2 entries we have in > arch/arm64/boot/dts are such "empty" entries. > True *so far*, we have not seen a case where we need to override the values read in a architectural way.
On Wed, Dec 09, 2015 at 05:58:38PM +0100, Dirk Behme wrote: > >>For what is the arm64 dts entry > >> > >>cpu@0 { > >> ... > >> next-level-cache = <&L2_0>; > >>}; > >> > >>L2_0: l2-cache0 { > >> compatible = "cache"; > >>}; > >> > >>good for at all, then? > > > >With the other properties from ePAPR you can acquire information on the > >geometry of the cache, which cannot be acquired from architected > >registers. > > > Just for my understanding: Yes, if other properties from ePAPR like > geometry of the cache are added to the device tree l2 cache entries > then it makes sense to have them. > > But an "empty" entry like the one given in the example above doesn't > make much sense and could be removed without loosing any > functionality? > > It looks to me that most of the L2 entries we have in > arch/arm64/boot/dts are such "empty" entries. > > Is this understanding correct? You are mostly correct, just missing some history. It was previously (erroneously) assumed that the cache geometry could be derived from architected registers used for set/way maintenance. However, we knew that this did not describe how those caches were linked to each other (e.g. which CPU shared with level x cache). So the description in DT was required to provide that. Now, we all know that the geometry is necessary too. Those DTs should be fixed. Sudeep, do you know what's happening on that front? Thanks, Mark.
On 09/12/15 17:21, Mark Rutland wrote: > On Wed, Dec 09, 2015 at 05:58:38PM +0100, Dirk Behme wrote: >>>> For what is the arm64 dts entry >>>> >>>> cpu@0 { >>>> ... >>>> next-level-cache = <&L2_0>; >>>> }; >>>> >>>> L2_0: l2-cache0 { >>>> compatible = "cache"; >>>> }; >>>> >>>> good for at all, then? >>> >>> With the other properties from ePAPR you can acquire information on the >>> geometry of the cache, which cannot be acquired from architected >>> registers. >> >> >> Just for my understanding: Yes, if other properties from ePAPR like >> geometry of the cache are added to the device tree l2 cache entries >> then it makes sense to have them. >> >> But an "empty" entry like the one given in the example above doesn't >> make much sense and could be removed without loosing any >> functionality? >> >> It looks to me that most of the L2 entries we have in >> arch/arm64/boot/dts are such "empty" entries. >> >> Is this understanding correct? > > You are mostly correct, just missing some history. > > It was previously (erroneously) assumed that the cache geometry could be > derived from architected registers used for set/way maintenance. > However, we knew that this did not describe how those caches were linked > to each other (e.g. which CPU shared with level x cache). So the > description in DT was required to provide that. > > Now, we all know that the geometry is necessary too. Those DTs should be > fixed. > > Sudeep, do you know what's happening on that front? > No updates yet. I thought Alex Van Brunt would pick that. I have a patch for PPC which never got tested/reviewed. It moves PPC code to reuse the generic cacheinfo. If I can revive that and look into ways of moving it to core code.
diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt b/Documentation/devicetree/bindings/arm/l2cc.txt index 06c88a4..f687aed 100644 --- a/Documentation/devicetree/bindings/arm/l2cc.txt +++ b/Documentation/devicetree/bindings/arm/l2cc.txt @@ -1,12 +1,18 @@ * ARM L2 Cache Controller -ARM cores often have a separate level 2 cache controller. There are various +ARM 32-bit cores often have a separate level 2 cache controller. There are various implementations of the L2 cache controller with compatible programming models. Some of the properties that are just prefixed "cache-*" are taken from section 3.7.3 of the ePAPR v1.1 specification which can be found at: https://www.power.org/wp-content/uploads/2012/06/Power_ePAPR_APPROVED_v1.1.pdf -The ARM L2 cache representation in the device tree should be done as follows: +ARM 64-bit cores (e.g. A15/A7/A57/A53) have an integrated level 2 cache which +doesn't make use of any values from the kernel device tree. There is no +L2 cache configuration done in the kernel. The L2 cache is assumed to be