Re: Serial port initialization broken on Armada 370/XP due to "serial: 8250_dw: Don't use UPF_FIXED_TYPE"

Re: Serial port initialization broken on Armada 370/XP due to "serial: 8250_dw: Don't use UPF_FIXED_TYPE"

[Gregory CLEMENT]

On 02/27/2013 03:40 PM, Gregory CLEMENT wrote:
> Hello,
> 

[ I have added linux-serial mailing list as I should added them
initially ]

> when I tried to use the linux-next git tree (next-20130226), I
> encountered a problem during boot: the serial port was no more
> initialized on my Armada XP (ARM SoC) base board . I get:
> 
> [...]
> Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
> sata_mv d00a0000.sata: slots 32 ports 2
> [...]
> turn off boot console earlycon0
> 
> 
> And then nothing.
> 
> So after git bisect I ended to the commit " serial: 8250_dw: Don't use
> UPF_FIXED_TYPE". Then by adding again the UPF_FIXED_TYPE flag (ie
> reverting this commit) I got the usual boot log:
> 
> [...]
> Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
> d0012000.serial: ttyS0 at MMIO 0xd0012000 (irq = 17) is a 8250
> console [ttyS0] enabled, bootconsole disabled
> console [ttyS0] enabled, bootconsole disabled
> d0012100.serial: ttyS1 at MMIO 0xd0012100 (irq = 18) is a 8250
> d0012200.serial: ttyS2 at MMIO 0xd0012200 (irq = 29) is a 8250
> d0012300.serial: ttyS3 at MMIO 0xd0012300 (irq = 30) is a 8250
> sata_mv d00a0000.sata: slots 32 ports 2
> [...]
> Freeing init memory: 2160K
> Starting logging: OK
> Initializing random number generator... done.
> Starting network...
> 
> Welcome to Buildroot
> buildroot login:
> 
> 
> 
> I understand that the purpose of this commit was to let the driver
> find by itself the port type, but I didn't find yet how it managed to
> do it and then why it failed in our case.
> 
> I will continue to investigate but any pointers are welcome.
> 

I found the root of the problem in drivers/tty/serial/8250/8250.c

in the autoconfig() function, when the IIR register is acceded, it is
done using serial_in(), this function return an int but is used as it
have returned a char. There is a lot of implicit cast to a char when
the returned value is put in a char variable, this seems to not be a
problem most of the time. The problematic line is the following:

scratch = serial_in(up, UART_IIR) >> 6;

the shift is done here before any cast or mask, and unfortunately my
hardware send 0xC1C1C1C1, that lead to get a '7' in the scratch
variable instead of a '3'.

Would you agree with this kind of patch to fix the issue?

diff --git a/drivers/tty/serial/8250/8250.c b/drivers/tty/serial/8250/8250.c
index e2ac25a..0b284c6 100644
--- a/drivers/tty/serial/8250/8250.c
+++ b/drivers/tty/serial/8250/8250.c
@@ -1119,7 +1119,7 @@ static void autoconfig(struct uart_8250_port *up, unsigned int probeflags)
        serial_out(up, UART_LCR, 0);

        serial_out(up, UART_FCR, UART_FCR_ENABLE_FIFO);
