[PATCH v1 0/3] PCIe and AR8151 on APQ8098/MSM8998

[PATCH v1 0/3] PCIe and AR8151 on APQ8098/MSM8998

[Marc Gonzalez]

Hello everyone,

After a lot of poking, I am finally able to use the AR8151 ethernet on the APQ8098 board.
The magic bits are the iommu-map prop and the PCIE20_PARF_BDF_TRANSLATE_N setup.

The WIP thread is archived here:
https://marc.info/?t=155059539200004&r=1&w=2


Marc Gonzalez (3):
  PCI: qcom: Setup PCIE20_PARF_BDF_TRANSLATE_N
  arm64: dts: qcom: msm8998: Add PCIe SMMU node
  arm64: dts: qcom: msm8998: Add PCIe PHY and RC nodes

 arch/arm64/boot/dts/qcom/msm8998.dtsi  | 93 ++++++++++++++++++++++++++
 drivers/pci/controller/dwc/pcie-qcom.c |  4 ++
 2 files changed, 97 insertions(+)

-- 
2.17.1

[PATCH v1 1/3] PCI: qcom: Setup PCIE20_PARF_BDF_TRANSLATE_N

[Marc Gonzalez]

Initialize PCIE20_PARF_BDF_TRANSLATE_N for ops_2_3_2.

Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 0ed235d560e3..5e5522a427b8 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -54,6 +54,7 @@
 #define PCIE20_PARF_LTSSM			0x1B0
 #define PCIE20_PARF_SID_OFFSET			0x234
 #define PCIE20_PARF_BDF_TRANSLATE_CFG		0x24C
+#define PCIE20_PARF_BDF_TRANSLATE_N		0x250
 
 #define PCIE20_ELBI_SYS_CTRL			0x04
 #define PCIE20_ELBI_SYS_CTRL_LT_ENABLE		BIT(0)
@@ -602,6 +603,9 @@ static int qcom_pcie_init_2_3_2(struct qcom_pcie *pcie)
 	val |= BIT(31);
 	writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2);
 
+	val = PCI_DEVID(1, 0);
+	writel(val, pcie->parf + PCIE20_PARF_BDF_TRANSLATE_N);
+
 	return 0;
 
 err_slave_clk:
-- 
2.17.1

Re: [PATCH v1 1/3] PCI: qcom: Setup PCIE20_PARF_BDF_TRANSLATE_N

[Stanimir Varbanov]

Hi Marc,

Thank you for the work on that!

On 3/28/19 7:01 PM, Marc Gonzalez wrote:
> Initialize PCIE20_PARF_BDF_TRANSLATE_N for ops_2_3_2.
> 
> Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 0ed235d560e3..5e5522a427b8 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -54,6 +54,7 @@
>  #define PCIE20_PARF_LTSSM			0x1B0
>  #define PCIE20_PARF_SID_OFFSET			0x234
>  #define PCIE20_PARF_BDF_TRANSLATE_CFG		0x24C
> +#define PCIE20_PARF_BDF_TRANSLATE_N		0x250
>  
>  #define PCIE20_ELBI_SYS_CTRL			0x04
>  #define PCIE20_ELBI_SYS_CTRL_LT_ENABLE		BIT(0)
> @@ -602,6 +603,9 @@ static int qcom_pcie_init_2_3_2(struct qcom_pcie *pcie)
>  	val |= BIT(31);
>  	writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2);
>  
> +	val = PCI_DEVID(1, 0);
> +	writel(val, pcie->parf + PCIE20_PARF_BDF_TRANSLATE_N);

I wonder, shouldn't this register manipulation happen in common code
e.g. pcie-designware-host.c and of course the manipulation should be
conditional because not all platforms might have iommu.

Also, could we have more generic iommu-map description like the one for
sdm845 at [1] ?

-- 
regards,
Stan

Re: [PATCH v1 1/3] PCI: qcom: Setup PCIE20_PARF_BDF_TRANSLATE_N

[Marc Gonzalez]

On 02/04/2019 10:08, Stanimir Varbanov wrote:

> On 3/28/19 7:01 PM, Marc Gonzalez wrote:
> 
>> Initialize PCIE20_PARF_BDF_TRANSLATE_N for ops_2_3_2.
>>
>> Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr>
>> ---
>>  drivers/pci/controller/dwc/pcie-qcom.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>> index 0ed235d560e3..5e5522a427b8 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>> @@ -54,6 +54,7 @@
>>  #define PCIE20_PARF_LTSSM			0x1B0
>>  #define PCIE20_PARF_SID_OFFSET			0x234
>>  #define PCIE20_PARF_BDF_TRANSLATE_CFG		0x24C
>> +#define PCIE20_PARF_BDF_TRANSLATE_N		0x250
>>  
>>  #define PCIE20_ELBI_SYS_CTRL			0x04
>>  #define PCIE20_ELBI_SYS_CTRL_LT_ENABLE		BIT(0)
>> @@ -602,6 +603,9 @@ static int qcom_pcie_init_2_3_2(struct qcom_pcie *pcie)
>>  	val |= BIT(31);
>>  	writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2);
>>  
>> +	val = PCI_DEVID(1, 0);
>> +	writel(val, pcie->parf + PCIE20_PARF_BDF_TRANSLATE_N);
> 
> I wonder, shouldn't this register manipulation happen in common code
> e.g. pcie-designware-host.c

