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
--
To unsubscribe from this list: send the line "unsubscribe linux-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
