Re: [PATCH 2/2] soc: qcom: Add Shared Memory Manager driver

[Bjorn Andersson <bjorn.andersson@sonymobile.com>]

On Fri 10 Apr 14:30 PDT 2015, Andy Gross wrote:

> On Fri, Apr 03, 2015 at 04:03:20PM -0700, Bjorn Andersson wrote:
> <snip>
> 
> > +static int qcom_smem_alloc_private(struct qcom_smem *smem,
> > +				   unsigned host,
> > +				   unsigned item,
> > +				   size_t size)
> > +{
> 
> <snip>
> 
> > +	alloc_size = sizeof(*hdr) + ALIGN(size, 8);
> > +	if (p + alloc_size >= (void *)phdr + phdr->offset_free_uncached) {
> > +		dev_err(smem->dev, "Out of memory\n");
> > +		return -ENOSPC;
> > +	}
> 
> This check always fails due to the fact that we always get a ptr that points to
> something beyond the free_uncached area.  We ought to use:
> alloc_size > phdr->offset_free_cached - phdr->offset_free_uncached
> 

Right, that's a typo on my part. I meant to compare with phdr +
offset_free_cached. Either way deserves a comment regarding the uncached
area growing from the start and cached from the end of the partition...

> > +
> > +	hdr = p;
> > +	hdr->canary = SMEM_PRIVATE_CANARY;
> > +	hdr->item = item;
> > +	hdr->size = ALIGN(size, 8);
> > +	hdr->padding_data = hdr->size - size;
> > +	hdr->padding_hdr = 0;
> > +
> 
> <snip>
> 
> > +static int qcom_smem_probe(struct platform_device *pdev)
> > +{
> 
> <snip>
> 
> > +	ret = of_address_to_resource(np, 0, &r);
> > +	of_node_put(np);
> > +	if (ret)
> > +		return ret;
> > +
> > +	smem->regions[0].aux_base = (u32)r.start;
> > +	smem->regions[0].size = resource_size(&r);
> > +	smem->regions[0].virt_base = devm_ioremap(&pdev->dev,
> > +						  r.start,
> > +						  resource_size(&r));
> 
> Need to use devm_ioremap_nocache() instead.  We need uncached accesses.
> 

On both arm and arm64 these are equivalent. So while we gain a grain
of clarity we're busting the 80 char limit. Or am I missing something?

> > +	if (!smem->regions[0].virt_base)
> > +		return -ENOMEM;
> > +
> > +	for (i = 1; i < num_regions; i++) {
> > +		res = platform_get_resource(pdev, IORESOURCE_MEM, i - 1);
> > +
> > +		smem->regions[i].aux_base = (u32)res->start;
> > +		smem->regions[i].size = resource_size(res);
> > +		smem->regions[i].virt_base = devm_ioremap(&pdev->dev,
> > +							  res->start,
> > +							  resource_size(res));
> 
> Same thing here.
> 
> > +		if (!smem->regions[i].virt_base)
> > +			return -ENOMEM;
> > +	}
> > +
> 
> <snip>
> 
> > diff --git a/include/linux/soc/qcom/smem.h b/include/linux/soc/qcom/smem.h
> > new file mode 100644
> > index 0000000..294070de
> > --- /dev/null
> > +++ b/include/linux/soc/qcom/smem.h
> > @@ -0,0 +1,14 @@
> > +#ifndef __QCOM_SMEM_H__
> > +#define __QCOM_SMEM_H__
> > +
> > +struct device_node;
> > +struct qcom_smem;
> > +
> > +#define QCOM_SMEM_HOST_ANY -1
> 
> Would it make sense to throw in the remote processor enumeration?  Same with the
> fixed/dynamic item list?
> 

I presume you mean a list of defines like:
#define QCOM_SMEM_HOST_WCNSS 6

In all cases I've hit so far (smd, smp2p, rproc-tz) this is instance
data that I (and caf) believe better come from DT. So such defines would
be beneficial to have available as dt-binding.

Both smd and smp2p are somewhat related to smem, but rproc-tz is not. So
I'm not sure that smem is the place to provide these defines.


The list of smem items defined in smem.h is a mishmash of legacy and
modern items. Several items have changed meaning and others have not
been used since msm7200...

Again, some of these numbers are used for instantiating e.g. smp2p
without hard coding things in the driver so some of them might be useful
in dt-bindings.

So for now I don't think we should add either of them.

> > +
> > +int qcom_smem_alloc(unsigned host, unsigned item, size_t size);
> > +int qcom_smem_get(unsigned host, unsigned item, void **ptr, size_t *size);
> > +
> > +int qcom_smem_get_free_space(unsigned host);

Thanks for the review.

Regards,
Bjorn