| Message ID | 1454708607-1155-1-git-send-email-eric@anholt.net |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Glass |
| Headers | show |
Hi Eric, On 5 February 2016 at 14:43, Eric Anholt <eric@anholt.net> wrote: > For Raspberry Pi, we had the input clock rate to the pl011 fixed in > the rpi.c file, but it may be changed by firmware due to user changes > to config.txt. Since the firmware always sets up the uart (default > 115200 output unless the user changes it), we can just skip our own > uart init to simplify the boot process and more reliably get serial > output. > > Signed-off-by: Eric Anholt <eric@anholt.net> > --- > board/raspberrypi/rpi/rpi.c | 3 +-- > drivers/serial/serial_pl01x.c | 4 ++++ > include/dm/platform_data/serial_pl01x.h | 1 + > 3 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/board/raspberrypi/rpi/rpi.c b/board/raspberrypi/rpi/rpi.c > index 4b80d7b..1fd3f97 100644 > --- a/board/raspberrypi/rpi/rpi.c > +++ b/board/raspberrypi/rpi/rpi.c > @@ -35,8 +35,7 @@ static const struct pl01x_serial_platdata serial_platdata = { > #else > .base = 0x20201000, > #endif > - .type = TYPE_PL011, > - .clock = 3000000, > + .type = TYPE_PL01X_PRECONFIGURED, > }; > > U_BOOT_DEVICE(bcm2835_serials) = { > diff --git a/drivers/serial/serial_pl01x.c b/drivers/serial/serial_pl01x.c > index 552c945..7d4d214 100644 > --- a/drivers/serial/serial_pl01x.c > +++ b/drivers/serial/serial_pl01x.c > @@ -83,6 +83,8 @@ static int pl01x_generic_serial_init(struct pl01x_regs *regs, > /* disable everything */ > writel(0, ®s->pl011_cr); > break; > + case TYPE_PL01X_PRECONFIGURED: > + break; > default: > return -EINVAL; > } > @@ -172,6 +174,8 @@ static int pl01x_generic_setbrg(struct pl01x_regs *regs, enum pl01x_type type, > writel(UART_PL011_CR_UARTEN | UART_PL011_CR_TXE | > UART_PL011_CR_RXE | UART_PL011_CR_RTS, ®s->pl011_cr); > break; > + case TYPE_PL01X_PRECONFIGURED: > + break; > } > default: > return -EINVAL; > diff --git a/include/dm/platform_data/serial_pl01x.h b/include/dm/platform_data/serial_pl01x.h > index 5e068f3..ae1bf31 100644 > --- a/include/dm/platform_data/serial_pl01x.h > +++ b/include/dm/platform_data/serial_pl01x.h > @@ -9,6 +9,7 @@ > enum pl01x_type { > TYPE_PL010, > TYPE_PL011, > + TYPE_PL01X_PRECONFIGURED, This seems odd. You are not adding a new UART type. How about a separate bool flag? Regards, Simon
On Sat, Feb 6, 2016 at 12:44 PM, Simon Glass <sjg@chromium.org> wrote: > Hi Eric, > > On 5 February 2016 at 14:43, Eric Anholt <eric@anholt.net> wrote: >> For Raspberry Pi, we had the input clock rate to the pl011 fixed in >> the rpi.c file, but it may be changed by firmware due to user changes >> to config.txt. Since the firmware always sets up the uart (default >> 115200 output unless the user changes it), we can just skip our own >> uart init to simplify the boot process and more reliably get serial >> output. >> >> Signed-off-by: Eric Anholt <eric@anholt.net> >> --- >> board/raspberrypi/rpi/rpi.c | 3 +-- >> drivers/serial/serial_pl01x.c | 4 ++++ >> include/dm/platform_data/serial_pl01x.h | 1 + >> 3 files changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/board/raspberrypi/rpi/rpi.c b/board/raspberrypi/rpi/rpi.c >> index 4b80d7b..1fd3f97 100644 >> --- a/board/raspberrypi/rpi/rpi.c >> +++ b/board/raspberrypi/rpi/rpi.c >> @@ -35,8 +35,7 @@ static const struct pl01x_serial_platdata serial_platdata = { >> #else >> .base = 0x20201000, >> #endif >> - .type = TYPE_PL011, >> - .clock = 3000000, >> + .type = TYPE_PL01X_PRECONFIGURED, >> }; >> >> U_BOOT_DEVICE(bcm2835_serials) = { >> diff --git a/drivers/serial/serial_pl01x.c b/drivers/serial/serial_pl01x.c >> index 552c945..7d4d214 100644 >> --- a/drivers/serial/serial_pl01x.c >> +++ b/drivers/serial/serial_pl01x.c >> @@ -83,6 +83,8 @@ static int pl01x_generic_serial_init(struct pl01x_regs *regs, >> /* disable everything */ >> writel(0, ®s->pl011_cr); >> break; >> + case TYPE_PL01X_PRECONFIGURED: >> + break; >> default: >> return -EINVAL; >> } >> @@ -172,6 +174,8 @@ static int pl01x_generic_setbrg(struct pl01x_regs *regs, enum pl01x_type type, >> writel(UART_PL011_CR_UARTEN | UART_PL011_CR_TXE | >> UART_PL011_CR_RXE | UART_PL011_CR_RTS, ®s->pl011_cr); >> break; >> + case TYPE_PL01X_PRECONFIGURED: >> + break; >> } >> default: >> return -EINVAL; >> diff --git a/include/dm/platform_data/serial_pl01x.h b/include/dm/platform_data/serial_pl01x.h >> index 5e068f3..ae1bf31 100644 >> --- a/include/dm/platform_data/serial_pl01x.h >> +++ b/include/dm/platform_data/serial_pl01x.h >> @@ -9,6 +9,7 @@ >> enum pl01x_type { >> TYPE_PL010, >> TYPE_PL011, >> + TYPE_PL01X_PRECONFIGURED, > > This seems odd. You are not adding a new UART type. > > How about a separate bool flag? I posted a patch with the same purpose a while ago http://lists.denx.de/pipermail/u-boot/2015-October/230079.html Regards, Sergey > > Regards, > Simon > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot
On Mon, Feb 08, 2016 at 11:26:04AM -0800, Sergei Temerkhanov wrote: > On Sat, Feb 6, 2016 at 12:44 PM, Simon Glass <sjg@chromium.org> wrote: > > Hi Eric, > > > > On 5 February 2016 at 14:43, Eric Anholt <eric@anholt.net> wrote: > >> For Raspberry Pi, we had the input clock rate to the pl011 fixed in > >> the rpi.c file, but it may be changed by firmware due to user changes > >> to config.txt. Since the firmware always sets up the uart (default > >> 115200 output unless the user changes it), we can just skip our own > >> uart init to simplify the boot process and more reliably get serial > >> output. > >> > >> Signed-off-by: Eric Anholt <eric@anholt.net> > >> --- > >> board/raspberrypi/rpi/rpi.c | 3 +-- > >> drivers/serial/serial_pl01x.c | 4 ++++ > >> include/dm/platform_data/serial_pl01x.h | 1 + > >> 3 files changed, 6 insertions(+), 2 deletions(-) > >> > >> diff --git a/board/raspberrypi/rpi/rpi.c b/board/raspberrypi/rpi/rpi.c > >> index 4b80d7b..1fd3f97 100644 > >> --- a/board/raspberrypi/rpi/rpi.c > >> +++ b/board/raspberrypi/rpi/rpi.c > >> @@ -35,8 +35,7 @@ static const struct pl01x_serial_platdata serial_platdata = { > >> #else > >> .base = 0x20201000, > >> #endif > >> - .type = TYPE_PL011, > >> - .clock = 3000000, > >> + .type = TYPE_PL01X_PRECONFIGURED, > >> }; > >> > >> U_BOOT_DEVICE(bcm2835_serials) = { > >> diff --git a/drivers/serial/serial_pl01x.c b/drivers/serial/serial_pl01x.c > >> index 552c945..7d4d214 100644 > >> --- a/drivers/serial/serial_pl01x.c > >> +++ b/drivers/serial/serial_pl01x.c > >> @@ -83,6 +83,8 @@ static int pl01x_generic_serial_init(struct pl01x_regs *regs, > >> /* disable everything */ > >> writel(0, ®s->pl011_cr); > >> break; > >> + case TYPE_PL01X_PRECONFIGURED: > >> + break; > >> default: > >> return -EINVAL; > >> } > >> @@ -172,6 +174,8 @@ static int pl01x_generic_setbrg(struct pl01x_regs *regs, enum pl01x_type type, > >> writel(UART_PL011_CR_UARTEN | UART_PL011_CR_TXE | > >> UART_PL011_CR_RXE | UART_PL011_CR_RTS, ®s->pl011_cr); > >> break; > >> + case TYPE_PL01X_PRECONFIGURED: > >> + break; > >> } > >> default: > >> return -EINVAL; > >> diff --git a/include/dm/platform_data/serial_pl01x.h b/include/dm/platform_data/serial_pl01x.h > >> index 5e068f3..ae1bf31 100644 > >> --- a/include/dm/platform_data/serial_pl01x.h > >> +++ b/include/dm/platform_data/serial_pl01x.h > >> @@ -9,6 +9,7 @@ > >> enum pl01x_type { > >> TYPE_PL010, > >> TYPE_PL011, > >> + TYPE_PL01X_PRECONFIGURED, > > > > This seems odd. You are not adding a new UART type. > > > > How about a separate bool flag? > > I posted a patch with the same purpose a while ago > http://lists.denx.de/pipermail/u-boot/2015-October/230079.html And then we got bogged down in "make a DT binding that's accepted" hell.
On 02/05/2016 09:19 PM, Stephen Warren wrote: > On 02/05/2016 02:43 PM, Eric Anholt wrote: >> For Raspberry Pi, we had the input clock rate to the pl011 fixed in >> the rpi.c file, but it may be changed by firmware due to user changes >> to config.txt. Since the firmware always sets up the uart (default >> 115200 output unless the user changes it), we can just skip our own >> uart init to simplify the boot process and more reliably get serial >> output. > > Tested-by: Stephen Warren <swarren@wwwdotorg.org> > (With a 6MHz init_uart_clock and with/without init_uart_baud=9600) Simon/Tom, did this slip through the cracks, or are you deferring it until the next release?
Hi Stephen, On 5 March 2016 at 23:26, Stephen Warren <swarren@wwwdotorg.org> wrote: > > On 02/05/2016 09:19 PM, Stephen Warren wrote: >> >> On 02/05/2016 02:43 PM, Eric Anholt wrote: >>> >>> For Raspberry Pi, we had the input clock rate to the pl011 fixed in >>> the rpi.c file, but it may be changed by firmware due to user changes >>> to config.txt. Since the firmware always sets up the uart (default >>> 115200 output unless the user changes it), we can just skip our own >>> uart init to simplify the boot process and more reliably get serial >>> output. >> >> >> Tested-by: Stephen Warren <swarren@wwwdotorg.org> >> (With a 6MHz init_uart_clock and with/without init_uart_baud=9600) > > > Simon/Tom, did this slip through the cracks, or are you deferring it until the next release? We haven't heard back on the review comments. I don't really mind if we go the DT approach or this one. But this patch needs a little rework I think. Regards, Simon
On Sun, Mar 06, 2016 at 07:38:47PM -0700, Simon Glass wrote: > Hi Stephen, > > On 5 March 2016 at 23:26, Stephen Warren <swarren@wwwdotorg.org> wrote: > > > > On 02/05/2016 09:19 PM, Stephen Warren wrote: > >> > >> On 02/05/2016 02:43 PM, Eric Anholt wrote: > >>> > >>> For Raspberry Pi, we had the input clock rate to the pl011 fixed in > >>> the rpi.c file, but it may be changed by firmware due to user changes > >>> to config.txt. Since the firmware always sets up the uart (default > >>> 115200 output unless the user changes it), we can just skip our own > >>> uart init to simplify the boot process and more reliably get serial > >>> output. > >> > >> > >> Tested-by: Stephen Warren <swarren@wwwdotorg.org> > >> (With a 6MHz init_uart_clock and with/without init_uart_baud=9600) > > > > > > Simon/Tom, did this slip through the cracks, or are you deferring it until the next release? > > We haven't heard back on the review comments. > > I don't really mind if we go the DT approach or this one. But this > patch needs a little rework I think. I thought the main review comment was "didn't we try and do this with DT?" ? I'm feeling like we might want to skip doing anything DT related for this part...
Hi Tom, On 7 March 2016 at 16:37, Tom Rini <trini@konsulko.com> wrote: > On Sun, Mar 06, 2016 at 07:38:47PM -0700, Simon Glass wrote: >> Hi Stephen, >> >> On 5 March 2016 at 23:26, Stephen Warren <swarren@wwwdotorg.org> wrote: >> > >> > On 02/05/2016 09:19 PM, Stephen Warren wrote: >> >> >> >> On 02/05/2016 02:43 PM, Eric Anholt wrote: >> >>> >> >>> For Raspberry Pi, we had the input clock rate to the pl011 fixed in >> >>> the rpi.c file, but it may be changed by firmware due to user changes >> >>> to config.txt. Since the firmware always sets up the uart (default >> >>> 115200 output unless the user changes it), we can just skip our own >> >>> uart init to simplify the boot process and more reliably get serial >> >>> output. >> >> >> >> >> >> Tested-by: Stephen Warren <swarren@wwwdotorg.org> >> >> (With a 6MHz init_uart_clock and with/without init_uart_baud=9600) >> > >> > >> > Simon/Tom, did this slip through the cracks, or are you deferring it until the next release? >> >> We haven't heard back on the review comments. >> >> I don't really mind if we go the DT approach or this one. But this >> patch needs a little rework I think. > > I thought the main review comment was "didn't we try and do this with > DT?" ? I'm feeling like we might want to skip doing anything DT related > for this part... My comment was on 6 Feb: > + TYPE_PL01X_PRECONFIGURED, This seems odd. You are not adding a new UART type. How about a separate bool flag? Regards, Simon
diff --git a/board/raspberrypi/rpi/rpi.c b/board/raspberrypi/rpi/rpi.c index 4b80d7b..1fd3f97 100644 --- a/board/raspberrypi/rpi/rpi.c +++ b/board/raspberrypi/rpi/rpi.c @@ -35,8 +35,7 @@ static const struct pl01x_serial_platdata serial_platdata = { #else .base = 0x20201000, #endif - .type = TYPE_PL011, - .clock = 3000000, + .type = TYPE_PL01X_PRECONFIGURED, }; U_BOOT_DEVICE(bcm2835_serials) = { diff --git a/drivers/serial/serial_pl01x.c b/drivers/serial/serial_pl01x.c index 552c945..7d4d214 100644 --- a/drivers/serial/serial_pl01x.c +++ b/drivers/serial/serial_pl01x.c @@ -83,6 +83,8 @@ static int pl01x_generic_serial_init(struct pl01x_regs *regs, /* disable everything */ writel(0, ®s->pl011_cr); break; + case TYPE_PL01X_PRECONFIGURED: + break; default: return -EINVAL; } @@ -172,6 +174,8 @@ static int pl01x_generic_setbrg(struct pl01x_regs *regs, enum pl01x_type type, writel(UART_PL011_CR_UARTEN | UART_PL011_CR_TXE | UART_PL011_CR_RXE | UART_PL011_CR_RTS, ®s->pl011_cr); break; + case TYPE_PL01X_PRECONFIGURED: + break; } default: return -EINVAL; diff --git a/include/dm/platform_data/serial_pl01x.h b/include/dm/platform_data/serial_pl01x.h index 5e068f3..ae1bf31 100644 --- a/include/dm/platform_data/serial_pl01x.h +++ b/include/dm/platform_data/serial_pl01x.h @@ -9,6 +9,7 @@ enum pl01x_type { TYPE_PL010, TYPE_PL011, + TYPE_PL01X_PRECONFIGURED, }; /*
For Raspberry Pi, we had the input clock rate to the pl011 fixed in the rpi.c file, but it may be changed by firmware due to user changes to config.txt. Since the firmware always sets up the uart (default 115200 output unless the user changes it), we can just skip our own uart init to simplify the boot process and more reliably get serial output. Signed-off-by: Eric Anholt <eric@anholt.net> --- board/raspberrypi/rpi/rpi.c | 3 +-- drivers/serial/serial_pl01x.c | 4 ++++ include/dm/platform_data/serial_pl01x.h | 1 + 3 files changed, 6 insertions(+), 2 deletions(-)