[PATCH v3 0/9] mhi: pci_generic: Misc improvements

[PATCH v3 0/9] mhi: pci_generic: Misc improvements

[Loic Poulain]

This series adjust some configuration values to ensure stability and
robustness of mhi pci devices (timeout, number of events, burst mode).

It also includes support for system sleep as well as a recovery procedure
that can be triggered when a PCI error is reported, either by PCI AER or by
the new health-check mechanism.

All these changes have been tested with Telit FN980m module

v2: 
  - Cancel recovery work on suspend
v3:
  - enable doorbell_mode_switch for burst channel (HW)
  - Add mhi_initialize_controller helper patch

Loic Poulain (9):
  mhi: Add mhi_controller_initialize helper
  mhi: pci-generic: Increase number of hardware events
  mhi: pci-generic: Perform hard reset on remove
  mhi: pci_generic: Enable burst mode for hardware channels
  mhi: pci_generic: Add support for reset
  mhi: pci_generic: Add suspend/resume/recovery procedure
  mhi: pci_generic: Add PCI error handlers
  mhi: pci_generic: Add health-check
  mhi: pci_generic: Increase controller timeout value

 drivers/bus/mhi/core/init.c   |   7 +
 drivers/bus/mhi/pci_generic.c | 354 ++++++++++++++++++++++++++++++++++++++++--
 include/linux/mhi.h           |   6 +
 3 files changed, 350 insertions(+), 17 deletions(-)

-- 
2.7.4

[PATCH v3 1/9] mhi: Add mhi_controller_initialize helper

[Loic Poulain]

This function allows to initialize a mhi_controller structure.
Today, it only zeroing the structure.

Use this function from mhi_alloc_controller so that any further
initialization can be factorized in initalize function.

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
---
 drivers/bus/mhi/core/init.c | 7 +++++++
 include/linux/mhi.h         | 6 ++++++
 2 files changed, 13 insertions(+)

diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
index 96cde9c..4acad28 100644
--- a/drivers/bus/mhi/core/init.c
+++ b/drivers/bus/mhi/core/init.c
@@ -1021,11 +1021,18 @@ void mhi_unregister_controller(struct mhi_controller *mhi_cntrl)
 }
 EXPORT_SYMBOL_GPL(mhi_unregister_controller);
 
+void mhi_initialize_controller(struct mhi_controller *mhi_cntrl)
+{
+	memset(mhi_cntrl, 0, sizeof(*mhi_cntrl));
+}
+EXPORT_SYMBOL_GPL(mhi_initialize_controller);
+
 struct mhi_controller *mhi_alloc_controller(void)
 {
 	struct mhi_controller *mhi_cntrl;
 
 	mhi_cntrl = kzalloc(sizeof(*mhi_cntrl), GFP_KERNEL);
+	mhi_initialize_controller(mhi_cntrl);
 
 	return mhi_cntrl;
 }
diff --git a/include/linux/mhi.h b/include/linux/mhi.h
index 5721a0a..30c676d 100644
--- a/include/linux/mhi.h
+++ b/include/linux/mhi.h
@@ -537,6 +537,12 @@ struct mhi_driver {
 #define to_mhi_device(dev) container_of(dev, struct mhi_device, dev)
 
 /**
+ * mhi_initialize_controller - Initialize MHI Controller structure
+ * @mhi_cntrl: MHI controller structure to initialize
+ */
+void mhi_initialize_controller(struct mhi_controller *mhi_cntrl);
+
+/**
  * mhi_alloc_controller - Allocate the MHI Controller structure
  * Allocate the mhi_controller structure using zero initialized memory
  */
-- 
2.7.4

[PATCH v3 2/9] mhi: pci-generic: Increase number of hardware events

[Loic Poulain]

If the IPA (IP hardware accelerator) is starved of event ring elements,
the modem is crashing (SDX55). That can be prevented by setting a
larger number of events (i.e 2 x number of channel ring elements).

Tested with FN980m module.

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
---
 drivers/bus/mhi/pci_generic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bus/mhi/pci_generic.c b/drivers/bus/mhi/pci_generic.c
index f5bee76..d3896ef 100644
--- a/drivers/bus/mhi/pci_generic.c
+++ b/drivers/bus/mhi/pci_generic.c
@@ -91,7 +91,7 @@ struct mhi_pci_dev_info {
 
 #define MHI_EVENT_CONFIG_HW_DATA(ev_ring, ch_num) \
 	{					\
-		.num_elements = 128,		\
+		.num_elements = 256,		\
 		.irq_moderation_ms = 5,		\
 		.irq = (ev_ring) + 1,		\
 		.priority = 1,			\
-- 
2.7.4

[PATCH v3 3/9] mhi: pci-generic: Perform hard reset on remove

[Loic Poulain]

Ensure that the device is hard-reset on remove to restore its initial
state and avoid further issues on subsequent probe.

This has been tested with Telit FN980m module.

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
---
 drivers/bus/mhi/pci_generic.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/bus/mhi/pci_generic.c b/drivers/bus/mhi/pci_generic.c
index d3896ef..4363676 100644
--- a/drivers/bus/mhi/pci_generic.c
+++ b/drivers/bus/mhi/pci_generic.c
@@ -15,6 +15,8 @@
 
 #define MHI_PCI_DEFAULT_BAR_NUM 0
 
+#define DEV_RESET_REG (0xB0)
+
 /**
  * struct mhi_pci_dev_info - MHI PCI device specific information
  * @config: MHI controller configuration
@@ -166,6 +168,11 @@ static void mhi_pci_status_cb(struct mhi_controller *mhi_cntrl,
 	/* Nothing to do for now */
 }
 
+static inline void mhi_pci_reset(struct mhi_controller *mhi_cntrl)
+{
+	writel(1, mhi_cntrl->regs + DEV_RESET_REG);
+}
+
 static int mhi_pci_claim(struct mhi_controller *mhi_cntrl,
 			 unsigned int bar_num, u64 dma_mask)
 {
@@ -329,6 +336,10 @@ static void mhi_pci_remove(struct pci_dev *pdev)
 	mhi_power_down(mhi_cntrl, true);
 	mhi_unprepare_after_power_down(mhi_cntrl);
 	mhi_unregister_controller(mhi_cntrl);
+
+	/* MHI-layer reset could not be enough, always hard-reset the device */
+	mhi_pci_reset(mhi_cntrl);
+
 	mhi_free_controller(mhi_cntrl);
 }
 
-- 
2.7.4

[PATCH v3 4/9] mhi: pci_generic: Enable burst mode for hardware channels

[Loic Poulain]

Hardware channels have a feature called burst mode that allows to
queue transfer ring element(s) (TRE) to a channel without ringing
the device doorbell. In that mode, the device is polling the channel
context for new elements. This reduces the frequency of host initiated
doorbells and increase throughput.

Create a new dedicated macro for hardware channels with burst enabled.

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
---
 drivers/bus/mhi/pci_generic.c | 34 ++++++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/drivers/bus/mhi/pci_generic.c b/drivers/bus/mhi/pci_generic.c
index 4363676..6b6e5bf 100644
--- a/drivers/bus/mhi/pci_generic.c
+++ b/drivers/bus/mhi/pci_generic.c
@@ -78,6 +78,36 @@ struct mhi_pci_dev_info {
 		.offload_channel = false,	\
 	}
 
+#define MHI_CHANNEL_CONFIG_HW_UL(ch_num, ch_name, el_count, ev_ring) \
+	{						\
+		.num = ch_num,				\
+		.name = ch_name,			\
+		.num_elements = el_count,		\
+		.event_ring = ev_ring,			\
+		.dir = DMA_TO_DEVICE,			\
+		.ee_mask = BIT(MHI_EE_AMSS),		\
+		.pollcfg = 0,				\
+		.doorbell = MHI_DB_BRST_ENABLE,	\
+		.lpm_notify = false,			\
+		.offload_channel = false,		\
+		.doorbell_mode_switch = true,		\
+	}						\
+
+#define MHI_CHANNEL_CONFIG_HW_DL(ch_num, ch_name, el_count, ev_ring) \
+	{						\
+		.num = ch_num,				\
+		.name = ch_name,			\
+		.num_elements = el_count,		\
+		.event_ring = ev_ring,			\
+		.dir = DMA_FROM_DEVICE,			\
+		.ee_mask = BIT(MHI_EE_AMSS),		\
+		.pollcfg = 0,				\
+		.doorbell = MHI_DB_BRST_ENABLE,	\
+		.lpm_notify = false,			\
+		.offload_channel = false,		\
+		.doorbell_mode_switch = true,		\
+	}
+
 #define MHI_EVENT_CONFIG_DATA(ev_ring)		\
 	{					\
 		.num_elements = 128,		\
@@ -112,8 +142,8 @@ static const struct mhi_channel_config modem_qcom_v1_mhi_channels[] = {
 	MHI_CHANNEL_CONFIG_DL(15, "QMI", 4, 0),
 	MHI_CHANNEL_CONFIG_UL(20, "IPCR", 8, 0),
 	MHI_CHANNEL_CONFIG_DL(21, "IPCR", 8, 0),
-	MHI_CHANNEL_CONFIG_UL(100, "IP_HW0", 128, 1),
-	MHI_CHANNEL_CONFIG_DL(101, "IP_HW0", 128, 2),
+	MHI_CHANNEL_CONFIG_HW_UL(100, "IP_HW0", 128, 1),
+	MHI_CHANNEL_CONFIG_HW_DL(101, "IP_HW0", 128, 2),
 };
 
 static const struct mhi_event_config modem_qcom_v1_mhi_events[] = {
-- 
2.7.4

[PATCH v3 5/9] mhi: pci_generic: Add support for reset

[Loic Poulain]

Add support for resetting the device, reset can be triggered in case
of error or manually via sysfs (/sys/bus/pci/devices/*/reset).

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
---
 drivers/bus/mhi/pci_generic.c | 119 +++++++++++++++++++++++++++++++++++++-----
 1 file changed, 106 insertions(+), 13 deletions(-)

diff --git a/drivers/bus/mhi/pci_generic.c b/drivers/bus/mhi/pci_generic.c
index 6b6e5bf..67fcbcf 100644
--- a/drivers/bus/mhi/pci_generic.c
+++ b/drivers/bus/mhi/pci_generic.c
@@ -8,6 +8,7 @@
  * Copyright (C) 2020 Linaro Ltd <loic.poulain@linaro.org>
  */
 
+#include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/mhi.h>
 #include <linux/module.h>
@@ -179,6 +180,16 @@ static const struct pci_device_id mhi_pci_id_table[] = {
 };
 MODULE_DEVICE_TABLE(pci, mhi_pci_id_table);
 
+enum mhi_pci_device_status {
+	MHI_PCI_DEV_STARTED,
+};
+
+struct mhi_pci_device {
+	struct mhi_controller mhi_cntrl;
+	struct pci_saved_state *pci_state;
+	unsigned long status;
+};
+
 static int mhi_pci_read_reg(struct mhi_controller *mhi_cntrl,
 			    void __iomem *addr, u32 *out)
 {
@@ -203,6 +214,20 @@ static inline void mhi_pci_reset(struct mhi_controller *mhi_cntrl)
 	writel(1, mhi_cntrl->regs + DEV_RESET_REG);
 }
 
+static bool mhi_pci_is_alive(struct mhi_controller *mhi_cntrl)
+{
+	struct pci_dev *pdev = to_pci_dev(mhi_cntrl->cntrl_dev);
+	u16 vendor = 0;
+
+	if (pci_read_config_word(pdev, PCI_VENDOR_ID, &vendor))
+		return false;
+
+	if (vendor == (u16) ~0 || vendor == 0)
+		return false;
+
+	return true;
+}
+
 static int mhi_pci_claim(struct mhi_controller *mhi_cntrl,
 			 unsigned int bar_num, u64 dma_mask)
 {
@@ -298,16 +323,20 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
 	const struct mhi_pci_dev_info *info = (struct mhi_pci_dev_info *) id->driver_data;
 	const struct mhi_controller_config *mhi_cntrl_config;
+	struct mhi_pci_device *mhi_pdev;
 	struct mhi_controller *mhi_cntrl;
 	int err;
 
 	dev_dbg(&pdev->dev, "MHI PCI device found: %s\n", info->name);
 
-	mhi_cntrl = mhi_alloc_controller();
-	if (!mhi_cntrl)
+	mhi_pdev = devm_kzalloc(&pdev->dev, sizeof(*mhi_pdev), GFP_KERNEL);
+	if (!mhi_pdev)
 		return -ENOMEM;
 
 	mhi_cntrl_config = info->config;
+	mhi_cntrl = &mhi_pdev->mhi_cntrl;
+
+	mhi_initialize_controller(mhi_cntrl);
 	mhi_cntrl->cntrl_dev = &pdev->dev;
 	mhi_cntrl->iova_start = 0;
 	mhi_cntrl->iova_stop = (dma_addr_t)DMA_BIT_MASK(info->dma_data_width);
@@ -322,17 +351,21 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	err = mhi_pci_claim(mhi_cntrl, info->bar_num, DMA_BIT_MASK(info->dma_data_width));
 	if (err)
-		goto err_release;
+		return err;
 
 	err = mhi_pci_get_irqs(mhi_cntrl, mhi_cntrl_config);
 	if (err)
-		goto err_release;
+		return err;
+
+	pci_set_drvdata(pdev, mhi_pdev);
 
-	pci_set_drvdata(pdev, mhi_cntrl);
+	/* Have stored pci confspace at hand for restore in sudden PCI error */
+	pci_save_state(pdev);
+	mhi_pdev->pci_state = pci_store_saved_state(pdev);
 
 	err = mhi_register_controller(mhi_cntrl, mhi_cntrl_config);
 	if (err)
-		goto err_release;
+		return err;
 
 	/* MHI bus does not power up the controller by default */
 	err = mhi_prepare_for_power_up(mhi_cntrl);
@@ -347,37 +380,97 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		goto err_unprepare;
 	}
 
+	set_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status);
+
 	return 0;
 
 err_unprepare:
 	mhi_unprepare_after_power_down(mhi_cntrl);
 err_unregister:
 	mhi_unregister_controller(mhi_cntrl);
-err_release:
-	mhi_free_controller(mhi_cntrl);
 
 	return err;
 }
 
 static void mhi_pci_remove(struct pci_dev *pdev)
 {
-	struct mhi_controller *mhi_cntrl = pci_get_drvdata(pdev);
+	struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev);
+	struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;
+
+	if (test_and_clear_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status)) {
+		mhi_power_down(mhi_cntrl, true);
+		mhi_unprepare_after_power_down(mhi_cntrl);
+	}
 
-	mhi_power_down(mhi_cntrl, true);
-	mhi_unprepare_after_power_down(mhi_cntrl);
 	mhi_unregister_controller(mhi_cntrl);
 
 	/* MHI-layer reset could not be enough, always hard-reset the device */
 	mhi_pci_reset(mhi_cntrl);
+}
+
+void mhi_pci_reset_prepare(struct pci_dev *pdev)
+{
+	struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev);
+	struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;
+
+	dev_info(&pdev->dev, "reset\n");
+
+	/* Clean up MHI state */
+	if (test_and_clear_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status)) {
+		mhi_power_down(mhi_cntrl, false);
+		mhi_unprepare_after_power_down(mhi_cntrl);
+	}
+
+	/* cause internal device reset */
+	mhi_pci_reset(mhi_cntrl);
+
+	/* Be sure device reset has been executed */
+	msleep(500);
+}
+
+void mhi_pci_reset_done(struct pci_dev *pdev)
+{
+	struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev);
+	struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;
+	int err;
+
+	/* Restore initial known working PCI state */
+	pci_load_saved_state(pdev, mhi_pdev->pci_state);
+	pci_restore_state(pdev);
+
+	/* Is device status available ? */
+	if (!mhi_pci_is_alive(mhi_cntrl)) {
+		dev_err(&pdev->dev, "reset failed\n");
+		return;
+	}
 
