[v2] Add sysfs entry to EDL mode

[PATCH v2 2/2] bus: mhi: host: pci_generic: Add edl callback to enter EDL

Posted by Qiang Yu 1 year, 10 months ago

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 <quic_qianyu@quicinc.com>
---
 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

Re: [PATCH v2 2/2] bus: mhi: host: pci_generic: Add edl callback to enter EDL

Posted by Jeffrey Hugo 1 year, 10 months ago

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 <quic_qianyu@quicinc.com>
> ---
>   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;

Re: [PATCH v2 2/2] bus: mhi: host: pci_generic: Add edl callback to enter EDL

Posted by Qiang Yu 1 year, 10 months ago

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 <quic_qianyu@quicinc.com>
>> ---
>>   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;
>

Re: [PATCH v2 2/2] bus: mhi: host: pci_generic: Add edl callback to enter EDL

Posted by Jeffrey Hugo 1 year, 10 months ago

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 <quic_qianyu@quicinc.com>
>>> ---
>>>   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;
>>

Re: [PATCH v2 2/2] bus: mhi: host: pci_generic: Add edl callback to enter EDL

Posted by Manivannan Sadhasivam 1 year, 10 months ago

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 <quic_qianyu@quicinc.com>
> > > > ---
> > > >   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;
> > > 
> 
> 

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

Re: [PATCH v2 2/2] bus: mhi: host: pci_generic: Add edl callback to enter EDL

Posted by Qiang Yu 1 year, 10 months ago

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 <quic_qianyu@quicinc.com>
>>>>> ---
>>>>>    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;
>>

[PATCH v2 1/2] bus: mhi: host: Add sysfs entry to force device to enter EDL
[PATCH v2 2/2] bus: mhi: host: pci_generic: Add edl callback to enter EDL