-       scratch = serial_in(up, UART_IIR) >> 6;
+       scratch = (serial_in(up, UART_IIR) & 0xFF) >> 6;

        switch (scratch) {
        case 0:




-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: Serial port initialization broken on Armada 370/XP due to "serial: 8250_dw: Don't use UPF_FIXED_TYPE"

[Heikki Krogerus]

Hi Gregory.

On Wed, Feb 27, 2013 at 05:08:04PM +0100, Gregory CLEMENT wrote:
> I found the root of the problem in drivers/tty/serial/8250/8250.c
> 
> in the autoconfig() function, when the IIR register is acceded, it is
> done using serial_in(), this function return an int but is used as it
> have returned a char. There is a lot of implicit cast to a char when
> the returned value is put in a char variable, this seems to not be a
> problem most of the time. The problematic line is the following:
> 
> scratch = serial_in(up, UART_IIR) >> 6;
> 
> the shift is done here before any cast or mask, and unfortunately my
> hardware send 0xC1C1C1C1, that lead to get a '7' in the scratch
> variable instead of a '3'.

OK, this is interesting. Why does it return that? dw_apb_uart_db.pdf I
have says that bits 31:8 read as zero?

> Would you agree with this kind of patch to fix the issue?
> 
> diff --git a/drivers/tty/serial/8250/8250.c b/drivers/tty/serial/8250/8250.c
> index e2ac25a..0b284c6 100644
> --- a/drivers/tty/serial/8250/8250.c
> +++ b/drivers/tty/serial/8250/8250.c
> @@ -1119,7 +1119,7 @@ static void autoconfig(struct uart_8250_port *up, unsigned int probeflags)
>         serial_out(up, UART_LCR, 0);
> 
>         serial_out(up, UART_FCR, UART_FCR_ENABLE_FIFO);
> -       scratch = serial_in(up, UART_IIR) >> 6;
> +       scratch = (serial_in(up, UART_IIR) & 0xFF) >> 6;
> 
>         switch (scratch) {
>         case 0:

Instead, can you test if it's enough for you to set the reg-io-width
to 1 instead of 4:

diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
index 4c0abe8..3a87a0e 100644
--- a/arch/arm/boot/dts/armada-370-xp.dtsi
+++ b/arch/arm/boot/dts/armada-370-xp.dtsi
@@ -54,7 +54,7 @@
                                reg = <0xd0012000 0x100>;
                                reg-shift = <2>;
                                interrupts = <41>;
-                               reg-io-width = <4>;
+                               reg-io-width = <1>;
                                status = "disabled";
                };
                serial@d0012100 {
@@ -62,7 +62,7 @@
                                reg = <0xd0012100 0x100>;
                                reg-shift = <2>;
                                interrupts = <42>;
-                               reg-io-width = <4>;
+                               reg-io-width = <1>;
                                status = "disabled";
                };
 
Thanks,

-- 
heikki

Re: Serial port initialization broken on Armada 370/XP due to "serial: 8250_dw: Don't use UPF_FIXED_TYPE"

[Gregory CLEMENT]

Hi Heikki,

On 02/28/2013 10:26 AM, Heikki Krogerus wrote:
> Hi Gregory.
> 
> On Wed, Feb 27, 2013 at 05:08:04PM +0100, Gregory CLEMENT wrote:
>> I found the root of the problem in drivers/tty/serial/8250/8250.c
>>
>> in the autoconfig() function, when the IIR register is acceded, it is
>> done using serial_in(), this function return an int but is used as it
>> have returned a char. There is a lot of implicit cast to a char when
>> the returned value is put in a char variable, this seems to not be a
>> problem most of the time. The problematic line is the following:
>>
>> scratch = serial_in(up, UART_IIR) >> 6;
>>
>> the shift is done here before any cast or mask, and unfortunately my
>> hardware send 0xC1C1C1C1, that lead to get a '7' in the scratch
>> variable instead of a '3'.
> 
> OK, this is interesting. Why does it return that? dw_apb_uart_db.pdf I
> have says that bits 31:8 read as zero?

The UART paragraphs on the Armada 370/XP datasheet also says the same
thing.  Actually for all the register the bits 31:8 should be 0.I
suspect an hardware issue (or let's call it an optimization), as the
upper bits are not supposed to be used.

> 
>> Would you agree with this kind of patch to fix the issue?
>>
>> diff --git a/drivers/tty/serial/8250/8250.c b/drivers/tty/serial/8250/8250.c
>> index e2ac25a..0b284c6 100644
>> --- a/drivers/tty/serial/8250/8250.c
>> +++ b/drivers/tty/serial/8250/8250.c
>> @@ -1119,7 +1119,7 @@ static void autoconfig(struct uart_8250_port *up, unsigned int probeflags)
>>         serial_out(up, UART_LCR, 0);
>>
>>         serial_out(up, UART_FCR, UART_FCR_ENABLE_FIFO);
>> -       scratch = serial_in(up, UART_IIR) >> 6;
>> +       scratch = (serial_in(up, UART_IIR) & 0xFF) >> 6;
>>
>>         switch (scratch) {
>>         case 0:
> 
> Instead, can you test if it's enough for you to set the reg-io-width
> to 1 instead of 4:

Yes indeed it worked and it seems to be the correct description of my
hardware. So I will fix the dtsi file.

However isn't buggy to use a function as it returned a char whereas
it returns an int?

Regards,

> 
> diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
> index 4c0abe8..3a87a0e 100644
> --- a/arch/arm/boot/dts/armada-370-xp.dtsi
> +++ b/arch/arm/boot/dts/armada-370-xp.dtsi
> @@ -54,7 +54,7 @@
>                                 reg = <0xd0012000 0x100>;
>                                 reg-shift = <2>;
>                                 interrupts = <41>;
> -                               reg-io-width = <4>;
> +                               reg-io-width = <1>;
>                                 status = "disabled";
>                 };
>                 serial@d0012100 {
> @@ -62,7 +62,7 @@
>                                 reg = <0xd0012100 0x100>;
>                                 reg-shift = <2>;
>                                 interrupts = <42>;
> -                               reg-io-width = <4>;
> +                               reg-io-width = <1>;
>                                 status = "disabled";
>                 };
>  
> Thanks,
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: Serial port initialization broken on Armada 370/XP due to "serial: 8250_dw: Don't use UPF_FIXED_TYPE"

[Heikki Krogerus]

Hi,

On Thu, Feb 28, 2013 at 12:42:06PM +0100, Gregory CLEMENT wrote:
> >> Would you agree with this kind of patch to fix the issue?
> >>
> >> diff --git a/drivers/tty/serial/8250/8250.c b/drivers/tty/serial/8250/8250.c
> >> index e2ac25a..0b284c6 100644
> >> --- a/drivers/tty/serial/8250/8250.c
> >> +++ b/drivers/tty/serial/8250/8250.c
> >> @@ -1119,7 +1119,7 @@ static void autoconfig(struct uart_8250_port *up, unsigned int probeflags)
> >>         serial_out(up, UART_LCR, 0);
> >>
> >>         serial_out(up, UART_FCR, UART_FCR_ENABLE_FIFO);
> >> -       scratch = serial_in(up, UART_IIR) >> 6;
> >> +       scratch = (serial_in(up, UART_IIR) & 0xFF) >> 6;
> >>
> >>         switch (scratch) {
> >>         case 0:
> > 
> > Instead, can you test if it's enough for you to set the reg-io-width
> > to 1 instead of 4:
> 
> Yes indeed it worked and it seems to be the correct description of my
> hardware. So I will fix the dtsi file.
> 
> However isn't buggy to use a function as it returned a char whereas
> it returns an int?

Yes, the driver should probable be cleaned.

It seems to be happening in quite a few places in 8250.c.
autoconfig_16550a() has pretty much identical code in it, where
UART_IIR is read to unsigned char and shifted without a mask.

Br,

-- 
heikki

Re: Serial port initialization broken on Armada 370/XP due to "serial: 8250_dw: Don't use UPF_FIXED_TYPE"

[Greg Kroah-Hartman]

On Thu, Feb 28, 2013 at 02:34:21PM +0200, Heikki Krogerus wrote:
> Hi,
> 
> On Thu, Feb 28, 2013 at 12:42:06PM +0100, Gregory CLEMENT wrote:
> > >> Would you agree with this kind of patch to fix the issue?
> > >>
> > >> diff --git a/drivers/tty/serial/8250/8250.c b/drivers/tty/serial/8250/8250.c
> > >> index e2ac25a..0b284c6 100644
> > >> --- a/drivers/tty/serial/8250/8250.c
> > >> +++ b/drivers/tty/serial/8250/8250.c
> > >> @@ -1119,7 +1119,7 @@ static void autoconfig(struct uart_8250_port *up, unsigned int probeflags)
> > >>         serial_out(up, UART_LCR, 0);
> > >>
> > >>         serial_out(up, UART_FCR, UART_FCR_ENABLE_FIFO);
> > >> -       scratch = serial_in(up, UART_IIR) >> 6;
> > >> +       scratch = (serial_in(up, UART_IIR) & 0xFF) >> 6;
> > >>
> > >>         switch (scratch) {
> > >>         case 0:
> > > 
> > > Instead, can you test if it's enough for you to set the reg-io-width
> > > to 1 instead of 4:
> > 
> > Yes indeed it worked and it seems to be the correct description of my
> > hardware. So I will fix the dtsi file.
> > 
> > However isn't buggy to use a function as it returned a char whereas
> > it returns an int?
> 
> Yes, the driver should probable be cleaned.
> 
> It seems to be happening in quite a few places in 8250.c.
> autoconfig_16550a() has pretty much identical code in it, where
> UART_IIR is read to unsigned char and shifted without a mask.

Can someone send me the correct fix here, if they want it included in
3.9-final?

thanks,

greg k-h

Re: Serial port initialization broken on Armada 370/XP due to "serial: 8250_dw: Don't use UPF_FIXED_TYPE"

[Jason Cooper]

On Fri, Mar 15, 2013 at 01:24:52PM -0700, Greg Kroah-Hartman wrote:
> On Thu, Feb 28, 2013 at 02:34:21PM +0200, Heikki Krogerus wrote:
> > Hi,
> > 
> > On Thu, Feb 28, 2013 at 12:42:06PM +0100, Gregory CLEMENT wrote:
> > > >> Would you agree with this kind of patch to fix the issue?
> > > >>
> > > >> diff --git a/drivers/tty/serial/8250/8250.c b/drivers/tty/serial/8250/8250.c
> > > >> index e2ac25a..0b284c6 100644
> > > >> --- a/drivers/tty/serial/8250/8250.c
> > > >> +++ b/drivers/tty/serial/8250/8250.c
> > > >> @@ -1119,7 +1119,7 @@ static void autoconfig(struct uart_8250_port *up, unsigned int probeflags)
> > > >>         serial_out(up, UART_LCR, 0);
> > > >>
> > > >>         serial_out(up, UART_FCR, UART_FCR_ENABLE_FIFO);
> > > >> -       scratch = serial_in(up, UART_IIR) >> 6;
> > > >> +       scratch = (serial_in(up, UART_IIR) & 0xFF) >> 6;
> > > >>
> > > >>         switch (scratch) {
> > > >>         case 0:
> > > > 
> > > > Instead, can you test if it's enough for you to set the reg-io-width
> > > > to 1 instead of 4:
> > > 
> > > Yes indeed it worked and it seems to be the correct description of my
> > > hardware. So I will fix the dtsi file.
> > > 
> > > However isn't buggy to use a function as it returned a char whereas
> > > it returns an int?
> > 
> > Yes, the driver should probable be cleaned.
> > 
> > It seems to be happening in quite a few places in 8250.c.
> > autoconfig_16550a() has pretty much identical code in it, where
> > UART_IIR is read to unsigned char and shifted without a mask.
> 
> Can someone send me the correct fix here, if they want it included in
> 3.9-final?

The fix is in arm-soc/fixes slated for merging into 3.9:

  commit e366154f70c54dee3665d1c0f780007e514412f3
  Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>
  Date:   Wed Mar 6 11:23:33 2013 +0100

      arm: mvebu: Reduce reg-io-width with UARTs

      Setting the reg-io-width to 1 byte represents more accurate
      description of the HW.

      This will fix an issue where UART driver causes kernel
      panic during bootup. Gregory CLEMENT traced the issue to
      autoconfig() in 8250.c, where the existence of FIFO is
      checked from UART_IIR register. The register is now read as
      32-bit value as the reg-io-width is set to 4-bytes. The
      retuned value seems to contain bogus data for bits 31:8,
      causing the issue.

      Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
      Cc: Gregory CLEMENT <gregory.clement@free-electrons.com>
      Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
      Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
      Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
      Tested-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
      Tested-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
      Signed-off-by: Jason Cooper <jason@lakedaemon.net>

   arch/arm/boot/dts/armada-370-xp.dtsi | 4 ++--
   arch/arm/boot/dts/armada-xp.dtsi     | 4 ++--

thx,

Jason.

Re: Serial port initialization broken on Armada 370/XP due to "serial: 8250_dw: Don't use UPF_FIXED_TYPE"

[Greg Kroah-Hartman]

On Fri, Mar 15, 2013 at 04:32:51PM -0400, Jason Cooper wrote:
> On Fri, Mar 15, 2013 at 01:24:52PM -0700, Greg Kroah-Hartman wrote:
> > On Thu, Feb 28, 2013 at 02:34:21PM +0200, Heikki Krogerus wrote:
> > > Hi,
> > > 
> > > On Thu, Feb 28, 2013 at 12:42:06PM +0100, Gregory CLEMENT wrote:
> > > > >> Would you agree with this kind of patch to fix the issue?
> > > > >>
> > > > >> diff --git a/drivers/tty/serial/8250/8250.c b/drivers/tty/serial/8250/8250.c
> > > > >> index e2ac25a..0b284c6 100644
> > > > >> --- a/drivers/tty/serial/8250/8250.c
> > > > >> +++ b/drivers/tty/serial/8250/8250.c
> > > > >> @@ -1119,7 +1119,7 @@ static void autoconfig(struct uart_8250_port *up, unsigned int probeflags)
> > > > >>         serial_out(up, UART_LCR, 0);
> > > > >>
> > > > >>         serial_out(up, UART_FCR, UART_FCR_ENABLE_FIFO);
> > > > >> -       scratch = serial_in(up, UART_IIR) >> 6;
> > > > >> +       scratch = (serial_in(up, UART_IIR) & 0xFF) >> 6;
> > > > >>
> > > > >>         switch (scratch) {
> > > > >>         case 0:
> > > > > 
> > > > > Instead, can you test if it's enough for you to set the reg-io-width
> > > > > to 1 instead of 4:
> > > > 
> > > > Yes indeed it worked and it seems to be the correct description of my
> > > > hardware. So I will fix the dtsi file.
> > > > 
> > > > However isn't buggy to use a function as it returned a char whereas
> > > > it returns an int?
> > > 
> > > Yes, the driver should probable be cleaned.
> > > 
> > > It seems to be happening in quite a few places in 8250.c.
> > > autoconfig_16550a() has pretty much identical code in it, where
> > > UART_IIR is read to unsigned char and shifted without a mask.
> > 
> > Can someone send me the correct fix here, if they want it included in
> > 3.9-final?
> 
> The fix is in arm-soc/fixes slated for merging into 3.9:

Nice, I like it when I don't have to do anything....

greg k-h