-	mhi_free_controller(mhi_cntrl);
+	err = mhi_prepare_for_power_up(mhi_cntrl);
+	if (err) {
+		dev_err(&pdev->dev, "failed to prepare MHI controller\n");
+		return;
+	}
+
+	err = mhi_sync_power_up(mhi_cntrl);
+	if (err) {
+		dev_err(&pdev->dev, "failed to power up MHI controller\n");
+		mhi_unprepare_after_power_down(mhi_cntrl);
+		return;
+	}
+
+	set_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status);
 }
 
+static const struct pci_error_handlers mhi_pci_err_handler = {
+	.reset_prepare = mhi_pci_reset_prepare,
+	.reset_done = mhi_pci_reset_done,
+};
+
 static struct pci_driver mhi_pci_driver = {
 	.name		= "mhi-pci-generic",
 	.id_table	= mhi_pci_id_table,
 	.probe		= mhi_pci_probe,
-	.remove		= mhi_pci_remove
+	.remove		= mhi_pci_remove,
+	.err_handler	= &mhi_pci_err_handler,
 };
 module_pci_driver(mhi_pci_driver);
 
-- 
2.7.4

[PATCH v3 6/9] mhi: pci_generic: Add suspend/resume/recovery procedure

[Loic Poulain]

Add support for system wide suspend/resume. During suspend, MHI
device controller must be put in M3 state and PCI bus in D3 state.

Add a recovery procedure allowing to reinitialize the device in case
of error during resume steps, which can happen if device loses power
(and so its context) while system suspend.

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
---
 drivers/bus/mhi/pci_generic.c | 102 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 102 insertions(+)

diff --git a/drivers/bus/mhi/pci_generic.c b/drivers/bus/mhi/pci_generic.c
index 67fcbcf..9919ad6 100644
--- a/drivers/bus/mhi/pci_generic.c
+++ b/drivers/bus/mhi/pci_generic.c
@@ -13,6 +13,7 @@
 #include <linux/mhi.h>
 #include <linux/module.h>
 #include <linux/pci.h>
+#include <linux/workqueue.h>
 
 #define MHI_PCI_DEFAULT_BAR_NUM 0
 
@@ -187,6 +188,7 @@ enum mhi_pci_device_status {
 struct mhi_pci_device {
 	struct mhi_controller mhi_cntrl;
 	struct pci_saved_state *pci_state;
+	struct work_struct recovery_work;
 	unsigned long status;
 };
 
@@ -319,6 +321,48 @@ static void mhi_pci_runtime_put(struct mhi_controller *mhi_cntrl)
 	/* no PM for now */
 }
 
+static void mhi_pci_recovery_work(struct work_struct *work)
+{
+	struct mhi_pci_device *mhi_pdev = container_of(work, struct mhi_pci_device,
+						       recovery_work);
+	struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;
+	struct pci_dev *pdev = to_pci_dev(mhi_cntrl->cntrl_dev);
+	int err;
+
+	dev_warn(&pdev->dev, "device recovery started\n");
+
+	/* Clean up MHI state */
+	if (test_and_clear_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status)) {
+		mhi_power_down(mhi_cntrl, false);
+		mhi_unprepare_after_power_down(mhi_cntrl);
+	}
+
+	/* Check if we can recover without full reset */
+	pci_set_power_state(pdev, PCI_D0);
+	pci_load_saved_state(pdev, mhi_pdev->pci_state);
+	pci_restore_state(pdev);
+
+	if (!mhi_pci_is_alive(mhi_cntrl))
+		goto err_try_reset;
+
+	err = mhi_prepare_for_power_up(mhi_cntrl);
+	if (err)
+		goto err_try_reset;
+
+	err = mhi_sync_power_up(mhi_cntrl);
+	if (err)
+		goto err_unprepare;
+
+	set_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status);
+	return;
+
+err_unprepare:
+	mhi_unprepare_after_power_down(mhi_cntrl);
+err_try_reset:
+	if (pci_reset_function(pdev))
+		dev_err(&pdev->dev, "Recovery failed\n");
+}
+
 static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
 	const struct mhi_pci_dev_info *info = (struct mhi_pci_dev_info *) id->driver_data;
@@ -333,6 +377,8 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (!mhi_pdev)
 		return -ENOMEM;
 
+	INIT_WORK(&mhi_pdev->recovery_work, mhi_pci_recovery_work);
+
 	mhi_cntrl_config = info->config;
 	mhi_cntrl = &mhi_pdev->mhi_cntrl;
 
@@ -397,6 +443,8 @@ static void mhi_pci_remove(struct pci_dev *pdev)
 	struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev);
 	struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;
 
+	cancel_work_sync(&mhi_pdev->recovery_work);
+
 	if (test_and_clear_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status)) {
 		mhi_power_down(mhi_cntrl, true);
 		mhi_unprepare_after_power_down(mhi_cntrl);
@@ -465,12 +513,66 @@ static const struct pci_error_handlers mhi_pci_err_handler = {
 	.reset_done = mhi_pci_reset_done,
 };
 
+int  __maybe_unused mhi_pci_suspend(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct mhi_pci_device *mhi_pdev = dev_get_drvdata(dev);
+	struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;
+
+	cancel_work_sync(&mhi_pdev->recovery_work);
+
+	/* Transition to M3 state */
+	mhi_pm_suspend(mhi_cntrl);
+
+	pci_save_state(pdev);
+	pci_disable_device(pdev);
+	pci_wake_from_d3(pdev, true);
+	pci_set_power_state(pdev, PCI_D3hot);
+
+	return 0;
+}
+
+static int __maybe_unused mhi_pci_resume(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct mhi_pci_device *mhi_pdev = dev_get_drvdata(dev);
+	struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;
+	int err;
+
+	pci_set_power_state(pdev, PCI_D0);
+	pci_restore_state(pdev);
+	pci_set_master(pdev);
+
+	err = pci_enable_device(pdev);
+	if (err)
+		goto err_recovery;
+
+	/* Exit M3, transition to M0 state */
+	err = mhi_pm_resume(mhi_cntrl);
+	if (err) {
+		dev_err(&pdev->dev, "failed to resume device: %d\n", err);
+		goto err_recovery;
+	}
+
+	return 0;
+
+err_recovery:
+	/* The device may have loose power or crashed, try recovering it */
+	queue_work(system_long_wq, &mhi_pdev->recovery_work);
+	return 0;
+}
+
+static const struct dev_pm_ops mhi_pci_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(mhi_pci_suspend, mhi_pci_resume)
+};
+
 static struct pci_driver mhi_pci_driver = {
 	.name		= "mhi-pci-generic",
 	.id_table	= mhi_pci_id_table,
 	.probe		= mhi_pci_probe,
 	.remove		= mhi_pci_remove,
 	.err_handler	= &mhi_pci_err_handler,
+	.driver.pm	= &mhi_pci_pm_ops
 };
 module_pci_driver(mhi_pci_driver);
 
-- 
2.7.4

[PATCH v3 7/9] mhi: pci_generic: Add PCI error handlers

[Loic Poulain]

In AER capable root complex, errors are reported to the host which
can then act accordingly and perform PCI recovering procedure.

This patch enables error reporting and implements error_detected,
slot_reset and resume callbacks.

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
---
 drivers/bus/mhi/pci_generic.c | 50 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/drivers/bus/mhi/pci_generic.c b/drivers/bus/mhi/pci_generic.c
index 9919ad6..3ac5cd2 100644
--- a/drivers/bus/mhi/pci_generic.c
+++ b/drivers/bus/mhi/pci_generic.c
@@ -8,6 +8,7 @@
  * Copyright (C) 2020 Linaro Ltd <loic.poulain@linaro.org>
  */
 
+#include <linux/aer.h>
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/mhi.h>
@@ -409,6 +410,8 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	pci_save_state(pdev);
 	mhi_pdev->pci_state = pci_store_saved_state(pdev);
 
+	pci_enable_pcie_error_reporting(pdev);
+
 	err = mhi_register_controller(mhi_cntrl, mhi_cntrl_config);
 	if (err)
 		return err;
@@ -508,7 +511,54 @@ void mhi_pci_reset_done(struct pci_dev *pdev)
 	set_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status);
 }
 
