Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 30 May 2023 14:39:31 -0700
From:   Bjorn Andersson <quic_bjorande@quicinc.com>
To:     Stephan Gerhold <stephan@gerhold.net>
CC:     Bjorn Andersson <andersson@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        <linux-arm-msm@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/2] soc: qcom: rmtfs: Support dynamic placement of region
Message-ID: <20230530213931.GB3645274@hu-bjorande-lv.qualcomm.com>
References: <20230530193436.3833889-1-quic_bjorande@quicinc.com>
 <20230530193436.3833889-3-quic_bjorande@quicinc.com>
 <ZHZTHlfDsngUrTRX@gerhold.net>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <ZHZTHlfDsngUrTRX@gerhold.net>
Precedence: bulk

On Tue, May 30, 2023 at 09:48:46PM +0200, Stephan Gerhold wrote:
> On Tue, May 30, 2023 at 12:34:36PM -0700, Bjorn Andersson wrote:
> > In some configurations, the exact placement of the rmtfs shared memory
> > region isn't so strict. In the current implementation the author of the
> > DeviceTree source is forced to make up a memory region.
> > 
> > Extend the rmtfs memory driver to relieve the author of this
> > responsibility by introducing support for using dynamic allocation in
> > the driver.
> > 
> > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> > ---
> >  arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 10 ++++
> >  drivers/soc/qcom/rmtfs_mem.c            | 66 +++++++++++++++++++------
> >  2 files changed, 61 insertions(+), 15 deletions(-)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> > index d1440b790fa6..e6191b8ba4c6 100644
> > --- a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> > +++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> > @@ -12,6 +12,8 @@
> >  #include "pm8998.dtsi"
> >  #include "pmi8998.dtsi"
> >  
> > +/delete-node/ &rmtfs_mem;
> > +
> >  / {
> >  	model = "Qualcomm Technologies, Inc. SDM845 MTP";
> >  	compatible = "qcom,sdm845-mtp", "qcom,sdm845";
> > @@ -48,6 +50,14 @@ vreg_s4a_1p8: pm8998-smps4 {
> >  		vin-supply = <&vph_pwr>;
> >  	};
> >  
> > +	rmtfs {
> > +		compatible = "qcom,rmtfs-mem";
> > +
> > +		qcom,alloc-size = <(2*1024*1024)>;
> > +		qcom,client-id = <1>;
> > +		qcom,vmid = <15>;
> > +	};
> > +
> 
> Couldn't you just use the existing dynamic allocation of
> reserved-memory, without any driver changes?
> 

That should give us a similar end result, but we have alloc-ranges as
well, if the placement needs to be further refined...

> / {
> 	reserved-memory {
> 		rmtfs {
> 			compatible = "qcom,rmtfs-mem";
> 			size = <0x0 (2*1024*1024)>;
> 			alignment = <0x0 ...>; // if you want a special one
> 			no-map; // don't we want to map this actually?
> 
> 			qcom,client-id = <1>;
> 			qcom,vmid = <15>;
> 		};
> 	};
> };
> 
> You won't get the 4K empty pages but I guess you just have them because
> you allocate the memory without proper alignment?
> 

With dynamic placement there's no guarantee that the region isn't
physically adjacent to another protected region, so this needs to be
handled somehow.

Perhaps the intention to include guard pages can be derived from the
size...

> Related patch series where I propose using it for most firmware memory
> regions:
> https://lore.kernel.org/linux-arm-msm/20230510-dt-resv-bottom-up-v1-5-3bf68873dbed@gerhold.net/
> 

Thanks for the suggestion,
Bjorn

> Thanks,
> Stephan