Re: [PATCH] arm64: dts: qcom: qcs6490-rb3gen2: Add PCIe nodes

[Krishna Chaitanya Chundru <quic_krichai@quicinc.com>]

On 10/1/2024 8:38 PM, Dmitry Baryshkov wrote:
> On October 1, 2024 5:19:48 PM GMT+03:00, Manivannan Sadhasivam <manivannan.sadhasivam@linaro.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@linaro.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@linaro.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@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.
>>>>>
>>>>> 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.
>>>e
>>> 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!
> 
Hi dimitry & mani,

This particular board variant doesn't expose any open slots to connect
a different endpoints like another switch(which might have BDF unknown
to us) so static table should be fine for this board variant.

I tries to add iommu-map-mask property, the issue with that property is
that the driver is applying the mask to the bdf before searching for the
entry in the table. If I use a mask value which satisfies all the
entries in the table ( mask as 0x718) and if a new bdf is enumerated
lets say 0x600 due to mask 0x718 its value is again 0x600 only.

Can we skip iommu-map-mask property and use only static table for this
board as we know this board doesn't expose any open slots.

- Krishna chaitanya.
variant doesn't expose any open slots >
>>
>> - Mani
>>
> 
>