+static pci_ers_result_t mhi_pci_error_detected(struct pci_dev *pdev,
+					       pci_channel_state_t state)
+{
+	struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev);
+	struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;
+
+	dev_err(&pdev->dev, "PCI error detected, state = %u\n", state);
+
+	if (state == pci_channel_io_perm_failure)
+		return PCI_ERS_RESULT_DISCONNECT;
+
+	/* Clean up MHI state */
+	if (test_and_clear_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status)) {
+		mhi_power_down(mhi_cntrl, false);
+		mhi_unprepare_after_power_down(mhi_cntrl);
+	} else {
+		/* Nothing to do */
+		return PCI_ERS_RESULT_RECOVERED;
+	}
+
+	pci_disable_device(pdev);
+
+	return PCI_ERS_RESULT_NEED_RESET;
+}
+
+static pci_ers_result_t mhi_pci_slot_reset(struct pci_dev *pdev)
+{
+	if (pci_enable_device(pdev)) {
+		dev_err(&pdev->dev, "Cannot re-enable PCI device after reset.\n");
+		return PCI_ERS_RESULT_DISCONNECT;
+	}
+
+	return PCI_ERS_RESULT_RECOVERED;
+}
+
+static void mhi_pci_io_resume(struct pci_dev *pdev)
+{
+	struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev);
+
+	dev_err(&pdev->dev, "PCI slot reset done\n");
+
+	queue_work(system_long_wq, &mhi_pdev->recovery_work);
+}
+
 static const struct pci_error_handlers mhi_pci_err_handler = {
+	.error_detected = mhi_pci_error_detected,
+	.slot_reset = mhi_pci_slot_reset,
+	.resume = mhi_pci_io_resume,
 	.reset_prepare = mhi_pci_reset_prepare,
 	.reset_done = mhi_pci_reset_done,
 };
-- 
2.7.4

[PATCH v3 8/9] mhi: pci_generic: Add health-check

[Loic Poulain]

If the modem crashes for any reason, we may not be able to detect
it at MHI level (MHI registers not reachable anymore).

This patch implements a health-check mechanism to check regularly
that device is alive (MHI layer can communicate with). If device
is not alive (because a crash or unexpected reset), the recovery
procedure is triggered.

Tested successfully with Telit FN980m module.

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
---
 drivers/bus/mhi/pci_generic.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/bus/mhi/pci_generic.c b/drivers/bus/mhi/pci_generic.c
index 3ac5cd2..26c2dfa 100644
--- a/drivers/bus/mhi/pci_generic.c
+++ b/drivers/bus/mhi/pci_generic.c
@@ -14,12 +14,15 @@
 #include <linux/mhi.h>
 #include <linux/module.h>
 #include <linux/pci.h>
+#include <linux/timer.h>
 #include <linux/workqueue.h>
 
 #define MHI_PCI_DEFAULT_BAR_NUM 0
 
 #define DEV_RESET_REG (0xB0)
 
+#define HEALTH_CHECK_PERIOD (HZ * 5)
+
 /**
  * struct mhi_pci_dev_info - MHI PCI device specific information
  * @config: MHI controller configuration
@@ -190,6 +193,7 @@ struct mhi_pci_device {
 	struct mhi_controller mhi_cntrl;
 	struct pci_saved_state *pci_state;
 	struct work_struct recovery_work;
+	struct timer_list health_check_timer;
 	unsigned long status;
 };
 
@@ -332,6 +336,8 @@ static void mhi_pci_recovery_work(struct work_struct *work)
 
 	dev_warn(&pdev->dev, "device recovery started\n");
 
+	del_timer(&mhi_pdev->health_check_timer);
+
 	/* Clean up MHI state */
 	if (test_and_clear_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status)) {
 		mhi_power_down(mhi_cntrl, false);
@@ -355,6 +361,7 @@ static void mhi_pci_recovery_work(struct work_struct *work)
 		goto err_unprepare;
 
 	set_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status);
+	mod_timer(&mhi_pdev->health_check_timer, jiffies + HEALTH_CHECK_PERIOD);
 	return;
 
 err_unprepare:
@@ -364,6 +371,21 @@ static void mhi_pci_recovery_work(struct work_struct *work)
 		dev_err(&pdev->dev, "Recovery failed\n");
 }
 
+static void health_check(struct timer_list *t)
+{
+	struct mhi_pci_device *mhi_pdev = from_timer(mhi_pdev, t, health_check_timer);
+	struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;
+
+	if (!mhi_pci_is_alive(mhi_cntrl)) {
+		dev_err(mhi_cntrl->cntrl_dev, "Device died\n");
+		queue_work(system_long_wq, &mhi_pdev->recovery_work);
+		return;
+	}
+
+	/* reschedule in two seconds */
+	mod_timer(&mhi_pdev->health_check_timer, jiffies + HEALTH_CHECK_PERIOD);
+}
+
 static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
 	const struct mhi_pci_dev_info *info = (struct mhi_pci_dev_info *) id->driver_data;
@@ -379,6 +401,7 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		return -ENOMEM;
 
 	INIT_WORK(&mhi_pdev->recovery_work, mhi_pci_recovery_work);
+	timer_setup(&mhi_pdev->health_check_timer, health_check, 0);
 
 	mhi_cntrl_config = info->config;
 	mhi_cntrl = &mhi_pdev->mhi_cntrl;
@@ -431,6 +454,9 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	set_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status);
 
+	/* start health check */
+	mod_timer(&mhi_pdev->health_check_timer, jiffies + HEALTH_CHECK_PERIOD);
+
 	return 0;
 
 err_unprepare:
@@ -446,6 +472,7 @@ static void mhi_pci_remove(struct pci_dev *pdev)
 	struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev);
 	struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;
 
+	del_timer(&mhi_pdev->health_check_timer);
 	cancel_work_sync(&mhi_pdev->recovery_work);
 
 	if (test_and_clear_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status)) {
@@ -466,6 +493,8 @@ void mhi_pci_reset_prepare(struct pci_dev *pdev)
 
 	dev_info(&pdev->dev, "reset\n");
 
+	del_timer(&mhi_pdev->health_check_timer);
+
 	/* Clean up MHI state */
 	if (test_and_clear_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status)) {
 		mhi_power_down(mhi_cntrl, false);
@@ -509,6 +538,7 @@ void mhi_pci_reset_done(struct pci_dev *pdev)
 	}
 
 	set_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status);
+	mod_timer(&mhi_pdev->health_check_timer, jiffies + HEALTH_CHECK_PERIOD);
 }
 
 static pci_ers_result_t mhi_pci_error_detected(struct pci_dev *pdev,
@@ -569,6 +599,7 @@ int  __maybe_unused mhi_pci_suspend(struct device *dev)
 	struct mhi_pci_device *mhi_pdev = dev_get_drvdata(dev);
 	struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;
 
+	del_timer(&mhi_pdev->health_check_timer);
 	cancel_work_sync(&mhi_pdev->recovery_work);
 
 	/* Transition to M3 state */
@@ -604,6 +635,9 @@ static int __maybe_unused mhi_pci_resume(struct device *dev)
 		goto err_recovery;
 	}
 
+	/* Resume health check */
+	mod_timer(&mhi_pdev->health_check_timer, jiffies + HEALTH_CHECK_PERIOD);
+
 	return 0;
 
 err_recovery:
-- 
2.7.4

[PATCH v3 9/9] mhi: pci_generic: Increase controller timeout value

[Loic Poulain]

On cold boot, device can take slightly more than 5 seconds to start.
Increase the timeout to prevent MHI power-up issues.

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
Reviewed-by: Hemant Kumar <hemantk@codeaurora.org>
---
 drivers/bus/mhi/pci_generic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bus/mhi/pci_generic.c b/drivers/bus/mhi/pci_generic.c
index 26c2dfa..bbecec0 100644
--- a/drivers/bus/mhi/pci_generic.c
+++ b/drivers/bus/mhi/pci_generic.c
@@ -162,7 +162,7 @@ static const struct mhi_event_config modem_qcom_v1_mhi_events[] = {
 
 static const struct mhi_controller_config modem_qcom_v1_mhiv_config = {
 	.max_channels = 128,
-	.timeout_ms = 5000,
+	.timeout_ms = 8000,
 	.num_channels = ARRAY_SIZE(modem_qcom_v1_mhi_channels),
 	.ch_cfg = modem_qcom_v1_mhi_channels,
 	.num_events = ARRAY_SIZE(modem_qcom_v1_mhi_events),
-- 
2.7.4

Re: [PATCH v3 3/9] mhi: pci-generic: Perform hard reset on remove

[Jeffrey Hugo]

On 11/26/2020 8:29 AM, Loic Poulain wrote:
> Ensure that the device is hard-reset on remove to restore its initial
> state and avoid further issues on subsequent probe.
> 
> This has been tested with Telit FN980m module.
> 
> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> ---
>   drivers/bus/mhi/pci_generic.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/bus/mhi/pci_generic.c b/drivers/bus/mhi/pci_generic.c
> index d3896ef..4363676 100644
> --- a/drivers/bus/mhi/pci_generic.c
> +++ b/drivers/bus/mhi/pci_generic.c
> @@ -15,6 +15,8 @@
>   
>   #define MHI_PCI_DEFAULT_BAR_NUM 0
>   
> +#define DEV_RESET_REG (0xB0)
> +
>   /**
>    * struct mhi_pci_dev_info - MHI PCI device specific information
>    * @config: MHI controller configuration
> @@ -166,6 +168,11 @@ static void mhi_pci_status_cb(struct mhi_controller *mhi_cntrl,
>   	/* Nothing to do for now */
>   }
>   
> +static inline void mhi_pci_reset(struct mhi_controller *mhi_cntrl)
> +{
> +	writel(1, mhi_cntrl->regs + DEV_RESET_REG);
> +}
> +
>   static int mhi_pci_claim(struct mhi_controller *mhi_cntrl,
>   			 unsigned int bar_num, u64 dma_mask)
>   {
> @@ -329,6 +336,10 @@ static void mhi_pci_remove(struct pci_dev *pdev)
>   	mhi_power_down(mhi_cntrl, true);
>   	mhi_unprepare_after_power_down(mhi_cntrl);
>   	mhi_unregister_controller(mhi_cntrl);
> +
> +	/* MHI-layer reset could not be enough, always hard-reset the device */
> +	mhi_pci_reset(mhi_cntrl);
> +
>   	mhi_free_controller(mhi_cntrl);
>   }
>   
> 

Did you miss my eariler comment asking to make this functionality common?

Also, I caution you to think carefully about this.  Its possible doing 
this type of reset can take down the PCI link, and the host would need 
to have PCI hotplug to recover.  If not, the device will become 
inaccessable until the host reboots.

-- 
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v3 3/9] mhi: pci-generic: Perform hard reset on remove

[Jeffrey Hugo]

On 11/27/2020 10:34 AM, Jeffrey Hugo wrote:
> On 11/26/2020 8:29 AM, Loic Poulain wrote:
>> Ensure that the device is hard-reset on remove to restore its initial
>> state and avoid further issues on subsequent probe.
>>
>> This has been tested with Telit FN980m module.
>>
>> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
>> ---
>>   drivers/bus/mhi/pci_generic.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/bus/mhi/pci_generic.c 
>> b/drivers/bus/mhi/pci_generic.c
>> index d3896ef..4363676 100644
>> --- a/drivers/bus/mhi/pci_generic.c
>> +++ b/drivers/bus/mhi/pci_generic.c
>> @@ -15,6 +15,8 @@
>>   #define MHI_PCI_DEFAULT_BAR_NUM 0
>> +#define DEV_RESET_REG (0xB0)
>> +
>>   /**
>>    * struct mhi_pci_dev_info - MHI PCI device specific information
>>    * @config: MHI controller configuration
>> @@ -166,6 +168,11 @@ static void mhi_pci_status_cb(struct 
>> mhi_controller *mhi_cntrl,
>>       /* Nothing to do for now */
>>   }
>> +static inline void mhi_pci_reset(struct mhi_controller *mhi_cntrl)
>> +{
>> +    writel(1, mhi_cntrl->regs + DEV_RESET_REG);
>> +}
>> +
>>   static int mhi_pci_claim(struct mhi_controller *mhi_cntrl,
>>                unsigned int bar_num, u64 dma_mask)
>>   {
>> @@ -329,6 +336,10 @@ static void mhi_pci_remove(struct pci_dev *pdev)
>>       mhi_power_down(mhi_cntrl, true);
>>       mhi_unprepare_after_power_down(mhi_cntrl);
>>       mhi_unregister_controller(mhi_cntrl);
>> +
>> +    /* MHI-layer reset could not be enough, always hard-reset the 
>> device */
>> +    mhi_pci_reset(mhi_cntrl);
>> +
>>       mhi_free_controller(mhi_cntrl);
>>   }
>>
> 
> Did you miss my eariler comment asking to make this functionality common?

