diff mbox series

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

Message ID 20220105093552.27542-1-pali@kernel.org
State New
Headers show
Series PCI: iproc: Set all 24 bits of PCI class code | expand

Commit Message

Pali Rohár Jan. 5, 2022, 9:35 a.m. UTC
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(-)

Comments

kernel test robot Jan. 5, 2022, 2:16 p.m. UTC | #1
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
Roman Bacik Jan. 5, 2022, 4:57 p.m. UTC | #2
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>
Ray Jui Jan. 5, 2022, 5:51 p.m. UTC | #3
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>
Pali Rohár Jan. 5, 2022, 6:13 p.m. UTC | #4
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!
kernel test robot Jan. 5, 2022, 9:05 p.m. UTC | #5
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
Bjorn Helgaas Jan. 6, 2022, 6 p.m. UTC | #6
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
Lorenzo Pieralisi Jan. 7, 2022, 11:43 a.m. UTC | #7
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
Lorenzo Pieralisi Feb. 11, 2022, 4:23 p.m. UTC | #8
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!
Pali Rohár Feb. 14, 2022, 11:49 a.m. UTC | #9
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 mbox series

Patch

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