[PATCH v4 0/2] Add IPROC PCIe new features

[PATCH v4 0/2] Add IPROC PCIe new features

[Srinath Mannam]

This patch set extends support of new IPROC PCIe host controller features
 - Add CRS check using controller register status flags
 - Add outbound window mapping configuration for 32-bit I/O region

This patch set is based on Linux-5.0-rc2.

Changes from v3:
  - Addressed Lorenzo Pieralisi comments.

Changes from v2:
  - Based on Lorenzo Pieralisi comments, commit logs are expanded.

Changes from v1:
  - Addressed Bjorn Helgaas comments.
  - Removed set order mode patch from patchset.

Srinath Mannam (2):
  PCI: iproc: Add CRS check in config read
  PCI: iproc: Add outbound configuration for 32-bit I/O region

 drivers/pci/controller/pcie-iproc.c | 44 +++++++++++++++++++++++++++++++++----
 1 file changed, 40 insertions(+), 4 deletions(-)

-- 
2.7.4

[PATCH v4 0/2] Add IPROC PCIe new features

[Srinath Mannam]

This patch set extends support of new IPROC PCIe host controller features
 - Add CRS check using controller register status flags
 - Add outbound window mapping configuration for 32-bit I/O region

This patch set is based on Linux-5.0-rc2.

Changes from v3:
  - Addressed Lorenzo Pieralisi comments.

Changes from v2:
  - Based on Lorenzo Pieralisi comments, commit logs are expanded.

Changes from v1:
  - Addressed Bjorn Helgaas comments.
  - Removed set order mode patch from patchset.

Srinath Mannam (2):
  PCI: iproc: Add CRS check in config read
  PCI: iproc: Add outbound configuration for 32-bit I/O region

 drivers/pci/controller/pcie-iproc.c | 44 +++++++++++++++++++++++++++++++++----
 1 file changed, 40 insertions(+), 4 deletions(-)

-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 1/2] PCI: iproc: Add CRS check in config read

[Srinath Mannam]

In the current implementation, config read output data 0xffff0001 is
assumed as CRS completion. But sometimes 0xffff0001 can be a valid data.

IPROC PCIe host controller PAXB v2 has a register to show config read
status flags like SC, UR, CRS and CA. So that extra check is added to
confirm the CRS using status flags before reissue config read.

Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>
---
 drivers/pci/controller/pcie-iproc.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c
index c20fd6b..b882255 100644
--- a/drivers/pci/controller/pcie-iproc.c
+++ b/drivers/pci/controller/pcie-iproc.c
@@ -60,6 +60,10 @@
 #define APB_ERR_EN_SHIFT		0
 #define APB_ERR_EN			BIT(APB_ERR_EN_SHIFT)
 
+#define CFG_RD_SUCCESS			0
+#define CFG_RD_UR			1
+#define CFG_RD_CRS			2
+#define CFG_RD_CA			3
 #define CFG_RETRY_STATUS		0xffff0001
 #define CFG_RETRY_STATUS_TIMEOUT_US	500000 /* 500 milliseconds */
 
