plat: Migrate pl011 to drivers/uktty #1025

michpappas · 2023-08-07T14:41:48Z

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): [N/A]
Platform(s): [N/A]
Application(s): [N/A]

Additional configuration

Description of changes

Migrate the PL011 driver to drivers/uktty

Signed-off-by: Michalis Pappas <michalis@unikraft.io>

razvanvirtan

Looks good to me, just a few typos in the commit messages:

f2ddbdb console
86e7140 chackpatch

Approved-by: Razvan Virtan virtanrazvan@gmail.com

Add skeleton for the `uktty` subsystem. This subsystem contains drivers for console devices such as UART and VGA. Signed-off-by: Michalis Pappas <michalis@unikraft.io>

michpappas · 2023-12-21T15:46:07Z

@razvanvirtan thanks a lot for reviewing. Both typos are fixed.

mogasergiu

Hi @michpappas! Thanks for not letting this PR rot 🙏 ! Had a quick look over the changes. I will have another run tomorrow on this and the others from your side I am assigned as reviewer to.

mogasergiu · 2023-12-21T16:12:01Z

drivers/uktty/pl011/pl011.c

@@ -128,7 +128,7 @@ void pl011_console_init(const void *dtb)
 		UK_CRASH("Could not find proper size cells!\n");

 	regs = fdt_getprop(dtb, offset, "reg", &len);
-	if (regs == NULL || (len < (int)sizeof(fdt32_t) * (naddr + nsize)))
+	if (!regs || (len < (int)sizeof(fdt32_t) * (naddr + nsize)))


Let's use unlikely on all UK_CRASH cases?

Good point, I'll do that. Thanks 👍🏼

mogasergiu · 2023-12-21T16:24:04Z

drivers/uktty/pl011/pl011.c

@@ -90,7 +90,7 @@ static void init_pl011(__u64 bas)

 	/* Mask all interrupts */
 	PL011_REG_WRITE(REG_UARTIMSC_OFFSET,
-		PL011_REG_READ(REG_UARTIMSC_OFFSET) & 0xf800);
+			PL011_REG_READ(REG_UARTIMSC_OFFSET) & 0xf800);


Could we maybe have drivers/uktty/pl011: Whitespace fixes for checkpatch squashed into the migration commit? 🤔

It's also one possible way of many. I considered both that option as well as the possibility to do all checkpatch changes in one commit (or even in the same one as the migration), but in the end I concluded it would be cleaner to do this in discrete steps. If you have a strong opinion / reasoning about it I don't mind to change it though.

If you have a strong opinion / reasoning about it I don't mind to change it though.

Nah, my only concern is usually when it comes to bisecting and looking at git history to not be unnecessarily contaminated.

I concluded it would be cleaner to do this in discrete steps

Ok I see. Sure, it's just one commit after all. Let's keep it like this if you say you considered it already and believe it to be cleaner: migration and checkpatch fixes. 👍

mogasergiu

Just a tiny comment. Other than that, LGTM!

Reviewed-by: Sergiu Moga sergiu.moga@unikraft.io

mogasergiu · 2023-12-23T13:58:07Z

drivers/uktty/pl011/pl011.c

 static uint8_t pl011_uart_initialized = 1;
-static uint64_t pl011_uart_bas = CONFIG_EARLY_PRINT_PL011_UART_ADDR;
+static uint64_t pl011_uart_bas = CONFIG_LIBUKTTY_PL011_EARLY_CONSOLE_BASE;
 #else


An /* !CONFIG_LIBUKTTY_PL011_EARLY_CONSOLE_BASE */ here?

Migrate the pl011 driver to the newly introduced uktty subsystem. Signed-off-by: Michalis Pappas <michalis@unikraft.io>

The `pl011` does not depend on the standand library. Replace stdint types with kernel types. Signed-off-by: Michalis Pappas <michalis@unikraft.io>

Replace bit shifts with UK_BIT in macros to comply with checkpath. Signed-off-by: Michalis Pappas <michalis@unikraft.io>