Sorry, I missed your reply to my reply.

> 
> Also, I caution you to think carefully about this.  Its possible doing 
> this type of reset can take down the PCI link, and the host would need 
> to have PCI hotplug to recover.  If not, the device will become 
> inaccessable until the host reboots.
> 


-- 
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v3 1/9] mhi: Add mhi_controller_initialize helper

[Manivannan Sadhasivam]

On Thu, Nov 26, 2020 at 04:28:59PM +0100, Loic Poulain wrote:
> This function allows to initialize a mhi_controller structure.
> Today, it only zeroing the structure.
> 

That's what kzalloc is also doing, right?

Thanks,
Mani

> Use this function from mhi_alloc_controller so that any further
> initialization can be factorized in initalize function.
> 
> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> ---
>  drivers/bus/mhi/core/init.c | 7 +++++++
>  include/linux/mhi.h         | 6 ++++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> index 96cde9c..4acad28 100644
> --- a/drivers/bus/mhi/core/init.c
> +++ b/drivers/bus/mhi/core/init.c
> @@ -1021,11 +1021,18 @@ void mhi_unregister_controller(struct mhi_controller *mhi_cntrl)
>  }
>  EXPORT_SYMBOL_GPL(mhi_unregister_controller);
>  
> +void mhi_initialize_controller(struct mhi_controller *mhi_cntrl)
> +{
> +	memset(mhi_cntrl, 0, sizeof(*mhi_cntrl));
> +}
> +EXPORT_SYMBOL_GPL(mhi_initialize_controller);
> +
>  struct mhi_controller *mhi_alloc_controller(void)
>  {
>  	struct mhi_controller *mhi_cntrl;
>  
>  	mhi_cntrl = kzalloc(sizeof(*mhi_cntrl), GFP_KERNEL);
> +	mhi_initialize_controller(mhi_cntrl);
>  
>  	return mhi_cntrl;
>  }
> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> index 5721a0a..30c676d 100644
> --- a/include/linux/mhi.h
> +++ b/include/linux/mhi.h
> @@ -537,6 +537,12 @@ struct mhi_driver {
>  #define to_mhi_device(dev) container_of(dev, struct mhi_device, dev)
>  
>  /**
> + * mhi_initialize_controller - Initialize MHI Controller structure
> + * @mhi_cntrl: MHI controller structure to initialize
> + */
> +void mhi_initialize_controller(struct mhi_controller *mhi_cntrl);
> +
> +/**
>   * mhi_alloc_controller - Allocate the MHI Controller structure
>   * Allocate the mhi_controller structure using zero initialized memory
>   */
> -- 
> 2.7.4
> 

Re: [PATCH v3 2/9] mhi: pci-generic: Increase number of hardware events

[Manivannan Sadhasivam]

On Thu, Nov 26, 2020 at 04:29:00PM +0100, Loic Poulain wrote:
> If the IPA (IP hardware accelerator) is starved of event ring elements,
> the modem is crashing (SDX55). That can be prevented by setting a
> larger number of events (i.e 2 x number of channel ring elements).
> 
> Tested with FN980m module.
> 
> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

> ---
>  drivers/bus/mhi/pci_generic.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/mhi/pci_generic.c b/drivers/bus/mhi/pci_generic.c
> index f5bee76..d3896ef 100644
> --- a/drivers/bus/mhi/pci_generic.c
> +++ b/drivers/bus/mhi/pci_generic.c
> @@ -91,7 +91,7 @@ struct mhi_pci_dev_info {
>  
>  #define MHI_EVENT_CONFIG_HW_DATA(ev_ring, ch_num) \
>  	{					\
> -		.num_elements = 128,		\
> +		.num_elements = 256,		\
>  		.irq_moderation_ms = 5,		\
>  		.irq = (ev_ring) + 1,		\
>  		.priority = 1,			\
> -- 
> 2.7.4
> 

Re: [PATCH v3 4/9] mhi: pci_generic: Enable burst mode for hardware channels

[Manivannan Sadhasivam]

On Thu, Nov 26, 2020 at 04:29:02PM +0100, Loic Poulain wrote:
> Hardware channels have a feature called burst mode that allows to
> queue transfer ring element(s) (TRE) to a channel without ringing
> the device doorbell. In that mode, the device is polling the channel
> context for new elements. This reduces the frequency of host initiated
> doorbells and increase throughput.
> 
> Create a new dedicated macro for hardware channels with burst enabled.
> 

We might need to move these macros to mhi.h at some point...

> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

> ---
>  drivers/bus/mhi/pci_generic.c | 34 ++++++++++++++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/bus/mhi/pci_generic.c b/drivers/bus/mhi/pci_generic.c
> index 4363676..6b6e5bf 100644
> --- a/drivers/bus/mhi/pci_generic.c
> +++ b/drivers/bus/mhi/pci_generic.c
> @@ -78,6 +78,36 @@ struct mhi_pci_dev_info {
>  		.offload_channel = false,	\
>  	}
>  
> +#define MHI_CHANNEL_CONFIG_HW_UL(ch_num, ch_name, el_count, ev_ring) \
> +	{						\
> +		.num = ch_num,				\
> +		.name = ch_name,			\
> +		.num_elements = el_count,		\
> +		.event_ring = ev_ring,			\
> +		.dir = DMA_TO_DEVICE,			\
> +		.ee_mask = BIT(MHI_EE_AMSS),		\
> +		.pollcfg = 0,				\
> +		.doorbell = MHI_DB_BRST_ENABLE,	\
> +		.lpm_notify = false,			\
> +		.offload_channel = false,		\
> +		.doorbell_mode_switch = true,		\
> +	}						\
> +
> +#define MHI_CHANNEL_CONFIG_HW_DL(ch_num, ch_name, el_count, ev_ring) \
> +	{						\
> +		.num = ch_num,				\
> +		.name = ch_name,			\
> +		.num_elements = el_count,		\
> +		.event_ring = ev_ring,			\
> +		.dir = DMA_FROM_DEVICE,			\
> +		.ee_mask = BIT(MHI_EE_AMSS),		\
> +		.pollcfg = 0,				\
> +		.doorbell = MHI_DB_BRST_ENABLE,	\
> +		.lpm_notify = false,			\
> +		.offload_channel = false,		\
> +		.doorbell_mode_switch = true,		\
> +	}
> +
>  #define MHI_EVENT_CONFIG_DATA(ev_ring)		\
>  	{					\
>  		.num_elements = 128,		\
> @@ -112,8 +142,8 @@ static const struct mhi_channel_config modem_qcom_v1_mhi_channels[] = {
>  	MHI_CHANNEL_CONFIG_DL(15, "QMI", 4, 0),
>  	MHI_CHANNEL_CONFIG_UL(20, "IPCR", 8, 0),
>  	MHI_CHANNEL_CONFIG_DL(21, "IPCR", 8, 0),
> -	MHI_CHANNEL_CONFIG_UL(100, "IP_HW0", 128, 1),
> -	MHI_CHANNEL_CONFIG_DL(101, "IP_HW0", 128, 2),
> +	MHI_CHANNEL_CONFIG_HW_UL(100, "IP_HW0", 128, 1),
> +	MHI_CHANNEL_CONFIG_HW_DL(101, "IP_HW0", 128, 2),
>  };
>  
>  static const struct mhi_event_config modem_qcom_v1_mhi_events[] = {
> -- 
> 2.7.4
> 

Re: [PATCH v3 5/9] mhi: pci_generic: Add support for reset

[Manivannan Sadhasivam]

On Thu, Nov 26, 2020 at 04:29:03PM +0100, Loic Poulain wrote:
> Add support for resetting the device, reset can be triggered in case
> of error or manually via sysfs (/sys/bus/pci/devices/*/reset).
> 
> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> ---
>  drivers/bus/mhi/pci_generic.c | 119 +++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 106 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/bus/mhi/pci_generic.c b/drivers/bus/mhi/pci_generic.c
> index 6b6e5bf..67fcbcf 100644
> --- a/drivers/bus/mhi/pci_generic.c
> +++ b/drivers/bus/mhi/pci_generic.c
> @@ -8,6 +8,7 @@
>   * Copyright (C) 2020 Linaro Ltd <loic.poulain@linaro.org>
>   */
>  
> +#include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/mhi.h>
>  #include <linux/module.h>
> @@ -179,6 +180,16 @@ static const struct pci_device_id mhi_pci_id_table[] = {
>  };
>  MODULE_DEVICE_TABLE(pci, mhi_pci_id_table);
>  
> +enum mhi_pci_device_status {
> +	MHI_PCI_DEV_STARTED,
> +};
> +
> +struct mhi_pci_device {
> +	struct mhi_controller mhi_cntrl;
> +	struct pci_saved_state *pci_state;
> +	unsigned long status;
> +};
> +
>  static int mhi_pci_read_reg(struct mhi_controller *mhi_cntrl,
>  			    void __iomem *addr, u32 *out)
>  {
> @@ -203,6 +214,20 @@ static inline void mhi_pci_reset(struct mhi_controller *mhi_cntrl)
>  	writel(1, mhi_cntrl->regs + DEV_RESET_REG);
>  }
>  
> +static bool mhi_pci_is_alive(struct mhi_controller *mhi_cntrl)
> +{
> +	struct pci_dev *pdev = to_pci_dev(mhi_cntrl->cntrl_dev);
> +	u16 vendor = 0;
> +
> +	if (pci_read_config_word(pdev, PCI_VENDOR_ID, &vendor))
> +		return false;
> +
> +	if (vendor == (u16) ~0 || vendor == 0)
> +		return false;
> +
> +	return true;
> +}
> +
>  static int mhi_pci_claim(struct mhi_controller *mhi_cntrl,
>  			 unsigned int bar_num, u64 dma_mask)
>  {
> @@ -298,16 +323,20 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  {
>  	const struct mhi_pci_dev_info *info = (struct mhi_pci_dev_info *) id->driver_data;
>  	const struct mhi_controller_config *mhi_cntrl_config;
> +	struct mhi_pci_device *mhi_pdev;
>  	struct mhi_controller *mhi_cntrl;
>  	int err;
>  
>  	dev_dbg(&pdev->dev, "MHI PCI device found: %s\n", info->name);
>  
> -	mhi_cntrl = mhi_alloc_controller();
> -	if (!mhi_cntrl)
> +	mhi_pdev = devm_kzalloc(&pdev->dev, sizeof(*mhi_pdev), GFP_KERNEL);
> +	if (!mhi_pdev)
>  		return -ENOMEM;

Still not agreeing to use the alloc API? I know that it does only one
job but the reason for pushing this API is that the MHI stack will
misbehave terribly if a non-initialized structure is passed to it. And
the only way to ensure is to provide an API and recommend the users to
use it.

>  
>  	mhi_cntrl_config = info->config;
> +	mhi_cntrl = &mhi_pdev->mhi_cntrl;
> +
> +	mhi_initialize_controller(mhi_cntrl);

No, please just stick to alloc API.

Thanks,
Mani

>  	mhi_cntrl->cntrl_dev = &pdev->dev;
>  	mhi_cntrl->iova_start = 0;
>  	mhi_cntrl->iova_stop = (dma_addr_t)DMA_BIT_MASK(info->dma_data_width);
> @@ -322,17 +351,21 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  
>  	err = mhi_pci_claim(mhi_cntrl, info->bar_num, DMA_BIT_MASK(info->dma_data_width));
>  	if (err)
> -		goto err_release;
> +		return err;
>  
>  	err = mhi_pci_get_irqs(mhi_cntrl, mhi_cntrl_config);
>  	if (err)
> -		goto err_release;
> +		return err;
> +
> +	pci_set_drvdata(pdev, mhi_pdev);
>  
> -	pci_set_drvdata(pdev, mhi_cntrl);
> +	/* Have stored pci confspace at hand for restore in sudden PCI error */
> +	pci_save_state(pdev);
> +	mhi_pdev->pci_state = pci_store_saved_state(pdev);
>  
>  	err = mhi_register_controller(mhi_cntrl, mhi_cntrl_config);
>  	if (err)
> -		goto err_release;
> +		return err;
>  
>  	/* MHI bus does not power up the controller by default */
>  	err = mhi_prepare_for_power_up(mhi_cntrl);
> @@ -347,37 +380,97 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  		goto err_unprepare;
>  	}
>  
> +	set_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status);
> +
>  	return 0;
>  
>  err_unprepare:
>  	mhi_unprepare_after_power_down(mhi_cntrl);
>  err_unregister:
>  	mhi_unregister_controller(mhi_cntrl);
> -err_release:
> -	mhi_free_controller(mhi_cntrl);
>  
>  	return err;
>  }
>  
>  static void mhi_pci_remove(struct pci_dev *pdev)
>  {
> -	struct mhi_controller *mhi_cntrl = pci_get_drvdata(pdev);
> +	struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev);
> +	struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;
> +
> +	if (test_and_clear_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status)) {
> +		mhi_power_down(mhi_cntrl, true);
> +		mhi_unprepare_after_power_down(mhi_cntrl);
> +	}
>  
> -	mhi_power_down(mhi_cntrl, true);
> -	mhi_unprepare_after_power_down(mhi_cntrl);
>  	mhi_unregister_controller(mhi_cntrl);
>  
>  	/* MHI-layer reset could not be enough, always hard-reset the device */
>  	mhi_pci_reset(mhi_cntrl);
> +}
> +
> +void mhi_pci_reset_prepare(struct pci_dev *pdev)
> +{
> +	struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev);
> +	struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;
> +
> +	dev_info(&pdev->dev, "reset\n");
> +
> +	/* Clean up MHI state */
> +	if (test_and_clear_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status)) {
> +		mhi_power_down(mhi_cntrl, false);
> +		mhi_unprepare_after_power_down(mhi_cntrl);
> +	}
> +
> +	/* cause internal device reset */
> +	mhi_pci_reset(mhi_cntrl);
> +
> +	/* Be sure device reset has been executed */
> +	msleep(500);
> +}
> +
> +void mhi_pci_reset_done(struct pci_dev *pdev)
> +{
> +	struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev);
> +	struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;
> +	int err;
> +
> +	/* Restore initial known working PCI state */
> +	pci_load_saved_state(pdev, mhi_pdev->pci_state);
> +	pci_restore_state(pdev);
> +
> +	/* Is device status available ? */
> +	if (!mhi_pci_is_alive(mhi_cntrl)) {
> +		dev_err(&pdev->dev, "reset failed\n");
> +		return;
> +	}
>  
> -	mhi_free_controller(mhi_cntrl);
> +	err = mhi_prepare_for_power_up(mhi_cntrl);
> +	if (err) {
> +		dev_err(&pdev->dev, "failed to prepare MHI controller\n");
> +		return;
> +	}
> +
> +	err = mhi_sync_power_up(mhi_cntrl);
> +	if (err) {
> +		dev_err(&pdev->dev, "failed to power up MHI controller\n");
> +		mhi_unprepare_after_power_down(mhi_cntrl);
> +		return;
> +	}
> +
> +	set_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status);
>  }
>  
> +static const struct pci_error_handlers mhi_pci_err_handler = {
> +	.reset_prepare = mhi_pci_reset_prepare,
> +	.reset_done = mhi_pci_reset_done,
> +};
> +
>  static struct pci_driver mhi_pci_driver = {
>  	.name		= "mhi-pci-generic",
>  	.id_table	= mhi_pci_id_table,
>  	.probe		= mhi_pci_probe,
> -	.remove		= mhi_pci_remove
> +	.remove		= mhi_pci_remove,
> +	.err_handler	= &mhi_pci_err_handler,
>  };
>  module_pci_driver(mhi_pci_driver);
>  
> -- 
> 2.7.4
> 

Re: [PATCH v3 9/9] mhi: pci_generic: Increase controller timeout value

[Manivannan Sadhasivam]

On Thu, Nov 26, 2020 at 04:29:07PM +0100, Loic Poulain wrote:
> On cold boot, device can take slightly more than 5 seconds to start.
> Increase the timeout to prevent MHI power-up issues.
> 
> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> Reviewed-by: Hemant Kumar <hemantk@codeaurora.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

> ---
>  drivers/bus/mhi/pci_generic.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/mhi/pci_generic.c b/drivers/bus/mhi/pci_generic.c
> index 26c2dfa..bbecec0 100644
> --- a/drivers/bus/mhi/pci_generic.c
> +++ b/drivers/bus/mhi/pci_generic.c
> @@ -162,7 +162,7 @@ static const struct mhi_event_config modem_qcom_v1_mhi_events[] = {
>  
>  static const struct mhi_controller_config modem_qcom_v1_mhiv_config = {
>  	.max_channels = 128,
> -	.timeout_ms = 5000,
> +	.timeout_ms = 8000,
>  	.num_channels = ARRAY_SIZE(modem_qcom_v1_mhi_channels),
>  	.ch_cfg = modem_qcom_v1_mhi_channels,
>  	.num_events = ARRAY_SIZE(modem_qcom_v1_mhi_events),
> -- 
> 2.7.4
> 

Re: [PATCH v3 8/9] mhi: pci_generic: Add health-check

[Manivannan Sadhasivam]

On Thu, Nov 26, 2020 at 04:29:06PM +0100, Loic Poulain wrote:
> If the modem crashes for any reason, we may not be able to detect
> it at MHI level (MHI registers not reachable anymore).
> 
> This patch implements a health-check mechanism to check regularly
> that device is alive (MHI layer can communicate with). If device
> is not alive (because a crash or unexpected reset), the recovery
> procedure is triggered.
> 
> Tested successfully with Telit FN980m module.
> 
> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>

Bhaumik, Hemant, does this looks reasonable to you? Or we can do a
better job in the MHI stack. To me this is not a specific usecase for
Telit.

If you guys plan to implement it later, I can just apply this patch in
the meantime as it looks good to me.

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

> ---
>  drivers/bus/mhi/pci_generic.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/drivers/bus/mhi/pci_generic.c b/drivers/bus/mhi/pci_generic.c
> index 3ac5cd2..26c2dfa 100644
> --- a/drivers/bus/mhi/pci_generic.c
> +++ b/drivers/bus/mhi/pci_generic.c
> @@ -14,12 +14,15 @@
>  #include <linux/mhi.h>
>  #include <linux/module.h>
>  #include <linux/pci.h>
> +#include <linux/timer.h>
>  #include <linux/workqueue.h>
>  
>  #define MHI_PCI_DEFAULT_BAR_NUM 0
>  
>  #define DEV_RESET_REG (0xB0)
>  
> +#define HEALTH_CHECK_PERIOD (HZ * 5)
> +
>  /**
>   * struct mhi_pci_dev_info - MHI PCI device specific information
>   * @config: MHI controller configuration
> @@ -190,6 +193,7 @@ struct mhi_pci_device {
>  	struct mhi_controller mhi_cntrl;
>  	struct pci_saved_state *pci_state;
>  	struct work_struct recovery_work;
> +	struct timer_list health_check_timer;
>  	unsigned long status;
>  };
>  
> @@ -332,6 +336,8 @@ static void mhi_pci_recovery_work(struct work_struct *work)
>  
>  	dev_warn(&pdev->dev, "device recovery started\n");
>  
> +	del_timer(&mhi_pdev->health_check_timer);
> +
>  	/* Clean up MHI state */
>  	if (test_and_clear_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status)) {
>  		mhi_power_down(mhi_cntrl, false);
> @@ -355,6 +361,7 @@ static void mhi_pci_recovery_work(struct work_struct *work)
>  		goto err_unprepare;
>  
>  	set_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status);
> +	mod_timer(&mhi_pdev->health_check_timer, jiffies + HEALTH_CHECK_PERIOD);
>  	return;
>  
>  err_unprepare:
> @@ -364,6 +371,21 @@ static void mhi_pci_recovery_work(struct work_struct *work)
>  		dev_err(&pdev->dev, "Recovery failed\n");
>  }
>  
> +static void health_check(struct timer_list *t)
> +{
> +	struct mhi_pci_device *mhi_pdev = from_timer(mhi_pdev, t, health_check_timer);
> +	struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;
> +
> +	if (!mhi_pci_is_alive(mhi_cntrl)) {
> +		dev_err(mhi_cntrl->cntrl_dev, "Device died\n");
> +		queue_work(system_long_wq, &mhi_pdev->recovery_work);
> +		return;
> +	}
> +
> +	/* reschedule in two seconds */
> +	mod_timer(&mhi_pdev->health_check_timer, jiffies + HEALTH_CHECK_PERIOD);
> +}
> +
>  static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  {
>  	const struct mhi_pci_dev_info *info = (struct mhi_pci_dev_info *) id->driver_data;
> @@ -379,6 +401,7 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  		return -ENOMEM;
>  
>  	INIT_WORK(&mhi_pdev->recovery_work, mhi_pci_recovery_work);
> +	timer_setup(&mhi_pdev->health_check_timer, health_check, 0);
>  
>  	mhi_cntrl_config = info->config;
>  	mhi_cntrl = &mhi_pdev->mhi_cntrl;
> @@ -431,6 +454,9 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  
>  	set_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status);
>  
> +	/* start health check */
> +	mod_timer(&mhi_pdev->health_check_timer, jiffies + HEALTH_CHECK_PERIOD);
> +
>  	return 0;
>  
>  err_unprepare:
> @@ -446,6 +472,7 @@ static void mhi_pci_remove(struct pci_dev *pdev)
>  	struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev);
>  	struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;
>  
> +	del_timer(&mhi_pdev->health_check_timer);
>  	cancel_work_sync(&mhi_pdev->recovery_work);
>  
>  	if (test_and_clear_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status)) {
> @@ -466,6 +493,8 @@ void mhi_pci_reset_prepare(struct pci_dev *pdev)
>  
>  	dev_info(&pdev->dev, "reset\n");
>  
> +	del_timer(&mhi_pdev->health_check_timer);
> +
>  	/* Clean up MHI state */
>  	if (test_and_clear_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status)) {
>  		mhi_power_down(mhi_cntrl, false);
> @@ -509,6 +538,7 @@ void mhi_pci_reset_done(struct pci_dev *pdev)
>  	}
>  
>  	set_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status);
> +	mod_timer(&mhi_pdev->health_check_timer, jiffies + HEALTH_CHECK_PERIOD);
>  }
>  
>  static pci_ers_result_t mhi_pci_error_detected(struct pci_dev *pdev,
> @@ -569,6 +599,7 @@ int  __maybe_unused mhi_pci_suspend(struct device *dev)
>  	struct mhi_pci_device *mhi_pdev = dev_get_drvdata(dev);
>  	struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;
>  
> +	del_timer(&mhi_pdev->health_check_timer);
>  	cancel_work_sync(&mhi_pdev->recovery_work);
>  
>  	/* Transition to M3 state */
> @@ -604,6 +635,9 @@ static int __maybe_unused mhi_pci_resume(struct device *dev)
>  		goto err_recovery;
>  	}
>  
> +	/* Resume health check */
> +	mod_timer(&mhi_pdev->health_check_timer, jiffies + HEALTH_CHECK_PERIOD);
> +
>  	return 0;
>  
>  err_recovery:
> -- 
> 2.7.4
> 

Re: [PATCH v3 0/9] mhi: pci_generic: Misc improvements

[Manivannan Sadhasivam]

Hi,

On Thu, Nov 26, 2020 at 04:28:58PM +0100, Loic Poulain wrote:
> This series adjust some configuration values to ensure stability and
> robustness of mhi pci devices (timeout, number of events, burst mode).
> 
> It also includes support for system sleep as well as a recovery procedure
> that can be triggered when a PCI error is reported, either by PCI AER or by
> the new health-check mechanism.
> 
> All these changes have been tested with Telit FN980m module
> 

I've excluded couple of patches while reviewing and waiting for Hemant or
Bhaumik to do their reviews first.

Thanks,
Mani

