[PATCH RFC 0/3] ARM64: bcm2837-rpi: Fix random crashes caused by i2c

[PATCH RFC 0/3] ARM64: bcm2837-rpi: Fix random crashes caused by i2c

[Stefan Wahren]

This patch series tries to fix the random crashes [1] which occur on the RPi 3.

The first patch fix the possible NULL pointer dereference and the rest tries
to fix the root cause of the unexpected interrupts.

Btw this should also fix the random i2c timeouts:

i2c-bcm2835 3f805000.i2c: i2c transfer timed out

In absence of a RPi 3 this is only compile tested.

[1] - https://github.com/anholt/linux/issues/89

Stefan Wahren (3):
  i2c: bcm2835: Avoid possible NULL ptr dereference
  ARM64: bcm2837: remove link to bcm2835
  ARM64: bcm2837-rpi: Fix pwm and i2c pin control

 arch/arm64/boot/dts/broadcom/bcm2835-rpi.dtsi    |    1 -
 arch/arm64/boot/dts/broadcom/bcm2837-rpi-3-b.dts |    2 +-
 arch/arm64/boot/dts/broadcom/bcm2837-rpi.dtsi    |   89 ++++++++++++++++++++++
 drivers/i2c/busses/i2c-bcm2835.c                 |    4 +-
 4 files changed, 93 insertions(+), 3 deletions(-)
 delete mode 120000 arch/arm64/boot/dts/broadcom/bcm2835-rpi.dtsi
 create mode 100644 arch/arm64/boot/dts/broadcom/bcm2837-rpi.dtsi

-- 
1.7.9.5

[PATCH RFC 1/3] i2c: bcm2835: Avoid possible NULL ptr dereference

[Stefan Wahren]

