Re: [EXT] Re: [PATCH v2 1/1] PCI: layerscape: Add the endpoint linkup notifier support

2023-05-09 Thread Bjorn Helgaas
On Mon, May 08, 2023 at 09:45:59PM +, Frank Li wrote:
> > > > Subject: [EXT] Re: [PATCH v2 1/1] PCI: layerscape: Add the endpoint
> > linkup
> > > > notifier support
> > 
> > All these quoted headers are redundant clutter since we've already
> > seen them when Manivannan sent his comments.  It would be nice if your
> > mailer could be configured to omit them.
> 
> Our email client quite stupid. 

Yeah, sometimes those are really hard to work around.

Bjorn


RE: [EXT] Re: [PATCH v2 1/1] PCI: layerscape: Add the endpoint linkup notifier support

2023-05-08 Thread Frank Li
> > > Subject: [EXT] Re: [PATCH v2 1/1] PCI: layerscape: Add the endpoint
> linkup
> > > notifier support
> 
> All these quoted headers are redundant clutter since we've already
> seen them when Manivannan sent his comments.  It would be nice if your
> mailer could be configured to omit them.

Our email client quite stupid. 

> 
> > > > +static int ls_pcie_ep_interrupt_init(struct ls_pcie_ep *pcie,
> > > > +  struct platform_device *pdev)
> > > > +{
> > > > + u32 val;
> > > > + int ret;
> > > > +
> > > > + pcie->irq = platform_get_irq_byname(pdev, "pme");
> > > > + if (pcie->irq < 0) {
> > > > + dev_err(>dev, "Can't get 'pme' IRQ\n");
> > >
> > > PME
> >
> > Here should be dts property `pme`, suppose should match
> > platform_get_irq_byname(pdev, "pme");
> 
> You can also edit out all the other context and questions if you're
> not responding to them.
> 
> There were a lot of other comments that were useful but are not
> relevant to this reply.

Okay, I found I mess up patch version number.
 
> 
> Bjorn


Re: [EXT] Re: [PATCH v2 1/1] PCI: layerscape: Add the endpoint linkup notifier support

2023-05-08 Thread Bjorn Helgaas
On Mon, May 08, 2023 at 01:31:26PM +, Frank Li wrote:
> > -Original Message-
> > From: Manivannan Sadhasivam 
> > Sent: Saturday, May 6, 2023 2:59 AM
> > To: Frank Li 
> > Cc: M.H. Lian ; Mingkai Hu
> > ; Roy Zang ; Lorenzo Pieralisi
> > ; Rob Herring ; Krzysztof
> > Wilczyński ; Bjorn Helgaas ; open
> > list:PCI DRIVER FOR FREESCALE LAYERSCAPE ;
> > open list:PCI DRIVER FOR FREESCALE LAYERSCAPE  > p...@vger.kernel.org>; moderated list:PCI DRIVER FOR FREESCALE
> > LAYERSCAPE ; open list  > ker...@vger.kernel.org>; i...@lists.linux.dev
> > Subject: [EXT] Re: [PATCH v2 1/1] PCI: layerscape: Add the endpoint linkup
> > notifier support

All these quoted headers are redundant clutter since we've already
seen them when Manivannan sent his comments.  It would be nice if your
mailer could be configured to omit them.

> > > +static int ls_pcie_ep_interrupt_init(struct ls_pcie_ep *pcie,
> > > +  struct platform_device *pdev)
> > > +{
> > > + u32 val;
> > > + int ret;
> > > +
> > > + pcie->irq = platform_get_irq_byname(pdev, "pme");
> > > + if (pcie->irq < 0) {
> > > + dev_err(>dev, "Can't get 'pme' IRQ\n");
> > 
> > PME
> 
> Here should be dts property `pme`, suppose should match
> platform_get_irq_byname(pdev, "pme");

You can also edit out all the other context and questions if you're
not responding to them.

There were a lot of other comments that were useful but are not
relevant to this reply.

Bjorn


RE: [EXT] Re: [PATCH v2 1/1] PCI: layerscape: Add the endpoint linkup notifier support

2023-05-08 Thread Frank Li


> -Original Message-
> From: Manivannan Sadhasivam 
> Sent: Saturday, May 6, 2023 2:59 AM
> To: Frank Li 
> Cc: M.H. Lian ; Mingkai Hu
> ; Roy Zang ; Lorenzo Pieralisi
> ; Rob Herring ; Krzysztof
> Wilczyński ; Bjorn Helgaas ; open
> list:PCI DRIVER FOR FREESCALE LAYERSCAPE ;
> open list:PCI DRIVER FOR FREESCALE LAYERSCAPE  p...@vger.kernel.org>; moderated list:PCI DRIVER FOR FREESCALE
> LAYERSCAPE ; open list  ker...@vger.kernel.org>; i...@lists.linux.dev
> Subject: [EXT] Re: [PATCH v2 1/1] PCI: layerscape: Add the endpoint linkup
> notifier support
> 
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report
> this email' button
> 
> 
> On Mon, May 01, 2023 at 10:48:06AM -0400, Frank Li wrote:
> > Layerscape has PME interrupt, which can be used as linkup notifier.
> > Set CFG_READY bit when linkup detected.
> 
> Where are you setting this bit?
> 
> >
> > Signed-off-by: Xiaowei Bao 
> > Signed-off-by: Frank Li 
> > ---
> > Change from v1 to v2
> > - pme -> PME
> > - irq -> IRQ
> > - update dev_info message according to Bjorn's suggestion
> > - remove '.' at error message
> >
> >  .../pci/controller/dwc/pci-layerscape-ep.c| 104 +-
> >  1 file changed, 103 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > index c640db60edc6..e974fbe3b6d8 100644
> > --- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > +++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > @@ -18,6 +18,20 @@
> >
> >  #include "pcie-designware.h"
> >
> > +#define PEX_PF0_CONFIG   0xC0014
> > +#define PEX_PF0_CFG_READYBIT(0)
> > +
> > +/* PEX PFa PCIE PME and message interrupt registers*/
> > +#define PEX_PF0_PME_MES_DR   0xC0020
> > +#define PEX_PF0_PME_MES_DR_LUD   BIT(7)
> > +#define PEX_PF0_PME_MES_DR_LDD   BIT(9)
> > +#define PEX_PF0_PME_MES_DR_HRD   BIT(10)
> > +
> > +#define PEX_PF0_PME_MES_IER  0xC0028
> > +#define PEX_PF0_PME_MES_IER_LUDIEBIT(7)
> > +#define PEX_PF0_PME_MES_IER_LDDIEBIT(9)
> > +#define PEX_PF0_PME_MES_IER_HRDIEBIT(10)
> > +
> >  #define to_ls_pcie_ep(x) dev_get_drvdata((x)->dev)
> >
> >  struct ls_pcie_ep_drvdata {
> > @@ -30,8 +44,88 @@ struct ls_pcie_ep {
> >   struct dw_pcie  *pci;
> >   struct pci_epc_features *ls_epc;
> >   const struct ls_pcie_ep_drvdata *drvdata;
> > + boolbig_endian;
> > + int irq;
> >  };
> >
> > +static u32 ls_lut_readl(struct ls_pcie_ep *pcie, u32 offset)
> > +{
> > + struct dw_pcie *pci = pcie->pci;
> > +
> > + if (pcie->big_endian)
> > + return ioread32be(pci->dbi_base + offset);
> > + else
> > + return ioread32(pci->dbi_base + offset);
> > +}
> > +
> > +static void ls_lut_writel(struct ls_pcie_ep *pcie, u32 offset,
> > +   u32 value)
> 
> Above function argument could be wrapped within 80 columns.
> 
> > +{
> > + struct dw_pcie *pci = pcie->pci;
> > +
> > + if (pcie->big_endian)
> > + iowrite32be(value, pci->dbi_base + offset);
> > + else
> > + iowrite32(value, pci->dbi_base + offset);
> > +}
> > +
> > +static irqreturn_t ls_pcie_ep_event_handler(int irq, void *dev_id)
> > +{
> > + struct ls_pcie_ep *pcie = (struct ls_pcie_ep *)dev_id;
> 
> No need to do explicit typecase for void pointer.
> 
> > + struct dw_pcie *pci = pcie->pci;
> > + u32 val, cfg;
> > +
> > + val = ls_lut_readl(pcie, PEX_PF0_PME_MES_DR);
> > + if (!val)
> > + return IRQ_NONE;
> > +
> > + if (val & PEX_PF0_PME_MES_DR_LUD) {
> > + cfg = ls_lut_readl(pcie, PEX_PF0_CONFIG);
> > + cfg |= PEX_PF0_CFG_READY;
> > + ls_lut_writel(pcie, PEX_PF0_CONFIG, cfg);
> > + dw_pcie_ep_linkup(>ep);
> > +
> > + dev_info(pci->dev, "Link up\n");
> 
> These messages could be demoted to dev_dbg() logs.
> 
> > + } else if (val & PEX_PF0_PME_MES_DR_LDD) {
> > + dev_info(pci->dev, "Link down\n");
> >