Message-ID: <CFF89D4D-8131-47C2-95B8-A0E130A16E46@linaro.org>
Date: Tue, 01 Oct 2024 18:08:19 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
CC: Krishna Chaitanya Chundru <quic_krichai@...cinc.com>,
 Bjorn Andersson <andersson@...nel.org>,
 Konrad Dybcio <konrad.dybcio@...aro.org>, Rob Herring <robh+dt@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Conor Dooley <conor+dt@...nel.org>, linux-arm-msm@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 quic_vbadigan@...cinc.com, quic_ramkri@...cinc.com,
 quic_nitegupt@...cinc.com, quic_skananth@...cinc.com,
 quic_parass@...cinc.com
Subject: Re: [PATCH] arm64: dts: qcom: qcs6490-rb3gen2: Add PCIe nodes

On October 1, 2024 5:19:48 PM GMT+03:00, Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org> wrote:
>On Tue, Oct 01, 2024 at 03:30:14PM +0300, Dmitry Baryshkov wrote:
>> On October 1, 2024 1:16:22 PM GMT+03:00, Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org> wrote:
>> >On Fri, Feb 09, 2024 at 12:56:18PM +0200, Dmitry Baryshkov wrote:
>> >> On Fri, 9 Feb 2024 at 09:57, Manivannan Sadhasivam
>> >> <manivannan.sadhasivam@...aro.org> wrote:
>> >> >
>> >> > 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@...cinc.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@...cinc.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@...cinc.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@...cinc.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.
>> >> 
>> >> This sounds like a sane default, indeed. Nevertheless, I see a point
>> >> in having per-device-SID assignment. This increases isolation and can
>> >> potentially prevent security issues. However in such case SID
>> >> assignment should be handled in some automagic way. In other words,
>> >> there must be no need to duplicate the topology of the PCIe bus in the
>> >> iommu-maps property.
>> >> 
>> >
>> >Agree with you on this. This is what I suggested some time back to have the
>> >logic in the SMMU/PCIe drivers to assign SIDs dynamically. Unfortunately, it is
>> >not a trivial work and it requires a broader discussion with the community.
>> >
>> >Also starting with SMMUv3, there are practically no limitations in SIDs and
>> >each device should get a unique SID by default.
>> >
>> >So I got convinced that we can have these static mappings in the DT *atm* for
>> >non SMMUv3 based hardwares and at the same time let the discussion happen with
>> >the community. But this static mapping solution is just an interim one and won't
>> >scale if more devices are added to the topology.
>> 
>> My main question to this approach is if it can support additional devices plugged into the switch. If there is no way to plug addon cards, then it is fine as a temporary measure.
>> 
>
>The logic here is that the fixed endpoints in the switch will get an unique SID
>and the devices getting attached to slots will share the same SID of the bus
>(this is the usual case with all Qcom SoCs).
>
>But I guess we would need 'iommu-map-mask' as well. Hope this addresses your
>concern.

Yes, thank you!


>
>- Mani
>


-- 
With best wishes
Dmitry

Powered by blists - more mailing lists