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

[Rob Herring <robh@kernel.org>]

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.

> > > +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.

Rob