@@ -289,6 +293,9 @@ enum iproc_pcie_reg {
 	IPROC_PCIE_IARR4,
 	IPROC_PCIE_IMAP4,
 
+	/* config read status */
+	IPROC_PCIE_CFG_RD_STATUS,
+
 	/* link status */
 	IPROC_PCIE_LINK_STATUS,
 
@@ -350,6 +357,7 @@ static const u16 iproc_pcie_reg_paxb_v2[] = {
 	[IPROC_PCIE_IMAP3]		= 0xe08,
 	[IPROC_PCIE_IARR4]		= 0xe68,
 	[IPROC_PCIE_IMAP4]		= 0xe70,
+	[IPROC_PCIE_CFG_RD_STATUS]	= 0xee0,
 	[IPROC_PCIE_LINK_STATUS]	= 0xf0c,
 	[IPROC_PCIE_APB_ERR_EN]		= 0xf40,
 };
@@ -474,10 +482,12 @@ static void __iomem *iproc_pcie_map_ep_cfg_reg(struct iproc_pcie *pcie,
 	return (pcie->base + offset);
 }
 
-static unsigned int iproc_pcie_cfg_retry(void __iomem *cfg_data_p)
+static unsigned int iproc_pcie_cfg_retry(struct iproc_pcie *pcie,
+					 void __iomem *cfg_data_p)
 {
 	int timeout = CFG_RETRY_STATUS_TIMEOUT_US;
 	unsigned int data;
+	u32 status;
 
 	/*
 	 * As per PCIe spec r3.1, sec 2.3.2, CRS Software Visibility only
@@ -498,6 +508,15 @@ static unsigned int iproc_pcie_cfg_retry(void __iomem *cfg_data_p)
 	 */
 	data = readl(cfg_data_p);
 	while (data == CFG_RETRY_STATUS && timeout--) {
+		/*
+		 * CRS state is set in CFG_RD status register
+		 * This will handle the case where CFG_RETRY_STATUS is
+		 * valid config data.
+		 */
+		status = iproc_pcie_read_reg(pcie, IPROC_PCIE_CFG_RD_STATUS);
+		if (status != CFG_RD_CRS)
+			return data;
+
 		udelay(1);
 		data = readl(cfg_data_p);
 	}
@@ -576,7 +595,7 @@ static int iproc_pcie_config_read(struct pci_bus *bus, unsigned int devfn,
 	if (!cfg_data_p)
 		return PCIBIOS_DEVICE_NOT_FOUND;
 
-	data = iproc_pcie_cfg_retry(cfg_data_p);
+	data = iproc_pcie_cfg_retry(pcie, cfg_data_p);
 
 	*val = data;
 	if (size <= 2)
-- 
2.7.4

[PATCH v4 1/2] PCI: iproc: Add CRS check in config read

[Srinath Mannam]

In the current implementation, config read output data 0xffff0001 is
assumed as CRS completion. But sometimes 0xffff0001 can be a valid data.

IPROC PCIe host controller PAXB v2 has a register to show config read
status flags like SC, UR, CRS and CA. So that extra check is added to
confirm the CRS using status flags before reissue config read.

Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>
---
 drivers/pci/controller/pcie-iproc.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c
index c20fd6b..b882255 100644
--- a/drivers/pci/controller/pcie-iproc.c
+++ b/drivers/pci/controller/pcie-iproc.c
@@ -60,6 +60,10 @@
 #define APB_ERR_EN_SHIFT		0
 #define APB_ERR_EN			BIT(APB_ERR_EN_SHIFT)
 
+#define CFG_RD_SUCCESS			0
+#define CFG_RD_UR			1
+#define CFG_RD_CRS			2
+#define CFG_RD_CA			3
 #define CFG_RETRY_STATUS		0xffff0001
 #define CFG_RETRY_STATUS_TIMEOUT_US	500000 /* 500 milliseconds */
 
@@ -289,6 +293,9 @@ enum iproc_pcie_reg {
 	IPROC_PCIE_IARR4,
 	IPROC_PCIE_IMAP4,
 
+	/* config read status */
+	IPROC_PCIE_CFG_RD_STATUS,
+
 	/* link status */
 	IPROC_PCIE_LINK_STATUS,
 
@@ -350,6 +357,7 @@ static const u16 iproc_pcie_reg_paxb_v2[] = {
 	[IPROC_PCIE_IMAP3]		= 0xe08,
 	[IPROC_PCIE_IARR4]		= 0xe68,
 	[IPROC_PCIE_IMAP4]		= 0xe70,
+	[IPROC_PCIE_CFG_RD_STATUS]	= 0xee0,
 	[IPROC_PCIE_LINK_STATUS]	= 0xf0c,
 	[IPROC_PCIE_APB_ERR_EN]		= 0xf40,
 };
@@ -474,10 +482,12 @@ static void __iomem *iproc_pcie_map_ep_cfg_reg(struct iproc_pcie *pcie,
 	return (pcie->base + offset);
 }
 
-static unsigned int iproc_pcie_cfg_retry(void __iomem *cfg_data_p)
+static unsigned int iproc_pcie_cfg_retry(struct iproc_pcie *pcie,
+					 void __iomem *cfg_data_p)
 {
 	int timeout = CFG_RETRY_STATUS_TIMEOUT_US;
 	unsigned int data;
+	u32 status;
 
 	/*
 	 * As per PCIe spec r3.1, sec 2.3.2, CRS Software Visibility only
@@ -498,6 +508,15 @@ static unsigned int iproc_pcie_cfg_retry(void __iomem *cfg_data_p)
 	 */
 	data = readl(cfg_data_p);
 	while (data == CFG_RETRY_STATUS && timeout--) {
+		/*
+		 * CRS state is set in CFG_RD status register
+		 * This will handle the case where CFG_RETRY_STATUS is
+		 * valid config data.
+		 */
+		status = iproc_pcie_read_reg(pcie, IPROC_PCIE_CFG_RD_STATUS);
+		if (status != CFG_RD_CRS)
+			return data;
+
 		udelay(1);
 		data = readl(cfg_data_p);
 	}
@@ -576,7 +595,7 @@ static int iproc_pcie_config_read(struct pci_bus *bus, unsigned int devfn,
 	if (!cfg_data_p)
 		return PCIBIOS_DEVICE_NOT_FOUND;
 
-	data = iproc_pcie_cfg_retry(cfg_data_p);
+	data = iproc_pcie_cfg_retry(pcie, cfg_data_p);
 
 	*val = data;
 	if (size <= 2)
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Srinath Mannam]

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

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

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

[Srinath Mannam]

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

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 0/2] Add IPROC PCIe new features

[Srinath Mannam]

Hi Lorenzo/Bjorn,

Could you please help to review this patch series when you have time?
I believe I have addressed all your review comments.

Thanks,
Srinath.

On Fri, Mar 1, 2019 at 10:22 AM Srinath Mannam
<srinath.mannam@broadcom.com> wrote:
>
> This patch set extends support of new IPROC PCIe host controller features
>  - Add CRS check using controller register status flags
>  - Add outbound window mapping configuration for 32-bit I/O region
>
> This patch set is based on Linux-5.0-rc2.
>
> Changes from v3:
>   - Addressed Lorenzo Pieralisi comments.
>
> Changes from v2:
>   - Based on Lorenzo Pieralisi comments, commit logs are expanded.
>
> Changes from v1:
>   - Addressed Bjorn Helgaas comments.
>   - Removed set order mode patch from patchset.
>
> Srinath Mannam (2):
>   PCI: iproc: Add CRS check in config read
>   PCI: iproc: Add outbound configuration for 32-bit I/O region
>
>  drivers/pci/controller/pcie-iproc.c | 44 +++++++++++++++++++++++++++++++++----
>  1 file changed, 40 insertions(+), 4 deletions(-)
>
> --
> 2.7.4
>

Re: [PATCH v4 0/2] Add IPROC PCIe new features

[Srinath Mannam]

Hi Lorenzo/Bjorn,

Could you please help to review this patch series when you have time?
I believe I have addressed all your review comments.

Thanks,
Srinath.

On Fri, Mar 1, 2019 at 10:22 AM Srinath Mannam
<srinath.mannam@broadcom.com> wrote:
>
> This patch set extends support of new IPROC PCIe host controller features
>  - Add CRS check using controller register status flags
>  - Add outbound window mapping configuration for 32-bit I/O region
>
> This patch set is based on Linux-5.0-rc2.
>
> Changes from v3:
>   - Addressed Lorenzo Pieralisi comments.
>
> Changes from v2:
>   - Based on Lorenzo Pieralisi comments, commit logs are expanded.
>
> Changes from v1:
>   - Addressed Bjorn Helgaas comments.
>   - Removed set order mode patch from patchset.
>
> Srinath Mannam (2):
>   PCI: iproc: Add CRS check in config read
>   PCI: iproc: Add outbound configuration for 32-bit I/O region
>
>  drivers/pci/controller/pcie-iproc.c | 44 +++++++++++++++++++++++++++++++++----
>  1 file changed, 40 insertions(+), 4 deletions(-)
>
> --
> 2.7.4
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 0/2] Add IPROC PCIe new features

[Lorenzo Pieralisi]

On Wed, Mar 27, 2019 at 02:08:46PM +0530, Srinath Mannam wrote:
> Hi Lorenzo/Bjorn,
> 
> Could you please help to review this patch series when you have time?
> I believe I have addressed all your review comments.

I will shortly keeping in mind that you should really ask HW designers
to get to grips with a proper design because there are ways too many HW
quirks this driver has to fix-up, it has become an unmanageable
collection of work arounds.

Lorenzo

> Thanks,
> Srinath.
> 
> On Fri, Mar 1, 2019 at 10:22 AM Srinath Mannam
> <srinath.mannam@broadcom.com> wrote:
> >
> > This patch set extends support of new IPROC PCIe host controller features
> >  - Add CRS check using controller register status flags
> >  - Add outbound window mapping configuration for 32-bit I/O region
> >
> > This patch set is based on Linux-5.0-rc2.
> >
> > Changes from v3:
> >   - Addressed Lorenzo Pieralisi comments.
> >
> > Changes from v2:
> >   - Based on Lorenzo Pieralisi comments, commit logs are expanded.
> >
> > Changes from v1:
> >   - Addressed Bjorn Helgaas comments.
> >   - Removed set order mode patch from patchset.
> >
> > Srinath Mannam (2):
> >   PCI: iproc: Add CRS check in config read
> >   PCI: iproc: Add outbound configuration for 32-bit I/O region
> >
> >  drivers/pci/controller/pcie-iproc.c | 44 +++++++++++++++++++++++++++++++++----
> >  1 file changed, 40 insertions(+), 4 deletions(-)
> >
> > --
> > 2.7.4
> >

Re: [PATCH v4 0/2] Add IPROC PCIe new features

[Lorenzo Pieralisi]

On Wed, Mar 27, 2019 at 02:08:46PM +0530, Srinath Mannam wrote:
> Hi Lorenzo/Bjorn,
> 
> Could you please help to review this patch series when you have time?
> I believe I have addressed all your review comments.

I will shortly keeping in mind that you should really ask HW designers
to get to grips with a proper design because there are ways too many HW
quirks this driver has to fix-up, it has become an unmanageable
collection of work arounds.

Lorenzo

> Thanks,
> Srinath.
> 
> On Fri, Mar 1, 2019 at 10:22 AM Srinath Mannam
> <srinath.mannam@broadcom.com> wrote:
> >
> > This patch set extends support of new IPROC PCIe host controller features
> >  - Add CRS check using controller register status flags
> >  - Add outbound window mapping configuration for 32-bit I/O region
> >
> > This patch set is based on Linux-5.0-rc2.
> >
> > Changes from v3:
> >   - Addressed Lorenzo Pieralisi comments.
> >
> > Changes from v2:
> >   - Based on Lorenzo Pieralisi comments, commit logs are expanded.
> >
> > Changes from v1:
> >   - Addressed Bjorn Helgaas comments.
> >   - Removed set order mode patch from patchset.
> >
> > Srinath Mannam (2):
> >   PCI: iproc: Add CRS check in config read
> >   PCI: iproc: Add outbound configuration for 32-bit I/O region
> >
> >  drivers/pci/controller/pcie-iproc.c | 44 +++++++++++++++++++++++++++++++++----
> >  1 file changed, 40 insertions(+), 4 deletions(-)
> >
> > --
> > 2.7.4
> >

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 0/2] Add IPROC PCIe new features

[Srinath Mannam]

Hi Lorenzo,

Thanks for feedback, I will talk to our HW engineer.

Regards,
Srinath.

On Wed, Mar 27, 2019 at 6:01 PM Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> On Wed, Mar 27, 2019 at 02:08:46PM +0530, Srinath Mannam wrote:
> > Hi Lorenzo/Bjorn,
> >
> > Could you please help to review this patch series when you have time?
> > I believe I have addressed all your review comments.
>
> I will shortly keeping in mind that you should really ask HW designers
> to get to grips with a proper design because there are ways too many HW
> quirks this driver has to fix-up, it has become an unmanageable
> collection of work arounds.
>
> Lorenzo
>
> > Thanks,
> > Srinath.
> >
> > On Fri, Mar 1, 2019 at 10:22 AM Srinath Mannam
> > <srinath.mannam@broadcom.com> wrote:
> > >
> > > This patch set extends support of new IPROC PCIe host controller features
> > >  - Add CRS check using controller register status flags
> > >  - Add outbound window mapping configuration for 32-bit I/O region
> > >
> > > This patch set is based on Linux-5.0-rc2.
> > >
> > > Changes from v3:
> > >   - Addressed Lorenzo Pieralisi comments.
> > >
> > > Changes from v2:
> > >   - Based on Lorenzo Pieralisi comments, commit logs are expanded.
> > >
> > > Changes from v1:
> > >   - Addressed Bjorn Helgaas comments.
> > >   - Removed set order mode patch from patchset.
> > >
> > > Srinath Mannam (2):
> > >   PCI: iproc: Add CRS check in config read
> > >   PCI: iproc: Add outbound configuration for 32-bit I/O region
> > >
> > >  drivers/pci/controller/pcie-iproc.c | 44 +++++++++++++++++++++++++++++++++----
> > >  1 file changed, 40 insertions(+), 4 deletions(-)
> > >
> > > --
> > > 2.7.4
> > >

Re: [PATCH v4 0/2] Add IPROC PCIe new features

[Srinath Mannam]

Hi Lorenzo,

Thanks for feedback, I will talk to our HW engineer.

Regards,
Srinath.

On Wed, Mar 27, 2019 at 6:01 PM Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> On Wed, Mar 27, 2019 at 02:08:46PM +0530, Srinath Mannam wrote:
> > Hi Lorenzo/Bjorn,
> >
> > Could you please help to review this patch series when you have time?
> > I believe I have addressed all your review comments.
>
> I will shortly keeping in mind that you should really ask HW designers
> to get to grips with a proper design because there are ways too many HW
> quirks this driver has to fix-up, it has become an unmanageable
> collection of work arounds.
>
> Lorenzo
>
> > Thanks,
> > Srinath.
> >
> > On Fri, Mar 1, 2019 at 10:22 AM Srinath Mannam
> > <srinath.mannam@broadcom.com> wrote:
> > >
> > > This patch set extends support of new IPROC PCIe host controller features
> > >  - Add CRS check using controller register status flags
> > >  - Add outbound window mapping configuration for 32-bit I/O region
> > >
> > > This patch set is based on Linux-5.0-rc2.
> > >
> > > Changes from v3:
> > >   - Addressed Lorenzo Pieralisi comments.
> > >
> > > Changes from v2:
> > >   - Based on Lorenzo Pieralisi comments, commit logs are expanded.
> > >
> > > Changes from v1:
> > >   - Addressed Bjorn Helgaas comments.
> > >   - Removed set order mode patch from patchset.
> > >
> > > Srinath Mannam (2):
> > >   PCI: iproc: Add CRS check in config read
> > >   PCI: iproc: Add outbound configuration for 32-bit I/O region
> > >
> > >  drivers/pci/controller/pcie-iproc.c | 44 +++++++++++++++++++++++++++++++++----
> > >  1 file changed, 40 insertions(+), 4 deletions(-)
> > >
> > > --
> > > 2.7.4
> > >

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 0/2] Add IPROC PCIe new features

[Lorenzo Pieralisi]

On Fri, Mar 01, 2019 at 10:22:14AM +0530, Srinath Mannam wrote:
> This patch set extends support of new IPROC PCIe host controller features
>  - Add CRS check using controller register status flags
>  - Add outbound window mapping configuration for 32-bit I/O region
> 
> This patch set is based on Linux-5.0-rc2.
> 
> Changes from v3:
>   - Addressed Lorenzo Pieralisi comments.
> 
> Changes from v2:
>   - Based on Lorenzo Pieralisi comments, commit logs are expanded.
> 
> Changes from v1:
>   - Addressed Bjorn Helgaas comments.
>   - Removed set order mode patch from patchset.
> 
> Srinath Mannam (2):
>   PCI: iproc: Add CRS check in config read
>   PCI: iproc: Add outbound configuration for 32-bit I/O region
> 
>  drivers/pci/controller/pcie-iproc.c | 44 +++++++++++++++++++++++++++++++++----

I need Ray a/o Scott ACK to merge this series, thanks.

Lorenzo

Re: [PATCH v4 0/2] Add IPROC PCIe new features

[Lorenzo Pieralisi]

On Fri, Mar 01, 2019 at 10:22:14AM +0530, Srinath Mannam wrote:
> This patch set extends support of new IPROC PCIe host controller features
>  - Add CRS check using controller register status flags
>  - Add outbound window mapping configuration for 32-bit I/O region
> 
> This patch set is based on Linux-5.0-rc2.
> 
> Changes from v3:
>   - Addressed Lorenzo Pieralisi comments.
> 
> Changes from v2:
>   - Based on Lorenzo Pieralisi comments, commit logs are expanded.
> 
> Changes from v1:
>   - Addressed Bjorn Helgaas comments.
>   - Removed set order mode patch from patchset.
> 
> Srinath Mannam (2):
>   PCI: iproc: Add CRS check in config read
>   PCI: iproc: Add outbound configuration for 32-bit I/O region
> 
>  drivers/pci/controller/pcie-iproc.c | 44 +++++++++++++++++++++++++++++++++----

I need Ray a/o Scott ACK to merge this series, thanks.

Lorenzo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 0/2] Add IPROC PCIe new features