Various whitespace fixes to comply with checkpatch. Signed-off-by: Michalis Pappas <michalis@unikraft.io>

Use shorhand syntax for checks against NULL to comply with checkpatch. Signed-off-by: Michalis Pappas <michalis@unikraft.io>

Fixes an issue when early console is not enabled where no output would be sent to the console due to the driver ignoring calls when the initialized flag is not set. Signed-off-by: Michalis Pappas <michalis@unikraft.io>

Use the `unlikely` macro in error conditions to hint the compiler to optimize the for the probable path. Signed-off-by: Michalis Pappas <michalis@unikraft.io>

razvand

Approved-by: Razvan Deaconescu razvand@unikraft.io

Add skeleton for the `uktty` subsystem. This subsystem contains drivers for console devices such as UART and VGA. Signed-off-by: Michalis Pappas <michalis@unikraft.io> Reviewed-by: Sergiu Moga <sergiu.moga@unikraft.io> Approved-by: Razvan Deaconescu <razvand@unikraft.io> GitHub-Closes: #1025

Migrate the pl011 driver to the newly introduced uktty subsystem. Signed-off-by: Michalis Pappas <michalis@unikraft.io> Reviewed-by: Sergiu Moga <sergiu.moga@unikraft.io> Approved-by: Razvan Deaconescu <razvand@unikraft.io> GitHub-Closes: #1025

The `pl011` does not depend on the standand library. Replace stdint types with kernel types. Signed-off-by: Michalis Pappas <michalis@unikraft.io> Reviewed-by: Sergiu Moga <sergiu.moga@unikraft.io> Approved-by: Razvan Deaconescu <razvand@unikraft.io> GitHub-Closes: #1025

Replace bit shifts with UK_BIT in macros to comply with checkpath. Signed-off-by: Michalis Pappas <michalis@unikraft.io> Reviewed-by: Sergiu Moga <sergiu.moga@unikraft.io> Approved-by: Razvan Deaconescu <razvand@unikraft.io> GitHub-Closes: #1025

Various whitespace fixes to comply with checkpatch. Signed-off-by: Michalis Pappas <michalis@unikraft.io> Reviewed-by: Sergiu Moga <sergiu.moga@unikraft.io> Approved-by: Razvan Deaconescu <razvand@unikraft.io> GitHub-Closes: #1025

Use shorhand syntax for checks against NULL to comply with checkpatch. Signed-off-by: Michalis Pappas <michalis@unikraft.io> Reviewed-by: Sergiu Moga <sergiu.moga@unikraft.io> Approved-by: Razvan Deaconescu <razvand@unikraft.io> GitHub-Closes: #1025

Fixes an issue when early console is not enabled where no output would be sent to the console due to the driver ignoring calls when the initialized flag is not set. Signed-off-by: Michalis Pappas <michalis@unikraft.io> Reviewed-by: Sergiu Moga <sergiu.moga@unikraft.io> Approved-by: Razvan Deaconescu <razvand@unikraft.io> GitHub-Closes: #1025

Use the `unlikely` macro in error conditions to hint the compiler to optimize the for the probable path. Signed-off-by: Michalis Pappas <michalis@unikraft.io> Reviewed-by: Sergiu Moga <sergiu.moga@unikraft.io> Approved-by: Razvan Deaconescu <razvand@unikraft.io> GitHub-Closes: #1025

michpappas requested review from a team as code owners August 7, 2023 14:41

This comment was marked as outdated.

Sign in to view

This was referenced Aug 7, 2023

plat: Migrate ns16550 to drivers/uktty #1026

Merged

plat/drivers: Provide a single API for UART drivers #986

Draft

michpappas added a commit to michpappas/unikraft that referenced this pull request Dec 17, 2023

plat/drivers: PR unikraft#1025 Migrate pl011 into drivers

42bbef1

Signed-off-by: Michalis Pappas <michalis@unikraft.io>

michpappas force-pushed the migrate_pl011_to_drivers branch 2 times, most recently from 1a5bc52 to 37118c8 Compare December 17, 2023 18:54

razvand modified the milestones: v0.17.0 (Calypso), v0.16.0 (Telesto) Dec 20, 2023

razvanvirtan approved these changes Dec 21, 2023

View reviewed changes

drivers: Introduce uktty for console devices

4cfff12

Add skeleton for the `uktty` subsystem. This subsystem contains drivers for console devices such as UART and VGA. Signed-off-by: Michalis Pappas <michalis@unikraft.io>

michpappas force-pushed the migrate_pl011_to_drivers branch from 37118c8 to 846fd85 Compare December 21, 2023 15:45

mogasergiu reviewed Dec 21, 2023

View reviewed changes

mogasergiu approved these changes Dec 23, 2023

View reviewed changes

michpappas added 7 commits December 23, 2023 15:16

plat/drivers: Migrate pl011 to drivers/uktty

9e812e7

Migrate the pl011 driver to the newly introduced uktty subsystem. Signed-off-by: Michalis Pappas <michalis@unikraft.io>

drivers/uktty/pl011: Replace stdint types with kernel types

7fbd0c7

The `pl011` does not depend on the standand library. Replace stdint types with kernel types. Signed-off-by: Michalis Pappas <michalis@unikraft.io>

drivers/uktty/pl011: Use UK_BIT in bit shift macros

2ca8888

Replace bit shifts with UK_BIT in macros to comply with checkpath. Signed-off-by: Michalis Pappas <michalis@unikraft.io>

drivers/uktty/pl011: Whitespace fixes for checkpatch

2258a66

Various whitespace fixes to comply with checkpatch. Signed-off-by: Michalis Pappas <michalis@unikraft.io>

drivers/uktty/pl011: Use shorthand syntax on NULL check

bf92600

Use shorhand syntax for checks against NULL to comply with checkpatch. Signed-off-by: Michalis Pappas <michalis@unikraft.io>

drivers/uktty/pl011: Use unlikely in error conditions

f29b5b8

Use the `unlikely` macro in error conditions to hint the compiler to optimize the for the probable path. Signed-off-by: Michalis Pappas <michalis@unikraft.io>

michpappas force-pushed the migrate_pl011_to_drivers branch from 8da0f1b to f29b5b8 Compare December 23, 2023 14:17

razvand approved these changes Dec 23, 2023

View reviewed changes

razvand changed the base branch from staging to staging-1025 December 23, 2023 14:26

razvand merged commit 1c7f7f0 into unikraft:staging-1025 Dec 23, 2023
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

plat: Migrate pl011 to drivers/uktty #1025

plat: Migrate pl011 to drivers/uktty #1025

michpappas commented Aug 7, 2023 •

edited

This comment was marked as outdated.

razvanvirtan left a comment

michpappas commented Dec 21, 2023

mogasergiu left a comment

mogasergiu Dec 21, 2023

michpappas Dec 23, 2023

mogasergiu Dec 21, 2023

michpappas Dec 23, 2023

mogasergiu Dec 23, 2023

mogasergiu left a comment

mogasergiu Dec 23, 2023

razvand left a comment

plat: Migrate pl011 to drivers/uktty #1025

plat: Migrate pl011 to drivers/uktty #1025

Conversation

michpappas commented Aug 7, 2023 • edited

Prerequisite checklist

Base target

Additional configuration

Description of changes

This comment was marked as outdated.

razvanvirtan left a comment

Choose a reason for hiding this comment

michpappas commented Dec 21, 2023

mogasergiu left a comment

Choose a reason for hiding this comment

mogasergiu Dec 21, 2023

Choose a reason for hiding this comment

michpappas Dec 23, 2023

Choose a reason for hiding this comment

mogasergiu Dec 21, 2023

Choose a reason for hiding this comment

michpappas Dec 23, 2023

Choose a reason for hiding this comment

mogasergiu Dec 23, 2023

Choose a reason for hiding this comment

mogasergiu left a comment

Choose a reason for hiding this comment

mogasergiu Dec 23, 2023

Choose a reason for hiding this comment

razvand left a comment

Choose a reason for hiding this comment

michpappas commented Aug 7, 2023 •

edited