I don't think so. PARF (PCIe Auxiliary Register File) appears to be
a vendor-specific area. Perhaps someone with access to the Designware
documentation could check. (Though it is possible that several vendors
have implemented exactly the same PARF layout.)

> and of course the manipulation should be conditional because
> not all platforms might have iommu.

My patch tweaks PCIE20_PARF_BDF_TRANSLATE_N in qcom_pcie_init_2_3_2()
which means 8996 and 8998 only (and possibly sdm660)

$ git grep smmu-exist vendor -- arch/arm/boot/dts/qcom
vendor:arch/arm/boot/dts/qcom/msm8996.dtsi:             qcom,smmu-exist;
vendor:arch/arm/boot/dts/qcom/msm8996.dtsi:             qcom,smmu-exist;
vendor:arch/arm/boot/dts/qcom/msm8996.dtsi:             qcom,smmu-exist;
vendor:arch/arm/boot/dts/qcom/msm8998-interposer-sdm660.dtsi:           qcom,smmu-exist;
vendor:arch/arm/boot/dts/qcom/msm8998.dtsi:             qcom,smmu-exist;

https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/drivers/pci/host/pci-msm.c?h=LE.UM.1.3.r3.25#n3880


> Also, could we have more generic iommu-map description like the one for
> sdm845 at [1] ?

I assume you meant https://patchwork.kernel.org/patch/10831293/

I don't think that such a map is quite right for msm8998 -- and even
for sdm845 :-p

There is only a single PCIe master, other than the root complex:

# lspci
00:00.0 PCI bridge: Qualcomm Device 0105
01:00.0 Ethernet controller: Qualcomm Atheros AR8151 v2.0 Gigabit Ethernet (rev c0)

Hence iommu-map = <0x100 &anoc1_smmu 0x1480 1>;
i.e. map stream-id 0x1480 to device-id 01:00.0

We could pick a different stream-id for 0x100 but I don't see any
advantage (??) in doing so...

If a SoC designer took the msm8998 and added a PCIe bridge to support
multiple PCIe EPs, then they would override the iommu-map in their
board-specific DTS.


I do have a nagging feeling that this patch is not perfect for 3 reasons:

1) Did msm8996 PCIe work without tweaking PCIE20_PARF_BDF_TRANSLATE_N?
If so, why? What is the difference with msm8998 PCIe?
"MSM8998 does not have static SID configuration for PCIe."
What about MSM8996?

2) OF: /soc/pci@1c00000: no iommu-map translation for rid 0x0 on           (null)
The kernel warns that the root complex has no asssigned stream-id.
I'm not sure that's actually an issue...

3) I just blindly write PCI_DEVID(1, 0) to PCIE20_PARF_BDF_TRANSLATE_N

To be really generic, I would have to walk the iommu-map and do

for_each_iommu_map_entry
	offset = PCIE20_PARF_BDF_TRANSLATE_N + (out_base - 0x1480) * 4;
	writel(rid_base, pcie->parf + offset);

Perhaps do this in of_pci_iommu_init() ?
Maybe have to tweak of_map_rid() or duplicate some code...

On first approximation, maybe I can just do the blind write, but only
for 8998. That way, I don't change anything for the other platforms.

Comments?

Regards.

[PATCH v1 2/3] arm64: dts: qcom: msm8998: Add PCIe SMMU node

[Marc Gonzalez]

ANOC1 SMMU filters PCIe accesses.

Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr>
---
 arch/arm64/boot/dts/qcom/msm8998.dtsi | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi
index f9a922fdae75..5a1c0961b281 100644
--- a/arch/arm64/boot/dts/qcom/msm8998.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi
@@ -606,6 +606,21 @@
 			#thermal-sensor-cells = <1>;
 		};
 