[Scott Branden]

On 2019-03-29 9:51 a.m., Lorenzo Pieralisi wrote:
> On Fri, Mar 01, 2019 at 10:22:14AM +0530, Srinath Mannam wrote:
>> This patch set extends support of new IPROC PCIe host controller features
>>   - Add CRS check using controller register status flags
>>   - Add outbound window mapping configuration for 32-bit I/O region
>>
>> This patch set is based on Linux-5.0-rc2.
>>
>> Changes from v3:
>>    - Addressed Lorenzo Pieralisi comments.
>>
>> Changes from v2:
>>    - Based on Lorenzo Pieralisi comments, commit logs are expanded.
>>
>> Changes from v1:
>>    - Addressed Bjorn Helgaas comments.
>>    - Removed set order mode patch from patchset.
>>
>> Srinath Mannam (2):
>>    PCI: iproc: Add CRS check in config read
>>    PCI: iproc: Add outbound configuration for 32-bit I/O region
>>
>>   drivers/pci/controller/pcie-iproc.c | 44 +++++++++++++++++++++++++++++++++----
> I need Ray a/o Scott ACK to merge this series, thanks.

Unfortunately we need this due to hardware design (which we continue to 
inform the silicon team to provide a proper design).


Acked-by: Scott Branden <scott.branden@broadcom.com>
>
> Lorenzo

