[2/3] PCI: layerscape: add suspend/resume for ls1021a

  ls1021a add suspend/resume support.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/pci/controller/dwc/pci-layerscape.c | 88 ++++++++++++++++++++-
 1 file changed, 87 insertions(+), 1 deletion(-)
  

On Fri, Sep 15, 2023 at 02:43:05PM -0400, Frank Li wrote:
> ls1021a add suspend/resume support.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---

ping

Frank

>  drivers/pci/controller/dwc/pci-layerscape.c | 88 ++++++++++++++++++++-
>  1 file changed, 87 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-layerscape.c b/drivers/pci/controller/dwc/pci-layerscape.c
> index 20c48c06e2248..bc5a8ff1a26ce 100644
> --- a/drivers/pci/controller/dwc/pci-layerscape.c
> +++ b/drivers/pci/controller/dwc/pci-layerscape.c
> @@ -35,6 +35,12 @@
>  #define PF_MCR_PTOMR		BIT(0)
>  #define PF_MCR_EXL2S		BIT(1)
>  
> +/* LS1021A PEXn PM Write Control Register */
> +#define SCFG_PEXPMWRCR(idx)	(0x5c + (idx) * 0x64)
> +#define PMXMTTURNOFF		BIT(31)
> +#define SCFG_PEXSFTRSTCR	0x190
> +#define PEXSR(idx)		BIT(idx)
> +
>  #define PCIE_IATU_NUM		6
>  
>  struct ls_pcie_drvdata {
> @@ -48,6 +54,8 @@ struct ls_pcie {
>  	struct dw_pcie *pci;
>  	const struct ls_pcie_drvdata *drvdata;
>  	void __iomem *pf_base;
> +	struct regmap *scfg;
> +	int index;
>  	bool big_endian;
>  };
>  
> @@ -170,13 +178,91 @@ static int ls_pcie_host_init(struct dw_pcie_rp *pp)
>  	return 0;
>  }
>  
> +static void ls1021a_pcie_send_turnoff_msg(struct dw_pcie_rp *pp)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct ls_pcie *pcie = to_ls_pcie(pci);
> +	u32 val;
> +
> +	if (!pcie->scfg) {
> +		dev_dbg(pcie->pci->dev, "SYSCFG is NULL\n");
> +		return;
> +	}
> +
> +	/* Send Turn_off message */
> +	regmap_read(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), &val);
> +	val |= PMXMTTURNOFF;
> +	regmap_write(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), val);
> +
> +	/* There are not register to check ACK, so wait PCIE_PME_TO_L2_TIMEOUT_US */
> +	mdelay(PCIE_PME_TO_L2_TIMEOUT_US/1000);
> +
> +	/* Clear Turn_off message */
> +	regmap_read(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), &val);
> +	val &= ~PMXMTTURNOFF;
> +	regmap_write(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), val);
> +}
> +
> +static void ls1021a_pcie_exit_from_l2(struct dw_pcie_rp *pp)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct ls_pcie *pcie = to_ls_pcie(pci);
> +	u32 val;
> +
> +	regmap_read(pcie->scfg, SCFG_PEXSFTRSTCR, &val);
> +	val |= PEXSR(pcie->index);
> +	regmap_write(pcie->scfg, SCFG_PEXSFTRSTCR, val);
> +
> +	regmap_read(pcie->scfg, SCFG_PEXSFTRSTCR, &val);
> +	val &= ~PEXSR(pcie->index);
> +	regmap_write(pcie->scfg, SCFG_PEXSFTRSTCR, val);
> +}
> +
> +static int ls1021a_pcie_host_init(struct dw_pcie_rp *pp)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct ls_pcie *pcie = to_ls_pcie(pci);
> +	struct device *dev = pcie->pci->dev;
> +	u32 index[2];
> +	int ret;
> +
> +	ret = ls_pcie_host_init(pp);
> +	if (ret)
> +		return ret;
> +
> +	pcie->scfg = syscon_regmap_lookup_by_phandle(dev->of_node, "fsl,pcie-scfg");
> +	if (IS_ERR(pcie->scfg)) {
> +		ret = PTR_ERR(pcie->scfg);
> +		dev_err(dev, "No syscfg phandle specified\n");
> +		pcie->scfg = NULL;
> +		return ret;
> +	}
> +
> +	ret = of_property_read_u32_array(dev->of_node, "fsl,pcie-scfg", index, 2);
> +	if (ret) {
> +		pcie->scfg = NULL;
> +		return ret;
> +	}
> +
> +	pcie->index = index[1];
> +
> +	return ret;
> +}
> +
>  static const struct dw_pcie_host_ops ls_pcie_host_ops = {
>  	.host_init = ls_pcie_host_init,
>  	.pme_turn_off = ls_pcie_send_turnoff_msg,
>  };
>  
> +static const struct dw_pcie_host_ops ls1021a_pcie_host_ops = {
> +	.host_init = ls1021a_pcie_host_init,
> +	.pme_turn_off = ls1021a_pcie_send_turnoff_msg,
> +};
> +
>  static const struct ls_pcie_drvdata ls1021a_drvdata = {
> -	.pm_support = false,
> +	.pm_support = true,
> +	.ops = &ls1021a_pcie_host_ops,
> +	.exit_from_l2 = ls1021a_pcie_exit_from_l2,
>  };
>  
>  static const struct ls_pcie_drvdata layerscape_drvdata = {
> -- 
> 2.34.1
>
  

On Wed, Oct 04, 2023 at 10:23:51AM -0400, Frank Li wrote:
> On Fri, Sep 15, 2023 at 02:43:05PM -0400, Frank Li wrote:
> > ls1021a add suspend/resume support.
> > 
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> 
> ping
> 
> Frank

Ping

Frank

> 
> >  drivers/pci/controller/dwc/pci-layerscape.c | 88 ++++++++++++++++++++-
> >  1 file changed, 87 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pci-layerscape.c b/drivers/pci/controller/dwc/pci-layerscape.c
> > index 20c48c06e2248..bc5a8ff1a26ce 100644
> > --- a/drivers/pci/controller/dwc/pci-layerscape.c
> > +++ b/drivers/pci/controller/dwc/pci-layerscape.c
> > @@ -35,6 +35,12 @@
> >  #define PF_MCR_PTOMR		BIT(0)
> >  #define PF_MCR_EXL2S		BIT(1)
> >  
> > +/* LS1021A PEXn PM Write Control Register */
> > +#define SCFG_PEXPMWRCR(idx)	(0x5c + (idx) * 0x64)
> > +#define PMXMTTURNOFF		BIT(31)
> > +#define SCFG_PEXSFTRSTCR	0x190
> > +#define PEXSR(idx)		BIT(idx)
> > +
> >  #define PCIE_IATU_NUM		6
> >  
> >  struct ls_pcie_drvdata {
> > @@ -48,6 +54,8 @@ struct ls_pcie {
> >  	struct dw_pcie *pci;
> >  	const struct ls_pcie_drvdata *drvdata;
> >  	void __iomem *pf_base;
> > +	struct regmap *scfg;
> > +	int index;
> >  	bool big_endian;
> >  };
> >  
> > @@ -170,13 +178,91 @@ static int ls_pcie_host_init(struct dw_pcie_rp *pp)
> >  	return 0;
> >  }
> >  
> > +static void ls1021a_pcie_send_turnoff_msg(struct dw_pcie_rp *pp)
> > +{
> > +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +	struct ls_pcie *pcie = to_ls_pcie(pci);
> > +	u32 val;
> > +
> > +	if (!pcie->scfg) {
> > +		dev_dbg(pcie->pci->dev, "SYSCFG is NULL\n");
> > +		return;
> > +	}
> > +
> > +	/* Send Turn_off message */
> > +	regmap_read(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), &val);
> > +	val |= PMXMTTURNOFF;
> > +	regmap_write(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), val);
> > +
> > +	/* There are not register to check ACK, so wait PCIE_PME_TO_L2_TIMEOUT_US */
> > +	mdelay(PCIE_PME_TO_L2_TIMEOUT_US/1000);
> > +
> > +	/* Clear Turn_off message */
> > +	regmap_read(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), &val);
> > +	val &= ~PMXMTTURNOFF;
> > +	regmap_write(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), val);
> > +}
> > +
> > +static void ls1021a_pcie_exit_from_l2(struct dw_pcie_rp *pp)
> > +{
> > +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +	struct ls_pcie *pcie = to_ls_pcie(pci);
> > +	u32 val;
> > +
> > +	regmap_read(pcie->scfg, SCFG_PEXSFTRSTCR, &val);
> > +	val |= PEXSR(pcie->index);
> > +	regmap_write(pcie->scfg, SCFG_PEXSFTRSTCR, val);
> > +
> > +	regmap_read(pcie->scfg, SCFG_PEXSFTRSTCR, &val);
> > +	val &= ~PEXSR(pcie->index);
> > +	regmap_write(pcie->scfg, SCFG_PEXSFTRSTCR, val);
> > +}
> > +
> > +static int ls1021a_pcie_host_init(struct dw_pcie_rp *pp)
> > +{
> > +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +	struct ls_pcie *pcie = to_ls_pcie(pci);
> > +	struct device *dev = pcie->pci->dev;
> > +	u32 index[2];
> > +	int ret;
> > +
> > +	ret = ls_pcie_host_init(pp);
> > +	if (ret)
> > +		return ret;
> > +
> > +	pcie->scfg = syscon_regmap_lookup_by_phandle(dev->of_node, "fsl,pcie-scfg");
> > +	if (IS_ERR(pcie->scfg)) {
> > +		ret = PTR_ERR(pcie->scfg);
> > +		dev_err(dev, "No syscfg phandle specified\n");
> > +		pcie->scfg = NULL;
> > +		return ret;
> > +	}
> > +
> > +	ret = of_property_read_u32_array(dev->of_node, "fsl,pcie-scfg", index, 2);
> > +	if (ret) {
> > +		pcie->scfg = NULL;
> > +		return ret;
> > +	}
> > +
> > +	pcie->index = index[1];
> > +
> > +	return ret;
> > +}
> > +
> >  static const struct dw_pcie_host_ops ls_pcie_host_ops = {
> >  	.host_init = ls_pcie_host_init,
> >  	.pme_turn_off = ls_pcie_send_turnoff_msg,
> >  };
> >  
> > +static const struct dw_pcie_host_ops ls1021a_pcie_host_ops = {
> > +	.host_init = ls1021a_pcie_host_init,
> > +	.pme_turn_off = ls1021a_pcie_send_turnoff_msg,
> > +};
> > +
> >  static const struct ls_pcie_drvdata ls1021a_drvdata = {
> > -	.pm_support = false,
> > +	.pm_support = true,
> > +	.ops = &ls1021a_pcie_host_ops,
> > +	.exit_from_l2 = ls1021a_pcie_exit_from_l2,
> >  };
> >  
> >  static const struct ls_pcie_drvdata layerscape_drvdata = {
> > -- 
> > 2.34.1
> >
  

On Tue, Oct 10, 2023 at 10:20:12AM -0400, Frank Li wrote:
> On Wed, Oct 04, 2023 at 10:23:51AM -0400, Frank Li wrote:
> > On Fri, Sep 15, 2023 at 02:43:05PM -0400, Frank Li wrote:
> > > ls1021a add suspend/resume support.
> > > 
> > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > ---
> > 
> > ping
> > 
> > Frank
> 
> Ping

Read and follow please (and then ping us):
https://lore.kernel.org/linux-pci/20171026223701.GA25649@bhelgaas-glaptop.roam.corp.google.com

> Frank
> 
> > 
> > >  drivers/pci/controller/dwc/pci-layerscape.c | 88 ++++++++++++++++++++-
> > >  1 file changed, 87 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pci-layerscape.c b/drivers/pci/controller/dwc/pci-layerscape.c
> > > index 20c48c06e2248..bc5a8ff1a26ce 100644
> > > --- a/drivers/pci/controller/dwc/pci-layerscape.c
> > > +++ b/drivers/pci/controller/dwc/pci-layerscape.c
> > > @@ -35,6 +35,12 @@
> > >  #define PF_MCR_PTOMR		BIT(0)
> > >  #define PF_MCR_EXL2S		BIT(1)
> > >  
> > > +/* LS1021A PEXn PM Write Control Register */
> > > +#define SCFG_PEXPMWRCR(idx)	(0x5c + (idx) * 0x64)
> > > +#define PMXMTTURNOFF		BIT(31)
> > > +#define SCFG_PEXSFTRSTCR	0x190
> > > +#define PEXSR(idx)		BIT(idx)
> > > +
> > >  #define PCIE_IATU_NUM		6
> > >  
> > >  struct ls_pcie_drvdata {
> > > @@ -48,6 +54,8 @@ struct ls_pcie {
> > >  	struct dw_pcie *pci;
> > >  	const struct ls_pcie_drvdata *drvdata;
> > >  	void __iomem *pf_base;
> > > +	struct regmap *scfg;
> > > +	int index;
> > >  	bool big_endian;
> > >  };
> > >  
> > > @@ -170,13 +178,91 @@ static int ls_pcie_host_init(struct dw_pcie_rp *pp)
> > >  	return 0;
> > >  }
> > >  
> > > +static void ls1021a_pcie_send_turnoff_msg(struct dw_pcie_rp *pp)
> > > +{
> > > +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > +	struct ls_pcie *pcie = to_ls_pcie(pci);
> > > +	u32 val;
> > > +
> > > +	if (!pcie->scfg) {
> > > +		dev_dbg(pcie->pci->dev, "SYSCFG is NULL\n");
> > > +		return;
> > > +	}
> > > +
> > > +	/* Send Turn_off message */
> > > +	regmap_read(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), &val);
> > > +	val |= PMXMTTURNOFF;
> > > +	regmap_write(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), val);
> > > +
> > > +	/* There are not register to check ACK, so wait PCIE_PME_TO_L2_TIMEOUT_US */
> > > +	mdelay(PCIE_PME_TO_L2_TIMEOUT_US/1000);
> > > +
> > > +	/* Clear Turn_off message */
> > > +	regmap_read(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), &val);
> > > +	val &= ~PMXMTTURNOFF;
> > > +	regmap_write(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), val);
> > > +}
> > > +
> > > +static void ls1021a_pcie_exit_from_l2(struct dw_pcie_rp *pp)
> > > +{
> > > +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > +	struct ls_pcie *pcie = to_ls_pcie(pci);
> > > +	u32 val;
> > > +
> > > +	regmap_read(pcie->scfg, SCFG_PEXSFTRSTCR, &val);
> > > +	val |= PEXSR(pcie->index);
> > > +	regmap_write(pcie->scfg, SCFG_PEXSFTRSTCR, val);
> > > +
> > > +	regmap_read(pcie->scfg, SCFG_PEXSFTRSTCR, &val);
> > > +	val &= ~PEXSR(pcie->index);
> > > +	regmap_write(pcie->scfg, SCFG_PEXSFTRSTCR, val);
> > > +}
> > > +
> > > +static int ls1021a_pcie_host_init(struct dw_pcie_rp *pp)
> > > +{
> > > +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > +	struct ls_pcie *pcie = to_ls_pcie(pci);
> > > +	struct device *dev = pcie->pci->dev;
> > > +	u32 index[2];
> > > +	int ret;
> > > +
> > > +	ret = ls_pcie_host_init(pp);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	pcie->scfg = syscon_regmap_lookup_by_phandle(dev->of_node, "fsl,pcie-scfg");
> > > +	if (IS_ERR(pcie->scfg)) {
> > > +		ret = PTR_ERR(pcie->scfg);
> > > +		dev_err(dev, "No syscfg phandle specified\n");
> > > +		pcie->scfg = NULL;
> > > +		return ret;
> > > +	}
> > > +
> > > +	ret = of_property_read_u32_array(dev->of_node, "fsl,pcie-scfg", index, 2);
> > > +	if (ret) {
> > > +		pcie->scfg = NULL;
> > > +		return ret;
> > > +	}
> > > +
> > > +	pcie->index = index[1];
> > > +
> > > +	return ret;
> > > +}
> > > +
> > >  static const struct dw_pcie_host_ops ls_pcie_host_ops = {
> > >  	.host_init = ls_pcie_host_init,
> > >  	.pme_turn_off = ls_pcie_send_turnoff_msg,
> > >  };
> > >  
> > > +static const struct dw_pcie_host_ops ls1021a_pcie_host_ops = {
> > > +	.host_init = ls1021a_pcie_host_init,
> > > +	.pme_turn_off = ls1021a_pcie_send_turnoff_msg,
> > > +};
> > > +
> > >  static const struct ls_pcie_drvdata ls1021a_drvdata = {
> > > -	.pm_support = false,
> > > +	.pm_support = true,
> > > +	.ops = &ls1021a_pcie_host_ops,
> > > +	.exit_from_l2 = ls1021a_pcie_exit_from_l2,
> > >  };
> > >  
> > >  static const struct ls_pcie_drvdata layerscape_drvdata = {
> > > -- 
> > > 2.34.1
> > >
  

On Tue, Oct 10, 2023 at 06:02:36PM +0200, Lorenzo Pieralisi wrote:
> On Tue, Oct 10, 2023 at 10:20:12AM -0400, Frank Li wrote:
> > On Wed, Oct 04, 2023 at 10:23:51AM -0400, Frank Li wrote:
> > > On Fri, Sep 15, 2023 at 02:43:05PM -0400, Frank Li wrote:
> > > > ls1021a add suspend/resume support.
> > > > 
> > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > ---
> > > 
> > > ping
> > > 
> > > Frank
> > 
> > Ping
> 
> Read and follow please (and then ping us):
> https://lore.kernel.org/linux-pci/20171026223701.GA25649@bhelgaas-glaptop.roam.corp.google.com

Could you please help point which specic one was not follow aboved guide?
Then I can update my code. I think that's efficial communication method. I
think I have read it serial times. But not sure which one violate the
guide?

@Bjorn Helgaas. How do you think so? 

best regards
Frank Li

> 
> > Frank
> > 
> > > 
> > > >  drivers/pci/controller/dwc/pci-layerscape.c | 88 ++++++++++++++++++++-
> > > >  1 file changed, 87 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/pci/controller/dwc/pci-layerscape.c b/drivers/pci/controller/dwc/pci-layerscape.c
> > > > index 20c48c06e2248..bc5a8ff1a26ce 100644
> > > > --- a/drivers/pci/controller/dwc/pci-layerscape.c
> > > > +++ b/drivers/pci/controller/dwc/pci-layerscape.c
> > > > @@ -35,6 +35,12 @@
> > > >  #define PF_MCR_PTOMR		BIT(0)
> > > >  #define PF_MCR_EXL2S		BIT(1)
> > > >  
> > > > +/* LS1021A PEXn PM Write Control Register */
> > > > +#define SCFG_PEXPMWRCR(idx)	(0x5c + (idx) * 0x64)
> > > > +#define PMXMTTURNOFF		BIT(31)
> > > > +#define SCFG_PEXSFTRSTCR	0x190
> > > > +#define PEXSR(idx)		BIT(idx)
> > > > +
> > > >  #define PCIE_IATU_NUM		6
> > > >  
> > > >  struct ls_pcie_drvdata {
> > > > @@ -48,6 +54,8 @@ struct ls_pcie {
> > > >  	struct dw_pcie *pci;
> > > >  	const struct ls_pcie_drvdata *drvdata;
> > > >  	void __iomem *pf_base;
> > > > +	struct regmap *scfg;
> > > > +	int index;
> > > >  	bool big_endian;
> > > >  };
> > > >  
> > > > @@ -170,13 +178,91 @@ static int ls_pcie_host_init(struct dw_pcie_rp *pp)
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +static void ls1021a_pcie_send_turnoff_msg(struct dw_pcie_rp *pp)
> > > > +{
> > > > +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > > +	struct ls_pcie *pcie = to_ls_pcie(pci);
> > > > +	u32 val;
> > > > +
> > > > +	if (!pcie->scfg) {
> > > > +		dev_dbg(pcie->pci->dev, "SYSCFG is NULL\n");
> > > > +		return;
> > > > +	}
> > > > +
> > > > +	/* Send Turn_off message */
> > > > +	regmap_read(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), &val);
> > > > +	val |= PMXMTTURNOFF;
> > > > +	regmap_write(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), val);
> > > > +
> > > > +	/* There are not register to check ACK, so wait PCIE_PME_TO_L2_TIMEOUT_US */
> > > > +	mdelay(PCIE_PME_TO_L2_TIMEOUT_US/1000);
> > > > +
> > > > +	/* Clear Turn_off message */
> > > > +	regmap_read(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), &val);
> > > > +	val &= ~PMXMTTURNOFF;
> > > > +	regmap_write(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), val);
> > > > +}
> > > > +
> > > > +static void ls1021a_pcie_exit_from_l2(struct dw_pcie_rp *pp)
> > > > +{
> > > > +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > > +	struct ls_pcie *pcie = to_ls_pcie(pci);
> > > > +	u32 val;
> > > > +
> > > > +	regmap_read(pcie->scfg, SCFG_PEXSFTRSTCR, &val);
> > > > +	val |= PEXSR(pcie->index);
> > > > +	regmap_write(pcie->scfg, SCFG_PEXSFTRSTCR, val);
> > > > +
> > > > +	regmap_read(pcie->scfg, SCFG_PEXSFTRSTCR, &val);
> > > > +	val &= ~PEXSR(pcie->index);
> > > > +	regmap_write(pcie->scfg, SCFG_PEXSFTRSTCR, val);
> > > > +}
> > > > +
> > > > +static int ls1021a_pcie_host_init(struct dw_pcie_rp *pp)
> > > > +{
> > > > +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > > +	struct ls_pcie *pcie = to_ls_pcie(pci);
> > > > +	struct device *dev = pcie->pci->dev;
> > > > +	u32 index[2];
> > > > +	int ret;
> > > > +
> > > > +	ret = ls_pcie_host_init(pp);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	pcie->scfg = syscon_regmap_lookup_by_phandle(dev->of_node, "fsl,pcie-scfg");
> > > > +	if (IS_ERR(pcie->scfg)) {
> > > > +		ret = PTR_ERR(pcie->scfg);
> > > > +		dev_err(dev, "No syscfg phandle specified\n");
> > > > +		pcie->scfg = NULL;
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	ret = of_property_read_u32_array(dev->of_node, "fsl,pcie-scfg", index, 2);
> > > > +	if (ret) {
> > > > +		pcie->scfg = NULL;
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	pcie->index = index[1];
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > >  static const struct dw_pcie_host_ops ls_pcie_host_ops = {
> > > >  	.host_init = ls_pcie_host_init,
> > > >  	.pme_turn_off = ls_pcie_send_turnoff_msg,
> > > >  };
> > > >  
> > > > +static const struct dw_pcie_host_ops ls1021a_pcie_host_ops = {
> > > > +	.host_init = ls1021a_pcie_host_init,
> > > > +	.pme_turn_off = ls1021a_pcie_send_turnoff_msg,
> > > > +};
> > > > +
> > > >  static const struct ls_pcie_drvdata ls1021a_drvdata = {
> > > > -	.pm_support = false,
> > > > +	.pm_support = true,
> > > > +	.ops = &ls1021a_pcie_host_ops,
> > > > +	.exit_from_l2 = ls1021a_pcie_exit_from_l2,
> > > >  };
> > > >  
> > > >  static const struct ls_pcie_drvdata layerscape_drvdata = {
> > > > -- 
> > > > 2.34.1
> > > >
  

On Mon, Oct 16, 2023 at 10:45:25AM -0400, Frank Li wrote:
> On Tue, Oct 10, 2023 at 06:02:36PM +0200, Lorenzo Pieralisi wrote:
> > On Tue, Oct 10, 2023 at 10:20:12AM -0400, Frank Li wrote:

> > > Ping
> > 
> > Read and follow please (and then ping us):
> > https://lore.kernel.org/linux-pci/20171026223701.GA25649@bhelgaas-glaptop.roam.corp.google.com
> 
> Could you please help point which specic one was not follow aboved guide?
> Then I can update my code. I think that's efficial communication method. I
> think I have read it serial times. But not sure which one violate the
> guide?
> 
> @Bjorn Helgaas. How do you think so? 

Since Lorenzo didn't point out anything specific in the patch itself,
I think he was probably referring to the subject line and this advice:

  - Follow the existing convention, i.e., run "git log --oneline
    <file>" and make yours match in format, capitalization, and
    sentence structure.  For example, native host bridge driver patch
    titles look like this:

      PCI: altera: Fix platform_get_irq() error handling
      PCI: vmd: Remove IRQ affinity so we can allocate more IRQs
      PCI: mediatek: Add MSI support for MT2712 and MT7622
      PCI: rockchip: Remove IRQ domain if probe fails

In this case, your subject line was:

  PCI: layerscape: add suspend/resume for ls1021a

The advice was to run this:

  $ git log --oneline drivers/pci/controller/dwc/pci-layerscape.c
  83c088148c8e PCI: Use PCI_HEADER_TYPE_* instead of literals
  9fda4d09905d PCI: layerscape: Add power management support for ls1028a
  277004d7a4a3 PCI: Remove unnecessary <linux/of_irq.h> includes
  60b3c27fb9b9 PCI: dwc: Rename struct pcie_port to dw_pcie_rp
  d23f0c11aca2 PCI: layerscape: Change to use the DWC common link-up check function
  7007b745a508 PCI: layerscape: Convert to builtin_platform_driver()
  60f5b73fa0f2 PCI: dwc: Remove unnecessary wrappers around dw_pcie_host_init()
  b9ac0f9dc8ea PCI: dwc: Move dw_pcie_setup_rc() to DWC common code
  f78f02638af5 PCI: dwc: Rework MSI initialization

Note that these summaries are all complete sentences that start with a
capital letter:

  Use PCI_HEADER_TYPE_* instead of literals
  Add power management support for ls1028a
  Remove unnecessary <linux/of_irq.h> includes
  ...

So yours could be this:

  PCI: layerscape: Add suspend/resume for ls1021a
                   ^

This is trivial, obviously.  But the uppercase/lowercase distinction
carries information, and it's an unnecessary distraction to notice
that "oh, this is different from the rest; is the difference
important or should I ignore it?"

Obviously Lorenzo *could* edit all your subject lines on your behalf,
but it makes everybody's life easier if people look at the existing
code and follow the style when making changes.

E.g., write subject lines that are similar in style to previous ones,
name local variables similarly to other functions, use line lengths
consistent with the rest of the file, etc.  After applying a change,
the file should look like a coherent whole; we should not be able to
tell that this hunk was added later by somebody else.  This all helps
make the code (and the git history) more readable and maintainable.

Bjorn
  

On Mon, Oct 16, 2023 at 10:22:11AM -0500, Bjorn Helgaas wrote:
> On Mon, Oct 16, 2023 at 10:45:25AM -0400, Frank Li wrote:
> > On Tue, Oct 10, 2023 at 06:02:36PM +0200, Lorenzo Pieralisi wrote:
> > > On Tue, Oct 10, 2023 at 10:20:12AM -0400, Frank Li wrote:
> 
> > > > Ping
> > > 
> > > Read and follow please (and then ping us):
> > > https://lore.kernel.org/linux-pci/20171026223701.GA25649@bhelgaas-glaptop.roam.corp.google.com
> > 
> > Could you please help point which specic one was not follow aboved guide?
> > Then I can update my code. I think that's efficial communication method. I
> > think I have read it serial times. But not sure which one violate the
> > guide?
> > 
> > @Bjorn Helgaas. How do you think so? 
> 
> Since Lorenzo didn't point out anything specific in the patch itself,
> I think he was probably referring to the subject line and this advice:
> 
>   - Follow the existing convention, i.e., run "git log --oneline
>     <file>" and make yours match in format, capitalization, and
>     sentence structure.  For example, native host bridge driver patch
>     titles look like this:
> 
>       PCI: altera: Fix platform_get_irq() error handling
>       PCI: vmd: Remove IRQ affinity so we can allocate more IRQs
>       PCI: mediatek: Add MSI support for MT2712 and MT7622
>       PCI: rockchip: Remove IRQ domain if probe fails
> 
> In this case, your subject line was:
> 
>   PCI: layerscape: add suspend/resume for ls1021a
> 
> The advice was to run this:
> 
>   $ git log --oneline drivers/pci/controller/dwc/pci-layerscape.c
>   83c088148c8e PCI: Use PCI_HEADER_TYPE_* instead of literals
>   9fda4d09905d PCI: layerscape: Add power management support for ls1028a
>   277004d7a4a3 PCI: Remove unnecessary <linux/of_irq.h> includes
>   60b3c27fb9b9 PCI: dwc: Rename struct pcie_port to dw_pcie_rp
>   d23f0c11aca2 PCI: layerscape: Change to use the DWC common link-up check function
>   7007b745a508 PCI: layerscape: Convert to builtin_platform_driver()
>   60f5b73fa0f2 PCI: dwc: Remove unnecessary wrappers around dw_pcie_host_init()
>   b9ac0f9dc8ea PCI: dwc: Move dw_pcie_setup_rc() to DWC common code
>   f78f02638af5 PCI: dwc: Rework MSI initialization
> 
> Note that these summaries are all complete sentences that start with a
> capital letter:
> 
>   Use PCI_HEADER_TYPE_* instead of literals
>   Add power management support for ls1028a
>   Remove unnecessary <linux/of_irq.h> includes
>   ...
> 
> So yours could be this:
> 
>   PCI: layerscape: Add suspend/resume for ls1021a
>                    ^
> 
> This is trivial, obviously.  But the uppercase/lowercase distinction
> carries information, and it's an unnecessary distraction to notice
> that "oh, this is different from the rest; is the difference
> important or should I ignore it?"

Thanks. Not everyone think it is trivial. Especially for the one, who
English are not native language.

> 
> Obviously Lorenzo *could* edit all your subject lines on your behalf,
> but it makes everybody's life easier if people look at the existing
> code and follow the style when making changes.

Understand, but simple mark 'a' and 'A' to me. I will update patches and
take care for next time instead search whole docuemnt to guess which one
violated. I know I make some mistakes at here. But I am working on many
difference kernel subsystems, some require upper case, some require low
case, someone doesn't care.
 
I respected all reviewer's and maintaner's time, but I hope respected
the contributor's time also.

Just simple words like , 'a' to 'A' or
run git log --oneline to check subject. I will know what exact means. 

Do you think it will better than below words

"Read and follow please (and then ping us):                           
https://lore.kernel.org/linux-pci/20171026223701.GA25649@bhelgaas-glaptop.roam.corp.google.com"

Frank

> 
> E.g., write subject lines that are similar in style to previous ones,
> name local variables similarly to other functions, use line lengths
> consistent with the rest of the file, etc.  After applying a change,
> the file should look like a coherent whole; we should not be able to
> tell that this hunk was added later by somebody else.  This all helps
> make the code (and the git history) more readable and maintainable.
> 
> Bjorn
  

On Mon, Oct 16, 2023 at 10:22:11AM -0500, Bjorn Helgaas wrote:
> On Mon, Oct 16, 2023 at 10:45:25AM -0400, Frank Li wrote:
> > On Tue, Oct 10, 2023 at 06:02:36PM +0200, Lorenzo Pieralisi wrote:
> > > On Tue, Oct 10, 2023 at 10:20:12AM -0400, Frank Li wrote:
> 
> > > > Ping
> > > 
> > > Read and follow please (and then ping us):
> > > https://lore.kernel.org/linux-pci/20171026223701.GA25649@bhelgaas-glaptop.roam.corp.google.com
> > 
> > Could you please help point which specic one was not follow aboved guide?
> > Then I can update my code. I think that's efficial communication method. I
> > think I have read it serial times. But not sure which one violate the
> > guide?
> > 
> > @Bjorn Helgaas. How do you think so? 
> 
> Since Lorenzo didn't point out anything specific in the patch itself,
> I think he was probably referring to the subject line and this advice:
> 
>   - Follow the existing convention, i.e., run "git log --oneline
>     <file>" and make yours match in format, capitalization, and
>     sentence structure.  For example, native host bridge driver patch
>     titles look like this:
> 
>       PCI: altera: Fix platform_get_irq() error handling
>       PCI: vmd: Remove IRQ affinity so we can allocate more IRQs
>       PCI: mediatek: Add MSI support for MT2712 and MT7622
>       PCI: rockchip: Remove IRQ domain if probe fails
> 
> In this case, your subject line was:
> 
>   PCI: layerscape: add suspend/resume for ls1021a
> 
> The advice was to run this:
> 
>   $ git log --oneline drivers/pci/controller/dwc/pci-layerscape.c
>   83c088148c8e PCI: Use PCI_HEADER_TYPE_* instead of literals
>   9fda4d09905d PCI: layerscape: Add power management support for ls1028a
>   277004d7a4a3 PCI: Remove unnecessary <linux/of_irq.h> includes
>   60b3c27fb9b9 PCI: dwc: Rename struct pcie_port to dw_pcie_rp
>   d23f0c11aca2 PCI: layerscape: Change to use the DWC common link-up check function
>   7007b745a508 PCI: layerscape: Convert to builtin_platform_driver()
>   60f5b73fa0f2 PCI: dwc: Remove unnecessary wrappers around dw_pcie_host_init()
>   b9ac0f9dc8ea PCI: dwc: Move dw_pcie_setup_rc() to DWC common code
>   f78f02638af5 PCI: dwc: Rework MSI initialization
> 
> Note that these summaries are all complete sentences that start with a
> capital letter:
> 
>   Use PCI_HEADER_TYPE_* instead of literals
>   Add power management support for ls1028a
>   Remove unnecessary <linux/of_irq.h> includes
>   ...
> 
> So yours could be this:
> 
>   PCI: layerscape: Add suspend/resume for ls1021a
>                    ^
> 
> This is trivial, obviously.  But the uppercase/lowercase distinction
> carries information, and it's an unnecessary distraction to notice
> that "oh, this is different from the rest; is the difference
> important or should I ignore it?"
> 
> Obviously Lorenzo *could* edit all your subject lines on your behalf,
> but it makes everybody's life easier if people look at the existing
> code and follow the style when making changes.
> 
> E.g., write subject lines that are similar in style to previous ones,
> name local variables similarly to other functions, use line lengths
> consistent with the rest of the file, etc.  After applying a change,
> the file should look like a coherent whole; we should not be able to
> tell that this hunk was added later by somebody else.  This all helps
> make the code (and the git history) more readable and maintainable.

Thank you very much Bjorn. Frank, now please resend your patches and
make sure that you follow these guidelines, they must be clear by now.

Lorenzo
  

On Mon, Oct 16, 2023 at 12:11:04PM -0400, Frank Li wrote:
> On Mon, Oct 16, 2023 at 10:22:11AM -0500, Bjorn Helgaas wrote:

> > Obviously Lorenzo *could* edit all your subject lines on your behalf,
> > but it makes everybody's life easier if people look at the existing
> > code and follow the style when making changes.
> 
> Understand, but simple mark 'a' and 'A' to me. I will update patches and
> take care for next time instead search whole docuemnt to guess which one
> violated. I know I make some mistakes at here. But I am working on many
> difference kernel subsystems, some require upper case, some require low
> case, someone doesn't care.

Right, that's why I always suggest following the example of the
surrounding code and history.  English is the only language I know,
but I speculate that this typographical detail probably doesn't make
sense in languages that don't have a similar upper/lowercase
distinction.

Thanks for persevering; we'd be having a lot more trouble if I tried
to send emails in your native language ;)

Bjorn
  

On Mon, Oct 16, 2023 at 12:11:04PM -0400, Frank Li wrote:
> On Mon, Oct 16, 2023 at 10:22:11AM -0500, Bjorn Helgaas wrote:
> > On Mon, Oct 16, 2023 at 10:45:25AM -0400, Frank Li wrote:
> > > On Tue, Oct 10, 2023 at 06:02:36PM +0200, Lorenzo Pieralisi wrote:
> > > > On Tue, Oct 10, 2023 at 10:20:12AM -0400, Frank Li wrote:
> > 
> > > > > Ping
> > > > 
> > > > Read and follow please (and then ping us):
> > > > https://lore.kernel.org/linux-pci/20171026223701.GA25649@bhelgaas-glaptop.roam.corp.google.com
> > > 
> > > Could you please help point which specic one was not follow aboved guide?
> > > Then I can update my code. I think that's efficial communication method. I
> > > think I have read it serial times. But not sure which one violate the
> > > guide?
> > > 
> > > @Bjorn Helgaas. How do you think so? 
> > 
> > Since Lorenzo didn't point out anything specific in the patch itself,
> > I think he was probably referring to the subject line and this advice:
> > 
> >   - Follow the existing convention, i.e., run "git log --oneline
> >     <file>" and make yours match in format, capitalization, and
> >     sentence structure.  For example, native host bridge driver patch
> >     titles look like this:
> > 
> >       PCI: altera: Fix platform_get_irq() error handling
> >       PCI: vmd: Remove IRQ affinity so we can allocate more IRQs
> >       PCI: mediatek: Add MSI support for MT2712 and MT7622
> >       PCI: rockchip: Remove IRQ domain if probe fails
> > 
> > In this case, your subject line was:
> > 
> >   PCI: layerscape: add suspend/resume for ls1021a
> > 
> > The advice was to run this:
> > 
> >   $ git log --oneline drivers/pci/controller/dwc/pci-layerscape.c
> >   83c088148c8e PCI: Use PCI_HEADER_TYPE_* instead of literals
> >   9fda4d09905d PCI: layerscape: Add power management support for ls1028a
> >   277004d7a4a3 PCI: Remove unnecessary <linux/of_irq.h> includes
> >   60b3c27fb9b9 PCI: dwc: Rename struct pcie_port to dw_pcie_rp
> >   d23f0c11aca2 PCI: layerscape: Change to use the DWC common link-up check function
> >   7007b745a508 PCI: layerscape: Convert to builtin_platform_driver()
> >   60f5b73fa0f2 PCI: dwc: Remove unnecessary wrappers around dw_pcie_host_init()
> >   b9ac0f9dc8ea PCI: dwc: Move dw_pcie_setup_rc() to DWC common code
> >   f78f02638af5 PCI: dwc: Rework MSI initialization
> > 
> > Note that these summaries are all complete sentences that start with a
> > capital letter:
> > 
> >   Use PCI_HEADER_TYPE_* instead of literals
> >   Add power management support for ls1028a
> >   Remove unnecessary <linux/of_irq.h> includes
> >   ...
> > 
> > So yours could be this:
> > 
> >   PCI: layerscape: Add suspend/resume for ls1021a
> >                    ^
> > 
> > This is trivial, obviously.  But the uppercase/lowercase distinction
> > carries information, and it's an unnecessary distraction to notice
> > that "oh, this is different from the rest; is the difference
> > important or should I ignore it?"
> 
> Thanks. Not everyone think it is trivial. Especially for the one, who
> English are not native language.
> 
> > 
> > Obviously Lorenzo *could* edit all your subject lines on your behalf,
> > but it makes everybody's life easier if people look at the existing
> > code and follow the style when making changes.
> 
> Understand, but simple mark 'a' and 'A' to me. I will update patches and
> take care for next time instead search whole docuemnt to guess which one
> violated. I know I make some mistakes at here. But I am working on many
> difference kernel subsystems, some require upper case, some require low
> case, someone doesn't care.
>  
> I respected all reviewer's and maintaner's time, but I hope respected
> the contributor's time also.

I do respect your time, please respect ours by following the guidelines I
provided you with multiple times already.

Thank you for resending the series.

Lorenzo
  

On Fri, Sep 15, 2023 at 02:43:05PM -0400, Frank Li wrote:
> ls1021a add suspend/resume support.
> 

Please add what the driver is doing during suspend/resume.

> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/pci/controller/dwc/pci-layerscape.c | 88 ++++++++++++++++++++-
>  1 file changed, 87 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-layerscape.c b/drivers/pci/controller/dwc/pci-layerscape.c
> index 20c48c06e2248..bc5a8ff1a26ce 100644
> --- a/drivers/pci/controller/dwc/pci-layerscape.c
> +++ b/drivers/pci/controller/dwc/pci-layerscape.c
> @@ -35,6 +35,12 @@
>  #define PF_MCR_PTOMR		BIT(0)
>  #define PF_MCR_EXL2S		BIT(1)
>  
> +/* LS1021A PEXn PM Write Control Register */
> +#define SCFG_PEXPMWRCR(idx)	(0x5c + (idx) * 0x64)
> +#define PMXMTTURNOFF		BIT(31)
> +#define SCFG_PEXSFTRSTCR	0x190
> +#define PEXSR(idx)		BIT(idx)
> +
>  #define PCIE_IATU_NUM		6
>  
>  struct ls_pcie_drvdata {
> @@ -48,6 +54,8 @@ struct ls_pcie {
>  	struct dw_pcie *pci;
>  	const struct ls_pcie_drvdata *drvdata;
>  	void __iomem *pf_base;
> +	struct regmap *scfg;
> +	int index;
>  	bool big_endian;
>  };
>  
> @@ -170,13 +178,91 @@ static int ls_pcie_host_init(struct dw_pcie_rp *pp)
>  	return 0;
>  }
>  
> +static void ls1021a_pcie_send_turnoff_msg(struct dw_pcie_rp *pp)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct ls_pcie *pcie = to_ls_pcie(pci);
> +	u32 val;
> +
> +	if (!pcie->scfg) {

Can this ever happen?

> +		dev_dbg(pcie->pci->dev, "SYSCFG is NULL\n");
> +		return;
> +	}
> +
> +	/* Send Turn_off message */

"Send PME_Turn_Off message"

> +	regmap_read(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), &val);
> +	val |= PMXMTTURNOFF;
> +	regmap_write(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), val);
> +
> +	/* There are not register to check ACK, so wait PCIE_PME_TO_L2_TIMEOUT_US */

"There is no specific register to check for PME_To_Ack from endpoint. So on the
safe side, wait for PCIE_PME_TO_L2_TIMEOUT_US."

> +	mdelay(PCIE_PME_TO_L2_TIMEOUT_US/1000);
> +
> +	/* Clear Turn_off message */

"PME_Turn_off". But I'm not sure if this is really required. Are you doing it
because the layerspace hw implements the PME_Turn_Off bit as "level triggered"?

> +	regmap_read(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), &val);
> +	val &= ~PMXMTTURNOFF;
> +	regmap_write(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), val);
> +}
> +
> +static void ls1021a_pcie_exit_from_l2(struct dw_pcie_rp *pp)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct ls_pcie *pcie = to_ls_pcie(pci);
> +	u32 val;
> +

