plat/drivers: Add two configurations to ns16550 #985

zhxj9823 · 2023-07-23T14:51:46Z

Prerequisite checklist

Read the contribution guidelines regarding submitting new changes to the project;
Tested your changes against relevant architectures and platforms;
Ran the checkpatch.uk on your commit series before opening this PR;
Updated relevant documentation.

Base target

Architecture(s): ARM64
Platform(s): kvm
Application(s): N/A

Additional configuration

Description of changes

The register shift of ns16550 is hard-coded in the driver, but it does not match the hardware on some platforms. The device-tree specification has a property "reg-shift" to describe the register shift of a device.
To match the specification, the register shift of ns16550 is retrieved from the device tree instead. If no such property is found, the default value 0 is used.

The register width of ns16550 varies on different platforms. u-boot uses a reg-io-width property in the device tree to determine it. If no such property is found, the default value 1 is used. The read and write macros are replaced by functions with the same name.

These two configurations break the compatibility of the driver. To keep the compatibility, the device tree should contain these properties.

michpappas

Hi @zhxj9823, thanks for that! 🚀 Besides the comments below, since this change breaks backwards compatibility, it is important to mention this in the commit message. Moreover, please include in the commit message justification for that, ie a reference to the spec.

plat/drivers/uart/ns16550.c

razvand · 2023-07-24T19:11:42Z

Hi, @zhxj9823. This has been marked as a draft PR. What needs to happen to make this a full PR?

razvand

@zhxj9823 , could you also elaborate what is the register shift? As someone with little knowledge about UART / NS16550, I would benefit from this information as part of the commit message. Likely others as well.

michpappas · 2023-07-25T02:40:50Z

@zhxj9823 , could you also elaborate what is the register shift? As someone with little knowledge about UART / NS16550, I would benefit from this information as part of the commit message. Likely others as well.

Personally I am against trivialization (in general), it is expected that the audience has some familiarity with the topic. Look at the commit history of other OSes for example. Besides, where would you draw the line. And this is a simple concept, more complex topics would require way more elaborate explanations.

Saying that, if you really think it's necessary, one sentence is okay 😛

michpappas · 2023-07-25T06:50:45Z

@zhxj9823 thanks for the updates. As requested by @razvand please remove the Draft status from your PR.

One small thing that could be improved could be explicitly setting the default reg_shift:

+static uint32_t ns16550_reg_shift = 0; /* device-tree spec v0.4 Sect. 4.2.2 */

This is of course not required for functionality, but makes it more clear when reading the code that the default value is derived by the spec.

Other than that, from my side it looks good 👍🏼

@kubanrob I am adding as a reviewer since that change affects you.

kubanrob

@michpappas I investigated for our plattform and do not see problems with switching to 8bit register-width in this driver. On the other hand, having the register-width configurable is will also not hurt anybody, and the reduces the risk for anybody currently using this driver.

Anyways any new variable should also be properly initialized if early boot UART is activated.

kubanrob · 2023-07-25T21:34:44Z

plat/drivers/uart/ns16550.c

+static uint32_t ns16550_reg_shift = 0; /* device-tree spec v0.4 Sect. 4.2.2 */
+static uint32_t ns16550_reg_width = 1; /* 8-bit register width by default */


These variables are missing for the early uart (see the lines above). It would make sense to introduce the corresponding config options (default values matching the device tree ones).

Good point, thanks! 👍🏼 @zhxj9823 will you please update to add these?

I have added the config options. Please check if these options work fine.

It also raises the question of how to switch between pl011 and ns16550 as the early debug console.

I have added the config options. Please check if these options work fine.

I think these two options should not be limited to EARLY_UART, but should be overriding the defaults when set, unconditionally. That would allow the driver to work correctly in any future platforms that do not use dts.

The priority should therefore be:

dts (overrides everything)

Kconfig (if set override defaults)

Defaults (according to the spec)

It also raises the question of how to switch between pl011 and ns16550 as the early debug console.

For that we should merge the options of the different drivers into a single one, and most likely also consider an option in the command line, but this is outside the scope of this PR.

I think these two options should not be limited to EARLY_UART, but should be overriding the defaults when set, unconditionally. That would allow the driver to work correctly in any future platforms that do not use dts.

The priority should therefore be:

dts (overrides everything)

Kconfig (if set override defaults)

Defaults (according to the spec)