Re: [PATCH v4 0/2] Add IPROC PCIe new features

[Scott Branden]

On 2019-03-29 9:51 a.m., Lorenzo Pieralisi wrote:
> On Fri, Mar 01, 2019 at 10:22:14AM +0530, Srinath Mannam wrote:
>> This patch set extends support of new IPROC PCIe host controller features
>>   - Add CRS check using controller register status flags
>>   - Add outbound window mapping configuration for 32-bit I/O region
>>
>> This patch set is based on Linux-5.0-rc2.
>>
>> Changes from v3:
>>    - Addressed Lorenzo Pieralisi comments.
>>
>> Changes from v2:
>>    - Based on Lorenzo Pieralisi comments, commit logs are expanded.
>>
>> Changes from v1:
>>    - Addressed Bjorn Helgaas comments.
>>    - Removed set order mode patch from patchset.
>>
>> Srinath Mannam (2):
>>    PCI: iproc: Add CRS check in config read
>>    PCI: iproc: Add outbound configuration for 32-bit I/O region
>>
>>   drivers/pci/controller/pcie-iproc.c | 44 +++++++++++++++++++++++++++++++++----
> I need Ray a/o Scott ACK to merge this series, thanks.

Unfortunately we need this due to hardware design (which we continue to 
inform the silicon team to provide a proper design).


