Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1301183 > unrolled thread

Re: [PATCH v4 3/3] serial: amba-pl011: add ACPI support to AMBA probe

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v4 3/3] serial: amba-pl011: add ACPI support to AMBA probe Timur Tabi <timur@codeaurora.org> - 2016-01-05 00:20 +0100
    Re: [PATCH v4 3/3] serial: amba-pl011: add ACPI support to AMBA probe G Gregory <graeme.gregory@linaro.org> - 2016-01-05 10:00 +0100
      Re: [PATCH v4 3/3] serial: amba-pl011: add ACPI support to AMBA probe Timur Tabi <timur@codeaurora.org> - 2016-01-05 17:30 +0100
        Re: [PATCH v4 3/3] serial: amba-pl011: add ACPI support to AMBA probe Graeme Gregory <gg@slimlogic.co.uk> - 2016-01-06 12:10 +0100
          Re: [PATCH v4 3/3] serial: amba-pl011: add ACPI support to AMBA probe Timur Tabi <timur@codeaurora.org> - 2016-01-11 22:40 +0100

#1301183 — Re: [PATCH v4 3/3] serial: amba-pl011: add ACPI support to AMBA probe

On Wed, Dec 23, 2015 at 8:19 AM, Aleksey Makarov
<aleksey.makarov@linaro.org> wrote:
> From: Graeme Gregory <graeme.gregory@linaro.org>
>
> In ACPI this device is only defined in SBSA mode so
> if we are coming from ACPI use this mode.
>
> Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org>
> Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>
> ---
>  drivers/tty/serial/amba-pl011.c | 37 ++++++++++++++++++++++++++-----------
>  1 file changed, 26 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 899a771..974cb9e 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -2368,18 +2368,33 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
>         if (!uap)
>                 return -ENOMEM;
>
> -       uap->clk = devm_clk_get(&dev->dev, NULL);
> -       if (IS_ERR(uap->clk))
> -               return PTR_ERR(uap->clk);
> -
> -       uap->vendor = vendor;
> -       uap->lcrh_rx = vendor->lcrh_rx;
> -       uap->lcrh_tx = vendor->lcrh_tx;
> -       uap->fifosize = vendor->get_fifosize(dev);
> -       uap->port.irq = dev->irq[0];
> -       uap->port.ops = &amba_pl011_pops;
> +       /* ACPI only defines SBSA variant */
> +       if (has_acpi_companion(&dev->dev)) {
> +               /*
> +                * According to ARM ARMH0011 is currently the only mapping
> +                * of pl011 in ACPI and it's mapped to SBSA UART mode
> +                */
> +               uap->vendor     = &vendor_sbsa;
> +               uap->fifosize   = 32;
> +               uap->port.ops   = &sbsa_uart_pops;
> +               uap->fixed_baud = 115200;

I'm confused by this patch.  We already have code like this in
tty-next, in the form of sbsa_uart_probe():

https://kernel.googlesource.com/pub/scm/linux/kernel/git/gregkh/tty/+/tty-next/drivers/tty/serial/amba-pl011.c#2553

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1301332

On 4 January 2016 at 23:13, Timur Tabi <timur@codeaurora.org> wrote:
> On Wed, Dec 23, 2015 at 8:19 AM, Aleksey Makarov
> <aleksey.makarov@linaro.org> wrote:
>> From: Graeme Gregory <graeme.gregory@linaro.org>
>>
>> In ACPI this device is only defined in SBSA mode so
>> if we are coming from ACPI use this mode.
>>
>> Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org>
>> Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>
>> ---
>>  drivers/tty/serial/amba-pl011.c | 37 ++++++++++++++++++++++++++-----------
>>  1 file changed, 26 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
>> index 899a771..974cb9e 100644
>> --- a/drivers/tty/serial/amba-pl011.c
>> +++ b/drivers/tty/serial/amba-pl011.c
>> @@ -2368,18 +2368,33 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
>>         if (!uap)
>>                 return -ENOMEM;
>>
>> -       uap->clk = devm_clk_get(&dev->dev, NULL);
>> -       if (IS_ERR(uap->clk))
>> -               return PTR_ERR(uap->clk);
>> -
>> -       uap->vendor = vendor;
>> -       uap->lcrh_rx = vendor->lcrh_rx;
>> -       uap->lcrh_tx = vendor->lcrh_tx;
>> -       uap->fifosize = vendor->get_fifosize(dev);
>> -       uap->port.irq = dev->irq[0];
>> -       uap->port.ops = &amba_pl011_pops;
>> +       /* ACPI only defines SBSA variant */
>> +       if (has_acpi_companion(&dev->dev)) {
>> +               /*
>> +                * According to ARM ARMH0011 is currently the only mapping
>> +                * of pl011 in ACPI and it's mapped to SBSA UART mode
>> +                */
>> +               uap->vendor     = &vendor_sbsa;
>> +               uap->fifosize   = 32;
>> +               uap->port.ops   = &sbsa_uart_pops;
>> +               uap->fixed_baud = 115200;
>
> I'm confused by this patch.  We already have code like this in
> tty-next, in the form of sbsa_uart_probe():
>
> https://kernel.googlesource.com/pub/scm/linux/kernel/git/gregkh/tty/+/tty-next/drivers/tty/serial/amba-pl011.c#2553
>
Because Russell expressed unhappiness at that code existing. So this
is an alternative method to do same thing with ACPI.

If the "arm,sbsa-uart" id was added to drivers/of/platform.c as an
AMBA id then the same could be done for DT as well.

Ultimately this patch is optional depending on maintainers opinion!

Graeme
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1301675

G Gregory wrote:
>> >I'm confused by this patch.  We already have code like this in
>> >tty-next, in the form of sbsa_uart_probe():
>> >
>> >https://kernel.googlesource.com/pub/scm/linux/kernel/git/gregkh/tty/+/tty-next/drivers/tty/serial/amba-pl011.c#2553
>> >
> Because Russell expressed unhappiness at that code existing. So this
> is an alternative method to do same thing with ACPI.

FYI, this patch doesn't apply on tty-next as-is, so it would need to be 
updated anyway.  Then again, considering the latest drama with that 
driver, who knows what it will look like?

> If the "arm,sbsa-uart" id was added to drivers/of/platform.c as an
> AMBA id then the same could be done for DT as well.
>
> Ultimately this patch is optional depending on maintainers opinion!

So with this patch, what is the difference between sbsa_uart_probe and 
pl011_probe?  Shouldn't the patch also remove sbsa_uart_probe?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1302654

On Tue, Jan 05, 2016 at 10:23:08AM -0600, Timur Tabi wrote:
> G Gregory wrote:
> >>>I'm confused by this patch.  We already have code like this in
> >>>tty-next, in the form of sbsa_uart_probe():
> >>>
> >>>https://kernel.googlesource.com/pub/scm/linux/kernel/git/gregkh/tty/+/tty-next/drivers/tty/serial/amba-pl011.c#2553
> >>>
> >Because Russell expressed unhappiness at that code existing. So this
> >is an alternative method to do same thing with ACPI.
> 
> FYI, this patch doesn't apply on tty-next as-is, so it would need to be
> updated anyway.  Then again, considering the latest drama with that driver,
> who knows what it will look like?
> 
> >If the "arm,sbsa-uart" id was added to drivers/of/platform.c as an
> >AMBA id then the same could be done for DT as well.
> >
> >Ultimately this patch is optional depending on maintainers opinion!
> 
> So with this patch, what is the difference between sbsa_uart_probe and
> pl011_probe?  Shouldn't the patch also remove sbsa_uart_probe?
> 

One is for amba_device and one is for platform_device and one maintainer
indicated displeasure at platfrom device being in an AMBA driver. So we would
like some guidance from maintainers what direction they would like to take.

We can either drop this patch and leave situation as is (and remove
ARMH0011 from scan handler) or add followup patches to also convert DT
usage of sbsa-uart to amba_device.

Graeme

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1306717

Graeme Gregory wrote:
>> >
>> >So with this patch, what is the difference between sbsa_uart_probe and
>> >pl011_probe?  Shouldn't the patch also remove sbsa_uart_probe?
>> >
> One is for amba_device and one is for platform_device and one maintainer
> indicated displeasure at platfrom device being in an AMBA driver.

Ok, I'm still a little confused, but it sounds to me like your patch 
should have also removed sbsa_uart_probe().

With your patches applied, under what circumstance would 
sbsa_uart_probe() still be called?  The amba-pl011.c driver already 
probes on ARMH0011, so shouldn't that be removed, to avoid a double probe?

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web