| Message ID | 1466066418-1141-1-git-send-email-jszhang@marvell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 16.06.2016 10:40, Jisheng Zhang wrote: > This patch adds the L2 cache topology for berlin4ct which has 1MB L2 > cache. > > Signed-off-by: Jisheng Zhang <jszhang@marvell.com> > --- > arch/arm64/boot/dts/marvell/berlin4ct.dtsi | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/arch/arm64/boot/dts/marvell/berlin4ct.dtsi b/arch/arm64/boot/dts/marvell/berlin4ct.dtsi > index 099ad93..c9e3a98 100644 > --- a/arch/arm64/boot/dts/marvell/berlin4ct.dtsi > +++ b/arch/arm64/boot/dts/marvell/berlin4ct.dtsi [...] > @@ -92,9 +95,14 @@ > device_type = "cpu"; > reg = <0x3>; > enable-method = "psci"; > + next-level-cache = <&L2_0>; > cpu-idle-states = <&CPU_SLEEP_0>; > }; > > + L2_0: l2-cache0 { Jisheng, The node name should just have a generic name that reflects the purpose of the unit it represents, i.e. s/l2-cache0/cache/ nits: - What is that "0" for? Please remove if there is no good reason. - Does the node label need to be upper-case? Please make it lower case. Sebastian > + compatible = "cache"; > + }; > + > idle-states { > entry-method = "psci"; > CPU_SLEEP_0: cpu-sleep-0 { >
Dear Sebastian, On Wed, 6 Jul 2016 19:49:01 +0200 Sebastian Hesselbarth wrote: > On 16.06.2016 10:40, Jisheng Zhang wrote: > > This patch adds the L2 cache topology for berlin4ct which has 1MB L2 > > cache. > > > > Signed-off-by: Jisheng Zhang <jszhang@marvell.com> > > --- > > arch/arm64/boot/dts/marvell/berlin4ct.dtsi | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/marvell/berlin4ct.dtsi b/arch/arm64/boot/dts/marvell/berlin4ct.dtsi > > index 099ad93..c9e3a98 100644 > > --- a/arch/arm64/boot/dts/marvell/berlin4ct.dtsi > > +++ b/arch/arm64/boot/dts/marvell/berlin4ct.dtsi > [...] > > @@ -92,9 +95,14 @@ > > device_type = "cpu"; > > reg = <0x3>; > > enable-method = "psci"; > > + next-level-cache = <&L2_0>; > > cpu-idle-states = <&CPU_SLEEP_0>; > > }; > > > > + L2_0: l2-cache0 { > > Jisheng, > > The node name should just have a generic name that reflects > the purpose of the unit it represents, i.e. > s/l2-cache0/cache/ IMHO, "cache" is too generic, this is L2 cache topology, so in v2, I use "l2-cache" instead. what do you think? PS: I found other arm64 SoCs also use "l2-cache" as the node name. > > nits: > - What is that "0" for? Please remove if there is no good reason. > - Does the node label need to be upper-case? Please make it lower case. > oh yeah, thanks for the hints! will do in v2. Thanks for reviewing, Jisheng > Sebastian > > > + compatible = "cache"; > > + }; > > + > > idle-states { > > entry-method = "psci"; > > CPU_SLEEP_0: cpu-sleep-0 { > > >
On 07.07.2016 07:48, Jisheng Zhang wrote: > On Wed, 6 Jul 2016 19:49:01 +0200 Sebastian Hesselbarth wrote: >> On 16.06.2016 10:40, Jisheng Zhang wrote: >>> This patch adds the L2 cache topology for berlin4ct which has 1MB L2 >>> cache. >>> >>> Signed-off-by: Jisheng Zhang <jszhang@marvell.com> >>> --- >>> arch/arm64/boot/dts/marvell/berlin4ct.dtsi | 8 ++++++++ >>> 1 file changed, 8 insertions(+) >>> >>> diff --git a/arch/arm64/boot/dts/marvell/berlin4ct.dtsi b/arch/arm64/boot/dts/marvell/berlin4ct.dtsi >>> index 099ad93..c9e3a98 100644 >>> --- a/arch/arm64/boot/dts/marvell/berlin4ct.dtsi >>> +++ b/arch/arm64/boot/dts/marvell/berlin4ct.dtsi >> [...] >>> @@ -92,9 +95,14 @@ >>> device_type = "cpu"; >>> reg = <0x3>; >>> enable-method = "psci"; >>> + next-level-cache = <&L2_0>; >>> cpu-idle-states = <&CPU_SLEEP_0>; >>> }; >>> >>> + L2_0: l2-cache0 { >> >> The node name should just have a generic name that reflects >> the purpose of the unit it represents, i.e. >> s/l2-cache0/cache/ > > IMHO, "cache" is too generic, this is L2 cache topology, so in v2, I use > "l2-cache" instead. what do you think? > > PS: I found other arm64 SoCs also use "l2-cache" as the node name. Yeah, I realized that too. Anyway, the node name should be as generic as possible. Moreover, the more specific compatible string below also is "cache", too. So I see no reason why the node name should be more specific than the compatible. >>> + compatible = "cache"; >>> + }; If you want to have the cache-level represented in the node, I guess you can use cache-level property. However, I cannot find any cache related binding documentation other than for arm(32) and powerpc that mentions cache-level property. If you are fine with it, I can pick up the v2 you sent earlier, rename the node to "cache" only, and add a cache-level = <2>; property while applying. Sebastian >>> idle-states { >>> entry-method = "psci"; >>> CPU_SLEEP_0: cpu-sleep-0 { >>> >> >
Dear Sebastian, On Thu, 7 Jul 2016 19:10:26 +0200 Sebastian Hesselbarth wrote: > On 07.07.2016 07:48, Jisheng Zhang wrote: > > On Wed, 6 Jul 2016 19:49:01 +0200 Sebastian Hesselbarth wrote: > >> On 16.06.2016 10:40, Jisheng Zhang wrote: > >>> This patch adds the L2 cache topology for berlin4ct which has 1MB L2 > >>> cache. > >>> > >>> Signed-off-by: Jisheng Zhang <jszhang@marvell.com> > >>> --- > >>> arch/arm64/boot/dts/marvell/berlin4ct.dtsi | 8 ++++++++ > >>> 1 file changed, 8 insertions(+) > >>> > >>> diff --git a/arch/arm64/boot/dts/marvell/berlin4ct.dtsi b/arch/arm64/boot/dts/marvell/berlin4ct.dtsi > >>> index 099ad93..c9e3a98 100644 > >>> --- a/arch/arm64/boot/dts/marvell/berlin4ct.dtsi > >>> +++ b/arch/arm64/boot/dts/marvell/berlin4ct.dtsi > >> [...] > >>> @@ -92,9 +95,14 @@ > >>> device_type = "cpu"; > >>> reg = <0x3>; > >>> enable-method = "psci"; > >>> + next-level-cache = <&L2_0>; > >>> cpu-idle-states = <&CPU_SLEEP_0>; > >>> }; > >>> > >>> + L2_0: l2-cache0 { > >> > >> The node name should just have a generic name that reflects > >> the purpose of the unit it represents, i.e. > >> s/l2-cache0/cache/ > > > > IMHO, "cache" is too generic, this is L2 cache topology, so in v2, I use > > "l2-cache" instead. what do you think? > > > > PS: I found other arm64 SoCs also use "l2-cache" as the node name. > > Yeah, I realized that too. Anyway, the node name should be as generic > as possible. Moreover, the more specific compatible string below also > is "cache", too. So I see no reason why the node name should be more > specific than the compatible. make sense, I agree with you now. > > >>> + compatible = "cache"; > >>> + }; > > If you want to have the cache-level represented in the node, I guess > you can use cache-level property. However, I cannot find any cache > related binding documentation other than for arm(32) and powerpc that > mentions cache-level property. > > If you are fine with it, I can pick up the v2 you sent earlier, rename > the node to "cache" only, and add a cache-level = <2>; property while > applying. Per my understanding of the code, drivers/base/cacheinfo.c and related source code, the "cache-level" property isn't used at all, so could you please only rename the node to "cache"? Thanks in advance, Jisheng
diff --git a/arch/arm64/boot/dts/marvell/berlin4ct.dtsi b/arch/arm64/boot/dts/marvell/berlin4ct.dtsi index 099ad93..c9e3a98 100644 --- a/arch/arm64/boot/dts/marvell/berlin4ct.dtsi +++ b/arch/arm64/boot/dts/marvell/berlin4ct.dtsi @@ -68,6 +68,7 @@ device_type = "cpu"; reg = <0x0>; enable-method = "psci"; + next-level-cache = <&L2_0>; cpu-idle-states = <&CPU_SLEEP_0>; }; @@ -76,6 +77,7 @@ device_type = "cpu"; reg = <0x1>; enable-method = "psci"; + next-level-cache = <&L2_0>; cpu-idle-states = <&CPU_SLEEP_0>; }; @@ -84,6 +86,7 @@ device_type = "cpu"; reg = <0x2>; enable-method = "psci"; + next-level-cache = <&L2_0>; cpu-idle-states = <&CPU_SLEEP_0>; }; @@ -92,9 +95,14 @@ device_type = "cpu"; reg = <0x3>; enable-method = "psci"; + next-level-cache = <&L2_0>; cpu-idle-states = <&CPU_SLEEP_0>; }; + L2_0: l2-cache0 { + compatible = "cache"; + }; + idle-states { entry-method = "psci"; CPU_SLEEP_0: cpu-sleep-0 {
This patch adds the L2 cache topology for berlin4ct which has 1MB L2 cache. Signed-off-by: Jisheng Zhang <jszhang@marvell.com> --- arch/arm64/boot/dts/marvell/berlin4ct.dtsi | 8 ++++++++ 1 file changed, 8 insertions(+)