Acked-by: Scott Branden <scott.branden@broadcom.com>
>
> Lorenzo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Lorenzo Pieralisi]

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.

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
> 

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

[Lorenzo Pieralisi]

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.

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
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Srinath Mannam]

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.
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
> >

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

[Srinath Mannam]

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.
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
> >

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Lorenzo Pieralisi]

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
> > >

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

[Lorenzo Pieralisi]

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
> > >

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Ray Jui]

Hi Lorenzo/Srinath,

I look at the commit message again and indeed it looks quite confusing.

I'll add my comment inline in the code section. I hope that will help to
make it more clear.

On 4/1/2019 9:44 AM, Lorenzo Pieralisi wrote:
> 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
>>>> +                      */

I think the code comment above is much more clear than the commit
message. It looks like the commit message was to describe a particular
use case that prompts the code change. However, if I remember correctly,
during our internal review, I already made some modification to the code
to make the change much more generic than dealing with a special use
case. This patch contains the generic change I made.

Basically, 'size' is the intended outbound mapping size (from DT or ACPI
or whatever, it does not really matter). 'window_size' is a specific
mapping window size our PCIe controller can support. Our PCIe controller
supports a fixed set of outbound mapping window sizes. I don't think
this is much different from some other PCIe controllers.

It looks like, there are cases where one cannot find an exact match
between 'size' and 'window_size'. In this case, and when we know we have
already exhausted all possible mapping windows, i.e., 'size_idx' == 0
AND 'window_idx' == 0, we take a compromise by programming the outbound
mapping by using a window size that's actually larger than the 'size'.

Srinath, please correct me if I'm wrong. But I carefully reviewed the
code again and I believe this is essentially what it is.

If so, then the commit message is quite misleading and can be changed to
above descriptions.

>>>> +                     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
>>>>

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

[Ray Jui]

Hi Lorenzo/Srinath,

I look at the commit message again and indeed it looks quite confusing.

I'll add my comment inline in the code section. I hope that will help to
make it more clear.

On 4/1/2019 9:44 AM, Lorenzo Pieralisi wrote:
> 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
>>>> +                      */

I think the code comment above is much more clear than the commit
message. It looks like the commit message was to describe a particular
use case that prompts the code change. However, if I remember correctly,
during our internal review, I already made some modification to the code
to make the change much more generic than dealing with a special use
case. This patch contains the generic change I made.

Basically, 'size' is the intended outbound mapping size (from DT or ACPI
or whatever, it does not really matter). 'window_size' is a specific
mapping window size our PCIe controller can support. Our PCIe controller
supports a fixed set of outbound mapping window sizes. I don't think
this is much different from some other PCIe controllers.

It looks like, there are cases where one cannot find an exact match
between 'size' and 'window_size'. In this case, and when we know we have
already exhausted all possible mapping windows, i.e., 'size_idx' == 0
AND 'window_idx' == 0, we take a compromise by programming the outbound
mapping by using a window size that's actually larger than the 'size'.

Srinath, please correct me if I'm wrong. But I carefully reviewed the
code again and I believe this is essentially what it is.

If so, then the commit message is quite misleading and can be changed to
above descriptions.

>>>> +                     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
>>>>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Srinath Mannam]

Hi Ray,

Thanks for detailed explanation.
Please see some more details below.

