serial: 8250: Add driver for Broadcom UART

Add DT bindings for the Broadcom 8250 based UART driver. This UART is based on an 8250 but adds additional functionality. The additional features include the ability to use DMA for transfers and a baud rate clock system that is more accurate at high baud rates. This UART is backward compatible with the standard 8250 UART.

Signed-off-by Al Cooper <[email protected]>

---
 .../bindings/serial/brcm,bcm7271-uart.yaml    | 94 +++++++++++++++++++
 1 file changed, 94 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serial/brcm,bcm7271-uart.yaml

Add a UART driver for the new Broadcom 8250 based STB UART. The new UART is backward compatible with the standard 8250, but has some additional features. The new features include a high accuracy baud rate clock system and DMA support.

The driver will use the new optional BAUD MUX clock to select the best one of the four master clocks (81MHz, 108MHz, 64MHz and 48MHz) to feed the baud rate selection logic for any requested baud rate. This allows for more accurate BAUD rates when high speed baud rates are selected.

The driver will use the new UART DMA hardware if the UART DMA registers are specified in Device Tree "reg" property. The DMA functionality can be disabled on kernel boot with the argument:
"8250_bcm7271.disable_dma=Y".

Quoted 10 lines · Al Cooper

Add a UART driver for the new Broadcom 8250 based STB UART. The new UART is backward compatible with the standard 8250, but has some additional features. The new features include a high accuracy baud rate clock system and DMA support.

The driver will use the new optional BAUD MUX clock to select the best one of the four master clocks (81MHz, 108MHz, 64MHz and 48MHz) to feed the baud rate selection logic for any requested baud rate. This allows for more accurate BAUD rates when high speed baud rates are selected.

Quoted 4 lines

The driver will use the new UART DMA hardware if the UART DMA registers are specified in Device Tree "reg" property. The DMA functionality can be disabled on kernel boot with the argument:
"8250_bcm7271.disable_dma=Y".

Shouldn't that be on a per-device basis, and not a per-driver basis?

And why would you want to disable this, if you have support for this in the DT? Why not just rely on the DT setting?

thanks,

greg k-h

Quoted 17 lines · Greg Kroah-Hartman

Quoted 14 lines · Al Cooper

Add a UART driver for the new Broadcom 8250 based STB UART. The new UART is backward compatible with the standard 8250, but has some additional features. The new features include a high accuracy baud rate clock system and DMA support.

The driver will use the new optional BAUD MUX clock to select the best one of the four master clocks (81MHz, 108MHz, 64MHz and 48MHz) to feed the baud rate selection logic for any requested baud rate. This allows for more accurate BAUD rates when high speed baud rates are selected.

The driver will use the new UART DMA hardware if the UART DMA registers are specified in Device Tree "reg" property. The DMA functionality can be disabled on kernel boot with the argument:
"8250_bcm7271.disable_dma=Y".

Quoted 1 line

Shouldn't that be on a per-device basis, and not a per-driver basis?

There is only one instance of the UART DMA hardware and it gets muxed to just one of the possible UARTS.

Quoted 2 lines

And why would you want to disable this, if you have support for this in the DT? Why not just rely on the DT setting?

The DMA feature is used when the UART is connected to a Bluetooth controller and the BAUD rate is typically 2-3Mbs. The ability to easily disable DMA is very useful when debugging BT communication problems in the field. DT settings could also be used to disable DMA, but knowing the correct modifications to the "reg" and "reg-names" properties is a lot more complicated.

Add a UART driver for the new Broadcom 8250 based STB UART. The new UART is backward compatible with the standard 8250, but has some additional features. The new features include a high accuracy baud rate clock system and DMA support.

The driver will use the new optional BAUD MUX clock to select the best one of the four master clocks (81MHz, 108MHz, 64MHz and 48MHz) to feed the baud rate selection logic for any requested baud rate. This allows for more accurate BAUD rates when high speed baud rates are selected.

The driver will use the new UART DMA hardware if the UART DMA registers are specified in Device Tree "reg" property. The DMA functionality can be disabled on kernel boot with the argument:
"8250_bcm7271.disable_dma=Y".

The driver also set the UPSTAT_AUTOCTS flag when hardware flow control is enabled. This flag is needed for UARTs that don't assert a CTS changed interrupt when CTS changes and AFE (Hardware Flow Control) is enabled.