A comment here would be good.

> +	regmap_read(pcie->scfg, SCFG_PEXSFTRSTCR, &val);
> +	val |= PEXSR(pcie->index);
> +	regmap_write(pcie->scfg, SCFG_PEXSFTRSTCR, val);
> +
> +	regmap_read(pcie->scfg, SCFG_PEXSFTRSTCR, &val);
> +	val &= ~PEXSR(pcie->index);
> +	regmap_write(pcie->scfg, SCFG_PEXSFTRSTCR, val);
> +}
> +
> +static int ls1021a_pcie_host_init(struct dw_pcie_rp *pp)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct ls_pcie *pcie = to_ls_pcie(pci);
> +	struct device *dev = pcie->pci->dev;
> +	u32 index[2];
> +	int ret;
> +
> +	ret = ls_pcie_host_init(pp);
> +	if (ret)
> +		return ret;
> +
> +	pcie->scfg = syscon_regmap_lookup_by_phandle(dev->of_node, "fsl,pcie-scfg");
> +	if (IS_ERR(pcie->scfg)) {
> +		ret = PTR_ERR(pcie->scfg);
> +		dev_err(dev, "No syscfg phandle specified\n");
> +		pcie->scfg = NULL;
> +		return ret;
> +	}
> +
> +	ret = of_property_read_u32_array(dev->of_node, "fsl,pcie-scfg", index, 2);
> +	if (ret) {
> +		pcie->scfg = NULL;
> +		return ret;
> +	}
> +
> +	pcie->index = index[1];
> +

