diff mbox

[U-Boot,RFC] drivers: mmc: omap_hsmmc: Fix IO Buffer on OMAP36XX

Message ID 1483985529-15644-1-git-send-email-aford173@gmail.com
State Changes Requested
Delegated to: Jaehoon Chung
Headers show

Commit Message

Adam Ford Jan. 9, 2017, 6:12 p.m. UTC 
From: Adam Ford <aford173@gmail.com>

On the OMAP36xx (and 37xx) the CONTROL_WKUP_CTRL register has
a field (bit 6) named GPIO_IO_PWRDNZ.  If 0, the IO buffers which
are related to the MMC are disabled. After the PBIAS is configured,
this bit should be set high to enable the MMC port.

Signed-off-by: Adam Ford <aford173@gmail.com>

Comments

Jaehoon Chung Jan. 10, 2017, 10:32 a.m. UTC | #1 
Hi Adam,

On 01/10/2017 03:12 AM, aford173@gmail.com wrote:
> From: Adam Ford <aford173@gmail.com>
> 
> On the OMAP36xx (and 37xx) the CONTROL_WKUP_CTRL register has
> a field (bit 6) named GPIO_IO_PWRDNZ.  If 0, the IO buffers which
> are related to the MMC are disabled. After the PBIAS is configured,
> this bit should be set high to enable the MMC port.
> 
> Signed-off-by: Adam Ford <aford173@gmail.com>
> 
> diff --git a/drivers/mmc/omap_hsmmc.c b/drivers/mmc/omap_hsmmc.c
> index 0a1ee40..069fd5a 100644
> --- a/drivers/mmc/omap_hsmmc.c
> +++ b/drivers/mmc/omap_hsmmc.c
> @@ -38,6 +38,7 @@
>  #include <asm/arch/sys_proto.h>
>  #endif
>  #include <dm.h>
> +#include <asm/arch-omap3/mux.h>
>  
>  DECLARE_GLOBAL_DATA_PTR;
>  
> @@ -115,6 +116,11 @@ static unsigned char mmc_board_init(struct mmc *mmc)
>  		PBIASSPEEDCTRL0 | PBIASLITEPWRDNZ0,
>  		&t2_base->pbias_lite);
>  
> +	if (get_cpu_family() == CPU_OMAP36XX)
> +		writel(readl(OMAP34XX_CTRL_WKUP_CTRL) |
> +				OMAP34XX_CTRL_WKUP_CTRL_GPIO_IO_PWRDNZ,
> +				OMAP34XX_CTRL_WKUP_CTRL);

get_cpu_family() is defined in arch/arm/mach-omap2/omap3/sys_info.c
When i have checked it, it's compiled when CONFIG_OMAP34XX is enabled.
Doesn't it affect? just curious.

Best Regards,
Jaehoon Chung

> +
>  	writel(readl(&t2_base->devconf0) | MMCSDIO1ADPCLKISEL,
>  		&t2_base->devconf0);
>  
>
Adam Ford Jan. 10, 2017, 2:59 p.m. UTC | #2 
On Tue, Jan 10, 2017 at 4:32 AM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
> Hi Adam,
>
> On 01/10/2017 03:12 AM, aford173@gmail.com wrote:
>> From: Adam Ford <aford173@gmail.com>
>>
>> On the OMAP36xx (and 37xx) the CONTROL_WKUP_CTRL register has
>> a field (bit 6) named GPIO_IO_PWRDNZ.  If 0, the IO buffers which
>> are related to the MMC are disabled. After the PBIAS is configured,
>> this bit should be set high to enable the MMC port.
>>
>> Signed-off-by: Adam Ford <aford173@gmail.com>
>>
>> diff --git a/drivers/mmc/omap_hsmmc.c b/drivers/mmc/omap_hsmmc.c
>> index 0a1ee40..069fd5a 100644
>> --- a/drivers/mmc/omap_hsmmc.c
>> +++ b/drivers/mmc/omap_hsmmc.c
>> @@ -38,6 +38,7 @@
>>  #include <asm/arch/sys_proto.h>
>>  #endif
>>  #include <dm.h>
>> +#include <asm/arch-omap3/mux.h>
>>
>>  DECLARE_GLOBAL_DATA_PTR;
>>
>> @@ -115,6 +116,11 @@ static unsigned char mmc_board_init(struct mmc *mmc)
>>               PBIASSPEEDCTRL0 | PBIASLITEPWRDNZ0,
>>               &t2_base->pbias_lite);
>>
>> +     if (get_cpu_family() == CPU_OMAP36XX)
>> +             writel(readl(OMAP34XX_CTRL_WKUP_CTRL) |
>> +                             OMAP34XX_CTRL_WKUP_CTRL_GPIO_IO_PWRDNZ,
>> +                             OMAP34XX_CTRL_WKUP_CTRL);
>
> get_cpu_family() is defined in arch/arm/mach-omap2/omap3/sys_info.c
> When i have checked it, it's compiled when CONFIG_OMAP34XX is enabled.
> Doesn't it affect? just curious.
>

