RE: [PATCH v3 2/2] PCI: keembay: Add support for Intel Keem Bay

["Wan Mohamad, Wan Ahmad Zainie" <wan.ahmad.zainie.wan.mohamad@intel.com>]

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@kernel.org>
> Sent: Friday, December 11, 2020 1:47 AM
> To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Wan Mohamad, Wan Ahmad Zainie
> <wan.ahmad.zainie.wan.mohamad@intel.com>; Bjorn Helgaas
> <bhelgaas@google.com>; Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>; PCI
> <linux-pci@vger.kernel.org>; devicetree@vger.kernel.org; Mark Gross
> <mgross@linux.intel.com>; Raja Subramanian, Lakshmi Bai
> <lakshmi.bai.raja.subramanian@intel.com>
> 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@linux.intel.com> 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?

> 
> Rob

Best regards,
Zainie