Skip to contentSkip to conversation

PCI: iproc: Set all 24 bits of PCI class code

PATCH In review Pali Rohár 6 participants 10 messages Jan 5, 2022 – Feb 14, 2022

Subsystems (3)
PCI PCI native host bridge and endpoint drivers Broadcom iproc Arm architecture
lore
Share & tools
lore

Pali Rohár

author
#0 lore raw

PATCH 1/1 PCI: iproc: Set all 24 bits of PCI class code

Register 0x43c in its low 24 bits contains PCI class code.

Update code to set all 24 bits of PCI class code and not only upper 16 bits of PCI class code.

Use a new macro PCI_CLASS_BRIDGE_PCI_NORMAL which represents whole 24 bits of normal PCI bridge class.

Signed-off-by Pali Rohár <[email protected]>

---
Roman helped me with this change and confirmed that class code is stored really in bits [23:0] of custom register 0x43c (normally class code is stored in bits [31:8] of pci register 0x08).

This patch depends on patch which adds PCI_CLASS_BRIDGE_PCI_NORMAL macro:
https://lore.kernel.org/linux-pci/[email protected]/

---
 drivers/pci/controller/pcie-iproc.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
drivers/pci/controller/pcie-iproc.c +4 -5
index 3df4ab209253..2519201b0e51 100644
@@ -789,14 +789,13 @@ static int iproc_pcie_check_link(struct iproc_pcie *pcie)
 		return -EFAULT;
 	}
 
-	/* force class to PCI_CLASS_BRIDGE_PCI (0x0604) */
+	/* force class to PCI_CLASS_BRIDGE_PCI_NORMAL (0x060400) */
 #define PCI_BRIDGE_CTRL_REG_OFFSET	0x43c
-#define PCI_CLASS_BRIDGE_MASK		0xffff00
-#define PCI_CLASS_BRIDGE_SHIFT		8
+#define PCI_BRIDGE_CTRL_REG_CLASS_MASK	0xffffff
 	iproc_pci_raw_config_read32(pcie, 0, PCI_BRIDGE_CTRL_REG_OFFSET,
 				    4, &class);
-	class &= ~PCI_CLASS_BRIDGE_MASK;
-	class |= (PCI_CLASS_BRIDGE_PCI << PCI_CLASS_BRIDGE_SHIFT);
+	class &= ~PCI_BRIDGE_CTRL_REG_CLASS_MASK;
+	class |= PCI_CLASS_BRIDGE_PCI_NORMAL;
 	iproc_pci_raw_config_write32(pcie, 0, PCI_BRIDGE_CTRL_REG_OFFSET,
 				     4, class);
Signature · 1 line
2.20.1
build bot
#1 lore raw
I love your patch! Yet something to improve:

Hi "Pali,

I love your patch! Yet something to improve:

