| Message ID | 1359445870-18925-14-git-send-email-nicolas.pitre@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tuesday 29 January 2013 01:21 PM, Nicolas Pitre wrote: > From: Dave Martin <dave.martin@linaro.org> > > Non-local variables used by the CCI management function called after > disabling the cache must be flushed out to main memory in advance, > otherwise incoherency of those values may occur if they are sitting > in the cache of some other CPU when cci_disable() executes. > Any CPU calling cci_disable() would have already cleaned its local cache and the snoop unit should take care of syncing the shared data before hand from other CPU local caches for shared accesses. May be I am unable to visualize the issue here or missing some key point. > This patch adds the appropriate flushing to the CCI driver to ensure > that the relevant data is available in RAM ahead of time. > > Because this creates a dependency on arch-specific cacheflushing > functions, this patch also makes ARM_CCI depend on ARM. > You should do that otherwise to avoid other arch building this driver for random builds and breaking their builds. > Signed-off-by: Dave Martin <dave.martin@linaro.org> > Signed-off-by: Nicolas Pitre <nico@linaro.org> > --- Patch is fine apart from the question. Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
On Fri, 1 Feb 2013, Santosh Shilimkar wrote: > On Tuesday 29 January 2013 01:21 PM, Nicolas Pitre wrote: > > From: Dave Martin <dave.martin@linaro.org> > > > > Non-local variables used by the CCI management function called after > > disabling the cache must be flushed out to main memory in advance, > > otherwise incoherency of those values may occur if they are sitting > > in the cache of some other CPU when cci_disable() executes. > > > Any CPU calling cci_disable() would have already cleaned its local > cache and the snoop unit should take care of syncing the shared data > before hand from other CPU local caches for shared accesses. > May be I am unable to visualize the issue here or missing some key > point. Let's suppose CPU0 initializes the CCI. Without this patch, the CCI base address might be sitting in CPU0's cache. The last CPU in a cluster to shut itself down is responsible for calling cci_disable(). And being the last, it is also responsible for flushing out its L1 and L2 caches before doing that. If CPU0 went down before that, it did flush its L1 already. So the base address will be flushed to RAM in that case. But if it is a CPU in _another_ cluster which is shutting down and becoming the last man _there_. It will flush its L1 and L2 cache before calling cci_disable(). And because the cache is disabled at that point, that CPU won't send any snoop request across to the other cluster where CPU0 holds the base address in its L1 or even L2 cache. This is why we must push out that value out to RAM before cci_disable() is used. > > This patch adds the appropriate flushing to the CCI driver to ensure > > that the relevant data is available in RAM ahead of time. > > > > Because this creates a dependency on arch-specific cacheflushing > > functions, this patch also makes ARM_CCI depend on ARM. > > > You should do that otherwise to avoid other arch building this > driver for random builds and breaking their builds. Before this patch the driver was buildable on any architecture. That's why this dependency is added only in this patch. > > Signed-off-by: Dave Martin <dave.martin@linaro.org> > > Signed-off-by: Nicolas Pitre <nico@linaro.org> > > --- > Patch is fine apart from the question. > > Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > Nicvolas
On Sunday 03 February 2013 03:53 AM, Nicolas Pitre wrote: > On Fri, 1 Feb 2013, Santosh Shilimkar wrote: > >> On Tuesday 29 January 2013 01:21 PM, Nicolas Pitre wrote: >>> From: Dave Martin <dave.martin@linaro.org> >>> >>> Non-local variables used by the CCI management function called after >>> disabling the cache must be flushed out to main memory in advance, >>> otherwise incoherency of those values may occur if they are sitting >>> in the cache of some other CPU when cci_disable() executes. >>> >> Any CPU calling cci_disable() would have already cleaned its local >> cache and the snoop unit should take care of syncing the shared data >> before hand from other CPU local caches for shared accesses. >> May be I am unable to visualize the issue here or missing some key >> point. > > Let's suppose CPU0 initializes the CCI. Without this patch, the CCI > base address might be sitting in CPU0's cache. > > The last CPU in a cluster to shut itself down is responsible for calling > cci_disable(). And being the last, it is also responsible for flushing > out its L1 and L2 caches before doing that. If CPU0 went down before > that, it did flush its L1 already. So the base address will be flushed > to RAM in that case. > Yes. This is valid case. Thanks for description. > But if it is a CPU in _another_ cluster which is shutting down and > becoming the last man _there_. It will flush its L1 and L2 cache > before calling cci_disable(). And > because the cache is disabled at that point, that CPU won't > send any snoop request across to the other cluster where CPU0 holds the > base address in its L1 or even L2 cache. > > This is why we must push out that value out to RAM before cci_disable() > is used. > >>> This patch adds the appropriate flushing to the CCI driver to ensure >>> that the relevant data is available in RAM ahead of time. >>> >>> Because this creates a dependency on arch-specific cacheflushing >>> functions, this patch also makes ARM_CCI depend on ARM. >>> >> You should do that otherwise to avoid other arch building this >> driver for random builds and breaking their builds. > > Before this patch the driver was buildable on any architecture. That's > why this dependency is added only in this patch. > I was just trying to counter the reasoning in the changelog which says dependency is added because of arch specific cache flushing function. Meaning even without that ARM dependency should be in place to avoid driver getting build for other archs. Regards, Santosh
On Sun, 3 Feb 2013, Santosh Shilimkar wrote: > On Sunday 03 February 2013 03:53 AM, Nicolas Pitre wrote: > > On Fri, 1 Feb 2013, Santosh Shilimkar wrote: > > > > > On Tuesday 29 January 2013 01:21 PM, Nicolas Pitre wrote: > > > > From: Dave Martin <dave.martin@linaro.org> > > > > > > > > Non-local variables used by the CCI management function called after > > > > disabling the cache must be flushed out to main memory in advance, > > > > otherwise incoherency of those values may occur if they are sitting > > > > in the cache of some other CPU when cci_disable() executes. > > > > > > > Any CPU calling cci_disable() would have already cleaned its local > > > cache and the snoop unit should take care of syncing the shared data > > > before hand from other CPU local caches for shared accesses. > > > May be I am unable to visualize the issue here or missing some key > > > point. > > > > Let's suppose CPU0 initializes the CCI. Without this patch, the CCI > > base address might be sitting in CPU0's cache. > > > > The last CPU in a cluster to shut itself down is responsible for calling > > cci_disable(). And being the last, it is also responsible for flushing > > out its L1 and L2 caches before doing that. If CPU0 went down before > > that, it did flush its L1 already. So the base address will be flushed > > to RAM in that case. > > > Yes. This is valid case. Thanks for description. > > > But if it is a CPU in _another_ cluster which is shutting down and > > becoming the last man _there_. It will flush its L1 and L2 cache > > before calling cci_disable(). And > > because the cache is disabled at that point, that CPU won't > > send any snoop request across to the other cluster where CPU0 holds the > > base address in its L1 or even L2 cache. > > > > This is why we must push out that value out to RAM before cci_disable() > > is used. > > > > > > This patch adds the appropriate flushing to the CCI driver to ensure > > > > that the relevant data is available in RAM ahead of time. > > > > > > > > Because this creates a dependency on arch-specific cacheflushing > > > > functions, this patch also makes ARM_CCI depend on ARM. > > > > > > > You should do that otherwise to avoid other arch building this > > > driver for random builds and breaking their builds. > > > > Before this patch the driver was buildable on any architecture. That's > > why this dependency is added only in this patch. > > > I was just trying to counter the reasoning in the changelog which says > dependency is added because of arch specific cache flushing function. > Meaning even without that ARM dependency should be in place to avoid > driver getting build for other archs. Well, some upstream maintainers' opinion is that you should not put artificial dependencies on a specific architecture if a driver is buildable on any architecture, even if the built driver is of no use to those other architectures. Nicolas
On Sunday 03 February 2013 11:59 PM, Nicolas Pitre wrote: > On Sun, 3 Feb 2013, Santosh Shilimkar wrote: > >> On Sunday 03 February 2013 03:53 AM, Nicolas Pitre wrote: >>> On Fri, 1 Feb 2013, Santosh Shilimkar wrote: >>> >>>> On Tuesday 29 January 2013 01:21 PM, Nicolas Pitre wrote: >>>>> From: Dave Martin <dave.martin@linaro.org> [..] >>>>> This patch adds the appropriate flushing to the CCI driver to ensure >>>>> that the relevant data is available in RAM ahead of time. >>>>> >>>>> Because this creates a dependency on arch-specific cacheflushing >>>>> functions, this patch also makes ARM_CCI depend on ARM. >>>>> >>>> You should do that otherwise to avoid other arch building this >>>> driver for random builds and breaking their builds. >>> >>> Before this patch the driver was buildable on any architecture. That's >>> why this dependency is added only in this patch. >>> >> I was just trying to counter the reasoning in the changelog which says >> dependency is added because of arch specific cache flushing function. >> Meaning even without that ARM dependency should be in place to avoid >> driver getting build for other archs. > > Well, some upstream maintainers' opinion is that you should not put > artificial dependencies on a specific architecture if a driver is > buildable on any architecture, even if the built driver is of no use to > those other architectures. > I see. I have seen some complaint about not adding the arch dependency. Anyways, the patch has the dependency set with ARM and hence its non-issue. Regards, Santosh
diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig index d032f74ff2..cd4ac9f001 100644 --- a/drivers/bus/Kconfig +++ b/drivers/bus/Kconfig @@ -22,5 +22,6 @@ config OMAP_INTERCONNECT config ARM_CCI bool "ARM CCI driver support" + depends on ARM endmenu diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c index 25ae156924..30e4a77da0 100644 --- a/drivers/bus/arm-cci.c +++ b/drivers/bus/arm-cci.c @@ -21,8 +21,16 @@ #include <linux/slab.h> #include <linux/arm-cci.h> -#define CCI400_EAG_OFFSET 0x4000 -#define CCI400_KF_OFFSET 0x5000 +#include <asm/cacheflush.h> +#include <asm/memory.h> +#include <asm/outercache.h> + +#include <asm/irq_regs.h> +#include <asm/pmu.h> + +#define CCI400_PMCR 0x0100 +#define CCI400_EAG_OFFSET 0x4000 +#define CCI400_KF_OFFSET 0x5000 #define DRIVER_NAME "CCI" struct cci_drvdata { @@ -73,6 +81,15 @@ static int cci_driver_probe(struct platform_device *pdev) goto ioremap_err; } + /* + * Multi-cluster systems may need this data when non-coherent, during + * cluster power-up/power-down. Make sure it reaches main memory: + */ + __cpuc_flush_dcache_area(info, sizeof *info); + __cpuc_flush_dcache_area(&info, sizeof info); + outer_clean_range(virt_to_phys(info), virt_to_phys(info + 1)); + outer_clean_range(virt_to_phys(&info), virt_to_phys(&info + 1)); + platform_set_drvdata(pdev, info); pr_info("CCI loaded at %p\n", info->baseaddr);