On Tue, Apr 2, 2019 at 3:33 AM Ray Jui <ray.jui@broadcom.com> wrote:
>
> Hi Lorenzo/Srinath,
>
> I look at the commit message again and indeed it looks quite confusing.
>
> I'll add my comment inline in the code section. I hope that will help to
> make it more clear.
>
> On 4/1/2019 9:44 AM, Lorenzo Pieralisi wrote:
> > 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
> >>>> +                      */
>
> I think the code comment above is much more clear than the commit
> message. It looks like the commit message was to describe a particular
> use case that prompts the code change. However, if I remember correctly,
> during our internal review, I already made some modification to the code
> to make the change much more generic than dealing with a special use
> case. This patch contains the generic change I made.
>
> Basically, 'size' is the intended outbound mapping size (from DT or ACPI
> or whatever, it does not really matter). 'window_size' is a specific
> mapping window size our PCIe controller can support. Our PCIe controller
> supports a fixed set of outbound mapping window sizes. I don't think
> this is much different from some other PCIe controllers.
>
> It looks like, there are cases where one cannot find an exact match
> between 'size' and 'window_size'. In this case, and when we know we have
> already exhausted all possible mapping windows, i.e., 'size_idx' == 0
> AND 'window_idx' == 0, we take a compromise by programming the outbound
> mapping by using a window size that's actually larger than the 'size'.
>
> Srinath, please correct me if I'm wrong. But I carefully reviewed the
> code again and I believe this is essentially what it is.
>
128MB is the minimum size of window available to configure outbound
memory. But size of 32bit AXI address region
of our controller is 32MB. Also this 32bit AXI address region has to
configure in OARR0. so that 32bit AXI address region
is configured in 128MB window of OARR0.
this is the reason in the below to allow ob window configure only when
size_idx is 0 which means window size 128MB
and window_idx is 0 means OARR0 else continue using (size_idx > 0 ||
window_idx > 0) condition.

Regards,
Srinath.
> If so, then the commit message is quite misleading and can be changed to
> above descriptions.
>
> >>>> +                     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
> >>>>

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

[Srinath Mannam]

Hi Ray,

Thanks for detailed explanation.
Please see some more details below.

On Tue, Apr 2, 2019 at 3:33 AM Ray Jui <ray.jui@broadcom.com> wrote:
>
> Hi Lorenzo/Srinath,
>
> I look at the commit message again and indeed it looks quite confusing.
>
> I'll add my comment inline in the code section. I hope that will help to
> make it more clear.
>
> On 4/1/2019 9:44 AM, Lorenzo Pieralisi wrote:
> > 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
> >>>> +                      */
>
> I think the code comment above is much more clear than the commit
> message. It looks like the commit message was to describe a particular
> use case that prompts the code change. However, if I remember correctly,
> during our internal review, I already made some modification to the code
> to make the change much more generic than dealing with a special use
> case. This patch contains the generic change I made.
>
> Basically, 'size' is the intended outbound mapping size (from DT or ACPI
> or whatever, it does not really matter). 'window_size' is a specific
> mapping window size our PCIe controller can support. Our PCIe controller
> supports a fixed set of outbound mapping window sizes. I don't think
> this is much different from some other PCIe controllers.
>
> It looks like, there are cases where one cannot find an exact match
> between 'size' and 'window_size'. In this case, and when we know we have
> already exhausted all possible mapping windows, i.e., 'size_idx' == 0
> AND 'window_idx' == 0, we take a compromise by programming the outbound
> mapping by using a window size that's actually larger than the 'size'.
>
> Srinath, please correct me if I'm wrong. But I carefully reviewed the
> code again and I believe this is essentially what it is.
>
128MB is the minimum size of window available to configure outbound
memory. But size of 32bit AXI address region
of our controller is 32MB. Also this 32bit AXI address region has to
configure in OARR0. so that 32bit AXI address region
is configured in 128MB window of OARR0.
this is the reason in the below to allow ob window configure only when
size_idx is 0 which means window size 128MB
and window_idx is 0 means OARR0 else continue using (size_idx > 0 ||
window_idx > 0) condition.

Regards,
Srinath.
> If so, then the commit message is quite misleading and can be changed to
> above descriptions.
>
> >>>> +                     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
> >>>>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Lorenzo Pieralisi]

On Tue, Apr 02, 2019 at 03:20:21PM +0530, Srinath Mannam wrote:
> Hi Ray,
> 
> Thanks for detailed explanation.
> Please see some more details below.
> 
> On Tue, Apr 2, 2019 at 3:33 AM Ray Jui <ray.jui@broadcom.com> wrote:
> >
> > Hi Lorenzo/Srinath,
> >
> > I look at the commit message again and indeed it looks quite confusing.
> >
> > I'll add my comment inline in the code section. I hope that will help to
> > make it more clear.
> >
> > On 4/1/2019 9:44 AM, Lorenzo Pieralisi wrote:
> > > 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
> > >>>> +                      */
> >
> > I think the code comment above is much more clear than the commit
> > message. It looks like the commit message was to describe a particular
> > use case that prompts the code change. However, if I remember correctly,
> > during our internal review, I already made some modification to the code
> > to make the change much more generic than dealing with a special use
> > case. This patch contains the generic change I made.
> >
> > Basically, 'size' is the intended outbound mapping size (from DT or ACPI
> > or whatever, it does not really matter). 'window_size' is a specific
> > mapping window size our PCIe controller can support. Our PCIe controller
> > supports a fixed set of outbound mapping window sizes. I don't think
> > this is much different from some other PCIe controllers.
> >
> > It looks like, there are cases where one cannot find an exact match
> > between 'size' and 'window_size'. In this case, and when we know we have
> > already exhausted all possible mapping windows, i.e., 'size_idx' == 0
> > AND 'window_idx' == 0, we take a compromise by programming the outbound
> > mapping by using a window size that's actually larger than the 'size'.
> >
> > Srinath, please correct me if I'm wrong. But I carefully reviewed the
> > code again and I believe this is essentially what it is.
> >
> 128MB is the minimum size of window available to configure outbound
> memory. But size of 32bit AXI address region
> of our controller is 32MB. Also this 32bit AXI address region has to
> configure in OARR0. so that 32bit AXI address region
> is configured in 128MB window of OARR0.
> this is the reason in the below to allow ob window configure only when
> size_idx is 0 which means window size 128MB
> and window_idx is 0 means OARR0 else continue using (size_idx > 0 ||
> window_idx > 0) condition.

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.

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).

