Re: [PATCH 10/11] sandbox: mmc: Support a backing file

[Simon Glass <sjg@chromium.org>]

Hi Sean,

On Sat, 28 Aug 2021 at 00:39, Sean Anderson <seanga2@gmail.com> wrote:
>
> On 8/18/21 11:40 PM, Simon Glass wrote:
> > Provide a way for sandbox MMC to present data from a backing file. This
> > allows a filesystem to be created on the host and easily served via an
> > emulated mmc device.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >   doc/device-tree-bindings/mmc/sandbox,mmc.txt | 18 ++++++++
> >   drivers/mmc/sandbox_mmc.c                    | 46 ++++++++++++++++----
> >   2 files changed, 55 insertions(+), 9 deletions(-)
> >   create mode 100644 doc/device-tree-bindings/mmc/sandbox,mmc.txt
> >
> > diff --git a/doc/device-tree-bindings/mmc/sandbox,mmc.txt b/doc/device-tree-bindings/mmc/sandbox,mmc.txt
> > new file mode 100644
> > index 00000000000..1170bcd6a00
> > --- /dev/null
> > +++ b/doc/device-tree-bindings/mmc/sandbox,mmc.txt
> > @@ -0,0 +1,18 @@
> > +Sandbox MMC
> > +===========
> > +
> > +Required properties:
> > +- compatible : "sandbox,mmc"
> > +
> > +Optional properties:
> > +- filename : Name of backing file, if any. This is mapped into the MMC device
> > +    so can be used to provide a filesystem or other test data
>
> Can we stick this in the command line arguments somehow? For testing
> with different images, I think that editing the device tree is not the
> best way to do things. It also makes it trickier to add automated tests.

We can, of course. We used to have that with SPI flash but we ended up
dropping it as it was unwieldy and disconnected from the devicetree .
The automated tests can work by setting up a file in advance. In fact
we do this with SPI flash and I2C eeprom.

So I think this works much better. See dm_test_spi_flash() for how the
file is set up in a unit test.

>
> > +
> > +
> > +Example
> > +-------
> > +
> > +mmc2 {
> > +     compatible = "sandbox,mmc";
> > +     non-removable;
> > +};
> > diff --git a/drivers/mmc/sandbox_mmc.c b/drivers/mmc/sandbox_mmc.c
> > index 895fbffecfc..1139951c626 100644
> > --- a/drivers/mmc/sandbox_mmc.c
> > +++ b/drivers/mmc/sandbox_mmc.c
> > @@ -9,23 +9,26 @@
> >   #include <errno.h>
> >   #include <fdtdec.h>
> >   #include <log.h>
> > +#include <malloc.h>
> >   #include <mmc.h>
> > +#include <os.h>
> >   #include <asm/test.h>
> >
> >   struct sandbox_mmc_plat {
> >       struct mmc_config cfg;
> >       struct mmc mmc;
> > +     const char *fname;
> >   };
> >
> > -#define MMC_CSIZE 0
> > -#define MMC_CMULT 8 /* 8 because the card is high-capacity */
> > -#define MMC_BL_LEN_SHIFT 10
> > -#define MMC_BL_LEN BIT(MMC_BL_LEN_SHIFT)
> > -#define MMC_CAPACITY (((MMC_CSIZE + 1) << (MMC_CMULT + 2)) \
> > -                   * MMC_BL_LEN) /* 1 MiB */
> > +#define MMC_CMULT            8 /* 8 because the card is high-capacity */
> > +#define MMC_BL_LEN_SHIFT     10
> > +#define MMC_BL_LEN           BIT(MMC_BL_LEN_SHIFT)
> > +#define SIZE_MULTIPLE                ((1 << (MMC_CMULT + 2)) * MMC_BL_LEN)
> >
> >   struct sandbox_mmc_priv {
> > -     u8 buf[MMC_CAPACITY];
> > +     int csize;      /* CSIZE value to report */
> > +     char *buf;
> > +     int size;
>
> nit: to save a whole 2 bytes on 64-bit you can reorder this like
>
> char *
> int
> int

OK will do.


>
> >   };
> >
> >   /**
> > @@ -60,8 +63,8 @@ static int sandbox_mmc_send_cmd(struct udevice *dev, struct mmc_cmd *cmd,
> >       case MMC_CMD_SEND_CSD:
> >               cmd->response[0] = 0;
> >               cmd->response[1] = (MMC_BL_LEN_SHIFT << 16) |
> > -                                ((MMC_CSIZE >> 16) & 0x3f);
> > -             cmd->response[2] = (MMC_CSIZE & 0xffff) << 16;
> > +                                ((priv->csize >> 16) & 0x3f);
> > +             cmd->response[2] = (priv->csize & 0xffff) << 16;
> >               cmd->response[3] = 0;
> >               break;
> >       case SD_CMD_SWITCH_FUNC: {
> > @@ -143,6 +146,8 @@ static int sandbox_mmc_of_to_plat(struct udevice *dev)
> >       struct blk_desc *blk;
> >       int ret;
> >
> > +     plat->fname = dev_read_string(dev, "filename");
> > +
> >       ret = mmc_of_parse(dev, cfg);
> >       if (ret)
> >               return ret;
> > @@ -156,6 +161,29 @@ static int sandbox_mmc_of_to_plat(struct udevice *dev)
> >   static int sandbox_mmc_probe(struct udevice *dev)
> >   {
> >       struct sandbox_mmc_plat *plat = dev_get_plat(dev);
> > +     struct sandbox_mmc_priv *priv = dev_get_priv(dev);
> > +     int ret;
> > +
> > +     if (plat->fname) {
> > +             ret = os_map_file(plat->fname, OS_O_RDWR | OS_O_CREAT,
> > +                               (void **)&priv->buf, &priv->size);
> > +             if (ret) {
> > +                     log_err("%s: Unable to map file '%s'\n", dev->name,
> > +                             plat->fname);
> > +                     return ret;
> > +             }
> > +             priv->csize = priv->size / SIZE_MULTIPLE - 1;
>
> Won't this cut off the end of the file? If we can't avoid this, we
> should at least warn the user that we are truncating it.

Yes it does and in fact the granularity is 1MB, I think. I can add a warning.

>
> --Sean
>
> > +     } else {
> > +             priv->csize = 0;
> > +             priv->size = (priv->csize + 1) * SIZE_MULTIPLE; /* 1 MiB */
> > +
> > +             priv->buf = malloc(priv->size);
> > +             if (!priv->buf) {
> > +                     log_err("%s: Not enough memory (%x bytes)\n",
> > +                             dev->name, priv->size);
> > +                     return -ENOMEM;
> > +             }
> > +     }
> >
> >       return mmc_init(&plat->mmc);
> >   }
> >
>

Regards,
Simon