On Mon, Dec 14, 2020 at 7:03 AM Wan Mohamad, Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@xxxxxxxxx> wrote: > > Hi Rob and Andy. > > Thanks for the reviews and feedback. And sorry for late reply. > I will answer the earlier review here and add my reply > on the two matters below. > > In v4, I will > - remove the keembay_pcie_{readl,writel} wrappers, > - remove the dead code related to unused irqs, and > - initialize enabled interrupts to a known state. > > > -----Original Message----- > > From: Rob Herring <robh@xxxxxxxxxx> > > Sent: Friday, December 11, 2020 1:47 AM > > To: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> > > Cc: Wan Mohamad, Wan Ahmad Zainie > > <wan.ahmad.zainie.wan.mohamad@xxxxxxxxx>; Bjorn Helgaas > > <bhelgaas@xxxxxxxxxx>; Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>; PCI > > <linux-pci@xxxxxxxxxxxxxxx>; devicetree@xxxxxxxxxxxxxxx; Mark Gross > > <mgross@xxxxxxxxxxxxxxx>; Raja Subramanian, Lakshmi Bai > > <lakshmi.bai.raja.subramanian@xxxxxxxxx> > > Subject: Re: [PATCH v3 2/2] PCI: keembay: Add support for Intel Keem Bay > > > > On Wed, Dec 9, 2020 at 12:41 PM Andy Shevchenko > > <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote: > > > > > > On Wed, Dec 09, 2020 at 12:13:50PM -0600, Rob Herring wrote: > > > > On Wed, Dec 02, 2020 at 03:31:56PM +0800, Wan Ahmad Zainie wrote: > > > > > > ... > > > > > > > > +static void keembay_pcie_ltssm_enable(struct keembay_pcie *pcie, > > > > > +bool enable) { > > > > > + u32 val; > > > > > + > > > > > + val = keembay_pcie_readl(pcie, PCIE_REGS_PCIE_APP_CNTRL); > > > > > + if (enable) > > > > > + val |= APP_LTSSM_ENABLE; > > > > > + else > > > > > + val &= ~APP_LTSSM_ENABLE; > > > > > + keembay_pcie_writel(pcie, PCIE_REGS_PCIE_APP_CNTRL, val); > > > > > > > > If this is the only bit in this register, do you really need RMW? > > > > > > I think it's safer than do direct write and have something wrong on > > > next generations of hardware. > > > > We have 2 Intel SoCs with 2 separate PCI drivers so far, is that really going to > > be a concern? :( (This bit in particular is Synopsys' > > fault. This is what happens when IP vendors just give you signals to hook up.) > > > > There's 2 other reasons why to not do a RMW. The firmware or bootloader > > could also change how the register is initialized which you may or may not > > want changed in Linux. Second, for maintaining this code when anyone > > familiar with this h/w disappears, I'd like to know if there's other bits in this > > register I might want to care about. > > Lightning Mountain and Keem Bay belongs to two different product lines i.e. > Atom-based family and ARM-based family, targeted for different > market/purpose. Unfortunately, I am unable to reuse or adapt LGM PCIe driver > code for KMB. > > On the first concern, firmware will also change BIT(9) of this register in EP > mode only during its initialization. RC mode is fully initialize and controlled by > Linux. This function is being used only in keembay_pcie_start_link(), and I have > added an if condition to return 0 if the mode is EP mode. > > On the second concern, this driver will touch BIT(0) only, in RC mode. > This driver does not change this register in EP mode. > > Hope my explanation clears this matter. > And I believe we can keep this piece of code as it is. > > > > > > > > +static int keembay_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 > > func_no, > > > > > + enum pci_epc_irq_type type, > > > > > + u16 interrupt_num) { > > > > > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > > > > + > > > > > + switch (type) { > > > > > + case PCI_EPC_IRQ_LEGACY: > > > > > + /* Legacy interrupts are not supported in Keem Bay */ > > > > > + dev_err(pci->dev, "Legacy IRQ is not supported\n"); > > > > > + return -EINVAL; > > > > > + case PCI_EPC_IRQ_MSI: > > > > > + return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num); > > > > > + case PCI_EPC_IRQ_MSIX: > > > > > + return dw_pcie_ep_raise_msix_irq(ep, func_no, > > interrupt_num); > > > > > + default: > > > > > + dev_err(pci->dev, "Unknown IRQ type %d\n", type); > > > > > + return -EINVAL; > > > > > + } > > > > > > > > Doesn't the lack of a 'return' give a warning? > > > > > > Where? I don't see any lack of return. > > > > Is the compiler smart enough to recognize that with a return in every 'case' > > that we don't need a return after the switch? I wouldn't have thought so, but > > I haven't checked. > > I have rebuild the code with W=1 and W=3, and do not see compiler throw > warning with regards to this piece of code. I am using gcc v9.2. > > Should I keep the code as it is or make the change i.e. move the last return > outside of switch? No, it's fine. It was just a question. Rob