+		anoc1_smmu: arm,smmu@1680000 {
+			compatible = "qcom,msm8996-smmu-v2", "qcom,smmu-v2";
+			reg = <0x01680000 0x10000>;
+			#iommu-cells = <1>;
+
+			#global-interrupts = <0>;
+			interrupts =
+				<GIC_SPI 364 IRQ_TYPE_EDGE_RISING>,
+				<GIC_SPI 365 IRQ_TYPE_EDGE_RISING>,
+				<GIC_SPI 366 IRQ_TYPE_EDGE_RISING>,
+				<GIC_SPI 367 IRQ_TYPE_EDGE_RISING>,
+				<GIC_SPI 368 IRQ_TYPE_EDGE_RISING>,
+				<GIC_SPI 369 IRQ_TYPE_EDGE_RISING>;
+		};
+
 		tcsr_mutex_regs: syscon@1f40000 {
 			compatible = "syscon";
 			reg = <0x1f40000 0x20000>;
-- 
2.17.1

Re: [PATCH v1 2/3] arm64: dts: qcom: msm8998: Add PCIe SMMU node

[Marc Gonzalez]

On 28/03/2019 18:05, Marc Gonzalez wrote:

> ANOC1 SMMU filters PCIe accesses.

I'm not sure this description is entirely accurate...

ANOC likely stands for "Application Network-On-Chip"


>  arch/arm64/boot/dts/qcom/msm8998.dtsi | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi
> index f9a922fdae75..5a1c0961b281 100644
> --- a/arch/arm64/boot/dts/qcom/msm8998.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi
> @@ -606,6 +606,21 @@
>  			#thermal-sensor-cells = <1>;
>  		};
>  
> +		anoc1_smmu: arm,smmu@1680000 {
> +			compatible = "qcom,msm8996-smmu-v2", "qcom,smmu-v2";

Maybe I should instead use "qcom,msm8998-smmu-v2", "qcom,smmu-v2";
and define "qcom,msm8998-smmu-v2" in
Documentation/devicetree/bindings/iommu/arm,smmu.txt ?
(Would the Documentation change need to be in a separate patch?)

> +			reg = <0x01680000 0x10000>;
> +			#iommu-cells = <1>;

I'm not sure about this property. IIRC, Robin said <0> is not valid,
but I don't have any iommus prop, only iommu-map.

> +
> +			#global-interrupts = <0>;
> +			interrupts =
> +				<GIC_SPI 364 IRQ_TYPE_EDGE_RISING>,
> +				<GIC_SPI 365 IRQ_TYPE_EDGE_RISING>,
> +				<GIC_SPI 366 IRQ_TYPE_EDGE_RISING>,
> +				<GIC_SPI 367 IRQ_TYPE_EDGE_RISING>,
> +				<GIC_SPI 368 IRQ_TYPE_EDGE_RISING>,
> +				<GIC_SPI 369 IRQ_TYPE_EDGE_RISING>;
> +		};
> +

The rest of the node looks fairly straight-forward.

DT code was adapted from:

https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/arch/arm/boot/dts/qcom/msm-arm-smmu-8998.dtsi?h=LE.UM.1.3.r3.25#n18

I left out the so-called implementation-defined init:

https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/arch/arm/boot/dts/qcom/msm-arm-smmu-impl-defs-8998.dtsi?h=LE.UM.1.3.r3.25#n123

Should I try to merge this part in mainline?
(I don't have any documentation for it though.)

Regards.

Re: [PATCH v1 2/3] arm64: dts: qcom: msm8998: Add PCIe SMMU node

[Robin Murphy]

On 29/03/2019 10:51, Marc Gonzalez wrote:
> On 28/03/2019 18:05, Marc Gonzalez wrote:
> 
>> ANOC1 SMMU filters PCIe accesses.
> 
> I'm not sure this description is entirely accurate...
> 
> ANOC likely stands for "Application Network-On-Chip"
> 
> 
>>   arch/arm64/boot/dts/qcom/msm8998.dtsi | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi
>> index f9a922fdae75..5a1c0961b281 100644
>> --- a/arch/arm64/boot/dts/qcom/msm8998.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi
>> @@ -606,6 +606,21 @@
>>   			#thermal-sensor-cells = <1>;
>>   		};
>>   
>> +		anoc1_smmu: arm,smmu@1680000 {
>> +			compatible = "qcom,msm8996-smmu-v2", "qcom,smmu-v2";
> 
> Maybe I should instead use "qcom,msm8998-smmu-v2", "qcom,smmu-v2";
> and define "qcom,msm8998-smmu-v2" in
> Documentation/devicetree/bindings/iommu/arm,smmu.txt ?

Yes please.

> (Would the Documentation change need to be in a separate patch?)

That's generally preferred, yes.

> 
>> +			reg = <0x01680000 0x10000>;
>> +			#iommu-cells = <1>;
> 
> I'm not sure about this property. IIRC, Robin said <0> is not valid,
> but I don't have any iommus prop, only iommu-map.

You have to join the dots between the various bindings a little, but the 
iommu-base part of each iommu-map entry is an IOMMU specifier, and IOMMU 
specifiers are #iommu-cells long.

To cut a long story short, 1 is definitely appropriate, because 
arm-smmu's definition of a 2-cell specifier wouldn't make much sense in 
the iommu-map context (and the current code for parsing iommu-map 
actually just assumes 1 anyway).

>> +
>> +			#global-interrupts = <0>;
>> +			interrupts =
>> +				<GIC_SPI 364 IRQ_TYPE_EDGE_RISING>,
>> +				<GIC_SPI 365 IRQ_TYPE_EDGE_RISING>,
>> +				<GIC_SPI 366 IRQ_TYPE_EDGE_RISING>,
>> +				<GIC_SPI 367 IRQ_TYPE_EDGE_RISING>,
>> +				<GIC_SPI 368 IRQ_TYPE_EDGE_RISING>,
>> +				<GIC_SPI 369 IRQ_TYPE_EDGE_RISING>;
>> +		};
>> +
> 
> The rest of the node looks fairly straight-forward.

You should probably have some "bus" and "iface" clocks too, per the 
requirement of "qcom,smmu-v2". Maybe Vivek might know what's relevant 
for MSM8998?

> 
> DT code was adapted from:
> 
> https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/arch/arm/boot/dts/qcom/msm-arm-smmu-8998.dtsi?h=LE.UM.1.3.r3.25#n18
> 
> I left out the so-called implementation-defined init:
> 
> https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/arch/arm/boot/dts/qcom/msm-arm-smmu-impl-defs-8998.dtsi?h=LE.UM.1.3.r3.25#n123
> 
> Should I try to merge this part in mainline?
> (I don't have any documentation for it though.)

"pls no :("

I'm not sure what route the path takes from "DT describes the platform" 
to get to "DT lists opaque register addresses and magic data to write 
into them", but I imagine it might involve getting hit in the head along 
the way.

Robin.

Re: [PATCH v1 2/3] arm64: dts: qcom: msm8998: Add PCIe SMMU node

[Jeffrey Hugo]

On 3/29/2019 12:29 PM, Robin Murphy wrote:
> On 29/03/2019 10:51, Marc Gonzalez wrote:
>> On 28/03/2019 18:05, Marc Gonzalez wrote:
>>> +
>>> +            #global-interrupts = <0>;
>>> +            interrupts =
>>> +                <GIC_SPI 364 IRQ_TYPE_EDGE_RISING>,
>>> +                <GIC_SPI 365 IRQ_TYPE_EDGE_RISING>,
>>> +                <GIC_SPI 366 IRQ_TYPE_EDGE_RISING>,
>>> +                <GIC_SPI 367 IRQ_TYPE_EDGE_RISING>,
>>> +                <GIC_SPI 368 IRQ_TYPE_EDGE_RISING>,
>>> +                <GIC_SPI 369 IRQ_TYPE_EDGE_RISING>;
>>> +        };
>>> +
>>
>> The rest of the node looks fairly straight-forward.
> 
> You should probably have some "bus" and "iface" clocks too, per the 
> requirement of "qcom,smmu-v2". Maybe Vivek might know what's relevant 
> for MSM8998?
> 

The clocks that power the SMMU are not under the control of Linux, but 
rather the RPM subsystem.  The interface to them is the interconnect 
framework, which does not yet support 8998 per my understanding.  The 
clocks will always be on, but perhaps not at the best rate for 
performance until the interconnect is brought in.

-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v1 2/3] arm64: dts: qcom: msm8998: Add PCIe SMMU node

[Vivek Gautam]

Hi Marc,

On Fri, Mar 29, 2019 at 11:59 PM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 29/03/2019 10:51, Marc Gonzalez wrote:
> > On 28/03/2019 18:05, Marc Gonzalez wrote:
> >
> >> ANOC1 SMMU filters PCIe accesses.
> >
> > I'm not sure this description is entirely accurate...
> >
> > ANOC likely stands for "Application Network-On-Chip"

How about simply saying - "Add device node for ANOC1 SMMU" for
commit title.
Commit text -
ANOC1 smmu services a list of peripherals - USB, UFS, BLSP2, and PCIe.
Add the device node for this smmu.

> >
> >
> >>   arch/arm64/boot/dts/qcom/msm8998.dtsi | 15 +++++++++++++++
> >>   1 file changed, 15 insertions(+)
> >>
> >> diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi
> >> index f9a922fdae75..5a1c0961b281 100644
> >> --- a/arch/arm64/boot/dts/qcom/msm8998.dtsi
> >> +++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi
> >> @@ -606,6 +606,21 @@
> >>                      #thermal-sensor-cells = <1>;
> >>              };
> >>
> >> +            anoc1_smmu: arm,smmu@1680000 {
> >> +                    compatible = "qcom,msm8996-smmu-v2", "qcom,smmu-v2";
> >
> > Maybe I should instead use "qcom,msm8998-smmu-v2", "qcom,smmu-v2";
> > and define "qcom,msm8998-smmu-v2" in
> > Documentation/devicetree/bindings/iommu/arm,smmu.txt ?
>
> Yes please.
>
> > (Would the Documentation change need to be in a separate patch?)
>
> That's generally preferred, yes.
>
> >
> >> +                    reg = <0x01680000 0x10000>;
> >> +                    #iommu-cells = <1>;
> >
> > I'm not sure about this property. IIRC, Robin said <0> is not valid,
> > but I don't have any iommus prop, only iommu-map.
>
> You have to join the dots between the various bindings a little, but the
> iommu-base part of each iommu-map entry is an IOMMU specifier, and IOMMU
> specifiers are #iommu-cells long.
>
> To cut a long story short, 1 is definitely appropriate, because
> arm-smmu's definition of a 2-cell specifier wouldn't make much sense in
> the iommu-map context (and the current code for parsing iommu-map
> actually just assumes 1 anyway).

Besides this,
Looking at the SID mappings for ANOC1 smmu, devices such as USB, and UFS
can very well enable iommu support, and thus iommu-cells = 1 seems
like the correct thingy.

>
> >> +
> >> +                    #global-interrupts = <0>;
> >> +                    interrupts =
> >> +                            <GIC_SPI 364 IRQ_TYPE_EDGE_RISING>,
> >> +                            <GIC_SPI 365 IRQ_TYPE_EDGE_RISING>,
> >> +                            <GIC_SPI 366 IRQ_TYPE_EDGE_RISING>,
> >> +                            <GIC_SPI 367 IRQ_TYPE_EDGE_RISING>,
> >> +                            <GIC_SPI 368 IRQ_TYPE_EDGE_RISING>,
> >> +                            <GIC_SPI 369 IRQ_TYPE_EDGE_RISING>;
> >> +            };
> >> +
> >
> > The rest of the node looks fairly straight-forward.
>
> You should probably have some "bus" and "iface" clocks too, per the
> requirement of "qcom,smmu-v2". Maybe Vivek might know what's relevant
> for MSM8998?

As Jeffrey rightly mentioned, these clocks are not under the control of Linux.
So, we won't need to add clocks to this SMMU.

Thanks
Vivek

>
> >
> > DT code was adapted from:
> >
> > https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/arch/arm/boot/dts/qcom/msm-arm-smmu-8998.dtsi?h=LE.UM.1.3.r3.25#n18
> >
> > I left out the so-called implementation-defined init:
> >
> > https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/arch/arm/boot/dts/qcom/msm-arm-smmu-impl-defs-8998.dtsi?h=LE.UM.1.3.r3.25#n123
> >
> > Should I try to merge this part in mainline?
> > (I don't have any documentation for it though.)
>
> "pls no :("
>
> I'm not sure what route the path takes from "DT describes the platform"
> to get to "DT lists opaque register addresses and magic data to write
> into them", but I imagine it might involve getting hit in the head along
> the way.
>
> Robin.
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu



-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v1 2/3] arm64: dts: qcom: msm8998: Add PCIe SMMU node

[Robin Murphy]

On 30/03/2019 14:18, Vivek Gautam wrote:
>> You should probably have some "bus" and "iface" clocks too, per the
>> requirement of "qcom,smmu-v2". Maybe Vivek might know what's relevant
>> for MSM8998?
> 
> As Jeffrey rightly mentioned, these clocks are not under the control of Linux.
> So, we won't need to add clocks to this SMMU.

OK, in that case the "clock-names" part of binding doc probably wants 
refining to reflect which implementations do actually require clocks.

Robin.

Re: [PATCH v1 2/3] arm64: dts: qcom: msm8998: Add PCIe SMMU node

[Vivek Gautam]

On 4/2/2019 7:24 PM, Robin Murphy wrote:
> On 30/03/2019 14:18, Vivek Gautam wrote:
>>> You should probably have some "bus" and "iface" clocks too, per the
>>> requirement of "qcom,smmu-v2". Maybe Vivek might know what's relevant
>>> for MSM8998?
>>
>> As Jeffrey rightly mentioned, these clocks are not under the control 
>> of Linux.
>> So, we won't need to add clocks to this SMMU.
>
> OK, in that case the "clock-names" part of binding doc probably wants 
> refining to reflect which implementations do actually require clocks.

Certainly.
Marc, do you want to push a patch for the same? Or, let me know I can 
prepare one.

Thanks
Vivek

>
> Robin.

[PATCH v1 3/3] arm64: dts: qcom: msm8998: Add PCIe PHY and RC nodes

[Marc Gonzalez]

Add MSM8998 PCIe QMP PHY and PCIe root complex DT nodes.

Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr>
---
 arch/arm64/boot/dts/qcom/msm8998.dtsi | 78 +++++++++++++++++++++++++++
 1 file changed, 78 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi
index 5a1c0961b281..9f979a51f679 100644
--- a/arch/arm64/boot/dts/qcom/msm8998.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi
@@ -621,6 +621,84 @@
 				<GIC_SPI 369 IRQ_TYPE_EDGE_RISING>;
 		};
 
