[PATCH] ARM: mach-shmobile: Add missing __initdata on intcs

[PATCH] ARM: mach-shmobile: Add missing __initdata on intcs

[Kuninori Morimoto]

Signed-off-by: Kuninori Morimoto <morimoto.kuninori@renesas.com>
---
 arch/arm/mach-shmobile/intc-sh7367.c |    6 +++---
 arch/arm/mach-shmobile/intc-sh7377.c |    6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-shmobile/intc-sh7367.c b/arch/arm/mach-shmobile/intc-sh7367.c
index 5ff70ca..b594969 100644
--- a/arch/arm/mach-shmobile/intc-sh7367.c
+++ b/arch/arm/mach-shmobile/intc-sh7367.c
@@ -75,7 +75,7 @@ enum {
 	ETM11, ARM11, USBHS, FLCTL, IIC1
 };
 
-static struct intc_vect intca_vectors[] = {
+static struct intc_vect intca_vectors[] __initdata = {
 	INTC_VECT(IRQ0A, 0x0200), INTC_VECT(IRQ1A, 0x0220),
 	INTC_VECT(IRQ2A, 0x0240), INTC_VECT(IRQ3A, 0x0260),
 	INTC_VECT(IRQ4A, 0x0280), INTC_VECT(IRQ5A, 0x02a0),
@@ -162,7 +162,7 @@ static struct intc_group intca_groups[] __initdata = {
 	INTC_GROUP(IIC1, IIC1_ALI1, IIC1_TACKI1, IIC1_WAITI1, IIC1_DTEI1),
 };
 
-static struct intc_mask_reg intca_mask_registers[] = {
+static struct intc_mask_reg intca_mask_registers[] __initdata = {
 	{ 0xe6900040, 0xe6900060, 8, /* INTMSK00A / INTMSKCLR00A */
 	  { IRQ0A, IRQ1A, IRQ2A, IRQ3A, IRQ4A, IRQ5A, IRQ6A, IRQ7A } },
 	{ 0xe6900044, 0xe6900064, 8, /* INTMSK10A / INTMSKCLR10A */
@@ -211,7 +211,7 @@ static struct intc_mask_reg intca_mask_registers[] = {
 	    MISTY, CMT3, RWDT1, RWDT0 } },
 };
 
-static struct intc_prio_reg intca_prio_registers[] = {
+static struct intc_prio_reg intca_prio_registers[] __initdata = {
 	{ 0xe6900010, 0, 32, 4, /* INTPRI00A */
 	  { IRQ0A, IRQ1A, IRQ2A, IRQ3A, IRQ4A, IRQ5A, IRQ6A, IRQ7A } },
 	{ 0xe6900014, 0, 32, 4, /* INTPRI10A */
diff --git a/arch/arm/mach-shmobile/intc-sh7377.c b/arch/arm/mach-shmobile/intc-sh7377.c
index 5c781e2..68901a4 100644
--- a/arch/arm/mach-shmobile/intc-sh7377.c
+++ b/arch/arm/mach-shmobile/intc-sh7377.c
@@ -90,7 +90,7 @@ enum {
 	ICUSB, ICUDMC
 };
 
-static struct intc_vect intca_vectors[] = {
+static struct intc_vect intca_vectors[] __initdata = {
 	INTC_VECT(IRQ0A, 0x0200), INTC_VECT(IRQ1A, 0x0220),
 	INTC_VECT(IRQ2A, 0x0240), INTC_VECT(IRQ3A, 0x0260),
 	INTC_VECT(IRQ4A, 0x0280), INTC_VECT(IRQ5A, 0x02a0),
@@ -202,7 +202,7 @@ static struct intc_group intca_groups[] __initdata = {
 	INTC_GROUP(ICUDMC, ICUDMC_ICUDMC1, ICUDMC_ICUDMC2),
 };
 
-static struct intc_mask_reg intca_mask_registers[] = {
+static struct intc_mask_reg intca_mask_registers[] __initdata = {
 	{ 0xe6900040, 0xe6900060, 8, /* INTMSK00A / INTMSKCLR00A */
 	  { IRQ0A, IRQ1A, IRQ2A, IRQ3A, IRQ4A, IRQ5A, IRQ6A, IRQ7A } },
 	{ 0xe6900044, 0xe6900064, 8, /* INTMSK10A / INTMSKCLR10A */
@@ -272,7 +272,7 @@ static struct intc_mask_reg intca_mask_registers[] = {
 	    SCIFA6, 0, 0, 0 } },
 };
 
-static struct intc_prio_reg intca_prio_registers[] = {
+static struct intc_prio_reg intca_prio_registers[] __initdata = {
 	{ 0xe6900010, 0, 32, 4, /* INTPRI00A */
 	  { IRQ0A, IRQ1A, IRQ2A, IRQ3A, IRQ4A, IRQ5A, IRQ6A, IRQ7A } },
 	{ 0xe6900014, 0, 32, 4, /* INTPRI10A */
-- 
1.6.3.3

[PATCH] ARM: mach-shmobile: add missing GPIO IRQ configuration on mackerel

[Guennadi Liakhovetski]

SDHI0 card-detect GPIO IRQ on mackarel currently works, because it is the
default configuration of IRQ26. However, we should not rely on this and
should configure the function explicitly.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 arch/arm/mach-shmobile/board-mackerel.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-shmobile/board-mackerel.c b/arch/arm/mach-shmobile/board-mackerel.c
index b577f7c..150122a 100644
--- a/arch/arm/mach-shmobile/board-mackerel.c
+++ b/arch/arm/mach-shmobile/board-mackerel.c
@@ -1512,6 +1512,9 @@ static void __init mackerel_init(void)
 	gpio_request(GPIO_FN_SDHID0_1, NULL);
 	gpio_request(GPIO_FN_SDHID0_0, NULL);
 
+	/* SDHI0 PORT172 card-detect IRQ26 */
+	gpio_request(GPIO_FN_IRQ26_172, NULL);
+
 #if !defined(CONFIG_MMC_SH_MMCIF) && !defined(CONFIG_MMC_SH_MMCIF_MODULE)
 	/* enable SDHI1 */
 	gpio_request(GPIO_FN_SDHICMD1, NULL);
-- 
1.7.2.5

Re: [PATCH] ARM: mach-shmobile: add missing GPIO IRQ configuration on mackerel

[Magnus Damm]

Hi Guennadi,

On Thu, May 24, 2012 at 10:18 PM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> SDHI0 card-detect GPIO IRQ on mackarel currently works, because it is the
> default configuration of IRQ26. However, we should not rely on this and
> should configure the function explicitly.
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

This looks like a regression fix. Can you please point out which
commit that caused this?

Thanks,

/ magnus

Re: [PATCH] ARM: mach-shmobile: add missing GPIO IRQ configuration on mackerel

[Guennadi Liakhovetski]

On Fri, 25 May 2012, Magnus Damm wrote:

> Hi Guennadi,
> 
> On Thu, May 24, 2012 at 10:18 PM, Guennadi Liakhovetski
> <g.liakhovetski@gmx.de> wrote:
> > SDHI0 card-detect GPIO IRQ on mackarel currently works, because it is the
> > default configuration of IRQ26. However, we should not rely on this and
> > should configure the function explicitly.
> >
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> 
> This looks like a regression fix. Can you please point out which
> commit that caused this?

No, I don't think it is one. My original commit, that added the SDHI0 
card-detect GPIO IRQ feature was already missing the configuration call:

commit 4bff4a7ee4b9e487a1bc1d31082e77b6a381418a
Author: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Date:   Wed May 11 16:51:20 2011 +0000

    ARM: arch-shmobile: support SDHI card detection on mackerel, using a GPIO

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] ARM: mach-shmobile: add missing GPIO IRQ configuration on mackerel

[Magnus Damm]

On Fri, May 25, 2012 at 3:52 PM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> On Fri, 25 May 2012, Magnus Damm wrote:
>
>> Hi Guennadi,
>>
>> On Thu, May 24, 2012 at 10:18 PM, Guennadi Liakhovetski
>> <g.liakhovetski@gmx.de> wrote:
>> > SDHI0 card-detect GPIO IRQ on mackarel currently works, because it is the
>> > default configuration of IRQ26. However, we should not rely on this and
>> > should configure the function explicitly.
>> >
>> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>>
>> This looks like a regression fix. Can you please point out which
>> commit that caused this?
>
> No, I don't think it is one. My original commit, that added the SDHI0
> card-detect GPIO IRQ feature was already missing the configuration call:
>
> commit 4bff4a7ee4b9e487a1bc1d31082e77b6a381418a
> Author: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> Date:   Wed May 11 16:51:20 2011 +0000
>
>    ARM: arch-shmobile: support SDHI card detection on mackerel, using a GPIO

Ah, so you mean that this breakage wasn't created recently enough and
therefore not a regression?

Just to know what is happening, I believe the breakage was caused by
this commit:

commit 2b3e38c4fbeb88092390f7c29b4934212abe9ded
Author: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Date:   Mon Apr 16 23:09:13 2012 +0200

    ARM: mach-shmobile: convert mackerel to use the generic MMC GPIO
hotplug helper

   This also fixes the following modular mmc build failure:

    arch/arm/mach-shmobile/built-in.o: In function `ag5evm_sdhi0_gpio_cd':
    pfc-sh73a0.c:(.text+0x7c0): undefined reference to `mmc_detect_change'

It contains this lovely hunk:

@@ -1481,7 +1471,6 @@ static void __init mackerel_init(void)
        irq_set_irq_type(IRQ21, IRQ_TYPE_LEVEL_HIGH);

        /* enable SDHI0 */
-       gpio_request(GPIO_FN_SDHICD0, NULL);
        gpio_request(GPIO_FN_SDHIWP0, NULL);
        gpio_request(GPIO_FN_SDHICMD0, NULL);
        gpio_request(GPIO_FN_SDHICLK0, NULL);

So I believe it worked fine before that commit, but of course the
above commit fixed another error.

I have now exceeded my yearly limit of SD related build and runtime
issues. In the future, _please_ try to cause less breakage.

/ magnus

Re: [PATCH] ARM: mach-shmobile: add missing GPIO IRQ configuration on mackerel

[Guennadi Liakhovetski]

On Sun, 27 May 2012, Magnus Damm wrote:

> On Fri, May 25, 2012 at 3:52 PM, Guennadi Liakhovetski
> <g.liakhovetski@gmx.de> wrote:
> > On Fri, 25 May 2012, Magnus Damm wrote:
> >
> >> Hi Guennadi,
> >>
> >> On Thu, May 24, 2012 at 10:18 PM, Guennadi Liakhovetski
> >> <g.liakhovetski@gmx.de> wrote:
> >> > SDHI0 card-detect GPIO IRQ on mackarel currently works, because it is the
> >> > default configuration of IRQ26. However, we should not rely on this and
> >> > should configure the function explicitly.
> >> >
> >> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> >>
> >> This looks like a regression fix. Can you please point out which
> >> commit that caused this?
> >
> > No, I don't think it is one. My original commit, that added the SDHI0
> > card-detect GPIO IRQ feature was already missing the configuration call:
> >
> > commit 4bff4a7ee4b9e487a1bc1d31082e77b6a381418a
> > Author: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > Date:   Wed May 11 16:51:20 2011 +0000
> >
> >    ARM: arch-shmobile: support SDHI card detection on mackerel, using a GPIO
> 
> Ah, so you mean that this breakage wasn't created recently enough and
> therefore not a regression?
> 
> Just to know what is happening, I believe the breakage was caused by
> this commit:
> 
> commit 2b3e38c4fbeb88092390f7c29b4934212abe9ded
> Author: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> Date:   Mon Apr 16 23:09:13 2012 +0200
> 
>     ARM: mach-shmobile: convert mackerel to use the generic MMC GPIO
> hotplug helper
> 
>    This also fixes the following modular mmc build failure:
> 
>     arch/arm/mach-shmobile/built-in.o: In function `ag5evm_sdhi0_gpio_cd':
>     pfc-sh73a0.c:(.text+0x7c0): undefined reference to `mmc_detect_change'
> 
> It contains this lovely hunk:
> 
> @@ -1481,7 +1471,6 @@ static void __init mackerel_init(void)
>         irq_set_irq_type(IRQ21, IRQ_TYPE_LEVEL_HIGH);
> 
>         /* enable SDHI0 */
> -       gpio_request(GPIO_FN_SDHICD0, NULL);
>         gpio_request(GPIO_FN_SDHIWP0, NULL);
>         gpio_request(GPIO_FN_SDHICMD0, NULL);
>         gpio_request(GPIO_FN_SDHICLK0, NULL);
> 
> So I believe it worked fine before that commit, but of course the
> above commit fixed another error.

I'm not sure this is the case. What is needed for PORT172 to be configured 
to trigger IRQ26 is a write to MSEL1CR, clearing bit 26:

	PINMUX_DATA(IRQ26_121_MARK,	PORT121_FN0,	MSEL1CR_26_1),
	PINMUX_DATA(IRQ26_172_MARK,	PORT172_FN0,	MSEL1CR_26_0),

right? Did the above

	gpio_request(GPIO_FN_SDHICD0, NULL);

really do that? My understanding was, that the above configured this

	PINMUX_DATA(SDHICD0_MARK,		PORT172_FN1),

Was I wrong?

Thanks
Guennadi

> I have now exceeded my yearly limit of SD related build and runtime
> issues. In the future, _please_ try to cause less breakage.
> 
> / magnus

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] ARM: mach-shmobile: add missing GPIO IRQ configuration on mackerel

[Magnus Damm]

Hi Guennadi,

On Sun, May 27, 2012 at 5:54 PM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> On Sun, 27 May 2012, Magnus Damm wrote:
>
>> On Fri, May 25, 2012 at 3:52 PM, Guennadi Liakhovetski
>> <g.liakhovetski@gmx.de> wrote:
>> > On Fri, 25 May 2012, Magnus Damm wrote:
>> >
>> >> Hi Guennadi,
>> >>
>> >> On Thu, May 24, 2012 at 10:18 PM, Guennadi Liakhovetski
>> >> <g.liakhovetski@gmx.de> wrote:
>> >> > SDHI0 card-detect GPIO IRQ on mackarel currently works, because it is the
>> >> > default configuration of IRQ26. However, we should not rely on this and
>> >> > should configure the function explicitly.
>> >> >
>> >> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>> >>
>> >> This looks like a regression fix. Can you please point out which
>> >> commit that caused this?
>> >
>> > No, I don't think it is one. My original commit, that added the SDHI0
>> > card-detect GPIO IRQ feature was already missing the configuration call:
>> >
>> > commit 4bff4a7ee4b9e487a1bc1d31082e77b6a381418a
>> > Author: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>> > Date:   Wed May 11 16:51:20 2011 +0000
>> >
>> >    ARM: arch-shmobile: support SDHI card detection on mackerel, using a GPIO
>>
>> Ah, so you mean that this breakage wasn't created recently enough and
>> therefore not a regression?
>>
>> Just to know what is happening, I believe the breakage was caused by
>> this commit:
>>
>> commit 2b3e38c4fbeb88092390f7c29b4934212abe9ded
>> Author: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>> Date:   Mon Apr 16 23:09:13 2012 +0200
>>
>>     ARM: mach-shmobile: convert mackerel to use the generic MMC GPIO
>> hotplug helper
>>
>>    This also fixes the following modular mmc build failure:
>>
>>     arch/arm/mach-shmobile/built-in.o: In function `ag5evm_sdhi0_gpio_cd':
>>     pfc-sh73a0.c:(.text+0x7c0): undefined reference to `mmc_detect_change'
>>
>> It contains this lovely hunk:
>>
>> @@ -1481,7 +1471,6 @@ static void __init mackerel_init(void)
>>         irq_set_irq_type(IRQ21, IRQ_TYPE_LEVEL_HIGH);
>>
>>         /* enable SDHI0 */
>> -       gpio_request(GPIO_FN_SDHICD0, NULL);
>>         gpio_request(GPIO_FN_SDHIWP0, NULL);
>>         gpio_request(GPIO_FN_SDHICMD0, NULL);
>>         gpio_request(GPIO_FN_SDHICLK0, NULL);
>>
>> So I believe it worked fine before that commit, but of course the
>> above commit fixed another error.
>
> I'm not sure this is the case. What is needed for PORT172 to be configured
> to trigger IRQ26 is a write to MSEL1CR, clearing bit 26:
>
>        PINMUX_DATA(IRQ26_121_MARK,     PORT121_FN0,    MSEL1CR_26_1),
>        PINMUX_DATA(IRQ26_172_MARK,     PORT172_FN0,    MSEL1CR_26_0),
>
> right? Did the above
>
>        gpio_request(GPIO_FN_SDHICD0, NULL);
>
> really do that? My understanding was, that the above configured this
>
>        PINMUX_DATA(SDHICD0_MARK,               PORT172_FN1),
>
> Was I wrong?

My point is that the code worked fine with the native SDHI CD hotplug
and after that it all seems to go downhill.

The original board code uses the SDHI native CD pin function for
hotplug. In such a case you should request GPIO_FN_SDHICD0 or perhaps
the pull-up version of that pin function.

When moving over to GPIO hotplug then you obviously shouldn't use the
CD pin function. You should use GPIO, and in this particular case
GPIO_FN_IRQ26_172.

It's as easy as that.

To clarify and map out invalid ways to use the hardware, if you use
the SDHI native CD without requesting the CD pin function then you're
relying on reset value or some boot loader setting. This is wrong, so
don't do that. If you're using the SDHI native CD but requesting the
GPIO pin then you're also wrong. You are relying on reserved behavior
of the hardware. This may work on some certain ES of the device but
not for instance on newer ones. So don't rely on reserved behavior. If
you use GPIO hotplug without requesting the pin then you're also
wrong. In general, if you are using a pin without requesting it then
you're wrong. Also, regarding IRQ pins, certain SoCs have multiple
versions of for instance IRQ26. So you need to request the correct pin
in that case too so you don't rely on some hardware setting that isn't
mapped out by software.

With the above in mind, commit
4bff4a7ee4b9e487a1bc1d31082e77b6a381418a is incorrect because it is
using SDHI native CD but assuming that you would get GPIO-based
interrupts. I assumed you developed this spending more time hacking it
until it "works" than actually thinking of if what you're doing is
right or not. Or perhaps it is difficult to tell right from wrong from
your side. Anyway, it may happen to work with this commit applied, but
just because it "works" doesn't mean you should write code like that.

Furthermore, commit 2b3e38c4fbeb88092390f7c29b4934212abe9ded is also
incorrect because you remove the SDHI native CD pin function and use
GPIO to read the pin state, but you forget to request the proper pin.
So you rely on the boot loader setting or reset value which is
incorrect. In this case it may also work by luck.

If you have problems using that GPIO with IRQs on a certain board then
don't move over to GPIO hotplug. We don't want hacks. If the MSEL bits
in the PFC code doesn't clear bits as you expect to get IRQs then you
need to map out those dependencies in the PFC code.

In the end, you need to request the correct pin. In the future with
PINCTRL we will use a different framework, but the idea is the same.
And you will get same failures if you forget to request a certain pin
state or if you rely on some reserved functionality.

So _please_ if you are unsure why code is there then don't remove it.
And it seems that I need to review your code in great detail. Sorry
for not doing that properly.

Thanks,

/ magnus

Re: [PATCH] ARM: mach-shmobile: add missing GPIO IRQ configuration on mackerel

[Rafael J. Wysocki]

On Thursday, May 24, 2012, Guennadi Liakhovetski wrote:
> SDHI0 card-detect GPIO IRQ on mackarel currently works, because it is the
> default configuration of IRQ26. However, we should not rely on this and
> should configure the function explicitly.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

Applied to the fixes branch of the renesas.git tree.

Thanks,
Rafael


> ---
>  arch/arm/mach-shmobile/board-mackerel.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-shmobile/board-mackerel.c b/arch/arm/mach-shmobile/board-mackerel.c
> index b577f7c..150122a 100644
> --- a/arch/arm/mach-shmobile/board-mackerel.c
> +++ b/arch/arm/mach-shmobile/board-mackerel.c
> @@ -1512,6 +1512,9 @@ static void __init mackerel_init(void)
>  	gpio_request(GPIO_FN_SDHID0_1, NULL);
>  	gpio_request(GPIO_FN_SDHID0_0, NULL);
>  
> +	/* SDHI0 PORT172 card-detect IRQ26 */
> +	gpio_request(GPIO_FN_IRQ26_172, NULL);
> +
>  #if !defined(CONFIG_MMC_SH_MMCIF) && !defined(CONFIG_MMC_SH_MMCIF_MODULE)
>  	/* enable SDHI1 */
>  	gpio_request(GPIO_FN_SDHICMD1, NULL);
>