[PATCH] ARM: PL330: Accept revisions of the dmac

[PATCH] ARM: PL330: Accept revisions of the dmac

[Jassi Brar]

From: Jassi Brar <jassi.brar@samsung.com>

The driver can handle different revisions of the core which
vary only minorly. So check for only the necessary part of
the IDs.

Signed-off-by: Jassi Brar <jassi.brar@samsung.com>
---
 arch/arm/common/pl330.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c
index 5ebbab6..8f0f86d 100644
--- a/arch/arm/common/pl330.c
+++ b/arch/arm/common/pl330.c
@@ -146,8 +146,7 @@
 #define DESIGNER	0x41
 #define REVISION	0x0
 #define INTEG_CFG	0x0
-#define PERIPH_ID_VAL	((PART << 0) | (DESIGNER << 12) \
-			  | (REVISION << 20) | (INTEG_CFG << 24))
+#define PERIPH_ID_VAL	((PART << 0) | (DESIGNER << 12))
 
 #define PCELL_ID_VAL	0xb105f00d
 
@@ -1859,10 +1858,10 @@ int pl330_add(struct pl330_info *pi)
 	regs = pi->base;
 
 	/* Check if we can handle this DMAC */
-	if (get_id(pi, PERIPH_ID) != PERIPH_ID_VAL
+	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",
-			readl(regs + PERIPH_ID), readl(regs + PCELL_ID));
+			get_id(pi, PERIPH_ID), get_id(pi, PCELL_ID));
 		return -EINVAL;
 	}
 
-- 
1.6.2.5

[PATCH] ARM: PL330: Accept revisions of the dmac

[Kukjin Kim]

Jassi Brar wrote:
> 
> From: Jassi Brar <jassi.brar@samsung.com>
> 
> The driver can handle different revisions of the core which
> vary only minorly. So check for only the necessary part of
> the IDs.
> 
> Signed-off-by: Jassi Brar <jassi.brar@samsung.com>
> ---
>  arch/arm/common/pl330.c |    7 +++----
>  1 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c
> index 5ebbab6..8f0f86d 100644
> --- a/arch/arm/common/pl330.c
> +++ b/arch/arm/common/pl330.c
> @@ -146,8 +146,7 @@
>  #define DESIGNER	0x41
>  #define REVISION	0x0
>  #define INTEG_CFG	0x0
> -#define PERIPH_ID_VAL	((PART << 0) | (DESIGNER << 12) \
> -			  | (REVISION << 20) | (INTEG_CFG << 24))
> +#define PERIPH_ID_VAL	((PART << 0) | (DESIGNER << 12))
> 
>  #define PCELL_ID_VAL	0xb105f00d
> 
> @@ -1859,10 +1858,10 @@ int pl330_add(struct pl330_info *pi)
>  	regs = pi->base;
> 
>  	/* Check if we can handle this DMAC */
> -	if (get_id(pi, PERIPH_ID) != PERIPH_ID_VAL
> +	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",
> -			readl(regs + PERIPH_ID), readl(regs + PCELL_ID));
> +			get_id(pi, PERIPH_ID), get_id(pi, PCELL_ID));

mm...really needed to change readl to get_id?

According to PL330 TRM 3-48, seems that PERIPH_ID includes all of other
PERIPH_ID_X's information.
And PCELL_ID is same...

But I'm not sure whether there is any update...

>  		return -EINVAL;
>  	}
> 
> --
> 1.6.2.5


Thanks.

Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.

[PATCH] ARM: PL330: Accept revisions of the dmac

[Jassi Brar]

On Sat, Sep 4, 2010 at 7:43 AM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> Jassi Brar wrote:
>>
>> From: Jassi Brar <jassi.brar@samsung.com>
>>
>> The driver can handle different revisions of the core which
>> vary only minorly. So check for only the necessary part of
>> the IDs.
>>
>> Signed-off-by: Jassi Brar <jassi.brar@samsung.com>
>> ---
>> ?arch/arm/common/pl330.c | ? ?7 +++----
>> ?1 files changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c
>> index 5ebbab6..8f0f86d 100644
>> --- a/arch/arm/common/pl330.c
>> +++ b/arch/arm/common/pl330.c
>> @@ -146,8 +146,7 @@
>> ?#define DESIGNER ? ? 0x41
>> ?#define REVISION ? ? 0x0
>> ?#define INTEG_CFG ? ?0x0
>> -#define PERIPH_ID_VAL ? ? ? ?((PART << 0) | (DESIGNER << 12) \
>> - ? ? ? ? ? ? ? ? ? ? ? | (REVISION << 20) | (INTEG_CFG << 24))
>> +#define PERIPH_ID_VAL ? ? ? ?((PART << 0) | (DESIGNER << 12))
>>
>> ?#define PCELL_ID_VAL 0xb105f00d
>>
>> @@ -1859,10 +1858,10 @@ int pl330_add(struct pl330_info *pi)
>> ? ? ? regs = pi->base;
>>
>> ? ? ? /* Check if we can handle this DMAC */
>> - ? ? if (get_id(pi, PERIPH_ID) != PERIPH_ID_VAL
>> + ? ? 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",
>> - ? ? ? ? ? ? ? ? ? ? readl(regs + PERIPH_ID), readl(regs + PCELL_ID));
>> + ? ? ? ? ? ? ? ? ? ? get_id(pi, PERIPH_ID), get_id(pi, PCELL_ID));
>
> mm...really needed to change readl to get_id?

Yes, get_id will read regs and encode the values to provide real IDs.


>
> According to PL330 TRM 3-48, seems that PERIPH_ID includes all of other
> PERIPH_ID_X's information.
> And PCELL_ID is same...
>
> But I'm not sure whether there is any update...
Our newer SoCs, C210 and V310 have revised versions and does not work without
this patch.

[PATCH] ARM: PL330: Accept revisions of the dmac

[Linus Walleij]

2010/9/3 Jassi Brar <jassisinghbrar@gmail.com>:

> -#define PERIPH_ID_VAL ?((PART << 0) | (DESIGNER << 12) \
> - ? ? ? ? ? ? ? ? ? ? ? ? | (REVISION << 20) | (INTEG_CFG << 24))
> +#define PERIPH_ID_VAL ?((PART << 0) | (DESIGNER << 12))
>
> ?#define PCELL_ID_VAL ? 0xb105f00d

Jassi, this kind of macros already exist in <linux/amba/bus.h>
can't you just #include that and use these instead?

Notice that you have the ID encoded in drivers/dma/pl330.c as
well so can you also update the mask/add a few ID:s to
pl330_ids[] in that file when you change this?

Yours,
Linus Walleij

[PATCH] ARM: PL330: Accept revisions of the dmac

[Jassi Brar]

On Sun, Sep 5, 2010 at 6:14 PM, Linus Walleij
<linus.ml.walleij@gmail.com> wrote:
> 2010/9/3 Jassi Brar <jassisinghbrar@gmail.com>:
>
>> -#define PERIPH_ID_VAL ?((PART << 0) | (DESIGNER << 12) \
>> - ? ? ? ? ? ? ? ? ? ? ? ? | (REVISION << 20) | (INTEG_CFG << 24))
>> +#define PERIPH_ID_VAL ?((PART << 0) | (DESIGNER << 12))
>>
>> ?#define PCELL_ID_VAL ? 0xb105f00d
>
> Jassi, this kind of macros already exist in <linux/amba/bus.h>
> can't you just #include that and use these instead?
You mean to use amba_config/rev et al ?
But they work on struct amba_device *, and I have to form those
structs just to make use of the macros. How useful would that be?
Or I got you wrong?
Thanks.

[PATCH] ARM: PL330: Accept revisions of the dmac

[Linus Walleij]

2010/9/5 Jassi Brar <jassisinghbrar@gmail.com>:
> [Me]
>> Jassi, this kind of macros already exist in <linux/amba/bus.h>
>> can't you just #include that and use these instead?
> You mean to use amba_config/rev et al ?
>
> But they work on struct amba_device *, and I have to form those
> structs just to make use of the macros.

Ah I get it...

But maybe we should refactor <linux/amba/bus.h> then?
Something like this:

From: Linus Walleij <linus.walleij@stericsson.com>
Date: Mon, 6 Sep 2010 10:29:26 +0200
Subject: [PATCH] ARM: move the PrimeCell IDs to use macros

This make four macros for the PrimeCell ID register available to
drivers that use them witout using the PrimeCell/AMBA bus
abstraction and struct amba_device. It also moves the magic
PrimeCell CID "B105F00D" to the bus.h header file.

Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
---
 drivers/amba/bus.c       |    2 +-
 include/linux/amba/bus.h |   15 +++++++++++----
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index d31590e..2737b97 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -298,7 +298,7 @@ int amba_device_register(struct amba_device *dev,
struct resource *parent)

                amba_put_disable_pclk(dev);

