Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751980AbbDKE6Q (ORCPT ); Sat, 11 Apr 2015 00:58:16 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:34437 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750906AbbDKE6M (ORCPT ); Sat, 11 Apr 2015 00:58:12 -0400 Message-ID: <58a6bdd12fe932b690fe2763a4ea0e30.squirrel@www.codeaurora.org> In-Reply-To: <20150410235542.GB12607@sonymobile.com> References: <1428102200-5948-1-git-send-email-bjorn.andersson@sonymobile.com> <1428102200-5948-2-git-send-email-bjorn.andersson@sonymobile.com> <20150410213000.GA11763@qualcomm.com> <20150410235542.GB12607@sonymobile.com> Date: Fri, 10 Apr 2015 23:58:11 -0500 Subject: Re: [PATCH 2/2] soc: qcom: Add Shared Memory Manager driver From: "Andy Gross" To: "Bjorn Andersson" Cc: "Andy Gross" , "Kumar Gala" , "David Brown" , "Jeffrey Hugo" , "linux-kernel@vger.kernel.org" , "linux-arm-msm@vger.kernel.org" , "linux-soc@vger.kernel.org" User-Agent: SquirrelMail/1.4.22-4.el6 MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7BIT X-Priority: 3 (Normal) Importance: Normal Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4570 Lines: 158 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: >> >> >> > +static int qcom_smem_alloc_private(struct qcom_smem *smem, >> > + unsigned host, >> > + unsigned item, >> > + size_t size) >> > +{ >> >> >> >> > + 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; >> > + >> >> >> >> > +static int qcom_smem_probe(struct platform_device *pdev) >> > +{ >> >> >> >> > + 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; >> > + } >> > + >> >> >> >> > 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-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/