* RE: [PATCH 2/2] arm: mvebu: Add hardware I/O Coherency support
2012-10-24 8:04 ` [PATCH 2/2] arm: mvebu: Add hardware I/O Coherency support Gregory CLEMENT
@ 2012-10-24 8:11 ` Yehuda Yitschak
2012-10-24 11:36 ` Arnd Bergmann
` (2 subsequent siblings)
3 siblings, 0 replies; 15+ messages in thread
From: Yehuda Yitschak @ 2012-10-24 8:11 UTC (permalink / raw)
To: Gregory CLEMENT, Jason Cooper, Andrew Lunn
Cc: linux-arm-kernel, Arnd Bergmann, Olof Johansson, Russell King,
Rob Herring, Ben Dooks, Ian Molton, Nicolas Pitre, Lior Amsalem,
Maen Suleiman, Tawfik Bayouk, Shadi Ammouri, Eran Ben-Avi,
Nadav Haklai, Ike Pan, Jani Monoses, Chris Van Hoof, Dan Frazier,
Thomas Petazzoni, Leif Lindholm, Jon Masters, David Marlin,
Sebastian Hesselbarth, linux-kernel
> -----Original Message-----
> From: Gregory CLEMENT [mailto:gregory.clement@free-electrons.com]
> Sent: Wednesday, October 24, 2012 10:04 AM
> To: Jason Cooper; Andrew Lunn; Gregory Clement
> Cc: linux-arm-kernel@lists.infradead.org; Arnd Bergmann; Olof Johansson;
> Russell King; Rob Herring; Ben Dooks; Ian Molton; Nicolas Pitre; Lior
> Amsalem; Maen Suleiman; Tawfik Bayouk; Shadi Ammouri; Eran Ben-Avi;
> Yehuda Yitschak; Nadav Haklai; Ike Pan; Jani Monoses; Chris Van Hoof; Dan
> Frazier; Thomas Petazzoni; Leif Lindholm; Jon Masters; David Marlin;
> Sebastian Hesselbarth; linux-kernel@vger.kernel.org
> Subject: [PATCH 2/2] arm: mvebu: Add hardware I/O Coherency support
>
> Armada 370 and XP come with an unit called coherency fabric. This unit
> allows to use the Armada XP as a nearly coherent architecture. The
> coherency mechanism uses snoop filters to ensure the coherency between
> caches, DRAM and devices. This mechanism needs a synchronization barrier
> which guarantees that all memory write initiated by the devices has
> reached their target and do not reside in intermediate write buffers. That's
> why the architecture is not totally coherent and we need to provide our
> own functions for some DMA operations.
>
> Beside the use of the coherency fabric, the device units will have to set the
> attribute flag to select the accurate coherency process for the memory
> transaction. This is done each device driver programs the DRAM address
> windows. The value of the attribute set by the driver is retrieved through
> the orion_addr_map_cfg struct filled during the early initialization of the
> platform.
>
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> Reviewed-by: Yehuda Yitschak <yehuday@marvell.com>
> ---
> arch/arm/boot/dts/armada-370-xp.dtsi | 3 +-
> arch/arm/mach-mvebu/addr-map.c | 3 ++
> arch/arm/mach-mvebu/armada-370-xp.c | 1 +
> arch/arm/mach-mvebu/coherency.c | 87
> ++++++++++++++++++++++++++++++++++
> arch/arm/mach-mvebu/common.h | 2 +
> 5 files changed, 95 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi
> b/arch/arm/boot/dts/armada-370-xp.dtsi
> index 18ba60b..af22e53 100644
> --- a/arch/arm/boot/dts/armada-370-xp.dtsi
> +++ b/arch/arm/boot/dts/armada-370-xp.dtsi
> @@ -38,7 +38,8 @@
>
> coherency-fabric@d0020200 {
> compatible = "marvell,coherency-fabric";
> - reg = <0xd0020200 0xb0>;
> + reg = <0xd0020200 0xb0>,
> + <0xd0021010 0x1c>;
> };
>
> soc {
> diff --git a/arch/arm/mach-mvebu/addr-map.c b/arch/arm/mach-
> mvebu/addr-map.c index fe454a4..595f6b7 100644
> --- a/arch/arm/mach-mvebu/addr-map.c
> +++ b/arch/arm/mach-mvebu/addr-map.c
> @@ -108,6 +108,9 @@ static int __init armada_setup_cpu_mbus(void)
>
> addr_map_cfg.bridge_virt_base = mbus_unit_addr_decoding_base;
>
> + if (of_find_compatible_node(NULL, NULL, "marvell,coherency-
> fabric"))
> + addr_map_cfg.hw_io_coherency = 1;
> +
> /*
> * Disable, clear and configure windows.
> */
> diff --git a/arch/arm/mach-mvebu/armada-370-xp.c b/arch/arm/mach-
> mvebu/armada-370-xp.c
> index 41431a1..3517f7d 100644
> --- a/arch/arm/mach-mvebu/armada-370-xp.c
> +++ b/arch/arm/mach-mvebu/armada-370-xp.c
> @@ -49,6 +49,7 @@ struct sys_timer armada_370_xp_timer = {
>
> static void __init armada_370_xp_dt_init(void) {
> + armada_370_xp_coherency_iocache_init();
> of_platform_populate(NULL, of_default_bus_match_table, NULL,
> NULL); }
>
> diff --git a/arch/arm/mach-mvebu/coherency.c b/arch/arm/mach-
> mvebu/coherency.c index 71e27ba..a596ca9 100644
> --- a/arch/arm/mach-mvebu/coherency.c
> +++ b/arch/arm/mach-mvebu/coherency.c
> @@ -22,6 +22,10 @@
> #include <linux/of_address.h>
> #include <linux/io.h>
> #include <linux/smp.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/platform_device.h>
> +#include <asm/smp_plat.h>
> +
> #include "armada-370-xp.h"
>
> /* Some functions in this file are called very early during SMP @@ -31,16
> +35,53 @@
> * value matching its virtual mapping
> */
> static void __iomem *coherency_base = ARMADA_370_XP_REGS_VIRT_BASE
> + 0x20200;
> +static void __iomem *coherency_cpu_base;
> +
> +struct dma_map_ops armada_xp_dma_ops;
>
> /* Coherency fabric registers */
> #define COHERENCY_FABRIC_CTL_OFFSET 0x0
> #define COHERENCY_FABRIC_CFG_OFFSET 0x4
>
> +#define IO_SYNC_BARRIER_CTL_OFFSET 0x0
> +
> static struct of_device_id of_coherency_table[] = {
> {.compatible = "marvell,coherency-fabric"},
> { /* end of list */ },
> };
>
> +static inline void armada_xp_sync_io_barrier(void) {
> + writel(0x1, coherency_cpu_base + IO_SYNC_BARRIER_CTL_OFFSET);
> + while (readl(coherency_cpu_base + IO_SYNC_BARRIER_CTL_OFFSET)
> & 0x1);
> +}
> +
> +dma_addr_t armada_xp_dma_map_page(struct device *dev, struct page
> *page,
> + unsigned long offset, size_t size,
> + enum dma_data_direction dir,
> + struct dma_attrs *attrs)
> +{
> + if (dir != DMA_TO_DEVICE)
> + armada_xp_sync_io_barrier();
> + return pfn_to_dma(dev, page_to_pfn(page)) + offset; }
> +
> +
> +void armada_xp_dma_unmap_page(struct device *dev, dma_addr_t
> dma_handle,
> + size_t size, enum dma_data_direction dir,
> + struct dma_attrs *attrs)
> +{
> + if (dir != DMA_TO_DEVICE)
> + armada_xp_sync_io_barrier();
> +}
> +
> +void armada_xp_dma_sync(struct device *dev, dma_addr_t dma_handle,
> + size_t size, enum dma_data_direction dir) {
> + if (dir != DMA_TO_DEVICE)
> + armada_xp_sync_io_barrier();
> +}
> +
Shouldn't all the 4 functions above start with armada_370_xp and not armada_xp ?
> int armada_xp_get_cpu_count(void)
This function can be limited to CONFIG_SP
> {
> int reg, cnt;
> @@ -74,6 +115,42 @@ int armada_370_xp_set_cpu_coherent(unsigned int
> hw_cpu_id, int smp_group_id)
> return 0;
> }
>
> +static int armada_xp_platform_notifier(struct notifier_block *nb,
> + unsigned long event, void *__dev) {
> + struct device *dev = __dev;
> +
> + if (event != BUS_NOTIFY_ADD_DEVICE)
> + return NOTIFY_DONE;
> + set_dma_ops(dev, &armada_xp_dma_ops);
> +
> + return NOTIFY_OK;
> +}
> +
> +static struct notifier_block armada_xp_platform_nb = {
> + .notifier_call = armada_xp_platform_notifier, };
> +
> +void __init armada_370_xp_coherency_iocache_init(void)
> +{
> + /* When the coherency fabric is available, the Armada XP and
> + * Aramada 370 are close to a coherent architecture, so we based
> + * our dma ops on the coherent one, and just changes the
> + * operations which need a arch io sync */
> + if (of_find_compatible_node(NULL, NULL, "marvell,coherency-
> fabric")) {
> + struct dma_map_ops *dma_ops = &armada_xp_dma_ops;
> + memcpy(dma_ops, &arm_coherent_dma_ops,
> sizeof(*dma_ops));
> + dma_ops->map_page = armada_xp_dma_map_page;
> + dma_ops->unmap_page = armada_xp_dma_unmap_page;
> + dma_ops->unmap_sg = arm_dma_ops.unmap_sg;
> + dma_ops->sync_single_for_cpu = armada_xp_dma_sync;
> + dma_ops->sync_single_for_device = armada_xp_dma_sync;
> + dma_ops->sync_sg_for_cpu =
> arm_dma_ops.sync_sg_for_cpu;
> + dma_ops->sync_sg_for_device =
> arm_dma_ops.sync_sg_for_device;
> + }
> + bus_register_notifier(&platform_bus_type,
> &armada_xp_platform_nb); }
> +
> int __init armada_370_xp_coherency_init(void)
> {
> struct device_node *np;
> @@ -82,7 +159,17 @@ int __init armada_370_xp_coherency_init(void)
> if (np) {
> pr_info("Initializing Coherency fabric\n");
> coherency_base = of_iomap(np, 0);
> + coherency_cpu_base = of_iomap(np, 1);
> + }
> +#ifndef CONFIG_SMP
> + if (coherency_base) {
> + /* In UP case, cpu coherency is enabled here, in SMP case
> cpu
> + * coherency is enabled for each CPU by
> + * armada_xp_smp_prepare_cpus() in platsmp.c */
> + int hw_cpuid = cpu_logical_map(smp_processor_id());
> + armada_370_xp_set_cpu_coherent(hw_cpuid, 0);
> }
> +#endif
>
> return 0;
> }
> diff --git a/arch/arm/mach-mvebu/common.h b/arch/arm/mach-
> mvebu/common.h index 86484bb..fff952e 100644
> --- a/arch/arm/mach-mvebu/common.h
> +++ b/arch/arm/mach-mvebu/common.h
> @@ -23,6 +23,8 @@ void armada_370_xp_handle_irq(struct pt_regs *regs);
>
> void armada_xp_cpu_die(unsigned int cpu);
>
> +void armada_370_xp_coherency_iocache_init(void);
> +
> int armada_370_xp_coherency_init(void);
> int armada_370_xp_pmsu_init(void);
> void armada_xp_secondary_startup(void);
> --
> 1.7.9.5
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 2/2] arm: mvebu: Add hardware I/O Coherency support
2012-10-24 8:04 ` [PATCH 2/2] arm: mvebu: Add hardware I/O Coherency support Gregory CLEMENT
2012-10-24 8:11 ` Yehuda Yitschak
@ 2012-10-24 11:36 ` Arnd Bergmann
2012-10-24 11:48 ` Gregory CLEMENT
[not found] ` <20121024082529.GZ11837@lunn.ch>
2012-10-24 12:27 ` Thomas Petazzoni
3 siblings, 1 reply; 15+ messages in thread
From: Arnd Bergmann @ 2012-10-24 11:36 UTC (permalink / raw)
To: Gregory CLEMENT
Cc: Jason Cooper, Andrew Lunn, linux-arm-kernel, Olof Johansson,
Russell King, Rob Herring, Ben Dooks, Ian Molton, Nicolas Pitre,
Lior Amsalem, Maen Suleiman, Tawfik Bayouk, Shadi Ammouri,
Eran Ben-Avi, Yehuda Yitschak, Nadav Haklai, Ike Pan,
Jani Monoses, Chris Van Hoof, Dan Frazier, Thomas Petazzoni,
Leif Lindholm, Jon Masters, David Marlin, Sebastian Hesselbarth,
linux-kernel
On Wednesday 24 October 2012, Gregory CLEMENT wrote:
> +void __init armada_370_xp_coherency_iocache_init(void)
> +{
> + /* When the coherency fabric is available, the Armada XP and
> + * Aramada 370 are close to a coherent architecture, so we based
> + * our dma ops on the coherent one, and just changes the
> + * operations which need a arch io sync */
> + if (of_find_compatible_node(NULL, NULL, "marvell,coherency-fabric")) {
> + struct dma_map_ops *dma_ops = &armada_xp_dma_ops;
> + memcpy(dma_ops, &arm_coherent_dma_ops, sizeof(*dma_ops));
> + dma_ops->map_page = armada_xp_dma_map_page;
> + dma_ops->unmap_page = armada_xp_dma_unmap_page;
> + dma_ops->unmap_sg = arm_dma_ops.unmap_sg;
> + dma_ops->sync_single_for_cpu = armada_xp_dma_sync;
> + dma_ops->sync_single_for_device = armada_xp_dma_sync;
> + dma_ops->sync_sg_for_cpu = arm_dma_ops.sync_sg_for_cpu;
> + dma_ops->sync_sg_for_device = arm_dma_ops.sync_sg_for_device;
> + }
> + bus_register_notifier(&platform_bus_type, &armada_xp_platform_nb);
I think it would be cleaner to statically define the operations in a constant
structure and point directly to the functions you need. If necessary, use
multiple structures.
Arnd
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 2/2] arm: mvebu: Add hardware I/O Coherency support
2012-10-24 11:36 ` Arnd Bergmann
@ 2012-10-24 11:48 ` Gregory CLEMENT
2012-10-24 11:53 ` Gregory CLEMENT
0 siblings, 1 reply; 15+ messages in thread
From: Gregory CLEMENT @ 2012-10-24 11:48 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Jason Cooper, Andrew Lunn, linux-arm-kernel, Olof Johansson,
Russell King, Rob Herring, Ben Dooks, Ian Molton, Nicolas Pitre,
Lior Amsalem, Maen Suleiman, Tawfik Bayouk, Shadi Ammouri,
Eran Ben-Avi, Yehuda Yitschak, Nadav Haklai, Ike Pan,
Jani Monoses, Chris Van Hoof, Dan Frazier, Thomas Petazzoni,
Leif Lindholm, Jon Masters, David Marlin, Sebastian Hesselbarth,
linux-kernel
On 10/24/2012 01:36 PM, Arnd Bergmann wrote:
> On Wednesday 24 October 2012, Gregory CLEMENT wrote:
>> +void __init armada_370_xp_coherency_iocache_init(void)
>> +{
>> + /* When the coherency fabric is available, the Armada XP and
>> + * Aramada 370 are close to a coherent architecture, so we based
>> + * our dma ops on the coherent one, and just changes the
>> + * operations which need a arch io sync */
>> + if (of_find_compatible_node(NULL, NULL, "marvell,coherency-fabric")) {
>> + struct dma_map_ops *dma_ops = &armada_xp_dma_ops;
>> + memcpy(dma_ops, &arm_coherent_dma_ops, sizeof(*dma_ops));
>> + dma_ops->map_page = armada_xp_dma_map_page;
>> + dma_ops->unmap_page = armada_xp_dma_unmap_page;
>> + dma_ops->unmap_sg = arm_dma_ops.unmap_sg;
>> + dma_ops->sync_single_for_cpu = armada_xp_dma_sync;
>> + dma_ops->sync_single_for_device = armada_xp_dma_sync;
>> + dma_ops->sync_sg_for_cpu = arm_dma_ops.sync_sg_for_cpu;
>> + dma_ops->sync_sg_for_device = arm_dma_ops.sync_sg_for_device;
>> + }
>> + bus_register_notifier(&platform_bus_type, &armada_xp_platform_nb);
>
> I think it would be cleaner to statically define the operations in a constant
> structure and point directly to the functions you need. If necessary, use
> multiple structures.
My problem was that these functions are not exposed, only arm_dma_op and
arm_coherent_dma_ops are exported.
Or do you think about something like this:
struct dma_map_ops *dma_ops = {
.alloc = arm_dma_ops.arm_coherent_dma_alloc,
.free = arm_dma_ops.arm_coherent_dma_free,
.mmap = arm_dma_ops.arm_dma_mmap,
.get_sgtable = arm_dma_ops.arm_dma_get_sgtable,
.map_sg = arm_dma_ops.arm_dma_map_sg,
.set_dma_mask = arm_dma_ops.arm_dma_set_mask,
.map_page = armada_xp_dma_map_page,
.unmap_page = armada_xp_dma_unmap_page,
.unmap_sg = arm_dma_ops.unmap_sg,
.sync_single_for_cpu = armada_xp_dma_sync,
.sync_single_for_device = armada_xp_dma_sync,
.sync_sg_for_cpu = arm_dma_ops.sync_sg_for_cpu,
.sync_sg_for_device = arm_dma_ops.sync_sg_for_device,
};
>
> Arnd
>
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 2/2] arm: mvebu: Add hardware I/O Coherency support
2012-10-24 11:48 ` Gregory CLEMENT
@ 2012-10-24 11:53 ` Gregory CLEMENT
2012-10-24 12:24 ` Arnd Bergmann
0 siblings, 1 reply; 15+ messages in thread
From: Gregory CLEMENT @ 2012-10-24 11:53 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Lior Amsalem, Andrew Lunn, Ike Pan, Nadav Haklai, Ian Molton,
David Marlin, Yehuda Yitschak, Jani Monoses, Russell King,
Tawfik Bayouk, Dan Frazier, Eran Ben-Avi, Leif Lindholm,
Sebastian Hesselbarth, Jason Cooper, Jon Masters, Rob Herring,
Ben Dooks, linux-arm-kernel, Thomas Petazzoni, Chris Van Hoof,
Nicolas Pitre, linux-kernel, Maen Suleiman, Shadi Ammouri,
Olof Johansson
On 10/24/2012 01:48 PM, Gregory CLEMENT wrote:
> On 10/24/2012 01:36 PM, Arnd Bergmann wrote:
>> On Wednesday 24 October 2012, Gregory CLEMENT wrote:
>>> +void __init armada_370_xp_coherency_iocache_init(void)
>>> +{
>>> + /* When the coherency fabric is available, the Armada XP and
>>> + * Aramada 370 are close to a coherent architecture, so we based
>>> + * our dma ops on the coherent one, and just changes the
>>> + * operations which need a arch io sync */
>>> + if (of_find_compatible_node(NULL, NULL, "marvell,coherency-fabric")) {
>>> + struct dma_map_ops *dma_ops = &armada_xp_dma_ops;
>>> + memcpy(dma_ops, &arm_coherent_dma_ops, sizeof(*dma_ops));
>>> + dma_ops->map_page = armada_xp_dma_map_page;
>>> + dma_ops->unmap_page = armada_xp_dma_unmap_page;
>>> + dma_ops->unmap_sg = arm_dma_ops.unmap_sg;
>>> + dma_ops->sync_single_for_cpu = armada_xp_dma_sync;
>>> + dma_ops->sync_single_for_device = armada_xp_dma_sync;
>>> + dma_ops->sync_sg_for_cpu = arm_dma_ops.sync_sg_for_cpu;
>>> + dma_ops->sync_sg_for_device = arm_dma_ops.sync_sg_for_device;
>>> + }
>>> + bus_register_notifier(&platform_bus_type, &armada_xp_platform_nb);
>>
>> I think it would be cleaner to statically define the operations in a constant
>> structure and point directly to the functions you need. If necessary, use
>> multiple structures.
>
> My problem was that these functions are not exposed, only arm_dma_op and
> arm_coherent_dma_ops are exported.
> Or do you think about something like this:
>
> struct dma_map_ops *dma_ops = {
> .alloc = arm_dma_ops.arm_coherent_dma_alloc,
> .free = arm_dma_ops.arm_coherent_dma_free,
> .mmap = arm_dma_ops.arm_dma_mmap,
> .get_sgtable = arm_dma_ops.arm_dma_get_sgtable,
> .map_sg = arm_dma_ops.arm_dma_map_sg,
> .set_dma_mask = arm_dma_ops.arm_dma_set_mask,
> .map_page = armada_xp_dma_map_page,
> .unmap_page = armada_xp_dma_unmap_page,
> .unmap_sg = arm_dma_ops.unmap_sg,
> .sync_single_for_cpu = armada_xp_dma_sync,
> .sync_single_for_device = armada_xp_dma_sync,
> .sync_sg_for_cpu = arm_dma_ops.sync_sg_for_cpu,
> .sync_sg_for_device = arm_dma_ops.sync_sg_for_device,
> };
I was too fast to reply this struct is wrong, it should be as this one:
struct dma_map_ops *dma_ops = {
.alloc = arm_coherent_dma_ops.arm_coherent_dma_alloc,
.free = arm_coherent_dma_ops.arm_coherent_dma_free,
.mmap = arm_coherent_dma_ops.arm_dma_mmap,
.get_sgtable = arm_coherent_dma_ops.arm_dma_get_sgtable,
.map_sg = arm_coherent_dma_ops.arm_dma_map_sg,
.set_dma_mask = arm_coherent_dma_ops.arm_dma_set_mask,
.map_page = armada_xp_dma_map_page,
.unmap_page = armada_xp_dma_unmap_page,
.unmap_sg = arm_dma_ops.unmap_sg,
.sync_single_for_cpu = armada_xp_dma_sync,
.sync_single_for_device = armada_xp_dma_sync,
.sync_sg_for_cpu = arm_dma_ops.sync_sg_for_cpu,
.sync_sg_for_device = arm_dma_ops.sync_sg_for_device,
};
Thanks,
Gregory
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 2/2] arm: mvebu: Add hardware I/O Coherency support
2012-10-24 11:53 ` Gregory CLEMENT
@ 2012-10-24 12:24 ` Arnd Bergmann
2012-10-24 13:56 ` Gregory CLEMENT
0 siblings, 1 reply; 15+ messages in thread
From: Arnd Bergmann @ 2012-10-24 12:24 UTC (permalink / raw)
To: Gregory CLEMENT
Cc: Lior Amsalem, Andrew Lunn, Ike Pan, Nadav Haklai, Ian Molton,
David Marlin, Yehuda Yitschak, Jani Monoses, Russell King,
Tawfik Bayouk, Dan Frazier, Eran Ben-Avi, Leif Lindholm,
Sebastian Hesselbarth, Jason Cooper, Jon Masters, Rob Herring,
Ben Dooks, linux-arm-kernel, Thomas Petazzoni, Chris Van Hoof,
Nicolas Pitre, linux-kernel, Maen Suleiman, Shadi Ammouri,
Olof Johansson
On Wednesday 24 October 2012, Gregory CLEMENT wrote:
> On 10/24/2012 01:48 PM, Gregory CLEMENT wrote:
> > On 10/24/2012 01:36 PM, Arnd Bergmann wrote:
> >>
> >> I think it would be cleaner to statically define the operations in a constant
> >> structure and point directly to the functions you need. If necessary, use
> >> multiple structures.
> >
> > My problem was that these functions are not exposed, only arm_dma_op and
> > arm_coherent_dma_ops are exported.
> > Or do you think about something like this:
>
> struct dma_map_ops *dma_ops = {
> .alloc = arm_coherent_dma_ops.arm_coherent_dma_alloc,
> .free = arm_coherent_dma_ops.arm_coherent_dma_free,
> .mmap = arm_coherent_dma_ops.arm_dma_mmap,
> .get_sgtable = arm_coherent_dma_ops.arm_dma_get_sgtable,
> .map_sg = arm_coherent_dma_ops.arm_dma_map_sg,
> .set_dma_mask = arm_coherent_dma_ops.arm_dma_set_mask,
> .map_page = armada_xp_dma_map_page,
> .unmap_page = armada_xp_dma_unmap_page,
> .unmap_sg = arm_dma_ops.unmap_sg,
> .sync_single_for_cpu = armada_xp_dma_sync,
> .sync_single_for_device = armada_xp_dma_sync,
> .sync_sg_for_cpu = arm_dma_ops.sync_sg_for_cpu,
> .sync_sg_for_device = arm_dma_ops.sync_sg_for_device,
> };
No, I was thinking of making the underlying functions globally visible
and have extern declarations in a header file so you can access them
directly.
Generally speaking, when you run into a problem with common code, your
first approach should be to fix the common code before you try to work
around it.
Arnd
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 2/2] arm: mvebu: Add hardware I/O Coherency support
2012-10-24 12:24 ` Arnd Bergmann
@ 2012-10-24 13:56 ` Gregory CLEMENT
2012-10-24 20:30 ` Arnd Bergmann
0 siblings, 1 reply; 15+ messages in thread
From: Gregory CLEMENT @ 2012-10-24 13:56 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Lior Amsalem, Andrew Lunn, Ike Pan, Nadav Haklai, Ian Molton,
David Marlin, Yehuda Yitschak, Jani Monoses, Russell King,
Tawfik Bayouk, Dan Frazier, Eran Ben-Avi, Leif Lindholm,
Sebastian Hesselbarth, Jason Cooper, Jon Masters, Rob Herring,
Ben Dooks, linux-arm-kernel, Thomas Petazzoni, Chris Van Hoof,
Nicolas Pitre, linux-kernel, Maen Suleiman, Shadi Ammouri,
Olof Johansson
On 10/24/2012 02:24 PM, Arnd Bergmann wrote:
> On Wednesday 24 October 2012, Gregory CLEMENT wrote:
>> On 10/24/2012 01:48 PM, Gregory CLEMENT wrote:
>>> On 10/24/2012 01:36 PM, Arnd Bergmann wrote:
>>>>
>>>> I think it would be cleaner to statically define the operations in a constant
>>>> structure and point directly to the functions you need. If necessary, use
>>>> multiple structures.
>>>
>>> My problem was that these functions are not exposed, only arm_dma_op and
>>> arm_coherent_dma_ops are exported.
>>> Or do you think about something like this:
>>
>> struct dma_map_ops *dma_ops = {
>> .alloc = arm_coherent_dma_ops.arm_coherent_dma_alloc,
>> .free = arm_coherent_dma_ops.arm_coherent_dma_free,
>> .mmap = arm_coherent_dma_ops.arm_dma_mmap,
>> .get_sgtable = arm_coherent_dma_ops.arm_dma_get_sgtable,
>> .map_sg = arm_coherent_dma_ops.arm_dma_map_sg,
>> .set_dma_mask = arm_coherent_dma_ops.arm_dma_set_mask,
>> .map_page = armada_xp_dma_map_page,
>> .unmap_page = armada_xp_dma_unmap_page,
>> .unmap_sg = arm_dma_ops.unmap_sg,
>> .sync_single_for_cpu = armada_xp_dma_sync,
>> .sync_single_for_device = armada_xp_dma_sync,
>> .sync_sg_for_cpu = arm_dma_ops.sync_sg_for_cpu,
>> .sync_sg_for_device = arm_dma_ops.sync_sg_for_device,
>> };
>
> No, I was thinking of making the underlying functions globally visible
> and have extern declarations in a header file so you can access them
> directly.
>
> Generally speaking, when you run into a problem with common code, your
> first approach should be to fix the common code before you try to work
> around it.
OK I thought it was done on purpose. But if you consider it needs to be
fixed I will add patch for it in next version.
>
> Arnd
>
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 2/2] arm: mvebu: Add hardware I/O Coherency support
2012-10-24 13:56 ` Gregory CLEMENT
@ 2012-10-24 20:30 ` Arnd Bergmann
0 siblings, 0 replies; 15+ messages in thread
From: Arnd Bergmann @ 2012-10-24 20:30 UTC (permalink / raw)
To: Gregory CLEMENT
Cc: Lior Amsalem, Andrew Lunn, Ike Pan, Nadav Haklai, Ian Molton,
David Marlin, Yehuda Yitschak, Jani Monoses, Russell King,
Tawfik Bayouk, Dan Frazier, Eran Ben-Avi, Leif Lindholm,
Sebastian Hesselbarth, Jason Cooper, Jon Masters, Rob Herring,
Ben Dooks, linux-arm-kernel, Thomas Petazzoni, Chris Van Hoof,
Nicolas Pitre, linux-kernel, Maen Suleiman, Shadi Ammouri,
Olof Johansson
On Wednesday 24 October 2012, Gregory CLEMENT wrote:
> > No, I was thinking of making the underlying functions globally visible
> > and have extern declarations in a header file so you can access them
> > directly.
> >
> > Generally speaking, when you run into a problem with common code, your
> > first approach should be to fix the common code before you try to work
> > around it.
>
> OK I thought it was done on purpose. But if you consider it needs to be
> fixed I will add patch for it in next version.
As long as the functions were only used locally in one file, it's
better not to make them globally visible as a rule. But if we know
that they are needed, that should be made explicit. Please fix this
by submitting another patch to make those functions global and then
rebase this series on top of that.
Arnd
^ permalink raw reply [flat|nested] 15+ messages in thread
[parent not found: <20121024082529.GZ11837@lunn.ch>]
* Re: [PATCH 2/2] arm: mvebu: Add hardware I/O Coherency support
[not found] ` <20121024082529.GZ11837@lunn.ch>
@ 2012-10-24 11:50 ` Gregory CLEMENT
0 siblings, 0 replies; 15+ messages in thread
From: Gregory CLEMENT @ 2012-10-24 11:50 UTC (permalink / raw)
To: Andrew Lunn
Cc: Lior Amsalem, Ike Pan, Nadav Haklai, Ian Molton, David Marlin,
Yehuda Yitschak, Jani Monoses, Russell King, Tawfik Bayouk,
Dan Frazier, Eran Ben-Avi, Leif Lindholm, Sebastian Hesselbarth,
Jason Cooper, Arnd Bergmann, Jon Masters, Rob Herring, Ben Dooks,
linux-arm-kernel, Thomas Petazzoni, Chris Van Hoof,
Nicolas Pitre, linux-kernel, Maen Suleiman, Shadi Ammouri,
Olof Johansson
On 10/24/2012 10:25 AM, Andrew Lunn wrote:
> On Wed, Oct 24, 2012 at 10:04:01AM +0200, Gregory CLEMENT wrote:
>> Armada 370 and XP come with an unit called coherency fabric. This unit
>> allows to use the Armada XP as a nearly coherent architecture. The
>> coherency mechanism uses snoop filters to ensure the coherency between
>> caches, DRAM and devices. This mechanism needs a synchronization
>> barrier which guarantees that all memory write initiated by the
>> devices has reached their target and do not reside in intermediate
>> write buffers. That's why the architecture is not totally coherent and
>> we need to provide our own functions for some DMA operations.
>>
>> Beside the use of the coherency fabric, the device units will have to
>> set the attribute flag to select the accurate coherency process for
>> the memory transaction. This is done each device driver programs the
>> DRAM address windows. The value of the attribute set by the driver is
>> retrieved through the orion_addr_map_cfg struct filled during the
>> early initialization of the platform.
>>
>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>> Reviewed-by: Yehuda Yitschak <yehuday@marvell.com>
>> ---
>> arch/arm/boot/dts/armada-370-xp.dtsi | 3 +-
>> arch/arm/mach-mvebu/addr-map.c | 3 ++
>> arch/arm/mach-mvebu/armada-370-xp.c | 1 +
>> arch/arm/mach-mvebu/coherency.c | 87 ++++++++++++++++++++++++++++++++++
>> arch/arm/mach-mvebu/common.h | 2 +
>> 5 files changed, 95 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
>> index 18ba60b..af22e53 100644
>> --- a/arch/arm/boot/dts/armada-370-xp.dtsi
>> +++ b/arch/arm/boot/dts/armada-370-xp.dtsi
>> @@ -38,7 +38,8 @@
>>
>> coherency-fabric@d0020200 {
>> compatible = "marvell,coherency-fabric";
>> - reg = <0xd0020200 0xb0>;
>> + reg = <0xd0020200 0xb0>,
>> + <0xd0021010 0x1c>;
>> };
>
> ...
>
>> int __init armada_370_xp_coherency_init(void)
>> {
>> struct device_node *np;
>> @@ -82,7 +159,17 @@ int __init armada_370_xp_coherency_init(void)
>> if (np) {
>> pr_info("Initializing Coherency fabric\n");
>> coherency_base = of_iomap(np, 0);
>> + coherency_cpu_base = of_iomap(np, 1);
>
> Is this already in the binding documentation?
No indeed, the documentation should be completed. I will do it
the V2
>
> Thanks
> Andrew
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] arm: mvebu: Add hardware I/O Coherency support
2012-10-24 8:04 ` [PATCH 2/2] arm: mvebu: Add hardware I/O Coherency support Gregory CLEMENT
` (2 preceding siblings ...)
[not found] ` <20121024082529.GZ11837@lunn.ch>
@ 2012-10-24 12:27 ` Thomas Petazzoni
2012-10-24 14:39 ` Gregory CLEMENT
3 siblings, 1 reply; 15+ messages in thread
From: Thomas Petazzoni @ 2012-10-24 12:27 UTC (permalink / raw)
To: Gregory CLEMENT
Cc: Jason Cooper, Andrew Lunn, linux-arm-kernel, Arnd Bergmann,
Olof Johansson, Russell King, Rob Herring, Ben Dooks, Ian Molton,
Nicolas Pitre, Lior Amsalem, Maen Suleiman, Tawfik Bayouk,
Shadi Ammouri, Eran Ben-Avi, Yehuda Yitschak, Nadav Haklai,
Ike Pan, Jani Monoses, Chris Van Hoof, Dan Frazier,
Leif Lindholm, Jon Masters, David Marlin, Sebastian Hesselbarth,
linux-kernel
Hello,
On Wed, 24 Oct 2012 10:04:01 +0200, Gregory CLEMENT wrote:
> Armada 370 and XP come with an unit called coherency fabric. This unit
> allows to use the Armada XP as a nearly coherent architecture. The
the Armada 370/XP
> coherency mechanism uses snoop filters to ensure the coherency between
> caches, DRAM and devices. This mechanism needs a synchronization
> barrier which guarantees that all memory write initiated by the
write -> writes
> devices has reached their target and do not reside in intermediate
has -> have
> write buffers. That's why the architecture is not totally coherent and
> we need to provide our own functions for some DMA operations.
>
> Beside the use of the coherency fabric, the device units will have to
> set the attribute flag to select the accurate coherency process for
the attribute flag of what?
> the memory transaction. This is done each device driver programs the
> DRAM address windows. The value of the attribute set by the driver is
> retrieved through the orion_addr_map_cfg struct filled during the
> early initialization of the platform.
> diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
> index 18ba60b..af22e53 100644
> --- a/arch/arm/boot/dts/armada-370-xp.dtsi
> +++ b/arch/arm/boot/dts/armada-370-xp.dtsi
> @@ -38,7 +38,8 @@
>
> coherency-fabric@d0020200 {
> compatible = "marvell,coherency-fabric";
> - reg = <0xd0020200 0xb0>;
> + reg = <0xd0020200 0xb0>,
> + <0xd0021010 0x1c>;
> };
As others mentioned, documentation update needed.
>
> soc {
> diff --git a/arch/arm/mach-mvebu/addr-map.c b/arch/arm/mach-mvebu/addr-map.c
> index fe454a4..595f6b7 100644
> --- a/arch/arm/mach-mvebu/addr-map.c
> +++ b/arch/arm/mach-mvebu/addr-map.c
> @@ -108,6 +108,9 @@ static int __init armada_setup_cpu_mbus(void)
>
> addr_map_cfg.bridge_virt_base = mbus_unit_addr_decoding_base;
>
> + if (of_find_compatible_node(NULL, NULL, "marvell,coherency-fabric"))
> + addr_map_cfg.hw_io_coherency = 1;
> +
I don't really like to have this of_find_compatible_node(NULL, NULL,
"marvell,coherency-fabric") test in two places, but I don't see a way
around it since armada_setup_cpu_mbus() is called as an initcall, so we
can't pass arguments to it.
> +struct dma_map_ops armada_xp_dma_ops;
static
> +static inline void armada_xp_sync_io_barrier(void)
> +{
> + writel(0x1, coherency_cpu_base + IO_SYNC_BARRIER_CTL_OFFSET);
> + while (readl(coherency_cpu_base + IO_SYNC_BARRIER_CTL_OFFSET) & 0x1);
> +}
> +
> +dma_addr_t armada_xp_dma_map_page(struct device *dev, struct page *page,
> + unsigned long offset, size_t size,
> + enum dma_data_direction dir,
> + struct dma_attrs *attrs)
> +{
> + if (dir != DMA_TO_DEVICE)
> + armada_xp_sync_io_barrier();
> + return pfn_to_dma(dev, page_to_pfn(page)) + offset;
> +}
> +
> +
> +void armada_xp_dma_unmap_page(struct device *dev, dma_addr_t dma_handle,
> + size_t size, enum dma_data_direction dir,
> + struct dma_attrs *attrs)
> +{
> + if (dir != DMA_TO_DEVICE)
> + armada_xp_sync_io_barrier();
> +}
> +
> +void armada_xp_dma_sync(struct device *dev, dma_addr_t dma_handle,
> + size_t size, enum dma_data_direction dir)
> +{
> + if (dir != DMA_TO_DEVICE)
> + armada_xp_sync_io_barrier();
> +}
As others said, the prefix is wrong. Since the file is named coherency,
maybe just "coherency_" as the prefix? Not sure, though. Shouldn't the
file be named coherency-armada-370-xp.c, as we have done for the irq
controller file? In that case, the armada_370_xp prefix would make
sense.
> int armada_xp_get_cpu_count(void)
> {
> int reg, cnt;
static?
> @@ -74,6 +115,42 @@ int armada_370_xp_set_cpu_coherent(unsigned int hw_cpu_id, int smp_group_id)
> return 0;
> }
>
> +static int armada_xp_platform_notifier(struct notifier_block *nb,
> + unsigned long event, void *__dev)
> +{
> + struct device *dev = __dev;
> +
> + if (event != BUS_NOTIFY_ADD_DEVICE)
> + return NOTIFY_DONE;
> + set_dma_ops(dev, &armada_xp_dma_ops);
> +
> + return NOTIFY_OK;
> +}
> +
> +static struct notifier_block armada_xp_platform_nb = {
> + .notifier_call = armada_xp_platform_notifier,
> +};
> +
> +void __init armada_370_xp_coherency_iocache_init(void)
> +{
> + /* When the coherency fabric is available, the Armada XP and
> + * Aramada 370 are close to a coherent architecture, so we based
> + * our dma ops on the coherent one, and just changes the
> + * operations which need a arch io sync */
> + if (of_find_compatible_node(NULL, NULL, "marvell,coherency-fabric")) {
> + struct dma_map_ops *dma_ops = &armada_xp_dma_ops;
> + memcpy(dma_ops, &arm_coherent_dma_ops, sizeof(*dma_ops));
> + dma_ops->map_page = armada_xp_dma_map_page;
> + dma_ops->unmap_page = armada_xp_dma_unmap_page;
> + dma_ops->unmap_sg = arm_dma_ops.unmap_sg;
> + dma_ops->sync_single_for_cpu = armada_xp_dma_sync;
> + dma_ops->sync_single_for_device = armada_xp_dma_sync;
> + dma_ops->sync_sg_for_cpu = arm_dma_ops.sync_sg_for_cpu;
> + dma_ops->sync_sg_for_device = arm_dma_ops.sync_sg_for_device;
> + }
> + bus_register_notifier(&platform_bus_type, &armada_xp_platform_nb);
As Arnd said, I would prefer a lot to have the armada_xp_dma_ops
structure be initialized statically, even though it means that if the
arm_coherent_dma_ops structure is changed, we'll have to update here as
well. But I think it's cleaner.
Also, the bus notifier should be registered only if we have the
coherency fabric, otherwise it is anyway going to be called, setting
invalid DMA operations for all the platform devices.
So:
static dma_map_ops armada_370_xp_dma_ops = {
...
};
void __init armada_370_xp_coherency_iocache_init(void)
{
if (! of_find_compatible_node(NULL, NULL, "marvell,coherency-fabric"))
return;
bus_register_notifier(&platform_bus_type, &armada_370_xp_platform_nb);
}
> int __init armada_370_xp_coherency_init(void)
> {
> struct device_node *np;
> @@ -82,7 +159,17 @@ int __init armada_370_xp_coherency_init(void)
> if (np) {
> pr_info("Initializing Coherency fabric\n");
> coherency_base = of_iomap(np, 0);
> + coherency_cpu_base = of_iomap(np, 1);
> + }
> +#ifndef CONFIG_SMP
> + if (coherency_base) {
> + /* In UP case, cpu coherency is enabled here, in SMP case cpu
> + * coherency is enabled for each CPU by
> + * armada_xp_smp_prepare_cpus() in platsmp.c */
> + int hw_cpuid = cpu_logical_map(smp_processor_id());
> + armada_370_xp_set_cpu_coherent(hw_cpuid, 0);
> }
> +#endif
I really don't like this part of the code. First, the test is done on
"coherency_base", while armada_370_xp_coherency_iocache_init() is
checking the existence of the DT node to find if we have a coherency
fabric or not. It should be consistent.
Then, I don't see why the code patch for SMP and UP should be
different. The code in platsmp.c:armada_xp_smp_prepare_cpus() only
enables the coherency unit for the boot CPU. The other CPUs are
enabling the mechanism in the assembly code in headsmp.S. In other
words, I think you can remove the call to
armada_370_xp_set_cpu_coherent() in armada_xp_smp_prepare_cpus(), and
make the call unconditionally here in coherency.c for the boot CPU.
It seems like there is a better way to do this.
> diff --git a/arch/arm/mach-mvebu/common.h b/arch/arm/mach-mvebu/common.h
> index 86484bb..fff952e 100644
> --- a/arch/arm/mach-mvebu/common.h
> +++ b/arch/arm/mach-mvebu/common.h
> @@ -23,6 +23,8 @@ void armada_370_xp_handle_irq(struct pt_regs *regs);
>
> void armada_xp_cpu_die(unsigned int cpu);
>
> +void armada_370_xp_coherency_iocache_init(void);
> +
> int armada_370_xp_coherency_init(void);
> int armada_370_xp_pmsu_init(void);
> void armada_xp_secondary_startup(void);
Do we have a good reason for having
armada_370_xp_coherency_iocache_init() separate from
armada_370_xp_coherency_iocache_init() ? I.e, what prevents us from
registering the bus notifier directly in armada_370_xp_coherency_init()
if the coherency unit is available?
Thomas
--
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 2/2] arm: mvebu: Add hardware I/O Coherency support
2012-10-24 12:27 ` Thomas Petazzoni
@ 2012-10-24 14:39 ` Gregory CLEMENT
2012-10-24 14:56 ` Thomas Petazzoni
0 siblings, 1 reply; 15+ messages in thread
From: Gregory CLEMENT @ 2012-10-24 14:39 UTC (permalink / raw)
To: Thomas Petazzoni
Cc: Lior Amsalem, Andrew Lunn, Ike Pan, Nadav Haklai, Ian Molton,
David Marlin, Yehuda Yitschak, Jani Monoses, Russell King,
Tawfik Bayouk, Dan Frazier, Eran Ben-Avi, Leif Lindholm,
Sebastian Hesselbarth, Jason Cooper, Arnd Bergmann, Jon Masters,
Rob Herring, Ben Dooks, linux-arm-kernel, Chris Van Hoof,
Nicolas Pitre, linux-kernel, Maen Suleiman, Shadi Ammouri,
Olof Johansson
On 10/24/2012 02:27 PM, Thomas Petazzoni wrote:
[...]
I will fixed the spelling and complete the comments as suggested
[...]
>> +struct dma_map_ops armada_xp_dma_ops;
>
> static
OK
>
>> +static inline void armada_xp_sync_io_barrier(void)
>> +{
>> + writel(0x1, coherency_cpu_base + IO_SYNC_BARRIER_CTL_OFFSET);
>> + while (readl(coherency_cpu_base + IO_SYNC_BARRIER_CTL_OFFSET) & 0x1);
>> +}
>> +
>> +dma_addr_t armada_xp_dma_map_page(struct device *dev, struct page *page,
>> + unsigned long offset, size_t size,
>> + enum dma_data_direction dir,
>> + struct dma_attrs *attrs)
>> +{
>> + if (dir != DMA_TO_DEVICE)
>> + armada_xp_sync_io_barrier();
>> + return pfn_to_dma(dev, page_to_pfn(page)) + offset;
>> +}
>> +
>> +
>> +void armada_xp_dma_unmap_page(struct device *dev, dma_addr_t dma_handle,
>> + size_t size, enum dma_data_direction dir,
>> + struct dma_attrs *attrs)
>> +{
>> + if (dir != DMA_TO_DEVICE)
>> + armada_xp_sync_io_barrier();
>> +}
>> +
>> +void armada_xp_dma_sync(struct device *dev, dma_addr_t dma_handle,
>> + size_t size, enum dma_data_direction dir)
>> +{
>> + if (dir != DMA_TO_DEVICE)
>> + armada_xp_sync_io_barrier();
>> +}
>
> As others said, the prefix is wrong. Since the file is named coherency,
> maybe just "coherency_" as the prefix? Not sure, though. Shouldn't the
> file be named coherency-armada-370-xp.c, as we have done for the irq
> controller file? In that case, the armada_370_xp prefix would make
> sense
I would prefer to just use coherency_ everywhere and keep the name of
the file as is. It made sense to suffix the irq file with
armada-370-xp because the other mvebu SoCs have their own irq
controller. Whereas, as far as I know the coherency unit is only
present in the Armada 370/XP.
>
>> int armada_xp_get_cpu_count(void)
>> {
>> int reg, cnt;
>
> static?
Which function?
armada_xp_get_cpu_count and armada_370_xp_set_cpu_coherent are exported
and moreover are not part of this patch set but the SMP one.
>
>> @@ -74,6 +115,42 @@ int armada_370_xp_set_cpu_coherent(unsigned int hw_cpu_id, int smp_group_id)
>> return 0;
>> }
>>
>> +static int armada_xp_platform_notifier(struct notifier_block *nb,
>> + unsigned long event, void *__dev)
>> +{
>> + struct device *dev = __dev;
>> +
>> + if (event != BUS_NOTIFY_ADD_DEVICE)
>> + return NOTIFY_DONE;
>> + set_dma_ops(dev, &armada_xp_dma_ops);
>> +
>> + return NOTIFY_OK;
>> +}
>> +
>> +static struct notifier_block armada_xp_platform_nb = {
>> + .notifier_call = armada_xp_platform_notifier,
>> +};
>> +
>> +void __init armada_370_xp_coherency_iocache_init(void)
>> +{
>> + /* When the coherency fabric is available, the Armada XP and
>> + * Aramada 370 are close to a coherent architecture, so we based
>> + * our dma ops on the coherent one, and just changes the
>> + * operations which need a arch io sync */
>> + if (of_find_compatible_node(NULL, NULL, "marvell,coherency-fabric")) {
>> + struct dma_map_ops *dma_ops = &armada_xp_dma_ops;
>> + memcpy(dma_ops, &arm_coherent_dma_ops, sizeof(*dma_ops));
>> + dma_ops->map_page = armada_xp_dma_map_page;
>> + dma_ops->unmap_page = armada_xp_dma_unmap_page;
>> + dma_ops->unmap_sg = arm_dma_ops.unmap_sg;
>> + dma_ops->sync_single_for_cpu = armada_xp_dma_sync;
>> + dma_ops->sync_single_for_device = armada_xp_dma_sync;
>> + dma_ops->sync_sg_for_cpu = arm_dma_ops.sync_sg_for_cpu;
>> + dma_ops->sync_sg_for_device = arm_dma_ops.sync_sg_for_device;
>> + }
>> + bus_register_notifier(&platform_bus_type, &armada_xp_platform_nb);
>
> As Arnd said, I would prefer a lot to have the armada_xp_dma_ops
> structure be initialized statically, even though it means that if the
> arm_coherent_dma_ops structure is changed, we'll have to update here as
> well. But I think it's cleaner.
>
> Also, the bus notifier should be registered only if we have the
> coherency fabric, otherwise it is anyway going to be called, setting
> invalid DMA operations for all the platform devices.
>
> So:
>
> static dma_map_ops armada_370_xp_dma_ops = {
> ...
> };
>
> void __init armada_370_xp_coherency_iocache_init(void)
> {
> if (! of_find_compatible_node(NULL, NULL, "marvell,coherency-fabric"))
> return;
>
> bus_register_notifier(&platform_bus_type, &armada_370_xp_platform_nb);
> }
OK I agree
>
>> int __init armada_370_xp_coherency_init(void)
>> {
>> struct device_node *np;
>> @@ -82,7 +159,17 @@ int __init armada_370_xp_coherency_init(void)
>> if (np) {
>> pr_info("Initializing Coherency fabric\n");
>> coherency_base = of_iomap(np, 0);
>> + coherency_cpu_base = of_iomap(np, 1);
>> + }
>> +#ifndef CONFIG_SMP
>> + if (coherency_base) {
>> + /* In UP case, cpu coherency is enabled here, in SMP case cpu
>> + * coherency is enabled for each CPU by
>> + * armada_xp_smp_prepare_cpus() in platsmp.c */
>> + int hw_cpuid = cpu_logical_map(smp_processor_id());
>> + armada_370_xp_set_cpu_coherent(hw_cpuid, 0);
>> }
>> +#endif
>
> I really don't like this part of the code. First, the test is done on
> "coherency_base", while armada_370_xp_coherency_iocache_init() is
> checking the existence of the DT node to find if we have a coherency
> fabric or not. It should be consistent.
OK
>
> Then, I don't see why the code patch for SMP and UP should be
> different. The code in platsmp.c:armada_xp_smp_prepare_cpus() only
> enables the coherency unit for the boot CPU. The other CPUs are
> enabling the mechanism in the assembly code in headsmp.S. In other
> words, I think you can remove the call to
> armada_370_xp_set_cpu_coherent() in armada_xp_smp_prepare_cpus(), and
> make the call unconditionally here in coherency.c for the boot CPU.
>
> It seems like there is a better way to do this.
Yes indeed code in headsmp.S and armada_xp_smp_prepare_cpus() are
redundant we can simplify it. I will change it in the SMP series and
for this series also.
>
>> diff --git a/arch/arm/mach-mvebu/common.h b/arch/arm/mach-mvebu/common.h
>> index 86484bb..fff952e 100644
>> --- a/arch/arm/mach-mvebu/common.h
>> +++ b/arch/arm/mach-mvebu/common.h
>> @@ -23,6 +23,8 @@ void armada_370_xp_handle_irq(struct pt_regs *regs);
>>
>> void armada_xp_cpu_die(unsigned int cpu);
>>
>> +void armada_370_xp_coherency_iocache_init(void);
>> +
>> int armada_370_xp_coherency_init(void);
>> int armada_370_xp_pmsu_init(void);
>> void armada_xp_secondary_startup(void);
>
> Do we have a good reason for having
> armada_370_xp_coherency_iocache_init() separate from
> armada_370_xp_coherency_iocache_init() ? I.e, what prevents us from
No good reason because they are the same! ;)
But more seriously, armada_370_xp_coherency_init() is called as an
early_init call whereas armada_370_xp_coherency_iocache_init() is called
later by armada_370_xp_dt_init(). I have to check if we can use
bus_register_notifier() from an early_init call or if we still need to
call armada_370_xp_coherency_init() as an early_init call.
> registering the bus notifier directly in armada_370_xp_coherency_init()
> if the coherency unit is available?
Thanks,
Gregory
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 2/2] arm: mvebu: Add hardware I/O Coherency support
2012-10-24 14:39 ` Gregory CLEMENT
@ 2012-10-24 14:56 ` Thomas Petazzoni
0 siblings, 0 replies; 15+ messages in thread
From: Thomas Petazzoni @ 2012-10-24 14:56 UTC (permalink / raw)
To: Gregory CLEMENT
Cc: Lior Amsalem, Andrew Lunn, Ike Pan, Nadav Haklai, Ian Molton,
David Marlin, Yehuda Yitschak, Jani Monoses, Russell King,
Tawfik Bayouk, Dan Frazier, Eran Ben-Avi, Leif Lindholm,
Sebastian Hesselbarth, Jason Cooper, Arnd Bergmann, Jon Masters,
Rob Herring, Ben Dooks, linux-arm-kernel, Chris Van Hoof,
Nicolas Pitre, linux-kernel, Maen Suleiman, Shadi Ammouri,
Olof Johansson
Dear Gregory CLEMENT,
On Wed, 24 Oct 2012 16:39:13 +0200, Gregory CLEMENT wrote:
> > As others said, the prefix is wrong. Since the file is named coherency,
> > maybe just "coherency_" as the prefix? Not sure, though. Shouldn't the
> > file be named coherency-armada-370-xp.c, as we have done for the irq
> > controller file? In that case, the armada_370_xp prefix would make
> > sense
>
> I would prefer to just use coherency_ everywhere and keep the name of
> the file as is. It made sense to suffix the irq file with
> armada-370-xp because the other mvebu SoCs have their own irq
> controller. Whereas, as far as I know the coherency unit is only
> present in the Armada 370/XP.
Well your argumentation may also be seen as an argument to name it
coherency-armada-370-xp.c :-) I don't have a strong opinion on this
though, and we can always rename it later if needed.
> >> int armada_xp_get_cpu_count(void)
> >> {
> >> int reg, cnt;
> >
> > static?
>
> Which function?
> armada_xp_get_cpu_count and armada_370_xp_set_cpu_coherent are exported
> and moreover are not part of this patch set but the SMP one.
Ok, then maybe this function shouldn't be between the DMA operations
implementation and the bus notifier callback.
> Yes indeed code in headsmp.S and armada_xp_smp_prepare_cpus() are
> redundant we can simplify it. I will change it in the SMP series and
> for this series also.
Great, thanks!
> > Do we have a good reason for having
> > armada_370_xp_coherency_iocache_init() separate from
> > armada_370_xp_coherency_iocache_init() ? I.e, what prevents us from
>
> No good reason because they are the same! ;)
Oops, indeed. but I see that you fixed the problem.
> But more seriously, armada_370_xp_coherency_init() is called as an
> early_init call whereas armada_370_xp_coherency_iocache_init() is called
> later by armada_370_xp_dt_init(). I have to check if we can use
> bus_register_notifier() from an early_init call or if we still need to
> call armada_370_xp_coherency_init() as an early_init call.
The early_initcall() is mandatory, see the comment on top of it:
+/* Coherency initialization have to be done before the SMP
+ * initialization of the CPUs*/
+early_initcall(armada_370_xp_coherency_init);
(BTW, there is a missing space between CPUs and the comment terminator).
So, my point was more: is it possible to register the bus notifier
as early as inside an early_initcall() callback.
Best regards,
Thomas
--
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
^ permalink raw reply [flat|nested] 15+ messages in thread