The above syscon parsing could be done conditionally during probe itself. There
is no need to do it during host_init() time.

- Mani

> +	return ret;
> +}
> +
>  static const struct dw_pcie_host_ops ls_pcie_host_ops = {
>  	.host_init = ls_pcie_host_init,
>  	.pme_turn_off = ls_pcie_send_turnoff_msg,
>  };
>  
> +static const struct dw_pcie_host_ops ls1021a_pcie_host_ops = {
> +	.host_init = ls1021a_pcie_host_init,
> +	.pme_turn_off = ls1021a_pcie_send_turnoff_msg,
> +};
> +
>  static const struct ls_pcie_drvdata ls1021a_drvdata = {
> -	.pm_support = false,
> +	.pm_support = true,
> +	.ops = &ls1021a_pcie_host_ops,
> +	.exit_from_l2 = ls1021a_pcie_exit_from_l2,
>  };
>  
>  static const struct ls_pcie_drvdata layerscape_drvdata = {
> -- 
> 2.34.1
>
  

On Mon, Oct 16, 2023 at 11:25:12AM -0500, Bjorn Helgaas wrote:
> On Mon, Oct 16, 2023 at 12:11:04PM -0400, Frank Li wrote:
> > On Mon, Oct 16, 2023 at 10:22:11AM -0500, Bjorn Helgaas wrote:
> 
> > > Obviously Lorenzo *could* edit all your subject lines on your behalf,
> > > but it makes everybody's life easier if people look at the existing
> > > code and follow the style when making changes.
> > 
> > Understand, but simple mark 'a' and 'A' to me. I will update patches and
> > take care for next time instead search whole docuemnt to guess which one
> > violated. I know I make some mistakes at here. But I am working on many
> > difference kernel subsystems, some require upper case, some require low
> > case, someone doesn't care.
> 
> Right, that's why I always suggest following the example of the
> surrounding code and history.  English is the only language I know,
> but I speculate that this typographical detail probably doesn't make
> sense in languages that don't have a similar upper/lowercase
> distinction.

If everyone thinks it is important. I suggest put it in checkpatch.pl
script. The only script check can prevent to human make mistakes.

I asked the same question at:
https://lore.kernel.org/imx/ZSV1sINV%2F2GrAYFr@lizhi-Precision-Tower-5810/T/#t

It lets teach kid mulitplication,  kid did 20 questions. only 1 failure.
The good teacher should tell which one is wrong and grade as 19/20 instead
of just grade 19/20 without any comments.

We are using email communication instead of face to face. The efficient of
communication is important. We have differece background, difference
native languadge, live on difference area in world and do the same jobs to
make linux kernel better.

The simple and straight forward's feedback can save both our time and
efforts.

Frank Li

> 
> Thanks for persevering; we'd be having a lot more trouble if I tried
> to send emails in your native language ;)
> 
> Bjorn
  