Please clarify.

Thanks,
Lorenzo

> Regards,
> Srinath.
> > If so, then the commit message is quite misleading and can be changed to
> > above descriptions.
> >
> > >>>> +                     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
> > >>>>

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

[Lorenzo Pieralisi]

On Tue, Apr 02, 2019 at 03:20:21PM +0530, Srinath Mannam wrote:
> Hi Ray,
> 
> Thanks for detailed explanation.
> Please see some more details below.
> 
> On Tue, Apr 2, 2019 at 3:33 AM Ray Jui <ray.jui@broadcom.com> wrote:
> >
> > Hi Lorenzo/Srinath,
> >
> > I look at the commit message again and indeed it looks quite confusing.
> >
> > I'll add my comment inline in the code section. I hope that will help to
> > make it more clear.
> >
> > On 4/1/2019 9:44 AM, Lorenzo Pieralisi wrote:
> > > 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
> > >>>> +                      */
> >
> > I think the code comment above is much more clear than the commit
> > message. It looks like the commit message was to describe a particular
> > use case that prompts the code change. However, if I remember correctly,
> > during our internal review, I already made some modification to the code
> > to make the change much more generic than dealing with a special use
> > case. This patch contains the generic change I made.
> >
> > Basically, 'size' is the intended outbound mapping size (from DT or ACPI
> > or whatever, it does not really matter). 'window_size' is a specific
> > mapping window size our PCIe controller can support. Our PCIe controller
> > supports a fixed set of outbound mapping window sizes. I don't think
> > this is much different from some other PCIe controllers.
> >
> > It looks like, there are cases where one cannot find an exact match
> > between 'size' and 'window_size'. In this case, and when we know we have
> > already exhausted all possible mapping windows, i.e., 'size_idx' == 0
> > AND 'window_idx' == 0, we take a compromise by programming the outbound
> > mapping by using a window size that's actually larger than the 'size'.
> >
> > Srinath, please correct me if I'm wrong. But I carefully reviewed the
> > code again and I believe this is essentially what it is.
> >
> 128MB is the minimum size of window available to configure outbound
> memory. But size of 32bit AXI address region
> of our controller is 32MB. Also this 32bit AXI address region has to
> configure in OARR0. so that 32bit AXI address region
> is configured in 128MB window of OARR0.
> this is the reason in the below to allow ob window configure only when
> size_idx is 0 which means window size 128MB
> and window_idx is 0 means OARR0 else continue using (size_idx > 0 ||
> window_idx > 0) condition.

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.

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).

Please clarify.

Thanks,
Lorenzo

> Regards,
> Srinath.
> > If so, then the commit message is quite misleading and can be changed to
> > above descriptions.
> >
> > >>>> +                     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
> > >>>>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Srinath Mannam]

Hi Lorenzo,

Please see my reply below,

On Tue, Apr 2, 2019 at 3:56 PM Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> On Tue, Apr 02, 2019 at 03:20:21PM +0530, Srinath Mannam wrote:
> > Hi Ray,
> >
> > Thanks for detailed explanation.
> > Please see some more details below.
> >
> > On Tue, Apr 2, 2019 at 3:33 AM Ray Jui <ray.jui@broadcom.com> wrote:
> > >
> > > Hi Lorenzo/Srinath,
> > >
> > > I look at the commit message again and indeed it looks quite confusing.
> > >
> > > I'll add my comment inline in the code section. I hope that will help to
> > > make it more clear.
> > >
> > > On 4/1/2019 9:44 AM, Lorenzo Pieralisi wrote:
> > > > 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
> > > >>>> +                      */
> > >
> > > I think the code comment above is much more clear than the commit
> > > message. It looks like the commit message was to describe a particular
> > > use case that prompts the code change. However, if I remember correctly,
> > > during our internal review, I already made some modification to the code
> > > to make the change much more generic than dealing with a special use
> > > case. This patch contains the generic change I made.
> > >
> > > Basically, 'size' is the intended outbound mapping size (from DT or ACPI
> > > or whatever, it does not really matter). 'window_size' is a specific
> > > mapping window size our PCIe controller can support. Our PCIe controller
> > > supports a fixed set of outbound mapping window sizes. I don't think
> > > this is much different from some other PCIe controllers.
> > >
> > > It looks like, there are cases where one cannot find an exact match
> > > between 'size' and 'window_size'. In this case, and when we know we have
> > > already exhausted all possible mapping windows, i.e., 'size_idx' == 0
> > > AND 'window_idx' == 0, we take a compromise by programming the outbound
> > > mapping by using a window size that's actually larger than the 'size'.
> > >
> > > Srinath, please correct me if I'm wrong. But I carefully reviewed the
> > > code again and I believe this is essentially what it is.
> > >
> > 128MB is the minimum size of window available to configure outbound
> > memory. But size of 32bit AXI address region
> > of our controller is 32MB. Also this 32bit AXI address region has to
> > configure in OARR0. so that 32bit AXI address region
> > is configured in 128MB window of OARR0.
> > this is the reason in the below to allow ob window configure only when
> > size_idx is 0 which means window size 128MB
> > and window_idx is 0 means OARR0 else continue using (size_idx > 0 ||
> > window_idx > 0) condition.
>
> 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,
        },
};

