From: Rob Herring <robh@kernel.org>
To: "Wan Mohamad,
Wan Ahmad Zainie" <wan.ahmad.zainie.wan.mohamad@intel.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
Bjorn Helgaas <bhelgaas@google.com>,
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
PCI <linux-pci@vger.kernel.org>,
"devicetree@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
Date: Mon, 14 Dec 2020 09:00:05 -0600 [thread overview]
Message-ID: <CAL_JsqL6smfyR6NvsW4BLdN02LtcMS1-==MsP3dS+xAbj6ZFfw@mail.gmail.com> (raw)
In-Reply-To: <DM6PR11MB3721054B20FE1853651B5E86DDC70@DM6PR11MB3721.namprd11.prod.outlook.com>
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
next prev parent reply other threads:[~2020-12-14 15:01 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-02 7:31 [PATCH v3 0/2] PCI: keembay: Add support for Intel Keem Bay Wan Ahmad Zainie
2020-12-02 7:31 ` [PATCH v3 1/2] dt-bindings: PCI: Add Intel Keem Bay PCIe controller Wan Ahmad Zainie
2020-12-09 18:15 ` Rob Herring
2020-12-02 7:31 ` [PATCH v3 2/2] PCI: keembay: Add support for Intel Keem Bay Wan Ahmad Zainie
2020-12-09 18:13 ` Rob Herring
2020-12-09 18:42 ` Andy Shevchenko
2020-12-10 17:46 ` Rob Herring
2020-12-14 13:03 ` Wan Mohamad, Wan Ahmad Zainie
2020-12-14 15:00 ` Rob Herring [this message]
2020-12-14 15:34 ` Andy Shevchenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAL_JsqL6smfyR6NvsW4BLdN02LtcMS1-==MsP3dS+xAbj6ZFfw@mail.gmail.com' \
--to=robh@kernel.org \
--cc=andriy.shevchenko@linux.intel.com \
--cc=bhelgaas@google.com \
--cc=devicetree@vger.kernel.org \
--cc=lakshmi.bai.raja.subramanian@intel.com \
--cc=linux-pci@vger.kernel.org \
--cc=lorenzo.pieralisi@arm.com \
--cc=mgross@linux.intel.com \
--cc=wan.ahmad.zainie.wan.mohamad@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).