Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use full UART TX buffer (shave 5s off booting) #18

Open
wants to merge 1 commit into
base: master
from

Conversation

@stewartsmith
Copy link

stewartsmith commented Jan 1, 2020

This patch makes us use the full TX buffer of the UART rather than
waiting for each character to go through. This practically means we
get a massive IPL speed up.

Measured on my Raptor Blackbird with P9N DD2.21, from "Welcome to SBE"
to "Booting Hostboot" (timed on a stopwatch):
Before (with UART output): 15 seconds
With this patch (with UART output): 10 seconds.

A full 33% reduction in boot time for SBE is nothing to sneeze at!

Signed-off-by: Stewart Smith stewart@flamingspork.com

This patch makes us use the full TX buffer of the UART rather than
waiting for each character to go through. This practically means we
get a *massive* IPL speed up.

Measured on my Raptor Blackbird with P9N DD2.21, from "Welcome to SBE"
to "Booting Hostboot" (timed on a stopwatch):
Before (with UART output): 15 seconds
With this patch (with UART output): 10 seconds.

A full 33% reduction in boot time for SBE is nothing to sneeze at!

Signed-off-by: Stewart Smith <stewart@flamingspork.com>

} while(0);
rc = writeReg(THR, c);

This comment has been minimized.

Copy link
@Shakeebbk

Shakeebbk Jan 1, 2020

Contributor

Not handling the case - when the buffer is full and if one of the errors - LSR read error, LSR_BAD or FIFO timeout.

This comment has been minimized.

Copy link
@stewartsmith

stewartsmith Jan 1, 2020

Author

The current code wasn't handling that either, it would just SBE_ERROR out and skip it.

We don't do any handling of that in skiboot, so in practice it's fine.

This comment has been minimized.

Copy link
@rajadas2

rajadas2 Jan 14, 2020

Contributor

Stewart, at least in the original patch set, we are bailing out the writeReg basis the readReg at line 177.

Any reason why you want to continue the writeReg even for readReg failure ?
Reason probably I see is that lower level call to lpc_rw will fail anyways while starting with the command register write to the LPC if there is an issue with readReg..

We probably can save little more cycles by not intentionally going into the writeReg if readReg fails.

Comments ?

This comment has been minimized.

Copy link
@rajadas2

rajadas2 Jan 16, 2020

Contributor

Stewart, Srikantha mentioned the point at line 197, where if you have data == LSR_BAD, then you will skip initialising tx_room to 16, and we will continue to do writeReg, if succeeds the success case will do tx_room -- and initialise the count to 256...the window will increase from 16 chars to 256 chars.

I have an opinion here to remove the readReg check altogether before every writeReg. Since inside the procedure code i.e lpc_rw, we have all the check happening. We say sucess/failure for a read/write operation only if LPC_STATUS_REG_DONE status is true/false. which is enough to identify if the current operation is success/failure.

readReg at the beginning is just adding some time buffer nothing else.

This comment has been minimized.

Copy link
@rajadas2

rajadas2 Jan 16, 2020

Contributor

I have pulled up this patchset into master/sbe branch, once we close on this review, I can go ahead and merge. thanks.

@shenki
shenki approved these changes Jan 14, 2020
break;
}
} else {
tx_room = 16;

This comment has been minimized.

Copy link
@srikantha-m

srikantha-m Jan 14, 2020

Stewart, Once we enter into "if (tx_room < 1)" and get into any error [ LSR read error, LSR_BAD or FIFO timeout ] then "tx_room = 16" never executes. And Line number 206: "tx_room--" leads to wrong value. Is this case cross check ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.