[PATCHv2] pinctrl: baytrail: Clear DIRECT_IRQ bit

[PATCHv2] pinctrl: baytrail: Clear DIRECT_IRQ bit

[Loic Poulain]

Direct Irq En bit can be initialized to a bad value.
This bit has to be cleared for io access mode.

Signed-off-by: Loic Poulain <loic.poulain@intel.com>
---
 v2: Apply over ff998356b644ebe723127bd9eec6040b59a4a4f6 + add Warning

 drivers/pinctrl/pinctrl-baytrail.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/pinctrl-baytrail.c b/drivers/pinctrl/pinctrl-baytrail.c
index 701a646..77db3db 100644
--- a/drivers/pinctrl/pinctrl-baytrail.c
+++ b/drivers/pinctrl/pinctrl-baytrail.c
@@ -230,10 +230,13 @@ static int byt_irq_type(struct irq_data *d, unsigned type)
 	spin_lock_irqsave(&vg->lock, flags);
 	value = readl(reg);

+	WARN(value & BYT_DIRECT_IRQ_EN, "Clearing direct_irq_en bit");
+
 	/* For level trigges the BYT_TRIG_POS and BYT_TRIG_NEG bits
 	 * are used to indicate high and low level triggering
 	 */
-	value &= ~(BYT_TRIG_POS | BYT_TRIG_NEG | BYT_TRIG_LVL);
+	value &= ~(BYT_DIRECT_IRQ_EN | BYT_TRIG_POS | BYT_TRIG_NEG |
+		   BYT_TRIG_LVL);

 	switch (type) {
 	case IRQ_TYPE_LEVEL_HIGH:
--
1.8.3.2

Re: [PATCHv2] pinctrl: baytrail: Clear DIRECT_IRQ bit

[Mika Westerberg]

On Wed, Sep 17, 2014 at 03:47:01PM +0200, Loic Poulain wrote:
> Direct Irq En bit can be initialized to a bad value.
> This bit has to be cleared for io access mode.

+Eric

I would like to have a bit better explanation *why* this bit needs to be
cleared.

Also want to ask Eric (who added the WARN()), is there something
preventing us to do this? I remember last time you said that we are not
supposed to change this bit runtime.

My preference is that we get rid of the WARN() and just unconditionally
clear the bit.

> 
> Signed-off-by: Loic Poulain <loic.poulain@intel.com>
> ---
>  v2: Apply over ff998356b644ebe723127bd9eec6040b59a4a4f6 + add Warning
> 
>  drivers/pinctrl/pinctrl-baytrail.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-baytrail.c b/drivers/pinctrl/pinctrl-baytrail.c
> index 701a646..77db3db 100644
> --- a/drivers/pinctrl/pinctrl-baytrail.c
> +++ b/drivers/pinctrl/pinctrl-baytrail.c
> @@ -230,10 +230,13 @@ static int byt_irq_type(struct irq_data *d, unsigned type)
>  	spin_lock_irqsave(&vg->lock, flags);
>  	value = readl(reg);
> 
> +	WARN(value & BYT_DIRECT_IRQ_EN, "Clearing direct_irq_en bit");
> +

I wonder how the average user react when this pops up in the dmesg :-)

