On Monday 18 April 2011, Marek Szyprowski wrote:
> From: Andrzej Pietrasiewicz <andrzej.p@xxxxxxxxxxx>
>
> This patch performs a complete rewrite of sysmmu driver for Samsung platform:
> - simplified the resource management: no more single platform
> device with 32 resources is needed, better fits into linux driver model,
> each sysmmu instance has it's own resource definition
> - the new version uses kernel wide common iommu api defined in include/iommu.h
> - cleaned support for sysmmu clocks
> - added support for custom fault handlers and tlb replacement policy
Looks like good progress, but I fear that there is still quite a bit more
work needed here.
> +static int debug;
> +module_param(debug, int, 0644);
> +
> +#define sysmmu_debug(level, fmt, arg...) \
> + do { \
> + if (debug >= level) \
> + printk(KERN_DEBUG "[%s] " fmt, __func__, ## arg);\
> + } while (0)
Just use dev_dbg() here, the kernel already has all the infrastructure.
> +
> +#define generic_extract(l, s, entry) \
> + ((entry) & l##LPT_##s##_MASK)
> +#define flpt_get_1m(entry) generic_extract(F, 1M, deref_va(entry))
> +#define flpt_get_16m(entry) generic_extract(F, 16M, deref_va(entry))
> +#define slpt_get_4k(entry) generic_extract(S, 4K, deref_va(entry))
> +#define slpt_get_64k(entry) generic_extract(S, 64K, deref_va(entry))
> +
> +#define generic_entry(l, s, entry) \
> + (generic_extract(l, s, entry) | PAGE_##s)
> +#define flpt_ent_4k_64k(entry) generic_entry(F, 4K_64K, entry)
> +#define flpt_ent_1m(entry) generic_entry(F, 1M, entry)
> +#define flpt_ent_16m(entry) generic_entry(F, 16M, entry)
> +#define slpt_ent_4k(entry) generic_entry(S, 4K, entry)
> +#define slpt_ent_64k(entry) generic_entry(S, 64K, entry)
> +
> +#define page_4k_64k(entry) (deref_va(entry) & PAGE_4K_64K)
> +#define page_1m(entry) (deref_va(entry) & PAGE_1M)
> +#define page_16m(entry) ((deref_va(entry) & PAGE_16M) == PAGE_16M)
> +#define page_4k(entry) (deref_va(entry) & PAGE_4K)
> +#define page_64k(entry) (deref_va(entry) & PAGE_64K)
> +
> +#define generic_pg_offs(l, s, va) \
> + (va & ~l##LPT_##s##_MASK)
> +#define pg_offs_1m(va) generic_pg_offs(F, 1M, va)
> +#define pg_offs_16m(va) generic_pg_offs(F, 16M, va)
> +#define pg_offs_4k(va) generic_pg_offs(S, 4K, va)
> +#define pg_offs_64k(va) generic_pg_offs(S, 64K, va)
> +
> +#define flpt_index(va) (((va) >> FLPT_IDX_SHIFT) & FLPT_IDX_MASK)
> +
> +#define generic_offset(l, va) (((va) >> l##LPT_OFFS_SHIFT) & l##LPT_OFFS_MASK)
> +#define flpt_offs(va) generic_offset(F, va)
> +#define slpt_offs(va) generic_offset(S, va)
> +
> +#define invalidate_slpt_ent(slpt_va) (deref_va(slpt_va) = 0UL)
> +
> +#define get_irq_callb(cb) \
> + (s5p_domain->irq_callb ? \
> + (s5p_domain->irq_callb->cb ? \
> + s5p_domain->irq_callb->cb : \
> + s5p_sysmmu_irq_callb.cb) \
> + : s5p_sysmmu_irq_callb.cb)
These macros are really confusing, especially the ones that implicitly
access variables with a specific name. How about converting them into
inline functions?
> +phys_addr_t s5p_iova_to_phys(struct iommu_domain *domain, unsigned long iova)
This should be static.
> +struct device *s5p_sysmmu_get(enum s5p_sysmmu_ip ip)
> +{
> + struct device *ret = NULL;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&sysmmu_slock, flags);
> + if (sysmmu_table[ip]) {
> + try_module_get(THIS_MODULE);
> + ret = sysmmu_table[ip]->dev;
> + }
> + spin_unlock_irqrestore(&sysmmu_slock, flags);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(s5p_sysmmu_get);
> +
> +void s5p_sysmmu_put(void *dev)
> +{
> + BUG_ON(!dev);
> + module_put(THIS_MODULE);
> +}
> +EXPORT_SYMBOL_GPL(s5p_sysmmu_put);
These look wrong for a number of reasons:
* try_module_get(THIS_MODULE) makes no sense at all, the idea of the
try_module_get is to pin down another module that was calling down,
which I suppose is not needed here.
* This extends the generic IOMMU API in platform specific ways, don't
do that.
* I think you can do without these functions by including a pointer
to the iommu structure in dev_archdata, see
arch/powerpc/include/asm/device.h for an example.
> +void s5p_sysmmu_domain_irq_callb(struct iommu_domain *domain,
> + struct s5p_sysmmu_irq_callb *ops, void *priv)
> +{
> + struct s5p_sysmmu_domain *s5p_domain = domain->priv;
> + s5p_domain->irq_callb = ops;
> + s5p_domain->irq_callb_priv = priv;
> +}
> +EXPORT_SYMBOL(s5p_sysmmu_domain_irq_callb);
> +
> +
> +void s5p_sysmmu_domain_tlb_policy(struct iommu_domain *domain, int policy)
> +{
> + struct s5p_sysmmu_domain *s5p_domain = domain->priv;
> + s5p_domain->policy = policy;
> +}
> +EXPORT_SYMBOL(s5p_sysmmu_domain_tlb_policy);
More private extensions that should not be here. Better extend the generic
IOMMU API to contain callbacks for these if they are required, and document
them in a generic location.
> +static void s5p_sysmmu_irq_page_fault(struct iommu_domain *domain, int reason,
> + unsigned long addr, void *priv)
> +{
> + sysmmu_debug(3, "%s: Faulting virtual address: 0x%08lx\n",
> + irq_reasons[reason], addr);
> + BUG();
> +}
> +
> +static void s5p_sysmmu_irq_generic_callb(struct iommu_domain *domain,
> + int reason, unsigned long addr,
> + void *priv)
> +{
> + sysmmu_debug(3, "%s\n", irq_reasons[reason]);
> + BUG();
> +}
Why BUG() here? The backtrace of an interrupt handler should be rather
uninteresting, and you just end up in panic() since this is run
from an interrupt handler.
> +static struct s5p_sysmmu_irq_callb s5p_sysmmu_irq_callb = {
> + .page_fault = s5p_sysmmu_irq_page_fault,
> + .ar_fault = s5p_sysmmu_irq_generic_callb,
> + .aw_fault = s5p_sysmmu_irq_generic_callb,
> + .bus_error = s5p_sysmmu_irq_generic_callb,
> + .ar_security = s5p_sysmmu_irq_generic_callb,
> + .ar_prot = s5p_sysmmu_irq_generic_callb,
> + .aw_security = s5p_sysmmu_irq_generic_callb,
> + .aw_prot = s5p_sysmmu_irq_generic_callb,
> +};
> +
> +static irqreturn_t s5p_sysmmu_irq(int irq, void *dev_id)
> +{
> + struct s5p_sysmmu_info *sysmmu = dev_id;
> + struct s5p_sysmmu_domain *s5p_domain = sysmmu->domain->priv;
> + unsigned int reg_INT_STATUS;
> +
> + if (false == sysmmu->enabled)
> + return IRQ_HANDLED;
> +
> + reg_INT_STATUS = readl(sysmmu->regs + S5P_INT_STATUS);
> + if (reg_INT_STATUS & 0xFF) {
> + S5P_IRQ_CB(cb);
> + enum s5p_sysmmu_fault reason = 0;
> + unsigned long fault = 0;
> + unsigned reg = 0;
> + cb = NULL;
> + switch (reg_INT_STATUS & 0xFF) {
> + case 0x1:
> + cb = get_irq_callb(page_fault);
> + reason = S5P_SYSMMU_PAGE_FAULT;
> + reg = S5P_PAGE_FAULT_ADDR;
> + break;
> + case 0x2:
> + cb = get_irq_callb(ar_fault);
> + reason = S5P_SYSMMU_AR_FAULT;
> + reg = S5P_AR_FAULT_ADDR;
> + break;
> + case 0x4:
> + cb = get_irq_callb(aw_fault);
> + reason = S5P_SYSMMU_AW_FAULT;
> + reg = S5P_AW_FAULT_ADDR;
> + break;
> + case 0x8:
> + cb = get_irq_callb(bus_error);
> + reason = S5P_SYSMMU_BUS_ERROR;
> + /* register common to page fault and bus error */
> + reg = S5P_PAGE_FAULT_ADDR;
> + break;
> + case 0x10:
> + cb = get_irq_callb(ar_security);
> + reason = S5P_SYSMMU_AR_SECURITY;
> + reg = S5P_AR_FAULT_ADDR;
> + break;
> + case 0x20:
> + cb = get_irq_callb(ar_prot);
> + reason = S5P_SYSMMU_AR_PROT;
> + reg = S5P_AR_FAULT_ADDR;
> + break;
> + case 0x40:
> + cb = get_irq_callb(aw_security);
> + reason = S5P_SYSMMU_AW_SECURITY;
> + reg = S5P_AW_FAULT_ADDR;
> + break;
> + case 0x80:
> + cb = get_irq_callb(aw_prot);
> + reason = S5P_SYSMMU_AW_PROT;
> + reg = S5P_AW_FAULT_ADDR;
> + break;
> + }
> + fault = readl(sysmmu->regs + reg);
> + cb(sysmmu->domain, reason, fault, s5p_domain->irq_callb_priv);
> + writel(reg_INT_STATUS, sysmmu->regs + S5P_INT_CLEAR);
> + }
> + return IRQ_HANDLED;
> +}
I think it would be more readable and more efficient to just use a lookup
table here instead of the long switch/case statement.
> +static int
> +s5p_sysmmu_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> + int ret = 0;
> + sysmmu_debug(3, "begin\n");
> +
> + return ret;
> +}
> +
> +static int s5p_sysmmu_resume(struct platform_device *pdev)
> +{
> + int ret = 0;
> + sysmmu_debug(3, "begin\n");
> +
> + return ret;
> +}
> +
> +static int s5p_sysmmu_runtime_suspend(struct device *dev)
> +{
> + sysmmu_debug(3, "begin\n");
> + return 0;
> +}
> +
> +static int s5p_sysmmu_runtime_resume(struct device *dev)
> +{
> + sysmmu_debug(3, "begin\n");
> + return 0;
> +}
Why even provide these when they don't do anything?
> +static int __init
> +s5p_sysmmu_register(void)
> +{
> + int ret;
> +
> + sysmmu_debug(3, "Registering sysmmu driver...\n");
> +
> + slpt_cache = kmem_cache_create("slpt_cache", 1024, 1024,
> + SLAB_HWCACHE_ALIGN, NULL);
> + if (!slpt_cache) {
> + printk(KERN_ERR
> + "%s: failed to allocated slpt cache\n", __func__);
> + return -ENOMEM;
> + }
> +
> + ret = platform_driver_register(&s5p_sysmmu_driver);
> +
> + if (ret) {
> + printk(KERN_ERR
> + "%s: failed to register sysmmu driver\n", __func__);
> + return -EINVAL;
> + }
> +
> + register_iommu(&s5p_sysmmu_ops);
> +
> + return ret;
> +}
When you register the iommu unconditionally, it becomes impossible for
this driver to coexist with other iommu drivers in the same kernel,
which does against the concept of having a platform driver for this.
It might be better to call the s5p_sysmmu_register function from
the board files and have no platform devices at all if each IOMMU
is always bound to a specific device anyway.
> diff --git a/arch/arm/plat-samsung/include/plat/devs.h b/arch/arm/plat-samsung/include/plat/devs.h
> index f0da6b7..0ae5dd0 100644
> --- a/arch/arm/plat-samsung/include/plat/devs.h
> +++ b/arch/arm/plat-samsung/include/plat/devs.h
> @@ -142,7 +142,7 @@ extern struct platform_device s5p_device_fimc3;
> extern struct platform_device s5p_device_mipi_csis0;
> extern struct platform_device s5p_device_mipi_csis1;
>
> -extern struct platform_device exynos4_device_sysmmu;
> +extern struct platform_device exynos4_device_sysmmu[];
Why is this a global variable? I would expect this to be private to the
implementation.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
