| Message ID | 1420220628-23742-3-git-send-email-nm@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Nishanth, 2015-01-03 2:43 GMT+09:00 Nishanth Menon <nm@ti.com>: > AM437x generation of processors support programming the PL310 L2Cache > controller's address filter start and end registers using a secure > montior service. typo: s/montior/monitor/ [snip] > + base = omap4_get_l2cache_base(); > + filter_start = (reg == L310_ADDR_FILTER_START) ? val : > + readl_relaxed(base + L310_ADDR_FILTER_START); > + filter_end = (reg == L310_ADDR_FILTER_END) ? val : > + readl_relaxed(base + L310_ADDR_FILTER_END); > + omap_smc1_2(AM43X_MON_L2X0_SETFILTER_INDEX, filter_start, > + filter_end); > + return; I don't have any significant comments about this patch in particular, but just noticed that you need to do read-backs here (and the typo thanks to the spell checker of my mailing app). Maybe you should consider switching to the .configure() API I introduced in my series? This would let you get rid of the hardcoded static mapping. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 15:40-20150103, Tomasz Figa wrote: > Hi Nishanth, > > 2015-01-03 2:43 GMT+09:00 Nishanth Menon <nm@ti.com>: > > AM437x generation of processors support programming the PL310 L2Cache > > controller's address filter start and end registers using a secure > > montior service. > > typo: s/montior/monitor/ > > [snip] Uggh.. yes indeed. I will post a v3 updating the comments. If the following is ok. > > > + base = omap4_get_l2cache_base(); > > + filter_start = (reg == L310_ADDR_FILTER_START) ? val : > > + readl_relaxed(base + L310_ADDR_FILTER_START); > > + filter_end = (reg == L310_ADDR_FILTER_END) ? val : > > + readl_relaxed(base + L310_ADDR_FILTER_END); > > + omap_smc1_2(AM43X_MON_L2X0_SETFILTER_INDEX, filter_start, > > + filter_end); > > + return; > > I don't have any significant comments about this patch in particular, > but just noticed that you need to do read-backs here (and the typo > thanks to the spell checker of my mailing app). Maybe you should > consider switching to the .configure() API I introduced in my series? > This would let you get rid of the hardcoded static mapping. Yeah, I have two choices there.. Either I provide the fundamental write function for the generic l2c code to use OR I provide a duplicate of resultant l2c_configure(aux write) + l2c310_configure. To allow for reuse of improvements or anything like errata implementations in the future, OMAP L2C implementation has chosen to provide the low level code and allow the higherlevel configure/write/whatever of the future to stay in arch/arm/mm/cache-l2x0.c. The write_sec operation is not too complicated enough to warrant a replication of l2c310_configure. So, I prefer the current implementation than providing a .configure handler for outer_cache.configure from SoC level. Let me know if anyone has a strong objection to this.
2015-01-04 0:34 GMT+09:00 Nishanth Menon <nm@ti.com>: > On 15:40-20150103, Tomasz Figa wrote: >> Hi Nishanth, >> >> 2015-01-03 2:43 GMT+09:00 Nishanth Menon <nm@ti.com>: >> > AM437x generation of processors support programming the PL310 L2Cache >> > controller's address filter start and end registers using a secure >> > montior service. >> >> typo: s/montior/monitor/ >> >> [snip] > > Uggh.. yes indeed. I will post a v3 updating the comments. If the > following is ok. >> >> > + base = omap4_get_l2cache_base(); >> > + filter_start = (reg == L310_ADDR_FILTER_START) ? val : >> > + readl_relaxed(base + L310_ADDR_FILTER_START); >> > + filter_end = (reg == L310_ADDR_FILTER_END) ? val : >> > + readl_relaxed(base + L310_ADDR_FILTER_END); >> > + omap_smc1_2(AM43X_MON_L2X0_SETFILTER_INDEX, filter_start, >> > + filter_end); >> > + return; >> >> I don't have any significant comments about this patch in particular, >> but just noticed that you need to do read-backs here (and the typo >> thanks to the spell checker of my mailing app). Maybe you should >> consider switching to the .configure() API I introduced in my series? >> This would let you get rid of the hardcoded static mapping. > > Yeah, I have two choices there.. Either I provide the fundamental > write function for the generic l2c code to use OR I provide a > duplicate of resultant l2c_configure(aux write) + l2c310_configure. > > To allow for reuse of improvements or anything like errata > implementations in the future, OMAP L2C implementation has chosen to provide the > low level code and allow the higherlevel configure/write/whatever of the > future to stay in arch/arm/mm/cache-l2x0.c. The write_sec operation is > not too complicated enough to warrant a replication of l2c310_configure. > > So, I prefer the current implementation than providing a .configure > handler for outer_cache.configure from SoC level. > > Let me know if anyone has a strong objection to this. Well, what l2c310_configure() does after my series is just writing the registers. If they cannot be written normally (without some tricks such as reading back other registers) then IMHO a separate function should be provided. This is becomes possible after patch 3/8 (ARM: l2c: Add interface to ask hypervisor to configure L2C) and what is used on Exynos which also updates multiple registers in single SMC calls. You can find an example of use in patch 6/8 (ARM: EXYNOS: Add .write_sec outer cache callback for L2C-310). What's even more interesting is that approaches similar to the one currently used on OMAP had been NAKed, when proposed for Exynos and this is why we have the solution proposed by my patches. Note that .write_sec() callback is still used for L2X0_CTRL and L2X0_DEBUG_CTRL registers, because there might be a need to write them separately (e.g. to disable the controller and to perform debug operations/workarounds when the controller is already enabled). Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/03/2015 10:16 AM, Tomasz Figa wrote: > 2015-01-04 0:34 GMT+09:00 Nishanth Menon <nm@ti.com>: >> On 15:40-20150103, Tomasz Figa wrote: >>> Hi Nishanth, >>> >>> 2015-01-03 2:43 GMT+09:00 Nishanth Menon <nm@ti.com>: >>>> AM437x generation of processors support programming the PL310 L2Cache >>>> controller's address filter start and end registers using a secure >>>> montior service. >>> >>> typo: s/montior/monitor/ >>> >>> [snip] >> >> Uggh.. yes indeed. I will post a v3 updating the comments. If the >> following is ok. >>> >>>> + base = omap4_get_l2cache_base(); >>>> + filter_start = (reg == L310_ADDR_FILTER_START) ? val : >>>> + readl_relaxed(base + L310_ADDR_FILTER_START); >>>> + filter_end = (reg == L310_ADDR_FILTER_END) ? val : >>>> + readl_relaxed(base + L310_ADDR_FILTER_END); >>>> + omap_smc1_2(AM43X_MON_L2X0_SETFILTER_INDEX, filter_start, >>>> + filter_end); >>>> + return; >>> >>> I don't have any significant comments about this patch in particular, >>> but just noticed that you need to do read-backs here (and the typo >>> thanks to the spell checker of my mailing app). Maybe you should >>> consider switching to the .configure() API I introduced in my series? >>> This would let you get rid of the hardcoded static mapping. >> >> Yeah, I have two choices there.. Either I provide the fundamental >> write function for the generic l2c code to use OR I provide a >> duplicate of resultant l2c_configure(aux write) + l2c310_configure. >> >> To allow for reuse of improvements or anything like errata >> implementations in the future, OMAP L2C implementation has chosen to provide the >> low level code and allow the higherlevel configure/write/whatever of the >> future to stay in arch/arm/mm/cache-l2x0.c. The write_sec operation is >> not too complicated enough to warrant a replication of l2c310_configure. >> >> So, I prefer the current implementation than providing a .configure >> handler for outer_cache.configure from SoC level. >> >> Let me know if anyone has a strong objection to this. > > Well, what l2c310_configure() does after my series is just writing the > registers. If they cannot be written normally (without some tricks > such as reading back other registers) then IMHO a separate function > should be provided. > > This is becomes possible after patch 3/8 (ARM: l2c: Add interface to > ask hypervisor to configure L2C) and what is used on Exynos which also > updates multiple registers in single SMC calls. You can find an > example of use in patch 6/8 (ARM: EXYNOS: Add .write_sec outer cache > callback for L2C-310). What's even more interesting is that approaches > similar to the one currently used on OMAP had been NAKed, when > proposed for Exynos and this is why we have the solution proposed by > my patches. > > Note that .write_sec() callback is still used for L2X0_CTRL and > L2X0_DEBUG_CTRL registers, because there might be a need to write them > separately (e.g. to disable the controller and to perform debug > operations/workarounds when the controller is already enabled). we dont have a machine descriptor for configure instead we overide the logic(in you case after firmware load, in OMAP case, I need to figure out). my point being unlike the exynos configure code, OMAP code will look exactly like current pl310_configure-2 lines of code which really does not benefit anything. Thinking again, in fact, i'd rather drop this series than have to do a duplicated configure code(and force a resultant maintenance for the future) in OMAP code since none of OMAP4 or AM437x series need these patches. Interest for this series was non-mandatory, but just to be complete from SoC definition point of view. Let me know which way you guys want me to go. --- Regards, Nishanth Menon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2015-01-04 1:45 GMT+09:00 Nishanth Menon <nm@ti.com>: > On 01/03/2015 10:16 AM, Tomasz Figa wrote: >> >> 2015-01-04 0:34 GMT+09:00 Nishanth Menon <nm@ti.com>: >>> >>> On 15:40-20150103, Tomasz Figa wrote: >>>> >>>> Hi Nishanth, >>>> >>>> 2015-01-03 2:43 GMT+09:00 Nishanth Menon <nm@ti.com>: >>>>> >>>>> AM437x generation of processors support programming the PL310 L2Cache >>>>> controller's address filter start and end registers using a secure >>>>> montior service. >>>> >>>> >>>> typo: s/montior/monitor/ >>>> >>>> [snip] >>> >>> >>> Uggh.. yes indeed. I will post a v3 updating the comments. If the >>> following is ok. >>>> >>>> >>>>> + base = omap4_get_l2cache_base(); >>>>> + filter_start = (reg == L310_ADDR_FILTER_START) ? val : >>>>> + readl_relaxed(base + >>>>> L310_ADDR_FILTER_START); >>>>> + filter_end = (reg == L310_ADDR_FILTER_END) ? val : >>>>> + readl_relaxed(base + >>>>> L310_ADDR_FILTER_END); >>>>> + omap_smc1_2(AM43X_MON_L2X0_SETFILTER_INDEX, >>>>> filter_start, >>>>> + filter_end); >>>>> + return; >>>> >>>> >>>> I don't have any significant comments about this patch in particular, >>>> but just noticed that you need to do read-backs here (and the typo >>>> thanks to the spell checker of my mailing app). Maybe you should >>>> consider switching to the .configure() API I introduced in my series? >>>> This would let you get rid of the hardcoded static mapping. >>> >>> >>> Yeah, I have two choices there.. Either I provide the fundamental >>> write function for the generic l2c code to use OR I provide a >>> duplicate of resultant l2c_configure(aux write) + l2c310_configure. >>> >>> To allow for reuse of improvements or anything like errata >>> implementations in the future, OMAP L2C implementation has chosen to >>> provide the >>> low level code and allow the higherlevel configure/write/whatever of the >>> future to stay in arch/arm/mm/cache-l2x0.c. The write_sec operation is >>> not too complicated enough to warrant a replication of l2c310_configure. >>> >>> So, I prefer the current implementation than providing a .configure >>> handler for outer_cache.configure from SoC level. >>> >>> Let me know if anyone has a strong objection to this. >> >> >> Well, what l2c310_configure() does after my series is just writing the >> registers. If they cannot be written normally (without some tricks >> such as reading back other registers) then IMHO a separate function >> should be provided. >> >> This is becomes possible after patch 3/8 (ARM: l2c: Add interface to >> ask hypervisor to configure L2C) and what is used on Exynos which also >> updates multiple registers in single SMC calls. You can find an >> example of use in patch 6/8 (ARM: EXYNOS: Add .write_sec outer cache >> callback for L2C-310). What's even more interesting is that approaches >> similar to the one currently used on OMAP had been NAKed, when >> proposed for Exynos and this is why we have the solution proposed by >> my patches. >> >> Note that .write_sec() callback is still used for L2X0_CTRL and >> L2X0_DEBUG_CTRL registers, because there might be a need to write them >> separately (e.g. to disable the controller and to perform debug >> operations/workarounds when the controller is already enabled). > > > > we dont have a machine descriptor for configure instead we overide the > logic(in you case after firmware load, in OMAP case, I need to figure out). > my point being unlike the exynos configure code, OMAP code will look exactly > like current pl310_configure-2 lines of code which really does not benefit > anything. > > > Thinking again, in fact, i'd rather drop this series than have to do a > duplicated configure code(and force a resultant maintenance for the future) > in OMAP code since none of OMAP4 or AM437x series need these patches. > Interest for this series was non-mandatory, but just to be complete from SoC > definition point of view. > > Let me know which way you guys want me to go. Right, dropping this series would definitely solve the the read-back issue. :) After all, if you don't need to override the latencies in the kernel (i.e. have sane firmware, unlike certain Exynos boards ;)), then I don't see a point of having this feature. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/mach-omap2/omap-secure.h b/arch/arm/mach-omap2/omap-secure.h index 338fdab..569e167 100644 --- a/arch/arm/mach-omap2/omap-secure.h +++ b/arch/arm/mach-omap2/omap-secure.h @@ -44,6 +44,7 @@ #define OMAP4_MON_L2X0_AUXCTRL_INDEX 0x109 #define OMAP4_MON_L2X0_SETLATENCY_INDEX 0x112 #define OMAP4_MON_L2X0_PREFETCH_INDEX 0x113 +#define AM43X_MON_L2X0_SETFILTER_INDEX 0x114 #define OMAP5_DRA7_MON_SET_CNTFRQ_INDEX 0x109 #define OMAP5_MON_AMBA_IF_INDEX 0x108 diff --git a/arch/arm/mach-omap2/omap4-common.c b/arch/arm/mach-omap2/omap4-common.c index 25a0b2f..0b1454d 100644 --- a/arch/arm/mach-omap2/omap4-common.c +++ b/arch/arm/mach-omap2/omap4-common.c @@ -206,6 +206,27 @@ void omap4_l2c310_write_sec(unsigned long val, unsigned reg) return; } + case L310_ADDR_FILTER_START: + case L310_ADDR_FILTER_END: + { + void __iomem *base; + u32 filter_start, filter_end; + + if (!soc_is_am437x()) { + pr_info_once("OMAP L2C310: ROM does not support filter setting\n"); + return; + } + + base = omap4_get_l2cache_base(); + filter_start = (reg == L310_ADDR_FILTER_START) ? val : + readl_relaxed(base + L310_ADDR_FILTER_START); + filter_end = (reg == L310_ADDR_FILTER_END) ? val : + readl_relaxed(base + L310_ADDR_FILTER_END); + omap_smc1_2(AM43X_MON_L2X0_SETFILTER_INDEX, filter_start, + filter_end); + return; + } + default: WARN_ONCE(1, "OMAP L2C310: ignoring write to reg 0x%x\n", reg); return;
AM437x generation of processors support programming the PL310 L2Cache controller's address filter start and end registers using a secure montior service. Unfortunately, this secure monitor service is not supported on OMAP4 generation of processors. Information based on: OMAP4430 Public ROM Code API Functional Specfication revision 0.6 (Oct 27, 2010) OMAP4440 Public ROM Code API Functional Specfication revision 0.1 (Oct 27, 2010) Aegis ROM Code Memory and Peripheral Booting Functional Specification version 1.00 (Jan 21, 2014) Signed-off-by: Nishanth Menon <nm@ti.com> --- Changes in V2: - smc call should use filter start in r0 and filter end in r1. - Document version update. V1: https://patchwork.kernel.org/patch/5560111/ arch/arm/mach-omap2/omap-secure.h | 1 + arch/arm/mach-omap2/omap4-common.c | 21 +++++++++++++++++++++ 2 files changed, 22 insertions(+)