diff mbox series

PCI: xilinx-nwl: Remove unnecessary code and updating ecam default value.

Message ID 20230803115016.4266-1-thippeswamy.havalige@amd.com
State New
Headers
Series PCI: xilinx-nwl: Remove unnecessary code and updating ecam default value. |

Commit Message

Havalige, Thippeswamy Aug. 3, 2023, 11:50 a.m. UTC 
  Remove reduntant code.
Change NWL_ECAM_VALUE_DEFAULT to 16 to support maximum 256 buses.

Signed-off-by: Thippeswamy Havalige <thippeswamy.havalige@amd.com>
---
 drivers/pci/controller/pcie-xilinx-nwl.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

Comments

Bjorn Helgaas Aug. 3, 2023, 4:55 p.m. UTC | #1 
On Thu, Aug 03, 2023 at 05:20:16PM +0530, Thippeswamy Havalige wrote:
> Remove reduntant code.
> Change NWL_ECAM_VALUE_DEFAULT to 16 to support maximum 256 buses.

Remove period from subject line.

Please mention the most important part first in the subject -- the
ECAM change sounds more important than removing redundant code.

s/ecam/ECAM/
s/reduntant/redundant/

Please elaborate on why this code is redundant.  What makes it
redundant?  Apparently the bus number registers default to the correct
values or some other software programs them?

I don't see the point of the struct nwl_pcie.ecam_value member.  It is
set once and never updated, so we could just use
NWL_ECAM_VALUE_DEFAULT instead.

"ECAM_VALUE" is not a very informative name.  I don't know what an
"ECAM value" would be.  How is the value 16 related to a maximum of
256 buses?  We only need 8 bits to address 256 buses, so it must be
something else.  The bus number appears at bits 20-27
(PCIE_ECAM_BUS_SHIFT) in a standard ECAM address, so probably not the
bit location?

Does this fix a problem?

> Signed-off-by: Thippeswamy Havalige <thippeswamy.havalige@amd.com>
> ---
>  drivers/pci/controller/pcie-xilinx-nwl.c | 11 +----------
>  1 file changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-xilinx-nwl.c b/drivers/pci/controller/pcie-xilinx-nwl.c
> index 176686b..6d40543 100644
> --- a/drivers/pci/controller/pcie-xilinx-nwl.c
> +++ b/drivers/pci/controller/pcie-xilinx-nwl.c
> @@ -126,7 +126,7 @@
>  #define E_ECAM_CR_ENABLE		BIT(0)
>  #define E_ECAM_SIZE_LOC			GENMASK(20, 16)
>  #define E_ECAM_SIZE_SHIFT		16
> -#define NWL_ECAM_VALUE_DEFAULT		12
> +#define NWL_ECAM_VALUE_DEFAULT		16
>  
>  #define CFG_DMA_REG_BAR			GENMASK(2, 0)
>  #define CFG_PCIE_CACHE			GENMASK(7, 0)
> @@ -683,15 +683,6 @@ static int nwl_pcie_bridge_init(struct nwl_pcie *pcie)
>  	nwl_bridge_writel(pcie, upper_32_bits(pcie->phys_ecam_base),
>  			  E_ECAM_BASE_HI);
>  
> -	/* Get bus range */
> -	ecam_val = nwl_bridge_readl(pcie, E_ECAM_CONTROL);
> -	pcie->last_busno = (ecam_val & E_ECAM_SIZE_LOC) >> E_ECAM_SIZE_SHIFT;
> -	/* Write primary, secondary and subordinate bus numbers */
> -	ecam_val = first_busno;
> -	ecam_val |= (first_busno + 1) << 8;
> -	ecam_val |= (pcie->last_busno << E_ECAM_SIZE_SHIFT);
> -	writel(ecam_val, (pcie->ecam_base + PCI_PRIMARY_BUS));

