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

[Lorenzo Pieralisi]

On Thu, Oct 24, 2019 at 08:52:41AM +0530, Abhishek Shah wrote:
> On Mon, Oct 21, 2019 at 4:08 PM Lorenzo Pieralisi
> <lorenzo.pieralisi@xxxxxxx> wrote:
> >
> > 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.
> >
> Okay, so you are suggesting to use variable/bitmap to hold status of
> ib/ob windows (mapped/unmapped) instead of using registers to check
> it. Please note that we would still be programming corresponding
> window register to mark it valid (HW requirement).

It is your choice.

> @Ray, could you please provide feedback on this? I think existing way
> is proper for given driver design.
> 
> Also, as internal review tags are irrelevant as suggested by Lorenzo
> earlier, could you please put sign again once reviewed?

Yes please, repost (with the solution you prefer) and Ray please
tag accordingly on ML.

Thanks,
Lorenzo

> Regards,
> Abhishek
> 
> > 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
> > > > >