| Message ID | 20220201185001.926338-1-athierry@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | serial: 8250_bcm2835aux: Add ACPI support | expand |
Hi, On 2/1/22 13:24, Florian Fainelli wrote: > > > On 2/1/2022 10:50 AM, Adrien Thierry wrote: >> Add ACPI support to 8250_bcm2835aux driver. This makes it possible to >> use the miniuart on the Raspberry Pi with the tianocore/edk2 UEFI >> firmware. >> >> Signed-off-by: Adrien Thierry <athierry@redhat.com> >> --- >> drivers/tty/serial/8250/8250_bcm2835aux.c | 103 +++++++++++++++++----- >> 1 file changed, 83 insertions(+), 20 deletions(-) >> >> diff --git a/drivers/tty/serial/8250/8250_bcm2835aux.c >> b/drivers/tty/serial/8250/8250_bcm2835aux.c >> index fd95860cd..b904b321e 100644 >> --- a/drivers/tty/serial/8250/8250_bcm2835aux.c >> +++ b/drivers/tty/serial/8250/8250_bcm2835aux.c >> @@ -12,6 +12,7 @@ >> * simultaneously to rs485. >> */ >> +#include <linux/acpi.h> >> #include <linux/clk.h> >> #include <linux/io.h> >> #include <linux/module.h> >> @@ -44,6 +45,10 @@ struct bcm2835aux_data { >> u32 cntl; >> }; >> +struct bcm2835_aux_serial_acpi_driver_data { >> + resource_size_t offset; >> +}; >> + >> static void bcm2835aux_rs485_start_tx(struct uart_8250_port *up) >> { >> if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX)) { >> @@ -82,8 +87,12 @@ static int bcm2835aux_serial_probe(struct >> platform_device *pdev) >> { >> struct uart_8250_port up = { }; >> struct bcm2835aux_data *data; >> + struct bcm2835_aux_serial_acpi_driver_data *acpi_data; >> struct resource *res; >> int ret; >> + resource_size_t mapbase; >> + resource_size_t mapsize; >> + unsigned int uartclk; >> /* allocate the custom structure */ >> data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); >> @@ -108,10 +117,12 @@ static int bcm2835aux_serial_probe(struct >> platform_device *pdev) >> platform_set_drvdata(pdev, data); >> - /* get the clock - this also enables the HW */ >> - data->clk = devm_clk_get(&pdev->dev, NULL); >> - if (IS_ERR(data->clk)) >> - return dev_err_probe(&pdev->dev, PTR_ERR(data->clk), "could >> not get clk\n"); >> + if (dev_of_node(&pdev->dev)) { >> + /* get the clock - this also enables the HW */ >> + data->clk = devm_clk_get(&pdev->dev, NULL); >> + if (IS_ERR(data->clk)) >> + return dev_err_probe(&pdev->dev, PTR_ERR(data->clk), >> "could not get clk\n"); >> + } > > This does not seem necessary, if the clk is NULL when probed via ACPI, > all of the clk_* APIs will deal with that gracefully. If you need not to > treat -ENOENT as a hard error here, consider switching to > devm_clk_get_optional(). Given that you look at the 'clock-frequency' > property, you can still have some generic code, something like: > > if (IS_ERR(data->clk)) { > ret = device_property_read_u32(&pdev->dev, "clock-frequency", > &uartclk); > if (ret) > return dev_err_probe(&pdev->dev, ret, "could not get clk\n"); > } > >> /* get the interrupt */ >> ret = platform_get_irq(pdev, 0); >> @@ -125,20 +136,59 @@ static int bcm2835aux_serial_probe(struct >> platform_device *pdev) >> dev_err(&pdev->dev, "memory resource not found"); >> return -EINVAL; >> } >> - up.port.mapbase = res->start; >> - up.port.mapsize = resource_size(res); >> - >> - /* Check for a fixed line number */ >> - ret = of_alias_get_id(pdev->dev.of_node, "serial"); >> - if (ret >= 0) >> - up.port.line = ret; >> - >> - /* enable the clock as a last step */ >> - ret = clk_prepare_enable(data->clk); >> - if (ret) { >> - dev_err(&pdev->dev, "unable to enable uart clock - %d\n", >> - ret); >> - return ret; > > All of that path can be common, and you can just define an offset to > apply to the resource at the top after you fetched the memory resource. > The offset will be non-0 for ACPI and 0 for non-ACPI. That is, no need > for the intermediate variables and conditional paths whether this is > ACPI apply this offset, or not. > >> + >> + mapbase = res->start; >> + mapsize = resource_size(res); >> + >> + if (has_acpi_companion(&pdev->dev)) { >> + const struct acpi_device_id *match; >> + >> + match = acpi_match_device(pdev->dev.driver->acpi_match_table, >> &pdev->dev); >> + if (!match) >> + return -ENODEV; >> + >> + acpi_data = (struct bcm2835_aux_serial_acpi_driver_data >> *)match->driver_data; >> + >> + /* Some UEFI implementations (e.g. tianocore/edk2 for the >> Raspberry Pi) >> + * describe the miniuart with a base address that encompasses >> the auxiliary >> + * registers shared between the miniuart and spi. >> + * >> + * This is due to historical reasons, see discussion here : >> + * https://edk2.groups.io/g/devel/topic/87501357#84349 >> + * >> + * We need to add the offset between the miniuart and auxiliary >> + * registers to get the real miniuart base address. > > And ACPI on the Pi4 is so widely deployed that fixing the miniuart > resources is not an option at all? This really really continues to > contribute to my impression that ACPI on the Pi4 is a fad more than a > real thing, sorry. The problem again, is that this resource is legacy and used by windows/vmware/etc on both the rpi3 and rpi4. So, unfortunately it cannot really be changed without breaking existing OSs. Thanks,
Hi, On 2/1/22 21:39, Florian Fainelli wrote: > > > On 2/1/2022 12:42 PM, Jeremy Linton wrote: >> Hi, >> >> On 2/1/22 13:24, Florian Fainelli wrote: >>> >>> >>> On 2/1/2022 10:50 AM, Adrien Thierry wrote: >>>> Add ACPI support to 8250_bcm2835aux driver. This makes it possible to >>>> use the miniuart on the Raspberry Pi with the tianocore/edk2 UEFI >>>> firmware. >>>> >>>> Signed-off-by: Adrien Thierry <athierry@redhat.com> >>>> --- >>>> drivers/tty/serial/8250/8250_bcm2835aux.c | 103 >>>> +++++++++++++++++----- >>>> 1 file changed, 83 insertions(+), 20 deletions(-) >>>> >>>> diff --git a/drivers/tty/serial/8250/8250_bcm2835aux.c >>>> b/drivers/tty/serial/8250/8250_bcm2835aux.c >>>> index fd95860cd..b904b321e 100644 >>>> --- a/drivers/tty/serial/8250/8250_bcm2835aux.c >>>> +++ b/drivers/tty/serial/8250/8250_bcm2835aux.c >>>> @@ -12,6 +12,7 @@ >>>> * simultaneously to rs485. >>>> */ >>>> +#include <linux/acpi.h> >>>> #include <linux/clk.h> >>>> #include <linux/io.h> >>>> #include <linux/module.h> >>>> @@ -44,6 +45,10 @@ struct bcm2835aux_data { >>>> u32 cntl; >>>> }; >>>> +struct bcm2835_aux_serial_acpi_driver_data { >>>> + resource_size_t offset; >>>> +}; >>>> + >>>> static void bcm2835aux_rs485_start_tx(struct uart_8250_port *up) >>>> { >>>> if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX)) { >>>> @@ -82,8 +87,12 @@ static int bcm2835aux_serial_probe(struct >>>> platform_device *pdev) >>>> { >>>> struct uart_8250_port up = { }; >>>> struct bcm2835aux_data *data; >>>> + struct bcm2835_aux_serial_acpi_driver_data *acpi_data; >>>> struct resource *res; >>>> int ret; >>>> + resource_size_t mapbase; >>>> + resource_size_t mapsize; >>>> + unsigned int uartclk; >>>> /* allocate the custom structure */ >>>> data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); >>>> @@ -108,10 +117,12 @@ static int bcm2835aux_serial_probe(struct >>>> platform_device *pdev) >>>> platform_set_drvdata(pdev, data); >>>> - /* get the clock - this also enables the HW */ >>>> - data->clk = devm_clk_get(&pdev->dev, NULL); >>>> - if (IS_ERR(data->clk)) >>>> - return dev_err_probe(&pdev->dev, PTR_ERR(data->clk), "could >>>> not get clk\n"); >>>> + if (dev_of_node(&pdev->dev)) { >>>> + /* get the clock - this also enables the HW */ >>>> + data->clk = devm_clk_get(&pdev->dev, NULL); >>>> + if (IS_ERR(data->clk)) >>>> + return dev_err_probe(&pdev->dev, PTR_ERR(data->clk), >>>> "could not get clk\n"); >>>> + } >>> >>> This does not seem necessary, if the clk is NULL when probed via >>> ACPI, all of the clk_* APIs will deal with that gracefully. If you >>> need not to treat -ENOENT as a hard error here, consider switching to >>> devm_clk_get_optional(). Given that you look at the 'clock-frequency' >>> property, you can still have some generic code, something like: >>> >>> if (IS_ERR(data->clk)) { >>> ret = device_property_read_u32(&pdev->dev, >>> "clock-frequency", &uartclk); >>> if (ret) >>> return dev_err_probe(&pdev->dev, ret, "could not get >>> clk\n"); >>> } >>> >>>> /* get the interrupt */ >>>> ret = platform_get_irq(pdev, 0); >>>> @@ -125,20 +136,59 @@ static int bcm2835aux_serial_probe(struct >>>> platform_device *pdev) >>>> dev_err(&pdev->dev, "memory resource not found"); >>>> return -EINVAL; >>>> } >>>> - up.port.mapbase = res->start; >>>> - up.port.mapsize = resource_size(res); >>>> - >>>> - /* Check for a fixed line number */ >>>> - ret = of_alias_get_id(pdev->dev.of_node, "serial"); >>>> - if (ret >= 0) >>>> - up.port.line = ret; >>>> - >>>> - /* enable the clock as a last step */ >>>> - ret = clk_prepare_enable(data->clk); >>>> - if (ret) { >>>> - dev_err(&pdev->dev, "unable to enable uart clock - %d\n", >>>> - ret); >>>> - return ret; >>> >>> All of that path can be common, and you can just define an offset to >>> apply to the resource at the top after you fetched the memory >>> resource. The offset will be non-0 for ACPI and 0 for non-ACPI. That >>> is, no need for the intermediate variables and conditional paths >>> whether this is ACPI apply this offset, or not. >>> >>>> + >>>> + mapbase = res->start; >>>> + mapsize = resource_size(res); >>>> + >>>> + if (has_acpi_companion(&pdev->dev)) { >>>> + const struct acpi_device_id *match; >>>> + >>>> + match = >>>> acpi_match_device(pdev->dev.driver->acpi_match_table, &pdev->dev); >>>> + if (!match) >>>> + return -ENODEV; >>>> + >>>> + acpi_data = (struct bcm2835_aux_serial_acpi_driver_data >>>> *)match->driver_data; >>>> + >>>> + /* Some UEFI implementations (e.g. tianocore/edk2 for the >>>> Raspberry Pi) >>>> + * describe the miniuart with a base address that >>>> encompasses the auxiliary >>>> + * registers shared between the miniuart and spi. >>>> + * >>>> + * This is due to historical reasons, see discussion here : >>>> + * https://edk2.groups.io/g/devel/topic/87501357#84349 >>>> + * >>>> + * We need to add the offset between the miniuart and >>>> auxiliary >>>> + * registers to get the real miniuart base address. >>> >>> And ACPI on the Pi4 is so widely deployed that fixing the miniuart >>> resources is not an option at all? This really really continues to >>> contribute to my impression that ACPI on the Pi4 is a fad more than a >>> real thing, sorry. >> >> The problem again, is that this resource is legacy and used by >> windows/vmware/etc on both the rpi3 and rpi4. So, unfortunately it >> cannot really be changed without breaking existing OSs. > > Cannot we create another entry that only Linux would match? This is the > first time that an attempt for ACPIfying the 8250_bcm2835aux is done as > far as upstream is concerned so any cruft of legacy that is artificial > should really be avoided and this appear to be some. Well, then we would have a SPCR+DBG2 mismatch when used as a console or debug port (which is an option here since the DT being used by the low level vc4 firmware selects between the console or bluetooth for the serial device. And in windows and likely other OS's the unused device is going to confuse people when it shows up but is missing a driver. So its not really workable without diverging the ACPI tables based on OS. MS actually went and created a new uart type for this device a number of years ago when the initial edk2 port was made, and apparently that definition was somewhat incorrect because it uses this offset to cover the entire block. So it puts us in a bit of a bind, where there were issues with their original definitions (like this). For the most part the edk2 maintainers/rpi OS community tend to work these out, but sometimes they leak through to an OS and require changes. In this case Adrien initially tried to change the uart definition, and it was strongly NAKed by other members because linux is the odd one out here. Thanks
Thanks Florian for your feedback! I have submitted a v2 with the changes you required. Adrien
diff --git a/drivers/tty/serial/8250/8250_bcm2835aux.c b/drivers/tty/serial/8250/8250_bcm2835aux.c index fd95860cd..b904b321e 100644 --- a/drivers/tty/serial/8250/8250_bcm2835aux.c +++ b/drivers/tty/serial/8250/8250_bcm2835aux.c @@ -12,6 +12,7 @@ * simultaneously to rs485. */ +#include <linux/acpi.h> #include <linux/clk.h> #include <linux/io.h> #include <linux/module.h> @@ -44,6 +45,10 @@ struct bcm2835aux_data { u32 cntl; }; +struct bcm2835_aux_serial_acpi_driver_data { + resource_size_t offset; +}; + static void bcm2835aux_rs485_start_tx(struct uart_8250_port *up) { if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX)) { @@ -82,8 +87,12 @@ static int bcm2835aux_serial_probe(struct platform_device *pdev) { struct uart_8250_port up = { }; struct bcm2835aux_data *data; + struct bcm2835_aux_serial_acpi_driver_data *acpi_data; struct resource *res; int ret; + resource_size_t mapbase; + resource_size_t mapsize; + unsigned int uartclk; /* allocate the custom structure */ data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); @@ -108,10 +117,12 @@ static int bcm2835aux_serial_probe(struct platform_device *pdev) platform_set_drvdata(pdev, data); - /* get the clock - this also enables the HW */ - data->clk = devm_clk_get(&pdev->dev, NULL); - if (IS_ERR(data->clk)) - return dev_err_probe(&pdev->dev, PTR_ERR(data->clk), "could not get clk\n"); + if (dev_of_node(&pdev->dev)) { + /* get the clock - this also enables the HW */ + data->clk = devm_clk_get(&pdev->dev, NULL); + if (IS_ERR(data->clk)) + return dev_err_probe(&pdev->dev, PTR_ERR(data->clk), "could not get clk\n"); + } /* get the interrupt */ ret = platform_get_irq(pdev, 0); @@ -125,20 +136,59 @@ static int bcm2835aux_serial_probe(struct platform_device *pdev) dev_err(&pdev->dev, "memory resource not found"); return -EINVAL; } - up.port.mapbase = res->start; - up.port.mapsize = resource_size(res); - - /* Check for a fixed line number */ - ret = of_alias_get_id(pdev->dev.of_node, "serial"); - if (ret >= 0) - up.port.line = ret; - - /* enable the clock as a last step */ - ret = clk_prepare_enable(data->clk); - if (ret) { - dev_err(&pdev->dev, "unable to enable uart clock - %d\n", - ret); - return ret; + + mapbase = res->start; + mapsize = resource_size(res); + + if (has_acpi_companion(&pdev->dev)) { + const struct acpi_device_id *match; + + match = acpi_match_device(pdev->dev.driver->acpi_match_table, &pdev->dev); + if (!match) + return -ENODEV; + + acpi_data = (struct bcm2835_aux_serial_acpi_driver_data *)match->driver_data; + + /* Some UEFI implementations (e.g. tianocore/edk2 for the Raspberry Pi) + * describe the miniuart with a base address that encompasses the auxiliary + * registers shared between the miniuart and spi. + * + * This is due to historical reasons, see discussion here : + * https://edk2.groups.io/g/devel/topic/87501357#84349 + * + * We need to add the offset between the miniuart and auxiliary + * registers to get the real miniuart base address. + */ + up.port.mapbase = mapbase + acpi_data->offset; + up.port.mapsize = mapsize - acpi_data->offset; + } else { + up.port.mapbase = mapbase; + up.port.mapsize = mapsize; + } + + if (dev_of_node(&pdev->dev)) { + /* Check for a fixed line number */ + ret = of_alias_get_id(pdev->dev.of_node, "serial"); + if (ret >= 0) + up.port.line = ret; + + /* enable the clock as a last step */ + ret = clk_prepare_enable(data->clk); + if (ret) { + dev_err(&pdev->dev, "unable to enable uart clock - %d\n", + ret); + return ret; + } + + uartclk = clk_get_rate(data->clk); + + + } else { + ret = device_property_read_u32(&pdev->dev, "clock-frequency", &uartclk); + if (ret) { + dev_err(&pdev->dev, "unable to get clock frequency\n"); + return ret; + } } /* the HW-clock divider for bcm2835aux is 8, @@ -146,7 +196,7 @@ static int bcm2835aux_serial_probe(struct platform_device *pdev) * so we have to multiply the actual clock by 2 * to get identical baudrates. */ - up.port.uartclk = clk_get_rate(data->clk) * 2; + up.port.uartclk = uartclk * 2; /* register the port */ ret = serial8250_register_8250_port(&up); @@ -159,7 +209,9 @@ static int bcm2835aux_serial_probe(struct platform_device *pdev) return 0; dis_clk: - clk_disable_unprepare(data->clk); + if (dev_of_node(&pdev->dev)) + clk_disable_unprepare(data->clk); + return ret; } @@ -173,16 +225,27 @@ static int bcm2835aux_serial_remove(struct platform_device *pdev) return 0; } +static const struct bcm2835_aux_serial_acpi_driver_data bcm2835_acpi_data = { + .offset = 0x40 +}; + static const struct of_device_id bcm2835aux_serial_match[] = { { .compatible = "brcm,bcm2835-aux-uart" }, { }, }; MODULE_DEVICE_TABLE(of, bcm2835aux_serial_match); +static const struct acpi_device_id bcm2835aux_serial_acpi_match[] = { + { "BCM2836", (kernel_ulong_t)&bcm2835_acpi_data }, + { }, +}; +MODULE_DEVICE_TABLE(acpi, bcm2835aux_serial_acpi_match); + static struct platform_driver bcm2835aux_serial_driver = { .driver = { .name = "bcm2835-aux-uart", .of_match_table = bcm2835aux_serial_match, + .acpi_match_table = bcm2835aux_serial_acpi_match, }, .probe = bcm2835aux_serial_probe, .remove = bcm2835aux_serial_remove,
Add ACPI support to 8250_bcm2835aux driver. This makes it possible to use the miniuart on the Raspberry Pi with the tianocore/edk2 UEFI firmware. Signed-off-by: Adrien Thierry <athierry@redhat.com> --- drivers/tty/serial/8250/8250_bcm2835aux.c | 103 +++++++++++++++++----- 1 file changed, 83 insertions(+), 20 deletions(-) base-commit: 26291c54e111ff6ba87a164d85d4a4e134b7315c