On Wed, Apr 03, 2019 at 08:41:44AM +0530, Srinath Mannam wrote: > Hi Lorenzo, > > Please see my reply below, > > On Tue, Apr 2, 2019 at 7:08 PM Lorenzo Pieralisi > <lorenzo.pieralisi@xxxxxxx> wrote: > > > > On Tue, Apr 02, 2019 at 04:16:13PM +0530, Srinath Mannam wrote: > > > > [...] > > > > > > Ok - I start to understand. What does it mean in HW terms that your > > > > 32bit AXI address region size is 32MB ? Please explain to me in details. > > > > > > > In our PCIe controller HW, AXI address from 0x42000000 to 0x44000000 > > > of 32MB size and . > > > AXI address from 0x400000000 to 0x480000000 of 2GB size are provided > > > to map ob address. > > > First IO region is inside 32bit address and second IO region is > > > outside 32bit address. > > > This code change is to map first IO region(0x42000000 to 0x44000000). > > > > > > > IIUC you are using an OARR0 of 128MB in size to map a 32MB address > > > > region, that's what I understand this patch does (and the lowest index > > > > corresponds to the smallest possible size - it is far from clear by > > > > looking at the patch). > > > Yes, lowest index corresponds to smallest possible size (128MB). > > > In our controller we have multiple windows like OARR0, OARR1, OARR2, > > > OARR3 all supports multiple sizes from 128MB to 1024MB. > > > These details are given at the top of this driver file, as shown > > > below. all windows supports 128MB size still we must use OARR0 window > > > to configure first IO region(0x42000000 to 0x44000000). > > > > > > static const struct iproc_pcie_ob_map paxb_v2_ob_map[] = { > > > { > > > /* OARR0/OMAP0 */ > > > .window_sizes = { 128, 256 }, > > > .nr_sizes = 2, > > > }, > > > { > > > /* OARR1/OMAP1 */ > > > .window_sizes = { 128, 256 }, > > > .nr_sizes = 2, > > > }, > > > { > > > /* OARR2/OMAP2 */ > > > .window_sizes = { 128, 256, 512, 1024 }, > > > .nr_sizes = 4, > > > }, > > > { > > > /* OARR3/OMAP3 */ > > > .window_sizes = { 128, 256, 512, 1024 }, > > > .nr_sizes = 4, > > > }, > > > }; > > > > Ok so this patch allows mapping an AXI I/O window that is smaller > > than OARR possible sizes, why it was not done from the beginning > > I really do not know. > > > Same Iproc driver we use for multiple SOCs, in previous SOCs does not > have 32-bit AXI address region to map ob. > In the present SOC, 32-bit AXI address region is available so that > this change is added. > > > Now explain this to me please: > > > > > 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 > > > > Why 1:1 AXI<->PCI address mapping is not possible in (1), how does the > > current code works on 32-bit systems and what's the benefit your change > > is bringing. > non-prefetchable memory range can only support 32-bit addresses, so > that we have taken 32-bit PCI bus address in (1). > current code does not work in 32-bit systems. In the present SOC with > this new change we can access from 32-bit CPU. Thank you. I rewrote the log and pushed patches to pci/iproc, please have a look (Ray/Scott please do have a look too) and report back if that's fine. Do you agree that the initial commit was lacking _significant_ information ? Please remember that the commit log plays a fundamental part in understanding a change and this one is a very important one so I am being pedantic on it. Thanks, Lorenzo