On Wed, Nov 29, 2023 at 04:44:10PM -0500, Frank Li wrote:
> ls1021a add suspend/resume support.
> 

"Add suspend/resume support for Layerscape LS1021a"

> In the suspend path, PME_Turn_Off message is sent to the endpoint to
> transition the link to L2/L3_Ready state. In this SoC, there is no way to
> check if the controller has received the PME_To_Ack from the endpoint or
> not. So to be on the safer side, the driver just waits for
> PCIE_PME_TO_L2_TIMEOUT_US before asserting the SoC specific PMXMTTURNOFF
> bit to complete the PME_Turn_Off handshake. This link would then enter

"Then the link would enter L2/L3 state depending on the VAUX supply."

> L2/L3 state depending on the VAUX supply.
> 
> In the resume path, the link is brought back from L2 to L0 by doing a
> software reset.
> 
> Signed-off-by: Frank Li <frank...@nxp.com>

Couple of comments below. But the patch is looking good now. Thanks for the
update.

> ---
> 
> Notes:
>     Change from v3 to v4
>     - update commit message.
>     - it is reset a glue logic part for PCI controller.
>     - use regmap_write_bits() to reduce code change.
>     
>     Change from v2 to v3
>     - update according to mani's feedback
>     change from v1 to v2
>     - change subject 'a' to 'A'
> 
>  drivers/pci/controller/dwc/pci-layerscape.c | 83 ++++++++++++++++++++-
>  1 file changed, 82 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-layerscape.c 
> b/drivers/pci/controller/dwc/pci-layerscape.c
> index aea89926bcc4f..42bca2c3b5c3e 100644
> --- a/drivers/pci/controller/dwc/pci-layerscape.c
> +++ b/drivers/pci/controller/dwc/pci-layerscape.c
> @@ -35,11 +35,19 @@
>  #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 {
>       const u32 pf_off;
> +     const struct dw_pcie_host_ops *ops;
>       int (*exit_from_l2)(struct dw_pcie_rp *pp);
> +     bool scfg_support;
>       bool pm_support;
>  };
>  
> @@ -47,6 +55,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;
>  };
>  
> @@ -171,13 +181,65 @@ static int ls_pcie_host_init(struct dw_pcie_rp *pp)
>       return 0;
>  }
>  
> +static void scfg_pcie_send_turnoff_msg(struct regmap *scfg, u32 reg, u32 
> mask)
> +{
> +     /* Send PME_Turn_Off message */
> +     regmap_write_bits(scfg, reg, mask, mask);
> +
> +     /*
> +      * 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_write_bits(scfg, reg, mask, 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);
> +
> +     scfg_pcie_send_turnoff_msg(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), 
> PMXMTTURNOFF);
> +}
> +
> +static int scfg_pcie_exit_from_l2(struct regmap *scfg, u32 reg, u32 mask)
> +{
> +     /* Only way exit from l2 is that do software reset */

"Only way exit from L2 is by doing a software reset of the controller"

I really hope that the reset is not a global controller reset i.e., not
resetting all CSRs.

> +     regmap_write_bits(scfg, reg, mask, mask);
> +

No need of a newline.

> +     regmap_write_bits(scfg, reg, mask, 0);
> +
> +     return 0;
> +}
> +
> +static int 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);
> +
> +     return scfg_pcie_exit_from_l2(pcie->scfg, SCFG_PEXSFTRSTCR, 
> PEXSR(pcie->index));
> +}
> +
>  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 = ls_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,
> +     .scfg_support = true,
> +     .ops = &ls1021a_pcie_host_ops,
> +     .exit_from_l2 = ls1021a_pcie_exit_from_l2,
>  };
>  
>  static const struct ls_pcie_drvdata layerscape_drvdata = {
> @@ -205,6 +267,8 @@ static int ls_pcie_probe(struct platform_device *pdev)
>       struct dw_pcie *pci;
>       struct ls_pcie *pcie;
>       struct resource *dbi_base;
> +     u32 index[2];
> +     int ret;
>  
>       pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
>       if (!pcie)
> @@ -220,6 +284,7 @@ static int ls_pcie_probe(struct platform_device *pdev)
>       pci->pp.ops = &ls_pcie_host_ops;
>  
>       pcie->pci = pci;
> +     pci->pp.ops = pcie->drvdata->ops ? pcie->drvdata->ops : 
> &ls_pcie_host_ops;

pp.ops is already assigned to &ls_pcie_host_ops above. But I think for
consistency, you should pass ls_pcie_host_ops to the "ops" member of
layerscape_drvdata and just use:

        pci->pp.ops = pcie->drvdata->ops;

It looks a lot cleaner.

>  
>       dbi_base = platform_get_resource_byname(pdev, IORESOURCE_MEM, "regs");
>       pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_base);
> @@ -230,6 +295,22 @@ static int ls_pcie_probe(struct platform_device *pdev)
>  
>       pcie->pf_base = pci->dbi_base + pcie->drvdata->pf_off;
>  
> +     if (pcie->drvdata->scfg_support) {
> +             pcie->scfg = syscon_regmap_lookup_by_phandle(dev->of_node, 
> "fsl,pcie-scfg");
> +             if (IS_ERR(pcie->scfg)) {
> +                     dev_err(dev, "No syscfg phandle specified\n");
> +                     return PTR_ERR(pcie->scfg);
> +             }
> +
> +             ret = of_property_read_u32_array(dev->of_node, "fsl,pcie-scfg", 
> index, 2);
> +             if (ret) {
> +                     pcie->scfg = NULL;

No need to set it to NULL in the case of probe failure.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Reply via email to