The 36XX is a superset of the 34XX and the entire chunck of code is
enclosed by and #if #endif that is only compiled in when
CONFIG_OMAP34XX is defined.  For OMAP36xx boards, they should have
CONFIG_OMAP34XX defined. If it's not an 34XX, it won't get included
and the check won't get run.  Do you disagree?


> Best Regards,
> Jaehoon Chung
>
>> +
>>       writel(readl(&t2_base->devconf0) | MMCSDIO1ADPCLKISEL,
>>               &t2_base->devconf0);
>>
>>
>
Tom Rini Jan. 10, 2017, 11:14 p.m. UTC | #3 
On Mon, Jan 09, 2017 at 12:12:09PM -0600, aford173@gmail.com wrote:

> From: Adam Ford <aford173@gmail.com>
> 
> On the OMAP36xx (and 37xx) the CONTROL_WKUP_CTRL register has
> a field (bit 6) named GPIO_IO_PWRDNZ.  If 0, the IO buffers which
> are related to the MMC are disabled. After the PBIAS is configured,
> this bit should be set high to enable the MMC port.
> 
> Signed-off-by: Adam Ford <aford173@gmail.com>
> 
> diff --git a/drivers/mmc/omap_hsmmc.c b/drivers/mmc/omap_hsmmc.c
> index 0a1ee40..069fd5a 100644
> --- a/drivers/mmc/omap_hsmmc.c
> +++ b/drivers/mmc/omap_hsmmc.c
> @@ -38,6 +38,7 @@
>  #include <asm/arch/sys_proto.h>
>  #endif
>  #include <dm.h>
> +#include <asm/arch-omap3/mux.h>
>  
>  DECLARE_GLOBAL_DATA_PTR;
>  
> @@ -115,6 +116,11 @@ static unsigned char mmc_board_init(struct mmc *mmc)
>  		PBIASSPEEDCTRL0 | PBIASLITEPWRDNZ0,
>  		&t2_base->pbias_lite);
>  
> +	if (get_cpu_family() == CPU_OMAP36XX)
> +		writel(readl(OMAP34XX_CTRL_WKUP_CTRL) |
> +				OMAP34XX_CTRL_WKUP_CTRL_GPIO_IO_PWRDNZ,
> +				OMAP34XX_CTRL_WKUP_CTRL);
> +
>  	writel(readl(&t2_base->devconf0) | MMCSDIO1ADPCLKISEL,
>  		&t2_base->devconf0);

I think I'd like to see this with a build-time check to avoid bloating
all of the other families.  Thanks!
Jaehoon Chung Jan. 11, 2017, 2:10 a.m. UTC | #4 
On 01/10/2017 11:59 PM, Adam Ford wrote:
> On Tue, Jan 10, 2017 at 4:32 AM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>> Hi Adam,
>>
>> On 01/10/2017 03:12 AM, aford173@gmail.com wrote:
>>> From: Adam Ford <aford173@gmail.com>
>>>
>>> On the OMAP36xx (and 37xx) the CONTROL_WKUP_CTRL register has
>>> a field (bit 6) named GPIO_IO_PWRDNZ.  If 0, the IO buffers which
>>> are related to the MMC are disabled. After the PBIAS is configured,
>>> this bit should be set high to enable the MMC port.
>>>
>>> Signed-off-by: Adam Ford <aford173@gmail.com>
>>>
>>> diff --git a/drivers/mmc/omap_hsmmc.c b/drivers/mmc/omap_hsmmc.c
>>> index 0a1ee40..069fd5a 100644
>>> --- a/drivers/mmc/omap_hsmmc.c
>>> +++ b/drivers/mmc/omap_hsmmc.c
>>> @@ -38,6 +38,7 @@
>>>  #include <asm/arch/sys_proto.h>
>>>  #endif
>>>  #include <dm.h>
>>> +#include <asm/arch-omap3/mux.h>
>>>
>>>  DECLARE_GLOBAL_DATA_PTR;
>>>
>>> @@ -115,6 +116,11 @@ static unsigned char mmc_board_init(struct mmc *mmc)
>>>               PBIASSPEEDCTRL0 | PBIASLITEPWRDNZ0,
>>>               &t2_base->pbias_lite);
>>>
>>> +     if (get_cpu_family() == CPU_OMAP36XX)
>>> +             writel(readl(OMAP34XX_CTRL_WKUP_CTRL) |
>>> +                             OMAP34XX_CTRL_WKUP_CTRL_GPIO_IO_PWRDNZ,
>>> +                             OMAP34XX_CTRL_WKUP_CTRL);
>>
>> get_cpu_family() is defined in arch/arm/mach-omap2/omap3/sys_info.c
>> When i have checked it, it's compiled when CONFIG_OMAP34XX is enabled.
>> Doesn't it affect? just curious.
>>
> 
> The 36XX is a superset of the 34XX and the entire chunck of code is
> enclosed by and #if #endif that is only compiled in when
> CONFIG_OMAP34XX is defined.  For OMAP36xx boards, they should have
> CONFIG_OMAP34XX defined. If it's not an 34XX, it won't get included
> and the check won't get run.  Do you disagree?