-               if (cid == 0xb105f00d)
+               if (cid == AMBA_CID)
                        dev->periphid = pid;

                if (!dev->periphid)
diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
index b0c1740..4c0f5a3 100644
--- a/include/linux/amba/bus.h
+++ b/include/linux/amba/bus.h
@@ -20,6 +20,7 @@
 #include <linux/resource.h>

 #define AMBA_NR_IRQS   2
+#define AMBA_CID       0xb105f00d

 struct clk;

@@ -70,9 +71,15 @@ void amba_release_regions(struct amba_device *);
 #define amba_pclk_disable(d)   \
        do { if (!IS_ERR((d)->pclk)) clk_disable((d)->pclk); } while (0)

-#define amba_config(d) (((d)->periphid >> 24) & 0xff)
-#define amba_rev(d)    (((d)->periphid >> 20) & 0x0f)
-#define amba_manf(d)   (((d)->periphid >> 12) & 0xff)
-#define amba_part(d)   ((d)->periphid & 0xfff)
+/* Some drivers don't use the struct amba_device */
+#define AMBA_CONFIG_BITS(a) ((a >> 24) & 0xff)
+#define AMBA_REV_BITS(a) ((a >> 20) & 0x0f)
+#define AMBA_MANF_BITS(a) ((a >> 12) & 0xff)
+#define AMBA_PART_BITS(a) (a & 0xfff)
+
+#define amba_config(d) AMBA_CONFIG_BITS((d)->periphid)
+#define amba_rev(d)    AMBA_REV_BITS((d)->periphid)
+#define amba_manf(d)   AMBA_MANF_BITS((d)->periphid)
+#define amba_part(d)   AMBA_PART_BITS((d)->periphid)

 #endif
-- 
1.7.2.2

Then you can use AMBA_CONFIG_BITS() etc.

If Russell likes it I'll put it in the patch tracker.

Come to think about it, we have this enum in the bus.h:

enum amba_vendor {
        AMBA_VENDOR_ARM = 0x41,
        AMBA_VENDOR_ST = 0x80,
};

What byte value is Samsung using for vendor?

Yours,
Linus Walleij

[PATCH] ARM: PL330: Accept revisions of the dmac

[Jassi Brar]

On Mon, Sep 6, 2010 at 5:35 PM, Linus Walleij
<linus.ml.walleij@gmail.com> wrote:
> 2010/9/5 Jassi Brar <jassisinghbrar@gmail.com>:
>> [Me]
>>> Jassi, this kind of macros already exist in <linux/amba/bus.h>
>>> can't you just #include that and use these instead?
>> You mean to use amba_config/rev et al ?
>>
>> But they work on struct amba_device *, and I have to form those
>> structs just to make use of the macros.
>
> Ah I get it...
>
> But maybe we should refactor <linux/amba/bus.h> then?
> Something like this:
>
> From: Linus Walleij <linus.walleij@stericsson.com>
> Date: Mon, 6 Sep 2010 10:29:26 +0200
> Subject: [PATCH] ARM: move the PrimeCell IDs to use macros
>
> This make four macros for the PrimeCell ID register available to
> drivers that use them witout using the PrimeCell/AMBA bus
> abstraction and struct amba_device. It also moves the magic
> PrimeCell CID "B105F00D" to the bus.h header file.
>
> Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
> ---
> ?drivers/amba/bus.c ? ? ? | ? ?2 +-
> ?include/linux/amba/bus.h | ? 15 +++++++++++----
> ?2 files changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index d31590e..2737b97 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -298,7 +298,7 @@ int amba_device_register(struct amba_device *dev,
> struct resource *parent)
>
> ? ? ? ? ? ? ? ?amba_put_disable_pclk(dev);
>
> - ? ? ? ? ? ? ? if (cid == 0xb105f00d)
> + ? ? ? ? ? ? ? if (cid == AMBA_CID)
> ? ? ? ? ? ? ? ? ? ? ? ?dev->periphid = pid;
>
> ? ? ? ? ? ? ? ?if (!dev->periphid)
> diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
> index b0c1740..4c0f5a3 100644
> --- a/include/linux/amba/bus.h
> +++ b/include/linux/amba/bus.h
> @@ -20,6 +20,7 @@
> ?#include <linux/resource.h>
>
> ?#define AMBA_NR_IRQS ? 2
> +#define AMBA_CID ? ? ? 0xb105f00d
>
> ?struct clk;
>
> @@ -70,9 +71,15 @@ void amba_release_regions(struct amba_device *);
> ?#define amba_pclk_disable(d) ? \
> ? ? ? ?do { if (!IS_ERR((d)->pclk)) clk_disable((d)->pclk); } while (0)
>
> -#define amba_config(d) (((d)->periphid >> 24) & 0xff)
> -#define amba_rev(d) ? ?(((d)->periphid >> 20) & 0x0f)
> -#define amba_manf(d) ? (((d)->periphid >> 12) & 0xff)
> -#define amba_part(d) ? ((d)->periphid & 0xfff)
> +/* Some drivers don't use the struct amba_device */
> +#define AMBA_CONFIG_BITS(a) ((a >> 24) & 0xff)
> +#define AMBA_REV_BITS(a) ((a >> 20) & 0x0f)
> +#define AMBA_MANF_BITS(a) ((a >> 12) & 0xff)
> +#define AMBA_PART_BITS(a) (a & 0xfff)
> +
> +#define amba_config(d) AMBA_CONFIG_BITS((d)->periphid)
> +#define amba_rev(d) ? ?AMBA_REV_BITS((d)->periphid)
> +#define amba_manf(d) ? AMBA_MANF_BITS((d)->periphid)
> +#define amba_part(d) ? AMBA_PART_BITS((d)->periphid)

