Re: [PATCH 2/3] PCI: layerscape: add suspend/resume for ls1021a
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
> > > ---
> > > 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_NUM6
> > >
> > > 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 |= 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 &= ~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 |= PEXSR(pcie->index);
> > > + regmap_write(pcie->scfg, SCFG_PEXSFTRSTCR, val);
> > > +
> > > + regmap_read(pcie->scfg, SCFG_PEXSFTRSTCR, );
> > > + 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
Re: [PATCH 2/3] PCI: layerscape: add suspend/resume for ls1021a
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
> > ---
> > 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 |= 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 &= ~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 |= PEXSR(pcie->index);
> > + regmap_write(pcie->scfg, SCFG_PEXSFTRSTCR, val);
> > +
> > + regmap_read(pcie->scfg, SCFG_PEXSFTRSTCR, );
> > + 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
Re: [PATCH 2/3] PCI: layerscape: add suspend/resume for ls1021a
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
Re: [PATCH 2/3] PCI: layerscape: add suspend/resume for ls1021a
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
> ---
> 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_NUM6
>
> 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 |= 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 &= ~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 |= PEXSR(pcie->index);
> + regmap_write(pcie->scfg, SCFG_PEXSFTRSTCR, val);
> +
> + regmap_read(pcie->scfg, SCFG_PEXSFTRSTCR, );
> + 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 = _pcie_host_ops,
> + .exit_from_l2 = ls1021a_pcie_exit_from_l2,
> };
>
> static const struct ls_pcie_drvdata layerscape_drvdata = {
> --
> 2.34.1
>
--
மணிவண்ணன் சதாசிவம்
Re: [PATCH 2/3] PCI: layerscape: add suspend/resume for ls1021a
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.ga25...@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 > > " 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 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 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
Re: [PATCH 2/3] PCI: layerscape: add suspend/resume for ls1021a
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
Re: [PATCH 2/3] PCI: layerscape: add suspend/resume for ls1021a
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.ga25...@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 > " 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 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 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
Re: [PATCH 2/3] PCI: layerscape: add suspend/resume for ls1021a
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.ga25...@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 > " 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 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 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.ga25...@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
Re: [PATCH 2/3] PCI: layerscape: add suspend/resume for ls1021a
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.ga25...@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 " 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 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 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
Re: [PATCH 2/3] PCI: layerscape: add suspend/resume for ls1021a
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
> > > > ---
> > >
> > > ping
> > >
> > > Frank
> >
> > Ping
>
> Read and follow please (and then ping us):
> https://lore.kernel.org/linux-pci/20171026223701.ga25...@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 |= 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 &= ~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 |= PEXSR(pcie->index);
> > > > + regmap_write(pcie->scfg, SCFG_PEXSFTRSTCR, val);
> > > > +
> > > > + regmap_read(pcie->scfg, SCFG_PEXSFTRSTCR, );
> > > > + 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;
> > > >
Re: [PATCH 2/3] PCI: layerscape: add suspend/resume for ls1021a
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
> > > ---
> >
> > ping
> >
> > Frank
>
> Ping
Read and follow please (and then ping us):
https://lore.kernel.org/linux-pci/20171026223701.ga25...@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_NUM6
> > >
> > > 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 |= 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 &= ~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 |= PEXSR(pcie->index);
> > > + regmap_write(pcie->scfg, SCFG_PEXSFTRSTCR, val);
> > > +
> > > + regmap_read(pcie->scfg, SCFG_PEXSFTRSTCR, );
> > > + 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 = _pcie_host_ops,
> > > + .exit_from_l2 = ls1021a_pcie_exit_from_l2,
> > > };
> > >
> > > static const struct ls_pcie_drvdata layerscape_drvdata = {
> > > --
> > > 2.34.1
> > >
Re: [PATCH 2/3] PCI: layerscape: add suspend/resume for ls1021a
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
> > ---
>
> 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 |= 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 &= ~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 |= PEXSR(pcie->index);
> > + regmap_write(pcie->scfg, SCFG_PEXSFTRSTCR, val);
> > +
> > + regmap_read(pcie->scfg, SCFG_PEXSFTRSTCR, );
> > + 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 = _pcie_host_ops,
> > + .exit_from_l2 = ls1021a_pcie_exit_from_l2,
> > };
> >
> > static const struct ls_pcie_drvdata layerscape_drvdata = {
> > --
> > 2.34.1
> >
Re: [PATCH 2/3] PCI: layerscape: add suspend/resume for ls1021a
On Fri, Sep 15, 2023 at 02:43:05PM -0400, Frank Li wrote:
> ls1021a add suspend/resume support.
>
> Signed-off-by: Frank Li
> ---
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_NUM6
>
> 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 |= 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 &= ~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 |= PEXSR(pcie->index);
> + regmap_write(pcie->scfg, SCFG_PEXSFTRSTCR, val);
> +
> + regmap_read(pcie->scfg, SCFG_PEXSFTRSTCR, );
> + 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 = _pcie_host_ops,
> + .exit_from_l2 = ls1021a_pcie_exit_from_l2,
> };
>
> static const struct ls_pcie_drvdata layerscape_drvdata = {
> --
> 2.34.1
>
[PATCH 2/3] PCI: layerscape: add suspend/resume for ls1021a
ls1021a add suspend/resume support.
Signed-off-by: Frank Li
---
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 |= 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 &= ~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 |= PEXSR(pcie->index);
+ regmap_write(pcie->scfg, SCFG_PEXSFTRSTCR, val);
+
+ regmap_read(pcie->scfg, SCFG_PEXSFTRSTCR, );
+ 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 = _pcie_host_ops,
+ .exit_from_l2 = ls1021a_pcie_exit_from_l2,
};
static const struct ls_pcie_drvdata layerscape_drvdata = {
--
2.34.1
