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

On Mon, May 24, 2021 at 12:58 PM Bjorn Helgaas [off-list ref] 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:

quoted

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.

quoted

Signed-off-by: Yuji Ishikawa <redacted>
Signed-off-by: Nobuhiro Iwamatsu <redacted>
---
 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

quoted

+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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel