Fix busy-detect in UART

[bsz_semihalf.com]

Summary

uart_dev_ns8250 now relies on compatible property instead of additional
'busy-detect' cell. All drivers with compatible = "snps,dw-apb-uart" have
busy detection turned on. DTS files of devices affected by the change
were modified and 'busy-detect' property was removed.

Obtained from: Semihalf
Sponsored by: Stormshield
Submitted by: Bartosz Szczepanek <[e-mail]>

[andrew]

sys/boot/fdt/dts/arm/armada-38x.dtsi

226 ↗

(On Diff #10347)

I don't see this in the devicetree binding documents, is there not a standard value we could use?

[bsz_semihalf.com]

sys/boot/fdt/dts/arm/armada-38x.dtsi

226 ↗

(On Diff #10347)

busy-detect property was added in this commit, perhaps it is not documented. However, it is used in several .dts files in this form: https://github.com/freebsd/freebsd/search?utf8=✓&q=busy-detect

[andrew]

sys/boot/fdt/dts/arm/armada-38x.dtsi

226 ↗

(On Diff #10347)

Linux enables whenever compatible = "snps,dw-apb-uart" is set. This would be a better solution than adding a non-standard property as this will help us use the same dtb as Linux.

[bsz_semihalf.com]

sys/boot/fdt/dts/arm/armada-38x.dtsi

226 ↗

(On Diff #10347)

In Linux there is no busy-detect property at all. And as this was introduced in FreeBSD, I wouldn't want to break the convention. If we really want to change it, I think that should be seperate patch that would remove busy-detect everywhere.
By then we would have to turn on busy-detect everywhere (on some platforms with ns16550 busy-detect is on, on some it is off), what could be potentially harmful.

[andrew]

sys/boot/fdt/dts/arm/armada-38x.dtsi

226 ↗

(On Diff #10347)

It's needed on all DeviceWare implementations of the ns16550, we can use the compatible property to detect this, we should use this rather than using our own custom property.

If I had reviewed the original change I would have said something similar.

[zbb]

sys/boot/fdt/dts/arm/armada-38x.dtsi

226 ↗

(On Diff #10347)

andrew is right about the method of how this should be eventually done.
However IMHO we should not change the busy detection code in the driver with THIS commit. This change is strictly linked to Armada 38X platform and relies on how current FreeBSD code deals with busy detection.

There are other platforms that need busy detection and their compatible property is not "dw-apb-uart" or other but simply "ns16550" (no difference with the platforms that don't need busy detect).

So I would like to see a separate commit that changes the way that busy detect is being found in ns8250 driver and proper changes in DTS files of related platforms (removal of "busy-detect" property and change in "compatible" if needed).

[andrew]

sys/boot/fdt/dts/arm/armada-38x.dtsi

226 ↗

(On Diff #10347)

If you fixed the code then this change becomes redundant, it would just be a few lines to check on the property.

[andrew]

sys/dev/uart/uart_dev_ns8250.c

431–433 ↗

(On Diff #10699)

Can you keep this for the case of a new kernel with an old device tree. It helps to keep compatibility around where possible to give people a chance to get both in sync.

[bsz_semihalf.com]

sys/dev/uart/uart_dev_ns8250.c

431–433 ↗

(On Diff #10965)

Corrected.

[andrew]

No need to resubmit just to change the comment.

sys/dev/uart/uart_dev_ns8250.c

432 ↗

(On Diff #10965)

When committed this should read something like the following.

/* XXX: This is kept for a short time for compatibility with older device trees */