[auto build test ERROR on helgaas-pci/next]
[also build test ERROR on v5.16-rc8 next-20220105]
[If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Pali-Roh-r/PCI-iproc-Set-all-24-bits-of-PCI-class-code/20220105-173704
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: arm-randconfig-c002-20220105 (https://download.01.org/0day-ci/archive/20220105/202201052225.XMsg2fG3-lkp(a)intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 11.2.0

reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/8ef1acfb84c08a0331930f9a60884fdd6d7c5e88
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Pali-Roh-r/PCI-iproc-Set-all-24-bits-of-PCI-class-code/20220105-173704 git checkout 8ef1acfb84c08a0331930f9a60884fdd6d7c5e88
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arm SHELL=/bin/bash drivers/pci/controller/

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

drivers/pci/controller/pcie-iproc.c: In function 'iproc_pcie_check_link':

>> drivers/pci/controller/pcie-iproc.c:798:18: error: 'PCI_CLASS_BRIDGE_PCI_NORMAL' undeclared (first use in this function); did you mean 'PCI_CLASS_BRIDGE_PCMCIA'?
     798 |         class |= PCI_CLASS_BRIDGE_PCI_NORMAL;
         |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
         |                  PCI_CLASS_BRIDGE_PCMCIA

drivers/pci/controller/pcie-iproc.c:798:18: note: each undeclared identifier is reported only once for each function it appears in

vim +798 drivers/pci/controller/pcie-iproc.c

765
766 static int iproc_pcie_check_link(struct iproc_pcie *pcie)
767 {
768 struct device *dev = pcie->dev;
769 u32 hdr_type, link_ctrl, link_status, class, val;
770 bool link_is_active = false;
771
772 /*
773 * PAXC connects to emulated endpoint devices directly and does not
774 * have a Serdes. Therefore skip the link detection logic here.
775 */
776 if (pcie->ep_is_internal)
777 return 0;
778
779 val = iproc_pcie_read_reg(pcie, IPROC_PCIE_LINK_STATUS);
780 if (!(val & PCIE_PHYLINKUP) || !(val & PCIE_DL_ACTIVE)) {
781 dev_err(dev, "PHY or data link is INACTIVE!\n");
782 return -ENODEV;
783 }
784
785 /* make sure we are not in EP mode */
786 iproc_pci_raw_config_read32(pcie, 0, PCI_HEADER_TYPE, 1, &hdr_type);
787 if ((hdr_type & 0x7f) != PCI_HEADER_TYPE_BRIDGE) {
788 dev_err(dev, "in EP mode, hdr=%#02x\n", hdr_type);
789 return -EFAULT;
790 }
791
792 /* force class to PCI_CLASS_BRIDGE_PCI_NORMAL (0x060400) */
793 #define PCI_BRIDGE_CTRL_REG_OFFSET 0x43c
794 #define PCI_BRIDGE_CTRL_REG_CLASS_MASK 0xffffff
795 iproc_pci_raw_config_read32(pcie, 0, PCI_BRIDGE_CTRL_REG_OFFSET,
796 4, &class);
797 class &= ~PCI_BRIDGE_CTRL_REG_CLASS_MASK;

 > 798		class |= PCI_CLASS_BRIDGE_PCI_NORMAL;

799 iproc_pci_raw_config_write32(pcie, 0, PCI_BRIDGE_CTRL_REG_OFFSET,
800 4, class);
801
802 /* check link status to see if link is active */
803 iproc_pci_raw_config_read32(pcie, 0, IPROC_PCI_EXP_CAP + PCI_EXP_LNKSTA,
804 2, &link_status);
805 if (link_status & PCI_EXP_LNKSTA_NLW)
806 link_is_active = true;
807
808 if (!link_is_active) {
809 /* try GEN 1 link speed */
810 #define PCI_TARGET_LINK_SPEED_MASK 0xf
811 #define PCI_TARGET_LINK_SPEED_GEN2 0x2
812 #define PCI_TARGET_LINK_SPEED_GEN1 0x1
813 iproc_pci_raw_config_read32(pcie, 0,
814 IPROC_PCI_EXP_CAP + PCI_EXP_LNKCTL2,
815 4, &link_ctrl);
816 if ((link_ctrl & PCI_TARGET_LINK_SPEED_MASK) ==
817 PCI_TARGET_LINK_SPEED_GEN2) {
818 link_ctrl &= ~PCI_TARGET_LINK_SPEED_MASK;
819 link_ctrl |= PCI_TARGET_LINK_SPEED_GEN1;
820 iproc_pci_raw_config_write32(pcie, 0,
821 IPROC_PCI_EXP_CAP + PCI_EXP_LNKCTL2,
822 4, link_ctrl);
823 msleep(100);
824
825 iproc_pci_raw_config_read32(pcie, 0,
826 IPROC_PCI_EXP_CAP + PCI_EXP_LNKSTA,
827 2, &link_status);
828 if (link_status & PCI_EXP_LNKSTA_NLW)
829 link_is_active = true;
830 }
831 }
832
833 dev_info(dev, "link: %s\n", link_is_active ? "UP" : "DOWN");
834
835 return link_is_active ? 0 : -ENODEV;
836 }
837

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Acked-by on v1 1/1
#2 lore raw
In reply to Pali Rohár
Quoted 46 lines · Pali Rohár

Register 0x43c in its low 24 bits contains PCI class code.

Update code to set all 24 bits of PCI class code and not only upper 16 bits of PCI class code.

Use a new macro PCI_CLASS_BRIDGE_PCI_NORMAL which represents whole 24 bits of normal PCI bridge class.

Signed-off-by Pali Rohár <[email protected]>

---
Roman helped me with this change and confirmed that class code is stored really in bits [23:0] of custom register 0x43c (normally class code is stored in bits [31:8] of pci register 0x08).

This patch depends on patch which adds PCI_CLASS_BRIDGE_PCI_NORMAL macro:
https://lore.kernel.org/linux-pci/[email protected]/

---
 drivers/pci/controller/pcie-iproc.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
drivers/pci/controller/pcie-iproc.c +4 -6
index 3df4ab209253..2519201b0e51 100644
@@ -789,14 +789,13 @@ static int iproc_pcie_check_link(struct iproc_pcie *pcie)
                return -EFAULT;
        }
 
-       /* force class to PCI_CLASS_BRIDGE_PCI (0x0604) */
+       /* force class to PCI_CLASS_BRIDGE_PCI_NORMAL (0x060400) */
 #define PCI_BRIDGE_CTRL_REG_OFFSET     0x43c
-#define PCI_CLASS_BRIDGE_MASK          0xffff00
-#define PCI_CLASS_BRIDGE_SHIFT         8
+#define PCI_BRIDGE_CTRL_REG_CLASS_MASK 0xffffff
        iproc_pci_raw_config_read32(pcie, 0, PCI_BRIDGE_CTRL_REG_OFFSET,
                                    4, &class);
-       class &= ~PCI_CLASS_BRIDGE_MASK;
-       class |= (PCI_CLASS_BRIDGE_PCI << PCI_CLASS_BRIDGE_SHIFT);
+       class &= ~PCI_BRIDGE_CTRL_REG_CLASS_MASK;
+       class |= PCI_CLASS_BRIDGE_PCI_NORMAL;
        iproc_pci_raw_config_write32(pcie, 0, PCI_BRIDGE_CTRL_REG_OFFSET,
                                     4, class);
 
--
Quoted 2 lines

2.20.1

Acked-by Roman Bacik <[email protected]>
Signature · 11 lines
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.
Acked-by on v1 1/1
#3 lore raw
In reply to Pali Rohár

Hi Pali,

Quoted 34 lines · Pali Rohár

Register 0x43c in its low 24 bits contains PCI class code.

Update code to set all 24 bits of PCI class code and not only upper 16 bits of PCI class code.

Use a new macro PCI_CLASS_BRIDGE_PCI_NORMAL which represents whole 24 bits of normal PCI bridge class.

Signed-off-by Pali Rohár <[email protected]>

---
Roman helped me with this change and confirmed that class code is stored really in bits [23:0] of custom register 0x43c (normally class code is stored in bits [31:8] of pci register 0x08).

This patch depends on patch which adds PCI_CLASS_BRIDGE_PCI_NORMAL macro:
https://lore.kernel.org/linux-pci/[email protected]/

---
 drivers/pci/controller/pcie-iproc.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
drivers/pci/controller/pcie-iproc.c +1 -3
index 3df4ab209253..2519201b0e51 100644
@@ -789,14 +789,13 @@ static int iproc_pcie_check_link(struct iproc_pcie *pcie)
 		return -EFAULT;
 	}
 
-	/* force class to PCI_CLASS_BRIDGE_PCI (0x0604) */
+	/* force class to PCI_CLASS_BRIDGE_PCI_NORMAL (0x060400) */
 #define PCI_BRIDGE_CTRL_REG_OFFSET	0x43c
-#define PCI_CLASS_BRIDGE_MASK		0xffff00
-#define PCI_CLASS_BRIDGE_SHIFT		8

drivers/pci/controller/pcie-iproc.c static int iproc_pcie_check_link(struct iproc_pcie *pcie) · line 801

Quoted 10 lines
drivers/pci/controller/pcie-iproc.c +3 -2
+#define PCI_BRIDGE_CTRL_REG_CLASS_MASK	0xffffff
 	iproc_pci_raw_config_read32(pcie, 0, PCI_BRIDGE_CTRL_REG_OFFSET,
 				    4, &class);
-	class &= ~PCI_CLASS_BRIDGE_MASK;
-	class |= (PCI_CLASS_BRIDGE_PCI << PCI_CLASS_BRIDGE_SHIFT);
+	class &= ~PCI_BRIDGE_CTRL_REG_CLASS_MASK;
+	class |= PCI_CLASS_BRIDGE_PCI_NORMAL;
 	iproc_pci_raw_config_write32(pcie, 0, PCI_BRIDGE_CTRL_REG_OFFSET,
 				     4, class);

I have two comments:

1. You do not seem to generate the email list using the get_maintainer.pl script, so the two maintainers for Broadcom ARM architecture (Ray Jui and Scott Branden) are left out.

2. I suppose 'PCI_CLASS_BRIDGE_PCI_NORMAL' is defined in some common PCI header in a separate patch as described in the commit message. Then how come these patches are not constructed with a patch series?

Other than, the change itself is exactly what I sent to Roman and looks good to me. Thanks.

Acked-by Ray Jui <[email protected]>
author
#4 lore raw

Hello!

Quoted 50 lines · On Wednesday 05 January 2022 09:51:48 Ray Jui wrote:

Hi Pali,

Quoted 34 lines · Pali Rohár

Register 0x43c in its low 24 bits contains PCI class code.

Update code to set all 24 bits of PCI class code and not only upper 16 bits of PCI class code.

Use a new macro PCI_CLASS_BRIDGE_PCI_NORMAL which represents whole 24 bits of normal PCI bridge class.

Signed-off-by Pali Rohár <[email protected]>

---
Roman helped me with this change and confirmed that class code is stored really in bits [23:0] of custom register 0x43c (normally class code is stored in bits [31:8] of pci register 0x08).

This patch depends on patch which adds PCI_CLASS_BRIDGE_PCI_NORMAL macro:
https://lore.kernel.org/linux-pci/[email protected]/

---
 drivers/pci/controller/pcie-iproc.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
drivers/pci/controller/pcie-iproc.c +1 -3
index 3df4ab209253..2519201b0e51 100644
@@ -789,14 +789,13 @@ static int iproc_pcie_check_link(struct iproc_pcie *pcie)
 		return -EFAULT;
 	}
 
-	/* force class to PCI_CLASS_BRIDGE_PCI (0x0604) */
+	/* force class to PCI_CLASS_BRIDGE_PCI_NORMAL (0x060400) */
 #define PCI_BRIDGE_CTRL_REG_OFFSET	0x43c
-#define PCI_CLASS_BRIDGE_MASK		0xffff00
-#define PCI_CLASS_BRIDGE_SHIFT		8

drivers/pci/controller/pcie-iproc.c static int iproc_pcie_check_link(struct iproc_pcie *pcie) · line 801

Quoted 10 lines
drivers/pci/controller/pcie-iproc.c +3 -2
+#define PCI_BRIDGE_CTRL_REG_CLASS_MASK	0xffffff
 	iproc_pci_raw_config_read32(pcie, 0, PCI_BRIDGE_CTRL_REG_OFFSET,
 				    4, &class);
-	class &= ~PCI_CLASS_BRIDGE_MASK;
-	class |= (PCI_CLASS_BRIDGE_PCI << PCI_CLASS_BRIDGE_SHIFT);
+	class &= ~PCI_BRIDGE_CTRL_REG_CLASS_MASK;
+	class |= PCI_CLASS_BRIDGE_PCI_NORMAL;
 	iproc_pci_raw_config_write32(pcie, 0, PCI_BRIDGE_CTRL_REG_OFFSET,
 				     4, class);

I have two comments:

Quoted 3 lines

1. You do not seem to generate the email list using the get_maintainer.pl script, so the two maintainers for Broadcom ARM architecture (Ray Jui and Scott Branden) are left out.

Ou, sorry for that! I have generated this patch for U-Boot and Linux kernel and probably mixed or forgot to include correct recipients for correct project.

Quoted 3 lines

2. I suppose 'PCI_CLASS_BRIDGE_PCI_NORMAL' is defined in some common PCI header in a separate patch as described in the commit message. Then how come these patches are not constructed with a patch series?

Yes, PCI_CLASS_BRIDGE_PCI_NORMAL is a new constant for common pci header file defined in patch linked in commit message.
https://lore.kernel.org/linux-pci/[email protected]/

Originally I included this change in v1 of linked patch in December but I realized that it does not match standard PCI config space (different offset 0x43c vs 0x08 and also different shift 0x8 vs 0x0) and probably there is something either incorrect or really non-standard. So later in December I dropped iproc_pcie_check_link() change in v2 of the linked patch where is introduced PCI_CLASS_BRIDGE_PCI_NORMAL and now sent new change for iproc_pcie_check_link() separately.

Technically, linked patch in commit message is just extracting code into the common macros without any functional changed. But change in this iproc_pcie_check_link() has also functional change as now also lower 8 bits of class code are changed. So in my opinion this patch should be really separate of linked patch.

I hope that Lorenzo and Bjorn take patches in correct order...

Quoted 4 lines

Other than, the change itself is exactly what I sent to Roman and looks good to me. Thanks.

