| Message ID | 20220105093552.27542-1-pali@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: iproc: Set all 24 bits of PCI class code | expand |
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@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 <lkp@intel.com>
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@lists.01.org
On Wed, Jan 5, 2022 at 1:36 AM Pali Rohár <pali@kernel.org> wrote: > > 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 <pali@kernel.org> > > --- > 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/20211220145140.31898-1-pali@kernel.org/ > --- > drivers/pci/controller/pcie-iproc.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c > index 3df4ab209253..2519201b0e51 100644 > --- a/drivers/pci/controller/pcie-iproc.c > +++ b/drivers/pci/controller/pcie-iproc.c > @@ -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); > > -- > 2.20.1 > Acked-by: Roman Bacik <roman.bacik@broadcom.com>
Hi Pali, On 1/5/2022 1:35 AM, Pali Rohár wrote: > 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 <pali@kernel.org> > > --- > 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/20211220145140.31898-1-pali@kernel.org/ > --- > drivers/pci/controller/pcie-iproc.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c > index 3df4ab209253..2519201b0e51 100644 > --- a/drivers/pci/controller/pcie-iproc.c > +++ b/drivers/pci/controller/pcie-iproc.c > @@ -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); > 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 <ray.jui@broadcom.com>
Hello! On Wednesday 05 January 2022 09:51:48 Ray Jui wrote: > Hi Pali, > > On 1/5/2022 1:35 AM, Pali Rohár wrote: > > 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 <pali@kernel.org> > > > > --- > > 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/20211220145140.31898-1-pali@kernel.org/ > > --- > > drivers/pci/controller/pcie-iproc.c | 9 ++++----- > > 1 file changed, 4 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c > > index 3df4ab209253..2519201b0e51 100644 > > --- a/drivers/pci/controller/pcie-iproc.c > > +++ b/drivers/pci/controller/pcie-iproc.c > > @@ -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); > > > > 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. 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. > 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/20211220145140.31898-1-pali@kernel.org/ 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... > Other than, the change itself is exactly what I sent to Roman and looks > good to me. Thanks. > > Acked-by: Ray Jui <ray.jui@broadcom.com> Perfect!
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@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 <lkp@intel.com>
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@lists.01.org
On Wed, Jan 05, 2022 at 07:13:06PM +0100, Pali Rohár wrote: > 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/20211220145140.31898-1-pali@kernel.org/ > > 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... 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
On Thu, Jan 06, 2022 at 12:00:26PM -0600, Bjorn Helgaas wrote: > On Wed, Jan 05, 2022 at 07:13:06PM +0100, Pali Rohár wrote: > > 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/20211220145140.31898-1-pali@kernel.org/ > > > > 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... > > 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
On Wed, Jan 05, 2022 at 07:13:06PM +0100, Pali Rohár wrote: > Hello! > > On Wednesday 05 January 2022 09:51:48 Ray Jui wrote: > > Hi Pali, > > > > On 1/5/2022 1:35 AM, Pali Rohár wrote: > > > 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 <pali@kernel.org> > > > > > > --- > > > 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/20211220145140.31898-1-pali@kernel.org/ > > > --- > > > drivers/pci/controller/pcie-iproc.c | 9 ++++----- > > > 1 file changed, 4 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c > > > index 3df4ab209253..2519201b0e51 100644 > > > --- a/drivers/pci/controller/pcie-iproc.c > > > +++ b/drivers/pci/controller/pcie-iproc.c > > > @@ -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); > > > > > > > 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. > > 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. > > > 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/20211220145140.31898-1-pali@kernel.org/ > > 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... Can you resend the patches in a series please, I will drop this one. Thanks, Lorenzo > > Other than, the change itself is exactly what I sent to Roman and looks > > good to me. Thanks. > > > > Acked-by: Ray Jui <ray.jui@broadcom.com> > > Perfect!
On Friday 11 February 2022 16:23:17 Lorenzo Pieralisi wrote: > On Wed, Jan 05, 2022 at 07:13:06PM +0100, Pali Rohár wrote: > > Hello! > > > > On Wednesday 05 January 2022 09:51:48 Ray Jui wrote: > > > Hi Pali, > > > > > > On 1/5/2022 1:35 AM, Pali Rohár wrote: > > > > 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 <pali@kernel.org> > > > > > > > > --- > > > > 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/20211220145140.31898-1-pali@kernel.org/ > > > > --- > > > > drivers/pci/controller/pcie-iproc.c | 9 ++++----- > > > > 1 file changed, 4 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c > > > > index 3df4ab209253..2519201b0e51 100644 > > > > --- a/drivers/pci/controller/pcie-iproc.c > > > > +++ b/drivers/pci/controller/pcie-iproc.c > > > > @@ -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); > > > > > > > > > > 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. > > > > 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. > > > > > 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/20211220145140.31898-1-pali@kernel.org/ > > > > 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... > > Can you resend the patches in a series please, I will drop this one. Done! https://lore.kernel.org/linux-pci/20220214114109.26809-2-pali@kernel.org/T/#u > Thanks, > Lorenzo > > > > Other than, the change itself is exactly what I sent to Roman and looks > > > good to me. Thanks. > > > > > > Acked-by: Ray Jui <ray.jui@broadcom.com> > > > > Perfect!
diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c index 3df4ab209253..2519201b0e51 100644 --- a/drivers/pci/controller/pcie-iproc.c +++ b/drivers/pci/controller/pcie-iproc.c @@ -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);
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 <pali@kernel.org> --- 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/20211220145140.31898-1-pali@kernel.org/ --- drivers/pci/controller/pcie-iproc.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)