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

[Nobuhiro Iwamatsu]

Hi,


Thanks for your comment.

On Wed, Jun 16, 2021 at 10:32:05AM -0600, Rob Herring wrote:
> 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.a

In this SoC specification, the CPU bus outputs the offset value from
0x40000000 to the PCIE bus, so 0x40000000 is subtracted from the CPU
bus address. This 0x40000000 is also based on io_base from DT.
Therefore, how about the following processing?

return cpu_addr - pp->io_base;

If I use a mask, it will be as follows.

return cpu_addr & ~pp->io_base;

Best regards,
  Nobuhiro