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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux