[U-Boot] [PATCH v2 1/6] dm: SMEM (Shared memory) uclass

Ramon Fried ramon.fried at gmail.com
Mon Jun 25 22:54:49 UTC 2018


Hi Simon

On Tue, Jun 26, 2018 at 6:59 AM Simon Glass <sjg at chromium.org> wrote:

> Hi Ramon,
>
> On 21 June 2018 at 20:11, Ramon Fried <ramon.fried at gmail.com> wrote:
> > This is a uclass for Shared memory manager drivers.
> >
> > A Shared Memory Manager driver implements an interface for allocating
> > and accessing items in the memory area shared among all of the
> > processors.
> >
> > Signed-off-by: Ramon Fried <ramon.fried at gmail.com>
> >
> > ---
> >
> > Changes in v2:
> > (As suggested by Simon Glass)
> > - Introduced a new dm class (CLASS_SMEM) instead of CLASS_SOC.
> > - Added sandbox driver
> > - Added testing for DM class.
> >
> >  drivers/Makefile           |  1 +
> >  drivers/smem/Kconfig       |  2 +
> >  drivers/smem/Makefile      |  5 +++
> >  drivers/smem/smem-uclass.c | 53 ++++++++++++++++++++++++
> >  include/dm/uclass-id.h     |  1 +
> >  include/smem.h             | 84 ++++++++++++++++++++++++++++++++++++++
> >  6 files changed, 146 insertions(+)
> >  create mode 100644 drivers/smem/Kconfig
> >  create mode 100644 drivers/smem/Makefile
> >  create mode 100644 drivers/smem/smem-uclass.c
> >  create mode 100644 include/smem.h
> >
>
> Reviewed-by: Simon Glass <sjg at chromium.org>
>
> A few nits below.
>
> > diff --git a/drivers/Makefile b/drivers/Makefile
> > index a213ea9671..ba4a561358 100644
> > --- a/drivers/Makefile
> > +++ b/drivers/Makefile
> > @@ -98,6 +98,7 @@ obj-y += pwm/
> >  obj-y += reset/
> >  obj-y += input/
> >  # SOC specific infrastructure drivers.
> > +obj-y += smem/
> >  obj-y += soc/
> >  obj-$(CONFIG_REMOTEPROC) += remoteproc/
> >  obj-y += thermal/
> > diff --git a/drivers/smem/Kconfig b/drivers/smem/Kconfig
> > new file mode 100644
> > index 0000000000..64337a8b8e
> > --- /dev/null
> > +++ b/drivers/smem/Kconfig
> > @@ -0,0 +1,2 @@
> > +menuconfig SMEM
> > +       bool  "SMEM (Shared Memory mamanger) support"
> > diff --git a/drivers/smem/Makefile b/drivers/smem/Makefile
> > new file mode 100644
> > index 0000000000..ca55c4512d
> > --- /dev/null
> > +++ b/drivers/smem/Makefile
> > @@ -0,0 +1,5 @@
> > +# SPDX-License-Identifier: GPL-2.0+
> > +#
> > +# Makefile for the U-Boot SMEM interface drivers
> > +
> > +obj-$(CONFIG_SMEM) += smem-uclass.o
> > diff --git a/drivers/smem/smem-uclass.c b/drivers/smem/smem-uclass.c
> > new file mode 100644
> > index 0000000000..07ed1a92d8
> > --- /dev/null
> > +++ b/drivers/smem/smem-uclass.c
> > @@ -0,0 +1,53 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (c) 2018 Ramon Fried <ramon.fried at gmail.com>
> > + */
> > +
> > +#include <common.h>
> > +#include <dm.h>
> > +#include <smem.h>
> > +
> > +int smem_alloc(struct udevice *dev, unsigned int host,
> > +               unsigned int item, size_t size)
> > +{
> > +       struct smem_ops *ops = smem_get_ops(dev);
> > +
> > +       if (!ops->alloc)
> > +               return -ENOSYS;
> > +
> > +       return ops->alloc(host, item, size);
> > +}
> > +
> > +void *smem_get(struct udevice *dev, unsigned int host,
> > +               unsigned int item, size_t *size)
> > +{
> > +       struct smem_ops *ops = smem_get_ops(dev);
> > +
> > +       if (!ops->get)
> > +               return NULL;
> > +
> > +       return ops->get(host, item, size);
> > +}
> > +
> > +int smem_get_free_space(struct udevice *dev, unsigned int host, size_t
> *free)
> > +{
> > +       struct smem_ops *ops = smem_get_ops(dev);
> > +
> > +       int ret;
> > +
> > +       if (!ops->get_free_space)
> > +               return -ENOSYS;
> > +
> > +       ret = ops->get_free_space(host);
> > +       if (ret > 0)
> > +               *free = ret;
> > +       else
> > +               return ret;
>
> It seems odd that get_free_space() has a different return value than
> smem_get_free_space(). Can you make the latter return the amount of
> free space? If not, change the op to return it in a param. You can use
> long as the return value if that helps.
>
I fixed smem_get_free_space() to look the same as get_free_space().


> > +
> > +       return 0;
> > +}
> > +
> > +UCLASS_DRIVER(smem) = {
> > +       .id     = UCLASS_SMEM,
> > +       .name       = "smem",
> > +};
> > diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> > index d7f9df3583..a39643ec5e 100644
> > --- a/include/dm/uclass-id.h
> > +++ b/include/dm/uclass-id.h
> > @@ -74,6 +74,7 @@ enum uclass_id {
> >         UCLASS_RTC,             /* Real time clock device */
> >         UCLASS_SCSI,            /* SCSI device */
> >         UCLASS_SERIAL,          /* Serial UART */
> > +       UCLASS_SMEM,            /* Shared memory interface */
> >         UCLASS_SPI,             /* SPI bus */
> >         UCLASS_SPMI,            /* System Power Management Interface bus
> */
> >         UCLASS_SPI_FLASH,       /* SPI flash */
> > diff --git a/include/smem.h b/include/smem.h
> > new file mode 100644
> > index 0000000000..201960232c
> > --- /dev/null
> > +++ b/include/smem.h
> > @@ -0,0 +1,84 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * header file for smem driver.
>
> If you have a README somewhere please point to it here.
>
> > + *
> > + * Copyright (c) 2018 Ramon Fried <ramon.fried at gmail.com>
> > + */
> > +
> > +#ifndef _smemh_
> > +#define _smemh_
> > +
> > +/* struct smem_ops: Operations for the SMEM uclass */
> > +struct smem_ops {
> > +       /**
> > +        * alloc() - allocate space for a smem item
> > +        *
> > +        * @host:       remote processor id, or -1
>
> What does -1 mean? Please expand commment
>
Done.

>
> > +        * @item:       smem item handle
>
> How does one choose this? What does it mean? Can you expand this comment a
> bit?
>
> > +        * @size:       number of bytes to be allocated
> > +        * @return 0 if OK, -ve on error
> > +        */
> > +       int (*alloc)(unsigned int host,
> > +               unsigned int item, size_t size);
>
> Fits on one line?
>
Checkpatch complained, but I agree, this should be one line.

> > +
> > +       /**
> > +        * get() - Resolve ptr of size of a smem item
> > +        *
> > +        * @host:       the remote processor, of -1
> > +        * @item:       smem item handle
> > +        * @size:       pointer to be filled out with the size of the
> item
> > +        * @return      pointer on success, NULL on error
> > +        */
> > +       void *(*get)(unsigned int host,
> > +               unsigned int item, size_t *size);
>
> Fits on one line?
>
> > +
> > +       /**
> > +        * get_free_space() - Set the PWM invert
> > +        *
> > +        * @host:   the remote processor identifying a partition, or -1
> > +        * @return      free space, -ve on error
>
> free space in bytes ?
>
Yes. Fixed.

>
> > +        */
> > +       int (*get_free_space)(unsigned int host);
> > +};
> > +
> > +#define smem_get_ops(dev)      ((struct smem_ops *)(dev)->driver->ops)
> > +
> > +/**
> > + * smem_alloc() - allocate space for a smem item
> > + * @host:      remote processor id, or -1
> > + * @item:      smem item handle
> > + * @size:      number of bytes to be allocated
> > + * @return 0 if OK, -ve on error
> > + *
> > + * Allocate space for a given smem item of size @size, given that the
> item is
> > + * not yet allocated.
> > + */
> > +int smem_alloc(struct udevice *dev, unsigned int host,
> > +               unsigned int item, size_t size);
> > +
> > +/**
> > + * smem_get() - resolve ptr of size of a smem item
> > + * @host:      the remote processor, or -1
> > + * @item:      smem item handle
> > + * @size:      pointer to be filled out with size of the item
> > + * @return     pointer on success, NULL on error
> > + *
> > + * Looks up smem item and returns pointer to it. Size of smem
> > + * item is returned in @size.
> > + */
> > +void *smem_get(struct udevice *dev, unsigned int host,
> > +               unsigned int item, size_t *size);
> > +
> > +/**
> > + * smem_get_free_space() - retrieve amount of free space in a partition
> > + * @host:      the remote processor identifying a partition, or -1
> > + * @free_space:        pointer to be filled out with free space
> > + * @return     0 if OK, -ve on error
> > + *
> > + * To be used by smem clients as a quick way to determine if any new
> > + * allocations has been made.
> > + */
> > +int smem_get_free_space(struct udevice *dev, unsigned int host, size_t
> *free_space);
> > +
> > +#endif /* _smem_h_ */
> > +
> > --
> > 2.17.1
> >
>
Thanks,
Ramon.

>
> Regards,
> Simon
>


More information about the U-Boot mailing list