Re: [PATCH v10 3/3] iommu/exynos: Add iommu driver for Exynos Platforms
On Wed, Mar 7, 2012 at 2:28 PM, InKi Dae wrote: >> +static irqreturn_t exynos_sysmmu_irq(int irq, void *dev_id) >> +{ >> + /* SYSMMU is in blocked when interrupt occurred. */ >> + struct sysmmu_drvdata *data = dev_id; >> + struct resource *irqres; >> + struct platform_device *pdev; >> + enum EXYNOS_SYSMMU_INTERRUPT_TYPE itype; > > how about to change EXYNOS_SYSMMU_INTERRUPT_TYPE to small letter? just > for code clean. > I will. please refer to the my reply to Kyungmin Park. >> + unsigned long addr = -1; >> + >> + int i, ret = -ENOSYS; >> + >> + read_lock(&data->lock); >> + >> + WARN_ON(!is_sysmmu_active(data)); >> + >> + pdev = to_platform_device(data->sysmmu); >> + for (i = 0; i < pdev->num_resources; i++) { >> + irqres = platform_get_resource(pdev, IORESOURCE_IRQ, i); >> + if (irqres && ((int)irqres->start == irq)) >> + break; >> + } > > is there any reason that it should get irq resources in interrupt > handler? if not so then how about to move it into probe()? > The platform device of ISP has a lot of IRQ sources. The above for clause determines which IRQ source (System MMU) generated the interrupt. >> + >> + if (i == pdev->num_resources) { >> + itype = SYSMMU_FAULT_UNKNOWN; >> + } else { >> + i /= 2; >> + >> + itype = (enum EXYNOS_SYSMMU_INTERRUPT_TYPE) > > ditto. > Thank you. Cho KyongHo. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 3/3] iommu/exynos: Add iommu driver for Exynos Platforms
> +static irqreturn_t exynos_sysmmu_irq(int irq, void *dev_id) > +{ > + /* SYSMMU is in blocked when interrupt occurred. */ > + struct sysmmu_drvdata *data = dev_id; > + struct resource *irqres; > + struct platform_device *pdev; > + enum EXYNOS_SYSMMU_INTERRUPT_TYPE itype; how about to change EXYNOS_SYSMMU_INTERRUPT_TYPE to small letter? just for code clean. > + unsigned long addr = -1; > + > + int i, ret = -ENOSYS; > + > + read_lock(&data->lock); > + > + WARN_ON(!is_sysmmu_active(data)); > + > + pdev = to_platform_device(data->sysmmu); > + for (i = 0; i < pdev->num_resources; i++) { > + irqres = platform_get_resource(pdev, IORESOURCE_IRQ, i); > + if (irqres && ((int)irqres->start == irq)) > + break; > + } is there any reason that it should get irq resources in interrupt handler? if not so then how about to move it into probe()? > + > + if (i == pdev->num_resources) { > + itype = SYSMMU_FAULT_UNKNOWN; > + } else { > + i /= 2; > + > + itype = (enum EXYNOS_SYSMMU_INTERRUPT_TYPE) ditto. > + __ffs(__raw_readl(data->sfrbases[i] + > REG_INT_STATUS)); > + if (WARN_ON(!((itype >= 0) && (itype < > SYSMMU_FAULT_UNKNOWN > + itype = SYSMMU_FAULT_UNKNOWN; > + else > + addr = __raw_readl( > + data->sfrbases[i] + fault_reg_offset[itype]); > + } > + -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 3/3] iommu/exynos: Add iommu driver for Exynos Platforms
On Tue, Mar 6, 2012 at 10:13 PM, Kyungmin Park wrote: > On Tue, Mar 6, 2012 at 5:31 PM, KyongHo Cho wrote: >> HAALgBjAGgAbwBAAHMAYQBtAHMAdQBuAGcALgBjAG8AbQA=;Tue, >> 06 Mar 2012 08:31:29 >> GMT;WwBQAEEAVABDAEgAIAB2ADEAMAAgADMALwAzAF0AIABpAG8AbQBtAHUALwBlAHgAeQBuAG8AcwA6ACAAQQBkAGQAIABpAG8AbQBtAHUAIABkAHIAaQB2AGUAcgAgAGYAbwByACAARQB4AHkAbgBvAHMAIABQAGwAYQB0AGYAbwByAG0AcwA= >> x-cr-puzzleid: {CF0AAF04-8639-4D69-B4E1-BF71EA1B0A70} > What's this? I think it is inserted by Outlook and it was not found in the email editor before sending this patch. I am unable to use sendmail due to security reason in my company. I will find a way not to insert the above message. Thank you. >> >> This is the System MMU driver and IOMMU API implementation for >> Exynos SOC platforms. Exynos platforms has more than 10 System >> MMUs dedicated for each multimedia accellerators. >> >> The System MMU driver is already in arc/arm/plat-s5p but it is >> moved to drivers/iommu due to Ohad Ben-Cohen gathered IOMMU drivers >> there >> >> Any device driver in Exynos platforms that needs to control its >> System MMU must call platform_set_sysmmu() to inform System MMU >> driver who will control it. >> platform_set_sysmmu() is defined in > Does it really required? if we can remove it. it can be compiled > without arch dependency. The function must be called while board initialization and all source files for the purpose is placed in 'mach' directory. Thus device driver is not dependent on arch except the definition of data structure conveyed in device.platform_data. Since our System MMUs are attached to each peripheral devices, it is required to define relationships between System MMUs and peripheral devices. platform_set_sysmmu() defines the relationship. >> + >> +#ifdef CONFIG_EXYNOS_IOMMU_DEBUG >> +#define DEBUG >> +#endif > If it's used only at here. you can add it at Makefile. > > ifeq ($(CONFIG_EXYNOS_IOMMU_DEBUG),y) > CFLAGS-exynos-iommu += -DDEBUG > endif > > Of course you can select as your preference. Thank you for advice. >> + >> +#define lv2ent_fault(pent) ((*(pent) & 3) == 0) >> +#define lv2ent_small(pent) ((*(pent) & 2) == 2) > #define lv2ent_small(pent) ((*(pent) & 3) == 2)? The last bit is 'don't-care'. It is intentionally ignored in the above comparison. >> + >> +#define REG_MMU_CTRL 0x000 >> +#define REG_MMU_CFG 0x004 >> +#define REG_MMU_STATUS 0x008 >> +#define REG_MMU_FLUSH 0x00C >> +#define REG_MMU_FLUSH_ENTRY 0x010 >> +#define REG_PT_BASE_ADDR 0x014 >> +#define REG_INT_STATUS 0x018 >> +#define REG_INT_CLEAR 0x01C >> +#define REG_MMU_VERSION 0x034 > Please place it in order. I found that a mistake while defining the offsets of control registers. I will fix it. >> + >> +enum EXYNOS_SYSMMU_INTERRUPT_TYPE { >> + SYSMMU_PAGEFAULT, >> + SYSMMU_AR_MULTIHIT, >> + SYSMMU_AW_MULTIHIT, >> + SYSMMU_BUSERROR, >> + SYSMMU_AR_SECURITY, >> + SYSMMU_AR_ACCESS, >> + SYSMMU_AW_SECURITY, >> + SYSMMU_AW_PROTECTION, /* 7 */ >> + SYSMMU_FAULT_UNKNOWN, >> + SYSMMU_FAULTS_NUM > missing comma, "SYSMMU_FAULTS_NUM," Yes it is. I don't think that it is needed to appending comma after the last element unless it is predictable that adding elements after the last element in the future. >> +}; >> + >> +typedef int (*sysmmu_fault_handler_t)(enum EXYNOS_SYSMMU_INTERRUPT_TYPE >> itype, >> + unsigned long pgtable_base, unsigned long >> fault_addr); >> + >> +static unsigned short fault_reg_offset[SYSMMU_FAULTS_NUM] = { >> + REG_PAGE_FAULT_ADDR, >> + REG_AR_FAULT_ADDR, >> + REG_AW_FAULT_ADDR, >> + REG_DEFAULT_SLAVE_ADDR, >> + REG_AR_FAULT_ADDR, >> + REG_AR_FAULT_ADDR, >> + REG_AW_FAULT_ADDR, >> + REG_AW_FAULT_ADDR > ditto. Answered above. >> +}; >> + >> +static char *sysmmu_fault_name[SYSMMU_FAULTS_NUM] = { >> + "PAGE FAULT", >> + "AR MULTI-HIT FAULT", >> + "AW MULTI-HIT FAULT", >> + "BUS ERROR", >> + "AR SECURITY PROTECTION FAULT", >> + "AR ACCESS PROTECTION FAULT", >> + "AW SECURITY PROTECTION FAULT", >> + "AW ACCESS PROTECTION FAULT", >> + "UNKNOWN FAULT" > ditto. Answered above. >> +void exynos_sysmmu_set_prefbuf(struct device *dev, >> + unsigned long base0, unsigned long size0, >> + unsigned long base1, unsigned long size1) >> +{ >> + struct sysmmu_drvdata *data = dev_get_drvdata(dev->archdata.iommu); >> + unsigned long flags; >> + int i; >> + >> + BUG_ON((base0 + size0) <= base0); >> + BUG_ON((size1 > 0) && ((base1 + size1) <= base1)); > It's bogus check. it's impossible. Do you think overflow case? Yes I do. It is prepared to detect invalid parameters from device drivers. >> + >> + read_lock_irqsave(&data->lock, flags); >> + if (!is_sysmmu_active(data)) >> +
Re: [PATCH v10 3/3] iommu/exynos: Add iommu driver for Exynos Platforms
Hi, some not critical comments. Thank you, Kyungmin Park On Tue, Mar 6, 2012 at 5:31 PM, KyongHo Cho wrote: > HAALgBjAGgAbwBAAHMAYQBtAHMAdQBuAGcALgBjAG8AbQA=;Tue, > 06 Mar 2012 08:31:29 > GMT;WwBQAEEAVABDAEgAIAB2ADEAMAAgADMALwAzAF0AIABpAG8AbQBtAHUALwBlAHgAeQBuAG8AcwA6ACAAQQBkAGQAIABpAG8AbQBtAHUAIABkAHIAaQB2AGUAcgAgAGYAbwByACAARQB4AHkAbgBvAHMAIABQAGwAYQB0AGYAbwByAG0AcwA= > x-cr-puzzleid: {CF0AAF04-8639-4D69-B4E1-BF71EA1B0A70} What's this? > > This is the System MMU driver and IOMMU API implementation for > Exynos SOC platforms. Exynos platforms has more than 10 System > MMUs dedicated for each multimedia accellerators. > > The System MMU driver is already in arc/arm/plat-s5p but it is > moved to drivers/iommu due to Ohad Ben-Cohen gathered IOMMU drivers > there > > Any device driver in Exynos platforms that needs to control its > System MMU must call platform_set_sysmmu() to inform System MMU > driver who will control it. > platform_set_sysmmu() is defined in Does it really required? if we can remove it. it can be compiled without arch dependency. > > Cc: Joerg Roedel > Cc: Kukjin Kim > Signed-off-by: KyongHo Cho > --- > drivers/iommu/Kconfig | 21 + > drivers/iommu/Makefile | 1 + > drivers/iommu/exynos-iommu.c | 1088 > ++ > 3 files changed, 1110 insertions(+), 0 deletions(-) > create mode 100644 drivers/iommu/exynos-iommu.c > > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig > index 6bea696..25d3eed 100644 > --- a/drivers/iommu/Kconfig > +++ b/drivers/iommu/Kconfig > @@ -142,4 +142,25 @@ config OMAP_IOMMU_DEBUG > > Say N unless you know you need this. > > +config EXYNOS_IOMMU > + bool "Exynos IOMMU Support" > + depends on EXYNOS_DEV_SYSMMU > + select IOMMU_API > + help > + Support for the IOMMU(System MMU) of Samsung Exynos application > + processor family. This enables H/W multimedia accellerators to see > + non-linear physical memory chunks as a linear memory in their > + address spaces > + > + If unsure, say N here. > + > +config EXYNOS_IOMMU_DEBUG > + bool "Debugging log for Exynos IOMMU" > + depends on EXYNOS_IOMMU > + help > + Select this to see the detailed log message that shows what > + happens in the IOMMU driver > + > + Say N unless you need kernel log message for IOMMU debugging > + > endif # IOMMU_SUPPORT > diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile > index 0e36b49..5a8fd97 100644 > --- a/drivers/iommu/Makefile > +++ b/drivers/iommu/Makefile > @@ -8,3 +8,4 @@ obj-$(CONFIG_IRQ_REMAP) += intr_remapping.o > obj-$(CONFIG_OMAP_IOMMU) += omap-iommu.o > obj-$(CONFIG_OMAP_IOVMM) += omap-iovmm.o > obj-$(CONFIG_OMAP_IOMMU_DEBUG) += omap-iommu-debug.o > +obj-$(CONFIG_EXYNOS_IOMMU) += exynos-iommu.o > diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c > new file mode 100644 > index 000..4c1c48a > --- /dev/null > +++ b/drivers/iommu/exynos-iommu.c > @@ -0,0 +1,1088 @@ > +/* linux/drivers/iommu/exynos_iommu.c > + * > + * Copyright (c) 2011 Samsung Electronics Co., Ltd. > + * http://www.samsung.com > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#ifdef CONFIG_EXYNOS_IOMMU_DEBUG > +#define DEBUG > +#endif If it's used only at here. you can add it at Makefile. ifeq ($(CONFIG_EXYNOS_IOMMU_DEBUG),y) CFLAGS-exynos-iommu+= -DDEBUG endif Of course you can select as your preference. > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > + > +#include > + > +#define S5P_MMU_CTRL 0x000 > +#define S5P_MMU_CFG 0x004 > +#define S5P_MMU_STATUS 0x008 > +#define S5P_MMU_FLUSH 0x00C > +#define S5P_PT_BASE_ADDR 0x014 > +#define S5P_INT_STATUS 0x018 > +#define S5P_INT_CLEAR 0x01C > +#define S5P_PAGE_FAULT_ADDR 0x024 > +#define S5P_AW_FAULT_ADDR 0x028 > +#define S5P_AR_FAULT_ADDR 0x02C > +#define S5P_DEFAULT_SLAVE_ADDR 0x030 > +#define S5P_MMU_VERSION 0x034 > +#define S5P_PB0_SADDR 0x04C > +#define S5P_PB0_EADDR 0x050 > +#define S5P_PB1_SADDR 0x054 > +#define S5P_PB1_EADDR 0x058 > + > +#define SECT_ORDER 20 > +#define LPAGE_ORDER 16 > +#define SPAGE_ORDER 12 > + > +#define SECT_SIZE (1 << SECT_ORDER) > +#define LPAGE_SIZE (1 << LPAGE_ORDER) > +#define SPAGE_SIZE (1 << SPAGE_ORDER) > + > +#define SECT_MASK (~(SECT_SIZE - 1)) > +#define LPAGE_MASK (~(LPAGE_SIZE - 1)) > +#define SPAGE_MASK (~(SPAG