On Mon, Oct 16, 2023 at 10:28:24PM +0530, Manivannan Sadhasivam wrote:
> On Fri, Sep 15, 2023 at 02:43:05PM -0400, Frank Li wrote:
> > ls1021a add suspend/resume support.
> > 
> 
> Please add what the driver is doing during suspend/resume.
> 
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  drivers/pci/controller/dwc/pci-layerscape.c | 88 ++++++++++++++++++++-
> >  1 file changed, 87 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pci-layerscape.c b/drivers/pci/controller/dwc/pci-layerscape.c
> > index 20c48c06e2248..bc5a8ff1a26ce 100644
> > --- a/drivers/pci/controller/dwc/pci-layerscape.c
> > +++ b/drivers/pci/controller/dwc/pci-layerscape.c
> > @@ -35,6 +35,12 @@
> >  #define PF_MCR_PTOMR		BIT(0)
> >  #define PF_MCR_EXL2S		BIT(1)
> >  
> > +/* LS1021A PEXn PM Write Control Register */
> > +#define SCFG_PEXPMWRCR(idx)	(0x5c + (idx) * 0x64)
> > +#define PMXMTTURNOFF		BIT(31)
> > +#define SCFG_PEXSFTRSTCR	0x190
> > +#define PEXSR(idx)		BIT(idx)
> > +
> >  #define PCIE_IATU_NUM		6
> >  
> >  struct ls_pcie_drvdata {
> > @@ -48,6 +54,8 @@ struct ls_pcie {
> >  	struct dw_pcie *pci;
> >  	const struct ls_pcie_drvdata *drvdata;
> >  	void __iomem *pf_base;
> > +	struct regmap *scfg;
> > +	int index;
> >  	bool big_endian;
> >  };
> >  
> > @@ -170,13 +178,91 @@ static int ls_pcie_host_init(struct dw_pcie_rp *pp)
> >  	return 0;
> >  }
> >  
> > +static void ls1021a_pcie_send_turnoff_msg(struct dw_pcie_rp *pp)
> > +{
> > +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +	struct ls_pcie *pcie = to_ls_pcie(pci);
> > +	u32 val;
> > +
> > +	if (!pcie->scfg) {
> 
> Can this ever happen?
> 
> > +		dev_dbg(pcie->pci->dev, "SYSCFG is NULL\n");
> > +		return;
> > +	}
> > +
> > +	/* Send Turn_off message */
> 
> "Send PME_Turn_Off message"
> 
> > +	regmap_read(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), &val);
> > +	val |= PMXMTTURNOFF;
> > +	regmap_write(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), val);
> > +
> > +	/* There are not register to check ACK, so wait PCIE_PME_TO_L2_TIMEOUT_US */
> 
> "There is no specific register to check for PME_To_Ack from endpoint. So on the
> safe side, wait for PCIE_PME_TO_L2_TIMEOUT_US."
> 
> > +	mdelay(PCIE_PME_TO_L2_TIMEOUT_US/1000);
> > +
> > +	/* Clear Turn_off message */
> 
> "PME_Turn_off". But I'm not sure if this is really required. Are you doing it
> because the layerspace hw implements the PME_Turn_Off bit as "level triggered"?