"ecam_val" looks like it's supposed to be the 32-bit value containing
PCI_PRIMARY_BUS (low 8 bits, from the pointless "first_busno" that is
always 0), PCI_SECONDARY_BUS (bits 8-15, always bus 1),
PCI_SUBORDINATE_BUS (bits 16-23, totally unrelated to
E_ECAM_SIZE_SHIFT although E_ECAM_SIZE_SHIFT happens to be the correct
value (16)), and PCI_SEC_LATENCY_TIMER (not applicable for PCIe).

So I guess the assumption is that these registers already contain the
correct values?

It looks like previously PCI_SUBORDINATE_BUS (i.e., pcie->last_busno)
was 12, since we wrote NWL_ECAM_VALUE_DEFAULT to E_ECAM_CONTROL and
then read it back?

And now pcie->last_busno is competely unused?

This all seems not quite baked.  Am I missing something?

Bjorn
kernel test robot Aug. 3, 2023, 7:39 p.m. UTC | #2 
Hi Thippeswamy,

kernel test robot noticed the following build warnings:

[auto build test WARNING on pci/next]
[also build test WARNING on pci/for-linus linus/master v6.5-rc4 next-20230803]
[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#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Thippeswamy-Havalige/PCI-xilinx-nwl-Remove-unnecessary-code-and-updating-ecam-default-value/20230803-195228
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link:    https://lore.kernel.org/r/20230803115016.4266-1-thippeswamy.havalige%40amd.com
patch subject: [PATCH] PCI: xilinx-nwl: Remove unnecessary code and updating ecam default value.
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20230804/202308040330.eMTjX3tF-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230804/202308040330.eMTjX3tF-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308040330.eMTjX3tF-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/pci/controller/pcie-xilinx-nwl.c: In function 'nwl_pcie_bridge_init':
>> drivers/pci/controller/pcie-xilinx-nwl.c:628:33: warning: unused variable 'first_busno' [-Wunused-variable]
     628 |         u32 breg_val, ecam_val, first_busno = 0;
         |                                 ^~~~~~~~~~~


vim +/first_busno +628 drivers/pci/controller/pcie-xilinx-nwl.c

ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  623  
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  624  static int nwl_pcie_bridge_init(struct nwl_pcie *pcie)
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  625  {
adf9e284b4f76d drivers/pci/host/pcie-xilinx-nwl.c       Bjorn Helgaas        2016-10-06  626  	struct device *dev = pcie->dev;
adf9e284b4f76d drivers/pci/host/pcie-xilinx-nwl.c       Bjorn Helgaas        2016-10-06  627  	struct platform_device *pdev = to_platform_device(dev);
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06 @628  	u32 breg_val, ecam_val, first_busno = 0;
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  629  	int err;
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  630  
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  631  	breg_val = nwl_bridge_readl(pcie, E_BREG_CAPABILITIES) & BREG_PRESENT;
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  632  	if (!breg_val) {
adf9e284b4f76d drivers/pci/host/pcie-xilinx-nwl.c       Bjorn Helgaas        2016-10-06  633  		dev_err(dev, "BREG is not present\n");
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  634  		return breg_val;
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  635  	}
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  636  
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  637  	/* Write bridge_off to breg base */
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  638  	nwl_bridge_writel(pcie, lower_32_bits(pcie->phys_breg_base),
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  639  			  E_BREG_BASE_LO);
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  640  	nwl_bridge_writel(pcie, upper_32_bits(pcie->phys_breg_base),
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  641  			  E_BREG_BASE_HI);
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  642  
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  643  	/* Enable BREG */
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  644  	nwl_bridge_writel(pcie, ~BREG_ENABLE_FORCE & BREG_ENABLE,
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  645  			  E_BREG_CONTROL);
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  646  
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  647  	/* Disable DMA channel registers */
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  648  	nwl_bridge_writel(pcie, nwl_bridge_readl(pcie, BRCFG_PCIE_RX0) |
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  649  			  CFG_DMA_REG_BAR, BRCFG_PCIE_RX0);
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  650  
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  651  	/* Enable Ingress subtractive decode translation */
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  652  	nwl_bridge_writel(pcie, SET_ISUB_CONTROL, I_ISUB_CONTROL);
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  653  
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  654  	/* Enable msg filtering details */
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  655  	nwl_bridge_writel(pcie, CFG_ENABLE_MSG_FILTER_MASK,
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  656  			  BRCFG_PCIE_RX_MSG_FILTER);
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  657  
213e1220523288 drivers/pci/controller/pcie-xilinx-nwl.c Bharat Kumar Gogada  2021-02-22  658  	/* This routes the PCIe DMA traffic to go through CCI path */
213e1220523288 drivers/pci/controller/pcie-xilinx-nwl.c Bharat Kumar Gogada  2021-02-22  659  	if (of_dma_is_coherent(dev->of_node))
213e1220523288 drivers/pci/controller/pcie-xilinx-nwl.c Bharat Kumar Gogada  2021-02-22  660  		nwl_bridge_writel(pcie, nwl_bridge_readl(pcie, BRCFG_PCIE_RX1) |
213e1220523288 drivers/pci/controller/pcie-xilinx-nwl.c Bharat Kumar Gogada  2021-02-22  661  				  CFG_PCIE_CACHE, BRCFG_PCIE_RX1);
213e1220523288 drivers/pci/controller/pcie-xilinx-nwl.c Bharat Kumar Gogada  2021-02-22  662  
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  663  	err = nwl_wait_for_link(pcie);
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  664  	if (err)
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  665  		return err;
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  666  
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  667  	ecam_val = nwl_bridge_readl(pcie, E_ECAM_CAPABILITIES) & E_ECAM_PRESENT;
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  668  	if (!ecam_val) {
adf9e284b4f76d drivers/pci/host/pcie-xilinx-nwl.c       Bjorn Helgaas        2016-10-06  669  		dev_err(dev, "ECAM is not present\n");
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  670  		return ecam_val;
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  671  	}
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  672  
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  673  	/* Enable ECAM */
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  674  	nwl_bridge_writel(pcie, nwl_bridge_readl(pcie, E_ECAM_CONTROL) |
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  675  			  E_ECAM_CR_ENABLE, E_ECAM_CONTROL);
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  676  
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  677  	nwl_bridge_writel(pcie, nwl_bridge_readl(pcie, E_ECAM_CONTROL) |
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  678  			  (pcie->ecam_value << E_ECAM_SIZE_SHIFT),
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  679  			  E_ECAM_CONTROL);
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  680  
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  681  	nwl_bridge_writel(pcie, lower_32_bits(pcie->phys_ecam_base),
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  682  			  E_ECAM_BASE_LO);
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  683  	nwl_bridge_writel(pcie, upper_32_bits(pcie->phys_ecam_base),
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  684  			  E_ECAM_BASE_HI);
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  685  
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  686  	if (nwl_pcie_link_up(pcie))
adf9e284b4f76d drivers/pci/host/pcie-xilinx-nwl.c       Bjorn Helgaas        2016-10-06  687  		dev_info(dev, "Link is UP\n");
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  688  	else
adf9e284b4f76d drivers/pci/host/pcie-xilinx-nwl.c       Bjorn Helgaas        2016-10-06  689  		dev_info(dev, "Link is DOWN\n");
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  690  
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  691  	/* Get misc IRQ number */
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  692  	pcie->irq_misc = platform_get_irq_byname(pdev, "misc");
caecb05c800081 drivers/pci/controller/pcie-xilinx-nwl.c Krzysztof Wilczyński 2020-08-02  693  	if (pcie->irq_misc < 0)
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  694  		return -EINVAL;
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  695  
adf9e284b4f76d drivers/pci/host/pcie-xilinx-nwl.c       Bjorn Helgaas        2016-10-06  696  	err = devm_request_irq(dev, pcie->irq_misc,
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  697  			       nwl_pcie_misc_handler, IRQF_SHARED,
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  698  			       "nwl_pcie:misc", pcie);
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  699  	if (err) {
adf9e284b4f76d drivers/pci/host/pcie-xilinx-nwl.c       Bjorn Helgaas        2016-10-06  700  		dev_err(dev, "fail to register misc IRQ#%d\n",
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  701  			pcie->irq_misc);
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  702  		return err;
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  703  	}
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  704  
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  705  	/* Disable all misc interrupts */
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  706  	nwl_bridge_writel(pcie, (u32)~MSGF_MISC_SR_MASKALL, MSGF_MISC_MASK);
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  707  
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  708  	/* Clear pending misc interrupts */
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  709  	nwl_bridge_writel(pcie, nwl_bridge_readl(pcie, MSGF_MISC_STATUS) &
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  710  			  MSGF_MISC_SR_MASKALL, MSGF_MISC_STATUS);
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  711  
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  712  	/* Enable all misc interrupts */
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  713  	nwl_bridge_writel(pcie, MSGF_MISC_SR_MASKALL, MSGF_MISC_MASK);
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  714  
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  715  	/* Disable all legacy interrupts */
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  716  	nwl_bridge_writel(pcie, (u32)~MSGF_LEG_SR_MASKALL, MSGF_LEG_MASK);
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  717  
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  718  	/* Clear pending legacy interrupts */
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  719  	nwl_bridge_writel(pcie, nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  720  			  MSGF_LEG_SR_MASKALL, MSGF_LEG_STATUS);
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  721  
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  722  	/* Enable all legacy interrupts */
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  723  	nwl_bridge_writel(pcie, MSGF_LEG_SR_MASKALL, MSGF_LEG_MASK);
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  724  
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  725  	/* Enable the bridge config interrupt */
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  726  	nwl_bridge_writel(pcie, nwl_bridge_readl(pcie, BRCFG_INTERRUPT) |
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  727  			  BRCFG_INTERRUPT_MASK, BRCFG_INTERRUPT);
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  728  
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  729  	return 0;
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  730  }
ab597d35ef11d2 drivers/pci/host/pcie-xilinx-nwl.c       Bharat Kumar Gogada  2016-03-06  731
Havalige, Thippeswamy Aug. 4, 2023, 7:05 p.m. UTC | #3 
Hi Bjorn,

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: Thursday, August 3, 2023 10:26 PM
> To: Havalige, Thippeswamy <thippeswamy.havalige@amd.com>
> Cc: linux-kernel@vger.kernel.org; robh+dt@kernel.org;
> bhelgaas@google.com; linux-pci@vger.kernel.org;
> krzysztof.kozlowski@linaro.org; lpieralisi@kernel.org; Gogada, Bharat Kumar
> <bharat.kumar.gogada@amd.com>; Simek, Michal
> <michal.simek@amd.com>; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH] PCI: xilinx-nwl: Remove unnecessary code and updating
> ecam default value.
> 
> On Thu, Aug 03, 2023 at 05:20:16PM +0530, Thippeswamy Havalige wrote:
> > Remove reduntant code.
> > Change NWL_ECAM_VALUE_DEFAULT to 16 to support maximum 256
> buses.
> 
> Remove period from subject line.
> 
> Please mention the most important part first in the subject -- the
> ECAM change sounds more important than removing redundant code.
> 
> s/ecam/ECAM/
> s/reduntant/redundant/
> 
> Please elaborate on why this code is redundant.  What makes it
> redundant?  Apparently the bus number registers default to the correct
> values or some other software programs them?


 - Yes, The  Primary,Secondary and sub-ordinate bus number registers  are programmed/updated as part of linux enumeration so updating these registers are redundant.

