Message ID | 20220114010627.21104-1-ricardo.martinez@linux.intel.com |
---|---|
Headers | show |
Series | net: wwan: t7xx: PCIe driver for MediaTek M.2 modem | expand |
From: Haijun Liu <haijun.liu@mediatek.com>
Implements suspend, resumes, freeze, thaw, poweroff, and restore
`dev_pm_ops` callbacks.
On Thu, Jan 13, 2022 at 06:06:15PM -0700, Ricardo Martinez wrote: > Add macros to get the next or previous entries and wraparound if > needed. For example, calling list_next_entry_circular() on the last > element should return the first element in the list. FWIW, Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Signed-off-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> > --- > include/linux/list.h | 26 ++++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/include/linux/list.h b/include/linux/list.h > index dd6c2041d09c..c147eeb2d39d 100644 > --- a/include/linux/list.h > +++ b/include/linux/list.h > @@ -563,6 +563,19 @@ static inline void list_splice_tail_init(struct list_head *list, > #define list_next_entry(pos, member) \ > list_entry((pos)->member.next, typeof(*(pos)), member) > > +/** > + * list_next_entry_circular - get the next element in list > + * @pos: the type * to cursor. > + * @head: the list head to take the element from. > + * @member: the name of the list_head within the struct. > + * > + * Wraparound if pos is the last element (return the first element). > + * Note, that list is expected to be not empty. > + */ > +#define list_next_entry_circular(pos, head, member) \ > + (list_is_last(&(pos)->member, head) ? \ > + list_first_entry(head, typeof(*(pos)), member) : list_next_entry(pos, member)) > + > /** > * list_prev_entry - get the prev element in list > * @pos: the type * to cursor > @@ -571,6 +584,19 @@ static inline void list_splice_tail_init(struct list_head *list, > #define list_prev_entry(pos, member) \ > list_entry((pos)->member.prev, typeof(*(pos)), member) > > +/** > + * list_prev_entry_circular - get the prev element in list > + * @pos: the type * to cursor. > + * @head: the list head to take the element from. > + * @member: the name of the list_head within the struct. > + * > + * Wraparound if pos is the first element (return the last element). > + * Note, that list is expected to be not empty. > + */ > +#define list_prev_entry_circular(pos, head, member) \ > + (list_is_first(&(pos)->member, head) ? \ > + list_last_entry(head, typeof(*(pos)), member) : list_prev_entry(pos, member)) > + > /** > * list_for_each - iterate over a list > * @pos: the &struct list_head to use as a loop cursor. > -- > 2.17.1 >
On Fri, 14 Jan 2022 at 02:06, Ricardo Martinez <ricardo.martinez@linux.intel.com> wrote: > > t7xx is the PCIe host device driver for Intel 5G 5000 M.2 solution which > is based on MediaTek's T700 modem to provide WWAN connectivity. > The driver uses the WWAN framework infrastructure to create the following > control ports and network interfaces: > * /dev/wwan0mbim0 - Interface conforming to the MBIM protocol. > Applications like libmbim [1] or Modem Manager [2] from v1.16 onwards > with [3][4] can use it to enable data communication towards WWAN. > * /dev/wwan0at0 - Interface that supports AT commands. > * wwan0 - Primary network interface for IP traffic. > > The main blocks in t7xx driver are: > * PCIe layer - Implements probe, removal, and power management callbacks. > * Port-proxy - Provides a common interface to interact with different types > of ports such as WWAN ports. > * Modem control & status monitor - Implements the entry point for modem > initialization, reset and exit, as well as exception handling. > * CLDMA (Control Layer DMA) - Manages the HW used by the port layer to send > control messages to the modem using MediaTek's CCCI (Cross-Core > Communication Interface) protocol. > * DPMAIF (Data Plane Modem AP Interface) - Controls the HW that provides > uplink and downlink queues for the data path. The data exchange takes > place using circular buffers to share data buffer addresses and metadata > to describe the packets. > * MHCCIF (Modem Host Cross-Core Interface) - Provides interrupt channels > for bidirectional event notification such as handshake, exception, PM and > port enumeration. > > The compilation of the t7xx driver is enabled by the CONFIG_MTK_T7XX config > option which depends on CONFIG_WWAN. > This driver was originally developed by MediaTek. Intel adapted t7xx to > the WWAN framework, optimized and refactored the driver source in close > collaboration with MediaTek. This will enable getting the t7xx driver on > Approved Vendor List for interested OEM's and ODM's productization plans > with Intel 5G 5000 M.2 solution.
On Thu, 13 Jan 2022, Ricardo Martinez wrote: > From: Haijun Liu <haijun.liu@mediatek.com> > > Implements suspend, resumes, freeze, thaw, poweroff, and restore > `dev_pm_ops` callbacks. > > >From the host point of view, the t7xx driver is one entity. But, the > device has several modules that need to be addressed in different ways > during power management (PM) flows. > The driver uses the term 'PM entities' to refer to the 2 DPMA and > 2 CLDMA HW blocks that need to be managed during PM flows. > When a dev_pm_ops function is called, the PM entities list is iterated > and the matching function is called for each entry in the list. > > Signed-off-by: Haijun Liu <haijun.liu@mediatek.com> > Signed-off-by: Chandrashekar Devegowda <chandrashekar.devegowda@intel.com> > Co-developed-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> > Signed-off-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> > --- > if (ret) { > dev_err(dev, "Failed to allocate RX/TX SW resources: %d\n", ret); > + t7xx_dpmaif_pm_entity_release(dpmaif_ctrl); > return NULL; Print after release. > +static int __t7xx_pci_pm_suspend(struct pci_dev *pdev) > +{ > + struct t7xx_pci_dev *t7xx_dev; > + struct md_pm_entity *entity; > + unsigned long wait_ret; > + enum t7xx_pm_id id; > + int ret = 0; > + > + t7xx_dev = pci_get_drvdata(pdev); > + if (atomic_read(&t7xx_dev->md_pm_state) <= MTK_PM_INIT) { > + dev_err(&pdev->dev, > + "[PM] Exiting suspend, because handshake failure or in an exception\n"); > + return -EFAULT; > + } > + > + iowrite32(L1_DISABLE_BIT(0), IREG_BASE(t7xx_dev) + DIS_ASPM_LOWPWR_SET_0); > + > + ret = t7xx_wait_pm_config(t7xx_dev); > + if (ret) > + return ret; Do you need to rollback the iowrite? > + atomic_set(&t7xx_dev->md_pm_state, MTK_PM_SUSPENDED); > + t7xx_pcie_mac_clear_int(t7xx_dev, SAP_RGU_INT); > + t7xx_dev->rgu_pci_irq_en = false; > + > + list_for_each_entry(entity, &t7xx_dev->md_pm_entities, entity) { > + if (entity->suspend) { > + ret = entity->suspend(t7xx_dev, entity->entity_param); > + if (ret) { > + id = entity->id; > + break; > + } > + } > + } > + > + if (ret) { > + dev_err(&pdev->dev, "[PM] Suspend error: %d, id: %d\n", ret, id); > + > + list_for_each_entry(entity, &t7xx_dev->md_pm_entities, entity) { > + if (id == entity->id) > + break; > + > + if (entity->resume) > + entity->resume(t7xx_dev, entity->entity_param); > + } > + > + goto suspend_complete; Suspend failure path(?) gotos to "suspend_complete"? > + } > + > + reinit_completion(&t7xx_dev->pm_sr_ack); > + t7xx_mhccif_h2d_swint_trigger(t7xx_dev, H2D_CH_SUSPEND_REQ); > + wait_ret = wait_for_completion_timeout(&t7xx_dev->pm_sr_ack, > + msecs_to_jiffies(PM_ACK_TIMEOUT_MS)); > + if (!wait_ret) > + dev_err(&pdev->dev, "[PM] Wait for device suspend ACK timeout-MD\n"); > + > + reinit_completion(&t7xx_dev->pm_sr_ack); > + t7xx_mhccif_h2d_swint_trigger(t7xx_dev, H2D_CH_SUSPEND_REQ_AP); > + wait_ret = wait_for_completion_timeout(&t7xx_dev->pm_sr_ack, > + msecs_to_jiffies(PM_ACK_TIMEOUT_MS)); > + if (!wait_ret) > + dev_err(&pdev->dev, "[PM] Wait for device suspend ACK timeout-SAP\n"); > + > + list_for_each_entry(entity, &t7xx_dev->md_pm_entities, entity) { > + if (entity->suspend_late) > + entity->suspend_late(t7xx_dev, entity->entity_param); > + } > + > +suspend_complete: > + iowrite32(L1_DISABLE_BIT(0), IREG_BASE(t7xx_dev) + DIS_ASPM_LOWPWR_CLR_0); > + > + if (ret) { > + atomic_set(&t7xx_dev->md_pm_state, MTK_PM_RESUMED); > + t7xx_pcie_mac_set_int(t7xx_dev, SAP_RGU_INT); > + } > + > + return ret; > +} Please check all paths in this function. I found enough oddities to not be able to convince myself I understood it all or found all the problems. As if, e.g., an ok path return would be missing above misnamed suspend_complete label (but then there's if (ret) below it which is kind of counterargument against my reasoning). I've no comments on patches 11-13.