Page MenuHomeFreeBSD

Fix busy-detect in UART
ClosedPublic

Authored by bsz_semihalf.com on Nov 19 2015, 12:31 PM.
Tags
Referenced Files
Unknown Object (File)
Apr 12 2017, 5:03 AM
Unknown Object (File)
Apr 9 2017, 1:53 AM
Unknown Object (File)
Mar 31 2017, 10:15 AM
Unknown Object (File)
Mar 1 2017, 8:27 PM
Unknown Object (File)
Feb 12 2017, 12:06 PM
Unknown Object (File)
Feb 5 2017, 8:27 PM
Unknown Object (File)
Jan 13 2017, 12:27 AM
Unknown Object (File)
Dec 31 2016, 8:44 PM
Subscribers

Details

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 <bsz@semihalf.com>

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bsz_semihalf.com retitled this revision from to Enable UART busy-detect handling to prevent interrupt storm.
bsz_semihalf.com updated this object.
bsz_semihalf.com edited the test plan for this revision. (Show Details)
bsz_semihalf.com added reviewers: ian, imp.
bsz_semihalf.com set the repository for this revision to rS FreeBSD src repository - subversion.
bsz_semihalf.com added a project: ARM.
bsz_semihalf.com added a subscriber: zbb.
imp edited edge metadata.
This revision is now accepted and ready to land.Nov 19 2015, 2:54 PM
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?

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

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.

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.

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.

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).

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.

bsz_semihalf.com retitled this revision from Enable UART busy-detect handling to prevent interrupt storm to Fix busy-detect in UART.
bsz_semihalf.com updated this object.
bsz_semihalf.com added a reviewer: andrew.
This revision now requires review to proceed.Dec 3 2015, 2:14 PM
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 edited edge metadata.
bsz_semihalf.com marked an inline comment as done.
bsz_semihalf.com added inline comments.
sys/dev/uart/uart_dev_ns8250.c
431–433 ↗(On Diff #10965)

Corrected.

ian edited edge metadata.
This revision is now accepted and ready to land.Dec 9 2015, 8:08 PM
andrew edited edge metadata.

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 */
This revision was automatically updated to reflect the committed changes.