> I don't see the point of the struct nwl_pcie.ecam_value member.  It is
> set once and never updated, so we could just use
> NWL_ECAM_VALUE_DEFAULT instead.
-Agreed, I ll update it in next patch. 


> "ECAM_VALUE" is not a very informative name.  I don't know what an
> "ECAM value" would be.  How is the value 16 related to a maximum of
> 256 buses?  We only need 8 bits to address 256 buses, so it must be
> something else.  The bus number appears at bits 20-27
> (PCIE_ECAM_BUS_SHIFT) in a standard ECAM address, so probably not the
> bit location?
Yes, Agreed I'll modify ECAM_VALUE as ECAM_SIZE here and it is not related to a maximum 256 buses.
> Does this fix a problem?

- Yes, It is fixing a problem. Our controller is expecting ECAM size to be programmed by software.  By programming "NWL_ECAM_VALUE_DEFAULT  12" controller can access upto 16MB ECAM region which is used to detect 16 buses so by updating "NWL_ECAM_VALUE_DEFAULT " to 16 controller can access upto 256 Mb ECAM region to detect 256 buses.

2^(ecam_size_offset+ecam_size) 

Here (ecam_size_offset=12 and ecam_size=16) --> 256Mb

> > Signed-off-by: Thippeswamy Havalige <thippeswamy.havalige@amd.com>
> > ---
> >  drivers/pci/controller/pcie-xilinx-nwl.c | 11 +----------
> >  1 file changed, 1 insertion(+), 10 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-xilinx-nwl.c
> b/drivers/pci/controller/pcie-xilinx-nwl.c
> > index 176686b..6d40543 100644
> > --- a/drivers/pci/controller/pcie-xilinx-nwl.c
> > +++ b/drivers/pci/controller/pcie-xilinx-nwl.c
> > @@ -126,7 +126,7 @@
> >  #define E_ECAM_CR_ENABLE		BIT(0)
> >  #define E_ECAM_SIZE_LOC			GENMASK(20, 16)
> >  #define E_ECAM_SIZE_SHIFT		16
> > -#define NWL_ECAM_VALUE_DEFAULT		12
> > +#define NWL_ECAM_VALUE_DEFAULT		16
> >
> >  #define CFG_DMA_REG_BAR			GENMASK(2, 0)
> >  #define CFG_PCIE_CACHE			GENMASK(7, 0)
> > @@ -683,15 +683,6 @@ static int nwl_pcie_bridge_init(struct nwl_pcie
> *pcie)
> >  	nwl_bridge_writel(pcie, upper_32_bits(pcie->phys_ecam_base),
> >  			  E_ECAM_BASE_HI);
> >
> > -	/* Get bus range */
> > -	ecam_val = nwl_bridge_readl(pcie, E_ECAM_CONTROL);
> > -	pcie->last_busno = (ecam_val & E_ECAM_SIZE_LOC) >>
> E_ECAM_SIZE_SHIFT;
> > -	/* Write primary, secondary and subordinate bus numbers */
> > -	ecam_val = first_busno;
> > -	ecam_val |= (first_busno + 1) << 8;
> > -	ecam_val |= (pcie->last_busno << E_ECAM_SIZE_SHIFT);
> > -	writel(ecam_val, (pcie->ecam_base + PCI_PRIMARY_BUS));
> 
> "ecam_val" looks like it's supposed to be the 32-bit value containing
> PCI_PRIMARY_BUS (low 8 bits, from the pointless "first_busno" that is
> always 0), PCI_SECONDARY_BUS (bits 8-15, always bus 1),
> PCI_SUBORDINATE_BUS (bits 16-23, totally unrelated to
> E_ECAM_SIZE_SHIFT although E_ECAM_SIZE_SHIFT happens to be the correct
> value (16)), and PCI_SEC_LATENCY_TIMER (not applicable for PCIe).
> 
> So I guess the assumption is that these registers already contain the
> correct values?
> 
> It looks like previously PCI_SUBORDINATE_BUS (i.e., pcie->last_busno)
> was 12, since we wrote NWL_ECAM_VALUE_DEFAULT to E_ECAM_CONTROL
> and
> then read it back?
> 
> And now pcie->last_busno is competely unused?
> 
> This all seems not quite baked.  Am I missing something?
> 
> Bjorn
Bjorn Helgaas Aug. 4, 2023, 7:58 p.m. UTC | #4 
On Fri, Aug 04, 2023 at 07:05:30PM +0000, Havalige, Thippeswamy wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas <helgaas@kernel.org>
> > On Thu, Aug 03, 2023 at 05:20:16PM +0530, Thippeswamy Havalige wrote:
> > > Remove reduntant code.
> > > Change NWL_ECAM_VALUE_DEFAULT to 16 to support maximum 256
> > > buses.
> > 
> > Remove period from subject line.
> > 
> > Please mention the most important part first in the subject -- the
> > ECAM change sounds more important than removing redundant code.
> > 
> > s/ecam/ECAM/
> > s/reduntant/redundant/
> > 
> > Please elaborate on why this code is redundant.  What makes it
> > redundant?  Apparently the bus number registers default to the correct
> > values or some other software programs them?
> 
>  - Yes, The  Primary,Secondary and sub-ordinate bus number registers
>  are programmed/updated as part of linux enumeration so updating
>  these registers are redundant.

