| Message ID | 1391093744-19905-2-git-send-email-patrice.chotard@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thursday 30 January 2014, Patrice CHOTARD wrote: > From: Alexandre TORGUE <alexandre.torgue@st.com> > > This patch adds support to STiD127 SoC. > The main adaptation is the L2 cache way size compare to STiH41x SoCs. > > Signed-off-by: alexandre torgue <alexandre.torgue@st.com> > Signed-off-by: Patrice Chotard <patrice.chotard@st.com> > --- > arch/arm/mach-sti/board-dt.c | 6 ++++++ > 1 file changed, 6 insertions(+) Wouldn't it be better to read this value from the l2 cache controller node? I'd assume there might be more SoCs that will need a similar change, so it's better to come up with a solution that doesn't involve changing the kernel every time. Arnd
On Thursday 30 January 2014, Arnd Bergmann wrote: > On Thursday 30 January 2014, Patrice CHOTARD wrote: > > From: Alexandre TORGUE <alexandre.torgue@st.com> > > > > This patch adds support to STiD127 SoC. > > The main adaptation is the L2 cache way size compare to STiH41x SoCs. > > > > Signed-off-by: alexandre torgue <alexandre.torgue@st.com> > > Signed-off-by: Patrice Chotard <patrice.chotard@st.com> > > --- > > arch/arm/mach-sti/board-dt.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > Wouldn't it be better to read this value from the l2 cache > controller node? I'd assume there might be more SoCs that > will need a similar change, so it's better to come up with > a solution that doesn't involve changing the kernel every > time. Actually reading the code in this file shows that the L2 cache initialization is the only nonstandard thing in there. We should really find a way to get rid of the entire function. Sorry if I missed the initial review, but can you explain why this is needed to start with? Arnd
Hi Arnd, On 30/01/14 18:39, Arnd Bergmann wrote: > Actually reading the code in this file shows that the L2 cache > initialization is the only nonstandard thing in there. We should > really find a way to get rid of the entire function. I think this will get rid of lot of code left in board-dt. > > Sorry if I missed the initial review, but can you explain > why this is needed to start with? On ST SoCs the default value for L2 AUX_CTRL register is 0x0, so we set the way-size explicit here. Thanks, srini
On Friday 31 January 2014, srinivas kandagatla wrote: > > Sorry if I missed the initial review, but can you explain > > why this is needed to start with? > > On ST SoCs the default value for L2 AUX_CTRL register is 0x0, so we set > the way-size explicit here. Unfortunately, we keep going back and forth on the L2 cache controller setup between "it should work automatically" and "we don't want to have configuration data in DT", where my personal opinion is that the first one is more important here. Now, there are a couple of properties that are defined in Documentation/devicetree/bindings/arm/l2cc.txt to let some of the things get set up automatically already. Can you check which bits are missing there, if any? Are they better described as "configuration" or "hardware" settings? Arnd
On 01/31/2014 09:15 PM, Arnd Bergmann wrote: > On Friday 31 January 2014, srinivas kandagatla wrote: > >>> Sorry if I missed the initial review, but can you explain >>> why this is needed to start with? >> On ST SoCs the default value for L2 AUX_CTRL register is 0x0, so we set >> the way-size explicit here. > Unfortunately, we keep going back and forth on the L2 cache controller > setup between "it should work automatically" and "we don't want to > have configuration data in DT", where my personal opinion is that > the first one is more important here. > > Now, there are a couple of properties that are defined in > Documentation/devicetree/bindings/arm/l2cc.txt to let some of the > things get set up automatically already. Can you check which bits > are missing there, if any? Are they better described as "configuration" > or "hardware" settings? Hi Arnd, Thanks for remarks. I will a have a look on it, but unfortunately not before 2 weeks. Alex. > > Arnd
Hi Arnd, On 31/01/14 20:15, Arnd Bergmann wrote: > On Friday 31 January 2014, srinivas kandagatla wrote: > >>> Sorry if I missed the initial review, but can you explain >>> why this is needed to start with? >> >> On ST SoCs the default value for L2 AUX_CTRL register is 0x0, so we set >> the way-size explicit here. > > Unfortunately, we keep going back and forth on the L2 cache controller > setup between "it should work automatically" and "we don't want to > have configuration data in DT", where my personal opinion is that > the first one is more important here. > > Now, there are a couple of properties that are defined in > Documentation/devicetree/bindings/arm/l2cc.txt to let some of the > things get set up automatically already. Can you check which bits > are missing there, if any? Are they better described as "configuration" > or "hardware" settings? Currently l2cc bindings has few optional properties like. - arm,data-latency - arm,tag-latency - arm,dirty-latency - arm,filter-ranges - interrupts : - cache-id-part: - wt-override: These does not include properties to set "way-size", "associativity", "enabling prefetching", "Prefetch drop enable", "prefetch offset", "Double linefill" and few more in prefect control register and aux-control register. This is not just a issue with STi SOCs, having a quick look, I can see that few more SOCs have similar requirements to set these properties. We could do two things to get l2 setup automatically on STi SOCS. 1> Either define these properties case-by-case basic, which might be useful for other SOCs too. 2> Or Add new compatible string for STi SoCs so that they can automatically setup these values in cache-l2x0.c Am Ok with either approaches. Thanks, srini > > Arnd > >
On Wednesday 05 February 2014, srinivas kandagatla wrote: > Currently l2cc bindings has few optional properties like. > > - arm,data-latency > - arm,tag-latency > - arm,dirty-latency > - arm,filter-ranges > - interrupts : > - cache-id-part: > - wt-override: > > These does not include properties to set "way-size", "associativity", > "enabling prefetching", "Prefetch drop enable", "prefetch offset", > "Double linefill" and few more in prefect control register and > aux-control register. > > This is not just a issue with STi SOCs, having a quick look, I can see > that few more SOCs have similar requirements to set these properties. > > We could do two things to get l2 setup automatically on STi SOCS. > > 1> Either define these properties case-by-case basic, which might be > useful for other SOCs too. > > 2> Or Add new compatible string for STi SoCs so that they can > automatically setup these values in cache-l2x0.c > > Am Ok with either approaches. > I suggested 1 in the past, but the objection that I saw (can't find the email at the moment) was that the additional settings are "configuration" rather than "hardware properties". What I'd really need to know from you is which of properties you listed as missing above are actually needed for your platform, and whether they can be classified as hardware specific or just configuration. Arnd
On 06/02/14 16:46, Arnd Bergmann wrote: > On Wednesday 05 February 2014, srinivas kandagatla wrote: >> Currently l2cc bindings has few optional properties like. >> >> - arm,data-latency >> - arm,tag-latency >> - arm,dirty-latency >> - arm,filter-ranges >> - interrupts : >> - cache-id-part: >> - wt-override: >> >> These does not include properties to set "way-size", "associativity", >> "enabling prefetching", "Prefetch drop enable", "prefetch offset", >> "Double linefill" and few more in prefect control register and >> aux-control register. >> >> This is not just a issue with STi SOCs, having a quick look, I can see >> that few more SOCs have similar requirements to set these properties. >> >> We could do two things to get l2 setup automatically on STi SOCS. >> >> 1> Either define these properties case-by-case basic, which might be >> useful for other SOCs too. >> >> 2> Or Add new compatible string for STi SoCs so that they can >> automatically setup these values in cache-l2x0.c >> >> Am Ok with either approaches. >> > > I suggested 1 in the past, but the objection that I saw (can't > find the email at the moment) was that the additional settings > are "configuration" rather than "hardware properties". What I'd > really need to know from you is which of properties you listed > as missing above are actually needed for your platform, and whether > they can be classified as hardware specific or just configuration. On STi Platforms we need below properties to got for option 1. arm,way-size; arm,instruction-prefetch-enable; arm,data-prefetch-enable; we also want a property or a way to set "Shareable attribute Override Enable" bit in the Auxiliary Control Register, bit[22]. Thanks, srini > > Arnd > >
Hi Patrice, Could you add an overview documentation as it has been done for other STi platforms? See Documentation/arm/sti/stih416-overview.txt Thanks, Maxime On 01/30/2014 03:55 PM, Patrice CHOTARD wrote: > From: Alexandre TORGUE <alexandre.torgue@st.com> > > This patch adds support to STiD127 SoC. > The main adaptation is the L2 cache way size compare to STiH41x SoCs. > > Signed-off-by: alexandre torgue <alexandre.torgue@st.com> > Signed-off-by: Patrice Chotard <patrice.chotard@st.com> > --- > arch/arm/mach-sti/board-dt.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/arch/arm/mach-sti/board-dt.c b/arch/arm/mach-sti/board-dt.c > index 1217fb5..be018a9 100644 > --- a/arch/arm/mach-sti/board-dt.c > +++ b/arch/arm/mach-sti/board-dt.c > @@ -9,6 +9,7 @@ > > #include <linux/irq.h> > #include <linux/of_platform.h> > +#include <linux/of.h> > #include <asm/hardware/cache-l2x0.h> > #include <asm/mach/arch.h> > > @@ -18,6 +19,10 @@ void __init stih41x_l2x0_init(void) > { > u32 way_size = 0x4; > u32 aux_ctrl; > + > + if (of_machine_is_compatible("st,stid127")) > + way_size = 0x3; > + > /* may be this can be encoded in macros like BIT*() */ > aux_ctrl = (0x1 << L2X0_AUX_CTRL_SHARE_OVERRIDE_SHIFT) | > (0x1 << L2X0_AUX_CTRL_DATA_PREFETCH_SHIFT) | > @@ -36,6 +41,7 @@ static void __init stih41x_machine_init(void) > static const char *stih41x_dt_match[] __initdata = { > "st,stih415", > "st,stih416", > + "st,stid127", > NULL > }; > >
Hi Maxime Thanks for reviewing. Yes for sure, i will add additional board informations. Patrice On 02/27/2014 01:23 PM, Maxime Coquelin wrote: > Hi Patrice, > > Could you add an overview documentation as it has been done for other > STi platforms? > > See Documentation/arm/sti/stih416-overview.txt > > Thanks, > Maxime > > On 01/30/2014 03:55 PM, Patrice CHOTARD wrote: >> From: Alexandre TORGUE <alexandre.torgue@st.com> >> >> This patch adds support to STiD127 SoC. >> The main adaptation is the L2 cache way size compare to STiH41x SoCs. >> >> Signed-off-by: alexandre torgue <alexandre.torgue@st.com> >> Signed-off-by: Patrice Chotard <patrice.chotard@st.com> >> --- >> arch/arm/mach-sti/board-dt.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/arch/arm/mach-sti/board-dt.c b/arch/arm/mach-sti/board-dt.c >> index 1217fb5..be018a9 100644 >> --- a/arch/arm/mach-sti/board-dt.c >> +++ b/arch/arm/mach-sti/board-dt.c >> @@ -9,6 +9,7 @@ >> >> #include <linux/irq.h> >> #include <linux/of_platform.h> >> +#include <linux/of.h> >> #include <asm/hardware/cache-l2x0.h> >> #include <asm/mach/arch.h> >> >> @@ -18,6 +19,10 @@ void __init stih41x_l2x0_init(void) >> { >> u32 way_size = 0x4; >> u32 aux_ctrl; >> + >> + if (of_machine_is_compatible("st,stid127")) >> + way_size = 0x3; >> + >> /* may be this can be encoded in macros like BIT*() */ >> aux_ctrl = (0x1 << L2X0_AUX_CTRL_SHARE_OVERRIDE_SHIFT) | >> (0x1 << L2X0_AUX_CTRL_DATA_PREFETCH_SHIFT) | >> @@ -36,6 +41,7 @@ static void __init stih41x_machine_init(void) >> static const char *stih41x_dt_match[] __initdata = { >> "st,stih415", >> "st,stih416", >> + "st,stid127", >> NULL >> }; >> >>
diff --git a/arch/arm/mach-sti/board-dt.c b/arch/arm/mach-sti/board-dt.c index 1217fb5..be018a9 100644 --- a/arch/arm/mach-sti/board-dt.c +++ b/arch/arm/mach-sti/board-dt.c @@ -9,6 +9,7 @@ #include <linux/irq.h> #include <linux/of_platform.h> +#include <linux/of.h> #include <asm/hardware/cache-l2x0.h> #include <asm/mach/arch.h> @@ -18,6 +19,10 @@ void __init stih41x_l2x0_init(void) { u32 way_size = 0x4; u32 aux_ctrl; + + if (of_machine_is_compatible("st,stid127")) + way_size = 0x3; + /* may be this can be encoded in macros like BIT*() */ aux_ctrl = (0x1 << L2X0_AUX_CTRL_SHARE_OVERRIDE_SHIFT) | (0x1 << L2X0_AUX_CTRL_DATA_PREFETCH_SHIFT) | @@ -36,6 +41,7 @@ static void __init stih41x_machine_init(void) static const char *stih41x_dt_match[] __initdata = { "st,stih415", "st,stih416", + "st,stid127", NULL };