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

[Rob Herring <robh@kernel.org>]

On Mon, Dec 14, 2020 at 7:03 AM Wan Mohamad, Wan Ahmad Zainie
<wan.ahmad.zainie.wan.mohamad@intel.com> 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@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?

No, it's fine. It was just a question.

Rob