| Message ID | 1450880383-29560-4-git-send-email-aleksey.makarov@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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?
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
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?
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; - snprintf(uap->type, sizeof(uap->type), "PL011 rev%u", amba_rev(dev)); + snprintf(uap->type, sizeof(uap->type), "SBSA"); + } else { + 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.ops = &amba_pl011_pops; + + snprintf(uap->type, sizeof(uap->type), "PL011 rev%u", + amba_rev(dev)); + } + uap->port.irq = dev->irq[0]; ret = pl011_setup_port(&dev->dev, uap, &dev->res, portnr); if (ret)