Re: [PATCH v6 3/5] PCI: qcom-ep: Add wake up host op to dw_pcie_ep_ops

[Bjorn Helgaas <helgaas@kernel.org>]

On Wed, Jul 10, 2024 at 04:46:10PM +0530, Krishna chaitanya chundru wrote:
> Add wakeup host op to dw_pcie_ep_ops to wake up host.
> If the wakeup type is PME, then trigger inband PME by writing to the PARF
> PARF_PM_CTRL register, otherwise toggle #WAKE.

Wrap into single paragraph or add blank line between.

> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
>  drivers/pci/controller/dwc/pcie-qcom-ep.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> index 627a33a1c5ca..d17e8542d07a 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> @@ -97,6 +97,7 @@
>  /* PARF_PM_CTRL register fields */
>  #define PARF_PM_CTRL_REQ_EXIT_L1		BIT(1)
>  #define PARF_PM_CTRL_READY_ENTR_L23		BIT(2)
> +#define PARF_PM_CTRL_XMT_PME			BIT(4)
>  #define PARF_PM_CTRL_REQ_NOT_ENTR_L1		BIT(5)
>  
>  /* PARF_MHI_CLOCK_RESET_CTRL fields */
> @@ -817,10 +818,34 @@ static void qcom_pcie_ep_init(struct dw_pcie_ep *ep)
>  		dw_pcie_ep_reset_bar(pci, bar);
>  }
>  
> +static bool qcom_pcie_ep_wakeup_host(struct dw_pcie_ep *ep, u8 func_no, bool send_pme)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +	struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci);
> +	struct device *dev = pci->dev;
> +	u32 val;
> +
> +	if (send_pme) {
> +		dev_dbg(dev, "Waking up the host using PME\n");
> +		val = readl_relaxed(pcie_ep->parf + PARF_PM_CTRL);
> +		writel_relaxed(val | PARF_PM_CTRL_XMT_PME, pcie_ep->parf + PARF_PM_CTRL);
> +		writel_relaxed(val, pcie_ep->parf + PARF_PM_CTRL);
> +
> +	} else {
> +		dev_dbg(dev, "Waking up the host by toggling WAKE#\n");
> +		gpiod_set_value_cansleep(pcie_ep->wake, 1);
> +		usleep_range(WAKE_DELAY_US, WAKE_DELAY_US + 500);

PCIe r6.0, sec 5.3.3.2, says

  When WAKE# is used as a wakeup mechanism, once WAKE# has been
  asserted, the asserting Function must continue to drive the signal
  low until main power has been restored to the component as indicated
  by Fundamental Reset going inactive.

That doesn't seem compatible with a simple delay as you have here.

> +		gpiod_set_value_cansleep(pcie_ep->wake, 0);
> +	}
> +
> +	return true;
> +}
> +
>  static const struct dw_pcie_ep_ops pci_ep_ops = {
>  	.init = qcom_pcie_ep_init,
>  	.raise_irq = qcom_pcie_ep_raise_irq,
>  	.get_features = qcom_pcie_epc_get_features,
> +	.wakeup_host = qcom_pcie_ep_wakeup_host,
>  };
>  
>  static int qcom_pcie_ep_probe(struct platform_device *pdev)
> 
> -- 
> 2.42.0
>