Received-SPF: pass (google.com: domain of linux-kernel+bounces-59096-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Fri, 9 Feb 2024 13:27:16 +0530
From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
To: Krishna Chaitanya Chundru <quic_krichai@quicinc.com>
Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
	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,
	quic_vbadigan@quicinc.com, quic_ramkri@quicinc.com,
	quic_nitegupt@quicinc.com, quic_skananth@quicinc.com,
	quic_parass@quicinc.com
Subject: Re: [PATCH] arm64: dts: qcom: qcs6490-rb3gen2: Add PCIe nodes
Message-ID: <20240209075716.GA12035@thinkpad>
References: <20240207-enable_pcie-v1-1-b684afa6371c@quicinc.com>
 <CAA8EJpqjm_2aE+7BtMkFUdet11q7v_jyHbUEpiDHSBSnzhndYA@mail.gmail.com>
 <dec2976e-6e1e-6121-e175-210377ff6925@quicinc.com>
 <CAA8EJprsm5Tw=vFpmfEKL8fxS-S+aW+YR0byfyL=v78k75TGEw@mail.gmail.com>
 <3ad77846-b4a8-80ee-e9e1-d5cbf4add6d8@quicinc.com>
 <CAA8EJprRF0tVFZK9c=MT8bSRcBdRvcugBaeEzpX5-wfRyNgc3Q@mail.gmail.com>
 <c8be2bbf-a51c-a38f-6e6f-a88801f953d5@quicinc.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <c8be2bbf-a51c-a38f-6e6f-a88801f953d5@quicinc.com>

On Fri, Feb 09, 2024 at 12:58:15PM +0530, Krishna Chaitanya Chundru wrote:
> 
> 
> On 2/8/2024 8:49 PM, Dmitry Baryshkov wrote:
> > On Thu, 8 Feb 2024 at 16:58, Krishna Chaitanya Chundru
> > <quic_krichai@quicinc.com> wrote:
> > > On 2/8/2024 12:21 PM, Dmitry Baryshkov wrote:
> > > > On Thu, 8 Feb 2024 at 08:14, Krishna Chaitanya Chundru
> > > > <quic_krichai@quicinc.com> wrote:
> > > > > 
> > > > > 
> > > > > 
> > > > > On 2/7/2024 5:17 PM, Dmitry Baryshkov wrote:
> > > > > > On Wed, 7 Feb 2024 at 12:42, Krishna chaitanya chundru
> > > > > > <quic_krichai@quicinc.com> wrote:
> > > > > > > 
> > > > > > > Enable PCIe1 controller and its corresponding PHY nodes on
> > > > > > > qcs6490-rb3g2 platform.
> > > > > > > 
> > > > > > > PCIe switch is connected to PCIe1, PCIe switch has multiple endpoints
> > > > > > > connected. For each endpoint a unique BDF will be assigned and should
> > > > > > > assign unique smmu id. So for each BDF add smmu id.
> > > > > > > 
> > > > > > > Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> > > > > > > ---
> > > > > > >     arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts | 42 ++++++++++++++++++++++++++++
> > > > > > >     1 file changed, 42 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> > > > > > > index 8bb7d13d85f6..0082a3399453 100644
> > > > > > > --- a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> > > > > > > +++ b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> > > > > > > @@ -413,6 +413,32 @@ vreg_bob_3p296: bob {
> > > > > > >            };
> > > > > > >     };
> > > > > > > 
> > > > > > > +&pcie1 {
> > > > > > > +       perst-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>;
> > > > > > > +
> > > > > > > +       pinctrl-0 = <&pcie1_reset_n>, <&pcie1_wake_n>;
> > > > > > > +       pinctrl-names = "default";
> > > > > > > +
> > > > > > > +       iommu-map = <0x0 &apps_smmu 0x1c80 0x1>,
> > > > > > > +                   <0x100 &apps_smmu 0x1c81 0x1>,
> > > > > > > +                   <0x208 &apps_smmu 0x1c84 0x1>,
> > > > > > > +                   <0x210 &apps_smmu 0x1c85 0x1>,
> > > > > > > +                   <0x218 &apps_smmu 0x1c86 0x1>,
> > > > > > > +                   <0x300 &apps_smmu 0x1c87 0x1>,
> > > > > > > +                   <0x400 &apps_smmu 0x1c88 0x1>,
> > > > > > > +                   <0x500 &apps_smmu 0x1c89 0x1>,
> > > > > > > +                   <0x501 &apps_smmu 0x1c90 0x1>;
> > > > > > 
> > > > > > Is the iommu-map really board specific?
> > > > > > 
> > > > > The iommu-map for PCIe varies if PCIe switch is connected.
> > > > > For this platform a PCIe switch is connected and for that reason
> > > > > we need to define additional smmu ID's for each BDF.
> > > > > 
> > > > > For that reason we defined here as these ID's are applicable only
> > > > > for this board.
> > > > 
> > > > So, these IDs are the same for all boards, just being unused on
> > > > devices which have no bridges / switches connected to this PCIe host.
> > > > If this is correct, please move them to sc7280.dtsi.
> > > > 
> > > Yes ID's will be same for all boards. we can move them sc7280.dtsi
> > > but the BDF to smmu mapping will be specific to this board only.
> > > if there is some other PCIe switch with different configuration is
> > > connected to different board of same variant in future again these
> > > mapping needs to updated.
> > 
> > Could you possibly clarify this? Are they assigned one at a time
> > manually? Or is it somehow handled by the board's TZ code, which
> > assigns them sequentially to the known endpoints? And is it done via
> > probing the link or via some static configuration?
> 
> There is no assignment of SID's in TZ for PCIe.
> PCIe controller has BDF to SID mapping table which we need to
> program with the iommu map table.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-qcom.c?h=v6.8-rc3#n997
> 
> Based upon switch the BDF to SID table will change for example I had two
> switches with one switch has 2 PCIe ports and other has 3 ports one
> embedded port which supports multiple functions.
> 
> For the first switch the BDF's are
> 	- 0x000(root complex),
> 	- 0x100(USP),
> 	- 0x208(DSP 0),
> 	- 0x210(DSP 1),
> 	- 0x300(endpoint connected to DSP 0),
> 	- 0x400( endpoint connected to DSP 1).
> 
> For 2nd switch the BDF's are
> 	- 0x000(root complex),
> 	- 0x100(USP),
> 	- 0x208(embeeded DSP 0),
> 	- 0x210(DSP 1),
> 	- 0x218 (DSP 2),
> 	- 0x300(embedded endpoint function 0),
> 	- 0x301 (embedded endpoint function 1)
> 	- 0x400( endpoint connected to DSP 1)
> 	- 0x500(endpoint connected to DSP2).
> 
> For these two switches we need different BDF to SID table so for that
> reason we are keeping iommu map here as this is specific to this board.
> 

I don't understand why the SID table has to change between PCIe devices. The SID
mapping should be part of the SoC dtsi, where a single SID would be defined for
the devices under a bus. And all the devices under the bus have to use the same
SID.

Perhaps you are missing iommu-map-mask?

- Mani

-- 
மணிவண்ணன் சதாசிவம்