> v2: 
>   - Cancel recovery work on suspend
> v3:
>   - enable doorbell_mode_switch for burst channel (HW)
>   - Add mhi_initialize_controller helper patch
> 
> Loic Poulain (9):
>   mhi: Add mhi_controller_initialize helper
>   mhi: pci-generic: Increase number of hardware events
>   mhi: pci-generic: Perform hard reset on remove
>   mhi: pci_generic: Enable burst mode for hardware channels
>   mhi: pci_generic: Add support for reset
>   mhi: pci_generic: Add suspend/resume/recovery procedure
>   mhi: pci_generic: Add PCI error handlers
>   mhi: pci_generic: Add health-check
>   mhi: pci_generic: Increase controller timeout value
> 
>  drivers/bus/mhi/core/init.c   |   7 +
>  drivers/bus/mhi/pci_generic.c | 354 ++++++++++++++++++++++++++++++++++++++++--
>  include/linux/mhi.h           |   6 +
>  3 files changed, 350 insertions(+), 17 deletions(-)
> 
> -- 
> 2.7.4
> 

Re: [PATCH v3 5/9] mhi: pci_generic: Add support for reset

[Loic Poulain]

Hi Mani, Bhaumik,

On Sat, 28 Nov 2020 at 06:49, Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> >  {
> > @@ -298,16 +323,20 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >  {
> >       const struct mhi_pci_dev_info *info = (struct mhi_pci_dev_info *) id->driver_data;
> >       const struct mhi_controller_config *mhi_cntrl_config;
> > +     struct mhi_pci_device *mhi_pdev;
> >       struct mhi_controller *mhi_cntrl;
> >       int err;
> >
> >       dev_dbg(&pdev->dev, "MHI PCI device found: %s\n", info->name);
> >
> > -     mhi_cntrl = mhi_alloc_controller();
> > -     if (!mhi_cntrl)
> > +     mhi_pdev = devm_kzalloc(&pdev->dev, sizeof(*mhi_pdev), GFP_KERNEL);
> > +     if (!mhi_pdev)
> >               return -ENOMEM;
>
> Still not agreeing to use the alloc API? I know that it does only one
> job but the reason for pushing this API is that the MHI stack will
> misbehave terribly if a non-initialized structure is passed to it. And
> the only way to ensure is to provide an API and recommend the users to
> use it.

I think there is a misunderstanding of my will here :-), actually, I'm
not against using a specific API, but as you see here I'm not
allocating a mhi_controller structure but a mhi_pci_device structure,
which in turn includes (or inherit from) mhi_controller struct.

If mhi_alloc_controller() is the only API allowing to
create+initialize a mhi_controller object, that implies:
a. Statically allocated mhi_controller is not possible
b. non-standalone mhi_controller structure is not possible (my case)

If you mandate this and do not allow a. and b. , then yes, I'll use
mhi_controller_alloc, but that would mean having two nested dynamic
allocations, one for mhi_pdev and one for mhi_pdev->mhi_cntrl,
cross-referencing (for finding mhi_pdev from mhi_cntrl since no more
container_of), and double freeing... that complicates a bit, is
suboptimal and does not really make sense, since conceptually,
mhi_pdev and its mhi_cntl member are the same 'object' (exactly like
mhi_dev and mhi_dev->dev are the same object in MHI core).

I understand we may have to perform some extra initialization and
cannot just do zeroed allocation for the mhi_controller, but what I
say is that this initialization should be do-able, regardless you want
to (also) dynamically allocate the device or not. That why I proposed
to introduce mhi_initialize_controller() as a solution to keep things
simple, in the same way as other subsystems: device_initialize,
snd_device_initialize, nand_controller_init...

Regards,
Loic


>
> >
> >       mhi_cntrl_config = info->config;
> > +     mhi_cntrl = &mhi_pdev->mhi_cntrl;
> > +
> > +     mhi_initialize_controller(mhi_cntrl);
>
> No, please just stick to alloc API.
>
> Thanks,
> Mani
>
> >       mhi_cntrl->cntrl_dev = &pdev->dev;
> >       mhi_cntrl->iova_start = 0;
> >       mhi_cntrl->iova_stop = (dma_addr_t)DMA_BIT_MASK(info->dma_data_width);
> > @@ -322,17 +351,21 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >
> >       err = mhi_pci_claim(mhi_cntrl, info->bar_num, DMA_BIT_MASK(info->dma_data_width));
> >       if (err)
> > -             goto err_release;
> > +             return err;
> >
> >       err = mhi_pci_get_irqs(mhi_cntrl, mhi_cntrl_config);
> >       if (err)
> > -             goto err_release;
> > +             return err;
> > +
> > +     pci_set_drvdata(pdev, mhi_pdev);
> >
> > -     pci_set_drvdata(pdev, mhi_cntrl);
> > +     /* Have stored pci confspace at hand for restore in sudden PCI error */
> > +     pci_save_state(pdev);
> > +     mhi_pdev->pci_state = pci_store_saved_state(pdev);
> >
> >       err = mhi_register_controller(mhi_cntrl, mhi_cntrl_config);
> >       if (err)
> > -             goto err_release;
> > +             return err;
> >
> >       /* MHI bus does not power up the controller by default */
> >       err = mhi_prepare_for_power_up(mhi_cntrl);
> > @@ -347,37 +380,97 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >               goto err_unprepare;
> >       }
> >
> > +     set_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status);
> > +
> >       return 0;
> >
> >  err_unprepare:
> >       mhi_unprepare_after_power_down(mhi_cntrl);
> >  err_unregister:
> >       mhi_unregister_controller(mhi_cntrl);
> > -err_release:
> > -     mhi_free_controller(mhi_cntrl);
> >
> >       return err;
> >  }
> >
> >  static void mhi_pci_remove(struct pci_dev *pdev)
> >  {
> > -     struct mhi_controller *mhi_cntrl = pci_get_drvdata(pdev);
> > +     struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev);
> > +     struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;
> > +
> > +     if (test_and_clear_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status)) {
> > +             mhi_power_down(mhi_cntrl, true);
> > +             mhi_unprepare_after_power_down(mhi_cntrl);
> > +     }
> >
> > -     mhi_power_down(mhi_cntrl, true);
> > -     mhi_unprepare_after_power_down(mhi_cntrl);
> >       mhi_unregister_controller(mhi_cntrl);
> >
> >       /* MHI-layer reset could not be enough, always hard-reset the device */
> >       mhi_pci_reset(mhi_cntrl);
> > +}
> > +
> > +void mhi_pci_reset_prepare(struct pci_dev *pdev)
> > +{
> > +     struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev);
> > +     struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;
> > +
> > +     dev_info(&pdev->dev, "reset\n");
> > +
> > +     /* Clean up MHI state */
> > +     if (test_and_clear_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status)) {
> > +             mhi_power_down(mhi_cntrl, false);
> > +             mhi_unprepare_after_power_down(mhi_cntrl);
> > +     }
> > +
> > +     /* cause internal device reset */
> > +     mhi_pci_reset(mhi_cntrl);
> > +
> > +     /* Be sure device reset has been executed */
> > +     msleep(500);
> > +}
> > +
> > +void mhi_pci_reset_done(struct pci_dev *pdev)
> > +{
> > +     struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev);
> > +     struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;
> > +     int err;
> > +
> > +     /* Restore initial known working PCI state */
> > +     pci_load_saved_state(pdev, mhi_pdev->pci_state);
> > +     pci_restore_state(pdev);
> > +
> > +     /* Is device status available ? */
> > +     if (!mhi_pci_is_alive(mhi_cntrl)) {
> > +             dev_err(&pdev->dev, "reset failed\n");
> > +             return;
> > +     }
> >
> > -     mhi_free_controller(mhi_cntrl);
> > +     err = mhi_prepare_for_power_up(mhi_cntrl);
> > +     if (err) {
> > +             dev_err(&pdev->dev, "failed to prepare MHI controller\n");
> > +             return;
> > +     }
> > +
> > +     err = mhi_sync_power_up(mhi_cntrl);
> > +     if (err) {
> > +             dev_err(&pdev->dev, "failed to power up MHI controller\n");
> > +             mhi_unprepare_after_power_down(mhi_cntrl);
> > +             return;
> > +     }
> > +
> > +     set_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status);
> >  }
> >
> > +static const struct pci_error_handlers mhi_pci_err_handler = {
> > +     .reset_prepare = mhi_pci_reset_prepare,
> > +     .reset_done = mhi_pci_reset_done,
> > +};
> > +
> >  static struct pci_driver mhi_pci_driver = {
> >       .name           = "mhi-pci-generic",
> >       .id_table       = mhi_pci_id_table,
> >       .probe          = mhi_pci_probe,
> > -     .remove         = mhi_pci_remove
> > +     .remove         = mhi_pci_remove,
> > +     .err_handler    = &mhi_pci_err_handler,
> >  };
> >  module_pci_driver(mhi_pci_driver);
> >
> > --
> > 2.7.4
> >

Re: [PATCH v3 8/9] mhi: pci_generic: Add health-check

[Hemant Kumar]

Hi Mani,

On 11/27/20 9:59 PM, Manivannan Sadhasivam wrote:
> On Thu, Nov 26, 2020 at 04:29:06PM +0100, Loic Poulain wrote:
>> If the modem crashes for any reason, we may not be able to detect
>> it at MHI level (MHI registers not reachable anymore).
>>
>> This patch implements a health-check mechanism to check regularly
>> that device is alive (MHI layer can communicate with). If device
>> is not alive (because a crash or unexpected reset), the recovery
>> procedure is triggered.
>>
>> Tested successfully with Telit FN980m module.
>>
>> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> 
> Bhaumik, Hemant, does this looks reasonable to you? Or we can do a
> better job in the MHI stack. To me this is not a specific usecase for
> Telit.

As far as i understood the change, MHI is unaware of this because this 
check is for underlying transport e.g. pci. This change looks good to 
me. Please apply this patch.