Acked-by Ray Jui <[email protected]>

Perfect!

#5 lore raw
Quoted 19 lines · Pali Rohár
Quoted 5 lines · On Wednesday 05 January 2022 09:51:48 Ray Jui wrote:

On 1/5/2022 1:35 AM, Pali Rohár wrote:

2. I suppose 'PCI_CLASS_BRIDGE_PCI_NORMAL' is defined in some common PCI header in a separate patch as described in the commit message. Then how come these patches are not constructed with a patch series?

Yes, PCI_CLASS_BRIDGE_PCI_NORMAL is a new constant for common pci header file defined in patch linked in commit message.
https://lore.kernel.org/linux-pci/[email protected]/

Originally I included this change in v1 of linked patch in December but I realized that it does not match standard PCI config space (different offset 0x43c vs 0x08 and also different shift 0x8 vs 0x0) and probably there is something either incorrect or really non-standard. So later in December I dropped iproc_pcie_check_link() change in v2 of the linked patch where is introduced PCI_CLASS_BRIDGE_PCI_NORMAL and now sent new change for iproc_pcie_check_link() separately.

Quoted 7 lines

Technically, linked patch in commit message is just extracting code into the common macros without any functional changed. But change in this iproc_pcie_check_link() has also functional change as now also lower 8 bits of class code are changed. So in my opinion this patch should be really separate of linked patch.

I hope that Lorenzo and Bjorn take patches in correct order...

If patches are not sent together in a series, you can't assume anything about the order they'll be applied in. Adding a note about "this patch depends patch X" helps a little but adds a fair amount of friction to the process.

Bjorn

#6 lore raw
Quoted 26 lines · Bjorn Helgaas
Quoted 25 lines · Pali Rohár
Quoted 5 lines · On Wednesday 05 January 2022 09:51:48 Ray Jui wrote:

On 1/5/2022 1:35 AM, Pali Rohár wrote:

2. I suppose 'PCI_CLASS_BRIDGE_PCI_NORMAL' is defined in some common PCI header in a separate patch as described in the commit message. Then how come these patches are not constructed with a patch series?

Yes, PCI_CLASS_BRIDGE_PCI_NORMAL is a new constant for common pci header file defined in patch linked in commit message.
https://lore.kernel.org/linux-pci/[email protected]/

Originally I included this change in v1 of linked patch in December but I realized that it does not match standard PCI config space (different offset 0x43c vs 0x08 and also different shift 0x8 vs 0x0) and probably there is something either incorrect or really non-standard. So later in December I dropped iproc_pcie_check_link() change in v2 of the linked patch where is introduced PCI_CLASS_BRIDGE_PCI_NORMAL and now sent new change for iproc_pcie_check_link() separately.

Technically, linked patch in commit message is just extracting code into the common macros without any functional changed. But change in this iproc_pcie_check_link() has also functional change as now also lower 8 bits of class code are changed. So in my opinion this patch should be really separate of linked patch.

Quoted 6 lines
Quoted 1 line

I hope that Lorenzo and Bjorn take patches in correct order...

If patches are not sent together in a series, you can't assume anything about the order they'll be applied in. Adding a note about "this patch depends patch X" helps a little but adds a fair amount of friction to the process.

Indeed, more so given that the dependency requires an ACK from other maintainers - I certainly can't pull this patch as-is.

Lorenzo

#7 lore raw
In reply to Pali Rohár
Quoted 77 lines · Pali Rohár

Hello!

Quoted 50 lines · On Wednesday 05 January 2022 09:51:48 Ray Jui wrote:

Hi Pali,

Quoted 34 lines · Pali Rohár

Register 0x43c in its low 24 bits contains PCI class code.

Update code to set all 24 bits of PCI class code and not only upper 16 bits of PCI class code.

Use a new macro PCI_CLASS_BRIDGE_PCI_NORMAL which represents whole 24 bits of normal PCI bridge class.

Signed-off-by Pali Rohár <[email protected]>

---
Roman helped me with this change and confirmed that class code is stored really in bits [23:0] of custom register 0x43c (normally class code is stored in bits [31:8] of pci register 0x08).

This patch depends on patch which adds PCI_CLASS_BRIDGE_PCI_NORMAL macro:
https://lore.kernel.org/linux-pci/[email protected]/

---
 drivers/pci/controller/pcie-iproc.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