Since commit e2474541032d ("bcm2835: Fix hang for writing messages
larger than 16 bytes") the interrupt handler is prone to a possible
NULL pointer dereference. This could happen if an interrupt fires
before curr_msg is set by bcm2835_i2c_xfer_msg() and randomly occurs
on the RPi 3. Even this is an unexpected behavior the driver must
handle that with an error instead of a crash.

CC: Noralf Trønnes <noralf@tronnes.org>
CC: Martin Sperl <kernel@martin.sperl.org>
Reported-by: Peter Robinson <pbrobinson@gmail.com>
Fixes: e2474541032d ("bcm2835: Fix hang for writing messages larger than 16 bytes")
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
 drivers/i2c/busses/i2c-bcm2835.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
index c3436f6..10e39c8 100644
--- a/drivers/i2c/busses/i2c-bcm2835.c
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -195,7 +195,9 @@ static irqreturn_t bcm2835_i2c_isr(int this_irq, void *data)
 	}
 
 	if (val & BCM2835_I2C_S_DONE) {
-		if (i2c_dev->curr_msg->flags & I2C_M_RD) {
+		if (!i2c_dev->curr_msg) {
+			dev_err(i2c_dev->dev, "Got unexpected interrupt (from firmware?)\n");
+		} else if (i2c_dev->curr_msg->flags & I2C_M_RD) {
 			bcm2835_drain_rxfifo(i2c_dev);
 			val = bcm2835_i2c_readl(i2c_dev, BCM2835_I2C_S);
 		}
-- 
1.7.9.5

Re: [PATCH RFC 1/3] i2c: bcm2835: Avoid possible NULL ptr dereference

[Noralf Trønnes]

Den 16.02.2017 22.20, skrev Stefan Wahren:
> Since commit e2474541032d ("bcm2835: Fix hang for writing messages
> larger than 16 bytes") the interrupt handler is prone to a possible
> NULL pointer dereference. This could happen if an interrupt fires
> before curr_msg is set by bcm2835_i2c_xfer_msg() and randomly occurs
> on the RPi 3. Even this is an unexpected behavior the driver must
> handle that with an error instead of a crash.
>
> CC: Noralf Trønnes <noralf-L59+Z2yzLopAfugRpC6u6w@public.gmane.org>
> CC: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
> Reported-by: Peter Robinson <pbrobinson-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Fixes: e2474541032d ("bcm2835: Fix hang for writing messages larger than 16 bytes")
> Signed-off-by: Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org>
> ---
>   drivers/i2c/busses/i2c-bcm2835.c |    4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
> index c3436f6..10e39c8 100644
> --- a/drivers/i2c/busses/i2c-bcm2835.c
> +++ b/drivers/i2c/busses/i2c-bcm2835.c
> @@ -195,7 +195,9 @@ static irqreturn_t bcm2835_i2c_isr(int this_irq, void *data)
>   	}
>   
>   	if (val & BCM2835_I2C_S_DONE) {
> -		if (i2c_dev->curr_msg->flags & I2C_M_RD) {
> +		if (!i2c_dev->curr_msg) {
> +			dev_err(i2c_dev->dev, "Got unexpected interrupt (from firmware?)\n");
> +		} else if (i2c_dev->curr_msg->flags & I2C_M_RD) {
>   			bcm2835_drain_rxfifo(i2c_dev);
>   			val = bcm2835_i2c_readl(i2c_dev, BCM2835_I2C_S);
>   		}

Thanks,

Acked-by: Noralf Trønnes <noralf-L59+Z2yzLopAfugRpC6u6w@public.gmane.org>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 1/3] i2c: bcm2835: Avoid possible NULL ptr dereference

[Stefan Wahren]

Hi,

> Noralf Trønnes <noralf-L59+Z2yzLopAfugRpC6u6w@public.gmane.org> hat am 18. Februar 2017 um 19:34 geschrieben:
> 
> 
> 
> Den 16.02.2017 22.20, skrev Stefan Wahren:
> > Since commit e2474541032d ("bcm2835: Fix hang for writing messages
> > larger than 16 bytes") the interrupt handler is prone to a possible
> > NULL pointer dereference. This could happen if an interrupt fires
> > before curr_msg is set by bcm2835_i2c_xfer_msg() and randomly occurs
> > on the RPi 3. Even this is an unexpected behavior the driver must
> > handle that with an error instead of a crash.
> >
> > CC: Noralf Trønnes <noralf-L59+Z2yzLopAfugRpC6u6w@public.gmane.org>
> > CC: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
> > Reported-by: Peter Robinson <pbrobinson-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > Fixes: e2474541032d ("bcm2835: Fix hang for writing messages larger than 16 bytes")
> > Signed-off-by: Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org>
> > ---
> >   drivers/i2c/busses/i2c-bcm2835.c |    4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
> > index c3436f6..10e39c8 100644
> > --- a/drivers/i2c/busses/i2c-bcm2835.c
> > +++ b/drivers/i2c/busses/i2c-bcm2835.c
> > @@ -195,7 +195,9 @@ static irqreturn_t bcm2835_i2c_isr(int this_irq, void *data)
> >   	}
> >   
> >   	if (val & BCM2835_I2C_S_DONE) {
> > -		if (i2c_dev->curr_msg->flags & I2C_M_RD) {
> > +		if (!i2c_dev->curr_msg) {
> > +			dev_err(i2c_dev->dev, "Got unexpected interrupt (from firmware?)\n");
> > +		} else if (i2c_dev->curr_msg->flags & I2C_M_RD) {
> >   			bcm2835_drain_rxfifo(i2c_dev);
> >   			val = bcm2835_i2c_readl(i2c_dev, BCM2835_I2C_S);
> >   		}
> 
> Thanks,
> 
> Acked-by: Noralf Trønnes <noralf-L59+Z2yzLopAfugRpC6u6w@public.gmane.org>
>

thanks, but i would be more happier to receive feedback for patches 2 and 3.

Stefan
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 1/3] i2c: bcm2835: Avoid possible NULL ptr dereference

[Noralf Trønnes]

Den 20.02.2017 20.40, skrev Stefan Wahren:
> Hi,
>
>> Noralf Trønnes <noralf-L59+Z2yzLopAfugRpC6u6w@public.gmane.org> hat am 18. Februar 2017 um 19:34 geschrieben:
>>
>>
>>
>> Den 16.02.2017 22.20, skrev Stefan Wahren:
>>> Since commit e2474541032d ("bcm2835: Fix hang for writing messages
>>> larger than 16 bytes") the interrupt handler is prone to a possible
>>> NULL pointer dereference. This could happen if an interrupt fires
>>> before curr_msg is set by bcm2835_i2c_xfer_msg() and randomly occurs
>>> on the RPi 3. Even this is an unexpected behavior the driver must
>>> handle that with an error instead of a crash.
>>>
>>> CC: Noralf Trønnes <noralf-L59+Z2yzLopAfugRpC6u6w@public.gmane.org>
>>> CC: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
>>> Reported-by: Peter Robinson <pbrobinson-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>> Fixes: e2474541032d ("bcm2835: Fix hang for writing messages larger than 16 bytes")
>>> Signed-off-by: Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org>
>>> ---
>>>    drivers/i2c/busses/i2c-bcm2835.c |    4 +++-
>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
>>> index c3436f6..10e39c8 100644
>>> --- a/drivers/i2c/busses/i2c-bcm2835.c
>>> +++ b/drivers/i2c/busses/i2c-bcm2835.c
>>> @@ -195,7 +195,9 @@ static irqreturn_t bcm2835_i2c_isr(int this_irq, void *data)
>>>    	}
>>>    
>>>    	if (val & BCM2835_I2C_S_DONE) {
>>> -		if (i2c_dev->curr_msg->flags & I2C_M_RD) {
>>> +		if (!i2c_dev->curr_msg) {
>>> +			dev_err(i2c_dev->dev, "Got unexpected interrupt (from firmware?)\n");
>>> +		} else if (i2c_dev->curr_msg->flags & I2C_M_RD) {
>>>    			bcm2835_drain_rxfifo(i2c_dev);
>>>    			val = bcm2835_i2c_readl(i2c_dev, BCM2835_I2C_S);
>>>    		}
>> Thanks,
>>
>> Acked-by: Noralf Trønnes <noralf-L59+Z2yzLopAfugRpC6u6w@public.gmane.org>
>>
> thanks, but i would be more happier to receive feedback for patches 2 and 3.

I have only worked on dts files downstream and never done any arm64
stuff, so I'm not up to speed on this.

Noralf.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 1/3] i2c: bcm2835: Avoid possible NULL ptr dereference

[Stefan Wahren]

Am 20.02.2017 um 22:22 schrieb Noralf Trønnes:
>
> Den 20.02.2017 20.40, skrev Stefan Wahren:
>> Hi,
>>
>>> Noralf Trønnes <noralf-L59+Z2yzLopAfugRpC6u6w@public.gmane.org> hat am 18. Februar 2017 um 19:34 
>>> geschrieben:
>>>
>>>
>>>
>>> Den 16.02.2017 22.20, skrev Stefan Wahren:
>>>> Since commit e2474541032d ("bcm2835: Fix hang for writing messages
>>>> larger than 16 bytes") the interrupt handler is prone to a possible
>>>> NULL pointer dereference. This could happen if an interrupt fires
>>>> before curr_msg is set by bcm2835_i2c_xfer_msg() and randomly occurs
>>>> on the RPi 3. Even this is an unexpected behavior the driver must
>>>> handle that with an error instead of a crash.
>>>>
>>>> CC: Noralf Trønnes <noralf-L59+Z2yzLopAfugRpC6u6w@public.gmane.org>
>>>> CC: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
>>>> Reported-by: Peter Robinson <pbrobinson-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>> Fixes: e2474541032d ("bcm2835: Fix hang for writing messages larger 
>>>> than 16 bytes")
>>>> Signed-off-by: Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org>
>>>> ---
>>>>    drivers/i2c/busses/i2c-bcm2835.c |    4 +++-
>>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/i2c/busses/i2c-bcm2835.c 
>>>> b/drivers/i2c/busses/i2c-bcm2835.c
>>>> index c3436f6..10e39c8 100644
>>>> --- a/drivers/i2c/busses/i2c-bcm2835.c
>>>> +++ b/drivers/i2c/busses/i2c-bcm2835.c
>>>> @@ -195,7 +195,9 @@ static irqreturn_t bcm2835_i2c_isr(int 
>>>> this_irq, void *data)
>>>>        }
>>>>           if (val & BCM2835_I2C_S_DONE) {
>>>> -        if (i2c_dev->curr_msg->flags & I2C_M_RD) {
>>>> +        if (!i2c_dev->curr_msg) {
>>>> +            dev_err(i2c_dev->dev, "Got unexpected interrupt (from 
>>>> firmware?)\n");
>>>> +        } else if (i2c_dev->curr_msg->flags & I2C_M_RD) {
>>>>                bcm2835_drain_rxfifo(i2c_dev);
>>>>                val = bcm2835_i2c_readl(i2c_dev, BCM2835_I2C_S);
>>>>            }
>>> Thanks,
>>>
>>> Acked-by: Noralf Trønnes <noralf-L59+Z2yzLopAfugRpC6u6w@public.gmane.org>
>>>
>> thanks, but i would be more happier to receive feedback for patches 2 
>> and 3.
>
> I have only worked on dts files downstream and never done any arm64
> stuff, so I'm not up to speed on this.
>
> Noralf.
>

Okay, at the end i only want to know 2 things:

Does new message "Got unexpected interrupt" still appear after applying 
all 3 patches?

Does the annoying message "i2c-bcm2835 3f805000.i2c: i2c transfer timed 
out" still appear after applying all 3 patches?

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 1/3] i2c: bcm2835: Avoid possible NULL ptr dereference

[kernel-TqfNSX0MhmxHKSADF0wUEw]

> On 21.02.2017, at 09:58, Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org> wrote:
> 
> Am 20.02.2017 um 22:22 schrieb Noralf Trønnes:
>> 
>> Den 20.02.2017 20.40, skrev Stefan Wahren:
>>> Hi,
>>> 
>>>> Noralf Trønnes <noralf-L59+Z2yzLopAfugRpC6u6w@public.gmane.org> hat am 18. Februar 2017 um 19:34 geschrieben:
>>>> 
>>>> 
>>>> 
>>>> Den 16.02.2017 22.20, skrev Stefan Wahren:
>>>>> Since commit e2474541032d ("bcm2835: Fix hang for writing messages
>>>>> larger than 16 bytes") the interrupt handler is prone to a possible
>>>>> NULL pointer dereference. This could happen if an interrupt fires
>>>>> before curr_msg is set by bcm2835_i2c_xfer_msg() and randomly occurs
>>>>> on the RPi 3. Even this is an unexpected behavior the driver must
>>>>> handle that with an error instead of a crash.
>>>>> 
>>>>> CC: Noralf Trønnes <noralf-L59+Z2yzLopAfugRpC6u6w@public.gmane.org>
>>>>> CC: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
>>>>> Reported-by: Peter Robinson <pbrobinson-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>>> Fixes: e2474541032d ("bcm2835: Fix hang for writing messages larger than 16 bytes")
>>>>> Signed-off-by: Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org>
>>>>> ---
>>>>>   drivers/i2c/busses/i2c-bcm2835.c |    4 +++-
>>>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>>> 
>>>>> diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
>>>>> index c3436f6..10e39c8 100644
>>>>> --- a/drivers/i2c/busses/i2c-bcm2835.c
>>>>> +++ b/drivers/i2c/busses/i2c-bcm2835.c
>>>>> @@ -195,7 +195,9 @@ static irqreturn_t bcm2835_i2c_isr(int this_irq, void *data)
>>>>>       }
>>>>>          if (val & BCM2835_I2C_S_DONE) {
>>>>> -        if (i2c_dev->curr_msg->flags & I2C_M_RD) {
>>>>> +        if (!i2c_dev->curr_msg) {
>>>>> +            dev_err(i2c_dev->dev, "Got unexpected interrupt (from firmware?)\n");
>>>>> +        } else if (i2c_dev->curr_msg->flags & I2C_M_RD) {
>>>>>               bcm2835_drain_rxfifo(i2c_dev);
>>>>>               val = bcm2835_i2c_readl(i2c_dev, BCM2835_I2C_S);
>>>>>           }
>>>> Thanks,
>>>> 
>>>> Acked-by: Noralf Trønnes <noralf-L59+Z2yzLopAfugRpC6u6w@public.gmane.org>
>>>> 
>>> thanks, but i would be more happier to receive feedback for patches 2 and 3.
>> 
>> I have only worked on dts files downstream and never done any arm64
>> stuff, so I'm not up to speed on this.
>> 
>> Noralf.
>> 
> 
> Okay, at the end i only want to know 2 things:
> 
> Does new message "Got unexpected interrupt" still appear after applying all 3 patches?
> 
> Does the annoying message "i2c-bcm2835 3f805000.i2c: i2c transfer timed out" still appear after applying all 3 patches?

I believe the issue is primarily that i2c0@3f805000 is 
“owned” by the firmware.

This is less of a problem on the rpi1 and rpi2, as there is 
little to no activity on those on i2c0 by the firmware
(besides when using the camera).

But on the rpi3 some Voltage sensors as well as GPIO 
extenders are attached to this bus, which are frequently
read/modified by the firmware.

So in principle this i2c bus should be disabled.
But if it is then there will be an interrupt triggered
on the VC4 core as well as on the ARM core - both will
see the interrupt.
but typically the RT-firmware will respond much faster
and disable the irq sources already before the arm gets
to “detect” the active interrupt in the irq handler.
But sometimes the firmware is not fast enough (or the irq
path is currently active in linux at the right time)
so the interrupt handler will trigger even if there
was no running i2c transfer on that bus.

The patch is basically defensive programming to avoid
such cases.

Martin


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 1/3] i2c: bcm2835: Avoid possible NULL ptr dereference

[Michael Zoran]

On Mon, 2017-02-20 at 22:22 +0100, Noralf Trønnes wrote:
> Den 20.02.2017 20.40, skrev Stefan Wahren:
> > Hi,
> > 
> > > Noralf Trønnes <noralf-L59+Z2yzLopAfugRpC6u6w@public.gmane.org> hat am 18. Februar 2017 um
> > > 19:34 geschrieben:
> > > 
> > > 
> > > 
> > > Den 16.02.2017 22.20, skrev Stefan Wahren:
> > > > Since commit e2474541032d ("bcm2835: Fix hang for writing
> > > > messages
> > > > larger than 16 bytes") the interrupt handler is prone to a
> > > > possible
> > > > NULL pointer dereference. This could happen if an interrupt
> > > > fires
> > > > before curr_msg is set by bcm2835_i2c_xfer_msg() and randomly
> > > > occurs
> > > > on the RPi 3. Even this is an unexpected behavior the driver
> > > > must
> > > > handle that with an error instead of a crash.
> > > > 
> > > > CC: Noralf Trønnes <noralf-L59+Z2yzLopAfugRpC6u6w@public.gmane.org>
> > > > CC: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
> > > > Reported-by: Peter Robinson <pbrobinson-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > > Fixes: e2474541032d ("bcm2835: Fix hang for writing messages
> > > > larger than 16 bytes")
> > > > Signed-off-by: Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org>
> > > > ---
> > > >    drivers/i2c/busses/i2c-bcm2835.c |    4 +++-
> > > >    1 file changed, 3 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/i2c/busses/i2c-bcm2835.c
> > > > b/drivers/i2c/busses/i2c-bcm2835.c
> > > > index c3436f6..10e39c8 100644
> > > > --- a/drivers/i2c/busses/i2c-bcm2835.c
> > > > +++ b/drivers/i2c/busses/i2c-bcm2835.c
> > > > @@ -195,7 +195,9 @@ static irqreturn_t bcm2835_i2c_isr(int
> > > > this_irq, void *data)
> > > >    	}
> > > >    
> > > >    	if (val & BCM2835_I2C_S_DONE) {
> > > > -		if (i2c_dev->curr_msg->flags & I2C_M_RD) {
> > > > +		if (!i2c_dev->curr_msg) {
> > > > +			dev_err(i2c_dev->dev, "Got unexpected
> > > > interrupt (from firmware?)\n");
> > > > +		} else if (i2c_dev->curr_msg->flags &
> > > > I2C_M_RD) {
> > > >    			bcm2835_drain_rxfifo(i2c_dev);
> > > >    			val = bcm2835_i2c_readl(i2c_dev,
> > > > BCM2835_I2C_S);
> > > >    		}
> > > 
> > > Thanks,
> > > 
> > > Acked-by: Noralf Trønnes <noralf-L59+Z2yzLopAfugRpC6u6w@public.gmane.org>
> > > 
> > 
> > thanks, but i would be more happier to receive feedback for patches
> > 2 and 3.
> 
> I have only worked on dts files downstream and never done any arm64
> stuff, so I'm not up to speed on this.
> 
> Noralf.
> 

Just a note, the original downstream driver returns IRQ_NONE in this
case.  Does that make any difference?

Also, the the original driver has the check at the start of the IRQ and
it doesn't log an error message.

I'm wondering if logging at err level is the best since it might make
people think a serious error has happened.  I think by default this
would show up on the console, correct?



--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 1/3] i2c: bcm2835: Avoid possible NULL ptr dereference

[Stefan Wahren]

Am 21.02.2017 um 16:07 schrieb Michael Zoran:
> On Mon, 2017-02-20 at 22:22 +0100, Noralf Trønnes wrote:
>> Den 20.02.2017 20.40, skrev Stefan Wahren:
>>> Hi,
>>>
>>>> Noralf Trønnes <noralf@tronnes.org> hat am 18. Februar 2017 um
>>>> 19:34 geschrieben:
>>>>
>>>>
>>>>
>>>> Den 16.02.2017 22.20, skrev Stefan Wahren:
>>>>> Since commit e2474541032d ("bcm2835: Fix hang for writing
>>>>> messages
>>>>> larger than 16 bytes") the interrupt handler is prone to a
>>>>> possible
>>>>> NULL pointer dereference. This could happen if an interrupt
>>>>> fires
>>>>> before curr_msg is set by bcm2835_i2c_xfer_msg() and randomly
>>>>> occurs
>>>>> on the RPi 3. Even this is an unexpected behavior the driver
>>>>> must
>>>>> handle that with an error instead of a crash.
>>>>>
>>>>> CC: Noralf Trønnes <noralf@tronnes.org>
>>>>> CC: Martin Sperl <kernel@martin.sperl.org>
>>>>> Reported-by: Peter Robinson <pbrobinson@gmail.com>
>>>>> Fixes: e2474541032d ("bcm2835: Fix hang for writing messages
>>>>> larger than 16 bytes")
>>>>> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
>>>>> ---
>>>>>     drivers/i2c/busses/i2c-bcm2835.c |    4 +++-
>>>>>     1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/i2c/busses/i2c-bcm2835.c
>>>>> b/drivers/i2c/busses/i2c-bcm2835.c
>>>>> index c3436f6..10e39c8 100644
>>>>> --- a/drivers/i2c/busses/i2c-bcm2835.c
>>>>> +++ b/drivers/i2c/busses/i2c-bcm2835.c
>>>>> @@ -195,7 +195,9 @@ static irqreturn_t bcm2835_i2c_isr(int
>>>>> this_irq, void *data)
>>>>>     	}
>>>>>     
>>>>>     	if (val & BCM2835_I2C_S_DONE) {
>>>>> -		if (i2c_dev->curr_msg->flags & I2C_M_RD) {
>>>>> +		if (!i2c_dev->curr_msg) {
>>>>> +			dev_err(i2c_dev->dev, "Got unexpected
>>>>> interrupt (from firmware?)\n");
>>>>> +		} else if (i2c_dev->curr_msg->flags &
>>>>> I2C_M_RD) {
>>>>>     			bcm2835_drain_rxfifo(i2c_dev);
>>>>>     			val = bcm2835_i2c_readl(i2c_dev,
>>>>> BCM2835_I2C_S);
>>>>>     		}
>>>> Thanks,
>>>>
>>>> Acked-by: Noralf Trønnes <noralf@tronnes.org>
>>>>
>>> thanks, but i would be more happier to receive feedback for patches
>>> 2 and 3.
>> I have only worked on dts files downstream and never done any arm64
>> stuff, so I'm not up to speed on this.
>>
>> Noralf.
>>
> Just a note, the original downstream driver returns IRQ_NONE in this
> case.  Does that make any difference?

Yes, i decided to handle the IRQ in order to avoid a possible interrupt 
storm. Please look at the github discussion [1] for more details.

>
> Also, the the original driver has the check at the start of the IRQ and
> it doesn't log an error message.
>
> I'm wondering if logging at err level is the best since it might make
> people think a serious error has happened.

It is a serious error and should be fixed by patch 2, 3 and a possible 
patch 4 later to disable i2c0.
But before i disable i2c0 we need the test results of patch 2 and 3. The 
reason why i don't disable i2c at first is that Gerd [2] reported that 
disabling the PWM fixed the i2c timeout issue, which doesn't fit to 
Martins theory.

[1] - https://github.com/anholt/linux/issues/89
[2] - 
http://lists.infradead.org/pipermail/linux-rpi-kernel/2016-June/003969.html

Re: [PATCH RFC 1/3] i2c: bcm2835: Avoid possible NULL ptr dereference

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 939 bytes --]

On Thu, Feb 16, 2017 at 09:20:45PM +0000, Stefan Wahren wrote:
> Since commit e2474541032d ("bcm2835: Fix hang for writing messages
> larger than 16 bytes") the interrupt handler is prone to a possible
> NULL pointer dereference. This could happen if an interrupt fires
> before curr_msg is set by bcm2835_i2c_xfer_msg() and randomly occurs
> on the RPi 3. Even this is an unexpected behavior the driver must
> handle that with an error instead of a crash.
> 
> CC: Noralf Trønnes <noralf@tronnes.org>
> CC: Martin Sperl <kernel@martin.sperl.org>
> Reported-by: Peter Robinson <pbrobinson@gmail.com>
> Fixes: e2474541032d ("bcm2835: Fix hang for writing messages larger than 16 bytes")
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>

Applied to for-next, thanks (will be in 4.11)!

Note for patches 2+3: I usually don't take DTS changes via I2C, so this
likely needs to go via arm-soc or some other bcm tree.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH RFC 1/3] i2c: bcm2835: Avoid possible NULL ptr dereference

[Stefan Wahren]

Hi Greg,
hi Wolfram,

Am 20.02.2017 um 19:22 schrieb Wolfram Sang:
> On Thu, Feb 16, 2017 at 09:20:45PM +0000, Stefan Wahren wrote:
>> Since commit e2474541032d ("bcm2835: Fix hang for writing messages
>> larger than 16 bytes") the interrupt handler is prone to a possible
>> NULL pointer dereference. This could happen if an interrupt fires
>> before curr_msg is set by bcm2835_i2c_xfer_msg() and randomly occurs
>> on the RPi 3. Even this is an unexpected behavior the driver must
>> handle that with an error instead of a crash.
>>
>> CC: Noralf Trønnes <noralf-L59+Z2yzLopAfugRpC6u6w@public.gmane.org>
>> CC: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
>> Reported-by: Peter Robinson <pbrobinson-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> Fixes: e2474541032d ("bcm2835: Fix hang for writing messages larger than 16 bytes")
>> Signed-off-by: Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org>
> Applied to for-next, thanks (will be in 4.11)!
>

since this patch is too late for 4.10, should i resent with CC to stable 
in order to get it into the next 4.10 release?

Stefan
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 1/3] i2c: bcm2835: Avoid possible NULL ptr dereference

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1013 bytes --]


> >On Thu, Feb 16, 2017 at 09:20:45PM +0000, Stefan Wahren wrote:
> >>Since commit e2474541032d ("bcm2835: Fix hang for writing messages
> >>larger than 16 bytes") the interrupt handler is prone to a possible
> >>NULL pointer dereference. This could happen if an interrupt fires
> >>before curr_msg is set by bcm2835_i2c_xfer_msg() and randomly occurs
> >>on the RPi 3. Even this is an unexpected behavior the driver must
> >>handle that with an error instead of a crash.
> >>
> >>CC: Noralf Trønnes <noralf@tronnes.org>
> >>CC: Martin Sperl <kernel@martin.sperl.org>
> >>Reported-by: Peter Robinson <pbrobinson@gmail.com>
> >>Fixes: e2474541032d ("bcm2835: Fix hang for writing messages larger than 16 bytes")
> >>Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> >Applied to for-next, thanks (will be in 4.11)!
> >
> 
> since this patch is too late for 4.10, should i resent with CC to stable in
> order to get it into the next 4.10 release?

It has the Fixes: tag, that will do.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH RFC 1/3] i2c: bcm2835: Avoid possible NULL ptr dereference

[Greg Kroah-Hartman]

On Tue, Feb 21, 2017 at 09:14:03PM +0100, Wolfram Sang wrote:
> 
> > >On Thu, Feb 16, 2017 at 09:20:45PM +0000, Stefan Wahren wrote:
> > >>Since commit e2474541032d ("bcm2835: Fix hang for writing messages
> > >>larger than 16 bytes") the interrupt handler is prone to a possible
> > >>NULL pointer dereference. This could happen if an interrupt fires
> > >>before curr_msg is set by bcm2835_i2c_xfer_msg() and randomly occurs
> > >>on the RPi 3. Even this is an unexpected behavior the driver must
> > >>handle that with an error instead of a crash.
> > >>
> > >>CC: Noralf Trønnes <noralf-L59+Z2yzLopAfugRpC6u6w@public.gmane.org>
> > >>CC: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
> > >>Reported-by: Peter Robinson <pbrobinson-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > >>Fixes: e2474541032d ("bcm2835: Fix hang for writing messages larger than 16 bytes")
> > >>Signed-off-by: Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org>
> > >Applied to for-next, thanks (will be in 4.11)!
> > >
> > 
> > since this patch is too late for 4.10, should i resent with CC to stable in
> > order to get it into the next 4.10 release?
> 
> It has the Fixes: tag, that will do.

But it moves it much lower on my "this needs to get into stable now!"
priority list.  I'll try to remember this one when it goes by...

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 1/3] i2c: bcm2835: Avoid possible NULL ptr dereference

[Stefan Wahren]

Hi Greg,

Am 22.02.2017 um 08:20 schrieb Greg Kroah-Hartman:
> On Tue, Feb 21, 2017 at 09:14:03PM +0100, Wolfram Sang wrote:
>>>> On Thu, Feb 16, 2017 at 09:20:45PM +0000, Stefan Wahren wrote:
>>>>> Since commit e2474541032d ("bcm2835: Fix hang for writing messages
>>>>> larger than 16 bytes") the interrupt handler is prone to a possible
>>>>> NULL pointer dereference. This could happen if an interrupt fires
>>>>> before curr_msg is set by bcm2835_i2c_xfer_msg() and randomly occurs
>>>>> on the RPi 3. Even this is an unexpected behavior the driver must
>>>>> handle that with an error instead of a crash.
>>>>>
>>>>> CC: Noralf Trønnes <noralf@tronnes.org>
>>>>> CC: Martin Sperl <kernel@martin.sperl.org>
>>>>> Reported-by: Peter Robinson <pbrobinson@gmail.com>
>>>>> Fixes: e2474541032d ("bcm2835: Fix hang for writing messages larger than 16 bytes")
>>>>> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
>>>> Applied to for-next, thanks (will be in 4.11)!
>>>>
>>> since this patch is too late for 4.10, should i resent with CC to stable in
>>> order to get it into the next 4.10 release?
>> It has the Fixes: tag, that will do.
> But it moves it much lower on my "this needs to get into stable now!"
> priority list.  I'll try to remember this one when it goes by...
>
> thanks,
>
> greg k-h

should i resend, since i didn't send you the initial patch?

Re: [PATCH RFC 1/3] i2c: bcm2835: Avoid possible NULL ptr dereference

[Greg Kroah-Hartman]

On Tue, Feb 28, 2017 at 01:42:42PM +0100, Stefan Wahren wrote:
> Hi Greg,
> 
> Am 22.02.2017 um 08:20 schrieb Greg Kroah-Hartman:
> > On Tue, Feb 21, 2017 at 09:14:03PM +0100, Wolfram Sang wrote:
> > > > > On Thu, Feb 16, 2017 at 09:20:45PM +0000, Stefan Wahren wrote:
> > > > > > Since commit e2474541032d ("bcm2835: Fix hang for writing messages
> > > > > > larger than 16 bytes") the interrupt handler is prone to a possible
> > > > > > NULL pointer dereference. This could happen if an interrupt fires
> > > > > > before curr_msg is set by bcm2835_i2c_xfer_msg() and randomly occurs
> > > > > > on the RPi 3. Even this is an unexpected behavior the driver must
> > > > > > handle that with an error instead of a crash.
> > > > > > 
> > > > > > CC: Noralf Trønnes <noralf-L59+Z2yzLopAfugRpC6u6w@public.gmane.org>
> > > > > > CC: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
> > > > > > Reported-by: Peter Robinson <pbrobinson-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > > > > Fixes: e2474541032d ("bcm2835: Fix hang for writing messages larger than 16 bytes")
> > > > > > Signed-off-by: Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org>
> > > > > Applied to for-next, thanks (will be in 4.11)!
> > > > > 
> > > > since this patch is too late for 4.10, should i resent with CC to stable in
> > > > order to get it into the next 4.10 release?
> > > It has the Fixes: tag, that will do.
> > But it moves it much lower on my "this needs to get into stable now!"
> > priority list.  I'll try to remember this one when it goes by...
> > 
> > thanks,
> > 
> > greg k-h
> 
> should i resend, since i didn't send you the initial patch?

Sure!
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RFC 2/3] ARM64: bcm2837-rpi: remove link to bcm2835-rpi.dtsi

[Stefan Wahren]

Before we can make any Raspberry Pi specific changes we need
a local copy of bcm2835-rpi.dtsi instead of a link. After that
we include this file and remove the unnecessary link.

Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
 arch/arm64/boot/dts/broadcom/bcm2835-rpi.dtsi    |    1 -
 arch/arm64/boot/dts/broadcom/bcm2837-rpi-3-b.dts |    2 +-
 arch/arm64/boot/dts/broadcom/bcm2837-rpi.dtsi    |   89 ++++++++++++++++++++++
 3 files changed, 90 insertions(+), 2 deletions(-)
 delete mode 120000 arch/arm64/boot/dts/broadcom/bcm2835-rpi.dtsi
 create mode 100644 arch/arm64/boot/dts/broadcom/bcm2837-rpi.dtsi

diff --git a/arch/arm64/boot/dts/broadcom/bcm2835-rpi.dtsi b/arch/arm64/boot/dts/broadcom/bcm2835-rpi.dtsi
deleted file mode 120000
index 3937b77..0000000
--- a/arch/arm64/boot/dts/broadcom/bcm2835-rpi.dtsi
+++ /dev/null
@@ -1 +0,0 @@
-../../../../arm/boot/dts/bcm2835-rpi.dtsi
\ No newline at end of file
diff --git a/arch/arm64/boot/dts/broadcom/bcm2837-rpi-3-b.dts b/arch/arm64/boot/dts/broadcom/bcm2837-rpi-3-b.dts
index c309633..e80db7e 100644
--- a/arch/arm64/boot/dts/broadcom/bcm2837-rpi-3-b.dts
+++ b/arch/arm64/boot/dts/broadcom/bcm2837-rpi-3-b.dts
@@ -1,6 +1,6 @@
 /dts-v1/;
 #include "bcm2837.dtsi"
-#include "bcm2835-rpi.dtsi"
+#include "bcm2837-rpi.dtsi"
 #include "bcm283x-rpi-smsc9514.dtsi"
 #include "bcm283x-rpi-usb-host.dtsi"
 
diff --git a/arch/arm64/boot/dts/broadcom/bcm2837-rpi.dtsi b/arch/arm64/boot/dts/broadcom/bcm2837-rpi.dtsi
new file mode 100644
index 0000000..6ddf7df
--- /dev/null
+++ b/arch/arm64/boot/dts/broadcom/bcm2837-rpi.dtsi
@@ -0,0 +1,89 @@
+#include <dt-bindings/power/raspberrypi-power.h>
+
+/ {
+	memory {
+		device_type = "memory";
+		reg = <0 0x10000000>;
+	};
+
+	leds {
+		compatible = "gpio-leds";
+
+		act {
+			label = "ACT";
+			default-state = "keep";
+			linux,default-trigger = "heartbeat";
+		};
+	};
+
+	soc {
+		firmware: firmware {
+			compatible = "raspberrypi,bcm2835-firmware";
+			mboxes = <&mailbox>;
+		};
+
+		power: power {
+			compatible = "raspberrypi,bcm2835-power";
+			firmware = <&firmware>;
+			#power-domain-cells = <1>;
+		};
+	};
+};
+
+&gpio {
+	pinctrl-names = "default";
+
+	gpioout: gpioout {
+		brcm,pins = <6>;
+		brcm,function = <BCM2835_FSEL_GPIO_OUT>;
+	};
+
+	alt0: alt0 {
+		brcm,pins = <4 5 7 8 9 10 11 14 15>;
+		brcm,function = <BCM2835_FSEL_ALT0>;
+	};
+};
+
+&i2c0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&i2c0_gpio0>;
+	status = "okay";
+	clock-frequency = <100000>;
+};
+
+&i2c1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&i2c1_gpio2>;
+	status = "okay";
+	clock-frequency = <100000>;
+};
+
+&i2c2 {
+	status = "okay";
+};
+
+&sdhci {
+	pinctrl-names = "default";
+	pinctrl-0 = <&emmc_gpio48>;
+	status = "okay";
+	bus-width = <4>;
+};
+
+&pwm {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pwm0_gpio40 &pwm1_gpio45>;
+	status = "okay";
+};
+
+&usb {
+	power-domains = <&power RPI_POWER_DOMAIN_USB>;
+};
+
+&v3d {
+	power-domains = <&power RPI_POWER_DOMAIN_V3D>;
+};
+
+&hdmi {
+	power-domains = <&power RPI_POWER_DOMAIN_HDMI>;
+	status = "okay";
+};
-- 
1.7.9.5

[PATCH RFC 3/3] ARM64: bcm2837-rpi: Fix pwm and i2c pin control

[Stefan Wahren]

According to the firmware DT blob [1] the RPi 3 uses different
pins for i2c0 and pwm.

[1] - https://github.com/raspberrypi/firmware/blob/master/extra/dt-blob.dts

Fixes: 9d56c22a7861 ("ARM: bcm2835: Add devicetree for the Raspberry Pi 3.")
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
 arch/arm64/boot/dts/broadcom/bcm2837-rpi.dtsi |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/broadcom/bcm2837-rpi.dtsi b/arch/arm64/boot/dts/broadcom/bcm2837-rpi.dtsi
index 6ddf7df..9f3f3ed 100644
--- a/arch/arm64/boot/dts/broadcom/bcm2837-rpi.dtsi
+++ b/arch/arm64/boot/dts/broadcom/bcm2837-rpi.dtsi
@@ -46,7 +46,7 @@
 
 &i2c0 {
 	pinctrl-names = "default";
-	pinctrl-0 = <&i2c0_gpio0>;
+	pinctrl-0 = <&i2c0_gpio44>;
 	status = "okay";
 	clock-frequency = <100000>;
 };
@@ -71,7 +71,7 @@
 
 &pwm {
 	pinctrl-names = "default";
-	pinctrl-0 = <&pwm0_gpio40 &pwm1_gpio45>;
+	pinctrl-0 = <&pwm0_gpio40 &pwm1_gpio41>;
 	status = "okay";
 };
 
-- 
1.7.9.5

Re: [PATCH RFC 3/3] ARM64: bcm2837-rpi: Fix pwm and i2c pin control

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 1251 bytes --]

Stefan Wahren <stefan.wahren@i2se.com> writes:

> According to the firmware DT blob [1] the RPi 3 uses different
> pins for i2c0 and pwm.
>
> [1] - https://github.com/raspberrypi/firmware/blob/master/extra/dt-blob.dts
>
> Fixes: 9d56c22a7861 ("ARM: bcm2835: Add devicetree for the Raspberry Pi 3.")
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>

Interesting.  The dt-blob does, I think, basically reflect the state
that the pins will be in when the firmware hands off to us.  Note that
the firmware also, at boot, sets ID_SDA/SCL to function 0 (i2c0), probes
for a HAT ROM and saves it, then restores those pins to whatever mux
they had before (presumably the dt-blob pin_config values).

Linux doesn't have any HAT probing code itself, so leaving pins 0/1 as
GPIO input (most boards) or i2c0 (rev2) should be fine.  I think this
patch should effectively be a no-op, unless you've enabled the pwm node.

My only suggestion for change is: These pin configurations are
board-specific, so could we just move them into the specific board
files, instead?  Basically leave bcm2835-rpi.dtsi as the "generic
configuration of bcm283x for the rpi firmware" rather than "things
common to bcm2835/6 with rpi firmware but not bcm2837 with rpi
firmware".

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH RFC 3/3] ARM64: bcm2837-rpi: Fix pwm and i2c pin control

[Stefan Wahren]

> Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org> hat am 27. Februar 2017 um 21:56 geschrieben:
> 
> 
> Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org> writes:
> 
> > According to the firmware DT blob [1] the RPi 3 uses different
> > pins for i2c0 and pwm.
> >
> > [1] - https://github.com/raspberrypi/firmware/blob/master/extra/dt-blob.dts
> >
> > Fixes: 9d56c22a7861 ("ARM: bcm2835: Add devicetree for the Raspberry Pi 3.")
> > Signed-off-by: Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org>
> 
> Interesting.  The dt-blob does, I think, basically reflect the state
> that the pins will be in when the firmware hands off to us.  Note that
> the firmware also, at boot, sets ID_SDA/SCL to function 0 (i2c0), probes
> for a HAT ROM and saves it, then restores those pins to whatever mux
> they had before (presumably the dt-blob pin_config values).
> 
> Linux doesn't have any HAT probing code itself, so leaving pins 0/1 as
> GPIO input (most boards) or i2c0 (rev2) should be fine.  I think this
> patch should effectively be a no-op, unless you've enabled the pwm node.
> 
> My only suggestion for change is: These pin configurations are
> board-specific, so could we just move them into the specific board
> files, instead? 

Yes, that would be better. In case of pin configurations do you mean only those affected by this patch or all pin configurations?

> Basically leave bcm2835-rpi.dtsi as the "generic
> configuration of bcm283x for the rpi firmware" rather than "things
> common to bcm2835/6 with rpi firmware but not bcm2837 with rpi
> firmware".

If this our intension, shouldn't we rename it to bcm283x-rpi.dtsi?

Ah, now i feel the pain about having symlinks from arm64/dts to arm/dts.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 3/3] ARM64: bcm2837-rpi: Fix pwm and i2c pin control

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 1809 bytes --]

Stefan Wahren <stefan.wahren@i2se.com> writes:

>> Eric Anholt <eric@anholt.net> hat am 27. Februar 2017 um 21:56 geschrieben:
>> 
>> 
>> Stefan Wahren <stefan.wahren@i2se.com> writes:
>> 
>> > According to the firmware DT blob [1] the RPi 3 uses different
>> > pins for i2c0 and pwm.
>> >
>> > [1] - https://github.com/raspberrypi/firmware/blob/master/extra/dt-blob.dts
>> >
>> > Fixes: 9d56c22a7861 ("ARM: bcm2835: Add devicetree for the Raspberry Pi 3.")
>> > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
>> 
>> Interesting.  The dt-blob does, I think, basically reflect the state
>> that the pins will be in when the firmware hands off to us.  Note that
>> the firmware also, at boot, sets ID_SDA/SCL to function 0 (i2c0), probes
>> for a HAT ROM and saves it, then restores those pins to whatever mux
>> they had before (presumably the dt-blob pin_config values).
>> 
>> Linux doesn't have any HAT probing code itself, so leaving pins 0/1 as
>> GPIO input (most boards) or i2c0 (rev2) should be fine.  I think this
>> patch should effectively be a no-op, unless you've enabled the pwm node.
>> 
>> My only suggestion for change is: These pin configurations are
>> board-specific, so could we just move them into the specific board
>> files, instead? 
>
> Yes, that would be better. In case of pin configurations do you mean
> only those affected by this patch or all pin configurations?

I was just thinking about the ones you're changing in this patch.

>> Basically leave bcm2835-rpi.dtsi as the "generic
>> configuration of bcm283x for the rpi firmware" rather than "things
>> common to bcm2835/6 with rpi firmware but not bcm2837 with rpi
>> firmware".
>
> If this our intension, shouldn't we rename it to bcm283x-rpi.dtsi?

That sounds good to me.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]