Thanks,
Hemant
> 
> If you guys plan to implement it later, I can just apply this patch in
> the meantime as it looks good to me.
> 
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> Thanks,
> Mani
> 
>> ---
>>   drivers/bus/mhi/pci_generic.c | 34 ++++++++++++++++++++++++++++++++++
>>   1 file changed, 34 insertions(+)
>>
>> diff --git a/drivers/bus/mhi/pci_generic.c b/drivers/bus/mhi/pci_generic.c
>> index 3ac5cd2..26c2dfa 100644
>> --- a/drivers/bus/mhi/pci_generic.c
>> +++ b/drivers/bus/mhi/pci_generic.c
>> @@ -14,12 +14,15 @@
>>   #include <linux/mhi.h>
>>   #include <linux/module.h>
>>   #include <linux/pci.h>
>> +#include <linux/timer.h>
>>   #include <linux/workqueue.h>
>>   
>>   #define MHI_PCI_DEFAULT_BAR_NUM 0
>>   
>>   #define DEV_RESET_REG (0xB0)
>>   
>> +#define HEALTH_CHECK_PERIOD (HZ * 5)
>> +
>>   /**
>>    * struct mhi_pci_dev_info - MHI PCI device specific information
>>    * @config: MHI controller configuration
>> @@ -190,6 +193,7 @@ struct mhi_pci_device {
>>   	struct mhi_controller mhi_cntrl;
>>   	struct pci_saved_state *pci_state;
>>   	struct work_struct recovery_work;
>> +	struct timer_list health_check_timer;
>>   	unsigned long status;
>>   };
>>   
>> @@ -332,6 +336,8 @@ static void mhi_pci_recovery_work(struct work_struct *work)
>>   
>>   	dev_warn(&pdev->dev, "device recovery started\n");
>>   
>> +	del_timer(&mhi_pdev->health_check_timer);
>> +
>>   	/* Clean up MHI state */
>>   	if (test_and_clear_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status)) {
>>   		mhi_power_down(mhi_cntrl, false);
>> @@ -355,6 +361,7 @@ static void mhi_pci_recovery_work(struct work_struct *work)
>>   		goto err_unprepare;
>>   
>>   	set_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status);
>> +	mod_timer(&mhi_pdev->health_check_timer, jiffies + HEALTH_CHECK_PERIOD);
>>   	return;
>>   
>>   err_unprepare:
>> @@ -364,6 +371,21 @@ static void mhi_pci_recovery_work(struct work_struct *work)
>>   		dev_err(&pdev->dev, "Recovery failed\n");
>>   }
>>   
>> +static void health_check(struct timer_list *t)
>> +{
>> +	struct mhi_pci_device *mhi_pdev = from_timer(mhi_pdev, t, health_check_timer);
>> +	struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;
>> +
>> +	if (!mhi_pci_is_alive(mhi_cntrl)) {
>> +		dev_err(mhi_cntrl->cntrl_dev, "Device died\n");
>> +		queue_work(system_long_wq, &mhi_pdev->recovery_work);
>> +		return;
>> +	}
>> +
>> +	/* reschedule in two seconds */
>> +	mod_timer(&mhi_pdev->health_check_timer, jiffies + HEALTH_CHECK_PERIOD);
>> +}
>> +
>>   static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>   {
>>   	const struct mhi_pci_dev_info *info = (struct mhi_pci_dev_info *) id->driver_data;
>> @@ -379,6 +401,7 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>   		return -ENOMEM;
>>   
>>   	INIT_WORK(&mhi_pdev->recovery_work, mhi_pci_recovery_work);
>> +	timer_setup(&mhi_pdev->health_check_timer, health_check, 0);
>>   
>>   	mhi_cntrl_config = info->config;
>>   	mhi_cntrl = &mhi_pdev->mhi_cntrl;
>> @@ -431,6 +454,9 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>   
>>   	set_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status);
>>   
>> +	/* start health check */
>> +	mod_timer(&mhi_pdev->health_check_timer, jiffies + HEALTH_CHECK_PERIOD);
>> +
>>   	return 0;
>>   
>>   err_unprepare:
>> @@ -446,6 +472,7 @@ static void mhi_pci_remove(struct pci_dev *pdev)
>>   	struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev);
>>   	struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;
>>   
>> +	del_timer(&mhi_pdev->health_check_timer);
>>   	cancel_work_sync(&mhi_pdev->recovery_work);
>>   
>>   	if (test_and_clear_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status)) {
>> @@ -466,6 +493,8 @@ void mhi_pci_reset_prepare(struct pci_dev *pdev)
>>   
>>   	dev_info(&pdev->dev, "reset\n");
>>   
>> +	del_timer(&mhi_pdev->health_check_timer);
>> +
>>   	/* Clean up MHI state */
>>   	if (test_and_clear_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status)) {
>>   		mhi_power_down(mhi_cntrl, false);
>> @@ -509,6 +538,7 @@ void mhi_pci_reset_done(struct pci_dev *pdev)
>>   	}
>>   
>>   	set_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status);
>> +	mod_timer(&mhi_pdev->health_check_timer, jiffies + HEALTH_CHECK_PERIOD);
>>   }
>>   
>>   static pci_ers_result_t mhi_pci_error_detected(struct pci_dev *pdev,
>> @@ -569,6 +599,7 @@ int  __maybe_unused mhi_pci_suspend(struct device *dev)
>>   	struct mhi_pci_device *mhi_pdev = dev_get_drvdata(dev);
>>   	struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;
>>   
>> +	del_timer(&mhi_pdev->health_check_timer);
>>   	cancel_work_sync(&mhi_pdev->recovery_work);
>>   
>>   	/* Transition to M3 state */
>> @@ -604,6 +635,9 @@ static int __maybe_unused mhi_pci_resume(struct device *dev)
>>   		goto err_recovery;
>>   	}
>>   
>> +	/* Resume health check */
>> +	mod_timer(&mhi_pdev->health_check_timer, jiffies + HEALTH_CHECK_PERIOD);
>> +
>>   	return 0;
>>   
>>   err_recovery:
>> -- 
>> 2.7.4
>>

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 8/9] mhi: pci_generic: Add health-check

[Hemant Kumar]

On 11/26/20 7:29 AM, Loic Poulain wrote:
> If the modem crashes for any reason, we may not be able to detect
> it at MHI level (MHI registers not reachable anymore).
> 
> This patch implements a health-check mechanism to check regularly
> that device is alive (MHI layer can communicate with). If device
> is not alive (because a crash or unexpected reset), the recovery
> procedure is triggered.
> 
> Tested successfully with Telit FN980m module.
> 
> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> ---

Reviewed-by: Hemant Kumar <hemantk@codeaurora.org>
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 5/9] mhi: pci_generic: Add support for reset

[Bhaumik Bhatt]

Hi Loic/Mani,
On 2020-11-30 01:08 AM, Loic Poulain wrote:
> Hi Mani, Bhaumik,
> 
> On Sat, 28 Nov 2020 at 06:49, Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
>> 
>> >  {
>> > @@ -298,16 +323,20 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>> >  {
>> >       const struct mhi_pci_dev_info *info = (struct mhi_pci_dev_info *) id->driver_data;
>> >       const struct mhi_controller_config *mhi_cntrl_config;
>> > +     struct mhi_pci_device *mhi_pdev;
>> >       struct mhi_controller *mhi_cntrl;
>> >       int err;
>> >
>> >       dev_dbg(&pdev->dev, "MHI PCI device found: %s\n", info->name);
>> >
>> > -     mhi_cntrl = mhi_alloc_controller();
>> > -     if (!mhi_cntrl)
>> > +     mhi_pdev = devm_kzalloc(&pdev->dev, sizeof(*mhi_pdev), GFP_KERNEL);
>> > +     if (!mhi_pdev)
>> >               return -ENOMEM;
>> 
>> Still not agreeing to use the alloc API? I know that it does only one
>> job but the reason for pushing this API is that the MHI stack will
>> misbehave terribly if a non-initialized structure is passed to it. And
>> the only way to ensure is to provide an API and recommend the users to
>> use it.
> 
> I think there is a misunderstanding of my will here :-), actually, I'm
> not against using a specific API, but as you see here I'm not
> allocating a mhi_controller structure but a mhi_pci_device structure,
> which in turn includes (or inherit from) mhi_controller struct.
> 
> If mhi_alloc_controller() is the only API allowing to
> create+initialize a mhi_controller object, that implies:
> a. Statically allocated mhi_controller is not possible
> b. non-standalone mhi_controller structure is not possible (my case)
> 
> If you mandate this and do not allow a. and b. , then yes, I'll use
> mhi_controller_alloc, but that would mean having two nested dynamic
> allocations, one for mhi_pdev and one for mhi_pdev->mhi_cntrl,
> cross-referencing (for finding mhi_pdev from mhi_cntrl since no more
> container_of), and double freeing... that complicates a bit, is
> suboptimal and does not really make sense, since conceptually,
> mhi_pdev and its mhi_cntl member are the same 'object' (exactly like
> mhi_dev and mhi_dev->dev are the same object in MHI core).
> 
> I understand we may have to perform some extra initialization and
> cannot just do zeroed allocation for the mhi_controller, but what I
> say is that this initialization should be do-able, regardless you want
> to (also) dynamically allocate the device or not. That why I proposed
> to introduce mhi_initialize_controller() as a solution to keep things
> simple, in the same way as other subsystems: device_initialize,
> snd_device_initialize, nand_controller_init...
> 
> Regards,
> Loic
> 
> 
I don't see using the new API as a problem but it is more work for the
controller and having one API is better than having two and having to 
explain
why we have them.

If Mani agrees with this approach I am OK with it.
>> 
>> >
>> >       mhi_cntrl_config = info->config;
>> > +     mhi_cntrl = &mhi_pdev->mhi_cntrl;
>> > +
>> > +     mhi_initialize_controller(mhi_cntrl);
>> 
>> No, please just stick to alloc API.
>> 
>> Thanks,
>> Mani
>> 
>> >       mhi_cntrl->cntrl_dev = &pdev->dev;
>> >       mhi_cntrl->iova_start = 0;
>> >       mhi_cntrl->iova_stop = (dma_addr_t)DMA_BIT_MASK(info->dma_data_width);
>> > @@ -322,17 +351,21 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>> >
>> >       err = mhi_pci_claim(mhi_cntrl, info->bar_num, DMA_BIT_MASK(info->dma_data_width));
>> >       if (err)
>> > -             goto err_release;
>> > +             return err;
>> >
>> >       err = mhi_pci_get_irqs(mhi_cntrl, mhi_cntrl_config);
>> >       if (err)
>> > -             goto err_release;
>> > +             return err;
>> > +
>> > +     pci_set_drvdata(pdev, mhi_pdev);
>> >
>> > -     pci_set_drvdata(pdev, mhi_cntrl);
>> > +     /* Have stored pci confspace at hand for restore in sudden PCI error */
>> > +     pci_save_state(pdev);
>> > +     mhi_pdev->pci_state = pci_store_saved_state(pdev);
>> >
>> >       err = mhi_register_controller(mhi_cntrl, mhi_cntrl_config);
>> >       if (err)
>> > -             goto err_release;
>> > +             return err;
>> >
>> >       /* MHI bus does not power up the controller by default */
>> >       err = mhi_prepare_for_power_up(mhi_cntrl);
>> > @@ -347,37 +380,97 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>> >               goto err_unprepare;
>> >       }
>> >
>> > +     set_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status);
>> > +
>> >       return 0;
>> >
>> >  err_unprepare:
>> >       mhi_unprepare_after_power_down(mhi_cntrl);
>> >  err_unregister:
>> >       mhi_unregister_controller(mhi_cntrl);
>> > -err_release:
>> > -     mhi_free_controller(mhi_cntrl);
>> >
>> >       return err;
>> >  }
>> >
>> >  static void mhi_pci_remove(struct pci_dev *pdev)
>> >  {
>> > -     struct mhi_controller *mhi_cntrl = pci_get_drvdata(pdev);
>> > +     struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev);
>> > +     struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;
>> > +
>> > +     if (test_and_clear_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status)) {
>> > +             mhi_power_down(mhi_cntrl, true);
>> > +             mhi_unprepare_after_power_down(mhi_cntrl);
>> > +     }
>> >
>> > -     mhi_power_down(mhi_cntrl, true);
>> > -     mhi_unprepare_after_power_down(mhi_cntrl);
>> >       mhi_unregister_controller(mhi_cntrl);
>> >
>> >       /* MHI-layer reset could not be enough, always hard-reset the device */
>> >       mhi_pci_reset(mhi_cntrl);
>> > +}
>> > +
>> > +void mhi_pci_reset_prepare(struct pci_dev *pdev)
>> > +{
>> > +     struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev);
>> > +     struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;
>> > +
>> > +     dev_info(&pdev->dev, "reset\n");
>> > +
>> > +     /* Clean up MHI state */
>> > +     if (test_and_clear_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status)) {
>> > +             mhi_power_down(mhi_cntrl, false);
>> > +             mhi_unprepare_after_power_down(mhi_cntrl);
>> > +     }
>> > +
>> > +     /* cause internal device reset */
>> > +     mhi_pci_reset(mhi_cntrl);
>> > +
>> > +     /* Be sure device reset has been executed */
>> > +     msleep(500);
>> > +}
>> > +
>> > +void mhi_pci_reset_done(struct pci_dev *pdev)
>> > +{
>> > +     struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev);
>> > +     struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;
>> > +     int err;
>> > +
>> > +     /* Restore initial known working PCI state */
>> > +     pci_load_saved_state(pdev, mhi_pdev->pci_state);
>> > +     pci_restore_state(pdev);
>> > +
>> > +     /* Is device status available ? */
>> > +     if (!mhi_pci_is_alive(mhi_cntrl)) {
>> > +             dev_err(&pdev->dev, "reset failed\n");
>> > +             return;
>> > +     }
>> >
>> > -     mhi_free_controller(mhi_cntrl);
>> > +     err = mhi_prepare_for_power_up(mhi_cntrl);
>> > +     if (err) {
>> > +             dev_err(&pdev->dev, "failed to prepare MHI controller\n");
>> > +             return;
>> > +     }
>> > +
>> > +     err = mhi_sync_power_up(mhi_cntrl);
>> > +     if (err) {
>> > +             dev_err(&pdev->dev, "failed to power up MHI controller\n");
>> > +             mhi_unprepare_after_power_down(mhi_cntrl);
>> > +             return;
>> > +     }
>> > +
>> > +     set_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status);
>> >  }
>> >
>> > +static const struct pci_error_handlers mhi_pci_err_handler = {
>> > +     .reset_prepare = mhi_pci_reset_prepare,
>> > +     .reset_done = mhi_pci_reset_done,
>> > +};
>> > +
>> >  static struct pci_driver mhi_pci_driver = {
>> >       .name           = "mhi-pci-generic",
>> >       .id_table       = mhi_pci_id_table,
>> >       .probe          = mhi_pci_probe,
>> > -     .remove         = mhi_pci_remove
>> > +     .remove         = mhi_pci_remove,
>> > +     .err_handler    = &mhi_pci_err_handler,
>> >  };
>> >  module_pci_driver(mhi_pci_driver);
>> >
>> > --
>> > 2.7.4
>> >