Regards,
Srinath.
>
> Please clarify.
>
> Thanks,
> Lorenzo
>
> > Regards,
> > Srinath.
> > > If so, then the commit message is quite misleading and can be changed to
> > > above descriptions.
> > >
> > > >>>> +                     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
> > > >>>>

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

[Srinath Mannam]

Hi Lorenzo,

Please see my reply below,

On Tue, Apr 2, 2019 at 3:56 PM Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> On Tue, Apr 02, 2019 at 03:20:21PM +0530, Srinath Mannam wrote:
> > Hi Ray,
> >
> > Thanks for detailed explanation.
> > Please see some more details below.
> >
> > On Tue, Apr 2, 2019 at 3:33 AM Ray Jui <ray.jui@broadcom.com> wrote:
> > >
> > > Hi Lorenzo/Srinath,
> > >
> > > I look at the commit message again and indeed it looks quite confusing.
> > >
> > > I'll add my comment inline in the code section. I hope that will help to
> > > make it more clear.
> > >
> > > On 4/1/2019 9:44 AM, Lorenzo Pieralisi wrote:
> > > > 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
> > > >>>> +                      */
> > >
> > > I think the code comment above is much more clear than the commit
> > > message. It looks like the commit message was to describe a particular
> > > use case that prompts the code change. However, if I remember correctly,
> > > during our internal review, I already made some modification to the code
> > > to make the change much more generic than dealing with a special use
> > > case. This patch contains the generic change I made.
> > >
> > > Basically, 'size' is the intended outbound mapping size (from DT or ACPI
> > > or whatever, it does not really matter). 'window_size' is a specific
> > > mapping window size our PCIe controller can support. Our PCIe controller
> > > supports a fixed set of outbound mapping window sizes. I don't think
> > > this is much different from some other PCIe controllers.
> > >
> > > It looks like, there are cases where one cannot find an exact match
> > > between 'size' and 'window_size'. In this case, and when we know we have
> > > already exhausted all possible mapping windows, i.e., 'size_idx' == 0
> > > AND 'window_idx' == 0, we take a compromise by programming the outbound
> > > mapping by using a window size that's actually larger than the 'size'.
> > >
> > > Srinath, please correct me if I'm wrong. But I carefully reviewed the
> > > code again and I believe this is essentially what it is.
> > >
> > 128MB is the minimum size of window available to configure outbound
> > memory. But size of 32bit AXI address region
> > of our controller is 32MB. Also this 32bit AXI address region has to
> > configure in OARR0. so that 32bit AXI address region
> > is configured in 128MB window of OARR0.
> > this is the reason in the below to allow ob window configure only when
> > size_idx is 0 which means window size 128MB
> > and window_idx is 0 means OARR0 else continue using (size_idx > 0 ||
> > window_idx > 0) condition.
>
> 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,
        },
};

Regards,
Srinath.
>
> Please clarify.
>
> Thanks,
> Lorenzo
>
> > Regards,
> > Srinath.
> > > If so, then the commit message is quite misleading and can be changed to
> > > above descriptions.
> > >
> > > >>>> +                     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
> > > >>>>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Lorenzo Pieralisi]

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.

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.

Thanks,
Lorenzo

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

[Lorenzo Pieralisi]

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.

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.

Thanks,
Lorenzo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Srinath Mannam]

Hi Lorenzo,

Please see my reply below,

On Tue, Apr 2, 2019 at 7:08 PM Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> 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.

Regards,
Srinath.
>
> Thanks,
> Lorenzo

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

[Srinath Mannam]

Hi Lorenzo,

Please see my reply below,

On Tue, Apr 2, 2019 at 7:08 PM Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> 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.

Regards,
Srinath.
>
> Thanks,
> Lorenzo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Lorenzo Pieralisi]

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@arm.com> 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

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

[Lorenzo Pieralisi]

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@arm.com> 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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Srinath Mannam]

Hi Lorenzo,

I am sorry, I took your long time. In my commit log I gave details
about purpose of feature instead of implementation.
Thanks a lot for all inputs and knowledge. I will remember and follow
these notes while writing commit log.
commit log re-written by you is very much impressive and have detailed
description of feature and implementation.

Thank again for you patience.

Regards,
Srinath.

On Wed, Apr 3, 2019 at 5:01 PM Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> 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@arm.com> 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

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

[Srinath Mannam]

Hi Lorenzo,

I am sorry, I took your long time. In my commit log I gave details
about purpose of feature instead of implementation.
Thanks a lot for all inputs and knowledge. I will remember and follow
these notes while writing commit log.
commit log re-written by you is very much impressive and have detailed
description of feature and implementation.

Thank again for you patience.

Regards,
Srinath.

On Wed, Apr 3, 2019 at 5:01 PM Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> 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@arm.com> 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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Ray Jui]

Hi Lorenzo,

On 4/3/2019 4:31 AM, Lorenzo Pieralisi wrote:
> 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@arm.com> 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.>

I reviewed the rephrased commit message by you in pci/iproc branch. It
looks very good to me. Thank you so much for helping with this!

Ray

> 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
> 

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

[Ray Jui]

Hi Lorenzo,

On 4/3/2019 4:31 AM, Lorenzo Pieralisi wrote:
> 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@arm.com> 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.>

I reviewed the rephrased commit message by you in pci/iproc branch. It
looks very good to me. Thank you so much for helping with this!

Ray

> 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
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel