Message ID | 1313569846-22902-1-git-send-email-linus.walleij@stericsson.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Wed, Aug 17, 2011 at 2:00 PM, Linus Walleij <linus.walleij@stericsson.com> wrote: > From: Linus Walleij <linus.walleij@linaro.org> > > Since three drivers use the PrimeCell scheme without using the > amba_bus driver logic, let's break the magic lookups out as > static inlines in the <linux/amba/bus.h> header so we get > some consolidation anyway. Delete the primecell ID check in > common/pl330.c since it will soon only be used from the amba_bus > driver in drivers/dma, which is already doing the same check > when probing in drivers/amba/bus.c. > > Cc: Jassi Brar <jassisinghbrar@gmail.com> > Cc: Kukjin Kim <kgene.kim@samsung.com> > Cc: Viresh Kumar <viresh.kumar@st.com> > Cc: H Hartley Sweeten <hartleys@visionengravers.com> > Acked-by: Boojin Kim <boojin.kim@samsung.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> Acked-by: Jassi Brar <jassisinghbrar@gmail.com> thnx
On Wednesday, August 17, 2011 1:31 AM, Linus Walleij wrote: > > Since three drivers use the PrimeCell scheme without using the > amba_bus driver logic, let's break the magic lookups out as > static inlines in the <linux/amba/bus.h> header so we get > some consolidation anyway. Delete the primecell ID check in > common/pl330.c since it will soon only be used from the amba_bus > driver in drivers/dma, which is already doing the same check > when probing in drivers/amba/bus.c. > > Cc: Jassi Brar <jassisinghbrar@gmail.com> > Cc: Kukjin Kim <kgene.kim@samsung.com> > Cc: Viresh Kumar <viresh.kumar@st.com> > Cc: H Hartley Sweeten <hartleys@visionengravers.com> > Acked-by: Boojin Kim <boojin.kim@samsung.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > arch/arm/common/pl330.c | 40 +++------------------------------------- > arch/arm/common/vic.c | 15 ++++++++------- > drivers/amba/bus.c | 19 +++---------------- > drivers/dma/ste_dma40.c | 14 ++++---------- > drivers/mtd/nand/fsmc_nand.c | 10 +++------- > include/linux/amba/bus.h | 26 ++++++++++++++++++++++++++ > 6 files changed, 47 insertions(+), 77 deletions(-) Acked-by: H Hartley Sweeten <hsweeten@visionengravers.com> Linus, One comment on the change to vic.c. > diff --git a/arch/arm/common/vic.c b/arch/arm/common/vic.c > index 7aa4262..b163cdd 100644 > --- a/arch/arm/common/vic.c > +++ b/arch/arm/common/vic.c > @@ -341,16 +341,17 @@ static void __init vic_init_st(void __iomem *base, unsigned int irq_start, > void __init vic_init(void __iomem *base, unsigned int irq_start, > u32 vic_sources, u32 resume_sources) > { > - unsigned int i; > u32 cellid = 0; > enum amba_vendor vendor; > > - /* Identify which VIC cell this one is, by reading the ID */ > - for (i = 0; i < 4; i++) { > - u32 addr = ((u32)base & PAGE_MASK) + 0xfe0 + (i * 4); > - cellid |= (readl(addr) & 0xff) << (8 * i); > - } > - vendor = (cellid >> 12) & 0xff; > + /* > + * Identify which VIC cell this one is, by reading the ID - some > + * implementations have two VICs in the same page but only one set > + * of ID registers at the end, so we need to adjust the base to > + * reference the page offset. All VIC:s are size 4K. > + */ > + cellid = amba_get_pid((void __iomem *)((u32)base & PAGE_MASK), SZ_4K); This produces a sparse warning. But, the same warning was present with the old code. arch/arm/common/vic.c:353:49: warning: cast removes address space of expression If you want, this warning can be fixed by adding __force to the u32 cast: + cellid = amba_get_pid((void __iomem *)((u32 __force)base & PAGE_MASK), SZ_4K); But, if pushes the line a bit over 80 chars... > + vendor = AMBA_MANF_BITS(cellid); > printk(KERN_INFO "VIC @%p: id 0x%08x, vendor 0x%02x\n", > base, cellid, vendor); Regards, Hartley
On Wed, Aug 17, 2011 at 12:03:57PM -0500, H Hartley Sweeten wrote: > > + cellid = amba_get_pid((void __iomem *)((u32)base & PAGE_MASK), SZ_4K); > > This produces a sparse warning. But, the same warning was present with the > old code. > > arch/arm/common/vic.c:353:49: warning: cast removes address space of expression That's prabably because casting from a pointer to a u32 (unsigned int) isn't the best thing to do. Casting to unsigned long is much better, and iirc doesn't provoke such a sparse warning.
On Wed, Aug 17, 2011 at 8:49 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Wed, Aug 17, 2011 at 12:03:57PM -0500, H Hartley Sweeten wrote: >> > + cellid = amba_get_pid((void __iomem *)((u32)base & PAGE_MASK), SZ_4K); >> >> This produces a sparse warning. But, the same warning was present with the >> old code. >> >> arch/arm/common/vic.c:353:49: warning: cast removes address space of expression > > That's prabably because casting from a pointer to a u32 (unsigned int) > isn't the best thing to do. Casting to unsigned long is much better, > and iirc doesn't provoke such a sparse warning. Yep it's correct, sparse is silent on me now, posting v4. Thanks! Linus Walleij
On Wed, 2011-08-17 at 19:49 +0100, Russell King - ARM Linux wrote: > On Wed, Aug 17, 2011 at 12:03:57PM -0500, H Hartley Sweeten wrote: > > > + cellid = amba_get_pid((void __iomem *)((u32)base & PAGE_MASK), SZ_4K); > > > > This produces a sparse warning. But, the same warning was present with the > > old code. > > > > arch/arm/common/vic.c:353:49: warning: cast removes address space of expression > > That's prabably because casting from a pointer to a u32 (unsigned int) > isn't the best thing to do. Casting to unsigned long is much better, > and iirc doesn't provoke such a sparse warning. Is there any reason not to use uintptr_t for things like this?
On Thu, Aug 18, 2011 at 11:08:07AM +0100, Tixy wrote: > On Wed, 2011-08-17 at 19:49 +0100, Russell King - ARM Linux wrote: > > On Wed, Aug 17, 2011 at 12:03:57PM -0500, H Hartley Sweeten wrote: > > > > + cellid = amba_get_pid((void __iomem *)((u32)base & PAGE_MASK), SZ_4K); > > > > > > This produces a sparse warning. But, the same warning was present with the > > > old code. > > > > > > arch/arm/common/vic.c:353:49: warning: cast removes address space of expression > > > > That's prabably because casting from a pointer to a u32 (unsigned int) > > isn't the best thing to do. Casting to unsigned long is much better, > > and iirc doesn't provoke such a sparse warning. > > Is there any reason not to use uintptr_t for things like this? Probably not - it's just history that we've tended to use the base C types rather than the typedef'd stuff for this kind of thing.
diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c index 97912fa..623ce74 100644 --- a/arch/arm/common/pl330.c +++ b/arch/arm/common/pl330.c @@ -27,6 +27,7 @@ #include <linux/delay.h> #include <linux/interrupt.h> #include <linux/dma-mapping.h> +#include <linux/amba/bus.h> #include <asm/hardware/pl330.h> @@ -111,9 +112,6 @@ #define CR4 0xe10 #define CRD 0xe14 -#define PERIPH_ID 0xfe0 -#define PCELL_ID 0xff0 - #define CR0_PERIPH_REQ_SET (1 << 0) #define CR0_BOOT_EN_SET (1 << 1) #define CR0_BOOT_MAN_NS (1 << 2) @@ -142,14 +140,6 @@ #define CRD_DATA_BUFF_SHIFT 20 #define CRD_DATA_BUFF_MASK 0x3ff -#define PART 0x330 -#define DESIGNER 0x41 -#define REVISION 0x0 -#define INTEG_CFG 0x0 -#define PERIPH_ID_VAL ((PART << 0) | (DESIGNER << 12)) - -#define PCELL_ID_VAL 0xb105f00d - #define PL330_STATE_STOPPED (1 << 0) #define PL330_STATE_EXECUTING (1 << 1) #define PL330_STATE_WFE (1 << 2) @@ -372,19 +362,6 @@ static inline bool _manager_ns(struct pl330_thread *thrd) return (pl330->pinfo->pcfg.mode & DMAC_MODE_NS) ? true : false; } -static inline u32 get_id(struct pl330_info *pi, u32 off) -{ - void __iomem *regs = pi->base; - u32 id = 0; - - id |= (readb(regs + off + 0x0) << 0); - id |= (readb(regs + off + 0x4) << 8); - id |= (readb(regs + off + 0x8) << 16); - id |= (readb(regs + off + 0xc) << 24); - - return id; -} - static inline u32 _emit_ADDH(unsigned dry_run, u8 buf[], enum pl330_dst da, u16 val) { @@ -1747,8 +1724,8 @@ static void read_dmac_config(struct pl330_info *pi) pi->pcfg.irq_ns = readl(regs + CR3); - pi->pcfg.periph_id = get_id(pi, PERIPH_ID); - pi->pcfg.pcell_id = get_id(pi, PCELL_ID); + pi->pcfg.periph_id = amba_get_pid(pi->base, PCELL_SIZE); + pi->pcfg.pcell_id = amba_get_cid(pi->base, PCELL_SIZE); } static inline void _reset_thread(struct pl330_thread *thrd) @@ -1838,7 +1815,6 @@ static int dmac_alloc_resources(struct pl330_dmac *pl330) int pl330_add(struct pl330_info *pi) { struct pl330_dmac *pl330; - void __iomem *regs; int i, ret; if (!pi || !pi->dev) @@ -1855,16 +1831,6 @@ int pl330_add(struct pl330_info *pi) if (pi->dmac_reset) pi->dmac_reset(pi); - regs = pi->base; - - /* Check if we can handle this DMAC */ - if ((get_id(pi, PERIPH_ID) & 0xfffff) != PERIPH_ID_VAL - || get_id(pi, PCELL_ID) != PCELL_ID_VAL) { - dev_err(pi->dev, "PERIPH_ID 0x%x, PCELL_ID 0x%x !\n", - get_id(pi, PERIPH_ID), get_id(pi, PCELL_ID)); - return -EINVAL; - } - /* Read the configuration of the DMAC */ read_dmac_config(pi); diff --git a/arch/arm/common/vic.c b/arch/arm/common/vic.c index 7aa4262..b163cdd 100644 --- a/arch/arm/common/vic.c +++ b/arch/arm/common/vic.c @@ -341,16 +341,17 @@ static void __init vic_init_st(void __iomem *base, unsigned int irq_start, void __init vic_init(void __iomem *base, unsigned int irq_start, u32 vic_sources, u32 resume_sources) { - unsigned int i; u32 cellid = 0; enum amba_vendor vendor; - /* Identify which VIC cell this one is, by reading the ID */ - for (i = 0; i < 4; i++) { - u32 addr = ((u32)base & PAGE_MASK) + 0xfe0 + (i * 4); - cellid |= (readl(addr) & 0xff) << (8 * i); - } - vendor = (cellid >> 12) & 0xff; + /* + * Identify which VIC cell this one is, by reading the ID - some + * implementations have two VICs in the same page but only one set + * of ID registers at the end, so we need to adjust the base to + * reference the page offset. All VIC:s are size 4K. + */ + cellid = amba_get_pid((void __iomem *)((u32)base & PAGE_MASK), SZ_4K); + vendor = AMBA_MANF_BITS(cellid); printk(KERN_INFO "VIC @%p: id 0x%08x, vendor 0x%02x\n", base, cellid, vendor); diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c index d74926e..7412671 100644 --- a/drivers/amba/bus.c +++ b/drivers/amba/bus.c @@ -579,7 +579,7 @@ int amba_device_register(struct amba_device *dev, struct resource *parent) { u32 size; void __iomem *tmp; - int i, ret; + int ret; device_initialize(&dev->dev); @@ -620,24 +620,11 @@ int amba_device_register(struct amba_device *dev, struct resource *parent) ret = amba_get_enable_pclk(dev); if (ret == 0) { - u32 pid, cid; - - /* - * Read pid and cid based on size of resource - * they are located at end of region - */ - for (pid = 0, i = 0; i < 4; i++) - pid |= (readl(tmp + size - 0x20 + 4 * i) & 255) << - (i * 8); - for (cid = 0, i = 0; i < 4; i++) - cid |= (readl(tmp + size - 0x10 + 4 * i) & 255) << - (i * 8); + if (amba_get_cid(tmp, size) == AMBA_CID) + dev->periphid = amba_get_pid(tmp, size); amba_put_disable_pclk(dev); - if (cid == AMBA_CID) - dev->periphid = pid; - if (!dev->periphid) ret = -ENODEV; } diff --git a/drivers/dma/ste_dma40.c b/drivers/dma/ste_dma40.c index cd3a7c7..6e11cef 100644 --- a/drivers/dma/ste_dma40.c +++ b/drivers/dma/ste_dma40.c @@ -2569,7 +2569,6 @@ static struct d40_base * __init d40_hw_detect_init(struct platform_device *pdev) int num_phy_chans; int i; u32 pid; - u32 cid; u8 rev; clk = clk_get(&pdev->dev, NULL); @@ -2594,18 +2593,13 @@ static struct d40_base * __init d40_hw_detect_init(struct platform_device *pdev) if (!virtbase) goto failure; - /* This is just a regular AMBA PrimeCell ID actually */ - for (pid = 0, i = 0; i < 4; i++) - pid |= (readl(virtbase + resource_size(res) - 0x20 + 4 * i) - & 255) << (i * 8); - for (cid = 0, i = 0; i < 4; i++) - cid |= (readl(virtbase + resource_size(res) - 0x10 + 4 * i) - & 255) << (i * 8); - - if (cid != AMBA_CID) { + /* Device ID use the AMBA PrimeCell scheme */ + if (amba_get_cid(virtbase, resource_size(res)) != AMBA_CID) { d40_err(&pdev->dev, "Unknown hardware! No PrimeCell ID\n"); goto failure; } + + pid = amba_get_pid(virtbase, resource_size(res)); if (AMBA_MANF_BITS(pid) != AMBA_VENDOR_ST) { d40_err(&pdev->dev, "Unknown designer! Got %x wanted %x\n", AMBA_MANF_BITS(pid), diff --git a/drivers/mtd/nand/fsmc_nand.c b/drivers/mtd/nand/fsmc_nand.c index e9b275a..836de62 100644 --- a/drivers/mtd/nand/fsmc_nand.c +++ b/drivers/mtd/nand/fsmc_nand.c @@ -541,8 +541,6 @@ static int __init fsmc_nand_probe(struct platform_device *pdev) struct fsmc_regs *regs; struct resource *res; int ret = 0; - u32 pid; - int i; if (!pdata) { dev_err(&pdev->dev, "platform data is NULL\n"); @@ -636,13 +634,11 @@ static int __init fsmc_nand_probe(struct platform_device *pdev) * This device ID is actually a common AMBA ID as used on the * AMBA PrimeCell bus. However it is not a PrimeCell. */ - for (pid = 0, i = 0; i < 4; i++) - pid |= (readl(host->regs_va + resource_size(res) - 0x20 + 4 * i) & 255) << (i * 8); - host->pid = pid; + host->pid = amba_get_pid(host->regs_va, resource_size(res)); dev_info(&pdev->dev, "FSMC device partno %03x, manufacturer %02x, " "revision %02x, config %02x\n", - AMBA_PART_BITS(pid), AMBA_MANF_BITS(pid), - AMBA_REV_BITS(pid), AMBA_CONFIG_BITS(pid)); + AMBA_PART_BITS(host->pid), AMBA_MANF_BITS(host->pid), + AMBA_REV_BITS(host->pid), AMBA_CONFIG_BITS(host->pid)); host->bank = pdata->bank; host->select_chip = pdata->select_bank; diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h index fcbbe71..008cfcf 100644 --- a/include/linux/amba/bus.h +++ b/include/linux/amba/bus.h @@ -19,6 +19,7 @@ #include <linux/err.h> #include <linux/resource.h> #include <linux/regulator/consumer.h> +#include <linux/io.h> #define AMBA_NR_IRQS 2 #define AMBA_CID 0xb105f00d @@ -94,4 +95,29 @@ void amba_release_regions(struct amba_device *); #define amba_manf(d) AMBA_MANF_BITS((d)->periphid) #define amba_part(d) AMBA_PART_BITS((d)->periphid) +/* + * Inlines to extract the PID and CID for a certain PrimeCell. These are at + * offset -0x20 and -0x10 from the end of the I/O region respectively. + */ +static inline u32 amba_get_magic(void __iomem *base, u32 size, u8 offset) +{ + u32 magic; + int i; + + for (magic = 0, i = 0; i < 4; i++) + magic |= (readl(base + size - offset + 4 * i) & 255) + << (i * 8); + return magic; +} + +static inline u32 amba_get_pid(void __iomem *base, u32 size) +{ + return amba_get_magic(base, size, 0x20); +} + +static inline u32 amba_get_cid(void __iomem *base, u32 size) +{ + return amba_get_magic(base, size, 0x10); +} + #endif