Thanks,
Bhaumik
---
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 1/9] mhi: Add mhi_controller_initialize helper

[Bhaumik Bhatt]

On 2020-11-27 09:42 PM, Manivannan Sadhasivam wrote:
> On Thu, Nov 26, 2020 at 04:28:59PM +0100, Loic Poulain wrote:
>> This function allows to initialize a mhi_controller structure.
>> Today, it only zeroing the structure.
>> 
> 
> That's what kzalloc is also doing, right?
> 
> Thanks,
> Mani
> 
>> Use this function from mhi_alloc_controller so that any further
>> initialization can be factorized in initalize function.
>> 
>> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
>> ---
>>  drivers/bus/mhi/core/init.c | 7 +++++++
>>  include/linux/mhi.h         | 6 ++++++
>>  2 files changed, 13 insertions(+)
>> 
>> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
>> index 96cde9c..4acad28 100644
>> --- a/drivers/bus/mhi/core/init.c
>> +++ b/drivers/bus/mhi/core/init.c
>> @@ -1021,11 +1021,18 @@ void mhi_unregister_controller(struct 
>> mhi_controller *mhi_cntrl)
>>  }
>>  EXPORT_SYMBOL_GPL(mhi_unregister_controller);
>> 
>> +void mhi_initialize_controller(struct mhi_controller *mhi_cntrl)
>> +{
>> +	memset(mhi_cntrl, 0, sizeof(*mhi_cntrl));
>> +}
>> +EXPORT_SYMBOL_GPL(mhi_initialize_controller);
>> +
>>  struct mhi_controller *mhi_alloc_controller(void)
>>  {
>>  	struct mhi_controller *mhi_cntrl;
>> 
>>  	mhi_cntrl = kzalloc(sizeof(*mhi_cntrl), GFP_KERNEL);
>> +	mhi_initialize_controller(mhi_cntrl);
This line is not required here.
>> 
>>  	return mhi_cntrl;
>>  }
>> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
>> index 5721a0a..30c676d 100644
>> --- a/include/linux/mhi.h
>> +++ b/include/linux/mhi.h
>> @@ -537,6 +537,12 @@ struct mhi_driver {
>>  #define to_mhi_device(dev) container_of(dev, struct mhi_device, dev)
>> 
>>  /**
>> + * mhi_initialize_controller - Initialize MHI Controller structure
>> + * @mhi_cntrl: MHI controller structure to initialize
>> + */
>> +void mhi_initialize_controller(struct mhi_controller *mhi_cntrl);
>> +
>> +/**
>>   * mhi_alloc_controller - Allocate the MHI Controller structure
>>   * Allocate the mhi_controller structure using zero initialized 
>> memory
>>   */
>> --
>> 2.7.4
>> 

Thanks,
Bhaumik
---
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 5/9] mhi: pci_generic: Add support for reset

[Loic Poulain]

Hi Mani,

On Wed, 2 Dec 2020 at 02:24, Bhaumik Bhatt <bbhatt@codeaurora.org> wrote:
> > On Sat, 28 Nov 2020 at 06:49, Manivannan Sadhasivam
> > <manivannan.sadhasivam@linaro.org> wrote:
> >>
> >> >  {
> >> > @@ -298,16 +323,20 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >> >  {
> >> >       const struct mhi_pci_dev_info *info = (struct mhi_pci_dev_info *) id->driver_data;
> >> >       const struct mhi_controller_config *mhi_cntrl_config;
> >> > +     struct mhi_pci_device *mhi_pdev;
> >> >       struct mhi_controller *mhi_cntrl;
> >> >       int err;
> >> >
> >> >       dev_dbg(&pdev->dev, "MHI PCI device found: %s\n", info->name);
> >> >
> >> > -     mhi_cntrl = mhi_alloc_controller();
> >> > -     if (!mhi_cntrl)
> >> > +     mhi_pdev = devm_kzalloc(&pdev->dev, sizeof(*mhi_pdev), GFP_KERNEL);
> >> > +     if (!mhi_pdev)
> >> >               return -ENOMEM;
> >>
> >> Still not agreeing to use the alloc API? I know that it does only one
> >> job but the reason for pushing this API is that the MHI stack will
> >> misbehave terribly if a non-initialized structure is passed to it. And
> >> the only way to ensure is to provide an API and recommend the users to
> >> use it.
> >
> > I think there is a misunderstanding of my will here :-), actually, I'm
> > not against using a specific API, but as you see here I'm not
> > allocating a mhi_controller structure but a mhi_pci_device structure,
> > which in turn includes (or inherit from) mhi_controller struct.
> >
> > If mhi_alloc_controller() is the only API allowing to
> > create+initialize a mhi_controller object, that implies:
> > a. Statically allocated mhi_controller is not possible
> > b. non-standalone mhi_controller structure is not possible (my case)
> >
> > If you mandate this and do not allow a. and b. , then yes, I'll use
> > mhi_controller_alloc, but that would mean having two nested dynamic
> > allocations, one for mhi_pdev and one for mhi_pdev->mhi_cntrl,
> > cross-referencing (for finding mhi_pdev from mhi_cntrl since no more
> > container_of), and double freeing... that complicates a bit, is
> > suboptimal and does not really make sense, since conceptually,
> > mhi_pdev and its mhi_cntl member are the same 'object' (exactly like
> > mhi_dev and mhi_dev->dev are the same object in MHI core).
> >
> > I understand we may have to perform some extra initialization and
> > cannot just do zeroed allocation for the mhi_controller, but what I
> > say is that this initialization should be do-able, regardless you want
> > to (also) dynamically allocate the device or not. That why I proposed
> > to introduce mhi_initialize_controller() as a solution to keep things
> > simple, in the same way as other subsystems: device_initialize,
> > snd_device_initialize, nand_controller_init...
> >
> > Regards,
> > Loic
> >
> >
> I don't see using the new API as a problem but it is more work for the
> controller and having one API is better than having two and having to
> explain
> why we have them.

Before going with V4, I would like your feedback here.

Thanks,
Loic

>
> If Mani agrees with this approach I am OK with it.
> >>
> >> >
> >> >       mhi_cntrl_config = info->config;
> >> > +     mhi_cntrl = &mhi_pdev->mhi_cntrl;
> >> > +
> >> > +     mhi_initialize_controller(mhi_cntrl);
> >>
> >> No, please just stick to alloc API.
> >>
> >> Thanks,
> >> Mani
> >>
> >> >       mhi_cntrl->cntrl_dev = &pdev->dev;
> >> >       mhi_cntrl->iova_start = 0;
> >> >       mhi_cntrl->iova_stop = (dma_addr_t)DMA_BIT_MASK(info->dma_data_width);
> >> > @@ -322,17 +351,21 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >> >
> >> >       err = mhi_pci_claim(mhi_cntrl, info->bar_num, DMA_BIT_MASK(info->dma_data_width));
> >> >       if (err)
> >> > -             goto err_release;
> >> > +             return err;
> >> >
> >> >       err = mhi_pci_get_irqs(mhi_cntrl, mhi_cntrl_config);
> >> >       if (err)
> >> > -             goto err_release;
> >> > +             return err;
> >> > +
> >> > +     pci_set_drvdata(pdev, mhi_pdev);
> >> >
> >> > -     pci_set_drvdata(pdev, mhi_cntrl);
> >> > +     /* Have stored pci confspace at hand for restore in sudden PCI error */
> >> > +     pci_save_state(pdev);
> >> > +     mhi_pdev->pci_state = pci_store_saved_state(pdev);
> >> >
> >> >       err = mhi_register_controller(mhi_cntrl, mhi_cntrl_config);
> >> >       if (err)
> >> > -             goto err_release;
> >> > +             return err;
> >> >
> >> >       /* MHI bus does not power up the controller by default */
> >> >       err = mhi_prepare_for_power_up(mhi_cntrl);
> >> > @@ -347,37 +380,97 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >> >               goto err_unprepare;
> >> >       }
> >> >
> >> > +     set_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status);
> >> > +
> >> >       return 0;
> >> >
> >> >  err_unprepare:
> >> >       mhi_unprepare_after_power_down(mhi_cntrl);
> >> >  err_unregister:
> >> >       mhi_unregister_controller(mhi_cntrl);
> >> > -err_release:
> >> > -     mhi_free_controller(mhi_cntrl);
> >> >
> >> >       return err;
> >> >  }
> >> >
> >> >  static void mhi_pci_remove(struct pci_dev *pdev)
> >> >  {
> >> > -     struct mhi_controller *mhi_cntrl = pci_get_drvdata(pdev);
> >> > +     struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev);
> >> > +     struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;
> >> > +
> >> > +     if (test_and_clear_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status)) {
> >> > +             mhi_power_down(mhi_cntrl, true);
> >> > +             mhi_unprepare_after_power_down(mhi_cntrl);
> >> > +     }
> >> >
> >> > -     mhi_power_down(mhi_cntrl, true);
> >> > -     mhi_unprepare_after_power_down(mhi_cntrl);
> >> >       mhi_unregister_controller(mhi_cntrl);
> >> >
> >> >       /* MHI-layer reset could not be enough, always hard-reset the device */
> >> >       mhi_pci_reset(mhi_cntrl);
> >> > +}
> >> > +
> >> > +void mhi_pci_reset_prepare(struct pci_dev *pdev)
> >> > +{
> >> > +     struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev);
> >> > +     struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;
> >> > +
> >> > +     dev_info(&pdev->dev, "reset\n");
> >> > +
> >> > +     /* Clean up MHI state */
> >> > +     if (test_and_clear_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status)) {
> >> > +             mhi_power_down(mhi_cntrl, false);
> >> > +             mhi_unprepare_after_power_down(mhi_cntrl);
> >> > +     }
> >> > +
> >> > +     /* cause internal device reset */
> >> > +     mhi_pci_reset(mhi_cntrl);
> >> > +
> >> > +     /* Be sure device reset has been executed */
> >> > +     msleep(500);
> >> > +}
> >> > +
> >> > +void mhi_pci_reset_done(struct pci_dev *pdev)
> >> > +{
> >> > +     struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev);
> >> > +     struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;
> >> > +     int err;
> >> > +
> >> > +     /* Restore initial known working PCI state */
> >> > +     pci_load_saved_state(pdev, mhi_pdev->pci_state);
> >> > +     pci_restore_state(pdev);
> >> > +
> >> > +     /* Is device status available ? */
> >> > +     if (!mhi_pci_is_alive(mhi_cntrl)) {
> >> > +             dev_err(&pdev->dev, "reset failed\n");
> >> > +             return;
> >> > +     }
> >> >
> >> > -     mhi_free_controller(mhi_cntrl);
> >> > +     err = mhi_prepare_for_power_up(mhi_cntrl);
> >> > +     if (err) {
> >> > +             dev_err(&pdev->dev, "failed to prepare MHI controller\n");
> >> > +             return;
> >> > +     }
> >> > +
> >> > +     err = mhi_sync_power_up(mhi_cntrl);
> >> > +     if (err) {
> >> > +             dev_err(&pdev->dev, "failed to power up MHI controller\n");
> >> > +             mhi_unprepare_after_power_down(mhi_cntrl);
> >> > +             return;
> >> > +     }
> >> > +
> >> > +     set_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status);
> >> >  }
> >> >
> >> > +static const struct pci_error_handlers mhi_pci_err_handler = {
> >> > +     .reset_prepare = mhi_pci_reset_prepare,
> >> > +     .reset_done = mhi_pci_reset_done,
> >> > +};
> >> > +
> >> >  static struct pci_driver mhi_pci_driver = {
> >> >       .name           = "mhi-pci-generic",
> >> >       .id_table       = mhi_pci_id_table,
> >> >       .probe          = mhi_pci_probe,
> >> > -     .remove         = mhi_pci_remove
> >> > +     .remove         = mhi_pci_remove,
> >> > +     .err_handler    = &mhi_pci_err_handler,
> >> >  };
> >> >  module_pci_driver(mhi_pci_driver);
> >> >
> >> > --
> >> > 2.7.4
> >> >
>
> Thanks,
> Bhaumik
> ---
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> Forum,
> a Linux Foundation Collaborative Project