Re: [PATCH 1/1] PCI: iproc: Invalidate PAXB address mapping before programming it

[Lorenzo Pieralisi]

On Thu, Oct 17, 2019 at 07:57:56PM +0530, Abhishek Shah wrote:
> Hi Lorenzo,
> 
> Please see my comments inline:
> 
> On Tue, Oct 15, 2019 at 10:13 PM Lorenzo Pieralisi
> <lorenzo.pieralisi@xxxxxxx> wrote:
> >
> > On Fri, Sep 06, 2019 at 09:28:13AM +0530, Abhishek Shah wrote:
> > > Invalidate PAXB inbound/outbound address mapping each time before
> > > programming it. This is helpful for the cases where we need to
> > > reprogram inbound/outbound address mapping without resetting PAXB.
> > > kexec kernel is one such example.
> >
> > This looks like a hack, explain to us please what it actually solves and
> > why a full reset is not necessary.
> >
> The PAXB IP performs address translation(PCI<->AXI address) for both inbound and
> outbound addresses (amongst other things) based on version of IP being used.
> It does so using the IMAP/IARR/OMAP/OARR registers.
> 
> These registers get programmed as per mappings specified in device tree during
> PCI driver probe for each RC and do not get reset when kexec/kdump kernel boots.
> This results in driver assuming valid mappings in place for some mapping windows
> during kexec/kdump kernel boot, consequently it skips those windows and
> we run out of available mapping windows, leading to mapping failure.
> 
> Normally, we take care of resetting PAXB block in firmware, but in
> primary kernel to kexec/kdump kernel handover, no firmware is executed
> in between.  So, we just, by default, invalidate the mapping registers
> each time before
> programming them to solve the issue described above..
> We do not need full reset for handling this.

I see. A simple bitmap to detect which windows are *actually*
programmed by the current kernel (that can be used by

iproc_pcie_ob_is_valid()

to carry out a valid check) would do as well instead of having to
invalidate all the OB registers.

It is up to you, let me know and I will merge code accordingly.

Lorenzo

> > > Signed-off-by: Abhishek Shah <abhishek.shah@xxxxxxxxxxxx>
> > > Reviewed-by: Ray Jui <ray.jui@xxxxxxxxxxxx>
> > > Reviewed-by: Vikram Mysore Prakash <vikram.prakash@xxxxxxxxxxxx>
> >
> > Patches are reviewed on public mailing lists, remove tags given
> > on internal reviews - they are not relevant.
> >
> Ok, will remove.
> 
> > > ---
> > >  drivers/pci/controller/pcie-iproc.c | 28 ++++++++++++++++++++++++++++
> > >  1 file changed, 28 insertions(+)
> > >
> > > diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c
> > > index e3ca46497470..99a9521ba7ab 100644
> > > --- a/drivers/pci/controller/pcie-iproc.c
> > > +++ b/drivers/pci/controller/pcie-iproc.c
> > > @@ -1245,6 +1245,32 @@ static int iproc_pcie_map_dma_ranges(struct iproc_pcie *pcie)
> > >       return ret;
> > >  }
> > >
> > > +static void iproc_pcie_invalidate_mapping(struct iproc_pcie *pcie)
> > > +{
> > > +     struct iproc_pcie_ib *ib = &pcie->ib;
> > > +     struct iproc_pcie_ob *ob = &pcie->ob;
> > > +     int idx;
> > > +
> > > +     if (pcie->ep_is_internal)
> >
> > What's this check for and why leaving mappings in place is safe for
> > this category of IPs ?
> For this category of IP(PAXC), no mappings need to be programmed in
> the first place.
> 
> >
> > > +             return;
> > > +
> > > +     if (pcie->need_ob_cfg) {
> > > +             /* iterate through all OARR mapping regions */
> > > +             for (idx = ob->nr_windows - 1; idx >= 0; idx--) {
> > > +                     iproc_pcie_write_reg(pcie,
> > > +                                          MAP_REG(IPROC_PCIE_OARR0, idx), 0);
> > > +             }
> > > +     }
> > > +
> > > +     if (pcie->need_ib_cfg) {
> > > +             /* iterate through all IARR mapping regions */
> > > +             for (idx = 0; idx < ib->nr_regions; idx++) {
> > > +                     iproc_pcie_write_reg(pcie,
> > > +                                          MAP_REG(IPROC_PCIE_IARR0, idx), 0);
> > > +             }
> > > +     }
> > > +}
> > > +
> > >  static int iproce_pcie_get_msi(struct iproc_pcie *pcie,
> > >                              struct device_node *msi_node,
> > >                              u64 *msi_addr)
> > > @@ -1517,6 +1543,8 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
> > >       iproc_pcie_perst_ctrl(pcie, true);
> > >       iproc_pcie_perst_ctrl(pcie, false);
> > >
> > > +     iproc_pcie_invalidate_mapping(pcie);
> >
> > It makes more sense to call this in the .shutdown() method if I
> > understand what it does.
> >
> It would work for kexec kernel, but not for kdump kernel as only for
> kexec'ed kernel,
> "device_shutdown" callback is present. We are here taking care of both the cases
> with this patch.
> 
> 
> Regards,
> Abhishek
> 
> > Lorenzo
> >
> > >       if (pcie->need_ob_cfg) {
> > >               ret = iproc_pcie_map_ranges(pcie, res);
> > >               if (ret) {
> > > --
> > > 2.17.1
> > >