diff mbox series

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

Message ID 20180622021133.24062-2-ramon.fried@gmail.com
State Superseded
Headers show
Series [U-Boot,v2,1/6] dm: SMEM (Shared memory) uclass | expand

Commit Message

Ramon Fried June 22, 2018, 2:11 a.m. UTC
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@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

Comments

Ramon Fried June 25, 2018, 10:54 p.m. UTC | #1
Hi Simon

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

> Hi Ramon,
>
> On 21 June 2018 at 20:11, Ramon Fried <ramon.fried@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@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@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@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@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
>
Simon Glass June 26, 2018, 3:59 a.m. UTC | #2
Hi Ramon,

On 21 June 2018 at 20:11, Ramon Fried <ramon.fried@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@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@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@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.

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

> +        * @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?
> +
> +       /**
> +        * 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 ?

> +        */
> +       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
>

Regards,
Simon
Philipp Tomsich June 26, 2018, 6:39 a.m. UTC | #3
> On 22 Jun 2018, at 04:11, Ramon Fried <ramon.fried@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@gmail.com>

Reviewed-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>

See below for a requested correction.

> 
> ---
> 
> 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
> 
> 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@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;
> +
> +	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.
> + *
> + * Copyright (c) 2018 Ramon Fried <ramon.fried@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
> +	 * @item:	smem item handle
> +	 * @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);
> +
> +	/**
> +	 * 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);
> +
> +	/**
> +	 * get_free_space() - Set the PWM invert

The “Set the PWM invert” part seems like a mistake here...

> +	 *
> +	 * @host:   the remote processor identifying a partition, or -1
> +	 * @return	free space, -ve on error
> +	 */
> +	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
>
Ramon Fried June 26, 2018, 10:15 a.m. UTC | #4
On Tue, Jun 26, 2018 at 6:59 AM Simon Glass <sjg@chromium.org> wrote:

> Hi Ramon,
>
> On 21 June 2018 at 20:11, Ramon Fried <ramon.fried@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@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@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@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.
>
> > +
> > +       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@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
>
> > +        * @item:       smem item handle
>
> How does one choose this? What does it mean? Can you expand this comment a
> bit?
>
It can be an index (like in QCOM implementation) but honestly, it can be a
hash as well. it's up to the implementation
to decide how to access the items.

>
> > +        * @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?
> > +
> > +       /**
> > +        * 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 ?
>
> > +        */
> > +       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
> >
>
> Regards,
> Simon
>
Ramon Fried June 26, 2018, 2:41 p.m. UTC | #5
On June 26, 2018 9:39:55 AM GMT+03:00, "Dr. Philipp Tomsich" <philipp.tomsich@theobroma-systems.com> wrote:
>
>> On 22 Jun 2018, at 04:11, Ramon Fried <ramon.fried@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@gmail.com>
>
>Reviewed-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>
>See below for a requested correction.
>
>> 
Thanks. Nice catch... 
>> ---
>> 
>> 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
>> 
>> 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@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;
>> +
>> +	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.
>> + *
>> + * Copyright (c) 2018 Ramon Fried <ramon.fried@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
>> +	 * @item:	smem item handle
>> +	 * @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);
>> +
>> +	/**
>> +	 * 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);
>> +
>> +	/**
>> +	 * get_free_space() - Set the PWM invert
>
>The “Set the PWM invert” part seems like a mistake here...
>
>> +	 *
>> +	 * @host:   the remote processor identifying a partition, or -1
>> +	 * @return	free space, -ve on error
>> +	 */
>> +	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
>>
Simon Glass June 26, 2018, 11:18 p.m. UTC | #6
HI Ramon,

On 26 June 2018 at 04:15, Ramon Fried <ramon.fried@gmail.com> wrote:
>
>
> On Tue, Jun 26, 2018 at 6:59 AM Simon Glass <sjg@chromium.org> wrote:
>>
>> Hi Ramon,
>>
>> On 21 June 2018 at 20:11, Ramon Fried <ramon.fried@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@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@chromium.org>
>>
>> A few nits below.
>>
[..]

>> > +/* 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
>>
>> > +        * @item:       smem item handle
>>
>> How does one choose this? What does it mean? Can you expand this comment a
>> bit?
>
> It can be an index (like in QCOM implementation) but honestly, it can be a
> hash as well. it's up to the implementation
> to decide how to access the items.

OK that's fine. The key thing is to document this. Your API at present
is pretty vague which is why I am asking for more comments showing
what is going on. Even a comment at the top of your header file with
an example of usage would be useful.

I just saw your latest v3 patch and it still seems to me that it needs
more details. I had to read the qualcomm driver to try to understand
how the uclass API is supposed to work.

Regards,
Simon
Ramon Fried June 28, 2018, 10:21 a.m. UTC | #7
On Wed, Jun 27, 2018 at 2:18 AM Simon Glass <sjg@chromium.org> wrote:

> HI Ramon,
>
> On 26 June 2018 at 04:15, Ramon Fried <ramon.fried@gmail.com> wrote:
> >
> >
> > On Tue, Jun 26, 2018 at 6:59 AM Simon Glass <sjg@chromium.org> wrote:
> >>
> >> Hi Ramon,
> >>
> >> On 21 June 2018 at 20:11, Ramon Fried <ramon.fried@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@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@chromium.org>
> >>
> >> A few nits below.
> >>
> [..]
>
> >> > +/* 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
> >>
> >> > +        * @item:       smem item handle
> >>
> >> How does one choose this? What does it mean? Can you expand this
> comment a
> >> bit?
> >
> > It can be an index (like in QCOM implementation) but honestly, it can be
> a
> > hash as well. it's up to the implementation
> > to decide how to access the items.
>
> OK that's fine. The key thing is to document this. Your API at present
> is pretty vague which is why I am asking for more comments showing
> what is going on. Even a comment at the top of your header file with
> an example of usage would be useful.
>
> I just saw your latest v3 patch and it still seems to me that it needs
> more details. I had to read the qualcomm driver to try to understand
> how the uclass API is supposed to work.
>
> Thanks for the feedback. I'll look in to it soon.
Thanks,
Ramon.

> Regards,
> Simon
>
diff mbox series

Patch

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@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;
+
+	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.
+ *
+ * Copyright (c) 2018 Ramon Fried <ramon.fried@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
+	 * @item:	smem item handle
+	 * @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);
+
+	/**
+	 * 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);
+
+	/**
+	 * get_free_space() - Set the PWM invert
+	 *
+	 * @host:   the remote processor identifying a partition, or -1
+	 * @return	free space, -ve on error
+	 */
+	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_ */
+