+		pcie0: pci@1c00000 {
+			compatible = "qcom,pcie-msm8996";
+			reg-names = "parf", "dbi", "elbi", "config";
+			reg =	<0x01c00000 0x2000>,
+				<0x1b000000 0xf1d>,
+				<0x1b000f20 0xa8>,
+				<0x1b100000 0x100000>;
+			device_type = "pci";
+			linux,pci-domain = <0>;
+			bus-range = <0x00 0xff>;
+			#address-cells = <3>;
+			#size-cells = <2>;
+			power-domains = <&gcc PCIE_0_GDSC>;
+
+			num-lanes = <1>;
+			phy-names = "pciephy";
+			phys = <&pciephy>;
+
+			ranges =
+			/*** downstream I/O ***/
+			<0x01000000 0x0 0x1b200000 0x1b200000 0x0 0x100000>,
+			/*** non-prefetchable memory ***/
+			<0x02000000 0x0 0x1b300000 0x1b300000 0x0 0xd00000>;
+
+			#interrupt-cells = <1>;
+			interrupt-names = "msi";
+			interrupts = <GIC_SPI 405 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-map-mask = <0 0 0 0x7>;
+			interrupt-map =
+				<0 0 0 1 &intc 0 135 IRQ_TYPE_LEVEL_HIGH>, /* int_a */
+				<0 0 0 2 &intc 0 136 IRQ_TYPE_LEVEL_HIGH>, /* int_b */
+				<0 0 0 3 &intc 0 138 IRQ_TYPE_LEVEL_HIGH>, /* int_c */
+				<0 0 0 4 &intc 0 139 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
+
+			clock-names = "pipe", "bus_master", "bus_slave", "cfg", "aux";
+			clocks =
+				<&gcc GCC_PCIE_0_PIPE_CLK>,
+				<&gcc GCC_PCIE_0_MSTR_AXI_CLK>,
+				<&gcc GCC_PCIE_0_SLV_AXI_CLK>,
+				<&gcc GCC_PCIE_0_CFG_AHB_CLK>,
+				<&gcc GCC_PCIE_0_AUX_CLK>;
+
+			iommu-map = <0x100 &anoc1_smmu 0x1480 1>;
+
+			/* PCIe Fundamental Reset */
+			perst-gpios = <&tlmm 35 GPIO_ACTIVE_LOW>;
+		};
+
+		phy@1c06000 {
+			compatible = "qcom,msm8998-qmp-pcie-phy";
+			reg = <0x01c06000 0x18c>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges;
+
+			clock-names = "aux", "cfg_ahb", "ref";
+			clocks =
+				<&gcc GCC_PCIE_PHY_AUX_CLK>,
+				<&gcc GCC_PCIE_0_CFG_AHB_CLK>,
+				<&gcc GCC_PCIE_CLKREF_CLK>;
+
+			reset-names = "phy", "common";
+			resets = <&gcc GCC_PCIE_0_PHY_BCR>, <&gcc GCC_PCIE_PHY_BCR>;
+
+			vdda-phy-supply = <&vreg_l1a_0p875>;
+			vdda-pll-supply = <&vreg_l2a_1p2>;
+
+			pciephy: lane@1c06800 {
+				reg = <0x01c06200 0x128>, <0x01c06400 0x1fc>, <0x01c06800 0x20c>;
+				#phy-cells = <0>;
+
+				clock-names = "pipe0";
+				clocks = <&gcc GCC_PCIE_0_PIPE_CLK>;
+				clock-output-names = "pcie_0_pipe_clk_src";
+				#clock-cells = <0>;
+			};
+		};
+
 		tcsr_mutex_regs: syscon@1f40000 {
 			compatible = "syscon";
 			reg = <0x1f40000 0x20000>;
-- 
2.17.1

Re: [PATCH v1 3/3] arm64: dts: qcom: msm8998: Add PCIe PHY and RC nodes

[Marc Gonzalez]

On 28/03/2019 18:06, Marc Gonzalez wrote:

> Add MSM8998 PCIe QMP PHY and PCIe root complex DT nodes.

I suppose I should add a reference to the downstream DTS:

https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/arch/arm/boot/dts/qcom/msm8998.dtsi?h=LE.UM.1.3.r3.25#n2537


> diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi
> index 5a1c0961b281..9f979a51f679 100644
> --- a/arch/arm64/boot/dts/qcom/msm8998.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi
> @@ -621,6 +621,84 @@
>  				<GIC_SPI 369 IRQ_TYPE_EDGE_RISING>;
>  		};
>  
> +		pcie0: pci@1c00000 {
> +			compatible = "qcom,pcie-msm8996";

Should probably be "qcom,pcie-msm8998"; in case we need to fudge something
in 8998 and not in 8996.

Apart from this minor issue, I think it's good to go for 5.2
(I'll respin without the SMMU patch)

I just need Kishon to take the PHY series, and Stephen to take the clock hack,
and we're rolling.

Oh and it would be useful to have someone review the PCIE20_PARF_BDF_TRANSLATE_N
patch.

Regards.

Re: [PATCH v1 3/3] arm64: dts: qcom: msm8998: Add PCIe PHY and RC nodes

[Stanimir Varbanov]

Hi Marc,

Few comments inline.

On 3/28/19 7:06 PM, Marc Gonzalez wrote:
> Add MSM8998 PCIe QMP PHY and PCIe root complex DT nodes.
> 
> Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr>
> ---
>  arch/arm64/boot/dts/qcom/msm8998.dtsi | 78 +++++++++++++++++++++++++++
>  1 file changed, 78 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi
> index 5a1c0961b281..9f979a51f679 100644
> --- a/arch/arm64/boot/dts/qcom/msm8998.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi
> @@ -621,6 +621,84 @@
>  				<GIC_SPI 369 IRQ_TYPE_EDGE_RISING>;
>  		};
>  
> +		pcie0: pci@1c00000 {
> +			compatible = "qcom,pcie-msm8996";
> +			reg-names = "parf", "dbi", "elbi", "config";
> +			reg =	<0x01c00000 0x2000>,
> +				<0x1b000000 0xf1d>,
> +				<0x1b000f20 0xa8>,
> +				<0x1b100000 0x100000>;

could you please populate reg property and after that reg-names property.

> +			device_type = "pci";
> +			linux,pci-domain = <0>;
> +			bus-range = <0x00 0xff>;
> +			#address-cells = <3>;
> +			#size-cells = <2>;
> +			power-domains = <&gcc PCIE_0_GDSC>;
> +
> +			num-lanes = <1>;
> +			phy-names = "pciephy";
> +			phys = <&pciephy>;
> +
> +			ranges =
> +			/*** downstream I/O ***/

could you make this a standard kernel comment or completely drop it

> +			<0x01000000 0x0 0x1b200000 0x1b200000 0x0 0x100000>,
> +			/*** non-prefetchable memory ***/

ditto

> +			<0x02000000 0x0 0x1b300000 0x1b300000 0x0 0xd00000>;
> +
> +			#interrupt-cells = <1>;
> +			interrupt-names = "msi";
> +			interrupts = <GIC_SPI 405 IRQ_TYPE_LEVEL_HIGH>;
> +			interrupt-map-mask = <0 0 0 0x7>;
> +			interrupt-map =
> +				<0 0 0 1 &intc 0 135 IRQ_TYPE_LEVEL_HIGH>, /* int_a */

move this to a line upper

> +				<0 0 0 2 &intc 0 136 IRQ_TYPE_LEVEL_HIGH>, /* int_b */
> +				<0 0 0 3 &intc 0 138 IRQ_TYPE_LEVEL_HIGH>, /* int_c */
> +				<0 0 0 4 &intc 0 139 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
> +
> +			clock-names = "pipe", "bus_master", "bus_slave", "cfg", "aux";
> +			clocks =
> +				<&gcc GCC_PCIE_0_PIPE_CLK>,

move this to a line upper

> +				<&gcc GCC_PCIE_0_MSTR_AXI_CLK>,
> +				<&gcc GCC_PCIE_0_SLV_AXI_CLK>,
> +				<&gcc GCC_PCIE_0_CFG_AHB_CLK>,
> +				<&gcc GCC_PCIE_0_AUX_CLK>;

please swap the order clocks and clock-names

> +
> +			iommu-map = <0x100 &anoc1_smmu 0x1480 1>;

iommu-map-mask? It is optional but I had to ask :)

> +
> +			/* PCIe Fundamental Reset */

this comment is useless :) please drop it

> +			perst-gpios = <&tlmm 35 GPIO_ACTIVE_LOW>;
> +		};
> +
> +		phy@1c06000 {
> +			compatible = "qcom,msm8998-qmp-pcie-phy";
> +			reg = <0x01c06000 0x18c>;
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			ranges;
> +
> +			clock-names = "aux", "cfg_ahb", "ref";
> +			clocks =
> +				<&gcc GCC_PCIE_PHY_AUX_CLK>,
> +				<&gcc GCC_PCIE_0_CFG_AHB_CLK>,
> +				<&gcc GCC_PCIE_CLKREF_CLK>;\

please, swap the order of clocks and clock-names, and move first clock a
line upper. Also delete '\' symbol at the end of last line.

> +
> +			reset-names = "phy", "common";
> +			resets = <&gcc GCC_PCIE_0_PHY_BCR>, <&gcc GCC_PCIE_PHY_BCR>;

resets prop and after that reset-names, please.

> +
> +			vdda-phy-supply = <&vreg_l1a_0p875>;
> +			vdda-pll-supply = <&vreg_l2a_1p2>;
> +
> +			pciephy: lane@1c06800 {
> +				reg = <0x01c06200 0x128>, <0x01c06400 0x1fc>, <0x01c06800 0x20c>;
> +				#phy-cells = <0>;
> +
> +				clock-names = "pipe0";
> +				clocks = <&gcc GCC_PCIE_0_PIPE_CLK>;

please, swap clocks and clock-names

> +				clock-output-names = "pcie_0_pipe_clk_src";
> +				#clock-cells = <0>;
> +			};
> +		};
> +
>  		tcsr_mutex_regs: syscon@1f40000 {
>  			compatible = "syscon";
>  			reg = <0x1f40000 0x20000>;
> 

-- 
regards,
Stan

Re: [PATCH v1 3/3] arm64: dts: qcom: msm8998: Add PCIe PHY and RC nodes

[Marc Gonzalez]

On 10/04/2019 17:32, Stanimir Varbanov wrote:

> Few comments inline.

I'll send v3.

Changes:
	- Move all X-names props *after* corresponding X(s) prop
	- Drop comments


>> +			iommu-map = <0x100 &anoc1_smmu 0x1480 1>;
> 
> iommu-map-mask? It is optional but I had to ask :)

The only RID in the system is 0x100.

# lspci
00:00.0 PCI bridge: Qualcomm Device 0105
01:00.0 Ethernet controller: Qualcomm Atheros AR8151 v2.0 Gigabit Ethernet (rev c0)

Since we just want to map 0x100, we don't need an iommu-map-mask.


>> +			/* PCIe Fundamental Reset */
> 
> this comment is useless :) please drop it

IMO, "perst" is a poor name. Can you guess what it stands for?

Regards.

Re: [PATCH v1 3/3] arm64: dts: qcom: msm8998: Add PCIe PHY and RC nodes

[Stanimir Varbanov]

Hi Marc,

On 4/11/19 11:44 AM, Marc Gonzalez wrote:
> On 10/04/2019 17:32, Stanimir Varbanov wrote:
> 
>> Few comments inline.
> 
> I'll send v3.
> 
> Changes:
> 	- Move all X-names props *after* corresponding X(s) prop
> 	- Drop comments
> 
> 
>>> +			iommu-map = <0x100 &anoc1_smmu 0x1480 1>;
>>
>> iommu-map-mask? It is optional but I had to ask :)
> 
> The only RID in the system is 0x100.
> 
> # lspci
> 00:00.0 PCI bridge: Qualcomm Device 0105
> 01:00.0 Ethernet controller: Qualcomm Atheros AR8151 v2.0 Gigabit Ethernet (rev c0)
> 
> Since we just want to map 0x100, we don't need an iommu-map-mask.

