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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/