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

["Andy Gross" <agross@codeaurora.org>]

On Fri, April 10, 2015 6:55 pm, Bjorn Andersson wrote:
> 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...

Right, that would be good to have an explanation.

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

I'd be for clarity.  OTOH,  if they continue to remain equivalent.....

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

Fair enough, and in the end this might be a include/dt-bindings thing anyway.

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


-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation