| Message ID | 20230803115016.4266-1-thippeswamy.havalige@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PCI: xilinx-nwl: Remove unnecessary code and updating ecam default value. | expand |
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
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
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
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
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 --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
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(-)