[PATCH] amba: consolidate PrimeCell magic

[PATCH] amba: consolidate PrimeCell magic

[Linus Walleij]

From: Linus Walleij <linus.walleij@linaro.org>

Since two 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.

Cc: Boojin Kim <boojin.kim@samsung.com>
Cc: Kukjin Kim <kgene.kim@samsung.com>
Cc: Viresh Kumar <viresh.kumar@st.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
Samsung folks: I cannot find a defconfig that actually compiles
this driver in, can you help me testing it so I didn't break
anything, and Ack if it looks OK? Thanks.
---
 arch/arm/common/pl330.c      |   42 ++++++++++++------------------------------
 drivers/amba/bus.c           |   19 +++----------------
 drivers/dma/ste_dma40.c      |   14 ++++----------
 drivers/mtd/nand/fsmc_nand.c |   10 +++-------
 include/linux/amba/bus.h     |   26 ++++++++++++++++++++++++++
 5 files changed, 48 insertions(+), 63 deletions(-)

diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c
index 97912fa..2b3b8b3 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,8 +112,8 @@
 #define CR4		0xe10
 #define CRD		0xe14
 
-#define PERIPH_ID	0xfe0
-#define PCELL_ID	0xff0
+/* Used as end offset to retrieve PrimeCell ID registers */
+#define PCELL_SIZE	0x1000
 
 #define CR0_PERIPH_REQ_SET	(1 << 0)
 #define CR0_BOOT_EN_SET		(1 << 1)
@@ -143,12 +144,6 @@
 #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)
@@ -372,19 +367,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 +1729,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,8 +1820,8 @@ 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;
+	u32 cid, pid;
 
 	if (!pi || !pi->dev)
 		return -EINVAL;
@@ -1855,13 +1837,13 @@ 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));
+	cid = amba_get_cid(pi->base, PCELL_SIZE);
+	pid = amba_get_pid(pi->base, PCELL_SIZE);
+	if (cid != AMBA_CID ||
+	    amba_manf(pid) != AMBA_VENDOR_ARM ||
+	    amba_part(pid) != PART) {
+		dev_err(pi->dev, "PID: 0x%x, CID: 0x%x !\n", pid, cid);
 		return -EINVAL;
 	}
 
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
-- 
1.7.3.2

[PATCH] amba: consolidate PrimeCell magic

[Russell King - ARM Linux]

On Mon, Aug 15, 2011 at 11:55:04AM +0200, Linus Walleij wrote:
> From: Linus Walleij <linus.walleij@linaro.org>
> 
> Since two 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.
> 
> Cc: Boojin Kim <boojin.kim@samsung.com>
> Cc: Kukjin Kim <kgene.kim@samsung.com>
> Cc: Viresh Kumar <viresh.kumar@st.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Samsung folks: I cannot find a defconfig that actually compiles
> this driver in, can you help me testing it so I didn't break
> anything, and Ack if it looks OK? Thanks.

Why not convert these to use the amba_bus driver?

[PATCH] amba: consolidate PrimeCell magic

[Linus Walleij]

On Mon, Aug 15, 2011 at 12:00 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Aug 15, 2011 at 11:55:04AM +0200, Linus Walleij wrote:
>> From: Linus Walleij <linus.walleij@linaro.org>
>>
>> Since two 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.
>
> Why not convert these to use the amba_bus driver?

