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
Open
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

Use full UART TX buffer

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>
  • Loading branch information
stewartsmith committed Dec 31, 2019
commit 8522639f495c02ee0a6eb0a34313d7d8971636dc
@@ -165,7 +165,9 @@ static void uartPutChar(char c)
{
#define SBE_FUNC "uartPutChar"
uint32_t rc = SBE_SEC_OPERATION_SUCCESSFUL;
do {
static unsigned char tx_room = 16;
if (tx_room < 1)
{
static const uint64_t DELAY_NS = 100;
static const uint64_t DELAY_LOOPS = 100000000;

@@ -188,27 +190,21 @@ static void uartPutChar(char c)
if(rc != SBE_SEC_OPERATION_SUCCESSFUL)
{
SBE_ERROR(SBE_FUNC " LSR read error.");
break;
}
if(data == LSR_BAD)
{
} else if(data == LSR_BAD) {
SBE_ERROR(SBE_FUNC " LSR_BAD data error.");
break;
}
if(loops >= DELAY_LOOPS)
{
} else if(loops >= DELAY_LOOPS) {
SBE_ERROR(SBE_FUNC " FIFO timeout.");
break;
}

rc = writeReg(THR, c);
if(rc != SBE_SEC_OPERATION_SUCCESSFUL)
{
SBE_ERROR(SBE_FUNC " failure to write THR");
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 ?

}
}

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

if(rc != SBE_SEC_OPERATION_SUCCESSFUL) {
SBE_ERROR(SBE_FUNC " failure to write THR");
} else {
tx_room--;
}

#undef SBE_FUNC
}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.