>  	/* For level trigges the BYT_TRIG_POS and BYT_TRIG_NEG bits
>  	 * are used to indicate high and low level triggering
>  	 */
> -	value &= ~(BYT_TRIG_POS | BYT_TRIG_NEG | BYT_TRIG_LVL);
> +	value &= ~(BYT_DIRECT_IRQ_EN | BYT_TRIG_POS | BYT_TRIG_NEG |
> +		   BYT_TRIG_LVL);
> 
>  	switch (type) {
>  	case IRQ_TYPE_LEVEL_HIGH:
> --
> 1.8.3.2

Re: [PATCHv2] pinctrl: baytrail: Clear DIRECT_IRQ bit

[Loic Poulain]

Warn seems necessary because we unconditionally change
the pin behavior, I didn't meet any case where direct irq is truly
used on our platform. But maybe it could happen?
Don't want to cause any hidden regression.

Moreover if it is confirmed that is an hardware issue (BIOS),
We can just keep this patch locally as a workaround and revert
it later once our BIOS fixed.

Regards,
Loic

On 18/09/2014 09:49, Mika Westerberg wrote:
> On Wed, Sep 17, 2014 at 03:47:01PM +0200, Loic Poulain wrote:
>> Direct Irq En bit can be initialized to a bad value.
>> This bit has to be cleared for io access mode.
> +Eric
>
> I would like to have a bit better explanation *why* this bit needs to be
> cleared.
>
> Also want to ask Eric (who added the WARN()), is there something
> preventing us to do this? I remember last time you said that we are not
> supposed to change this bit runtime.
>
> My preference is that we get rid of the WARN() and just unconditionally
> clear the bit.
>
>> Signed-off-by: Loic Poulain <loic.poulain@intel.com>
>> ---
>>   v2: Apply over ff998356b644ebe723127bd9eec6040b59a4a4f6 + add Warning
>>
>>   drivers/pinctrl/pinctrl-baytrail.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pinctrl/pinctrl-baytrail.c b/drivers/pinctrl/pinctrl-baytrail.c
>> index 701a646..77db3db 100644
>> --- a/drivers/pinctrl/pinctrl-baytrail.c
>> +++ b/drivers/pinctrl/pinctrl-baytrail.c
>> @@ -230,10 +230,13 @@ static int byt_irq_type(struct irq_data *d, unsigned type)
>>   	spin_lock_irqsave(&vg->lock, flags);
>>   	value = readl(reg);
>>
>> +	WARN(value & BYT_DIRECT_IRQ_EN, "Clearing direct_irq_en bit");
>> +
> I wonder how the average user react when this pops up in the dmesg :-)
>
>>   	/* For level trigges the BYT_TRIG_POS and BYT_TRIG_NEG bits
>>   	 * are used to indicate high and low level triggering
>>   	 */
>> -	value &= ~(BYT_TRIG_POS | BYT_TRIG_NEG | BYT_TRIG_LVL);
>> +	value &= ~(BYT_DIRECT_IRQ_EN | BYT_TRIG_POS | BYT_TRIG_NEG |
>> +		   BYT_TRIG_LVL);
>>
>>   	switch (type) {
>>   	case IRQ_TYPE_LEVEL_HIGH:
>> --
>> 1.8.3.2

-- 
Intel Open Source Technology Center
http://oss.intel.com/

Re: [PATCHv2] pinctrl: baytrail: Clear DIRECT_IRQ bit

[Mika Westerberg]

On Thu, Sep 18, 2014 at 11:31:35AM +0200, Loic Poulain wrote:
> Warn seems necessary because we unconditionally change
> the pin behavior, I didn't meet any case where direct irq is truly
> used on our platform. But maybe it could happen?
> Don't want to cause any hidden regression.

The datasheet says 

"""
This bit should be cleared if io access mode is selected
for this pad
"""

If I interpret it right, it means that whenever this pad is functioning
as a GPIO the flag should be cleared. That's precisely the case here.

> Moreover if it is confirmed that is an hardware issue (BIOS),
> We can just keep this patch locally as a workaround and revert
> it later once our BIOS fixed.

True but only if there aren't any machines out there with this flaw in
the BIOS.

Re: [PATCHv2] pinctrl: baytrail: Clear DIRECT_IRQ bit

[Samuel Ortiz]

Hi Mika,

On Thu, Sep 18, 2014 at 10:49:43AM +0300, Mika Westerberg wrote:
> On Wed, Sep 17, 2014 at 03:47:01PM +0200, Loic Poulain wrote:
> > Direct Irq En bit can be initialized to a bad value.
> > This bit has to be cleared for io access mode.
> 
> +Eric
> 
> I would like to have a bit better explanation *why* this bit needs to be
> cleared.
> 
> Also want to ask Eric (who added the WARN()), is there something
> preventing us to do this? I remember last time you said that we are not
> supposed to change this bit runtime.
> 
> My preference is that we get rid of the WARN() and just unconditionally
> clear the bit.
I'd keep the warn though, as it most likely shows a buggy firmware
implementation.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/
---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris, 
92196 Meudon Cedex, France
Registration Number:  302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCHv2] pinctrl: baytrail: Clear DIRECT_IRQ bit

[Mika Westerberg]

On Thu, Sep 18, 2014 at 11:41:13AM +0200, Samuel Ortiz wrote:
> Hi Mika,
> 
> On Thu, Sep 18, 2014 at 10:49:43AM +0300, Mika Westerberg wrote:
> > On Wed, Sep 17, 2014 at 03:47:01PM +0200, Loic Poulain wrote:
> > > Direct Irq En bit can be initialized to a bad value.
> > > This bit has to be cleared for io access mode.
> > 
> > +Eric
> > 
> > I would like to have a bit better explanation *why* this bit needs to be
> > cleared.
> > 
> > Also want to ask Eric (who added the WARN()), is there something
> > preventing us to do this? I remember last time you said that we are not
> > supposed to change this bit runtime.
> > 
> > My preference is that we get rid of the WARN() and just unconditionally
> > clear the bit.
> I'd keep the warn though, as it most likely shows a buggy firmware
> implementation.

