amba: consolidate PrimeCell magic

Commit Message

boojin.kim Aug. 16, 2011, 7:50 a.m. UTC

Linus Walleij wrote:
>
> From: Linus Walleij <[e-mail]>
>
> 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 <[e-mail]>
> Cc: Kukjin Kim <[e-mail]>
> Cc: Viresh Kumar <[e-mail]>
> Signed-off-by: Linus Walleij <[e-mail]>
> ---
> 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 <[e-mail]>
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 <[e-mail]>
---
 arch/arm/common/pl330.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

--
1.7.1


Tested-by: Boojin Kim <[e-mail]>
with above on smdkc210 board

If my ack is required on this,
Acked-by: Boojin Kim <[e-mail]>

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

Comments

Linus Walleij Aug. 16, 2011, 8:36 a.m. UTC | #1

On Tue, Aug 16, 2011 at 9:50 AM, Boojin Kim <[e-mail]> 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

Linus Walleij Aug. 16, 2011, 9:25 a.m. UTC | #2

2011/8/16 Boojin Kim <[e-mail]>:

> 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

boojin.kim Aug. 17, 2011, 1:33 a.m. UTC | #3

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

Patch

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;
 	}