drivers/pci/controller/pcie-iproc.c +1 -3
index 3df4ab209253..2519201b0e51 100644
@@ -789,14 +789,13 @@ static int iproc_pcie_check_link(struct iproc_pcie *pcie)
 		return -EFAULT;
 	}
 
-	/* force class to PCI_CLASS_BRIDGE_PCI (0x0604) */
+	/* force class to PCI_CLASS_BRIDGE_PCI_NORMAL (0x060400) */
 #define PCI_BRIDGE_CTRL_REG_OFFSET	0x43c
-#define PCI_CLASS_BRIDGE_MASK		0xffff00
-#define PCI_CLASS_BRIDGE_SHIFT		8

drivers/pci/controller/pcie-iproc.c static int iproc_pcie_check_link(struct iproc_pcie *pcie) · line 801

Quoted 10 lines
drivers/pci/controller/pcie-iproc.c +3 -2
+#define PCI_BRIDGE_CTRL_REG_CLASS_MASK	0xffffff
 	iproc_pci_raw_config_read32(pcie, 0, PCI_BRIDGE_CTRL_REG_OFFSET,
 				    4, &class);
-	class &= ~PCI_CLASS_BRIDGE_MASK;
-	class |= (PCI_CLASS_BRIDGE_PCI << PCI_CLASS_BRIDGE_SHIFT);
+	class &= ~PCI_BRIDGE_CTRL_REG_CLASS_MASK;
+	class |= PCI_CLASS_BRIDGE_PCI_NORMAL;
 	iproc_pci_raw_config_write32(pcie, 0, PCI_BRIDGE_CTRL_REG_OFFSET,
 				     4, class);

I have two comments:

Quoted 3 lines

1. You do not seem to generate the email list using the get_maintainer.pl script, so the two maintainers for Broadcom ARM architecture (Ray Jui and Scott Branden) are left out.

Ou, sorry for that! I have generated this patch for U-Boot and Linux kernel and probably mixed or forgot to include correct recipients for correct project.

Quoted 3 lines

2. I suppose 'PCI_CLASS_BRIDGE_PCI_NORMAL' is defined in some common PCI header in a separate patch as described in the commit message. Then how come these patches are not constructed with a patch series?

Yes, PCI_CLASS_BRIDGE_PCI_NORMAL is a new constant for common pci header file defined in patch linked in commit message.
https://lore.kernel.org/linux-pci/[email protected]/

Originally I included this change in v1 of linked patch in December but I realized that it does not match standard PCI config space (different offset 0x43c vs 0x08 and also different shift 0x8 vs 0x0) and probably there is something either incorrect or really non-standard. So later in December I dropped iproc_pcie_check_link() change in v2 of the linked patch where is introduced PCI_CLASS_BRIDGE_PCI_NORMAL and now sent new change for iproc_pcie_check_link() separately.

Quoted 7 lines

Technically, linked patch in commit message is just extracting code into the common macros without any functional changed. But change in this iproc_pcie_check_link() has also functional change as now also lower 8 bits of class code are changed. So in my opinion this patch should be really separate of linked patch.

I hope that Lorenzo and Bjorn take patches in correct order...

Can you resend the patches in a series please, I will drop this one.

Thanks,
Lorenzo

Quoted 6 lines
Quoted 4 lines

Other than, the change itself is exactly what I sent to Roman and looks good to me. Thanks.

Acked-by Ray Jui <[email protected]>

Perfect!

author
#8 lore raw
Quoted 84 lines · On Friday 11 February 2022 16:23:17 Lorenzo Pieralisi wrote:
Quoted 83 lines · Pali Rohár

Hello!

Quoted 50 lines · On Wednesday 05 January 2022 09:51:48 Ray Jui wrote:

Hi Pali,

Quoted 34 lines · Pali Rohár

Register 0x43c in its low 24 bits contains PCI class code.

Update code to set all 24 bits of PCI class code and not only upper 16 bits of PCI class code.

Use a new macro PCI_CLASS_BRIDGE_PCI_NORMAL which represents whole 24 bits of normal PCI bridge class.

Signed-off-by Pali Rohár <[email protected]>

---
Roman helped me with this change and confirmed that class code is stored really in bits [23:0] of custom register 0x43c (normally class code is stored in bits [31:8] of pci register 0x08).

This patch depends on patch which adds PCI_CLASS_BRIDGE_PCI_NORMAL macro:
https://lore.kernel.org/linux-pci/[email protected]/

---
 drivers/pci/controller/pcie-iproc.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
drivers/pci/controller/pcie-iproc.c +1 -3
index 3df4ab209253..2519201b0e51 100644
@@ -789,14 +789,13 @@ static int iproc_pcie_check_link(struct iproc_pcie *pcie)
 		return -EFAULT;
 	}
 
-	/* force class to PCI_CLASS_BRIDGE_PCI (0x0604) */
+	/* force class to PCI_CLASS_BRIDGE_PCI_NORMAL (0x060400) */
 #define PCI_BRIDGE_CTRL_REG_OFFSET	0x43c
-#define PCI_CLASS_BRIDGE_MASK		0xffff00
-#define PCI_CLASS_BRIDGE_SHIFT		8

drivers/pci/controller/pcie-iproc.c static int iproc_pcie_check_link(struct iproc_pcie *pcie) · line 801

Quoted 10 lines
drivers/pci/controller/pcie-iproc.c +3 -2
+#define PCI_BRIDGE_CTRL_REG_CLASS_MASK	0xffffff
 	iproc_pci_raw_config_read32(pcie, 0, PCI_BRIDGE_CTRL_REG_OFFSET,
 				    4, &class);
-	class &= ~PCI_CLASS_BRIDGE_MASK;
-	class |= (PCI_CLASS_BRIDGE_PCI << PCI_CLASS_BRIDGE_SHIFT);
+	class &= ~PCI_BRIDGE_CTRL_REG_CLASS_MASK;
+	class |= PCI_CLASS_BRIDGE_PCI_NORMAL;
 	iproc_pci_raw_config_write32(pcie, 0, PCI_BRIDGE_CTRL_REG_OFFSET,
 				     4, class);

I have two comments:

Quoted 3 lines

1. You do not seem to generate the email list using the get_maintainer.pl script, so the two maintainers for Broadcom ARM architecture (Ray Jui and Scott Branden) are left out.

Ou, sorry for that! I have generated this patch for U-Boot and Linux kernel and probably mixed or forgot to include correct recipients for correct project.

Quoted 3 lines

2. I suppose 'PCI_CLASS_BRIDGE_PCI_NORMAL' is defined in some common PCI header in a separate patch as described in the commit message. Then how come these patches are not constructed with a patch series?

Yes, PCI_CLASS_BRIDGE_PCI_NORMAL is a new constant for common pci header file defined in patch linked in commit message.
https://lore.kernel.org/linux-pci/[email protected]/

Originally I included this change in v1 of linked patch in December but I realized that it does not match standard PCI config space (different offset 0x43c vs 0x08 and also different shift 0x8 vs 0x0) and probably there is something either incorrect or really non-standard. So later in December I dropped iproc_pcie_check_link() change in v2 of the linked patch where is introduced PCI_CLASS_BRIDGE_PCI_NORMAL and now sent new change for iproc_pcie_check_link() separately.

Technically, linked patch in commit message is just extracting code into the common macros without any functional changed. But change in this iproc_pcie_check_link() has also functional change as now also lower 8 bits of class code are changed. So in my opinion this patch should be really separate of linked patch.

Quoted 3 lines
Quoted 1 line

I hope that Lorenzo and Bjorn take patches in correct order...

Can you resend the patches in a series please, I will drop this one.

Done!
https://lore.kernel.org/linux-pci/[email protected]/T/#u

Quoted 9 lines

Thanks,
Lorenzo

Quoted 6 lines
Quoted 4 lines

Other than, the change itself is exactly what I sent to Roman and looks good to me. Thanks.

Acked-by Ray Jui <[email protected]>

Perfect!

build bot
#9 lore raw
In reply to Pali Rohár
I love your patch! Yet something to improve:

Hi "Pali,

I love your patch! Yet something to improve:

[auto build test ERROR on helgaas-pci/next]
[also build test ERROR on v5.16-rc8 next-20220105]
[If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Pali-Roh-r/PCI-iproc-Set-all-24-bits-of-PCI-class-code/20220105-173704
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: arm-randconfig-r001-20220105 (https://download.01.org/0day-ci/archive/20220106/202201060511.mczwwaNF-lkp(a)intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project d5b6e30ed3acad794dd0aec400e617daffc6cc3d)

reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# install arm cross compiling tool for clang build
# apt-get install binutils-arm-linux-gnueabi
# https://github.com/0day-ci/linux/commit/8ef1acfb84c08a0331930f9a60884fdd6d7c5e88
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Pali-Roh-r/PCI-iproc-Set-all-24-bits-of-PCI-class-code/20220105-173704 git checkout 8ef1acfb84c08a0331930f9a60884fdd6d7c5e88
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

>> drivers/pci/controller/pcie-iproc.c:798:11: error: use of undeclared identifier 'PCI_CLASS_BRIDGE_PCI_NORMAL'
           class |= PCI_CLASS_BRIDGE_PCI_NORMAL;
                    ^

1 error generated.

vim +/PCI_CLASS_BRIDGE_PCI_NORMAL +798 drivers/pci/controller/pcie-iproc.c

765
766 static int iproc_pcie_check_link(struct iproc_pcie *pcie)
767 {
768 struct device *dev = pcie->dev;
769 u32 hdr_type, link_ctrl, link_status, class, val;
770 bool link_is_active = false;
771
772 /*
773 * PAXC connects to emulated endpoint devices directly and does not
774 * have a Serdes. Therefore skip the link detection logic here.
775 */
776 if (pcie->ep_is_internal)
777 return 0;
778
779 val = iproc_pcie_read_reg(pcie, IPROC_PCIE_LINK_STATUS);
780 if (!(val & PCIE_PHYLINKUP) || !(val & PCIE_DL_ACTIVE)) {
781 dev_err(dev, "PHY or data link is INACTIVE!\n");
782 return -ENODEV;
783 }
784
785 /* make sure we are not in EP mode */
786 iproc_pci_raw_config_read32(pcie, 0, PCI_HEADER_TYPE, 1, &hdr_type);
787 if ((hdr_type & 0x7f) != PCI_HEADER_TYPE_BRIDGE) {
788 dev_err(dev, "in EP mode, hdr=%#02x\n", hdr_type);
789 return -EFAULT;
790 }
791
792 /* force class to PCI_CLASS_BRIDGE_PCI_NORMAL (0x060400) */
793 #define PCI_BRIDGE_CTRL_REG_OFFSET 0x43c
794 #define PCI_BRIDGE_CTRL_REG_CLASS_MASK 0xffffff
795 iproc_pci_raw_config_read32(pcie, 0, PCI_BRIDGE_CTRL_REG_OFFSET,
796 4, &class);
797 class &= ~PCI_BRIDGE_CTRL_REG_CLASS_MASK;

 > 798		class |= PCI_CLASS_BRIDGE_PCI_NORMAL;

799 iproc_pci_raw_config_write32(pcie, 0, PCI_BRIDGE_CTRL_REG_OFFSET,
800 4, class);
801
802 /* check link status to see if link is active */
803 iproc_pci_raw_config_read32(pcie, 0, IPROC_PCI_EXP_CAP + PCI_EXP_LNKSTA,
804 2, &link_status);
805 if (link_status & PCI_EXP_LNKSTA_NLW)
806 link_is_active = true;
807
808 if (!link_is_active) {
809 /* try GEN 1 link speed */
810 #define PCI_TARGET_LINK_SPEED_MASK 0xf
811 #define PCI_TARGET_LINK_SPEED_GEN2 0x2
812 #define PCI_TARGET_LINK_SPEED_GEN1 0x1
813 iproc_pci_raw_config_read32(pcie, 0,
814 IPROC_PCI_EXP_CAP + PCI_EXP_LNKCTL2,
815 4, &link_ctrl);
816 if ((link_ctrl & PCI_TARGET_LINK_SPEED_MASK) ==
817 PCI_TARGET_LINK_SPEED_GEN2) {
818 link_ctrl &= ~PCI_TARGET_LINK_SPEED_MASK;
819 link_ctrl |= PCI_TARGET_LINK_SPEED_GEN1;
820 iproc_pci_raw_config_write32(pcie, 0,
821 IPROC_PCI_EXP_CAP + PCI_EXP_LNKCTL2,
822 4, link_ctrl);
823 msleep(100);
824
825 iproc_pci_raw_config_read32(pcie, 0,
826 IPROC_PCI_EXP_CAP + PCI_EXP_LNKSTA,
827 2, &link_status);
828 if (link_status & PCI_EXP_LNKSTA_NLW)
829 link_is_active = true;
830 }
831 }
832
833 dev_info(dev, "link: %s\n", link_is_active ? "UP" : "DOWN");
834
835 return link_is_active ? 0 : -ENODEV;
836 }
837

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org