Re: [PATCH v3 2/3] PCI: Visconti: Add Toshiba Visconti PCIe host controller driver

[Rob Herring <robh+dt@kernel.org>]

On Mon, May 24, 2021 at 12:58 PM Bjorn Helgaas <helgaas@kernel.org> 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@toshiba.co.jp>
> > Signed-off-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>
> > ---
> >  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