[v5,05/11] drivers: PL011: refactor pl011_probe()

Commit Message

Timur Tabi Aug. 9, 2015, 12:59 a.m. UTC

On Thu, May 21, 2015 at 9:26 AM, Andre Przywara <[e-mail]> wrote:
> +static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
> +{
> +       struct uart_amba_port *uap;
> +       struct vendor_data *vendor = id->data;
> +       int portnr, ret;
> +
> +       portnr = pl011_find_free_port();
> +       if (portnr < 0)
> +               return portnr;
> +
> +       uap = devm_kzalloc(&dev->dev, sizeof(struct uart_amba_port),
> +                          GFP_KERNEL);
> +       if (!uap)
> +               return -ENOMEM;
> +
> +       uap->clk = devm_clk_get(&dev->dev, NULL);
> +       if (IS_ERR(uap->clk))
> +               return PTR_ERR(uap->clk);
> +
> +       = vendor;

I'm having trouble with this.  Specifically, I can't get SBSA early
console to work, because uap->vendor is uninitialized when
pl011_early_write() is called.

I don't have a non-SBSA system to test, but looking at the code, I'm
not sure how early console is supposed to work any more.  In both SBSA
and non-SBSA cases, uap->vendor is not initialized until the probe
function is called.  Isn't that too late?  I thought early console was
supposed to be running before the driver is probed.

If I make this change, then early console works on SBSA systems (with
earlycon=pl011,<address>).  But of course, this only works for SBSA
and breaks regular PL011.

Comments

Andre Przywara Sept. 3, 2015, 4:54 p.m. UTC | #1

Hi Timur,

(sorry for the delay, just found your email which arrived during my
vacation)

On 09/08/15 01:59, Timur Tabi wrote:
> On Thu, May 21, 2015 at 9:26 AM, Andre Przywara <[e-mail]> wrote:
>> +static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
>> +{
>> +       struct uart_amba_port *uap;
>> +       struct vendor_data *vendor = id->data;
>> +       int portnr, ret;
>> +
>> +       portnr = pl011_find_free_port();
>> +       if (portnr < 0)
>> +               return portnr;
>> +
>> +       uap = devm_kzalloc(&dev->dev, sizeof(struct uart_amba_port),
>> +                          GFP_KERNEL);
>> +       if (!uap)
>> +               return -ENOMEM;
>> +
>> +       uap->clk = devm_clk_get(&dev->dev, NULL);
>> +       if (IS_ERR(uap->clk))
>> +               return PTR_ERR(uap->clk);
>> +
>> +       = vendor;
> 
> I'm having trouble with this.  Specifically, I can't get SBSA early
> console to work, because uap->vendor is uninitialized when
> pl011_early_write() is called.

I don't get where pl011_early_write() would need any kind of PL011 struct.
It only references structures defined in the serial core, so it's
completely dumb MMIO writes and reads, using only the base address
provided on the command line.
Are you by any chance mixing up early console and the normal console? To
me it looks like early console just uses the three tiny functions
defined just before the declaration and it does not touch vendor at all,
isn't it?

As for the broken part, any chance you were testing -next, which has
some patches that broke early console, as we learned today?[1]

Cheers,
Andre.
[1]:
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-September/367744.html

> 
> I don't have a non-SBSA system to test, but looking at the code, I'm
> not sure how early console is supposed to work any more.  In both SBSA
> and non-SBSA cases, uap->vendor is not initialized until the probe
> function is called.  Isn't that too late?  I thought early console was
> supposed to be running before the driver is probed.
> 
> If I make this change, then early console works on SBSA systems (with
> earlycon=pl011,<address>).  But of course, this only works for SBSA
> and breaks regular PL011.
> 
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -2326,9 +2326,15 @@ static void pl011_early_write(struct console *con, const
>  static int __init pl011_early_console_setup(struct earlycon_device *device,
>                                             const char *opt)
>  {
> +       struct uart_amba_port *uap =
> +           container_of(&device->port, struct uart_amba_port, port);
> +
>         if (!device->port.membase)
>                 return -ENODEV;
> +       if (!uap->vendor)
> +               uap->vendor = &vendor_sbsa;
> +
>         device->con->write = pl011_early_write;
>         return 0;
>  }
>

Timur Tabi Sept. 3, 2015, 6:09 p.m. UTC | #2

On 09/03/2015 11:54 AM, Andre Przywara wrote:

> I don't get where pl011_early_write() would need any kind of PL011 struct.
> It only references structures defined in the serial core, so it's
> completely dumb MMIO writes and reads, using only the base address
> provided on the command line.
> Are you by any chance mixing up early console and the normal console? To
> me it looks like early console just uses the three tiny functions
> defined just before the declaration and it does not touch vendor at all,
> isn't it?
>
> As for the broken part, any chance you were testing -next, which has
> some patches that broke early console, as we learned today?[1]

I have to apologize -- I forgot that I applied an internal patch that 
modified this code, and the bug is in that patch, not yours.  Your code 
is fine as is.

Patch

--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -2326,9 +2326,15 @@  static void pl011_early_write(struct console *con, const
 static int __init pl011_early_console_setup(struct earlycon_device *device,
                                            const char *opt)
 {
+       struct uart_amba_port *uap =
+           container_of(&device->port, struct uart_amba_port, port);
+
        if (!device->port.membase)
                return -ENODEV;
+       if (!uap->vendor)
+               uap->vendor = &vendor_sbsa;
+
        device->con->write = pl011_early_write;
        return 0;
 }