Re: [PATCH] serial: 8250_bcm2835aux: Add ACPI support

[Jeremy Linton <jeremy.linton@arm.com>]

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,