* [Qemu-devel] [PATCH 0/3] sdhci patches to enable Raspberry Pi @ 2015-12-16 19:47 Andrew Baumann 2015-12-16 19:47 ` [Qemu-devel] [PATCH 1/3] sd: sdhci: Delete over-zealous power check Andrew Baumann ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Andrew Baumann @ 2015-12-16 19:47 UTC (permalink / raw) To: qemu-devel Cc: Igor Mitsyanko, Peter Maydell, Peter Crosthwaite, Andrew Baumann, Stefan Hajnoczi This is a series of three tweaks needed to enable the generic sdhci controller to emulate Raspberry Pi (bcm2835/2836), and boot Linux and Windows. There was some discussion of these changes in the following thread: https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg01401.html Cheers, Andrew Andrew Baumann (3): sd: sdhci: Delete over-zealous power check sdhci: don't raise a command index error for an unexpected response sdhci: add optional quirk property to disable card insertion/removal interrupts hw/sd/sdhci.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) -- 2.5.3 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH 1/3] sd: sdhci: Delete over-zealous power check 2015-12-16 19:47 [Qemu-devel] [PATCH 0/3] sdhci patches to enable Raspberry Pi Andrew Baumann @ 2015-12-16 19:47 ` Andrew Baumann 2015-12-16 20:00 ` Peter Crosthwaite 2015-12-16 19:47 ` [Qemu-devel] [PATCH 2/3] sdhci: don't raise a command index error for an unexpected response Andrew Baumann 2015-12-16 19:47 ` [Qemu-devel] [PATCH 3/3] sdhci: add optional quirk property to disable card insertion/removal interrupts Andrew Baumann 2 siblings, 1 reply; 8+ messages in thread From: Andrew Baumann @ 2015-12-16 19:47 UTC (permalink / raw) To: qemu-devel Cc: Peter Maydell, Peter Crosthwaite, Igor Mitsyanko, Andrew Baumann, Peter Crosthwaite, Stefan Hajnoczi This check was conditionalising SD card operation on the card being powered by the SDHCI host controller. It is however possible (particularly in embedded systems) for the power control of the SD card to be managed outside of SDHCI. This can be as trivial as hard-wiring the SD slot VCC to a constant power-rail. This means the guest SDHCI can validly opt-out of the SDHCI power control feature while still using the card. So delete this check to allow operation of the card with SDHCI power control. This is needed for at least Xilinx Zynq and also makes Freescale i.MX25 work for me. The digilent Zybo board has a public schematic which shows SD VCC hardwiring: http://digilentinc.com/Data/Products/ZYBO/ZYBO_sch_VB.3.pdf bottom of page 3. Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com> Reviewed-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com> Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com> --- Notes: This is exactly equivalent to Peter's patch here: https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg06271.html ... which is why I've copied the signed-off-by lines (I assume that is legit; if not, my apologies!). It is also needed for Pi. hw/sd/sdhci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c index 8612760..bc39d48 100644 --- a/hw/sd/sdhci.c +++ b/hw/sd/sdhci.c @@ -831,7 +831,7 @@ static void sdhci_data_transfer(void *opaque) static bool sdhci_can_issue_command(SDHCIState *s) { - if (!SDHC_CLOCK_IS_ON(s->clkcon) || !(s->pwrcon & SDHC_POWER_ON) || + if (!SDHC_CLOCK_IS_ON(s->clkcon) || (((s->prnsts & SDHC_DATA_INHIBIT) || s->stopped_state) && ((s->cmdreg & SDHC_CMD_DATA_PRESENT) || ((s->cmdreg & SDHC_CMD_RESPONSE) == SDHC_CMD_RSP_WITH_BUSY && -- 2.5.3 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] sd: sdhci: Delete over-zealous power check 2015-12-16 19:47 ` [Qemu-devel] [PATCH 1/3] sd: sdhci: Delete over-zealous power check Andrew Baumann @ 2015-12-16 20:00 ` Peter Crosthwaite 2015-12-16 20:28 ` Andrew Baumann 0 siblings, 1 reply; 8+ messages in thread From: Peter Crosthwaite @ 2015-12-16 20:00 UTC (permalink / raw) To: Andrew Baumann Cc: Igor Mitsyanko, Peter Maydell, qemu-devel@nongnu.org Developers, Stefan Hajnoczi, Peter Crosthwaite On Wed, Dec 16, 2015 at 11:47 AM, Andrew Baumann <Andrew.Baumann@microsoft.com> wrote: > This check was conditionalising SD card operation on the card being > powered by the SDHCI host controller. It is however possible > (particularly in embedded systems) for the power control of the SD card > to be managed outside of SDHCI. This can be as trivial as hard-wiring > the SD slot VCC to a constant power-rail. > > This means the guest SDHCI can validly opt-out of the SDHCI power > control feature while still using the card. So delete this check to > allow operation of the card with SDHCI power control. > > This is needed for at least Xilinx Zynq and also makes Freescale i.MX25 > work for me. The digilent Zybo board has a public schematic > which shows SD VCC hardwiring: > You should add a note that it is needed for rPI as well. > http://digilentinc.com/Data/Products/ZYBO/ZYBO_sch_VB.3.pdf > bottom of page 3. > > Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com> > Reviewed-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com> You should add your tested-by as it is valuable as non-author. Pavan probably should be CC'd singly (got can be configured to do this automatically) but I would just CC him whole series as he has does some SD work. At this point here you add committable notes about what you have changed since the original authorship. If you added the note about fixing rPI, something like: [ AB changes: * Add rPI to list of devices fixed in commit message ] > Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com> > --- > > Notes: > This is exactly equivalent to Peter's patch here: > https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg06271.html > ... which is why I've copied the signed-off-by lines (I assume that is > legit; if not, my apologies!). It is also needed for Pi. > The correct process it to apply the patch locally as originally, authored then append your SoB and any notes/changes using git commit --amend. Git am can apply the patch from an mbox, or the patches utility can be used to download and apply patches off the list. You could in this instance grab the original patch from my github tree: https://github.com/pcrost/qemu/commit/db8e44ad1d31418fb4b5aaacc8552a39bb2d670f Regards, Peter > hw/sd/sdhci.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c > index 8612760..bc39d48 100644 > --- a/hw/sd/sdhci.c > +++ b/hw/sd/sdhci.c > @@ -831,7 +831,7 @@ static void sdhci_data_transfer(void *opaque) > > static bool sdhci_can_issue_command(SDHCIState *s) > { > - if (!SDHC_CLOCK_IS_ON(s->clkcon) || !(s->pwrcon & SDHC_POWER_ON) || > + if (!SDHC_CLOCK_IS_ON(s->clkcon) || > (((s->prnsts & SDHC_DATA_INHIBIT) || s->stopped_state) && > ((s->cmdreg & SDHC_CMD_DATA_PRESENT) || > ((s->cmdreg & SDHC_CMD_RESPONSE) == SDHC_CMD_RSP_WITH_BUSY && > -- > 2.5.3 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] sd: sdhci: Delete over-zealous power check 2015-12-16 20:00 ` Peter Crosthwaite @ 2015-12-16 20:28 ` Andrew Baumann 0 siblings, 0 replies; 8+ messages in thread From: Andrew Baumann @ 2015-12-16 20:28 UTC (permalink / raw) To: Peter Crosthwaite Cc: Igor Mitsyanko, Peter Maydell, qemu-devel@nongnu.org Developers, Stefan Hajnoczi, Peter Crosthwaite > From: Peter Crosthwaite [mailto:crosthwaitepeter@gmail.com] > Sent: Wednesday, 16 December 2015 12:00 > On Wed, Dec 16, 2015 at 11:47 AM, Andrew Baumann > <Andrew.Baumann@microsoft.com> wrote: > > This is exactly equivalent to Peter's patch here: > > https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg06271.html > > ... which is why I've copied the signed-off-by lines (I assume that is > > legit; if not, my apologies!). It is also needed for Pi. > > > > The correct process it to apply the patch locally as originally, > authored then append your SoB and any notes/changes using git commit > --amend. Git am can apply the patch from an mbox, or the patches > utility can be used to download and apply patches off the list. Thanks Peter. I'll do this when I redo the patch series with any other feedback. Andrew ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH 2/3] sdhci: don't raise a command index error for an unexpected response 2015-12-16 19:47 [Qemu-devel] [PATCH 0/3] sdhci patches to enable Raspberry Pi Andrew Baumann 2015-12-16 19:47 ` [Qemu-devel] [PATCH 1/3] sd: sdhci: Delete over-zealous power check Andrew Baumann @ 2015-12-16 19:47 ` Andrew Baumann 2015-12-20 21:44 ` Peter Crosthwaite 2015-12-16 19:47 ` [Qemu-devel] [PATCH 3/3] sdhci: add optional quirk property to disable card insertion/removal interrupts Andrew Baumann 2 siblings, 1 reply; 8+ messages in thread From: Andrew Baumann @ 2015-12-16 19:47 UTC (permalink / raw) To: qemu-devel Cc: Igor Mitsyanko, Peter Maydell, Peter Crosthwaite, Andrew Baumann, Stefan Hajnoczi This deletes a block of code that raised a command index error if a command returned response data, but the guest did not set the appropriate bits in the response register to handle such a response. I cannot find any documentation that suggests the controller should behave in this way, the error code doesn't make sense (command index error is defined for the case where the index in a response does not match that of the issued command), and in at least one case (CMD23 issued by UEFI on Raspberry Pi 2), actual hardware does not do this. Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com> --- hw/sd/sdhci.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c index bc39d48..dd83e89 100644 --- a/hw/sd/sdhci.c +++ b/hw/sd/sdhci.c @@ -243,9 +243,6 @@ static void sdhci_send_command(SDHCIState *s) (s->cmdreg & SDHC_CMD_RESPONSE) == SDHC_CMD_RSP_WITH_BUSY) { s->norintsts |= SDHC_NIS_TRSCMP; } - } else if (rlen != 0 && (s->errintstsen & SDHC_EISEN_CMDIDX)) { - s->errintsts |= SDHC_EIS_CMDIDX; - s->norintsts |= SDHC_NIS_ERR; } if (s->norintstsen & SDHC_NISEN_CMDCMP) { -- 2.5.3 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] sdhci: don't raise a command index error for an unexpected response 2015-12-16 19:47 ` [Qemu-devel] [PATCH 2/3] sdhci: don't raise a command index error for an unexpected response Andrew Baumann @ 2015-12-20 21:44 ` Peter Crosthwaite 0 siblings, 0 replies; 8+ messages in thread From: Peter Crosthwaite @ 2015-12-20 21:44 UTC (permalink / raw) To: Andrew Baumann Cc: Igor Mitsyanko, Peter Maydell, qemu-devel@nongnu.org Developers, Stefan Hajnoczi On Wed, Dec 16, 2015 at 11:47 AM, Andrew Baumann <Andrew.Baumann@microsoft.com> wrote: > This deletes a block of code that raised a command index error if a > command returned response data, but the guest did not set the > appropriate bits in the response register to handle such a response. I > cannot find any documentation that suggests the controller should > behave in this way, the error code doesn't make sense (command index > error is defined for the case where the index in a response does not > match that of the issued command), and in at least one case (CMD23 > issued by UEFI on Raspberry Pi 2), actual hardware does not do this. > > Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com> Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com> All I can think of, is the original author is assuming that any mismatch in response length can only occur on a mismatch of commands. If we really need _CMDIDX it should be done directly as a check of the command index in the response. Regards, Peter > --- > hw/sd/sdhci.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c > index bc39d48..dd83e89 100644 > --- a/hw/sd/sdhci.c > +++ b/hw/sd/sdhci.c > @@ -243,9 +243,6 @@ static void sdhci_send_command(SDHCIState *s) > (s->cmdreg & SDHC_CMD_RESPONSE) == SDHC_CMD_RSP_WITH_BUSY) { > s->norintsts |= SDHC_NIS_TRSCMP; > } > - } else if (rlen != 0 && (s->errintstsen & SDHC_EISEN_CMDIDX)) { > - s->errintsts |= SDHC_EIS_CMDIDX; > - s->norintsts |= SDHC_NIS_ERR; > } > > if (s->norintstsen & SDHC_NISEN_CMDCMP) { > -- > 2.5.3 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH 3/3] sdhci: add optional quirk property to disable card insertion/removal interrupts 2015-12-16 19:47 [Qemu-devel] [PATCH 0/3] sdhci patches to enable Raspberry Pi Andrew Baumann 2015-12-16 19:47 ` [Qemu-devel] [PATCH 1/3] sd: sdhci: Delete over-zealous power check Andrew Baumann 2015-12-16 19:47 ` [Qemu-devel] [PATCH 2/3] sdhci: don't raise a command index error for an unexpected response Andrew Baumann @ 2015-12-16 19:47 ` Andrew Baumann 2015-12-20 22:34 ` Peter Crosthwaite 2 siblings, 1 reply; 8+ messages in thread From: Andrew Baumann @ 2015-12-16 19:47 UTC (permalink / raw) To: qemu-devel Cc: Igor Mitsyanko, Peter Maydell, Peter Crosthwaite, Andrew Baumann, Stefan Hajnoczi This is needed for a quirk of the Raspberry Pi (bcm2835/6) MMC controller, where the card insert bit is documented as unimplemented (always reads zero, doesn't generate interrupts) but is in fact observed on hardware as set at power on, but is cleared (and remains clear) on subsequent controller resets. Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com> --- hw/sd/sdhci.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c index dd83e89..61f919b 100644 --- a/hw/sd/sdhci.c +++ b/hw/sd/sdhci.c @@ -193,7 +193,9 @@ static void sdhci_reset(SDHCIState *s) * initialization */ memset(&s->sdmasysad, 0, (uintptr_t)&s->capareg - (uintptr_t)&s->sdmasysad); - sd_set_cb(s->card, s->ro_cb, s->eject_cb); + if (!s->noeject_quirk) { + sd_set_cb(s->card, s->ro_cb, s->eject_cb); + } s->data_count = 0; s->stopped_state = sdhc_not_stopped; } @@ -1208,6 +1210,7 @@ const VMStateDescription sdhci_vmstate = { VMSTATE_UINT16(data_count, SDHCIState), VMSTATE_UINT64(admasysaddr, SDHCIState), VMSTATE_UINT8(stopped_state, SDHCIState), + VMSTATE_BOOL(noeject_quirk, SDHCIState), VMSTATE_VBUFFER_UINT32(fifo_buffer, SDHCIState, 1, NULL, 0, buf_maxsz), VMSTATE_TIMER_PTR(insert_timer, SDHCIState), VMSTATE_TIMER_PTR(transfer_timer, SDHCIState), @@ -1276,6 +1279,7 @@ static Property sdhci_sysbus_properties[] = { DEFINE_PROP_UINT32("capareg", SDHCIState, capareg, SDHC_CAPAB_REG_DEFAULT), DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0), + DEFINE_PROP_BOOL("noeject-quirk", SDHCIState, noeject_quirk, false), DEFINE_PROP_END_OF_LIST(), }; -- 2.5.3 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] sdhci: add optional quirk property to disable card insertion/removal interrupts 2015-12-16 19:47 ` [Qemu-devel] [PATCH 3/3] sdhci: add optional quirk property to disable card insertion/removal interrupts Andrew Baumann @ 2015-12-20 22:34 ` Peter Crosthwaite 0 siblings, 0 replies; 8+ messages in thread From: Peter Crosthwaite @ 2015-12-20 22:34 UTC (permalink / raw) To: Andrew Baumann Cc: Igor Mitsyanko, Peter Maydell, qemu-devel@nongnu.org Developers, Stefan Hajnoczi On Wed, Dec 16, 2015 at 11:47 AM, Andrew Baumann <Andrew.Baumann@microsoft.com> wrote: > This is needed for a quirk of the Raspberry Pi (bcm2835/6) MMC > controller, where the card insert bit is documented as unimplemented > (always reads zero, doesn't generate interrupts) but is in fact > observed on hardware as set at power on, but is cleared (and remains > clear) on subsequent controller resets. > Yep. Thats what the doc is saying. FWIW , that set-on-reset behaviour makes me very suspicious that this is the SoC layer or pin mux tying off this pin incorrectly. A bug in the pin-mux would cause exactly this symptom. Unfortunately we (or at least I) don't have access to Arasan's doc to tell if this this is SoC errata or SDHCI errata and the Broadcom doc quite unhelpfully refers the reader to: sd3.0_host_ahb_emmc4.4_usersguide_ver5.9_jan11_10.pdf Which doesn't google. > Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com> Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com> > --- > hw/sd/sdhci.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c > index dd83e89..61f919b 100644 > --- a/hw/sd/sdhci.c > +++ b/hw/sd/sdhci.c > @@ -193,7 +193,9 @@ static void sdhci_reset(SDHCIState *s) > * initialization */ > memset(&s->sdmasysad, 0, (uintptr_t)&s->capareg - (uintptr_t)&s->sdmasysad); > > - sd_set_cb(s->card, s->ro_cb, s->eject_cb); > + if (!s->noeject_quirk) { > + sd_set_cb(s->card, s->ro_cb, s->eject_cb); > + } This will conflict with PMMs SD busification work but the resolution should be reasonably straightforward. The Busifed SDHCI can chose to ignore the inserted cb from the bus instead of the card. Regards, Peter > s->data_count = 0; > s->stopped_state = sdhc_not_stopped; > } > @@ -1208,6 +1210,7 @@ const VMStateDescription sdhci_vmstate = { > VMSTATE_UINT16(data_count, SDHCIState), > VMSTATE_UINT64(admasysaddr, SDHCIState), > VMSTATE_UINT8(stopped_state, SDHCIState), > + VMSTATE_BOOL(noeject_quirk, SDHCIState), > VMSTATE_VBUFFER_UINT32(fifo_buffer, SDHCIState, 1, NULL, 0, buf_maxsz), > VMSTATE_TIMER_PTR(insert_timer, SDHCIState), > VMSTATE_TIMER_PTR(transfer_timer, SDHCIState), > @@ -1276,6 +1279,7 @@ static Property sdhci_sysbus_properties[] = { > DEFINE_PROP_UINT32("capareg", SDHCIState, capareg, > SDHC_CAPAB_REG_DEFAULT), > DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0), > + DEFINE_PROP_BOOL("noeject-quirk", SDHCIState, noeject_quirk, false), > DEFINE_PROP_END_OF_LIST(), > }; > > -- > 2.5.3 > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-12-20 22:34 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-12-16 19:47 [Qemu-devel] [PATCH 0/3] sdhci patches to enable Raspberry Pi Andrew Baumann 2015-12-16 19:47 ` [Qemu-devel] [PATCH 1/3] sd: sdhci: Delete over-zealous power check Andrew Baumann 2015-12-16 20:00 ` Peter Crosthwaite 2015-12-16 20:28 ` Andrew Baumann 2015-12-16 19:47 ` [Qemu-devel] [PATCH 2/3] sdhci: don't raise a command index error for an unexpected response Andrew Baumann 2015-12-20 21:44 ` Peter Crosthwaite 2015-12-16 19:47 ` [Qemu-devel] [PATCH 3/3] sdhci: add optional quirk property to disable card insertion/removal interrupts Andrew Baumann 2015-12-20 22:34 ` Peter Crosthwaite
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.