I'm just curious for it. because there is using the "ifdef" about "get_cpu_family()".

Best Regards,
Jaehoon Chung

> 
> 
>> Best Regards,
>> Jaehoon Chung
>>
>>> +
>>>       writel(readl(&t2_base->devconf0) | MMCSDIO1ADPCLKISEL,
>>>               &t2_base->devconf0);
>>>
>>>
>>
> 
> 
>
Adam Ford Jan. 11, 2017, 2:21 p.m. UTC | #5 
On Tue, Jan 10, 2017 at 5:14 PM, Tom Rini <trini@konsulko.com> wrote:
> On Mon, Jan 09, 2017 at 12:12:09PM -0600, aford173@gmail.com wrote:
>
>> From: Adam Ford <aford173@gmail.com>
>>
>> On the OMAP36xx (and 37xx) the CONTROL_WKUP_CTRL register has
>> a field (bit 6) named GPIO_IO_PWRDNZ.  If 0, the IO buffers which
>> are related to the MMC are disabled. After the PBIAS is configured,
>> this bit should be set high to enable the MMC port.
>>
>> Signed-off-by: Adam Ford <aford173@gmail.com>
>>
>> diff --git a/drivers/mmc/omap_hsmmc.c b/drivers/mmc/omap_hsmmc.c
>> index 0a1ee40..069fd5a 100644
>> --- a/drivers/mmc/omap_hsmmc.c
>> +++ b/drivers/mmc/omap_hsmmc.c
>> @@ -38,6 +38,7 @@
>>  #include <asm/arch/sys_proto.h>
>>  #endif
>>  #include <dm.h>
>> +#include <asm/arch-omap3/mux.h>
>>
>>  DECLARE_GLOBAL_DATA_PTR;
>>
>> @@ -115,6 +116,11 @@ static unsigned char mmc_board_init(struct mmc *mmc)
>>               PBIASSPEEDCTRL0 | PBIASLITEPWRDNZ0,
>>               &t2_base->pbias_lite);
>>
#ifdef OMAP36_MMC1_PINS
>> +     if (get_cpu_family() == CPU_OMAP36XX)
>> +             writel(readl(OMAP34XX_CTRL_WKUP_CTRL) |
>> +                             OMAP34XX_CTRL_WKUP_CTRL_GPIO_IO_PWRDNZ,
>> +                             OMAP34XX_CTRL_WKUP_CTRL);
>> +
#endif

>>       writel(readl(&t2_base->devconf0) | MMCSDIO1ADPCLKISEL,
>>               &t2_base->devconf0);
>
> I think I'd like to see this with a build-time check to avoid bloating
> all of the other families.  Thanks!

Since it's only on the OMAP36xx and 37xx (that I am aware) and it's
specific for a couple pins on the MMC1 bus using GPIO_126, 127, and
129.  If we encapsulated the update into and #ifdef, would that
satisfy you?

I added my proposed change above.  I would also add the define into
the omap3_logic_defconfig file.  Any other OMAP36/37 boards could add
it in.

Does that work for you?  If so, I'll submit an updated patch

>
> --
> Tom
diff mbox

Patch

diff --git a/drivers/mmc/omap_hsmmc.c b/drivers/mmc/omap_hsmmc.c
index 0a1ee40..069fd5a 100644
--- a/drivers/mmc/omap_hsmmc.c
+++ b/drivers/mmc/omap_hsmmc.c
@@ -38,6 +38,7 @@ 
 #include <asm/arch/sys_proto.h>
 #endif
 #include <dm.h>
+#include <asm/arch-omap3/mux.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -115,6 +116,11 @@  static unsigned char mmc_board_init(struct mmc *mmc)
 		PBIASSPEEDCTRL0 | PBIASLITEPWRDNZ0,
 		&t2_base->pbias_lite);
 
+	if (get_cpu_family() == CPU_OMAP36XX)
+		writel(readl(OMAP34XX_CTRL_WKUP_CTRL) |
+				OMAP34XX_CTRL_WKUP_CTRL_GPIO_IO_PWRDNZ,
+				OMAP34XX_CTRL_WKUP_CTRL);
+
 	writel(readl(&t2_base->devconf0) | MMCSDIO1ADPCLKISEL,
 		&t2_base->devconf0);