[Qemu-devel] [PATCH v1 0/2] i.MX25 SD support

[Qemu-devel] [PATCH v1 0/2] i.MX25 SD support

[Peter Crosthwaite]

Hi Jean Christophe,

This patch series adds support for SD on i.MX25. This is usable for the
PDK board.

P1 is a bugfix in SD that is needed. P2 adds the SD controller to the
SoC.

Regards,
Peter


Peter Crosthwaite (2):
  sd: sdhci: Delete over-zealous power check
  arm: fsl-imx25: Add SD support

 hw/arm/fsl-imx25.c         | 26 ++++++++++++++++++++++++++
 hw/sd/sdhci.c              |  2 +-
 include/hw/arm/fsl-imx25.h |  9 +++++++++
 3 files changed, 36 insertions(+), 1 deletion(-)

-- 
1.9.1

[Qemu-devel] [PATCH v1 1/2] sd: sdhci: Delete over-zealous power check

[Peter Crosthwaite]

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

 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 d70d1a6..ed536ee 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 &&
-- 
1.9.1

Re: [Qemu-devel] [PATCH v1 2/2] arm: fsl-imx25: Add SD support

[Jean-Christophe DUBOIS]

Hello Peter,

Le 28/11/2015 21:50, Peter Crosthwaite a écrit :
> Add the two SD card controllers as the generic sysbus SDHCI device.
> Tested as sucessfully working with Linux 4.2. using the SD card for the
> root file system.

I guess the SDHCI device present in Qemu is (surprisingly) close enough 
to the i.MX version so that it works with a "generic" guest driver not 
using all device features which is nice.

But I am afraid the Freescale version is not 100% compatible. To me it 
looks like some registers are different or not supported in i.MX25.

I guess we could "fix it" later when we meet some different guest driver.

JC