Sorry I missed that I need to read periph_id[0,3] regs and form
Peripheral-ID from them,
These macros do the reverse - extract fields from the Peripheral-ID.
btw, you might want to close argument 'a' in parens as in ((a) & 0xfff) ?

........

>
> enum amba_vendor {
> ? ? ? ?AMBA_VENDOR_ARM = 0x41,
> ? ? ? ?AMBA_VENDOR_ST = 0x80,
> };
>
> What byte value is Samsung using for vendor?
I haven't come across it in any device so far. I have only seen
ARM IPs being used.

[PATCH] ARM: PL330: Accept revisions of the dmac

[Linus Walleij]

2010/9/7 Jassi Brar <jassisinghbrar@gmail.com>:

>> +#define amba_config(d) AMBA_CONFIG_BITS((d)->periphid)
>> +#define amba_rev(d) ? ?AMBA_REV_BITS((d)->periphid)
>> +#define amba_manf(d) ? AMBA_MANF_BITS((d)->periphid)
>> +#define amba_part(d) ? AMBA_PART_BITS((d)->periphid)
>
> Sorry I missed that I need to read periph_id[0,3] regs and form
> Peripheral-ID from them,
> These macros do the reverse - extract fields from the Peripheral-ID.
> btw, you might want to close argument 'a' in parens as in ((a) & 0xfff) ?

Oh OK I'll fix the macros and put the patch for Russells review in the
patch tracker anyway, atleast getting the B105F00D magic defined
somewhere can be helpful.

Yours,
Linus Walleij

[PATCH] ARM: PL330: Accept revisions of the dmac

[Linus Walleij]

2010/9/7 Linus Walleij <linus.ml.walleij@gmail.com>:
> 2010/9/7 Jassi Brar <jassisinghbrar@gmail.com>:
>
>>> +#define amba_config(d) AMBA_CONFIG_BITS((d)->periphid)
>>> +#define amba_rev(d) ? ?AMBA_REV_BITS((d)->periphid)
>>> +#define amba_manf(d) ? AMBA_MANF_BITS((d)->periphid)
>>> +#define amba_part(d) ? AMBA_PART_BITS((d)->periphid)
>>
>> Sorry I missed that I need to read periph_id[0,3] regs and form
>> Peripheral-ID from them,
>> These macros do the reverse - extract fields from the Peripheral-ID.
>> btw, you might want to close argument 'a' in parens as in ((a) & 0xfff) ?
>
> Oh OK I'll fix the macros and put the patch for Russells review in the
> patch tracker anyway, atleast getting the B105F00D magic defined
> somewhere can be helpful.

Russell has merged the apropriate field extraction macros to
<linux/amba/bus.h> so now you can use these, please respin
patch 6367/1 on top of that.

Yours,
Linus Walleij