Fair enough :)

Maybe it could be more informative.

Re: [PATCHv2] pinctrl: baytrail: Clear DIRECT_IRQ bit

[eric ernst]

On 14-09-18 02:55 AM, Mika Westerberg wrote:
> On Thu, Sep 18, 2014 at 11:41:13AM +0200, Samuel Ortiz wrote:
>> Hi Mika,
>>
>> On Thu, Sep 18, 2014 at 10:49:43AM +0300, Mika Westerberg wrote:
>>> On Wed, Sep 17, 2014 at 03:47:01PM +0200, Loic Poulain wrote:
>>>> Direct Irq En bit can be initialized to a bad value.
>>>> This bit has to be cleared for io access mode.
>>> +Eric
>>>
>>> I would like to have a bit better explanation *why* this bit needs to be
>>> cleared.
>>>
>>> Also want to ask Eric (who added the WARN()), is there something
>>> preventing us to do this? I remember last time you said that we are not
>>> supposed to change this bit runtime.
>>>
>>> My preference is that we get rid of the WARN() and just unconditionally
>>> clear the bit.
>> I'd keep the warn though, as it most likely shows a buggy firmware
>> implementation.
> Fair enough :)
>
> Maybe it could be more informative.
Agreed - I think its safest to say "you're shooting yourself in the 
foot," rather than reinforcing what we think are the right pad settings.

Re: [PATCHv2] pinctrl: baytrail: Clear DIRECT_IRQ bit

[Linus Walleij]

On Wed, Sep 17, 2014 at 3:47 PM, Loic Poulain <loic.poulain@intel.com> wrote:

> Direct Irq En bit can be initialized to a bad value.
> This bit has to be cleared for io access mode.
>
> Signed-off-by: Loic Poulain <loic.poulain@intel.com>
> ---
>  v2: Apply over ff998356b644ebe723127bd9eec6040b59a4a4f6 + add Warning

I can't figure out if the maintainers like this patch or not.

If you want it applied, please ACK it explicitly.

Yours,
Linus Walleij

Re: [PATCHv2] pinctrl: baytrail: Clear DIRECT_IRQ bit

[Westerberg, Mika]

On Tue, Sep 23, 2014 at 05:44:43PM +0200, Linus Walleij wrote:
> On Wed, Sep 17, 2014 at 3:47 PM, Loic Poulain <loic.poulain@intel.com> wrote:
> 
> > Direct Irq En bit can be initialized to a bad value.
> > This bit has to be cleared for io access mode.
> >
> > Signed-off-by: Loic Poulain <loic.poulain@intel.com>
> > ---
> >  v2: Apply over ff998356b644ebe723127bd9eec6040b59a4a4f6 + add Warning
> 
> I can't figure out if the maintainers like this patch or not.
> 
> If you want it applied, please ACK it explicitly.

I would like to get a bit better information in the changelog and the
warning message should be more informative. I'll ACK it after that is
done.

Loic, are you going to do that?

Re: [PATCHv2] pinctrl: baytrail: Clear DIRECT_IRQ bit

[Loic Poulain]

Yes I'm going to do a v3.

Regards,
Loic

On 23/09/2014 20:36, Westerberg, Mika wrote:
> On Tue, Sep 23, 2014 at 05:44:43PM +0200, Linus Walleij wrote:
>> On Wed, Sep 17, 2014 at 3:47 PM, Loic Poulain <loic.poulain@intel.com> wrote:
>>
>>> Direct Irq En bit can be initialized to a bad value.
>>> This bit has to be cleared for io access mode.
>>>
>>> Signed-off-by: Loic Poulain <loic.poulain@intel.com>
>>> ---
>>>   v2: Apply over ff998356b644ebe723127bd9eec6040b59a4a4f6 + add Warning
>> I can't figure out if the maintainers like this patch or not.
>>
>> If you want it applied, please ACK it explicitly.
> I would like to get a bit better information in the changelog and the
> warning message should be more informative. I'll ACK it after that is
> done.
>
> Loic, are you going to do that?

-- 
Intel Open Source Technology Center
http://oss.intel.com/