>
> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> ---
>
>   hw/arm/fsl-imx25.c         | 26 ++++++++++++++++++++++++++
>   include/hw/arm/fsl-imx25.h |  9 +++++++++
>   2 files changed, 35 insertions(+)
>
> diff --git a/hw/arm/fsl-imx25.c b/hw/arm/fsl-imx25.c
> index e1cadac..9ccbd03 100644
> --- a/hw/arm/fsl-imx25.c
> +++ b/hw/arm/fsl-imx25.c
> @@ -68,6 +68,11 @@ static void fsl_imx25_init(Object *obj)
>           object_initialize(&s->gpio[i], sizeof(s->gpio[i]), TYPE_IMX_GPIO);
>           qdev_set_parent_bus(DEVICE(&s->gpio[i]), sysbus_get_default());
>       }
> +
> +    for (i = 0; i < FSL_IMX25_NUM_ESDHCS; i++) {
> +        object_initialize(&s->esdhc[i], sizeof(s->esdhc[i]), TYPE_SYSBUS_SDHCI);
> +        qdev_set_parent_bus(DEVICE(&s->esdhc[i]), sysbus_get_default());
> +    }
>   }
>   
>   static void fsl_imx25_realize(DeviceState *dev, Error **errp)
> @@ -243,6 +248,27 @@ static void fsl_imx25_realize(DeviceState *dev, Error **errp)
>                                               gpio_table[i].irq));
>       }
>   
> +    /* Initialize all eSDHCs */
> +    for (i = 0; i < FSL_IMX25_NUM_ESDHCS; i++) {
> +        static const struct {
> +            hwaddr addr;
> +            unsigned int irq;
> +        } esdhc_table[FSL_IMX25_NUM_ESDHCS] = {
> +            { FSL_IMX25_ESDHC1_ADDR, FSL_IMX25_ESDHC1_IRQ },
> +            { FSL_IMX25_ESDHC2_ADDR, FSL_IMX25_ESDHC2_IRQ }
> +        };
> +
> +        object_property_set_bool(OBJECT(&s->esdhc[i]), true, "realized", &err);
> +        if (err) {
> +            error_propagate(errp, err);
> +            return;
> +        }
> +        sysbus_mmio_map(SYS_BUS_DEVICE(&s->esdhc[i]), 0, esdhc_table[i].addr);
> +        sysbus_connect_irq(SYS_BUS_DEVICE(&s->esdhc[i]), 0,
> +                           qdev_get_gpio_in(DEVICE(&s->avic),
> +                                            esdhc_table[i].irq));
> +    }
> +
>       /* initialize 2 x 16 KB ROM */
>       memory_region_init_rom_device(&s->rom[0], NULL, NULL, NULL,
>                                     "imx25.rom0", FSL_IMX25_ROM0_SIZE, &err);
> diff --git a/include/hw/arm/fsl-imx25.h b/include/hw/arm/fsl-imx25.h
> index 73f50c6..ade4d42 100644
> --- a/include/hw/arm/fsl-imx25.h
> +++ b/include/hw/arm/fsl-imx25.h
> @@ -26,6 +26,7 @@
>   #include "hw/net/imx_fec.h"
>   #include "hw/i2c/imx_i2c.h"
>   #include "hw/gpio/imx_gpio.h"
> +#include "hw/sd/sdhci.h"
>   #include "exec/memory.h"
>   
>   #define TYPE_FSL_IMX25 "fsl,imx25"
> @@ -36,6 +37,7 @@
>   #define FSL_IMX25_NUM_EPITS 2
>   #define FSL_IMX25_NUM_I2CS 3
>   #define FSL_IMX25_NUM_GPIOS 4
> +#define FSL_IMX25_NUM_ESDHCS 2
>   
>   typedef struct FslIMX25State {
>       /*< private >*/
> @@ -51,6 +53,7 @@ typedef struct FslIMX25State {
>       IMXFECState    fec;
>       IMXI2CState    i2c[FSL_IMX25_NUM_I2CS];
>       IMXGPIOState   gpio[FSL_IMX25_NUM_GPIOS];
> +    SDHCIState     esdhc[2];
>       MemoryRegion   rom[2];
>       MemoryRegion   iram;
>       MemoryRegion   iram_alias;
> @@ -211,6 +214,10 @@ typedef struct FslIMX25State {
>   #define FSL_IMX25_GPIO4_SIZE    0x4000
>   #define FSL_IMX25_GPIO3_ADDR    0x53FA4000
>   #define FSL_IMX25_GPIO3_SIZE    0x4000
> +#define FSL_IMX25_ESDHC1_ADDR   0x53FB4000
> +#define FSL_IMX25_ESDHC1_SIZE   0x4000
> +#define FSL_IMX25_ESDHC2_ADDR   0x53FBB000
> +#define FSL_IMX25_ESDHC2_SIZE   0x4000
>   #define FSL_IMX25_GPIO1_ADDR    0x53FCC000
>   #define FSL_IMX25_GPIO1_SIZE    0x4000
>   #define FSL_IMX25_GPIO2_ADDR    0x53FD0000
> @@ -245,5 +252,7 @@ typedef struct FslIMX25State {
>   #define FSL_IMX25_GPIO2_IRQ     51
>   #define FSL_IMX25_GPIO3_IRQ     16
>   #define FSL_IMX25_GPIO4_IRQ     23
> +#define FSL_IMX25_ESDHC1_IRQ    9
> +#define FSL_IMX25_ESDHC2_IRQ    8
>   
>   #endif /* FSL_IMX25_H */

Re: [Qemu-devel] [PATCH v1 1/2] sd: sdhci: Delete over-zealous power check

[Sai Pavan Boddu]

> -----Original Message-----
> From: Peter Crosthwaite [mailto:crosthwaitepeter@gmail.com]
> Sent: Sunday, November 29, 2015 2:21 AM
> To: qemu-devel@nongnu.org; jcd@tribudubois.net
> Cc: Sai Pavan Boddu; qemu-block@nongnu.org; Peter Crosthwaite
> Subject: [PATCH v1 1/2] sd: sdhci: Delete over-zealous power check
> 
> 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>
Patch is good with the change.

Reviewed-by: Sai Pavan Boddu <saipava@xilinx.com>

Thanks,
Sai Pavan


> ---
> 
>  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 d70d1a6..ed536ee 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 &&
> --
> 1.9.1

Re: [Qemu-devel] [PATCH v1 1/2] sd: sdhci: Delete over-zealous power check

[Peter Crosthwaite]

CCing Andrew who ran into a similar issue for rPI.

On Sat, Nov 28, 2015 at 12:50 PM, Peter Crosthwaite
<crosthwaitepeter@gmail.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:
>
> http://digilentinc.com/Data/Products/ZYBO/ZYBO_sch_VB.3.pdf
> bottom of page 3.
>
> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> ---
>
>  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 d70d1a6..ed536ee 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 &&
> --
> 1.9.1
>

Re: [Qemu-devel] [PATCH v1 1/2] sd: sdhci: Delete over-zealous power check

[Andrew Baumann]

Raspberry Pi needs this change as well. I don't mind if it's as implemented below, or conditional under a flag property.

Tested-By: Andrew Baumann <Andrew.Baumann@microsoft.com>

Thanks,
Andrew

> From: Peter Crosthwaite [mailto:crosthwaitepeter@gmail.com]
> Sent: Thursday, 10 December 2015 12:55
> 
> CCing Andrew who ran into a similar issue for rPI.
> 
> On Sat, Nov 28, 2015 at 12:50 PM, Peter Crosthwaite
> <crosthwaitepeter@gmail.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:
> >
> > http://digilentinc.com/Data/Products/ZYBO/ZYBO_sch_VB.3.pdf
> > bottom of page 3.
> >
> > Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> > ---
> >
> >  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 d70d1a6..ed536ee 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 &&
> > --
> > 1.9.1
> >