On Mon, May 24, 2021 at 12:58 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
>
> [+cc Kishon for cpu_addr_fixup() question]
>
> Please make the subject "PCI: visconti: Add ..." since the driver
> names are usually lower-case. When referring to the hardware itself,
> use "Visconti", of course.
>
> On Mon, May 24, 2021 at 03:30:03PM +0900, Nobuhiro Iwamatsu wrote:
> > Add support to PCIe RC controller on Toshiba Visconti ARM SoCs. PCIe
> > controller is based of Synopsys DesignWare PCIe core.
> >
> > This patch does not yet use the clock framework to control the clock.
> > This will be replaced in the future.
> >
> > v2 -> v3:
> > - Update subject.
> > - Wrap description in 75 columns.
> > - Change config name to PCIE_VISCONTI_HOST.
> > - Update Kconfig text.
> > - Drop blank lines.
> > - Adjusted to 80 columns.
> > - Drop inline from functions for register access.
> > - Changed function name from visconti_pcie_check_link_status to
> > visconti_pcie_link_up.
> > - Update to using dw_pcie_host_init().
> > - Reorder these in the order of use in visconti_pcie_establish_link.
> > - Rewrite visconti_pcie_host_init() without dw_pcie_setup_rc().
> > - Change function name from visconti_device_turnon() to
> > visconti_pcie_power_on().
> > - Unify formats such as dev_err().
> > - Drop error label in visconti_add_pcie_port().
> >
> > v1 -> v2:
> > - Fix typo in commit message.
> > - Drop "depends on OF && HAS_IOMEM" from Kconfig.
> > - Stop using the pointer of struct dw_pcie.
> > - Use _relaxed variant.
> > - Drop dw_pcie_wait_for_link.
> > - Drop dbi resource processing.
> > - Drop MSI IRQ initialization processing.
>
> Thanks for the changelog. Please move it after the "---" line for
> future versions. That way it won't appear in the commit log when this
> is merged. The notes about v1->v2, v2->v3, etc are useful during
> review, but not after this is merged.
>
> > Signed-off-by: Yuji Ishikawa <yuji2.ishikawa@xxxxxxxxxxxxx>
> > Signed-off-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@xxxxxxxxxxxxx>
> > ---
> > drivers/pci/controller/dwc/Kconfig | 9 +
> > drivers/pci/controller/dwc/Makefile | 1 +
> > drivers/pci/controller/dwc/pcie-visconti.c | 369 +++++++++++++++++++++
> > 3 files changed, 379 insertions(+)
> > create mode 100644 drivers/pci/controller/dwc/pcie-visconti.c
> > +static u64 visconti_pcie_cpu_addr_fixup(struct dw_pcie *pci, u64 pci_addr)
> > +{
> > + return pci_addr - PCIE_BUS_OFFSET;
>
> This is called from __dw_pcie_prog_outbound_atu() as:
>
> cpu_addr = pci->ops->cpu_addr_fixup(pci, cpu_addr);
>
> so I think the parameter here should be *cpu_addr*, not pci_addr.
>
> dra7xx and artpec6 also call it "pci_addr", which is at best
> confusing.
>
> I'm also confused about exactly what .cpu_addr_fixup() does. Is it
> applying an offset that cannot be deduced from the DT description? If
> so, *should* this offset be described in DT?
It could be perhaps, but it would be a custom property, not something
we can handle in 'ranges'. I'd rather it be implicit from the
compatible than a custom property.
AIUI, the issue is the cpu address gets masked (high bits discarded).
This can happen when the internal bus address decoding throws away
upper address bits.
For example:
0xa0000000 -> 0x20000000 -> 0x00000000
cpu addr -> DW local addr -> PCI bus addr
DT has the first and last addresses, but iATU needs the middle and last address.
This could be just a data value rather than an ops function. While a
subtract works here, that's fragile (the DT needs to match the
#define) and I think a mask would be more appropriate.
Rob