Ah, so the Linux PCI core can handle updating these from whatever the
power-up values are.  Good material for the revised commit log.

> > "ECAM_VALUE" is not a very informative name.  I don't know what an
> > "ECAM value" would be.  How is the value 16 related to a maximum of
> > 256 buses?  We only need 8 bits to address 256 buses, so it must be
> > something else.  The bus number appears at bits 20-27
> > (PCIE_ECAM_BUS_SHIFT) in a standard ECAM address, so probably not the
> > bit location?
>
> Yes, Agreed I'll modify ECAM_VALUE as ECAM_SIZE here and it is not
> related to a maximum 256 buses.

Well, it sounds like this value *does* determine the size of the ECAM
region, which does constrain the number of buses you can address via
ECAM.

> > Does this fix a problem?
> 
> - Yes, It is fixing a problem. Our controller is expecting ECAM size
> to be programmed by software.  By programming
> "NWL_ECAM_VALUE_DEFAULT  12" controller can access upto 16MB ECAM
> region which is used to detect 16 buses so by updating
> "NWL_ECAM_VALUE_DEFAULT " to 16 controller can access upto 256 Mb
> ECAM region to detect 256 buses.
> 
> 2^(ecam_size_offset+ecam_size) 
> 
> Here (ecam_size_offset=12 and ecam_size=16) --> 256Mb

More good material for the commit log.  (1) Change in ECAM region
size, (2) previously could only address 16 buses, now can address 256
buses.

Is there any impact on DT from the address map change?

Bjorn
Havalige, Thippeswamy Aug. 5, 2023, 7:07 p.m. UTC | #5 
Hi Bjorn,

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: Saturday, August 5, 2023 1:28 AM
> To: Havalige, Thippeswamy <thippeswamy.havalige@amd.com>
> Cc: linux-kernel@vger.kernel.org; robh+dt@kernel.org;
> bhelgaas@google.com; linux-pci@vger.kernel.org;
> krzysztof.kozlowski@linaro.org; lpieralisi@kernel.org; Gogada, Bharat Kumar
> <bharat.kumar.gogada@amd.com>; Simek, Michal
> <michal.simek@amd.com>; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH] PCI: xilinx-nwl: Remove unnecessary code and updating
> ecam default value.
> 
> On Fri, Aug 04, 2023 at 07:05:30PM +0000, Havalige, Thippeswamy wrote:
> > > -----Original Message-----
> > > From: Bjorn Helgaas <helgaas@kernel.org>
> > > On Thu, Aug 03, 2023 at 05:20:16PM +0530, Thippeswamy Havalige wrote:
> > > > Remove reduntant code.
> > > > Change NWL_ECAM_VALUE_DEFAULT to 16 to support maximum 256
> > > > buses.
> > >
> > > Remove period from subject line.
> > >
> > > Please mention the most important part first in the subject -- the
> > > ECAM change sounds more important than removing redundant code.
> > >
> > > s/ecam/ECAM/
> > > s/reduntant/redundant/
> > >
> > > Please elaborate on why this code is redundant.  What makes it
> > > redundant?  Apparently the bus number registers default to the correct
> > > values or some other software programs them?
> >
> >  - Yes, The  Primary,Secondary and sub-ordinate bus number registers
> >  are programmed/updated as part of linux enumeration so updating
> >  these registers are redundant.
> 
> Ah, so the Linux PCI core can handle updating these from whatever the
> power-up values are.  Good material for the revised commit log.
> 
> > > "ECAM_VALUE" is not a very informative name.  I don't know what an
> > > "ECAM value" would be.  How is the value 16 related to a maximum of
> > > 256 buses?  We only need 8 bits to address 256 buses, so it must be
> > > something else.  The bus number appears at bits 20-27
> > > (PCIE_ECAM_BUS_SHIFT) in a standard ECAM address, so probably not
> the
> > > bit location?
> >
> > Yes, Agreed I'll modify ECAM_VALUE as ECAM_SIZE here and it is not
> > related to a maximum 256 buses.
> 
> Well, it sounds like this value *does* determine the size of the ECAM
> region, which does constrain the number of buses you can address via
> ECAM.
> 
- Yes, This ecam_size does determine the number of buses can be addressed via ECAM.
> > > Does this fix a problem?
> >
> > - Yes, It is fixing a problem. Our controller is expecting ECAM size
> > to be programmed by software.  By programming
> > "NWL_ECAM_VALUE_DEFAULT  12" controller can access upto 16MB ECAM
> > region which is used to detect 16 buses so by updating
> > "NWL_ECAM_VALUE_DEFAULT " to 16 controller can access upto 256 Mb
> > ECAM region to detect 256 buses.
> >
> > 2^(ecam_size_offset+ecam_size)
> >
> > Here (ecam_size_offset=12 and ecam_size=16) --> 256Mb
> 
> More good material for the commit log.  (1) Change in ECAM region
> size, (2) previously could only address 16 buses, now can address 256
> buses.
> 
> Is there any impact on DT from the address map change?
> 
- Yes. Now device tree ECAM size needs to be modified to 256Mb.
- I'll update device tree changes along with next patch.
> Bjorn

