* [PATCH] mmc: dw_mmc: modify DATA register offset
@ 2011-10-17 7:05 Jaehoon Chung
2011-10-17 8:27 ` James Hogan
0 siblings, 1 reply; 6+ messages in thread
From: Jaehoon Chung @ 2011-10-17 7:05 UTC (permalink / raw)
To: linux-mmc; +Cc: Chris Ball, Kyungmin Park, Will Newton, James Hogan
In dw_mmc 2.40a spec, Data register's offset is changed.
Now we used Data register offset is 0x100. but if somebody use 2.40a
controller, must use 0x200 for Data register.
This patch is added version-id checking point and using SDMMC_DATA(x)
instead of SDMMC_DATA. (assume 2.40a is the latest version)
Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
drivers/mmc/host/dw_mmc.c | 66 ++++++++++++++++++++++++++++++--------------
drivers/mmc/host/dw_mmc.h | 10 ++++++-
include/linux/mmc/dw_mmc.h | 2 +
3 files changed, 56 insertions(+), 22 deletions(-)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 701f14e..3aaeb08 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1043,7 +1043,8 @@ static void dw_mci_push_data16(struct dw_mci *host, void *buf, int cnt)
buf += len;
cnt -= len;
if (!sg_next(host->sg) || host->part_buf_count == 2) {
- mci_writew(host, DATA, host->part_buf16);
+ mci_writew(host, DATA(host->data_offset),
+ host->part_buf16);
host->part_buf_count = 0;
}
}
@@ -1060,21 +1061,23 @@ static void dw_mci_push_data16(struct dw_mci *host, void *buf, int cnt)
cnt -= len;
/* push data from aligned buffer into fifo */
for (i = 0; i < items; ++i)
- mci_writew(host, DATA, aligned_buf[i]);
+ mci_writew(host, DATA(host->data_offset),
+ aligned_buf[i]);
}
} else
#endif
{
u16 *pdata = buf;
for (; cnt >= 2; cnt -= 2)
- mci_writew(host, DATA, *pdata++);
+ mci_writew(host, DATA(host->data_offset), *pdata++);
buf = pdata;
}
/* put anything remaining in the part_buf */
if (cnt) {
dw_mci_set_part_bytes(host, buf, cnt);
if (!sg_next(host->sg))
- mci_writew(host, DATA, host->part_buf16);
+ mci_writew(host, DATA(host->data_offset),
+ host->part_buf16);
}
}
@@ -1089,7 +1092,8 @@ static void dw_mci_pull_data16(struct dw_mci *host, void *buf, int cnt)
int items = len >> 1;
int i;
for (i = 0; i < items; ++i)
- aligned_buf[i] = mci_readw(host, DATA);
+ aligned_buf[i] = mci_readw(host,
+ DATA(host->data_offset));
/* memcpy from aligned buffer into output buffer */
memcpy(buf, aligned_buf, len);
buf += len;
@@ -1100,11 +1104,11 @@ static void dw_mci_pull_data16(struct dw_mci *host, void *buf, int cnt)
{
u16 *pdata = buf;
for (; cnt >= 2; cnt -= 2)
- *pdata++ = mci_readw(host, DATA);
+ *pdata++ = mci_readw(host, DATA(host->data_offset));
buf = pdata;
}
if (cnt) {
- host->part_buf16 = mci_readw(host, DATA);
+ host->part_buf16 = mci_readw(host, DATA(host->data_offset));
dw_mci_pull_final_bytes(host, buf, cnt);
}
}
@@ -1117,7 +1121,8 @@ static void dw_mci_push_data32(struct dw_mci *host, void *buf, int cnt)
buf += len;
cnt -= len;
if (!sg_next(host->sg) || host->part_buf_count == 4) {
- mci_writel(host, DATA, host->part_buf32);
+ mci_writel(host, DATA(host->data_offset),
+ host->part_buf32);
host->part_buf_count = 0;
}
}
@@ -1134,21 +1139,23 @@ static void dw_mci_push_data32(struct dw_mci *host, void *buf, int cnt)
cnt -= len;
/* push data from aligned buffer into fifo */
for (i = 0; i < items; ++i)
- mci_writel(host, DATA, aligned_buf[i]);
+ mci_writel(host, DATA(host->data_offset),
+ aligned_buf[i]);
}
} else
#endif
{
u32 *pdata = buf;
for (; cnt >= 4; cnt -= 4)
- mci_writel(host, DATA, *pdata++);
+ mci_writel(host, DATA(host->data_offset), *pdata++);
buf = pdata;
}
/* put anything remaining in the part_buf */
if (cnt) {
dw_mci_set_part_bytes(host, buf, cnt);
if (!sg_next(host->sg))
- mci_writel(host, DATA, host->part_buf32);
+ mci_writel(host, DATA(host->data_offset),
+ host->part_buf32);
}
}
@@ -1163,7 +1170,8 @@ static void dw_mci_pull_data32(struct dw_mci *host, void *buf, int cnt)
int items = len >> 2;
int i;
for (i = 0; i < items; ++i)
- aligned_buf[i] = mci_readl(host, DATA);
+ aligned_buf[i] = mci_readl(host,
+ DATA(host->data_offset));
/* memcpy from aligned buffer into output buffer */
memcpy(buf, aligned_buf, len);
buf += len;
@@ -1174,11 +1182,11 @@ static void dw_mci_pull_data32(struct dw_mci *host, void *buf, int cnt)
{
u32 *pdata = buf;
for (; cnt >= 4; cnt -= 4)
- *pdata++ = mci_readl(host, DATA);
+ *pdata++ = mci_readl(host, DATA(host->data_offset));
buf = pdata;
}
if (cnt) {
- host->part_buf32 = mci_readl(host, DATA);
+ host->part_buf32 = mci_readl(host, DATA(host->data_offset));
dw_mci_pull_final_bytes(host, buf, cnt);
}
}
@@ -1191,7 +1199,8 @@ static void dw_mci_push_data64(struct dw_mci *host, void *buf, int cnt)
buf += len;
cnt -= len;
if (!sg_next(host->sg) || host->part_buf_count == 8) {
- mci_writew(host, DATA, host->part_buf);
+ mci_writew(host, DATA(host->data_offset),
+ host->part_buf);
host->part_buf_count = 0;
}
}
@@ -1208,21 +1217,23 @@ static void dw_mci_push_data64(struct dw_mci *host, void *buf, int cnt)
cnt -= len;
/* push data from aligned buffer into fifo */
for (i = 0; i < items; ++i)
- mci_writeq(host, DATA, aligned_buf[i]);
+ mci_writeq(host, DATA(host->data_offset),
+ aligned_buf[i]);
}
} else
#endif
{
u64 *pdata = buf;
for (; cnt >= 8; cnt -= 8)
- mci_writeq(host, DATA, *pdata++);
+ mci_writeq(host, DATA(host->data_offset), *pdata++);
buf = pdata;
}
/* put anything remaining in the part_buf */
if (cnt) {
dw_mci_set_part_bytes(host, buf, cnt);
if (!sg_next(host->sg))
- mci_writeq(host, DATA, host->part_buf);
+ mci_writeq(host, DATA(host->data_offset),
+ host->part_buf);
}
}
@@ -1237,7 +1248,8 @@ static void dw_mci_pull_data64(struct dw_mci *host, void *buf, int cnt)
int items = len >> 3;
int i;
for (i = 0; i < items; ++i)
- aligned_buf[i] = mci_readq(host, DATA);
+ aligned_buf[i] = mci_readq(host,
+ DATA(host->data_offset));
/* memcpy from aligned buffer into output buffer */
memcpy(buf, aligned_buf, len);
buf += len;
@@ -1248,11 +1260,11 @@ static void dw_mci_pull_data64(struct dw_mci *host, void *buf, int cnt)
{
u64 *pdata = buf;
for (; cnt >= 8; cnt -= 8)
- *pdata++ = mci_readq(host, DATA);
+ *pdata++ = mci_readq(host, DATA(host->data_offset));
buf = pdata;
}
if (cnt) {
- host->part_buf = mci_readq(host, DATA);
+ host->part_buf = mci_readq(host, DATA(host->data_offset));
dw_mci_pull_final_bytes(host, buf, cnt);
}
}
@@ -1952,6 +1964,18 @@ static int dw_mci_probe(struct platform_device *pdev)
}
/*
+ * In 2.40a spec, Data offset is changed.
+ * Need to check the version-id and set data-offset for DATA register.
+ */
+ host->verid = SDMMC_GET_VERID(mci_readl(host, VERID));
+ dev_info(&pdev->dev, "Version ID is %04x\n", host->verid);
+
+ if (host->verid < DW_MMC_240A)
+ host->data_offset = DATA_OFFSET;
+ else
+ host->data_offset = DATA_240A_OFFSET;
+
+ /*
* Enable interrupts for command done, data over, data empty, card det,
* receive ready and error such as transmit, receive timeout, crc error
*/
diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
index bfa3c1c..965fd19 100644
--- a/drivers/mmc/host/dw_mmc.h
+++ b/drivers/mmc/host/dw_mmc.h
@@ -14,6 +14,8 @@
#ifndef _DW_MMC_H_
#define _DW_MMC_H_
+#define DW_MMC_240A 0x240a
+
#define SDMMC_CTRL 0x000
#define SDMMC_PWREN 0x004
#define SDMMC_CLKDIV 0x008
@@ -51,7 +53,11 @@
#define SDMMC_IDINTEN 0x090
#define SDMMC_DSCADDR 0x094
#define SDMMC_BUFADDR 0x098
-#define SDMMC_DATA 0x100
+#define SDMMC_DATA(x) (0x100 + (x))
+
+/* Data offset is difference according to Verision */
+#define DATA_OFFSET 0
+#define DATA_240A_OFFSET 0x100
/* shift bit field */
#define _SBF(f, v) ((v) << (f))
@@ -130,6 +136,8 @@
#define SDMMC_IDMAC_ENABLE BIT(7)
#define SDMMC_IDMAC_FB BIT(1)
#define SDMMC_IDMAC_SWRESET BIT(0)
+/* Version ID register define */
+#define SDMMC_GET_VERID(x) ((x) & 0xFFFF)
/* Register access macros */
#define mci_readl(dev, reg) \
diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
index 6b46819..6928e29 100644
--- a/include/linux/mmc/dw_mmc.h
+++ b/include/linux/mmc/dw_mmc.h
@@ -147,6 +147,8 @@ struct dw_mci {
u32 current_speed;
u32 num_slots;
u32 fifoth_val;
+ u16 verid;
+ u16 data_offset;
struct platform_device *pdev;
struct dw_mci_board *pdata;
struct dw_mci_slot *slot[MAX_MCI_SLOTS];
--
1.7.4.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] mmc: dw_mmc: modify DATA register offset
2011-10-17 7:05 [PATCH] mmc: dw_mmc: modify DATA register offset Jaehoon Chung
@ 2011-10-17 8:27 ` James Hogan
2011-10-17 8:46 ` Jaehoon Chung
0 siblings, 1 reply; 6+ messages in thread
From: James Hogan @ 2011-10-17 8:27 UTC (permalink / raw)
To: Jaehoon Chung; +Cc: linux-mmc, Chris Ball, Kyungmin Park, Will Newton
Hi,
On 10/17/2011 08:05 AM, Jaehoon Chung wrote:
> In dw_mmc 2.40a spec, Data register's offset is changed.
> Now we used Data register offset is 0x100. but if somebody use 2.40a
> controller, must use 0x200 for Data register.
>
> This patch is added version-id checking point and using SDMMC_DATA(x)
> instead of SDMMC_DATA. (assume 2.40a is the latest version)
>
> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
> drivers/mmc/host/dw_mmc.c | 66 ++++++++++++++++++++++++++++++--------------
> drivers/mmc/host/dw_mmc.h | 10 ++++++-
> include/linux/mmc/dw_mmc.h | 2 +
> 3 files changed, 56 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 701f14e..3aaeb08 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1043,7 +1043,8 @@ static void dw_mci_push_data16(struct dw_mci *host, void *buf, int cnt)
> buf += len;
> cnt -= len;
> if (!sg_next(host->sg) || host->part_buf_count == 2) {
> - mci_writew(host, DATA, host->part_buf16);
> + mci_writew(host, DATA(host->data_offset),
> + host->part_buf16);
> host->part_buf_count = 0;
> }
> }
I really think it would be more concise to just have something like this:
mci_writew(host, host->data_offset, host->part_buf16);
...
> +#define DATA_OFFSET 0
> +#define DATA_240A_OFFSET 0x100
and then have these as register positions like the other #defines, e.g.
#define SDMMC_DATA 0x100
#define SDMMC_DATA_240A 0x200
> @@ -1952,6 +1964,18 @@ static int dw_mci_probe(struct platform_device *pdev)
> }
>
> /*
> + * In 2.40a spec, Data offset is changed.
> + * Need to check the version-id and set data-offset for DATA register.
> + */
> + host->verid = SDMMC_GET_VERID(mci_readl(host, VERID));
> + dev_info(&pdev->dev, "Version ID is %04x\n", host->verid);
> +
> + if (host->verid < DW_MMC_240A)
> + host->data_offset = DATA_OFFSET;
> + else
> + host->data_offset = DATA_240A_OFFSET;
> +
> + /*
> * Enable interrupts for command done, data over, data empty, card det,
> * receive ready and error such as transmit, receive timeout, crc error
> */
> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
> index bfa3c1c..965fd19 100644
> --- a/drivers/mmc/host/dw_mmc.h
> +++ b/drivers/mmc/host/dw_mmc.h
> @@ -14,6 +14,8 @@
> #ifndef _DW_MMC_H_
> #define _DW_MMC_H_
>
> +#define DW_MMC_240A 0x240a
> +
> #define SDMMC_CTRL 0x000
> #define SDMMC_PWREN 0x004
> #define SDMMC_CLKDIV 0x008
> @@ -51,7 +53,11 @@
> #define SDMMC_IDINTEN 0x090
> #define SDMMC_DSCADDR 0x094
> #define SDMMC_BUFADDR 0x098
> -#define SDMMC_DATA 0x100
> +#define SDMMC_DATA(x) (0x100 + (x))
> +
> +/* Data offset is difference according to Verision */
should that be "version"?
> +#define DATA_OFFSET 0
> +#define DATA_240A_OFFSET 0x100
>
> /* shift bit field */
> #define _SBF(f, v) ((v) << (f))
> @@ -130,6 +136,8 @@
> #define SDMMC_IDMAC_ENABLE BIT(7)
> #define SDMMC_IDMAC_FB BIT(1)
> #define SDMMC_IDMAC_SWRESET BIT(0)
> +/* Version ID register define */
> +#define SDMMC_GET_VERID(x) ((x) & 0xFFFF)
>
> /* Register access macros */
> #define mci_readl(dev, reg) \
> diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
> index 6b46819..6928e29 100644
> --- a/include/linux/mmc/dw_mmc.h
> +++ b/include/linux/mmc/dw_mmc.h
> @@ -147,6 +147,8 @@ struct dw_mci {
> u32 current_speed;
> u32 num_slots;
> u32 fifoth_val;
> + u16 verid;
> + u16 data_offset;
> struct platform_device *pdev;
> struct dw_mci_board *pdata;
> struct dw_mci_slot *slot[MAX_MCI_SLOTS];
The kerneldoc comment above struct dw_mci should be updated to describe
the new fields.
Other than that it looks good to me.
Thanks
James
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mmc: dw_mmc: modify DATA register offset
2011-10-17 8:27 ` James Hogan
@ 2011-10-17 8:46 ` Jaehoon Chung
2011-10-17 9:00 ` James Hogan
0 siblings, 1 reply; 6+ messages in thread
From: Jaehoon Chung @ 2011-10-17 8:46 UTC (permalink / raw)
To: James Hogan
Cc: Jaehoon Chung, linux-mmc, Chris Ball, Kyungmin Park, Will Newton
Hi James.
On 10/17/2011 05:27 PM, James Hogan wrote:
> Hi,
>
> On 10/17/2011 08:05 AM, Jaehoon Chung wrote:
>> In dw_mmc 2.40a spec, Data register's offset is changed.
>> Now we used Data register offset is 0x100. but if somebody use 2.40a
>> controller, must use 0x200 for Data register.
>>
>> This patch is added version-id checking point and using SDMMC_DATA(x)
>> instead of SDMMC_DATA. (assume 2.40a is the latest version)
>>
>> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>> drivers/mmc/host/dw_mmc.c | 66 ++++++++++++++++++++++++++++++--------------
>> drivers/mmc/host/dw_mmc.h | 10 ++++++-
>> include/linux/mmc/dw_mmc.h | 2 +
>> 3 files changed, 56 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> index 701f14e..3aaeb08 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -1043,7 +1043,8 @@ static void dw_mci_push_data16(struct dw_mci *host, void *buf, int cnt)
>> buf += len;
>> cnt -= len;
>> if (!sg_next(host->sg) || host->part_buf_count == 2) {
>> - mci_writew(host, DATA, host->part_buf16);
>> + mci_writew(host, DATA(host->data_offset),
>> + host->part_buf16);
>> host->part_buf_count = 0;
>> }
>> }
>
> I really think it would be more concise to just have something like this:
> mci_writew(host, host->data_offset, host->part_buf16);
> ...
>
>> +#define DATA_OFFSET 0
>> +#define DATA_240A_OFFSET 0x100
>
> and then have these as register positions like the other #defines, e.g.
> #define SDMMC_DATA 0x100
> #define SDMMC_DATA_240A 0x200
>
Sorry, if change your suggestion, how do you control SDMMC_##reg?
mci_readl(dev, reg) __raw_readl(dev->regs + SDMMC_##reg)
>> @@ -1952,6 +1964,18 @@ static int dw_mci_probe(struct platform_device *pdev)
>> }
>>
>> /*
>> + * In 2.40a spec, Data offset is changed.
>> + * Need to check the version-id and set data-offset for DATA register.
>> + */
>> + host->verid = SDMMC_GET_VERID(mci_readl(host, VERID));
>> + dev_info(&pdev->dev, "Version ID is %04x\n", host->verid);
>> +
>> + if (host->verid < DW_MMC_240A)
>> + host->data_offset = DATA_OFFSET;
>> + else
>> + host->data_offset = DATA_240A_OFFSET;
>> +
>> + /*
>> * Enable interrupts for command done, data over, data empty, card det,
>> * receive ready and error such as transmit, receive timeout, crc error
>> */
>> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
>> index bfa3c1c..965fd19 100644
>> --- a/drivers/mmc/host/dw_mmc.h
>> +++ b/drivers/mmc/host/dw_mmc.h
>> @@ -14,6 +14,8 @@
>> #ifndef _DW_MMC_H_
>> #define _DW_MMC_H_
>>
>> +#define DW_MMC_240A 0x240a
>> +
>> #define SDMMC_CTRL 0x000
>> #define SDMMC_PWREN 0x004
>> #define SDMMC_CLKDIV 0x008
>> @@ -51,7 +53,11 @@
>> #define SDMMC_IDINTEN 0x090
>> #define SDMMC_DSCADDR 0x094
>> #define SDMMC_BUFADDR 0x098
>> -#define SDMMC_DATA 0x100
>> +#define SDMMC_DATA(x) (0x100 + (x))
>> +
>> +/* Data offset is difference according to Verision */
>
> should that be "version"?
Typo..should fix that.
>
>> +#define DATA_OFFSET 0
>> +#define DATA_240A_OFFSET 0x100
>>
>> /* shift bit field */
>> #define _SBF(f, v) ((v) << (f))
>> @@ -130,6 +136,8 @@
>> #define SDMMC_IDMAC_ENABLE BIT(7)
>> #define SDMMC_IDMAC_FB BIT(1)
>> #define SDMMC_IDMAC_SWRESET BIT(0)
>> +/* Version ID register define */
>> +#define SDMMC_GET_VERID(x) ((x) & 0xFFFF)
>>
>> /* Register access macros */
>> #define mci_readl(dev, reg) \
>> diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
>> index 6b46819..6928e29 100644
>> --- a/include/linux/mmc/dw_mmc.h
>> +++ b/include/linux/mmc/dw_mmc.h
>> @@ -147,6 +147,8 @@ struct dw_mci {
>> u32 current_speed;
>> u32 num_slots;
>> u32 fifoth_val;
>> + u16 verid;
>> + u16 data_offset;
>> struct platform_device *pdev;
>> struct dw_mci_board *pdata;
>> struct dw_mci_slot *slot[MAX_MCI_SLOTS];
>
> The kerneldoc comment above struct dw_mci should be updated to describe
> the new fields.
I will add the comment for new fields,
Best Regards,
Jaehoon Chung
>
> Other than that it looks good to me.
>
> Thanks
> James
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mmc: dw_mmc: modify DATA register offset
2011-10-17 8:46 ` Jaehoon Chung
@ 2011-10-17 9:00 ` James Hogan
2011-10-17 10:04 ` Jaehoon Chung
0 siblings, 1 reply; 6+ messages in thread
From: James Hogan @ 2011-10-17 9:00 UTC (permalink / raw)
To: Jaehoon Chung; +Cc: linux-mmc, Chris Ball, Kyungmin Park, Will Newton
Hi
On 10/17/2011 09:46 AM, Jaehoon Chung wrote:
> Hi James.
>
> On 10/17/2011 05:27 PM, James Hogan wrote:
>
>> Hi,
>>
>> On 10/17/2011 08:05 AM, Jaehoon Chung wrote:
>>> In dw_mmc 2.40a spec, Data register's offset is changed.
>>> Now we used Data register offset is 0x100. but if somebody use 2.40a
>>> controller, must use 0x200 for Data register.
>>>
>>> This patch is added version-id checking point and using SDMMC_DATA(x)
>>> instead of SDMMC_DATA. (assume 2.40a is the latest version)
>>>
>>> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>>> ---
>>> drivers/mmc/host/dw_mmc.c | 66 ++++++++++++++++++++++++++++++--------------
>>> drivers/mmc/host/dw_mmc.h | 10 ++++++-
>>> include/linux/mmc/dw_mmc.h | 2 +
>>> 3 files changed, 56 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>> index 701f14e..3aaeb08 100644
>>> --- a/drivers/mmc/host/dw_mmc.c
>>> +++ b/drivers/mmc/host/dw_mmc.c
>>> @@ -1043,7 +1043,8 @@ static void dw_mci_push_data16(struct dw_mci *host, void *buf, int cnt)
>>> buf += len;
>>> cnt -= len;
>>> if (!sg_next(host->sg) || host->part_buf_count == 2) {
>>> - mci_writew(host, DATA, host->part_buf16);
>>> + mci_writew(host, DATA(host->data_offset),
>>> + host->part_buf16);
>>> host->part_buf_count = 0;
>>> }
>>> }
>>
>> I really think it would be more concise to just have something like this:
>> mci_writew(host, host->data_offset, host->part_buf16);
>> ...
>>
>>> +#define DATA_OFFSET 0
>>> +#define DATA_240A_OFFSET 0x100
>>
>> and then have these as register positions like the other #defines, e.g.
>> #define SDMMC_DATA 0x100
>> #define SDMMC_DATA_240A 0x200
>>
>
>
> Sorry, if change your suggestion, how do you control SDMMC_##reg?
> mci_readl(dev, reg) __raw_readl(dev->regs + SDMMC_##reg)
Ah ok, sorry. I see what you mean now. I'd forgotton the mci_readl macro
did that!
I suppose there's a couple of ways that you could avoid the offset from
0x100.
1) could define a register macro which takes a raw offset:
#define SDMMC_RAW(x) (x)
mci_writew(host, RAW(host->data_offset), host->part_buf16);
2) could define the DATA register macro which takes a struct dw_mci* as
an argument:
#define SDMMC_DATA(HOST) ((HOST)->data_offset)
mci_writew(host, DATA(host), host->part_buf16);
I don't have a strong preference between these.
Thanks
James
>
>>> @@ -1952,6 +1964,18 @@ static int dw_mci_probe(struct platform_device *pdev)
>>> }
>>>
>>> /*
>>> + * In 2.40a spec, Data offset is changed.
>>> + * Need to check the version-id and set data-offset for DATA register.
>>> + */
>>> + host->verid = SDMMC_GET_VERID(mci_readl(host, VERID));
>>> + dev_info(&pdev->dev, "Version ID is %04x\n", host->verid);
>>> +
>>> + if (host->verid < DW_MMC_240A)
>>> + host->data_offset = DATA_OFFSET;
>>> + else
>>> + host->data_offset = DATA_240A_OFFSET;
>>> +
>>> + /*
>>> * Enable interrupts for command done, data over, data empty, card det,
>>> * receive ready and error such as transmit, receive timeout, crc error
>>> */
>>> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
>>> index bfa3c1c..965fd19 100644
>>> --- a/drivers/mmc/host/dw_mmc.h
>>> +++ b/drivers/mmc/host/dw_mmc.h
>>> @@ -14,6 +14,8 @@
>>> #ifndef _DW_MMC_H_
>>> #define _DW_MMC_H_
>>>
>>> +#define DW_MMC_240A 0x240a
>>> +
>>> #define SDMMC_CTRL 0x000
>>> #define SDMMC_PWREN 0x004
>>> #define SDMMC_CLKDIV 0x008
>>> @@ -51,7 +53,11 @@
>>> #define SDMMC_IDINTEN 0x090
>>> #define SDMMC_DSCADDR 0x094
>>> #define SDMMC_BUFADDR 0x098
>>> -#define SDMMC_DATA 0x100
>>> +#define SDMMC_DATA(x) (0x100 + (x))
>>> +
>>> +/* Data offset is difference according to Verision */
>>
>> should that be "version"?
>
> Typo..should fix that.
>
>>
>>> +#define DATA_OFFSET 0
>>> +#define DATA_240A_OFFSET 0x100
>>>
>>> /* shift bit field */
>>> #define _SBF(f, v) ((v) << (f))
>>> @@ -130,6 +136,8 @@
>>> #define SDMMC_IDMAC_ENABLE BIT(7)
>>> #define SDMMC_IDMAC_FB BIT(1)
>>> #define SDMMC_IDMAC_SWRESET BIT(0)
>>> +/* Version ID register define */
>>> +#define SDMMC_GET_VERID(x) ((x) & 0xFFFF)
>>>
>>> /* Register access macros */
>>> #define mci_readl(dev, reg) \
>>> diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
>>> index 6b46819..6928e29 100644
>>> --- a/include/linux/mmc/dw_mmc.h
>>> +++ b/include/linux/mmc/dw_mmc.h
>>> @@ -147,6 +147,8 @@ struct dw_mci {
>>> u32 current_speed;
>>> u32 num_slots;
>>> u32 fifoth_val;
>>> + u16 verid;
>>> + u16 data_offset;
>>> struct platform_device *pdev;
>>> struct dw_mci_board *pdata;
>>> struct dw_mci_slot *slot[MAX_MCI_SLOTS];
>>
>> The kerneldoc comment above struct dw_mci should be updated to describe
>> the new fields.
>
> I will add the comment for new fields,
>
> Best Regards,
> Jaehoon Chung
>
>>
>
>> Other than that it looks good to me.
>>
>> Thanks
>> James
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mmc: dw_mmc: modify DATA register offset
2011-10-17 9:00 ` James Hogan
@ 2011-10-17 10:04 ` Jaehoon Chung
2011-10-17 10:22 ` James Hogan
0 siblings, 1 reply; 6+ messages in thread
From: Jaehoon Chung @ 2011-10-17 10:04 UTC (permalink / raw)
To: James Hogan
Cc: Jaehoon Chung, linux-mmc, Chris Ball, Kyungmin Park, Will Newton
On 10/17/2011 06:00 PM, James Hogan wrote:
> Hi
>
> On 10/17/2011 09:46 AM, Jaehoon Chung wrote:
>> Hi James.
>>
>> On 10/17/2011 05:27 PM, James Hogan wrote:
>>
>>> Hi,
>>>
>>> On 10/17/2011 08:05 AM, Jaehoon Chung wrote:
>>>> In dw_mmc 2.40a spec, Data register's offset is changed.
>>>> Now we used Data register offset is 0x100. but if somebody use 2.40a
>>>> controller, must use 0x200 for Data register.
>>>>
>>>> This patch is added version-id checking point and using SDMMC_DATA(x)
>>>> instead of SDMMC_DATA. (assume 2.40a is the latest version)
>>>>
>>>> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
>>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>>>> ---
>>>> drivers/mmc/host/dw_mmc.c | 66 ++++++++++++++++++++++++++++++--------------
>>>> drivers/mmc/host/dw_mmc.h | 10 ++++++-
>>>> include/linux/mmc/dw_mmc.h | 2 +
>>>> 3 files changed, 56 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>>> index 701f14e..3aaeb08 100644
>>>> --- a/drivers/mmc/host/dw_mmc.c
>>>> +++ b/drivers/mmc/host/dw_mmc.c
>>>> @@ -1043,7 +1043,8 @@ static void dw_mci_push_data16(struct dw_mci *host, void *buf, int cnt)
>>>> buf += len;
>>>> cnt -= len;
>>>> if (!sg_next(host->sg) || host->part_buf_count == 2) {
>>>> - mci_writew(host, DATA, host->part_buf16);
>>>> + mci_writew(host, DATA(host->data_offset),
>>>> + host->part_buf16);
>>>> host->part_buf_count = 0;
>>>> }
>>>> }
>>>
>>> I really think it would be more concise to just have something like this:
>>> mci_writew(host, host->data_offset, host->part_buf16);
>>> ...
>>>
>>>> +#define DATA_OFFSET 0
>>>> +#define DATA_240A_OFFSET 0x100
>>>
>>> and then have these as register positions like the other #defines, e.g.
>>> #define SDMMC_DATA 0x100
>>> #define SDMMC_DATA_240A 0x200
>>>
>>
>>
>> Sorry, if change your suggestion, how do you control SDMMC_##reg?
>> mci_readl(dev, reg) __raw_readl(dev->regs + SDMMC_##reg)
>
> Ah ok, sorry. I see what you mean now. I'd forgotton the mci_readl macro
> did that!
>
> I suppose there's a couple of ways that you could avoid the offset from
> 0x100.
>
> 1) could define a register macro which takes a raw offset:
> #define SDMMC_RAW(x) (x)
> mci_writew(host, RAW(host->data_offset), host->part_buf16);
>
> 2) could define the DATA register macro which takes a struct dw_mci* as
> an argument:
> #define SDMMC_DATA(HOST) ((HOST)->data_offset)
> mci_writew(host, DATA(host), host->part_buf16);
>
> I don't have a strong preference between these.
My suggestion is also similar to your suggestions.
But your suggestions is used data->offset assigned 0x100 or 0x200. right?
My suggestions is used (DATA + (x)).
All of them must use the macro like DATA(x). right?
My suggestion and yours are difference which offset used.
I will resend this patch after modify...
Best regards,
Jaehoon Chung
>
> Thanks
> James
>
>>
>>>> @@ -1952,6 +1964,18 @@ static int dw_mci_probe(struct platform_device *pdev)
>>>> }
>>>>
>>>> /*
>>>> + * In 2.40a spec, Data offset is changed.
>>>> + * Need to check the version-id and set data-offset for DATA register.
>>>> + */
>>>> + host->verid = SDMMC_GET_VERID(mci_readl(host, VERID));
>>>> + dev_info(&pdev->dev, "Version ID is %04x\n", host->verid);
>>>> +
>>>> + if (host->verid < DW_MMC_240A)
>>>> + host->data_offset = DATA_OFFSET;
>>>> + else
>>>> + host->data_offset = DATA_240A_OFFSET;
>>>> +
>>>> + /*
>>>> * Enable interrupts for command done, data over, data empty, card det,
>>>> * receive ready and error such as transmit, receive timeout, crc error
>>>> */
>>>> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
>>>> index bfa3c1c..965fd19 100644
>>>> --- a/drivers/mmc/host/dw_mmc.h
>>>> +++ b/drivers/mmc/host/dw_mmc.h
>>>> @@ -14,6 +14,8 @@
>>>> #ifndef _DW_MMC_H_
>>>> #define _DW_MMC_H_
>>>>
>>>> +#define DW_MMC_240A 0x240a
>>>> +
>>>> #define SDMMC_CTRL 0x000
>>>> #define SDMMC_PWREN 0x004
>>>> #define SDMMC_CLKDIV 0x008
>>>> @@ -51,7 +53,11 @@
>>>> #define SDMMC_IDINTEN 0x090
>>>> #define SDMMC_DSCADDR 0x094
>>>> #define SDMMC_BUFADDR 0x098
>>>> -#define SDMMC_DATA 0x100
>>>> +#define SDMMC_DATA(x) (0x100 + (x))
>>>> +
>>>> +/* Data offset is difference according to Verision */
>>>
>>> should that be "version"?
>>
>> Typo..should fix that.
>>
>>>
>>>> +#define DATA_OFFSET 0
>>>> +#define DATA_240A_OFFSET 0x100
>>>>
>>>> /* shift bit field */
>>>> #define _SBF(f, v) ((v) << (f))
>>>> @@ -130,6 +136,8 @@
>>>> #define SDMMC_IDMAC_ENABLE BIT(7)
>>>> #define SDMMC_IDMAC_FB BIT(1)
>>>> #define SDMMC_IDMAC_SWRESET BIT(0)
>>>> +/* Version ID register define */
>>>> +#define SDMMC_GET_VERID(x) ((x) & 0xFFFF)
>>>>
>>>> /* Register access macros */
>>>> #define mci_readl(dev, reg) \
>>>> diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
>>>> index 6b46819..6928e29 100644
>>>> --- a/include/linux/mmc/dw_mmc.h
>>>> +++ b/include/linux/mmc/dw_mmc.h
>>>> @@ -147,6 +147,8 @@ struct dw_mci {
>>>> u32 current_speed;
>>>> u32 num_slots;
>>>> u32 fifoth_val;
>>>> + u16 verid;
>>>> + u16 data_offset;
>>>> struct platform_device *pdev;
>>>> struct dw_mci_board *pdata;
>>>> struct dw_mci_slot *slot[MAX_MCI_SLOTS];
>>>
>>> The kerneldoc comment above struct dw_mci should be updated to describe
>>> the new fields.
>>
>> I will add the comment for new fields,
>>
>> Best Regards,
>> Jaehoon Chung
>>
>>>
>>
>>> Other than that it looks good to me.
>>>
>>> Thanks
>>> James
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mmc: dw_mmc: modify DATA register offset
2011-10-17 10:04 ` Jaehoon Chung
@ 2011-10-17 10:22 ` James Hogan
0 siblings, 0 replies; 6+ messages in thread
From: James Hogan @ 2011-10-17 10:22 UTC (permalink / raw)
To: Jaehoon Chung; +Cc: linux-mmc, Chris Ball, Kyungmin Park, Will Newton
On 10/17/2011 11:04 AM, Jaehoon Chung wrote:
> On 10/17/2011 06:00 PM, James Hogan wrote:
>
>> Hi
>>
>> On 10/17/2011 09:46 AM, Jaehoon Chung wrote:
>>> Hi James.
>>>
>>> On 10/17/2011 05:27 PM, James Hogan wrote:
>>>
>>>> Hi,
>>>>
>>>> On 10/17/2011 08:05 AM, Jaehoon Chung wrote:
>>>>> In dw_mmc 2.40a spec, Data register's offset is changed.
>>>>> Now we used Data register offset is 0x100. but if somebody use 2.40a
>>>>> controller, must use 0x200 for Data register.
>>>>>
>>>>> This patch is added version-id checking point and using SDMMC_DATA(x)
>>>>> instead of SDMMC_DATA. (assume 2.40a is the latest version)
>>>>>
>>>>> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
>>>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>>>>> ---
>>>>> drivers/mmc/host/dw_mmc.c | 66 ++++++++++++++++++++++++++++++--------------
>>>>> drivers/mmc/host/dw_mmc.h | 10 ++++++-
>>>>> include/linux/mmc/dw_mmc.h | 2 +
>>>>> 3 files changed, 56 insertions(+), 22 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>>>> index 701f14e..3aaeb08 100644
>>>>> --- a/drivers/mmc/host/dw_mmc.c
>>>>> +++ b/drivers/mmc/host/dw_mmc.c
>>>>> @@ -1043,7 +1043,8 @@ static void dw_mci_push_data16(struct dw_mci *host, void *buf, int cnt)
>>>>> buf += len;
>>>>> cnt -= len;
>>>>> if (!sg_next(host->sg) || host->part_buf_count == 2) {
>>>>> - mci_writew(host, DATA, host->part_buf16);
>>>>> + mci_writew(host, DATA(host->data_offset),
>>>>> + host->part_buf16);
>>>>> host->part_buf_count = 0;
>>>>> }
>>>>> }
>>>>
>>>> I really think it would be more concise to just have something like this:
>>>> mci_writew(host, host->data_offset, host->part_buf16);
>>>> ...
>>>>
>>>>> +#define DATA_OFFSET 0
>>>>> +#define DATA_240A_OFFSET 0x100
>>>>
>>>> and then have these as register positions like the other #defines, e.g.
>>>> #define SDMMC_DATA 0x100
>>>> #define SDMMC_DATA_240A 0x200
>>>>
>>>
>>>
>>> Sorry, if change your suggestion, how do you control SDMMC_##reg?
>>> mci_readl(dev, reg) __raw_readl(dev->regs + SDMMC_##reg)
>>
>> Ah ok, sorry. I see what you mean now. I'd forgotton the mci_readl macro
>> did that!
>>
>> I suppose there's a couple of ways that you could avoid the offset from
>> 0x100.
>>
>> 1) could define a register macro which takes a raw offset:
>> #define SDMMC_RAW(x) (x)
>> mci_writew(host, RAW(host->data_offset), host->part_buf16);
>>
>> 2) could define the DATA register macro which takes a struct dw_mci* as
>> an argument:
>> #define SDMMC_DATA(HOST) ((HOST)->data_offset)
>> mci_writew(host, DATA(host), host->part_buf16);
>>
>> I don't have a strong preference between these.
>
> My suggestion is also similar to your suggestions.
> But your suggestions is used data->offset assigned 0x100 or 0x200. right?
yes, that's right. Using (host->regs + 0x100 + host->data_offset) is 2
additions, which is one reason why I suggest having just (host->regs +
host->data_offset).
> My suggestions is used (DATA + (x)).
> All of them must use the macro like DATA(x). right?
yes, unfortunately, unless the mci_write* macros are altered, something
like what you did before, but I think since DATA is still being treated
as a single register, changing the SDMMC_DATA macro would be better.
Cheers
James
>
> My suggestion and yours are difference which offset used.
> I will resend this patch after modify...
>
> Best regards,
> Jaehoon Chung
>
>>
>> Thanks
>> James
>>
>>>
>>>>> @@ -1952,6 +1964,18 @@ static int dw_mci_probe(struct platform_device *pdev)
>>>>> }
>>>>>
>>>>> /*
>>>>> + * In 2.40a spec, Data offset is changed.
>>>>> + * Need to check the version-id and set data-offset for DATA register.
>>>>> + */
>>>>> + host->verid = SDMMC_GET_VERID(mci_readl(host, VERID));
>>>>> + dev_info(&pdev->dev, "Version ID is %04x\n", host->verid);
>>>>> +
>>>>> + if (host->verid < DW_MMC_240A)
>>>>> + host->data_offset = DATA_OFFSET;
>>>>> + else
>>>>> + host->data_offset = DATA_240A_OFFSET;
>>>>> +
>>>>> + /*
>>>>> * Enable interrupts for command done, data over, data empty, card det,
>>>>> * receive ready and error such as transmit, receive timeout, crc error
>>>>> */
>>>>> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
>>>>> index bfa3c1c..965fd19 100644
>>>>> --- a/drivers/mmc/host/dw_mmc.h
>>>>> +++ b/drivers/mmc/host/dw_mmc.h
>>>>> @@ -14,6 +14,8 @@
>>>>> #ifndef _DW_MMC_H_
>>>>> #define _DW_MMC_H_
>>>>>
>>>>> +#define DW_MMC_240A 0x240a
>>>>> +
>>>>> #define SDMMC_CTRL 0x000
>>>>> #define SDMMC_PWREN 0x004
>>>>> #define SDMMC_CLKDIV 0x008
>>>>> @@ -51,7 +53,11 @@
>>>>> #define SDMMC_IDINTEN 0x090
>>>>> #define SDMMC_DSCADDR 0x094
>>>>> #define SDMMC_BUFADDR 0x098
>>>>> -#define SDMMC_DATA 0x100
>>>>> +#define SDMMC_DATA(x) (0x100 + (x))
>>>>> +
>>>>> +/* Data offset is difference according to Verision */
>>>>
>>>> should that be "version"?
>>>
>>> Typo..should fix that.
>>>
>>>>
>>>>> +#define DATA_OFFSET 0
>>>>> +#define DATA_240A_OFFSET 0x100
>>>>>
>>>>> /* shift bit field */
>>>>> #define _SBF(f, v) ((v) << (f))
>>>>> @@ -130,6 +136,8 @@
>>>>> #define SDMMC_IDMAC_ENABLE BIT(7)
>>>>> #define SDMMC_IDMAC_FB BIT(1)
>>>>> #define SDMMC_IDMAC_SWRESET BIT(0)
>>>>> +/* Version ID register define */
>>>>> +#define SDMMC_GET_VERID(x) ((x) & 0xFFFF)
>>>>>
>>>>> /* Register access macros */
>>>>> #define mci_readl(dev, reg) \
>>>>> diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
>>>>> index 6b46819..6928e29 100644
>>>>> --- a/include/linux/mmc/dw_mmc.h
>>>>> +++ b/include/linux/mmc/dw_mmc.h
>>>>> @@ -147,6 +147,8 @@ struct dw_mci {
>>>>> u32 current_speed;
>>>>> u32 num_slots;
>>>>> u32 fifoth_val;
>>>>> + u16 verid;
>>>>> + u16 data_offset;
>>>>> struct platform_device *pdev;
>>>>> struct dw_mci_board *pdata;
>>>>> struct dw_mci_slot *slot[MAX_MCI_SLOTS];
>>>>
>>>> The kerneldoc comment above struct dw_mci should be updated to describe
>>>> the new fields.
>>>
>>> I will add the comment for new fields,
>>>
>>> Best Regards,
>>> Jaehoon Chung
>>>
>>>>
>>>
>>>> Other than that it looks good to me.
>>>>
>>>> Thanks
>>>> James
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>
>>>
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-10-17 10:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-10-17 7:05 [PATCH] mmc: dw_mmc: modify DATA register offset Jaehoon Chung
2011-10-17 8:27 ` James Hogan
2011-10-17 8:46 ` Jaehoon Chung
2011-10-17 9:00 ` James Hogan
2011-10-17 10:04 ` Jaehoon Chung
2011-10-17 10:22 ` James Hogan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).