Do you see warnings during boot about missing property?

> 
> 
>>> +			/* PCIe Fundamental Reset */
>>
>> this comment is useless :) please drop it
> 
> IMO, "perst" is a poor name. Can you guess what it stands for?

The name is got from PCIE base specification. See 6.6.1.
Conventional Reset from "PCI EXPRESS BASE SPECIFICATION, REV. 3.0"

-- 
regards,
Stan

Re: [PATCH v1 3/3] arm64: dts: qcom: msm8998: Add PCIe PHY and RC nodes

[Marc Gonzalez]

On 11/04/2019 11:09, Stanimir Varbanov wrote:

> On 4/11/19 11:44 AM, Marc Gonzalez wrote:
> 
>> Since we just want to map 0x100, we don't need an iommu-map-mask.
> 
> Do you see warnings during boot about missing property?

Absent iommu-map-mask property is expected. No warning:

https://elixir.bootlin.com/linux/latest/source/drivers/of/base.c#L2245

	/* The default is to select all bits. */
	map_mask = 0xffffffff;

	/*
	 * Can be overridden by "{iommu,msi}-map-mask" property.
	 * If of_property_read_u32() fails, the default is used.
	 */
	if (map_mask_name)
		of_property_read_u32(np, map_mask_name, &map_mask);


>>>> +			/* PCIe Fundamental Reset */
>>>
>>> this comment is useless :) please drop it
>>
>> IMO, "perst" is a poor name. Can you guess what it stands for?
> 
> The name is got from PCIE base specification. See 6.6.1.
> Conventional Reset from "PCI EXPRESS BASE SPECIFICATION, REV. 3.0"

Indeed... Well, "perst" still sucks :-p

Regards.

Re: [PATCH v1 0/3] PCIe and AR8151 on APQ8098/MSM8998

[Lorenzo Pieralisi]

On Thu, Mar 28, 2019 at 05:59:48PM +0100, Marc Gonzalez wrote:
> Hello everyone,
> 
> After a lot of poking, I am finally able to use the AR8151 ethernet on the APQ8098 board.
> The magic bits are the iommu-map prop and the PCIE20_PARF_BDF_TRANSLATE_N setup.
> 
> The WIP thread is archived here:
> https://marc.info/?t=155059539200004&r=1&w=2
> 
> 
> Marc Gonzalez (3):
>   PCI: qcom: Setup PCIE20_PARF_BDF_TRANSLATE_N
>   arm64: dts: qcom: msm8998: Add PCIe SMMU node
>   arm64: dts: qcom: msm8998: Add PCIe PHY and RC nodes
> 
>  arch/arm64/boot/dts/qcom/msm8998.dtsi  | 93 ++++++++++++++++++++++++++
>  drivers/pci/controller/dwc/pcie-qcom.c |  4 ++
>  2 files changed, 97 insertions(+)

Marc,

what's the plan with this series ? Please let me know so that
I can handle it correctly in the PCI patch queue, I am not
sure by reading comments it has evolved much since posting.

Thanks,
Lorenzo

Re: [PATCH v1 0/3] PCIe and AR8151 on APQ8098/MSM8998

[Marc Gonzalez]

[ Trimming recipients list ]

On 11/06/2019 17:55, Lorenzo Pieralisi wrote:

> On Thu, Mar 28, 2019 at 05:59:48PM +0100, Marc Gonzalez wrote:
> 
>> After a lot of poking, I am finally able to use the AR8151 ethernet on the APQ8098 board.
>> The magic bits are the iommu-map prop and the PCIE20_PARF_BDF_TRANSLATE_N setup.
>>
>> The WIP thread is archived here:
>> https://marc.info/?t=155059539200004&r=1&w=2
>>
>>
>> Marc Gonzalez (3):
>>   PCI: qcom: Setup PCIE20_PARF_BDF_TRANSLATE_N
>>   arm64: dts: qcom: msm8998: Add PCIe SMMU node
>>   arm64: dts: qcom: msm8998: Add PCIe PHY and RC nodes
>>
>>  arch/arm64/boot/dts/qcom/msm8998.dtsi  | 93 ++++++++++++++++++++++++++
>>  drivers/pci/controller/dwc/pcie-qcom.c |  4 ++
>>  2 files changed, 97 insertions(+)
> 
> Marc,
> 
> what's the plan with this series ? Please let me know so that
> I can handle it correctly in the PCI patch queue, I am not
> sure by reading comments it has evolved much since posting.

Hello Lorenzo,

You can ignore/drop this series, it has been superseded; FWIW, the latest
patches no longer touch drivers/pci

The hold-up was finding an acceptable work-around for the $&#! bug in qcom's
SMMU emulation code. I hope Joerg will push it ASAP into linux-next so it can
receive some testing before 5.3-rc1.

The current incarnation of the PCIe PHY and RC nodes is:
https://patchwork.kernel.org/patch/10895341/

Regards.