| Message ID | 1480313126-4470-1-git-send-email-mirza.krak@gmail.com |
|---|---|
| State | Not Applicable |
| Delegated to: | Tom Rini |
| Headers | show |
On 11/27/2016 11:05 PM, Mirza Krak wrote: > From: Mirza Krak <mirza.krak@gmail.com> > > Recently the default UART clock rate has been changed to 48 MHz on all > pi`s in the firmware files, which broke UART support in u-boot. > > Align configuration to boot firmware. > diff --git a/board/raspberrypi/rpi/rpi.c b/board/raspberrypi/rpi/rpi.c > @@ -37,7 +37,7 @@ static const struct pl01x_serial_platdata serial_platdata = { > .base = 0x20201000, > #endif > .type = TYPE_PL011, > - .clock = 3000000, > + .clock = 48000000, I'm not sure this is the best fix, since it will prevent the latest U-Boot from working with the FW before the change. Better would be: a) Revert the change in the FW, since it violates the contract that previously existed between FW and OS, and will likely break UART usage on (almost?) all SW. b) Detect the correct clock rate at run-time rather than hard-coding it. b1) Perhaps the rate is available in the DT passed by the FW, which following http://patchwork.ozlabs.org/patch/690769/ "rpi: save firmware provided boot param for later use" should now be available within U-Boot to parse. b2) Perhaps query the rate from the FW via the mailbox API; see https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface
2016-11-30 5:05 GMT+01:00 Stephen Warren <swarren@wwwdotorg.org>: > On 11/27/2016 11:05 PM, Mirza Krak wrote: >> >> From: Mirza Krak <mirza.krak@gmail.com> >> >> Recently the default UART clock rate has been changed to 48 MHz on all >> pi`s in the firmware files, which broke UART support in u-boot. >> >> Align configuration to boot firmware. > > >> diff --git a/board/raspberrypi/rpi/rpi.c b/board/raspberrypi/rpi/rpi.c > > >> @@ -37,7 +37,7 @@ static const struct pl01x_serial_platdata >> serial_platdata = { >> .base = 0x20201000, >> #endif >> .type = TYPE_PL011, >> - .clock = 3000000, >> + .clock = 48000000, > > > I'm not sure this is the best fix, since it will prevent the latest U-Boot > from working with the FW before the change. Agree that there are better ways of doing this. My purpose with this was mainly bringing this problem to your attention as well. > > Better would be: > > a) Revert the change in the FW, since it violates the contract that > previously existed between FW and OS, and will likely break UART usage on > (almost?) all SW. I am not really in position to tell anyone reverting anything. I am merely a user reporting a problem. This does not break UART on Linux, if you skip U-boot. Because the FW patches the DT with the correct value. But it is a problem if you use U-boot and then load the DT from U-boot, then you will lose the "patched" DT and break UART on Linux as well. Should be said that the "init_uart_clock" has always been a configurable option for raspberry pi, U-boot has never actually supported configuration of that property and hence there is a problem now when the default value of this property has changed. So the best solutions is probably supporting parsing the options from firmware to setup the UART clock rate correctly (DT or Mailbox). Jumping to b1), for more text. > > b) Detect the correct clock rate at run-time rather than hard-coding it. > > b1) Perhaps the rate is available in the DT passed by the FW, which > following http://patchwork.ozlabs.org/patch/690769/ "rpi: save firmware > provided boot param for later use" should now be available within U-Boot to > parse. This is good to know that it is possible too do, since the FW provided DT does contain the correct value for UART clock and I think that this would be the best approach for solving this and also supporting any value that the user sees fit (by changing the init_uart_clock property). > > b2) Perhaps query the rate from the FW via the mailbox API; see > https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface Is this API implemented in u-boot? Could be even better to use this, then we are not depended on a DT file from firmware (which is my use case, where I do not provide a DT to the firmware at all). Best Regards Mirza
On 11/29/2016 11:47 PM, Mirza Krak wrote: > 2016-11-30 5:05 GMT+01:00 Stephen Warren <swarren@wwwdotorg.org>: >> On 11/27/2016 11:05 PM, Mirza Krak wrote: >>> >>> From: Mirza Krak <mirza.krak@gmail.com> >>> >>> Recently the default UART clock rate has been changed to 48 MHz on all >>> pi`s in the firmware files, which broke UART support in u-boot. >>> >>> Align configuration to boot firmware. >> >> >>> diff --git a/board/raspberrypi/rpi/rpi.c b/board/raspberrypi/rpi/rpi.c >> >> >>> @@ -37,7 +37,7 @@ static const struct pl01x_serial_platdata >>> serial_platdata = { >>> .base = 0x20201000, >>> #endif >>> .type = TYPE_PL011, >>> - .clock = 3000000, >>> + .clock = 48000000, Now that I look into this some more, I will point out that this code doesn't even exist in the latest U-Boot, so your patch doesn't apply. Instead, the UART is now instantiated from DT:-( Presumably the UART drivers attempt to dynamically query the clock rate from the clock provider described in DT, yet since there is no such provider implemented in U-Boot, this must currently fail, and I presume that if U-Boot still works at all on the Pi, then the UART drivers must simply skip programming the divider at all, and thus rely on the FW having initialized it. Does updating to the latest U-Boot code solve your problem? I've left my earlier reply below, but I suspect it's no longer relevant. >> I'm not sure this is the best fix, since it will prevent the latest U-Boot >> from working with the FW before the change. > > Agree that there are better ways of doing this. My purpose with this > was mainly bringing this problem to your attention as well. > >> >> Better would be: >> >> a) Revert the change in the FW, since it violates the contract that >> previously existed between FW and OS, and will likely break UART usage on >> (almost?) all SW. > > I am not really in position to tell anyone reverting anything. I am > merely a user reporting a problem. > > This does not break UART on Linux, if you skip U-boot. Because the FW > patches the DT with the correct value. But it is a problem if you use > U-boot and then load the DT from U-boot, then you will lose the > "patched" DT and break UART on Linux as well. > > Should be said that the "init_uart_clock" has always been a > configurable option for raspberry pi, U-boot has never actually > supported configuration of that property and hence there is a problem > now when the default value of this property has changed. So the best > solutions is probably supporting parsing the options from firmware to > setup the UART clock rate correctly (DT or Mailbox). > > Jumping to b1), for more text. > >> b) Detect the correct clock rate at run-time rather than hard-coding it. >> >> b1) Perhaps the rate is available in the DT passed by the FW, which >> following http://patchwork.ozlabs.org/patch/690769/ "rpi: save firmware >> provided boot param for later use" should now be available within U-Boot to >> parse. > > This is good to know that it is possible too do, since the FW provided > DT does contain the correct value for UART clock and I think that this > would be the best approach for solving this and also supporting any > value that the user sees fit (by changing the init_uart_clock > property). > >> >> b2) Perhaps query the rate from the FW via the mailbox API; see >> https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface > > Is this API implemented in u-boot? Could be even better to use this, > then we are not depended on a DT file from firmware (which is my use > case, where I do not provide a DT to the firmware at all). U-Boot already queries the MMC clock rate using the mbox API. I imagine all you'd need to do is duplicate the logic in board_mmc_init(), and update it to query the UART clock rate rather than the MMC clock rate. Be aware that the "UART clock" only feeds one of the two UARTs in the system; this new query code should probably only run if the appropriate UART is in use; see code that's #ifdef CONFIG_PL01X_SERIAL. If you could test this with the FW both before and after the default rate change that'd be great, just to be sure. I do think querying the FW rather than parsing the DT is the better option, since that isolates U-Boot from any DT binding changes in this area. There shouldn't be any, but you never know. DT is often more fluid than it should be. (Update having noticed the RPi DT conversion: To make U-Boot query the clock from the FW, you should implement a clock provider that talks to the FW, and presumably the UART drivers will automatically use this)
2016-11-30 19:07 GMT+01:00 Stephen Warren <swarren@wwwdotorg.org>: > On 11/29/2016 11:47 PM, Mirza Krak wrote: >> >> 2016-11-30 5:05 GMT+01:00 Stephen Warren <swarren@wwwdotorg.org>: >>> >>> On 11/27/2016 11:05 PM, Mirza Krak wrote: >>>> >>>> >>>> From: Mirza Krak <mirza.krak@gmail.com> >>>> >>>> Recently the default UART clock rate has been changed to 48 MHz on all >>>> pi`s in the firmware files, which broke UART support in u-boot. >>>> >>>> Align configuration to boot firmware. >>> >>> >>> >>>> diff --git a/board/raspberrypi/rpi/rpi.c b/board/raspberrypi/rpi/rpi.c >>> >>> >>> >>>> @@ -37,7 +37,7 @@ static const struct pl01x_serial_platdata >>>> serial_platdata = { >>>> .base = 0x20201000, >>>> #endif >>>> .type = TYPE_PL011, >>>> - .clock = 3000000, >>>> + .clock = 48000000, > > > Now that I look into this some more, I will point out that this code doesn't > even exist in the latest U-Boot, so your patch doesn't apply. Yes, my bad. I am on 2016.03. Looking in upstream code I can see that some changes have been made regarding this. > > Instead, the UART is now instantiated from DT:-( Presumably the UART drivers > attempt to dynamically query the clock rate from the clock provider > described in DT, yet since there is no such provider implemented in U-Boot, > this must currently fail, and I presume that if U-Boot still works at all on > the Pi, then the UART drivers must simply skip programming the divider at > all, and thus rely on the FW having initialized it. From what I see it no longer tries to initialize the UART on RPi at all and does rely on the FW having initialized it, which seems to be intentional. See [1]. > > Does updating to the latest U-Boot code solve your problem? Will give it a go, but probably solves my problems. [1]. http://git.denx.de/?p=u-boot.git;a=commit;h=cd0fa5bff8052b19bde6967c2734f323c9848568 Best Regards Mirza
2016-12-01 10:34 GMT+01:00 Mirza Krak <mirza.krak@gmail.com>: > 2016-11-30 19:07 GMT+01:00 Stephen Warren <swarren@wwwdotorg.org>: >> On 11/29/2016 11:47 PM, Mirza Krak wrote: >>> >>> 2016-11-30 5:05 GMT+01:00 Stephen Warren <swarren@wwwdotorg.org>: >>>> >>>> On 11/27/2016 11:05 PM, Mirza Krak wrote: >>>>> >>>>> >>>>> From: Mirza Krak <mirza.krak@gmail.com> >>>>> >>>>> Recently the default UART clock rate has been changed to 48 MHz on all >>>>> pi`s in the firmware files, which broke UART support in u-boot. >>>>> >>>>> Align configuration to boot firmware. >>>> >>>> >>>> >>>>> diff --git a/board/raspberrypi/rpi/rpi.c b/board/raspberrypi/rpi/rpi.c >>>> >>>> >>>> >>>>> @@ -37,7 +37,7 @@ static const struct pl01x_serial_platdata >>>>> serial_platdata = { >>>>> .base = 0x20201000, >>>>> #endif >>>>> .type = TYPE_PL011, >>>>> - .clock = 3000000, >>>>> + .clock = 48000000, >> >> >> Now that I look into this some more, I will point out that this code doesn't >> even exist in the latest U-Boot, so your patch doesn't apply. > > Yes, my bad. I am on 2016.03. Looking in upstream code I can see that > some changes have been made regarding this. > >> >> Instead, the UART is now instantiated from DT:-( Presumably the UART drivers >> attempt to dynamically query the clock rate from the clock provider >> described in DT, yet since there is no such provider implemented in U-Boot, >> this must currently fail, and I presume that if U-Boot still works at all on >> the Pi, then the UART drivers must simply skip programming the divider at >> all, and thus rely on the FW having initialized it. > > From what I see it no longer tries to initialize the UART on RPi at > all and does rely on the FW having initialized it, which seems to be > intentional. See [1]. > >> >> Does updating to the latest U-Boot code solve your problem? > > Will give it a go, but probably solves my problems. Did a test run on 2016.11 U-boot and works like a charm. Best Regards Mirza
diff --git a/board/raspberrypi/rpi/rpi.c b/board/raspberrypi/rpi/rpi.c index 1d3a4e0..a8996d7 100644 --- a/board/raspberrypi/rpi/rpi.c +++ b/board/raspberrypi/rpi/rpi.c @@ -37,7 +37,7 @@ static const struct pl01x_serial_platdata serial_platdata = { .base = 0x20201000, #endif .type = TYPE_PL011, - .clock = 3000000, + .clock = 48000000, }; U_BOOT_DEVICE(bcm2835_serials) = {