I update the patch. But I think it would be better if the two configs depend on something like uart_ns16550.

I had a brief look on how the reg-shift and width for the earlycon are specified on linux. There these are not compiled in, but rather derived from the command-line options:

https://www.kernel.org/doc/html/v4.14/admin-guide/kernel-parameters.html

https://elixir.bootlin.com/linux/latest/source/drivers/tty/serial/earlycon.c#L93

The latter derives the regshift from the mmio width, ie 0 by default, 1 for MMIO16, and 2 for MMIO32 access. Right now we use 16-bit access and a regshift of 2 which doesn't make sense. @kubanrob should that be 32-bit access instead? Also would the command line be a valid option for earlycon in your case, assuming we implemented that? I imagine that since it's used on linux, it should be no problem for Unikraft.

Edit: Actually nevermind, as right now libukparam only supports paramters in the the form of <libname>.<paramname>=, so parameters without a scope like earlycon= or console= won't work.

michpappas · 2023-08-07T14:54:44Z

@zhxj9823 please move these changes to the new location of ns16550 under drivers (rebase to #1026).

michpappas · 2023-08-14T14:18:08Z

@zhxj9823 please move these changes to the new location of ns16550 under drivers (rebase to #1026).

@zhxj9823 will you kindly rebase this on top of #1048 instead? It only needs a small change in Config.uk other than that it applies cleanly. Thanks a lot! 🚀

razvand

@zhxj9823, @michpappas, this is OK from my side. I've tested it together with #1048 and it works OK.

Reviewed-by: Razvan Deaconescu razvand@unikraft.io

kubanrob

Looks good to me, thanks you @zhxj9823!

Reviewed-by: Robert Kuban robert.kuban@opensynergy.com

michpappas · 2023-08-15T18:23:51Z

@zhxj9823 now that the parent PR has been merged, please rebase to staging to remove the dependency commit from this PR, so that we can merge it too. Thanks 🚀

The register shift of ns16550 is hard-coded in the driver, but it does not match the hardware on some platforms. The device-tree specification has a property "reg-shift" to describe the register shift of a device. To match the specification, the register shift of ns16550 is retrieved from the device tree instead. If no such property is found, the default value 0 is used. The register width of ns16550 varies on different platforms. u-boot uses a reg-io-width property in the device tree to determine it. If no such property is found, the default value 1 is used. The read and write macros are replaced by functions with the same name. This patch also adds the corresponding configurations to override the values when dts does not specify them. These two configurations break the compatibility of the driver. To keep the compatibility, the device tree should contain these properties or set them in configurations. Signed-off-by: Xingjian Zhang <zhxj9823@qq.com>

unikraft-bot · 2023-08-16T06:09:53Z

✅ Checkpatch passed

Beep boop! I ran Unikraft's checkpatch.pl support script on your pull request and it all looks good!

SHA	commit	checkpatch
`cee85ff`	plat/drivers: Add two configurations to ns16550	✅

michpappas

Thank you @zhxj9823 🚀

Reviewed-by: Michalis Pappas michalis@unikraft.io
Approved-by: Michalis Pappas michalis@unikraft.io

michpappas requested changes Jul 24, 2023

View reviewed changes

plat/drivers/uart/ns16550.c Outdated Show resolved Hide resolved

plat/drivers/uart/ns16550.c Outdated Show resolved Hide resolved

michpappas mentioned this pull request Jul 24, 2023

plat/kvm/arm64: Firecracker support on arm64 #989

Closed

4 tasks

razvand assigned michpappas Jul 24, 2023

razvand requested review from razvanvirtan and mariasfiraiala July 24, 2023 06:15

razvand added area/plat Unikraft Patform plat/kvm Unikraft for KVM arch/arm64 plat/driver labels Jul 24, 2023

razvand added this to the v0.14.0 (Prometheus) milestone Jul 24, 2023

razvand reviewed Jul 24, 2023

View reviewed changes

zhxj9823 force-pushed the ns16550_shift branch 2 times, most recently from e038647 to e2d8a65 Compare July 25, 2023 06:41

michpappas requested a review from kubanrob July 25, 2023 06:51

zhxj9823 force-pushed the ns16550_shift branch from e2d8a65 to 2d06d95 Compare July 25, 2023 07:04

zhxj9823 marked this pull request as ready for review July 25, 2023 07:04

zhxj9823 requested a review from a team as a code owner July 25, 2023 07:04

unikraft-bot added the lang/c Issues or PRs to do with C/C++ label Jul 25, 2023

zhxj9823 changed the title ~~plat/drivers: Add reg-shift option to ns16550~~ plat/drivers: Add two configurations to ns16550 Jul 25, 2023

kubanrob reviewed Jul 26, 2023

View reviewed changes

zhxj9823 force-pushed the ns16550_shift branch from 2d06d95 to 5f79c81 Compare July 27, 2023 06:43

zhxj9823 requested a review from a team as a code owner July 27, 2023 06:43

zhxj9823 force-pushed the ns16550_shift branch 2 times, most recently from e7b227d to c668d9c Compare July 28, 2023 15:29

razvand modified the milestones: v0.14.0 (Prometheus), v0.15.0 (Pandora) Aug 9, 2023

michpappas modified the milestones: v0.15.0 (Pandora), v0.14.0 (Prometheus) Aug 14, 2023

zhxj9823 force-pushed the ns16550_shift branch from c668d9c to 226c0cc Compare August 14, 2023 15:30

razvand requested review from razvand, kubanrob and michpappas and removed request for a team, razvanvirtan and mariasfiraiala August 15, 2023 14:10

razvand approved these changes Aug 15, 2023

View reviewed changes

kubanrob approved these changes Aug 15, 2023

View reviewed changes

zhxj9823 force-pushed the ns16550_shift branch from 226c0cc to cee85ff Compare August 16, 2023 05:43

michpappas approved these changes Aug 16, 2023

View reviewed changes

unikraft-bot closed this in df423aa Aug 16, 2023

unikraft-bot added the ci/merged Merged by CI label Aug 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

plat/drivers: Add two configurations to ns16550 #985

plat/drivers: Add two configurations to ns16550 #985

zhxj9823 commented Jul 23, 2023 •

edited

Loading

michpappas left a comment

razvand commented Jul 24, 2023

razvand left a comment

michpappas commented Jul 25, 2023

michpappas commented Jul 25, 2023

kubanrob left a comment •

edited

Loading

kubanrob Jul 25, 2023

michpappas Jul 27, 2023

zhxj9823 Jul 27, 2023

michpappas Jul 28, 2023

zhxj9823 Jul 28, 2023

michpappas Aug 1, 2023 •

edited

Loading

michpappas commented Aug 7, 2023

michpappas commented Aug 14, 2023

razvand left a comment

kubanrob left a comment

michpappas commented Aug 15, 2023

unikraft-bot commented Aug 16, 2023

michpappas left a comment

		static uint32_t ns16550_reg_shift = 0; /* device-tree spec v0.4 Sect. 4.2.2 */
		static uint32_t ns16550_reg_width = 1; /* 8-bit register width by default */

plat/drivers: Add two configurations to ns16550 #985

plat/drivers: Add two configurations to ns16550 #985

Conversation

zhxj9823 commented Jul 23, 2023 • edited Loading

Prerequisite checklist

Base target

Additional configuration

Description of changes

michpappas left a comment

Choose a reason for hiding this comment

razvand commented Jul 24, 2023

razvand left a comment

Choose a reason for hiding this comment

michpappas commented Jul 25, 2023

michpappas commented Jul 25, 2023

kubanrob left a comment • edited Loading

Choose a reason for hiding this comment

kubanrob Jul 25, 2023

Choose a reason for hiding this comment

michpappas Jul 27, 2023

Choose a reason for hiding this comment

zhxj9823 Jul 27, 2023

Choose a reason for hiding this comment

michpappas Jul 28, 2023

Choose a reason for hiding this comment

zhxj9823 Jul 28, 2023

Choose a reason for hiding this comment

michpappas Aug 1, 2023 • edited Loading

Choose a reason for hiding this comment

michpappas commented Aug 7, 2023

michpappas commented Aug 14, 2023

razvand left a comment

Choose a reason for hiding this comment

kubanrob left a comment

Choose a reason for hiding this comment

michpappas commented Aug 15, 2023

unikraft-bot commented Aug 16, 2023

michpappas left a comment

Choose a reason for hiding this comment

zhxj9823 commented Jul 23, 2023 •

edited

Loading

kubanrob left a comment •

edited

Loading

michpappas Aug 1, 2023 •

edited

Loading