Re: [PATCH v4 2/2] PCI: iproc: Add outbound configuration for 32-bit I/O region

[Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>]

On Mon, Apr 01, 2019 at 11:04:48AM +0530, Srinath Mannam wrote:
> Hi Lorenzo,
> 
> Please see my reply below,
> 
> On Fri, Mar 29, 2019 at 11:06 PM Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> >
> > On Fri, Mar 01, 2019 at 10:22:16AM +0530, Srinath Mannam wrote:
> > > In the present driver outbound window configuration is done to map above
> > > 32-bit address I/O regions with corresponding PCI memory range given in
> > > ranges DT property.
> > >
> > > This patch add outbound window configuration to map below 32-bit I/O range
> > > with corresponding PCI memory, which helps to access I/O region in ARM
> > > 32-bit and one to one mapping of I/O region to PCI memory.
> > >
> > > Ex:
> > > 1. ranges DT property given for current driver is,
> > >     ranges = <0x83000000 0x0 0x40000000 0x4 0x00000000 0 0x40000000>;
> > >     I/O region address is 0x400000000
> > > 2. ranges DT property can be given after this patch,
> > >     ranges = <0x83000000 0x0 0x42000000 0x0 0x42000000 0 0x2000000>;
> > >     I/O region address is 0x42000000
> >
> > I was applying this patch but I don't understand the commit log and
> > how it matches the code, please explain it to me and I will reword
> > it.
> Iproc PCIe host controller supports outbound address translation
> feature to translate AXI address to PCI bus address.
> IO address ranges (AXI and PCI) given through ranges DT property have
> to program to controller outbound window registers.
> Present driver has the support for only 64bit AXI address. so that
> ranges DT property has given as 64bit AXI address and 32 bit
> PCI bus address.
> But with this patch 32-bit AXI address also could be programmed to
> Iproc host controller outbound window registers. so that ranges
> DT property can have 32bit AXI address which can map to 32-bit PCI bus address.

The code change seems to add a check for the window size, I see no
notion of 64 vs 32 bit addressing there so I am pretty sure there is
something you are not telling me that is implicit in the IProc outbound
window configuration, for instance why is the lowest index window
considered for 32-bit.

AFAICS you are adding code to allow a window whose size is < than
the lowest index in the ob_map array. How this relates to 64 vs
32 bit addresses is not clear but it should be.

When I read your commit log it is impossible to understand how it
correlates to the code you are changing - I still have not figured it
out myself.

Please explain in detail to me how this works, forget DT changes I
want to understand how HW works.

Lorenzo

> Example given in commit log is describing ranges DT property changes
> with and without this patch.
> In the case, without this patch AXI address is more than 32bit
> "0x400000000". and with this patch AXI address is 32-bit "0x42000000".
> PCI bus address is 32 bit address in both the cases "0x40000000" and 0x42000000.
> 
> Regards,
> Srinath.
> > Thanks,
> > Lorenzo
> >
> > > Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>
> > > Signed-off-by: Abhishek Shah <abhishek.shah@broadcom.com>
> > > Signed-off-by: Ray Jui <ray.jui@broadcom.com>
> > > ---
> > >  drivers/pci/controller/pcie-iproc.c | 21 +++++++++++++++++++--
> > >  1 file changed, 19 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c
> > > index b882255..080f142 100644
> > > --- a/drivers/pci/controller/pcie-iproc.c
> > > +++ b/drivers/pci/controller/pcie-iproc.c
> > > @@ -955,8 +955,25 @@ static int iproc_pcie_setup_ob(struct iproc_pcie *pcie, u64 axi_addr,
> > >                       resource_size_t window_size =
> > >                               ob_map->window_sizes[size_idx] * SZ_1M;
> > >
> > > -                     if (size < window_size)
> > > -                             continue;
> > > +                     /*
> > > +                      * Keep iterating until we reach the last window and
> > > +                      * with the minimal window size at index zero. In this
> > > +                      * case, we take a compromise by mapping it using the
> > > +                      * minimum window size that can be supported
> > > +                      */
> > > +                     if (size < window_size) {
> > > +                             if (size_idx > 0 || window_idx > 0)
> > > +                                     continue;
> > > +
> > > +                             /*
> > > +                              * For the corner case of reaching the minimal
> > > +                              * window size that can be supported on the
> > > +                              * last window
> > > +                              */
> > > +                             axi_addr = ALIGN_DOWN(axi_addr, window_size);
> > > +                             pci_addr = ALIGN_DOWN(pci_addr, window_size);
> > > +                             size = window_size;
> > > +                     }
> > >
> > >                       if (!IS_ALIGNED(axi_addr, window_size) ||
> > >                           !IS_ALIGNED(pci_addr, window_size)) {
> > > --
> > > 2.7.4
> > >