[PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

[Arnd Bergmann]

On Wednesday 14 October 2015 17:44:11 Zhou Wang wrote:
> On 2015/10/14 17:06, Arnd Bergmann wrote:
> > On Wednesday 14 October 2015 16:59:03 Zhou Wang wrote:
> >>
> >> Hi Arnd,
> >>
> >> In Hip05 PCIe host, it uses GITS_TRANSLATER's address to get TLP package
> >> which contains MSI address and MSI data, and then combine BDF and MSI data
> >> to a 32 bit data which will be writen to GITS_TRANSLATER register of ITS.
> >>
> >> I think maybe this is a defect of our PCIe controller.
> > 
> > I'd consider it a bug in the firmware if this is not set up correctly
> > before boot.
> > 
> >>> I don't think what you do here is safe because the 'reg' property
> >>> of the MSI controller might point to the address that is used for
> >>> the message directly.
> >>
> >> I see your point, however we must get address of GITS_TRANSLATER and
> >> set it to PCIe host. How about adding necessary comments here?
> > 
> > This seems to just be static setup that should be done before Linux
> > is even loaded. Any reason you can't do it that way?
> >
> 
> There are some ITSs in Hip05-D02 platform, in fact, we can use any of them
> as a msi-controller,  which we can configure in dts. I am afraid that
> hard-setting the value in BIOS would lead to restrictions in terms of flexibility,
> as with the current implementation the same BIOS-driver can fit different
> DTS structures.

The dtb generally should be expected to match whatever the firmware sets up,
so if there is one reasonable setting here, I see no problem with hardcoding
it that way. In particular on server systems, we usually expect the firmware
to configure almost everything in advance and just tell us how it is
configured, while on embedded systems we can't trust the bootload and
usually set it all up in the kernel from scratch.

What would be a reason to pick one ITS over another?

On a related note, don't you also need to describe in DT how PCI B/D/F
function numbers get turned into addresses in the ITS? Does that also
require configuration in the driver? I see this code here:


+       hisi_pcie_apb_writel(pcie, PCIE_MSI_ASID_ENABLE | PCIE_MSI_ASID_VALUE,
+                            PCIE_SLV_MSI_ASID);
+       hisi_pcie_apb_writel(pcie, PCIE_MSI_TRANS_ENABLE, PCIE_MSI_TRANS_REG);
+       hisi_pcie_change_apb_mode(pcie, PCIE_SLV_DBI_MODE);

plus all of hisi_pcie_config_context(). This looks like it will change
the way the MSI is interpreted. This also seems like something that
could be done in the firmware in advance, and just get reported in DT.

	Arnd