On Thu, Nov 02, 2023 at 11:09:00PM +0530, Manivannan Sadhasivam wrote: > On Tue, Oct 17, 2023 at 03:31:45PM -0400, Frank Li wrote: > > ls1043a add suspend/resume support. > > Implement ls1043a_pcie_send_turnoff_msg() to send PME_Turn_Off message. > > Implement ls1043a_pcie_exit_from_l2() to exit from L2 state. > > > > Please use the suggestion I gave in patch 2/4. > > > Signed-off-by: Frank Li <Frank.Li@xxxxxxx> > > --- > > > > Notes: > > Change from v2 to v3 > > - Remove ls_pcie_lut_readl(writel) function > > > > Change from v1 to v2 > > - Update subject 'a' to 'A' > > > > drivers/pci/controller/dwc/pci-layerscape.c | 86 ++++++++++++++++++++- > > 1 file changed, 85 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/pci/controller/dwc/pci-layerscape.c b/drivers/pci/controller/dwc/pci-layerscape.c > > index 4b663b20d8612..9656224960b0c 100644 > > --- a/drivers/pci/controller/dwc/pci-layerscape.c > > +++ b/drivers/pci/controller/dwc/pci-layerscape.c > > @@ -41,6 +41,15 @@ > > #define SCFG_PEXSFTRSTCR 0x190 > > #define PEXSR(idx) BIT(idx) > > > > +/* LS1043A PEX PME control register */ > > +#define SCFG_PEXPMECR 0x144 > > +#define PEXPME(idx) BIT(31 - (idx) * 4) > > + > > +/* LS1043A PEX LUT debug register */ > > +#define LS_PCIE_LDBG 0x7fc > > +#define LDBG_SR BIT(30) > > +#define LDBG_WE BIT(31) > > + > > #define PCIE_IATU_NUM 6 > > > > #define LS_PCIE_DRV_SCFG BIT(0) > > @@ -227,6 +236,68 @@ static int ls1021a_pcie_exit_from_l2(struct dw_pcie_rp *pp) > > return 0; > > } > > > > +static void ls1043a_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; > > + } > > Why scfg is optional for this SoC and not for the other one added in patch 2/4? No, it is not optional for this SoC. This check can be removed as your previous comments about 2/4. > > > + > > + /* Send Turn_off message */ > > + regmap_read(pcie->scfg, SCFG_PEXPMECR, &val); > > + val |= PEXPME(pcie->index); > > + regmap_write(pcie->scfg, SCFG_PEXPMECR, val); > > + > > In my previous review, I asked you to use a common function and just pass the > offsets, as the sequence is same for both the SoCs. But you ignored it :/ > Sorry, I will fixed it at next version. > > + /* > > + * 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); > > + > > + /* > > + * Layerscape hardware reference manual recommends clearing the PMXMTTURNOFF bit > > + * to complete the PME_Turn_Off handshake. > > + */ > > + regmap_read(pcie->scfg, SCFG_PEXPMECR, &val); > > + val &= ~PEXPME(pcie->index); > > + regmap_write(pcie->scfg, SCFG_PEXPMECR, val); > > +} > > + > > +static int ls1043a_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; > > + > > + /* > > + * Only way let PEX module exit L2 is do a software reset. > > Same comment applies as patch 2/4. > > - Mani > > -- > மணிவண்ணன் சதாசிவம்