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

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

[Krishna chaitanya chundru]

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>;
+
+	status = "okay";
+};
+
+&pcie1_phy {
+	vdda-phy-supply = <&vreg_l10c_0p88>;
+	vdda-pll-supply = <&vreg_l6b_1p2>;
+
+	status = "okay";
+};
+
 &qupv3_id_0 {
 	status = "okay";
 };
@@ -420,6 +446,22 @@ &qupv3_id_0 {
 &tlmm {
 	gpio-reserved-ranges = <32 2>, /* ADSP */
 			       <48 4>; /* NFC */
+
+	pcie1_reset_n: pcie1-reset-n-state {
+		pins = "gpio2";
+		function = "gpio";
+		drive-strength = <16>;
+		output-low;
+		bias-disable;
+	};
+
+	pcie1_wake_n: pcie1-wake-n-state {
+		pins = "gpio3";
+		function = "gpio";
+		drive-strength = <2>;
+		bias-pull-up;
+	};
+
 };
 
 &uart5 {

---
base-commit: 70d201a40823acba23899342d62bc2644051ad2e
change-id: 20240207-enable_pcie-95b1d6612b27

Best regards,
-- 
Krishna chaitanya chundru <quic_krichai@quicinc.com>

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

[Dmitry Baryshkov]

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?

> +
> +       status = "okay";
> +};
> +
> +&pcie1_phy {
> +       vdda-phy-supply = <&vreg_l10c_0p88>;
> +       vdda-pll-supply = <&vreg_l6b_1p2>;
> +
> +       status = "okay";
> +};
> +
>  &qupv3_id_0 {
>         status = "okay";
>  };
> @@ -420,6 +446,22 @@ &qupv3_id_0 {
>  &tlmm {
>         gpio-reserved-ranges = <32 2>, /* ADSP */
>                                <48 4>; /* NFC */
> +
> +       pcie1_reset_n: pcie1-reset-n-state {
> +               pins = "gpio2";
> +               function = "gpio";
> +               drive-strength = <16>;
> +               output-low;
> +               bias-disable;
> +       };
> +
> +       pcie1_wake_n: pcie1-wake-n-state {
> +               pins = "gpio3";
> +               function = "gpio";
> +               drive-strength = <2>;
> +               bias-pull-up;
> +       };
> +
>  };
>
>  &uart5 {
>
> ---
> base-commit: 70d201a40823acba23899342d62bc2644051ad2e
> change-id: 20240207-enable_pcie-95b1d6612b27
>
> Best regards,
> --
> Krishna chaitanya chundru <quic_krichai@quicinc.com>
>
>


-- 
With best wishes
Dmitry

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

[Krishna Chaitanya Chundru]

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.

- Krishna Chaitanya.
>> +
>> +       status = "okay";
>> +};
>> +
>> +&pcie1_phy {
>> +       vdda-phy-supply = <&vreg_l10c_0p88>;
>> +       vdda-pll-supply = <&vreg_l6b_1p2>;
>> +
>> +       status = "okay";
>> +};
>> +
>>   &qupv3_id_0 {
>>          status = "okay";
>>   };
>> @@ -420,6 +446,22 @@ &qupv3_id_0 {
>>   &tlmm {
>>          gpio-reserved-ranges = <32 2>, /* ADSP */
>>                                 <48 4>; /* NFC */
>> +
>> +       pcie1_reset_n: pcie1-reset-n-state {
>> +               pins = "gpio2";
>> +               function = "gpio";
>> +               drive-strength = <16>;
>> +               output-low;
>> +               bias-disable;
>> +       };
>> +
>> +       pcie1_wake_n: pcie1-wake-n-state {
>> +               pins = "gpio3";
>> +               function = "gpio";
>> +               drive-strength = <2>;
>> +               bias-pull-up;
>> +       };
>> +
>>   };
>>
>>   &uart5 {
>>
>> ---
>> base-commit: 70d201a40823acba23899342d62bc2644051ad2e
>> change-id: 20240207-enable_pcie-95b1d6612b27
>>
>> Best regards,
>> --
>> Krishna chaitanya chundru <quic_krichai@quicinc.com>
>>
>>
> 
> 

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

[Dmitry Baryshkov]

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.

>
> - Krishna Chaitanya.
> >> +
> >> +       status = "okay";
> >> +};
> >> +
> >> +&pcie1_phy {
> >> +       vdda-phy-supply = <&vreg_l10c_0p88>;
> >> +       vdda-pll-supply = <&vreg_l6b_1p2>;
> >> +
> >> +       status = "okay";
> >> +};
> >> +
> >>   &qupv3_id_0 {
> >>          status = "okay";
> >>   };
> >> @@ -420,6 +446,22 @@ &qupv3_id_0 {
> >>   &tlmm {
> >>          gpio-reserved-ranges = <32 2>, /* ADSP */
> >>                                 <48 4>; /* NFC */
> >> +
> >> +       pcie1_reset_n: pcie1-reset-n-state {
> >> +               pins = "gpio2";
> >> +               function = "gpio";
> >> +               drive-strength = <16>;
> >> +               output-low;
> >> +               bias-disable;
> >> +       };
> >> +
> >> +       pcie1_wake_n: pcie1-wake-n-state {
> >> +               pins = "gpio3";
> >> +               function = "gpio";
> >> +               drive-strength = <2>;
> >> +               bias-pull-up;
> >> +       };
> >> +
> >>   };
> >>
> >>   &uart5 {
> >>
> >> ---
> >> base-commit: 70d201a40823acba23899342d62bc2644051ad2e
> >> change-id: 20240207-enable_pcie-95b1d6612b27
> >>
> >> Best regards,
> >> --
> >> Krishna chaitanya chundru <quic_krichai@quicinc.com>
> >>
> >>
> >
> >



-- 
With best wishes
Dmitry

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

[Krishna Chaitanya Chundru]

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.

For that reason I tried to add it here.

- Krishna Chaitanya.
>>
>> - Krishna Chaitanya.
>>>> +
>>>> +       status = "okay";
>>>> +};
>>>> +
>>>> +&pcie1_phy {
>>>> +       vdda-phy-supply = <&vreg_l10c_0p88>;
>>>> +       vdda-pll-supply = <&vreg_l6b_1p2>;
>>>> +
>>>> +       status = "okay";
>>>> +};
>>>> +
>>>>    &qupv3_id_0 {
>>>>           status = "okay";
>>>>    };
>>>> @@ -420,6 +446,22 @@ &qupv3_id_0 {
>>>>    &tlmm {
>>>>           gpio-reserved-ranges = <32 2>, /* ADSP */
>>>>                                  <48 4>; /* NFC */
>>>> +
>>>> +       pcie1_reset_n: pcie1-reset-n-state {
>>>> +               pins = "gpio2";
>>>> +               function = "gpio";
>>>> +               drive-strength = <16>;
>>>> +               output-low;
>>>> +               bias-disable;
>>>> +       };
>>>> +
>>>> +       pcie1_wake_n: pcie1-wake-n-state {
>>>> +               pins = "gpio3";
>>>> +               function = "gpio";
>>>> +               drive-strength = <2>;
>>>> +               bias-pull-up;
>>>> +       };
>>>> +
>>>>    };
>>>>
>>>>    &uart5 {
>>>>
>>>> ---
>>>> base-commit: 70d201a40823acba23899342d62bc2644051ad2e
>>>> change-id: 20240207-enable_pcie-95b1d6612b27
>>>>
>>>> Best regards,
>>>> --
>>>> Krishna chaitanya chundru <quic_krichai@quicinc.com>
>>>>
>>>>
>>>
>>>
> 
> 
> 

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

[Dmitry Baryshkov]

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?

>
> For that reason I tried to add it here.
>
> - Krishna Chaitanya.
> >>
> >> - Krishna Chaitanya.
> >>>> +
> >>>> +       status = "okay";
> >>>> +};
> >>>> +
> >>>> +&pcie1_phy {
> >>>> +       vdda-phy-supply = <&vreg_l10c_0p88>;
> >>>> +       vdda-pll-supply = <&vreg_l6b_1p2>;
> >>>> +
> >>>> +       status = "okay";
> >>>> +};
> >>>> +
> >>>>    &qupv3_id_0 {
> >>>>           status = "okay";
> >>>>    };
> >>>> @@ -420,6 +446,22 @@ &qupv3_id_0 {
> >>>>    &tlmm {
> >>>>           gpio-reserved-ranges = <32 2>, /* ADSP */
> >>>>                                  <48 4>; /* NFC */
> >>>> +
> >>>> +       pcie1_reset_n: pcie1-reset-n-state {
> >>>> +               pins = "gpio2";
> >>>> +               function = "gpio";
> >>>> +               drive-strength = <16>;
> >>>> +               output-low;
> >>>> +               bias-disable;
> >>>> +       };
> >>>> +
> >>>> +       pcie1_wake_n: pcie1-wake-n-state {
> >>>> +               pins = "gpio3";
> >>>> +               function = "gpio";
> >>>> +               drive-strength = <2>;
> >>>> +               bias-pull-up;
> >>>> +       };
> >>>> +
> >>>>    };
> >>>>
> >>>>    &uart5 {
> >>>>
> >>>> ---
> >>>> base-commit: 70d201a40823acba23899342d62bc2644051ad2e
> >>>> change-id: 20240207-enable_pcie-95b1d6612b27
> >>>>
> >>>> Best regards,
> >>>> --
> >>>> Krishna chaitanya chundru <quic_krichai@quicinc.com>
> >>>>
> >>>>
> >>>
> >>>
> >
> >
> >



-- 
With best wishes
Dmitry

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

[Krishna Chaitanya Chundru]

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.

- Krishna Chaitanya.

>>
>> For that reason I tried to add it here.
>>
>> - Krishna Chaitanya.
>>>>
>>>> - Krishna Chaitanya.
>>>>>> +
>>>>>> +       status = "okay";
>>>>>> +};
>>>>>> +
>>>>>> +&pcie1_phy {
>>>>>> +       vdda-phy-supply = <&vreg_l10c_0p88>;
>>>>>> +       vdda-pll-supply = <&vreg_l6b_1p2>;
>>>>>> +
>>>>>> +       status = "okay";
>>>>>> +};
>>>>>> +
>>>>>>     &qupv3_id_0 {
>>>>>>            status = "okay";
>>>>>>     };
>>>>>> @@ -420,6 +446,22 @@ &qupv3_id_0 {
>>>>>>     &tlmm {
>>>>>>            gpio-reserved-ranges = <32 2>, /* ADSP */
>>>>>>                                   <48 4>; /* NFC */
>>>>>> +
>>>>>> +       pcie1_reset_n: pcie1-reset-n-state {
>>>>>> +               pins = "gpio2";
>>>>>> +               function = "gpio";
>>>>>> +               drive-strength = <16>;
>>>>>> +               output-low;
>>>>>> +               bias-disable;
>>>>>> +       };
>>>>>> +
>>>>>> +       pcie1_wake_n: pcie1-wake-n-state {
>>>>>> +               pins = "gpio3";
>>>>>> +               function = "gpio";
>>>>>> +               drive-strength = <2>;
>>>>>> +               bias-pull-up;
>>>>>> +       };
>>>>>> +
>>>>>>     };
>>>>>>
>>>>>>     &uart5 {
>>>>>>
>>>>>> ---
>>>>>> base-commit: 70d201a40823acba23899342d62bc2644051ad2e
>>>>>> change-id: 20240207-enable_pcie-95b1d6612b27
>>>>>>
>>>>>> Best regards,
>>>>>> --
>>>>>> Krishna chaitanya chundru <quic_krichai@quicinc.com>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>
>>>
>>>
> 
> 
> 

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

[Manivannan Sadhasivam]

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

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

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

[Dmitry Baryshkov]

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.

>
> Perhaps you are missing iommu-map-mask?


-- 
With best wishes
Dmitry

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

[Manivannan Sadhasivam]

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.
> 

Yes, address space isolation is the primary motive behind this patch. But as
you said, we should not do it by hardcoding the SIDs in the board DTS. It won't
scale and is not a proper solution.

Instead, the issue should be addressed in the IOMMU layer by working with the
IOMMU folks.

It should be noted that we _cannot_ use any arbitrary SID for PCIe bus. HYP/TZ
will fault the transactions coming with different SIDs than the ones assigned
to them. So we still need to pass that info from DT to IOMMU layer.

- Mani

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

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

[Dmitry Baryshkov]

On Mon, 12 Feb 2024 at 15:16, 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.
> >
>
> Yes, address space isolation is the primary motive behind this patch. But as
> you said, we should not do it by hardcoding the SIDs in the board DTS. It won't
> scale and is not a proper solution.
>
> Instead, the issue should be addressed in the IOMMU layer by working with the
> IOMMU folks.
>
> It should be noted that we _cannot_ use any arbitrary SID for PCIe bus. HYP/TZ
> will fault the transactions coming with different SIDs than the ones assigned
> to them. So we still need to pass that info from DT to IOMMU layer.

Yes, passing a range or a masked value sounds logical. Passing 1:1
mapping for a dynamic bus doesn't.


-- 
With best wishes
Dmitry

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

[Manivannan Sadhasivam]

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.

- Mani

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

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

[Dmitry Baryshkov]

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.

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.

>
>- Mani
>


-- 
With best wishes
Dmitry

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

[Manivannan Sadhasivam]

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.
> 
> 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.

- Mani

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

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

[Dmitry Baryshkov]

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.
>> 
>> 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

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

[Krishna Chaitanya Chundru]

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
>>
> 
> 

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

[Manivannan Sadhasivam]

On Fri, Oct 11, 2024 at 05:24:29PM +0530, Krishna Chaitanya Chundru wrote:

[...]

> > > 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.
> 

Hmm, I was not aware that it doesn't have open slots. Fine with me then.

- Mani

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

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

[Dmitry Baryshkov]

On Sat, Oct 12, 2024 at 06:13:34PM +0530, Manivannan Sadhasivam wrote:
> On Fri, Oct 11, 2024 at 05:24:29PM +0530, Krishna Chaitanya Chundru wrote:
> 
> [...]
> 
> > > > 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.
> > 
> 
> Hmm, I was not aware that it doesn't have open slots. Fine with me then.

It doesn't feature open slots, but it has two PCIe connections on HS2 /
HS3. Users might attach external PCIe devices.

Krishna, could you please clarify, how those two connections are routed?

-- 
With best wishes
Dmitry

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

[Krishna Chaitanya Chundru]

On 10/14/2024 4:55 AM, Dmitry Baryshkov wrote:
> On Sat, Oct 12, 2024 at 06:13:34PM +0530, Manivannan Sadhasivam wrote:
>> On Fri, Oct 11, 2024 at 05:24:29PM +0530, Krishna Chaitanya Chundru wrote:
>>
>> [...]
>>
>>>>> 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.
>>>
>>
>> Hmm, I was not aware that it doesn't have open slots. Fine with me then.
> 
> It doesn't feature open slots, but it has two PCIe connections on HS2 /
> HS3. Users might attach external PCIe devices.
> 
> Krishna, could you please clarify, how those two connections are routed?
> 
For this qps615 board to one of the downstream port (pcie to usb) usb
hub is connected and to the other downstream port NVMe will be
connected.

- Krishna Chaitanya.

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

[Dmitry Baryshkov]

On Wed, Oct 16, 2024 at 10:43:19AM +0530, Krishna Chaitanya Chundru wrote:
> 
> 
> On 10/14/2024 4:55 AM, Dmitry Baryshkov wrote:
> > On Sat, Oct 12, 2024 at 06:13:34PM +0530, Manivannan Sadhasivam wrote:
> > > On Fri, Oct 11, 2024 at 05:24:29PM +0530, Krishna Chaitanya Chundru wrote:
> > > 
> > > [...]
> > > 
> > > > > > 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.
> > > > 
> > > 
> > > Hmm, I was not aware that it doesn't have open slots. Fine with me then.
> > 
> > It doesn't feature open slots, but it has two PCIe connections on HS2 /
> > HS3. Users might attach external PCIe devices.
> > 
> > Krishna, could you please clarify, how those two connections are routed?
> > 
> For this qps615 board to one of the downstream port (pcie to usb) usb
> hub is connected and to the other downstream port NVMe will be
> connected.

The board has two PCIe links routed to the HS2 and HS3 connectors. Are
they routed to the PCIe switch?

Yes, they are not standard slots, but still the board is expandable and
it is possible to connect external PCIe devices. As such it is not
possible to have static SID mapping.

-- 
With best wishes
Dmitry

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

[Manivannan Sadhasivam]

On Thu, Oct 17, 2024 at 02:12:00PM +0300, Dmitry Baryshkov wrote:
> On Wed, Oct 16, 2024 at 10:43:19AM +0530, Krishna Chaitanya Chundru wrote:
> > 
> > 
> > On 10/14/2024 4:55 AM, Dmitry Baryshkov wrote:
> > > On Sat, Oct 12, 2024 at 06:13:34PM +0530, Manivannan Sadhasivam wrote:
> > > > On Fri, Oct 11, 2024 at 05:24:29PM +0530, Krishna Chaitanya Chundru wrote:
> > > > 
> > > > [...]
> > > > 
> > > > > > > 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.
> > > > > 
> > > > 
> > > > Hmm, I was not aware that it doesn't have open slots. Fine with me then.
> > > 
> > > It doesn't feature open slots, but it has two PCIe connections on HS2 /
> > > HS3. Users might attach external PCIe devices.
> > > 
> > > Krishna, could you please clarify, how those two connections are routed?
> > > 
> > For this qps615 board to one of the downstream port (pcie to usb) usb
> > hub is connected and to the other downstream port NVMe will be
> > connected.
> 
> The board has two PCIe links routed to the HS2 and HS3 connectors. Are
> they routed to the PCIe switch?
> 
> Yes, they are not standard slots, but still the board is expandable and
> it is possible to connect external PCIe devices. As such it is not
> possible to have static SID mapping.
> 

Sorry, I think the conversation got deviated. We have concluded that the
endpoints fixed (soldered) in the board will get a fixed SID (because we know
what they are) and all other devices going to get connected to HS/LS connectors
will get shared SID (because we don't know what they are).

Any concern with that?

- Mani

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

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

[Dmitry Baryshkov]

On Tue, 22 Oct 2024 at 18:10, Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Thu, Oct 17, 2024 at 02:12:00PM +0300, Dmitry Baryshkov wrote:
> > On Wed, Oct 16, 2024 at 10:43:19AM +0530, Krishna Chaitanya Chundru wrote:
> > >
> > >
> > > On 10/14/2024 4:55 AM, Dmitry Baryshkov wrote:
> > > > On Sat, Oct 12, 2024 at 06:13:34PM +0530, Manivannan Sadhasivam wrote:
> > > > > On Fri, Oct 11, 2024 at 05:24:29PM +0530, Krishna Chaitanya Chundru wrote:
> > > > >
> > > > > [...]
> > > > >
> > > > > > > > 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.
> > > > > >
> > > > >
> > > > > Hmm, I was not aware that it doesn't have open slots. Fine with me then.
> > > >
> > > > It doesn't feature open slots, but it has two PCIe connections on HS2 /
> > > > HS3. Users might attach external PCIe devices.
> > > >
> > > > Krishna, could you please clarify, how those two connections are routed?
> > > >
> > > For this qps615 board to one of the downstream port (pcie to usb) usb
> > > hub is connected and to the other downstream port NVMe will be
> > > connected.
> >
> > The board has two PCIe links routed to the HS2 and HS3 connectors. Are
> > they routed to the PCIe switch?
> >
> > Yes, they are not standard slots, but still the board is expandable and
> > it is possible to connect external PCIe devices. As such it is not
> > possible to have static SID mapping.
> >
>
> Sorry, I think the conversation got deviated. We have concluded that the
> endpoints fixed (soldered) in the board will get a fixed SID (because we know
> what they are) and all other devices going to get connected to HS/LS connectors
> will get shared SID (because we don't know what they are).
>
> Any concern with that?

This is perfectly fine with me.

-- 
With best wishes
Dmitry