| Message ID | 1386129715-5820-1-git-send-email-swarren@wwwdotorg.org |
|---|---|
| State | Superseded |
| Delegated to: | Albert ARIBAUD |
| Headers | show |
On 12/03/2013 09:01 PM, Stephen Warren wrote: > Send RPC commands to the VideoCore to turn on the SDHCI and USB modules. > For SDHCI this isn't needed in practice, since the firmware already > turned on the power in order to load U-Boot. However, it's best to be > explicit. For USB, this is necessary, since the module isn't powered > otherwise. This will allow the kernel USB driver to work. BTW, I should mention that I'd particularly like to get this patch into 2014.01, since Linux kernel 3.14 will rely on the USB controller being powered, since kernel USB support is now enabled in linux-next.
Hi Stephen, On Tue, Dec 03, 2013 at 09:01:55PM -0700, Stephen Warren wrote: > Send RPC commands to the VideoCore to turn on the SDHCI and USB modules. > For SDHCI this isn't needed in practice, since the firmware already > turned on the power in order to load U-Boot. However, it's best to be > explicit. For USB, this is necessary, since the module isn't powered > otherwise. This will allow the kernel USB driver to work. I didn't test this patch yet, but from skimming over it it looks similar to what I tried with barebox a while back. What I did notice with the "set power" mbox call is that it takes way longer than 100ms (the current mbox call timeout) to finish on a cold boot. You don't seem to bump the timeout here, and with 100ms I always hit it and hence the mbox call failed for me. Don't you get these huge delays? Regards, Andre
On 12/06/2013 06:37 AM, Andre Heider wrote: > Hi Stephen, > > On Tue, Dec 03, 2013 at 09:01:55PM -0700, Stephen Warren wrote: >> Send RPC commands to the VideoCore to turn on the SDHCI and USB modules. >> For SDHCI this isn't needed in practice, since the firmware already >> turned on the power in order to load U-Boot. However, it's best to be >> explicit. For USB, this is necessary, since the module isn't powered >> otherwise. This will allow the kernel USB driver to work. > > I didn't test this patch yet, but from skimming over it it looks similar to > what I tried with barebox a while back. > > What I did notice with the "set power" mbox call is that it takes way longer > than 100ms (the current mbox call timeout) to finish on a cold boot. You > don't seem to bump the timeout here, and with 100ms I always hit it and > hence the mbox call failed for me. Don't you get these huge delays? I didn't notice this, but I'll have to double-check to be completely sure. What firmware version are you using? I'm currently using the latest firmware (as of a week or two ago) from the git repo in github. I do recall issues with the set_power call operating incorrectly, although I think not a timeout, in the past, but that issue was mostly fixed in a firmware update quite a while ago. For details, see: https://github.com/raspberrypi/firmware/issues/106
On 12/06/2013 06:37 AM, Andre Heider wrote: > Hi Stephen, > > On Tue, Dec 03, 2013 at 09:01:55PM -0700, Stephen Warren wrote: >> Send RPC commands to the VideoCore to turn on the SDHCI and USB modules. >> For SDHCI this isn't needed in practice, since the firmware already >> turned on the power in order to load U-Boot. However, it's best to be >> explicit. For USB, this is necessary, since the module isn't powered >> otherwise. This will allow the kernel USB driver to work. > > I didn't test this patch yet, but from skimming over it it looks similar to > what I tried with barebox a while back. > > What I did notice with the "set power" mbox call is that it takes way longer > than 100ms (the current mbox call timeout) to finish on a cold boot. You > don't seem to bump the timeout here, and with 100ms I always hit it and > hence the mbox call failed for me. Don't you get these huge delays? I have firmware commit b38194c "kernel: Bump version to 3.10.19", and I'm seeing a valid non-error response to both the SD and USB set_power requests, with no timeouts.
On Fri, Dec 06, 2013 at 09:50:24PM -0700, Stephen Warren wrote: > On 12/06/2013 06:37 AM, Andre Heider wrote: > > Hi Stephen, > > > > On Tue, Dec 03, 2013 at 09:01:55PM -0700, Stephen Warren wrote: > >> Send RPC commands to the VideoCore to turn on the SDHCI and USB modules. > >> For SDHCI this isn't needed in practice, since the firmware already > >> turned on the power in order to load U-Boot. However, it's best to be > >> explicit. For USB, this is necessary, since the module isn't powered > >> otherwise. This will allow the kernel USB driver to work. > > > > I didn't test this patch yet, but from skimming over it it looks similar to > > what I tried with barebox a while back. > > > > What I did notice with the "set power" mbox call is that it takes way longer > > than 100ms (the current mbox call timeout) to finish on a cold boot. You > > don't seem to bump the timeout here, and with 100ms I always hit it and > > hence the mbox call failed for me. Don't you get these huge delays? > > I have firmware commit b38194c "kernel: Bump version to 3.10.19", and > I'm seeing a valid non-error response to both the SD and USB set_power > requests, with no timeouts. This patch indeed works and does not trigger the timeout, but after a bit of digging and cross checking it seems that's because the used timeout is using the wrong unit ;) get_timer() returns mS, while arch/arm/cpu/arm1176/bcm2835/mbox.c uses: #define TIMEOUT (100 * 1000) /* 100mS in uS */ Hacking the mbox call to run into that timeout confirms its waiting 100s instead of 100mS. And fixing this to: #define TIMEOUT (100) /* 100mS */ then triggers the timeout I was seeing with barebox. While I ported your mbox driver over I didn't notice this, I just went by the comment and used 100mS there ;) Regards, Andre
On 12/09/2013 12:52 PM, Andre Heider wrote: > On Fri, Dec 06, 2013 at 09:50:24PM -0700, Stephen Warren wrote: >> On 12/06/2013 06:37 AM, Andre Heider wrote: >>> Hi Stephen, >>> >>> On Tue, Dec 03, 2013 at 09:01:55PM -0700, Stephen Warren wrote: >>>> Send RPC commands to the VideoCore to turn on the SDHCI and USB modules. >>>> For SDHCI this isn't needed in practice, since the firmware already >>>> turned on the power in order to load U-Boot. However, it's best to be >>>> explicit. For USB, this is necessary, since the module isn't powered >>>> otherwise. This will allow the kernel USB driver to work. >>> >>> I didn't test this patch yet, but from skimming over it it looks similar to >>> what I tried with barebox a while back. >>> >>> What I did notice with the "set power" mbox call is that it takes way longer >>> than 100ms (the current mbox call timeout) to finish on a cold boot. You >>> don't seem to bump the timeout here, and with 100ms I always hit it and >>> hence the mbox call failed for me. Don't you get these huge delays? >> >> I have firmware commit b38194c "kernel: Bump version to 3.10.19", and >> I'm seeing a valid non-error response to both the SD and USB set_power >> requests, with no timeouts. > > This patch indeed works and does not trigger the timeout, but after a > bit of digging and cross checking it seems that's because the used > timeout is using the wrong unit ;) > > get_timer() returns mS, while arch/arm/cpu/arm1176/bcm2835/mbox.c uses: > #define TIMEOUT (100 * 1000) /* 100mS in uS */ > > Hacking the mbox call to run into that timeout confirms its waiting 100s > instead of 100mS. > > And fixing this to: > #define TIMEOUT (100) /* 100mS */ > > then triggers the timeout I was seeing with barebox. So, is the issue simply that this message needs longer than 100mS to execute? If so, do you know exactly how long? Perhaps we should just set the timeout to e.g. 1s rather than fixing it to the originally-intended 100ms? > While I ported your > mbox driver over I didn't notice this, I just went by the comment and used > 100mS there ;) > > Regards, > Andre
On Mon, Dec 09, 2013 at 01:16:45PM -0700, Stephen Warren wrote: > On 12/09/2013 12:52 PM, Andre Heider wrote: > > On Fri, Dec 06, 2013 at 09:50:24PM -0700, Stephen Warren wrote: > >> On 12/06/2013 06:37 AM, Andre Heider wrote: > >>> Hi Stephen, > >>> > >>> On Tue, Dec 03, 2013 at 09:01:55PM -0700, Stephen Warren wrote: > >>>> Send RPC commands to the VideoCore to turn on the SDHCI and USB modules. > >>>> For SDHCI this isn't needed in practice, since the firmware already > >>>> turned on the power in order to load U-Boot. However, it's best to be > >>>> explicit. For USB, this is necessary, since the module isn't powered > >>>> otherwise. This will allow the kernel USB driver to work. > >>> > >>> I didn't test this patch yet, but from skimming over it it looks similar to > >>> what I tried with barebox a while back. > >>> > >>> What I did notice with the "set power" mbox call is that it takes way longer > >>> than 100ms (the current mbox call timeout) to finish on a cold boot. You > >>> don't seem to bump the timeout here, and with 100ms I always hit it and > >>> hence the mbox call failed for me. Don't you get these huge delays? > >> > >> I have firmware commit b38194c "kernel: Bump version to 3.10.19", and > >> I'm seeing a valid non-error response to both the SD and USB set_power > >> requests, with no timeouts. > > > > This patch indeed works and does not trigger the timeout, but after a > > bit of digging and cross checking it seems that's because the used > > timeout is using the wrong unit ;) > > > > get_timer() returns mS, while arch/arm/cpu/arm1176/bcm2835/mbox.c uses: > > #define TIMEOUT (100 * 1000) /* 100mS in uS */ > > > > Hacking the mbox call to run into that timeout confirms its waiting 100s > > instead of 100mS. > > > > And fixing this to: > > #define TIMEOUT (100) /* 100mS */ > > > > then triggers the timeout I was seeing with barebox. > > So, is the issue simply that this message needs longer than 100mS to > execute? If so, do you know exactly how long? Perhaps we should just set > the timeout to e.g. 1s rather than fixing it to the originally-intended > 100ms? Yeah, the power on calls just take that long, at least with the WAIT flag set. 500mS wasn't enough in my case for USB, 750mS works but I didn't check how close it is to hit it, so 1s sounds good to me. On the other side, there's a "get timing" mbox call [0], which would allow us to use a dynamic timeout, so supposed to be fast mbox calls don't unnecessary wait for too long upon failing. You mentioned getting this patch into 2014.01, so maybe that's something for another day. [0] https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface#get-timing Regards, Andre
diff --git a/arch/arm/include/asm/arch-bcm2835/mbox.h b/arch/arm/include/asm/arch-bcm2835/mbox.h index 6b806ec..38cb42a 100644 --- a/arch/arm/include/asm/arch-bcm2835/mbox.h +++ b/arch/arm/include/asm/arch-bcm2835/mbox.h @@ -133,6 +133,54 @@ struct bcm2835_mbox_tag_get_arm_mem { } body; }; +#define BCM2835_MBOX_POWER_DEVID_SDHCI 0 +#define BCM2835_MBOX_POWER_DEVID_UART0 1 +#define BCM2835_MBOX_POWER_DEVID_UART1 2 +#define BCM2835_MBOX_POWER_DEVID_USB_HCD 3 +#define BCM2835_MBOX_POWER_DEVID_I2C0 4 +#define BCM2835_MBOX_POWER_DEVID_I2C1 5 +#define BCM2835_MBOX_POWER_DEVID_I2C2 6 +#define BCM2835_MBOX_POWER_DEVID_SPI 7 +#define BCM2835_MBOX_POWER_DEVID_CCP2TX 8 + +#define BCM2835_MBOX_POWER_STATE_RESP_ON (1 << 1) +/* Device doesn't exist */ +#define BCM2835_MBOX_POWER_STATE_RESP_NODEV (1 << 1) + +#define BCM2835_MBOX_TAG_GET_POWER_STATE 0x00020001 + +struct bcm2835_mbox_tag_get_power_state { + struct bcm2835_mbox_tag_hdr tag_hdr; + union { + struct { + u32 device_id; + } req; + struct { + u32 device_id; + u32 state; + } resp; + } body; +}; + +#define BCM2835_MBOX_TAG_SET_POWER_STATE 0x00028001 + +#define BCM2835_MBOX_SET_POWER_STATE_REQ_ON (1 << 0) +#define BCM2835_MBOX_SET_POWER_STATE_REQ_WAIT (1 << 1) + +struct bcm2835_mbox_tag_set_power_state { + struct bcm2835_mbox_tag_hdr tag_hdr; + union { + struct { + u32 device_id; + u32 state; + } req; + struct { + u32 device_id; + u32 state; + } resp; + } body; +}; + #define BCM2835_MBOX_TAG_GET_CLOCK_RATE 0x00030002 #define BCM2835_MBOX_CLOCK_ID_EMMC 1 diff --git a/board/raspberrypi/rpi_b/rpi_b.c b/board/raspberrypi/rpi_b/rpi_b.c index 16d442a..f33fae9 100644 --- a/board/raspberrypi/rpi_b/rpi_b.c +++ b/board/raspberrypi/rpi_b/rpi_b.c @@ -29,6 +29,12 @@ struct msg_get_arm_mem { u32 end_tag; }; +struct msg_set_power_state { + struct bcm2835_mbox_hdr hdr; + struct bcm2835_mbox_tag_set_power_state set_power_state; + u32 end_tag; +}; + struct msg_get_clock_rate { struct bcm2835_mbox_hdr hdr; struct bcm2835_mbox_tag_get_clock_rate get_clock_rate; @@ -54,11 +60,35 @@ int dram_init(void) return 0; } +static int power_on_module(u32 module) +{ + ALLOC_ALIGN_BUFFER(struct msg_set_power_state, msg_pwr, 1, 16); + int ret; + + BCM2835_MBOX_INIT_HDR(msg_pwr); + BCM2835_MBOX_INIT_TAG(&msg_pwr->set_power_state, + SET_POWER_STATE); + msg_pwr->set_power_state.body.req.device_id = module; + msg_pwr->set_power_state.body.req.state = + BCM2835_MBOX_SET_POWER_STATE_REQ_ON | + BCM2835_MBOX_SET_POWER_STATE_REQ_WAIT; + + ret = bcm2835_mbox_call_prop(BCM2835_MBOX_PROP_CHAN, + &msg_pwr->hdr); + if (ret) { + printf("bcm2835: Could not set module %u power state\n", + module); + return -1; + } + + return 0; +} + int board_init(void) { gd->bd->bi_boot_params = 0x100; - return 0; + return power_on_module(BCM2835_MBOX_POWER_DEVID_USB_HCD); } int board_mmc_init(void) @@ -66,6 +96,8 @@ int board_mmc_init(void) ALLOC_ALIGN_BUFFER(struct msg_get_clock_rate, msg_clk, 1, 16); int ret; + power_on_module(BCM2835_MBOX_POWER_DEVID_SDHCI); + BCM2835_MBOX_INIT_HDR(msg_clk); BCM2835_MBOX_INIT_TAG(&msg_clk->get_clock_rate, GET_CLOCK_RATE); msg_clk->get_clock_rate.body.req.clock_id = BCM2835_MBOX_CLOCK_ID_EMMC;
Send RPC commands to the VideoCore to turn on the SDHCI and USB modules. For SDHCI this isn't needed in practice, since the firmware already turned on the power in order to load U-Boot. However, it's best to be explicit. For USB, this is necessary, since the module isn't powered otherwise. This will allow the kernel USB driver to work. Signed-off-by: Stephen Warren <swarren@wwwdotorg.org> --- arch/arm/include/asm/arch-bcm2835/mbox.h | 48 ++++++++++++++++++++++++++++++ board/raspberrypi/rpi_b/rpi_b.c | 34 ++++++++++++++++++++- 2 files changed, 81 insertions(+), 1 deletion(-)