I am not sure how hardware implement this. But reference manual said:
 
PMXMTTURNOFF:
Generate PM turnoff message for power management of PCI Express controllers.
This bit should be cleared by software.
0 Clear PM turnoff (default)
1 Trigger PM turnoff

Frank

> 
> > +	regmap_read(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), &val);
> > +	val &= ~PMXMTTURNOFF;
> > +	regmap_write(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), val);
> > +}
> > +
> > +static void ls1021a_pcie_exit_from_l2(struct dw_pcie_rp *pp)
> > +{
> > +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +	struct ls_pcie *pcie = to_ls_pcie(pci);
> > +	u32 val;
> > +
> 
> A comment here would be good.
> 
> > +	regmap_read(pcie->scfg, SCFG_PEXSFTRSTCR, &val);
> > +	val |= PEXSR(pcie->index);
> > +	regmap_write(pcie->scfg, SCFG_PEXSFTRSTCR, val);
> > +
> > +	regmap_read(pcie->scfg, SCFG_PEXSFTRSTCR, &val);
> > +	val &= ~PEXSR(pcie->index);
> > +	regmap_write(pcie->scfg, SCFG_PEXSFTRSTCR, val);
> > +}
> > +
> > +static int ls1021a_pcie_host_init(struct dw_pcie_rp *pp)
> > +{
> > +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +	struct ls_pcie *pcie = to_ls_pcie(pci);
> > +	struct device *dev = pcie->pci->dev;
> > +	u32 index[2];
> > +	int ret;
> > +
> > +	ret = ls_pcie_host_init(pp);
> > +	if (ret)
> > +		return ret;
> > +
> > +	pcie->scfg = syscon_regmap_lookup_by_phandle(dev->of_node, "fsl,pcie-scfg");
> > +	if (IS_ERR(pcie->scfg)) {
> > +		ret = PTR_ERR(pcie->scfg);
> > +		dev_err(dev, "No syscfg phandle specified\n");
> > +		pcie->scfg = NULL;
> > +		return ret;
> > +	}
> > +
> > +	ret = of_property_read_u32_array(dev->of_node, "fsl,pcie-scfg", index, 2);
> > +	if (ret) {
> > +		pcie->scfg = NULL;
> > +		return ret;
> > +	}
> > +
> > +	pcie->index = index[1];
> > +
> 
> The above syscon parsing could be done conditionally during probe itself. There
> is no need to do it during host_init() time.
> 
> - Mani
> 
> > +	return ret;
> > +}
> > +
> >  static const struct dw_pcie_host_ops ls_pcie_host_ops = {
> >  	.host_init = ls_pcie_host_init,
> >  	.pme_turn_off = ls_pcie_send_turnoff_msg,
> >  };
> >  
> > +static const struct dw_pcie_host_ops ls1021a_pcie_host_ops = {
> > +	.host_init = ls1021a_pcie_host_init,
> > +	.pme_turn_off = ls1021a_pcie_send_turnoff_msg,
> > +};
> > +
> >  static const struct ls_pcie_drvdata ls1021a_drvdata = {
> > -	.pm_support = false,
> > +	.pm_support = true,
> > +	.ops = &ls1021a_pcie_host_ops,
> > +	.exit_from_l2 = ls1021a_pcie_exit_from_l2,
> >  };
> >  
> >  static const struct ls_pcie_drvdata layerscape_drvdata = {
> > -- 
> > 2.34.1
> > 
> 
> -- 
> மணிவண்ணன் சதாசிவம்
  

On Mon, Oct 16, 2023 at 04:18:36PM -0400, Frank Li wrote:
> On Mon, Oct 16, 2023 at 10:28:24PM +0530, Manivannan Sadhasivam wrote:
> > On Fri, Sep 15, 2023 at 02:43:05PM -0400, Frank Li wrote:
> > > ls1021a add suspend/resume support.
> > > 
> > 
> > Please add what the driver is doing during suspend/resume.
> > 
> > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > ---
> > >  drivers/pci/controller/dwc/pci-layerscape.c | 88 ++++++++++++++++++++-
> > >  1 file changed, 87 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pci-layerscape.c b/drivers/pci/controller/dwc/pci-layerscape.c
> > > index 20c48c06e2248..bc5a8ff1a26ce 100644
> > > --- a/drivers/pci/controller/dwc/pci-layerscape.c
> > > +++ b/drivers/pci/controller/dwc/pci-layerscape.c
> > > @@ -35,6 +35,12 @@
> > >  #define PF_MCR_PTOMR		BIT(0)
> > >  #define PF_MCR_EXL2S		BIT(1)
> > >  
> > > +/* LS1021A PEXn PM Write Control Register */
> > > +#define SCFG_PEXPMWRCR(idx)	(0x5c + (idx) * 0x64)
> > > +#define PMXMTTURNOFF		BIT(31)
> > > +#define SCFG_PEXSFTRSTCR	0x190
> > > +#define PEXSR(idx)		BIT(idx)
> > > +
> > >  #define PCIE_IATU_NUM		6
> > >  
> > >  struct ls_pcie_drvdata {
> > > @@ -48,6 +54,8 @@ struct ls_pcie {
> > >  	struct dw_pcie *pci;
> > >  	const struct ls_pcie_drvdata *drvdata;
> > >  	void __iomem *pf_base;
> > > +	struct regmap *scfg;
> > > +	int index;
> > >  	bool big_endian;
> > >  };
> > >  
> > > @@ -170,13 +178,91 @@ static int ls_pcie_host_init(struct dw_pcie_rp *pp)
> > >  	return 0;
> > >  }
> > >  
> > > +static void ls1021a_pcie_send_turnoff_msg(struct dw_pcie_rp *pp)
> > > +{
> > > +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > +	struct ls_pcie *pcie = to_ls_pcie(pci);
> > > +	u32 val;
> > > +
> > > +	if (!pcie->scfg) {
> > 
> > Can this ever happen?
> > 
> > > +		dev_dbg(pcie->pci->dev, "SYSCFG is NULL\n");
> > > +		return;
> > > +	}
> > > +
> > > +	/* Send Turn_off message */
> > 
> > "Send PME_Turn_Off message"
> > 
> > > +	regmap_read(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), &val);
> > > +	val |= PMXMTTURNOFF;
> > > +	regmap_write(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), val);
> > > +
> > > +	/* There are not register to check ACK, so wait PCIE_PME_TO_L2_TIMEOUT_US */
> > 
> > "There is no specific register to check for PME_To_Ack from endpoint. So on the
> > safe side, wait for PCIE_PME_TO_L2_TIMEOUT_US."
> > 
> > > +	mdelay(PCIE_PME_TO_L2_TIMEOUT_US/1000);
> > > +
> > > +	/* Clear Turn_off message */
> > 
> > "PME_Turn_off". But I'm not sure if this is really required. Are you doing it
> > because the layerspace hw implements the PME_Turn_Off bit as "level triggered"?
> 
> I am not sure how hardware implement this. But reference manual said:
>  
> PMXMTTURNOFF:
> Generate PM turnoff message for power management of PCI Express controllers.
> This bit should be cleared by software.
> 0 Clear PM turnoff (default)
> 1 Trigger PM turnoff
> 

Hmm, okay. Atleast add the below comment to make it understandable in the
future:

"Layerscape hardware reference manual recommends clearing the PMXMTTURNOFF bit
to complete the PME_Turn_Off handshake."

- Mani

> Frank
> 
> > 
> > > +	regmap_read(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), &val);
> > > +	val &= ~PMXMTTURNOFF;
> > > +	regmap_write(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), val);
> > > +}
> > > +
> > > +static void ls1021a_pcie_exit_from_l2(struct dw_pcie_rp *pp)
> > > +{
> > > +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > +	struct ls_pcie *pcie = to_ls_pcie(pci);
> > > +	u32 val;
> > > +
> > 
> > A comment here would be good.
> > 
> > > +	regmap_read(pcie->scfg, SCFG_PEXSFTRSTCR, &val);
> > > +	val |= PEXSR(pcie->index);
> > > +	regmap_write(pcie->scfg, SCFG_PEXSFTRSTCR, val);
> > > +
> > > +	regmap_read(pcie->scfg, SCFG_PEXSFTRSTCR, &val);
> > > +	val &= ~PEXSR(pcie->index);
> > > +	regmap_write(pcie->scfg, SCFG_PEXSFTRSTCR, val);
> > > +}
> > > +
> > > +static int ls1021a_pcie_host_init(struct dw_pcie_rp *pp)
> > > +{
> > > +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > +	struct ls_pcie *pcie = to_ls_pcie(pci);
> > > +	struct device *dev = pcie->pci->dev;
> > > +	u32 index[2];
> > > +	int ret;
> > > +
> > > +	ret = ls_pcie_host_init(pp);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	pcie->scfg = syscon_regmap_lookup_by_phandle(dev->of_node, "fsl,pcie-scfg");
> > > +	if (IS_ERR(pcie->scfg)) {
> > > +		ret = PTR_ERR(pcie->scfg);
> > > +		dev_err(dev, "No syscfg phandle specified\n");
> > > +		pcie->scfg = NULL;
> > > +		return ret;
> > > +	}
> > > +
> > > +	ret = of_property_read_u32_array(dev->of_node, "fsl,pcie-scfg", index, 2);
> > > +	if (ret) {
> > > +		pcie->scfg = NULL;
> > > +		return ret;
> > > +	}
> > > +
> > > +	pcie->index = index[1];
> > > +
> > 
> > The above syscon parsing could be done conditionally during probe itself. There
> > is no need to do it during host_init() time.
> > 
> > - Mani
> > 
> > > +	return ret;
> > > +}
> > > +
> > >  static const struct dw_pcie_host_ops ls_pcie_host_ops = {
> > >  	.host_init = ls_pcie_host_init,
> > >  	.pme_turn_off = ls_pcie_send_turnoff_msg,
> > >  };
> > >  
> > > +static const struct dw_pcie_host_ops ls1021a_pcie_host_ops = {
> > > +	.host_init = ls1021a_pcie_host_init,
> > > +	.pme_turn_off = ls1021a_pcie_send_turnoff_msg,
> > > +};
> > > +
> > >  static const struct ls_pcie_drvdata ls1021a_drvdata = {
> > > -	.pm_support = false,
> > > +	.pm_support = true,
> > > +	.ops = &ls1021a_pcie_host_ops,
> > > +	.exit_from_l2 = ls1021a_pcie_exit_from_l2,
> > >  };
> > >  
> > >  static const struct ls_pcie_drvdata layerscape_drvdata = {
> > > -- 
> > > 2.34.1
> > > 
> > 
> > -- 
> > மணிவண்ணன் சதாசிவம்