Add EDL sysfs entry for mhi controller that provides edl_trigger callback.
Add mhi_pci_generic_edl_trigger for qualcomm sdx55,sdx65 and sdx75 as
edl_trigger callback.
v1->v2:
1. Move all process needed by qualcomm sdx55,sdx65,sdx75 to enter EDL into
mhi_pci_generic_edl_trigger() as the callback to edl_trigger.
2. MHI stack creates EDL sysfs entry to invoke edl_trigger callback so
that devices need different mechanism to enter EDL can provide its own
edl_trigger callabck .
Qiang Yu (2):
bus: mhi: host: Add sysfs entry to force device to enter EDL
bus: mhi: host: pci_generic: Add edl callback to enter EDL
drivers/bus/mhi/host/init.c | 35 ++++++++++++++++++++++++++
drivers/bus/mhi/host/pci_generic.c | 50 ++++++++++++++++++++++++++++++++++++++
include/linux/mhi.h | 1 +
3 files changed, 86 insertions(+)
--
2.7.4
Add sysfs entry to allow users of MHI bus force device to enter EDL.
Considering that the way to enter EDL mode varies from device to device and
some devices even do not support EDL. Hence, add a callback edl_trigger in
mhi controller as part of the sysfs entry to be invoked and MHI core will
only create EDL sysfs entry for mhi controller that provides edl_trigger
callback. All of the process a specific device required to enter EDL mode
can be placed in this callback.
Signed-off-by: Qiang Yu <[email protected]>
---
drivers/bus/mhi/host/init.c | 35 +++++++++++++++++++++++++++++++++++
include/linux/mhi.h | 1 +
2 files changed, 36 insertions(+)
diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
index 44f9349..333ac94 100644
--- a/drivers/bus/mhi/host/init.c
+++ b/drivers/bus/mhi/host/init.c
@@ -127,6 +127,32 @@ static ssize_t soc_reset_store(struct device *dev,
}
static DEVICE_ATTR_WO(soc_reset);
+static ssize_t force_edl_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct mhi_device *mhi_dev = to_mhi_device(dev);
+ struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
+ unsigned long val;
+ int ret;
+
+ ret = kstrtoul(buf, 10, &val);
+ if (ret < 0) {
+ dev_err(dev, "Could not parse string: %d\n", ret);
+ return ret;
+ }
+
+ if (!val)
+ return count;
+
+ ret = mhi_cntrl->edl_trigger(mhi_cntrl);
+ if (ret)
+ return ret;
+
+ return count;
+}
+static DEVICE_ATTR_WO(force_edl);
+
static struct attribute *mhi_dev_attrs[] = {
&dev_attr_serial_number.attr,
&dev_attr_oem_pk_hash.attr,
@@ -1018,6 +1044,12 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
if (ret)
goto err_release_dev;
+ if (mhi_cntrl->edl_trigger) {
+ ret = sysfs_create_file(&mhi_dev->dev.kobj, &dev_attr_force_edl.attr);
+ if (ret)
+ goto err_release_dev;
+ }
+
mhi_cntrl->mhi_dev = mhi_dev;
mhi_create_debugfs(mhi_cntrl);
@@ -1051,6 +1083,9 @@ void mhi_unregister_controller(struct mhi_controller *mhi_cntrl)
mhi_deinit_free_irq(mhi_cntrl);
mhi_destroy_debugfs(mhi_cntrl);
+ if (mhi_cntrl->edl_trigger)
+ sysfs_remove_file(&mhi_dev->dev.kobj, &dev_attr_force_edl.attr);
+
destroy_workqueue(mhi_cntrl->hiprio_wq);
kfree(mhi_cntrl->mhi_cmd);
kfree(mhi_cntrl->mhi_event);
diff --git a/include/linux/mhi.h b/include/linux/mhi.h
index cde01e1..8db56d7 100644
--- a/include/linux/mhi.h
+++ b/include/linux/mhi.h
@@ -435,6 +435,7 @@ struct mhi_controller {
void (*write_reg)(struct mhi_controller *mhi_cntrl, void __iomem *addr,
u32 val);
void (*reset)(struct mhi_controller *mhi_cntrl);
+ int (*edl_trigger)(struct mhi_controller *mhi_cntrl);
size_t buffer_len;
int index;
--
2.7.4
Add mhi_pci_generic_edl_trigger as edl_trigger for some devices (eg. SDX65)
to enter EDL mode by writing the 0xEDEDEDED cookie to the channel 91
doorbell register and forcing an SOC reset afterwards.
Signed-off-by: Qiang Yu <[email protected]>
---
drivers/bus/mhi/host/pci_generic.c | 50 ++++++++++++++++++++++++++++++++++++++
1 file changed, 50 insertions(+)
diff --git a/drivers/bus/mhi/host/pci_generic.c b/drivers/bus/mhi/host/pci_generic.c
index 51639bf..a529815 100644
--- a/drivers/bus/mhi/host/pci_generic.c
+++ b/drivers/bus/mhi/host/pci_generic.c
@@ -27,12 +27,23 @@
#define PCI_VENDOR_ID_THALES 0x1269
#define PCI_VENDOR_ID_QUECTEL 0x1eac
+#define MHI_EDL_DB 91
+#define MHI_EDL_COOKIE 0xEDEDEDED
+
+/* Device can enter EDL by first setting edl cookie then issuing inband reset*/
+#define MHI_PCI_GENERIC_EDL_TRIGGER BIT(0)
+
+#define CHDBOFF 0x18
+#define CHDBOFF_CHDBOFF_MASK 0xFFFFFFFF
+#define CHDBOFF_CHDBOFF_SHIFT 0
+
/**
* struct mhi_pci_dev_info - MHI PCI device specific information
* @config: MHI controller configuration
* @name: name of the PCI module
* @fw: firmware path (if any)
* @edl: emergency download mode firmware path (if any)
+ * @edl_trigger: each bit represents a different way to enter EDL
* @bar_num: PCI base address register to use for MHI MMIO register space
* @dma_data_width: DMA transfer word size (32 or 64 bits)
* @mru_default: default MRU size for MBIM network packets
@@ -44,6 +55,7 @@ struct mhi_pci_dev_info {
const char *name;
const char *fw;
const char *edl;
+ unsigned int edl_trigger;
unsigned int bar_num;
unsigned int dma_data_width;
unsigned int mru_default;
@@ -292,6 +304,7 @@ static const struct mhi_pci_dev_info mhi_qcom_sdx75_info = {
.name = "qcom-sdx75m",
.fw = "qcom/sdx75m/xbl.elf",
.edl = "qcom/sdx75m/edl.mbn",
+ .edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
.config = &modem_qcom_v2_mhiv_config,
.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
.dma_data_width = 32,
@@ -302,6 +315,7 @@ static const struct mhi_pci_dev_info mhi_qcom_sdx65_info = {
.name = "qcom-sdx65m",
.fw = "qcom/sdx65m/xbl.elf",
.edl = "qcom/sdx65m/edl.mbn",
+ .edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
.config = &modem_qcom_v1_mhiv_config,
.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
.dma_data_width = 32,
@@ -312,6 +326,7 @@ static const struct mhi_pci_dev_info mhi_qcom_sdx55_info = {
.name = "qcom-sdx55m",
.fw = "qcom/sdx55m/sbl1.mbn",
.edl = "qcom/sdx55m/edl.mbn",
+ .edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
.config = &modem_qcom_v1_mhiv_config,
.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
.dma_data_width = 32,
@@ -928,6 +943,38 @@ static void health_check(struct timer_list *t)
mod_timer(&mhi_pdev->health_check_timer, jiffies + HEALTH_CHECK_PERIOD);
}
+static int mhi_pci_generic_edl_trigger(struct mhi_controller *mhi_cntrl)
+{
+ int ret;
+ u32 val;
+ void __iomem *edl_db;
+ void __iomem *base = mhi_cntrl->regs;
+
+ ret = mhi_device_get_sync(mhi_cntrl->mhi_dev);
+ if (ret) {
+ dev_err(mhi_cntrl->cntrl_dev, "Wake up device fail before trigger EDL\n");
+ return ret;
+ }
+
+ pm_wakeup_event(&mhi_cntrl->mhi_dev->dev, 0);
+ mhi_cntrl->runtime_get(mhi_cntrl);
+
+ mhi_cntrl->read_reg(mhi_cntrl, base + CHDBOFF, &val);
+ val = (val & CHDBOFF_CHDBOFF_MASK) >> CHDBOFF_CHDBOFF_SHIFT;
+
+ edl_db = base + val + (8 * MHI_EDL_DB);
+
+ mhi_cntrl->write_reg(mhi_cntrl, edl_db + 4, upper_32_bits(MHI_EDL_COOKIE));
+ mhi_cntrl->write_reg(mhi_cntrl, edl_db, lower_32_bits(MHI_EDL_COOKIE));
+
+ mhi_soc_reset(mhi_cntrl);
+
+ mhi_cntrl->runtime_put(mhi_cntrl);
+ mhi_device_put(mhi_cntrl->mhi_dev);
+
+ return 0;
+}
+
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;
@@ -962,6 +1009,9 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
mhi_cntrl->runtime_put = mhi_pci_runtime_put;
mhi_cntrl->mru = info->mru_default;
+ if (info->edl_trigger & MHI_PCI_GENERIC_EDL_TRIGGER)
+ mhi_cntrl->edl_trigger = mhi_pci_generic_edl_trigger;
+
if (info->sideband_wake) {
mhi_cntrl->wake_get = mhi_pci_wake_get_nop;
mhi_cntrl->wake_put = mhi_pci_wake_put_nop;
--
2.7.4
On 4/10/2024 9:15 PM, Qiang Yu wrote:
> Add sysfs entry to allow users of MHI bus force device to enter EDL.
> Considering that the way to enter EDL mode varies from device to device and
> some devices even do not support EDL. Hence, add a callback edl_trigger in
> mhi controller as part of the sysfs entry to be invoked and MHI core will
> only create EDL sysfs entry for mhi controller that provides edl_trigger
> callback. All of the process a specific device required to enter EDL mode
> can be placed in this callback.
>
> Signed-off-by: Qiang Yu <[email protected]>
> ---
> drivers/bus/mhi/host/init.c | 35 +++++++++++++++++++++++++++++++++++
> include/linux/mhi.h | 1 +
No update to Documentation/ABI/stable/sysfs-bus-mhi ?
> 2 files changed, 36 insertions(+)
>
> diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
> index 44f9349..333ac94 100644
> --- a/drivers/bus/mhi/host/init.c
> +++ b/drivers/bus/mhi/host/init.c
> @@ -127,6 +127,32 @@ static ssize_t soc_reset_store(struct device *dev,
> }
> static DEVICE_ATTR_WO(soc_reset);
>
> +static ssize_t force_edl_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct mhi_device *mhi_dev = to_mhi_device(dev);
> + struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
> + unsigned long val;
> + int ret;
> +
> + ret = kstrtoul(buf, 10, &val);
> + if (ret < 0) {
> + dev_err(dev, "Could not parse string: %d\n", ret);
> + return ret;
> + }
> +
> + if (!val)
> + return count;
> +
> + ret = mhi_cntrl->edl_trigger(mhi_cntrl);
> + if (ret)
> + return ret;
> +
> + return count;
> +}
> +static DEVICE_ATTR_WO(force_edl);
> +
> static struct attribute *mhi_dev_attrs[] = {
> &dev_attr_serial_number.attr,
> &dev_attr_oem_pk_hash.attr,
> @@ -1018,6 +1044,12 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
> if (ret)
> goto err_release_dev;
>
> + if (mhi_cntrl->edl_trigger) {
> + ret = sysfs_create_file(&mhi_dev->dev.kobj, &dev_attr_force_edl.attr);
> + if (ret)
> + goto err_release_dev;
> + }
> +
> mhi_cntrl->mhi_dev = mhi_dev;
>
> mhi_create_debugfs(mhi_cntrl);
> @@ -1051,6 +1083,9 @@ void mhi_unregister_controller(struct mhi_controller *mhi_cntrl)
> mhi_deinit_free_irq(mhi_cntrl);
> mhi_destroy_debugfs(mhi_cntrl);
>
> + if (mhi_cntrl->edl_trigger)
> + sysfs_remove_file(&mhi_dev->dev.kobj, &dev_attr_force_edl.attr);
> +
> destroy_workqueue(mhi_cntrl->hiprio_wq);
> kfree(mhi_cntrl->mhi_cmd);
> kfree(mhi_cntrl->mhi_event);
> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> index cde01e1..8db56d7 100644
> --- a/include/linux/mhi.h
> +++ b/include/linux/mhi.h
> @@ -435,6 +435,7 @@ struct mhi_controller {
> void (*write_reg)(struct mhi_controller *mhi_cntrl, void __iomem *addr,
> u32 val);
> void (*reset)(struct mhi_controller *mhi_cntrl);
> + int (*edl_trigger)(struct mhi_controller *mhi_cntrl);
No update to the documentation for this struct?
On 4/10/2024 9:15 PM, Qiang Yu wrote:
> Add mhi_pci_generic_edl_trigger as edl_trigger for some devices (eg. SDX65)
> to enter EDL mode by writing the 0xEDEDEDED cookie to the channel 91
> doorbell register and forcing an SOC reset afterwards.
>
> Signed-off-by: Qiang Yu <[email protected]>
> ---
> drivers/bus/mhi/host/pci_generic.c | 50 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 50 insertions(+)
>
> diff --git a/drivers/bus/mhi/host/pci_generic.c b/drivers/bus/mhi/host/pci_generic.c
> index 51639bf..a529815 100644
> --- a/drivers/bus/mhi/host/pci_generic.c
> +++ b/drivers/bus/mhi/host/pci_generic.c
> @@ -27,12 +27,23 @@
> #define PCI_VENDOR_ID_THALES 0x1269
> #define PCI_VENDOR_ID_QUECTEL 0x1eac
>
> +#define MHI_EDL_DB 91
> +#define MHI_EDL_COOKIE 0xEDEDEDED
> +
> +/* Device can enter EDL by first setting edl cookie then issuing inband reset*/
> +#define MHI_PCI_GENERIC_EDL_TRIGGER BIT(0)
> +
> +#define CHDBOFF 0x18
This is already in drivers/bus/mhi/common.h why duplicate it here?
> +#define CHDBOFF_CHDBOFF_MASK 0xFFFFFFFF
> +#define CHDBOFF_CHDBOFF_SHIFT 0
> +
> /**
> * struct mhi_pci_dev_info - MHI PCI device specific information
> * @config: MHI controller configuration
> * @name: name of the PCI module
> * @fw: firmware path (if any)
> * @edl: emergency download mode firmware path (if any)
> + * @edl_trigger: each bit represents a different way to enter EDL
> * @bar_num: PCI base address register to use for MHI MMIO register space
> * @dma_data_width: DMA transfer word size (32 or 64 bits)
> * @mru_default: default MRU size for MBIM network packets
> @@ -44,6 +55,7 @@ struct mhi_pci_dev_info {
> const char *name;
> const char *fw;
> const char *edl;
> + unsigned int edl_trigger;
> unsigned int bar_num;
> unsigned int dma_data_width;
> unsigned int mru_default;
> @@ -292,6 +304,7 @@ static const struct mhi_pci_dev_info mhi_qcom_sdx75_info = {
> .name = "qcom-sdx75m",
> .fw = "qcom/sdx75m/xbl.elf",
> .edl = "qcom/sdx75m/edl.mbn",
> + .edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
> .config = &modem_qcom_v2_mhiv_config,
> .bar_num = MHI_PCI_DEFAULT_BAR_NUM,
> .dma_data_width = 32,
> @@ -302,6 +315,7 @@ static const struct mhi_pci_dev_info mhi_qcom_sdx65_info = {
> .name = "qcom-sdx65m",
> .fw = "qcom/sdx65m/xbl.elf",
> .edl = "qcom/sdx65m/edl.mbn",
> + .edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
> .config = &modem_qcom_v1_mhiv_config,
> .bar_num = MHI_PCI_DEFAULT_BAR_NUM,
> .dma_data_width = 32,
> @@ -312,6 +326,7 @@ static const struct mhi_pci_dev_info mhi_qcom_sdx55_info = {
> .name = "qcom-sdx55m",
> .fw = "qcom/sdx55m/sbl1.mbn",
> .edl = "qcom/sdx55m/edl.mbn",
> + .edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
> .config = &modem_qcom_v1_mhiv_config,
> .bar_num = MHI_PCI_DEFAULT_BAR_NUM,
> .dma_data_width = 32,
> @@ -928,6 +943,38 @@ static void health_check(struct timer_list *t)
> mod_timer(&mhi_pdev->health_check_timer, jiffies + HEALTH_CHECK_PERIOD);
> }
>
> +static int mhi_pci_generic_edl_trigger(struct mhi_controller *mhi_cntrl)
> +{
> + int ret;
> + u32 val;
> + void __iomem *edl_db;
> + void __iomem *base = mhi_cntrl->regs;
It looks like this file follows reverse christmas tree, but this
function does not. you should fix it.
> +
> + ret = mhi_device_get_sync(mhi_cntrl->mhi_dev);
> + if (ret) {
> + dev_err(mhi_cntrl->cntrl_dev, "Wake up device fail before trigger EDL\n");
> + return ret;
> + }
> +
> + pm_wakeup_event(&mhi_cntrl->mhi_dev->dev, 0);
> + mhi_cntrl->runtime_get(mhi_cntrl);
> +
> + mhi_cntrl->read_reg(mhi_cntrl, base + CHDBOFF, &val);
> + val = (val & CHDBOFF_CHDBOFF_MASK) >> CHDBOFF_CHDBOFF_SHIFT;
> +
> + edl_db = base + val + (8 * MHI_EDL_DB);
> +
> + mhi_cntrl->write_reg(mhi_cntrl, edl_db + 4, upper_32_bits(MHI_EDL_COOKIE));
> + mhi_cntrl->write_reg(mhi_cntrl, edl_db, lower_32_bits(MHI_EDL_COOKIE));
> +
> + mhi_soc_reset(mhi_cntrl);
> +
> + mhi_cntrl->runtime_put(mhi_cntrl);
> + mhi_device_put(mhi_cntrl->mhi_dev);
> +
> + return 0;
> +}
> +
> 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;
> @@ -962,6 +1009,9 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> mhi_cntrl->runtime_put = mhi_pci_runtime_put;
> mhi_cntrl->mru = info->mru_default;
>
> + if (info->edl_trigger & MHI_PCI_GENERIC_EDL_TRIGGER)
> + mhi_cntrl->edl_trigger = mhi_pci_generic_edl_trigger;
> +
> if (info->sideband_wake) {
> mhi_cntrl->wake_get = mhi_pci_wake_get_nop;
> mhi_cntrl->wake_put = mhi_pci_wake_put_nop;
On 4/11/2024 10:46 PM, Jeffrey Hugo wrote:
> On 4/10/2024 9:15 PM, Qiang Yu wrote:
>> Add mhi_pci_generic_edl_trigger as edl_trigger for some devices (eg.
>> SDX65)
>> to enter EDL mode by writing the 0xEDEDEDED cookie to the channel 91
>> doorbell register and forcing an SOC reset afterwards.
>>
>> Signed-off-by: Qiang Yu <[email protected]>
>> ---
>> drivers/bus/mhi/host/pci_generic.c | 50
>> ++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 50 insertions(+)
>>
>> diff --git a/drivers/bus/mhi/host/pci_generic.c
>> b/drivers/bus/mhi/host/pci_generic.c
>> index 51639bf..a529815 100644
>> --- a/drivers/bus/mhi/host/pci_generic.c
>> +++ b/drivers/bus/mhi/host/pci_generic.c
>> @@ -27,12 +27,23 @@
>> #define PCI_VENDOR_ID_THALES 0x1269
>> #define PCI_VENDOR_ID_QUECTEL 0x1eac
>> +#define MHI_EDL_DB 91
>> +#define MHI_EDL_COOKIE 0xEDEDEDED
>> +
>> +/* Device can enter EDL by first setting edl cookie then issuing
>> inband reset*/
>> +#define MHI_PCI_GENERIC_EDL_TRIGGER BIT(0)
>> +
>> +#define CHDBOFF 0x18
>
> This is already in drivers/bus/mhi/common.h why duplicate it here?
I only see common.h be included in ep/internal.h host/internal.h and
host/trace.h. So I thought it can only be used by MHI stack. Can we
include common.h in pci_generic.c?
>
>> +#define CHDBOFF_CHDBOFF_MASK 0xFFFFFFFF
>> +#define CHDBOFF_CHDBOFF_SHIFT 0
>> +
>> /**
>> * struct mhi_pci_dev_info - MHI PCI device specific information
>> * @config: MHI controller configuration
>> * @name: name of the PCI module
>> * @fw: firmware path (if any)
>> * @edl: emergency download mode firmware path (if any)
>> + * @edl_trigger: each bit represents a different way to enter EDL
>> * @bar_num: PCI base address register to use for MHI MMIO register
>> space
>> * @dma_data_width: DMA transfer word size (32 or 64 bits)
>> * @mru_default: default MRU size for MBIM network packets
>> @@ -44,6 +55,7 @@ struct mhi_pci_dev_info {
>> const char *name;
>> const char *fw;
>> const char *edl;
>> + unsigned int edl_trigger;
>> unsigned int bar_num;
>> unsigned int dma_data_width;
>> unsigned int mru_default;
>> @@ -292,6 +304,7 @@ static const struct mhi_pci_dev_info
>> mhi_qcom_sdx75_info = {
>> .name = "qcom-sdx75m",
>> .fw = "qcom/sdx75m/xbl.elf",
>> .edl = "qcom/sdx75m/edl.mbn",
>> + .edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
>> .config = &modem_qcom_v2_mhiv_config,
>> .bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>> .dma_data_width = 32,
>> @@ -302,6 +315,7 @@ static const struct mhi_pci_dev_info
>> mhi_qcom_sdx65_info = {
>> .name = "qcom-sdx65m",
>> .fw = "qcom/sdx65m/xbl.elf",
>> .edl = "qcom/sdx65m/edl.mbn",
>> + .edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
>> .config = &modem_qcom_v1_mhiv_config,
>> .bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>> .dma_data_width = 32,
>> @@ -312,6 +326,7 @@ static const struct mhi_pci_dev_info
>> mhi_qcom_sdx55_info = {
>> .name = "qcom-sdx55m",
>> .fw = "qcom/sdx55m/sbl1.mbn",
>> .edl = "qcom/sdx55m/edl.mbn",
>> + .edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
>> .config = &modem_qcom_v1_mhiv_config,
>> .bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>> .dma_data_width = 32,
>> @@ -928,6 +943,38 @@ static void health_check(struct timer_list *t)
>> mod_timer(&mhi_pdev->health_check_timer, jiffies +
>> HEALTH_CHECK_PERIOD);
>> }
>> +static int mhi_pci_generic_edl_trigger(struct mhi_controller
>> *mhi_cntrl)
>> +{
>> + int ret;
>> + u32 val;
>> + void __iomem *edl_db;
>> + void __iomem *base = mhi_cntrl->regs;
>
> It looks like this file follows reverse christmas tree, but this
> function does not. you should fix it.
Will fix it in next version patch.
>
>> +
>> + ret = mhi_device_get_sync(mhi_cntrl->mhi_dev);
>> + if (ret) {
>> + dev_err(mhi_cntrl->cntrl_dev, "Wake up device fail before
>> trigger EDL\n");
>> + return ret;
>> + }
>> +
>> + pm_wakeup_event(&mhi_cntrl->mhi_dev->dev, 0);
>> + mhi_cntrl->runtime_get(mhi_cntrl);
>> +
>> + mhi_cntrl->read_reg(mhi_cntrl, base + CHDBOFF, &val);
>> + val = (val & CHDBOFF_CHDBOFF_MASK) >> CHDBOFF_CHDBOFF_SHIFT;
>> +
>> + edl_db = base + val + (8 * MHI_EDL_DB);
>> +
>> + mhi_cntrl->write_reg(mhi_cntrl, edl_db + 4,
>> upper_32_bits(MHI_EDL_COOKIE));
>> + mhi_cntrl->write_reg(mhi_cntrl, edl_db,
>> lower_32_bits(MHI_EDL_COOKIE));
>> +
>> + mhi_soc_reset(mhi_cntrl);
>> +
>> + mhi_cntrl->runtime_put(mhi_cntrl);
>> + mhi_device_put(mhi_cntrl->mhi_dev);
>> +
>> + return 0;
>> +}
>> +
>> 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;
>> @@ -962,6 +1009,9 @@ static int mhi_pci_probe(struct pci_dev *pdev,
>> const struct pci_device_id *id)
>> mhi_cntrl->runtime_put = mhi_pci_runtime_put;
>> mhi_cntrl->mru = info->mru_default;
>> + if (info->edl_trigger & MHI_PCI_GENERIC_EDL_TRIGGER)
>> + mhi_cntrl->edl_trigger = mhi_pci_generic_edl_trigger;
>> +
>> if (info->sideband_wake) {
>> mhi_cntrl->wake_get = mhi_pci_wake_get_nop;
>> mhi_cntrl->wake_put = mhi_pci_wake_put_nop;
>
On 4/12/2024 1:13 AM, Qiang Yu wrote:
>
> On 4/11/2024 10:46 PM, Jeffrey Hugo wrote:
>> On 4/10/2024 9:15 PM, Qiang Yu wrote:
>>> Add mhi_pci_generic_edl_trigger as edl_trigger for some devices (eg.
>>> SDX65)
>>> to enter EDL mode by writing the 0xEDEDEDED cookie to the channel 91
>>> doorbell register and forcing an SOC reset afterwards.
>>>
>>> Signed-off-by: Qiang Yu <[email protected]>
>>> ---
>>> drivers/bus/mhi/host/pci_generic.c | 50
>>> ++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 50 insertions(+)
>>>
>>> diff --git a/drivers/bus/mhi/host/pci_generic.c
>>> b/drivers/bus/mhi/host/pci_generic.c
>>> index 51639bf..a529815 100644
>>> --- a/drivers/bus/mhi/host/pci_generic.c
>>> +++ b/drivers/bus/mhi/host/pci_generic.c
>>> @@ -27,12 +27,23 @@
>>> #define PCI_VENDOR_ID_THALES 0x1269
>>> #define PCI_VENDOR_ID_QUECTEL 0x1eac
>>> +#define MHI_EDL_DB 91
>>> +#define MHI_EDL_COOKIE 0xEDEDEDED
>>> +
>>> +/* Device can enter EDL by first setting edl cookie then issuing
>>> inband reset*/
>>> +#define MHI_PCI_GENERIC_EDL_TRIGGER BIT(0)
>>> +
>>> +#define CHDBOFF 0x18
>>
>> This is already in drivers/bus/mhi/common.h why duplicate it here?
>
> I only see common.h be included in ep/internal.h host/internal.h and
> host/trace.h. So I thought it can only be used by MHI stack. Can we
> include common.h in pci_generic.c?
Up to Mani, but duplicating the definition seems like it would result in
maintence overhead over time. An alternative to including the header
might be a new API between MHI and controller which allow the setting of
a CHDB to a specific value.
>>
>>> +#define CHDBOFF_CHDBOFF_MASK 0xFFFFFFFF
>>> +#define CHDBOFF_CHDBOFF_SHIFT 0
>>> +
>>> /**
>>> * struct mhi_pci_dev_info - MHI PCI device specific information
>>> * @config: MHI controller configuration
>>> * @name: name of the PCI module
>>> * @fw: firmware path (if any)
>>> * @edl: emergency download mode firmware path (if any)
>>> + * @edl_trigger: each bit represents a different way to enter EDL
>>> * @bar_num: PCI base address register to use for MHI MMIO register
>>> space
>>> * @dma_data_width: DMA transfer word size (32 or 64 bits)
>>> * @mru_default: default MRU size for MBIM network packets
>>> @@ -44,6 +55,7 @@ struct mhi_pci_dev_info {
>>> const char *name;
>>> const char *fw;
>>> const char *edl;
>>> + unsigned int edl_trigger;
>>> unsigned int bar_num;
>>> unsigned int dma_data_width;
>>> unsigned int mru_default;
>>> @@ -292,6 +304,7 @@ static const struct mhi_pci_dev_info
>>> mhi_qcom_sdx75_info = {
>>> .name = "qcom-sdx75m",
>>> .fw = "qcom/sdx75m/xbl.elf",
>>> .edl = "qcom/sdx75m/edl.mbn",
>>> + .edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
>>> .config = &modem_qcom_v2_mhiv_config,
>>> .bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>>> .dma_data_width = 32,
>>> @@ -302,6 +315,7 @@ static const struct mhi_pci_dev_info
>>> mhi_qcom_sdx65_info = {
>>> .name = "qcom-sdx65m",
>>> .fw = "qcom/sdx65m/xbl.elf",
>>> .edl = "qcom/sdx65m/edl.mbn",
>>> + .edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
>>> .config = &modem_qcom_v1_mhiv_config,
>>> .bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>>> .dma_data_width = 32,
>>> @@ -312,6 +326,7 @@ static const struct mhi_pci_dev_info
>>> mhi_qcom_sdx55_info = {
>>> .name = "qcom-sdx55m",
>>> .fw = "qcom/sdx55m/sbl1.mbn",
>>> .edl = "qcom/sdx55m/edl.mbn",
>>> + .edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
>>> .config = &modem_qcom_v1_mhiv_config,
>>> .bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>>> .dma_data_width = 32,
>>> @@ -928,6 +943,38 @@ static void health_check(struct timer_list *t)
>>> mod_timer(&mhi_pdev->health_check_timer, jiffies +
>>> HEALTH_CHECK_PERIOD);
>>> }
>>> +static int mhi_pci_generic_edl_trigger(struct mhi_controller
>>> *mhi_cntrl)
>>> +{
>>> + int ret;
>>> + u32 val;
>>> + void __iomem *edl_db;
>>> + void __iomem *base = mhi_cntrl->regs;
>>
>> It looks like this file follows reverse christmas tree, but this
>> function does not. you should fix it.
>
> Will fix it in next version patch.
>>
>>> +
>>> + ret = mhi_device_get_sync(mhi_cntrl->mhi_dev);
>>> + if (ret) {
>>> + dev_err(mhi_cntrl->cntrl_dev, "Wake up device fail before
>>> trigger EDL\n");
>>> + return ret;
>>> + }
>>> +
>>> + pm_wakeup_event(&mhi_cntrl->mhi_dev->dev, 0);
>>> + mhi_cntrl->runtime_get(mhi_cntrl);
>>> +
>>> + mhi_cntrl->read_reg(mhi_cntrl, base + CHDBOFF, &val);
>>> + val = (val & CHDBOFF_CHDBOFF_MASK) >> CHDBOFF_CHDBOFF_SHIFT;
>>> +
>>> + edl_db = base + val + (8 * MHI_EDL_DB);
>>> +
>>> + mhi_cntrl->write_reg(mhi_cntrl, edl_db + 4,
>>> upper_32_bits(MHI_EDL_COOKIE));
>>> + mhi_cntrl->write_reg(mhi_cntrl, edl_db,
>>> lower_32_bits(MHI_EDL_COOKIE));
>>> +
>>> + mhi_soc_reset(mhi_cntrl);
>>> +
>>> + mhi_cntrl->runtime_put(mhi_cntrl);
>>> + mhi_device_put(mhi_cntrl->mhi_dev);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> 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;
>>> @@ -962,6 +1009,9 @@ static int mhi_pci_probe(struct pci_dev *pdev,
>>> const struct pci_device_id *id)
>>> mhi_cntrl->runtime_put = mhi_pci_runtime_put;
>>> mhi_cntrl->mru = info->mru_default;
>>> + if (info->edl_trigger & MHI_PCI_GENERIC_EDL_TRIGGER)
>>> + mhi_cntrl->edl_trigger = mhi_pci_generic_edl_trigger;
>>> +
>>> if (info->sideband_wake) {
>>> mhi_cntrl->wake_get = mhi_pci_wake_get_nop;
>>> mhi_cntrl->wake_put = mhi_pci_wake_put_nop;
>>
On Fri, Apr 12, 2024 at 08:16:52AM -0600, Jeffrey Hugo wrote:
> On 4/12/2024 1:13 AM, Qiang Yu wrote:
> >
> > On 4/11/2024 10:46 PM, Jeffrey Hugo wrote:
> > > On 4/10/2024 9:15 PM, Qiang Yu wrote:
> > > > Add mhi_pci_generic_edl_trigger as edl_trigger for some devices
> > > > (eg. SDX65)
> > > > to enter EDL mode by writing the 0xEDEDEDED cookie to the channel 91
> > > > doorbell register and forcing an SOC reset afterwards.
> > > >
> > > > Signed-off-by: Qiang Yu <[email protected]>
> > > > ---
> > > > drivers/bus/mhi/host/pci_generic.c | 50
> > > > ++++++++++++++++++++++++++++++++++++++
> > > > 1 file changed, 50 insertions(+)
> > > >
> > > > diff --git a/drivers/bus/mhi/host/pci_generic.c
> > > > b/drivers/bus/mhi/host/pci_generic.c
> > > > index 51639bf..a529815 100644
> > > > --- a/drivers/bus/mhi/host/pci_generic.c
> > > > +++ b/drivers/bus/mhi/host/pci_generic.c
> > > > @@ -27,12 +27,23 @@
> > > > #define PCI_VENDOR_ID_THALES 0x1269
> > > > #define PCI_VENDOR_ID_QUECTEL 0x1eac
> > > > +#define MHI_EDL_DB 91
> > > > +#define MHI_EDL_COOKIE 0xEDEDEDED
> > > > +
> > > > +/* Device can enter EDL by first setting edl cookie then
> > > > issuing inband reset*/
> > > > +#define MHI_PCI_GENERIC_EDL_TRIGGER BIT(0)
> > > > +
> > > > +#define CHDBOFF 0x18
> > >
> > > This is already in drivers/bus/mhi/common.h why duplicate it here?
> >
> > I only see common.h be included in ep/internal.h host/internal.h and
> > host/trace.h. So I thought it can only be used by MHI stack. Can we
> > include common.h in pci_generic.c?
>
> Up to Mani, but duplicating the definition seems like it would result in
> maintence overhead over time. An alternative to including the header might
> be a new API between MHI and controller which allow the setting of a CHDB to
> a specific value.
>
+1 to the new API suggestion.
- Mani
> > >
> > > > +#define CHDBOFF_CHDBOFF_MASK 0xFFFFFFFF
> > > > +#define CHDBOFF_CHDBOFF_SHIFT 0
> > > > +
> > > > /**
> > > > * struct mhi_pci_dev_info - MHI PCI device specific information
> > > > * @config: MHI controller configuration
> > > > * @name: name of the PCI module
> > > > * @fw: firmware path (if any)
> > > > * @edl: emergency download mode firmware path (if any)
> > > > + * @edl_trigger: each bit represents a different way to enter EDL
> > > > * @bar_num: PCI base address register to use for MHI MMIO
> > > > register space
> > > > * @dma_data_width: DMA transfer word size (32 or 64 bits)
> > > > * @mru_default: default MRU size for MBIM network packets
> > > > @@ -44,6 +55,7 @@ struct mhi_pci_dev_info {
> > > > const char *name;
> > > > const char *fw;
> > > > const char *edl;
> > > > + unsigned int edl_trigger;
> > > > unsigned int bar_num;
> > > > unsigned int dma_data_width;
> > > > unsigned int mru_default;
> > > > @@ -292,6 +304,7 @@ static const struct mhi_pci_dev_info
> > > > mhi_qcom_sdx75_info = {
> > > > .name = "qcom-sdx75m",
> > > > .fw = "qcom/sdx75m/xbl.elf",
> > > > .edl = "qcom/sdx75m/edl.mbn",
> > > > + .edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
> > > > .config = &modem_qcom_v2_mhiv_config,
> > > > .bar_num = MHI_PCI_DEFAULT_BAR_NUM,
> > > > .dma_data_width = 32,
> > > > @@ -302,6 +315,7 @@ static const struct mhi_pci_dev_info
> > > > mhi_qcom_sdx65_info = {
> > > > .name = "qcom-sdx65m",
> > > > .fw = "qcom/sdx65m/xbl.elf",
> > > > .edl = "qcom/sdx65m/edl.mbn",
> > > > + .edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
> > > > .config = &modem_qcom_v1_mhiv_config,
> > > > .bar_num = MHI_PCI_DEFAULT_BAR_NUM,
> > > > .dma_data_width = 32,
> > > > @@ -312,6 +326,7 @@ static const struct mhi_pci_dev_info
> > > > mhi_qcom_sdx55_info = {
> > > > .name = "qcom-sdx55m",
> > > > .fw = "qcom/sdx55m/sbl1.mbn",
> > > > .edl = "qcom/sdx55m/edl.mbn",
> > > > + .edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
> > > > .config = &modem_qcom_v1_mhiv_config,
> > > > .bar_num = MHI_PCI_DEFAULT_BAR_NUM,
> > > > .dma_data_width = 32,
> > > > @@ -928,6 +943,38 @@ static void health_check(struct timer_list *t)
> > > > mod_timer(&mhi_pdev->health_check_timer, jiffies +
> > > > HEALTH_CHECK_PERIOD);
> > > > }
> > > > +static int mhi_pci_generic_edl_trigger(struct mhi_controller
> > > > *mhi_cntrl)
> > > > +{
> > > > + int ret;
> > > > + u32 val;
> > > > + void __iomem *edl_db;
> > > > + void __iomem *base = mhi_cntrl->regs;
> > >
> > > It looks like this file follows reverse christmas tree, but this
> > > function does not. you should fix it.
> >
> > Will fix it in next version patch.
> > >
> > > > +
> > > > + ret = mhi_device_get_sync(mhi_cntrl->mhi_dev);
> > > > + if (ret) {
> > > > + dev_err(mhi_cntrl->cntrl_dev, "Wake up device fail
> > > > before trigger EDL\n");
> > > > + return ret;
> > > > + }
> > > > +
> > > > + pm_wakeup_event(&mhi_cntrl->mhi_dev->dev, 0);
> > > > + mhi_cntrl->runtime_get(mhi_cntrl);
> > > > +
> > > > + mhi_cntrl->read_reg(mhi_cntrl, base + CHDBOFF, &val);
> > > > + val = (val & CHDBOFF_CHDBOFF_MASK) >> CHDBOFF_CHDBOFF_SHIFT;
> > > > +
> > > > + edl_db = base + val + (8 * MHI_EDL_DB);
> > > > +
> > > > + mhi_cntrl->write_reg(mhi_cntrl, edl_db + 4,
> > > > upper_32_bits(MHI_EDL_COOKIE));
> > > > + mhi_cntrl->write_reg(mhi_cntrl, edl_db,
> > > > lower_32_bits(MHI_EDL_COOKIE));
> > > > +
> > > > + mhi_soc_reset(mhi_cntrl);
> > > > +
> > > > + mhi_cntrl->runtime_put(mhi_cntrl);
> > > > + mhi_device_put(mhi_cntrl->mhi_dev);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > 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;
> > > > @@ -962,6 +1009,9 @@ static int mhi_pci_probe(struct pci_dev
> > > > *pdev, const struct pci_device_id *id)
> > > > mhi_cntrl->runtime_put = mhi_pci_runtime_put;
> > > > mhi_cntrl->mru = info->mru_default;
> > > > + if (info->edl_trigger & MHI_PCI_GENERIC_EDL_TRIGGER)
> > > > + mhi_cntrl->edl_trigger = mhi_pci_generic_edl_trigger;
> > > > +
> > > > if (info->sideband_wake) {
> > > > mhi_cntrl->wake_get = mhi_pci_wake_get_nop;
> > > > mhi_cntrl->wake_put = mhi_pci_wake_put_nop;
> > >
>
>
--
மணிவண்ணன் சதாசிவம்
On 4/13/2024 1:09 AM, Manivannan Sadhasivam wrote:
> On Fri, Apr 12, 2024 at 08:16:52AM -0600, Jeffrey Hugo wrote:
>> On 4/12/2024 1:13 AM, Qiang Yu wrote:
>>> On 4/11/2024 10:46 PM, Jeffrey Hugo wrote:
>>>> On 4/10/2024 9:15 PM, Qiang Yu wrote:
>>>>> Add mhi_pci_generic_edl_trigger as edl_trigger for some devices
>>>>> (eg. SDX65)
>>>>> to enter EDL mode by writing the 0xEDEDEDED cookie to the channel 91
>>>>> doorbell register and forcing an SOC reset afterwards.
>>>>>
>>>>> Signed-off-by: Qiang Yu <[email protected]>
>>>>> ---
>>>>> drivers/bus/mhi/host/pci_generic.c | 50
>>>>> ++++++++++++++++++++++++++++++++++++++
>>>>> 1 file changed, 50 insertions(+)
>>>>>
>>>>> diff --git a/drivers/bus/mhi/host/pci_generic.c
>>>>> b/drivers/bus/mhi/host/pci_generic.c
>>>>> index 51639bf..a529815 100644
>>>>> --- a/drivers/bus/mhi/host/pci_generic.c
>>>>> +++ b/drivers/bus/mhi/host/pci_generic.c
>>>>> @@ -27,12 +27,23 @@
>>>>> #define PCI_VENDOR_ID_THALES 0x1269
>>>>> #define PCI_VENDOR_ID_QUECTEL 0x1eac
>>>>> +#define MHI_EDL_DB 91
>>>>> +#define MHI_EDL_COOKIE 0xEDEDEDED
>>>>> +
>>>>> +/* Device can enter EDL by first setting edl cookie then
>>>>> issuing inband reset*/
>>>>> +#define MHI_PCI_GENERIC_EDL_TRIGGER BIT(0)
>>>>> +
>>>>> +#define CHDBOFF 0x18
>>>> This is already in drivers/bus/mhi/common.h why duplicate it here?
>>> I only see common.h be included in ep/internal.h host/internal.h and
>>> host/trace.h. So I thought it can only be used by MHI stack. Can we
>>> include common.h in pci_generic.c?
>> Up to Mani, but duplicating the definition seems like it would result in
>> maintence overhead over time. An alternative to including the header might
>> be a new API between MHI and controller which allow the setting of a CHDB to
>> a specific value.
>>
> +1 to the new API suggestion.
>
> - Mani
OK, will add a new API in MHI to allow controller get CHDB address.
>>>>> +#define CHDBOFF_CHDBOFF_MASK 0xFFFFFFFF
>>>>> +#define CHDBOFF_CHDBOFF_SHIFT 0
>>>>> +
>>>>> /**
>>>>> * struct mhi_pci_dev_info - MHI PCI device specific information
>>>>> * @config: MHI controller configuration
>>>>> * @name: name of the PCI module
>>>>> * @fw: firmware path (if any)
>>>>> * @edl: emergency download mode firmware path (if any)
>>>>> + * @edl_trigger: each bit represents a different way to enter EDL
>>>>> * @bar_num: PCI base address register to use for MHI MMIO
>>>>> register space
>>>>> * @dma_data_width: DMA transfer word size (32 or 64 bits)
>>>>> * @mru_default: default MRU size for MBIM network packets
>>>>> @@ -44,6 +55,7 @@ struct mhi_pci_dev_info {
>>>>> const char *name;
>>>>> const char *fw;
>>>>> const char *edl;
>>>>> + unsigned int edl_trigger;
>>>>> unsigned int bar_num;
>>>>> unsigned int dma_data_width;
>>>>> unsigned int mru_default;
>>>>> @@ -292,6 +304,7 @@ static const struct mhi_pci_dev_info
>>>>> mhi_qcom_sdx75_info = {
>>>>> .name = "qcom-sdx75m",
>>>>> .fw = "qcom/sdx75m/xbl.elf",
>>>>> .edl = "qcom/sdx75m/edl.mbn",
>>>>> + .edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
>>>>> .config = &modem_qcom_v2_mhiv_config,
>>>>> .bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>>>>> .dma_data_width = 32,
>>>>> @@ -302,6 +315,7 @@ static const struct mhi_pci_dev_info
>>>>> mhi_qcom_sdx65_info = {
>>>>> .name = "qcom-sdx65m",
>>>>> .fw = "qcom/sdx65m/xbl.elf",
>>>>> .edl = "qcom/sdx65m/edl.mbn",
>>>>> + .edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
>>>>> .config = &modem_qcom_v1_mhiv_config,
>>>>> .bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>>>>> .dma_data_width = 32,
>>>>> @@ -312,6 +326,7 @@ static const struct mhi_pci_dev_info
>>>>> mhi_qcom_sdx55_info = {
>>>>> .name = "qcom-sdx55m",
>>>>> .fw = "qcom/sdx55m/sbl1.mbn",
>>>>> .edl = "qcom/sdx55m/edl.mbn",
>>>>> + .edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
>>>>> .config = &modem_qcom_v1_mhiv_config,
>>>>> .bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>>>>> .dma_data_width = 32,
>>>>> @@ -928,6 +943,38 @@ static void health_check(struct timer_list *t)
>>>>> mod_timer(&mhi_pdev->health_check_timer, jiffies +
>>>>> HEALTH_CHECK_PERIOD);
>>>>> }
>>>>> +static int mhi_pci_generic_edl_trigger(struct mhi_controller
>>>>> *mhi_cntrl)
>>>>> +{
>>>>> + int ret;
>>>>> + u32 val;
>>>>> + void __iomem *edl_db;
>>>>> + void __iomem *base = mhi_cntrl->regs;
>>>> It looks like this file follows reverse christmas tree, but this
>>>> function does not. you should fix it.
>>> Will fix it in next version patch.
>>>>> +
>>>>> + ret = mhi_device_get_sync(mhi_cntrl->mhi_dev);
>>>>> + if (ret) {
>>>>> + dev_err(mhi_cntrl->cntrl_dev, "Wake up device fail
>>>>> before trigger EDL\n");
>>>>> + return ret;
>>>>> + }
>>>>> +
>>>>> + pm_wakeup_event(&mhi_cntrl->mhi_dev->dev, 0);
>>>>> + mhi_cntrl->runtime_get(mhi_cntrl);
>>>>> +
>>>>> + mhi_cntrl->read_reg(mhi_cntrl, base + CHDBOFF, &val);
>>>>> + val = (val & CHDBOFF_CHDBOFF_MASK) >> CHDBOFF_CHDBOFF_SHIFT;
>>>>> +
>>>>> + edl_db = base + val + (8 * MHI_EDL_DB);
>>>>> +
>>>>> + mhi_cntrl->write_reg(mhi_cntrl, edl_db + 4,
>>>>> upper_32_bits(MHI_EDL_COOKIE));
>>>>> + mhi_cntrl->write_reg(mhi_cntrl, edl_db,
>>>>> lower_32_bits(MHI_EDL_COOKIE));
>>>>> +
>>>>> + mhi_soc_reset(mhi_cntrl);
>>>>> +
>>>>> + mhi_cntrl->runtime_put(mhi_cntrl);
>>>>> + mhi_device_put(mhi_cntrl->mhi_dev);
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> 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;
>>>>> @@ -962,6 +1009,9 @@ static int mhi_pci_probe(struct pci_dev
>>>>> *pdev, const struct pci_device_id *id)
>>>>> mhi_cntrl->runtime_put = mhi_pci_runtime_put;
>>>>> mhi_cntrl->mru = info->mru_default;
>>>>> + if (info->edl_trigger & MHI_PCI_GENERIC_EDL_TRIGGER)
>>>>> + mhi_cntrl->edl_trigger = mhi_pci_generic_edl_trigger;
>>>>> +
>>>>> if (info->sideband_wake) {
>>>>> mhi_cntrl->wake_get = mhi_pci_wake_get_nop;
>>>>> mhi_cntrl->wake_put = mhi_pci_wake_put_nop;
>>