The PL330 bit is already a backend for the drivers/dma/pl330.c
driver which is amba_bus. However it can also be used in
isolation (IIRC), but I think Boojin is consolidating that driver
and once the common/pl330.c code can be moved to
drivers/dma/* it will be converted and these ID checks
dropped I guess.

As for drivers/dma/ste_dma40.c I think it has some
dependencies to the platform bus but I will look into it.

I fear there is a lot of stuff out there that have PrimeCell
ID's and could be using amba_bus actually :-(

Linus Walleij

[PATCH] amba: consolidate PrimeCell magic

[Boojin Kim]

Linus Walleij wrote:
>
> From: Linus Walleij <linus.walleij@linaro.org>
>
> Since two 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.
>
> Cc: Boojin Kim <boojin.kim@samsung.com>
> Cc: Kukjin Kim <kgene.kim@samsung.com>
> Cc: Viresh Kumar <viresh.kumar@st.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Samsung folks: I cannot find a defconfig that actually compiles
> this driver in, can you help me testing it so I didn't break
> anything, and Ack if it looks OK? Thanks.

Hello Linus,

I tested this patch on SMDKC210 but happened following build error with
exynos4_defconfig.

arch/arm/common/pl330.c: In function 'pl330_add':
arch/arm/common/pl330.c:1844: error: invalid type argument of '->' (have
'u32')
arch/arm/common/pl330.c:1845: error: invalid type argument of '->' (have
'u32')

So I added following.

From: Boojin Kim <boojin.kim@samsung.com>
Date: Tue, 16 Aug 2011 16:27:22 +0900
Subject: [PATCH] ARM: PL330: use amba_device structure to get amba
information

Signed-off-by: Boojin Kim <boojin.kim@samsung.com>
---
 arch/arm/common/pl330.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c
index 2b3b8b3..af6d96d 100644
--- a/arch/arm/common/pl330.c
+++ b/arch/arm/common/pl330.c
@@ -1822,6 +1822,8 @@ int pl330_add(struct pl330_info *pi)
 	struct pl330_dmac *pl330;
 	int i, ret;
 	u32 cid, pid;
+	struct amba_device *adev = container_of(pi->dev,
+					struct amba_device, dev);

 	if (!pi || !pi->dev)
 		return -EINVAL;
@@ -1841,8 +1843,8 @@ int pl330_add(struct pl330_info *pi)
 	cid = amba_get_cid(pi->base, PCELL_SIZE);
 	pid = amba_get_pid(pi->base, PCELL_SIZE);
 	if (cid != AMBA_CID ||
-	    amba_manf(pid) != AMBA_VENDOR_ARM ||
-	    amba_part(pid) != PART) {
+	    amba_manf(adev) != AMBA_VENDOR_ARM ||
+	    amba_part(adev) != PART) {
 		dev_err(pi->dev, "PID: 0x%x, CID: 0x%x !\n", pid, cid);
 		return -EINVAL;
 	}
--
1.7.1


Tested-by: Boojin Kim <boojin.Kim@samsung.com>
with above on smdkc210 board

If my ack is required on this,
Acked-by: Boojin Kim <boojin.Kim@samsung.com>

If you need more test, please kindly let me know.

[PATCH] amba: consolidate PrimeCell magic

[Jassi Brar]

On Mon, Aug 15, 2011 at 3:25 PM, Linus Walleij
<linus.walleij@stericsson.com> wrote:
> From: Linus Walleij <linus.walleij@linaro.org>
>
> Since two 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.
>
> Cc: Boojin Kim <boojin.kim@samsung.com>
> Cc: Kukjin Kim <kgene.kim@samsung.com>
> Cc: Viresh Kumar <viresh.kumar@st.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

You might also CC the author of relevant code. That of pl330.c in this case.

.....

> +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);
0xff looks much better than 255, esp when we play with bits.
Also you might simply use readb and drop the sieve ?
Anyways, nothing serious. You might choose to keep it as such
for you personal style.

Other than fixing the compilation breakage. Looks ok to me.

[PATCH] amba: consolidate PrimeCell magic

[Russell King - ARM Linux]

On Tue, Aug 16, 2011 at 01:57:08PM +0530, Jassi Brar wrote:
> On Mon, Aug 15, 2011 at 3:25 PM, Linus Walleij
> <linus.walleij@stericsson.com> wrote:
> > +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);
> 0xff looks much better than 255, esp when we play with bits.
> Also you might simply use readb and drop the sieve ?

You're making the assumption that using byte reads are permitted.  Given
that these are described as 32-bit registers, and some primecells require
reads of certain sizes, I believe the code to be the most correct solution.

[PATCH] amba: consolidate PrimeCell magic

[Linus Walleij]

On Tue, Aug 16, 2011 at 9:50 AM, Boojin Kim <boojin.kim@samsung.com> wrote:

> diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c
> index 2b3b8b3..af6d96d 100644
> --- a/arch/arm/common/pl330.c
> +++ b/arch/arm/common/pl330.c
> @@ -1822,6 +1822,8 @@ int pl330_add(struct pl330_info *pi)
> ? ? ? ?struct pl330_dmac *pl330;
> ? ? ? ?int i, ret;
> ? ? ? ?u32 cid, pid;
> + ? ? ? struct amba_device *adev = container_of(pi->dev,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct amba_device, dev);
>
> ? ? ? ?if (!pi || !pi->dev)
> ? ? ? ? ? ? ? ?return -EINVAL;
> @@ -1841,8 +1843,8 @@ int pl330_add(struct pl330_info *pi)
> ? ? ? ?cid = amba_get_cid(pi->base, PCELL_SIZE);
> ? ? ? ?pid = amba_get_pid(pi->base, PCELL_SIZE);
> ? ? ? ?if (cid != AMBA_CID ||
> - ? ? ? ? ? amba_manf(pid) != AMBA_VENDOR_ARM ||
> - ? ? ? ? ? amba_part(pid) != PART) {
> + ? ? ? ? ? amba_manf(adev) != AMBA_VENDOR_ARM ||
> + ? ? ? ? ? amba_part(adev) != PART) {
> ? ? ? ? ? ? ? ?dev_err(pi->dev, "PID: 0x%x, CID: 0x%x !\n", pid, cid);
> ? ? ? ? ? ? ? ?return -EINVAL;
> ? ? ? ?}

Ah! If I can access the adev from here it must have probed
earlier, and then the entire check is superfluous, since the AMBA
probe has already checked this in drivers/amba/bus.c.

I guess it should just be deleted then?

Linus

[PATCH] amba: consolidate PrimeCell magic

[Jassi Brar]

On 16 August 2011 14:05, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Aug 16, 2011 at 01:57:08PM +0530, Jassi Brar wrote:
>> On Mon, Aug 15, 2011 at 3:25 PM, Linus Walleij
>> <linus.walleij@stericsson.com> wrote:
>> > +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);
>> 0xff looks much better than 255, esp when we play with bits.
>> Also you might simply use readb and drop the sieve ?
>
> You're making the assumption that using byte reads are permitted. ?Given
> that these are described as 32-bit registers, and some primecells require
> reads of certain sizes, I believe the code to be the most correct solution.
>
If you notice the '?'
I already doubt that, so I simply asked Linus to ponder the possibility.

[PATCH] amba: consolidate PrimeCell magic

[Russell King - ARM Linux]

On Tue, Aug 16, 2011 at 02:08:18PM +0530, Jassi Brar wrote:
> On 16 August 2011 14:05, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Tue, Aug 16, 2011 at 01:57:08PM +0530, Jassi Brar wrote:
> >> On Mon, Aug 15, 2011 at 3:25 PM, Linus Walleij
> >> <linus.walleij@stericsson.com> wrote:
> >> > +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);
> >> 0xff looks much better than 255, esp when we play with bits.
> >> Also you might simply use readb and drop the sieve ?
> >
> > You're making the assumption that using byte reads are permitted. ?Given
> > that these are described as 32-bit registers, and some primecells require
> > reads of certain sizes, I believe the code to be the most correct solution.
> >
> If you notice the '?'
> I already doubt that, so I simply asked Linus to ponder the possibility.

Stop being an idiot.

If you notice, I'm giving you THE INFORMATION as to why its done that way.
I wrote the fucking code originally.  Don't you think I know what was
doing.

[PATCH] amba: consolidate PrimeCell magic

[Jassi Brar]

On Tue, Aug 16, 2011 at 2:09 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Aug 16, 2011 at 02:08:18PM +0530, Jassi Brar wrote:
>> On 16 August 2011 14:05, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> > On Tue, Aug 16, 2011 at 01:57:08PM +0530, Jassi Brar wrote:
>> >> On Mon, Aug 15, 2011 at 3:25 PM, Linus Walleij
>> >> <linus.walleij@stericsson.com> wrote:
>> >> > +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);
>> >> 0xff looks much better than 255, esp when we play with bits.
>> >> Also you might simply use readb and drop the sieve ?
>> >
>> > You're making the assumption that using byte reads are permitted. ?Given
>> > that these are described as 32-bit registers, and some primecells require
>> > reads of certain sizes, I believe the code to be the most correct solution.
>> >
>> If you notice the '?'
>> I already doubt that, so I simply asked Linus to ponder the possibility.
>
> Stop being an idiot.
>
Yup. Sorry, I misread your reply this time. Thanks for the info.

[PATCH] amba: consolidate PrimeCell magic

[Boojin Kim]

Linus Walleij [mailto:linus.walleij at linaro.org] wrote:
> Sent: Tuesday, August 16, 2011 5:36 PM
> To: Boojin Kim
> Cc: Linus Walleij; linux-arm-kernel at lists.infradead.org; Lee Jones;
> Kukjin Kim; Viresh Kumar
> Subject: Re: [PATCH] amba: consolidate PrimeCell magic
>
> On Tue, Aug 16, 2011 at 9:50 AM, Boojin Kim <boojin.kim@samsung.com>
> wrote:
>
> > diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c
> > index 2b3b8b3..af6d96d 100644
> > --- a/arch/arm/common/pl330.c
> > +++ b/arch/arm/common/pl330.c
> > @@ -1822,6 +1822,8 @@ int pl330_add(struct pl330_info *pi)
> > ? ? ? ?struct pl330_dmac *pl330;
> > ? ? ? ?int i, ret;
> > ? ? ? ?u32 cid, pid;
> > + ? ? ? struct amba_device *adev = container_of(pi->dev,
> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct amba_device, dev);
> >
> > ? ? ? ?if (!pi || !pi->dev)
> > ? ? ? ? ? ? ? ?return -EINVAL;
> > @@ -1841,8 +1843,8 @@ int pl330_add(struct pl330_info *pi)
> > ? ? ? ?cid = amba_get_cid(pi->base, PCELL_SIZE);
> > ? ? ? ?pid = amba_get_pid(pi->base, PCELL_SIZE);
> > ? ? ? ?if (cid != AMBA_CID ||
> > - ? ? ? ? ? amba_manf(pid) != AMBA_VENDOR_ARM ||
> > - ? ? ? ? ? amba_part(pid) != PART) {
> > + ? ? ? ? ? amba_manf(adev) != AMBA_VENDOR_ARM ||
> > + ? ? ? ? ? amba_part(adev) != PART) {
> > ? ? ? ? ? ? ? ?dev_err(pi->dev, "PID: 0x%x, CID: 0x%x !\n", pid, cid);
> > ? ? ? ? ? ? ? ?return -EINVAL;
> > ? ? ? ?}
>
> Ah! If I can access the adev from here it must have probed
> earlier, and then the entire check is superfluous, since the AMBA
> probe has already checked this in drivers/amba/bus.c.
>
> I guess it should just be deleted then?
Actually, I tested this patch after applying my patches that uses
'dma-pl330' driver instead of the samsung specific 's3c-pl330' driver.
'dma-pl330' driver is registered as 'amba_device'. So, this has already
checked on 'drivers/amba/bus.c' as your notice.
But, 's3c-pl330' driver is registered as 'platform_device'. So, This check
is required.
In my opinion, This check can be deleted if caller device is registered as
'amba_device'.

>
> Linus

[PATCH] amba: consolidate PrimeCell magic

[Linus Walleij]

2011/8/16 Boojin Kim <boojin.kim@samsung.com>:

> Actually, I tested this patch after applying my patches that uses
> 'dma-pl330' driver instead of the samsung specific 's3c-pl330' driver.
> 'dma-pl330' driver is registered as 'amba_device'. So, this has already
> checked on 'drivers/amba/bus.c' as your notice.
> But, 's3c-pl330' driver is registered as 'platform_device'. So, This check
> is required.
> In my opinion, This check can be deleted if caller device is registered as
> 'amba_device'.

My latest (v2) version of the patch deletes the check.

But doing that is dependent on the s3c-pl330 going away and
being replaced by the dmaengine driver, else I think we shall try to
keep the check (I will have to make it work without
dereferencing the amba_device or it will break).

Is it your plan to replace s3c-pl330 with the drivers/dma/pl330.c
driver, so I can safely delete this code?

Thanks,
Linus Walleij

[PATCH] amba: consolidate PrimeCell magic

[Boojin Kim]

Linus Walleij wrote:

> > Actually, I tested this patch after applying my patches that uses
> > 'dma-pl330' driver instead of the samsung specific 's3c-pl330'
> driver.
> > 'dma-pl330' driver is registered as 'amba_device'. So, this has
> already
> > checked on 'drivers/amba/bus.c' as your notice.
> > But, 's3c-pl330' driver is registered as 'platform_device'. So, This
> check
> > is required.
> > In my opinion, This check can be deleted if caller device is
> registered as
> > 'amba_device'.
>
> My latest (v2) version of the patch deletes the check.
>
> But doing that is dependent on the s3c-pl330 going away and
> being replaced by the dmaengine driver, else I think we shall try to
> keep the check (I will have to make it work without
> dereferencing the amba_device or it will break).
>
> Is it your plan to replace s3c-pl330 with the drivers/dma/pl330.c
> driver, so I can safely delete this code?
>
> Thanks,
> Linus Walleij

Yes, I made some patches that replaces s3c-pl330 with dma-pl330. And they
are waiting to merge mainline.
So, You can remove this code because the patches are forecasted to merge
soon.

Thanks,
Boojin Kim