Add support of UART_DEV_DBGPORT to uart_bus_fdt.c
ClosedPublic
Actions

Authored by dsl on Nov 30 2020, 3:18 PM.

Details

Reviewers

markj
mhorne
andrew

Commits

rS368284: uart: allow UART_DEV_DBGPORT for fdt consoles

Summary

NOTE: This patch is a re-worked copy of an abandoned one attached to the review D5986.

Support of UART_DEV_DBGPORT in addition to UART_DEV_CONSOLE added to
uart_bus_fdt.c where "hw.fdt.debug" can override the FDT settings.
The following properties are looked at in /chosen for the debug node:
freebsd,debug-path.

Test Plan

I tested it on BeagleBone Black with kgdb connected, but I also have BeagleBone (white)
and RPi2 model B to check this patch if necessary.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

dsl created this revision.Nov 30 2020, 3:18 PM

Herald added subscribers: emaste, andrew. · View Herald TranscriptNov 30 2020, 3:18 PM

dsl requested review of this revision.Nov 30 2020, 3:18 PM

Hi! Thanks for updating this change, I was actually looking just last week at why this isn't supported.

sys/dev/uart/uart_bus_fdt.c
182 ↗	(On Diff #80151)	`propnames_dbg` seems preferable to me since this is a list.
193 ↗	(On Diff #80151)	Perhaps this was already discussed in the old review, but `hw.fdt.debug` seems vague to me. Maybe `hw.fdt.debug_console` or `hw.fdt.dbgport`?
196 ↗	(On Diff #80151)	Although it should not be an issue in practice, `UART_DEV_CONSOLE` should be an explicit case with `default` being a catch-all returning `ENXIO`.
sys/dev/uart/uart_cpu_arm64.c
104 ↗	(On Diff #80151)	You will need to allow `UART_DEV_DBGPORT` here as well or it will fail to attach.
sys/dev/uart/uart_cpu_fdt.c
91 ↗	(On Diff #80151)	I suspect you don't need the line break. FreeBSD's `style(9)` allows lines to be 80 characters in length.

I've tried to follow all of the comments during the review.

Please, take a look at the updated patch.

sys/dev/uart/uart_bus_fdt.c
182 ↗	(On Diff #80151)	I've re-named it as `propnames_dbgport` in order to align with `hw.fdt.dbgport`.

andrew added inline comments.Dec 1 2020, 4:42 PM

sys/dev/uart/uart_cpu_arm64.c
104 ↗	(On Diff #80151)	It should be removed with `uart_cpu_fdt_probe` checking for console types it supports.
sys/dev/uart/uart_cpu_fdt.c
90 ↗	(On Diff #80192)	You don't need the extra parentheses

Unnecessary tests of the devtype argument have been removed from uart_cpu_getdev() in sys/dev/uart/uart_cpu_arm64.c and sys/dev/uart/uart_cpu_fdt.c. uart_cpu_fdt_probe() checks for console types it supports.

dsl marked 2 inline comments as done.Dec 2 2020, 11:05 AM

mhorne accepted this revision.Dec 2 2020, 2:31 PM

This revision is now accepted and ready to land.Dec 2 2020, 2:31 PM

@mhorne Should I update a patch attached to the PR also: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=251053 ?

andrew accepted this revision.Dec 2 2020, 4:52 PM

In D27422#613267, @dsl_mcusim.org wrote:

@mhorne Should I update a patch attached to the PR also: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=251053 ?

Linking the review in the PR is enough. We should wait a little longer for any further comments, and if there are none I can commit the change.

Edit: I missed andrew's acceptance. I can commit it shortly.

@mhorne, @andrew, thanks for help here :)

Closed by commit rS368284: uart: allow UART_DEV_DBGPORT for fdt consoles (authored by mhorne). · Explain WhyDec 2 2020, 9:02 PM

This revision was automatically updated to reflect the committed changes.

mhorne added a commit: rS368284: uart: allow UART_DEV_DBGPORT for fdt consoles.

Herald added a subscriber: imp. · View Herald TranscriptDec 2 2020, 9:02 PM