The driver also contains a workaround for a bug in the Synopsis 8250 core. The problem is that at high baud rates, the RX partial FIFO timeout interrupt can occur but there is no RX data (DR not set in the LSR register). In this case the driver will not read the Receive Buffer Register, which clears the interrupt, and the system will get continuous UART interrupts until the next RX character arrives. The fix originally suggested by Synopsis was to read the Receive Buffer Register and discard the character when the DR bit in the LSR was not set, to clear the interrupt. The problem was that occasionally a character would arrive just after the DR bit check and a valid character would be discarded. The fix that was added will clear

receive interrupts to stop the interrupt, deassert RTS to insure that no new data can arrive, wait for 1.5 character times for the sender to react to RTS and then check for data and either do a dummy read or a valid read. Sysfs error counters were also added and were used to help create test software that would cause the error condition. The counters can be found at:
/sys/devices/platform/rdb/*serial/rx_bad_timeout_late_char /sys/devices/platform/rdb/*serial/rx_bad_timeout_no_char

Quoted 32 lines · Al Cooper

Add a UART driver for the new Broadcom 8250 based STB UART. The new UART is backward compatible with the standard 8250, but has some additional features. The new features include a high accuracy baud rate clock system and DMA support.

The driver will use the new optional BAUD MUX clock to select the best one of the four master clocks (81MHz, 108MHz, 64MHz and 48MHz) to feed the baud rate selection logic for any requested baud rate. This allows for more accurate BAUD rates when high speed baud rates are selected.

The driver will use the new UART DMA hardware if the UART DMA registers are specified in Device Tree "reg" property. The DMA functionality can be disabled on kernel boot with the argument:
"8250_bcm7271.disable_dma=Y".

The driver also set the UPSTAT_AUTOCTS flag when hardware flow control is enabled. This flag is needed for UARTs that don't assert a CTS changed interrupt when CTS changes and AFE (Hardware Flow Control) is enabled.

The driver also contains a workaround for a bug in the Synopsis 8250 core. The problem is that at high baud rates, the RX partial FIFO timeout interrupt can occur but there is no RX data (DR not set in the LSR register). In this case the driver will not read the Receive Buffer Register, which clears the interrupt, and the system will get continuous UART interrupts until the next RX character arrives. The fix originally suggested by Synopsis was to read the Receive Buffer Register and discard the character when the DR bit in the LSR was not set, to clear the interrupt. The problem was that occasionally a character would arrive just after the DR bit check and a valid character would be discarded. The fix that was added will clear

Quoted 8 lines

receive interrupts to stop the interrupt, deassert RTS to insure that no new data can arrive, wait for 1.5 character times for the sender to react to RTS and then check for data and either do a dummy read or a valid read. Sysfs error counters were also added and were used to help create test software that would cause the error condition. The counters can be found at:
/sys/devices/platform/rdb/*serial/rx_bad_timeout_late_char /sys/devices/platform/rdb/*serial/rx_bad_timeout_no_char

Brief looking into the code raises several questions:
- is it driver from the last decade?

- why it's not using what kernel provides?
- we have a lot of nice helpers:
- DMA Engine API

- BIT() and GENMASK() macros
- tons of different helpers like regmap API (if you wish to dump
registers via debugfs)

Can you shrink this driver by 20-30% (I truly believe it's possible) and split DMA driver to drivers/dma (which may already have something similar there)?

Quoted 42 lines · Andy Shevchenko

Quoted 32 lines · Al Cooper

Add a UART driver for the new Broadcom 8250 based STB UART. The new UART is backward compatible with the standard 8250, but has some additional features. The new features include a high accuracy baud rate clock system and DMA support.

The driver will use the new optional BAUD MUX clock to select the best one of the four master clocks (81MHz, 108MHz, 64MHz and 48MHz) to feed the baud rate selection logic for any requested baud rate. This allows for more accurate BAUD rates when high speed baud rates are selected.

The driver will use the new UART DMA hardware if the UART DMA registers are specified in Device Tree "reg" property. The DMA functionality can be disabled on kernel boot with the argument:
"8250_bcm7271.disable_dma=Y".

The driver also set the UPSTAT_AUTOCTS flag when hardware flow control is enabled. This flag is needed for UARTs that don't assert a CTS changed interrupt when CTS changes and AFE (Hardware Flow Control) is enabled.

The driver also contains a workaround for a bug in the Synopsis 8250 core. The problem is that at high baud rates, the RX partial FIFO timeout interrupt can occur but there is no RX data (DR not set in the LSR register). In this case the driver will not read the Receive Buffer Register, which clears the interrupt, and the system will get continuous UART interrupts until the next RX character arrives. The fix originally suggested by Synopsis was to read the Receive Buffer Register and discard the character when the DR bit in the LSR was not set, to clear the interrupt. The problem was that occasionally a character would arrive just after the DR bit check and a valid character would be discarded. The fix that was added will clear

Quoted 8 lines

receive interrupts to stop the interrupt, deassert RTS to insure that no new data can arrive, wait for 1.5 character times for the sender to react to RTS and then check for data and either do a dummy read or a valid read. Sysfs error counters were also added and were used to help create test software that would cause the error condition. The counters can be found at:
/sys/devices/platform/rdb/*serial/rx_bad_timeout_late_char /sys/devices/platform/rdb/*serial/rx_bad_timeout_no_char

Quoted 2 lines

Brief looking into the code raises several questions:
- is it driver from the last decade?

Work on this driver started back in 2018, that was indeed the last decade.

Quoted 3 lines

- why it's not using what kernel provides?
- we have a lot of nice helpers:
- DMA Engine API

Not sure this makes sense, given that the DMA hardware that was added to this UART block is only used by the UART block and no other pieces of HW in the system, nor will they ever be. Not sure it makes sense to pay the cost of an extra indirection and subsystem unless there are at least two consumers of that DMA hardware to warrant modeling it after a dmaengine driver. I also remember that Al researched before whether 8250_dma.c could work, and came to the conclusion that it would not, but I will let him comment on the specifics.

Quoted 7 lines

- BIT() and GENMASK() macros
- tons of different helpers like regmap API (if you wish to dump
registers via debugfs)

Can you shrink this driver by 20-30% (I truly believe it's possible) and split DMA driver to drivers/dma (which may already have something similar there)?

See previous response.

Quoted 52 lines · Florian Fainelli

Quoted 42 lines · Andy Shevchenko

Quoted 32 lines · Al Cooper

Add a UART driver for the new Broadcom 8250 based STB UART. The new UART is backward compatible with the standard 8250, but has some additional features. The new features include a high accuracy baud rate clock system and DMA support.

The driver will use the new optional BAUD MUX clock to select the best one of the four master clocks (81MHz, 108MHz, 64MHz and 48MHz) to feed the baud rate selection logic for any requested baud rate. This allows for more accurate BAUD rates when high speed baud rates are selected.

The driver will use the new UART DMA hardware if the UART DMA registers are specified in Device Tree "reg" property. The DMA functionality can be disabled on kernel boot with the argument:
"8250_bcm7271.disable_dma=Y".

The driver also set the UPSTAT_AUTOCTS flag when hardware flow control is enabled. This flag is needed for UARTs that don't assert a CTS changed interrupt when CTS changes and AFE (Hardware Flow Control) is enabled.

The driver also contains a workaround for a bug in the Synopsis 8250 core. The problem is that at high baud rates, the RX partial FIFO timeout interrupt can occur but there is no RX data (DR not set in the LSR register). In this case the driver will not read the Receive Buffer Register, which clears the interrupt, and the system will get continuous UART interrupts until the next RX character arrives. The fix originally suggested by Synopsis was to read the Receive Buffer Register and discard the character when the DR bit in the LSR was not set, to clear the interrupt. The problem was that occasionally a character would arrive just after the DR bit check and a valid character would be discarded. The fix that was added will clear

Quoted 8 lines

receive interrupts to stop the interrupt, deassert RTS to insure that no new data can arrive, wait for 1.5 character times for the sender to react to RTS and then check for data and either do a dummy read or a valid read. Sysfs error counters were also added and were used to help create test software that would cause the error condition. The counters can be found at:
/sys/devices/platform/rdb/*serial/rx_bad_timeout_late_char /sys/devices/platform/rdb/*serial/rx_bad_timeout_no_char

Quoted 2 lines

Brief looking into the code raises several questions:
- is it driver from the last decade?

Work on this driver started back in 2018, that was indeed the last decade.

Quoted 3 lines

- why it's not using what kernel provides?
- we have a lot of nice helpers:
- DMA Engine API

Quoted 8 lines

Not sure this makes sense, given that the DMA hardware that was added to this UART block is only used by the UART block and no other pieces of HW in the system, nor will they ever be. Not sure it makes sense to pay the cost of an extra indirection and subsystem unless there are at least two consumers of that DMA hardware to warrant modeling it after a dmaengine driver. I also remember that Al researched before whether 8250_dma.c could work, and came to the conclusion that it would not, but I will let him comment on the specifics.

I see. In any case I still believe that the driver can be shrinked by a notable amount of lines.

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Florian Fainelli

Quoted 1 line · Andy Shevchenko

On Fri, Jan 15, 2021 at 11:19 PM Al Cooper <[email protected]> wrote:

Quoted 9 lines

Quoted 8 lines

Not sure this makes sense, given that the DMA hardware that was added to this UART block is only used by the UART block and no other pieces of HW in the system, nor will they ever be. Not sure it makes sense to pay the cost of an extra indirection and subsystem unless there are at least two consumers of that DMA hardware to warrant modeling it after a dmaengine driver. I also remember that Al researched before whether 8250_dma.c could work, and came to the conclusion that it would not, but I will let him comment on the specifics.

Quoted 2 lines

I see. In any case I still believe that the driver can be shrinked by a notable amount of lines.

Patches always gladly accepted :)

Quoted 4 lines · Greg Kroah-Hartman

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Florian Fainelli

Quoted 1 line · Andy Shevchenko

On Fri, Jan 15, 2021 at 11:19 PM Al Cooper <[email protected]> wrote:

...

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

Not sure this makes sense, given that the DMA hardware that was added to this UART block is only used by the UART block and no other pieces of HW in the system, nor will they ever be. Not sure it makes sense to pay the cost of an extra indirection and subsystem unless there are at least two consumers of that DMA hardware to warrant modeling it after a dmaengine driver. I also remember that Al researched before whether 8250_dma.c could work, and came to the conclusion that it would not, but I will let him comment on the specifics.

Quoted 4 lines

Quoted 2 lines

I see. In any case I still believe that the driver can be shrinked by a notable amount of lines.

Patches always gladly accepted :)

Or a good review... :-)

Quoted 52 lines · Florian Fainelli

Quoted 42 lines · Andy Shevchenko

Quoted 32 lines · Al Cooper

Add a UART driver for the new Broadcom 8250 based STB UART. The new UART is backward compatible with the standard 8250, but has some additional features. The new features include a high accuracy baud rate clock system and DMA support.

The driver will use the new optional BAUD MUX clock to select the best one of the four master clocks (81MHz, 108MHz, 64MHz and 48MHz) to feed the baud rate selection logic for any requested baud rate. This allows for more accurate BAUD rates when high speed baud rates are selected.

The driver will use the new UART DMA hardware if the UART DMA registers are specified in Device Tree "reg" property. The DMA functionality can be disabled on kernel boot with the argument:
"8250_bcm7271.disable_dma=Y".

The driver also set the UPSTAT_AUTOCTS flag when hardware flow control is enabled. This flag is needed for UARTs that don't assert a CTS changed interrupt when CTS changes and AFE (Hardware Flow Control) is enabled.

The driver also contains a workaround for a bug in the Synopsis 8250 core. The problem is that at high baud rates, the RX partial FIFO timeout interrupt can occur but there is no RX data (DR not set in the LSR register). In this case the driver will not read the Receive Buffer Register, which clears the interrupt, and the system will get continuous UART interrupts until the next RX character arrives. The fix originally suggested by Synopsis was to read the Receive Buffer Register and discard the character when the DR bit in the LSR was not set, to clear the interrupt. The problem was that occasionally a character would arrive just after the DR bit check and a valid character would be discarded. The fix that was added will clear

Quoted 8 lines

receive interrupts to stop the interrupt, deassert RTS to insure that no new data can arrive, wait for 1.5 character times for the sender to react to RTS and then check for data and either do a dummy read or a valid read. Sysfs error counters were also added and were used to help create test software that would cause the error condition. The counters can be found at:
/sys/devices/platform/rdb/*serial/rx_bad_timeout_late_char /sys/devices/platform/rdb/*serial/rx_bad_timeout_no_char

Quoted 2 lines

Brief looking into the code raises several questions:
- is it driver from the last decade?

Work on this driver started back in 2018, that was indeed the last decade.

Quoted 3 lines

- why it's not using what kernel provides?
- we have a lot of nice helpers:
- DMA Engine API

Quoted 8 lines

Not sure this makes sense, given that the DMA hardware that was added to this UART block is only used by the UART block and no other pieces of HW in the system, nor will they ever be. Not sure it makes sense to pay the cost of an extra indirection and subsystem unless there are at least two consumers of that DMA hardware to warrant modeling it after a dmaengine driver. I also remember that Al researched before whether 8250_dma.c could work, and came to the conclusion that it would not, but I will let him comment on the specifics.

I see. In any case I still believe that the driver can be shrinked by a notable amount of lines.

Quoted 9 lines

Quoted 7 lines

- BIT() and GENMASK() macros
- tons of different helpers like regmap API (if you wish to dump
registers via debugfs)

Can you shrink this driver by 20-30% (I truly believe it's possible) and split DMA driver to drivers/dma (which may already have something similar there)?

See previous response.

--
With Best Regards,
Andy Shevchenko