Regards,
Thippeswamy H
diff mbox series

Patch

diff --git a/drivers/pci/controller/pcie-xilinx-nwl.c b/drivers/pci/controller/pcie-xilinx-nwl.c
index 176686b..6d40543 100644
--- a/drivers/pci/controller/pcie-xilinx-nwl.c
+++ b/drivers/pci/controller/pcie-xilinx-nwl.c
@@ -126,7 +126,7 @@ 
 #define E_ECAM_CR_ENABLE		BIT(0)
 #define E_ECAM_SIZE_LOC			GENMASK(20, 16)
 #define E_ECAM_SIZE_SHIFT		16
-#define NWL_ECAM_VALUE_DEFAULT		12
+#define NWL_ECAM_VALUE_DEFAULT		16
 
 #define CFG_DMA_REG_BAR			GENMASK(2, 0)
 #define CFG_PCIE_CACHE			GENMASK(7, 0)
@@ -683,15 +683,6 @@  static int nwl_pcie_bridge_init(struct nwl_pcie *pcie)
 	nwl_bridge_writel(pcie, upper_32_bits(pcie->phys_ecam_base),
 			  E_ECAM_BASE_HI);
 
-	/* Get bus range */
-	ecam_val = nwl_bridge_readl(pcie, E_ECAM_CONTROL);
-	pcie->last_busno = (ecam_val & E_ECAM_SIZE_LOC) >> E_ECAM_SIZE_SHIFT;
-	/* Write primary, secondary and subordinate bus numbers */
-	ecam_val = first_busno;
-	ecam_val |= (first_busno + 1) << 8;
-	ecam_val |= (pcie->last_busno << E_ECAM_SIZE_SHIFT);
-	writel(ecam_val, (pcie->ecam_base + PCI_PRIMARY_BUS));
-
 	if (nwl_pcie_link_up(pcie))
 		dev_info(dev, "Link is UP\n");
 	else