Received-SPF: pass (google.com: domain of linux-kernel+bounces-147873-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <ab27b383-dcbf-4337-b3ea-da91763e834a@quicinc.com>
Date: Wed, 17 Apr 2024 11:01:41 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 3/3] bus: mhi: host: pci_generic: Add edl callback to
 enter EDL
To: Mayank Rana <quic_mrana@quicinc.com>, <mani@kernel.org>,
        <quic_jhugo@quicinc.com>
CC: <mhi@lists.linux.dev>, <linux-arm-msm@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <quic_cang@quicinc.com>
References: <1713170945-44640-1-git-send-email-quic_qianyu@quicinc.com>
 <1713170945-44640-4-git-send-email-quic_qianyu@quicinc.com>
 <17d94b91-137c-409c-8af3-f32f1af2eb71@quicinc.com>
 <4b684db2-d384-404a-9c54-60d79ac7cf9f@quicinc.com>
 <adb9ab3d-0fd2-4afe-96d7-573b1822e0c3@quicinc.com>
Content-Language: en-US
From: Qiang Yu <quic_qianyu@quicinc.com>
In-Reply-To: <adb9ab3d-0fd2-4afe-96d7-573b1822e0c3@quicinc.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit


On 4/17/2024 2:12 AM, Mayank Rana wrote:
>
>
> On 4/15/2024 8:50 PM, Qiang Yu wrote:
>>
>> On 4/16/2024 7:53 AM, Mayank Rana wrote:
>>> Hi Qiang
>>>
>>> On 4/15/2024 1:49 AM, 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 | 47 
>>>> ++++++++++++++++++++++++++++++++++++++
>>>>   1 file changed, 47 insertions(+)
>>>>
>>>> diff --git a/drivers/bus/mhi/host/pci_generic.c 
>>>> b/drivers/bus/mhi/host/pci_generic.c
>>>> index 51639bf..cbf8a58 100644
>>>> --- a/drivers/bus/mhi/host/pci_generic.c
>>>> +++ b/drivers/bus/mhi/host/pci_generic.c
>>>> @@ -27,12 +27,19 @@
>>>>   #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)
>>>> +
>>>>   /**
>>>>    * 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 +51,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 +300,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 +311,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 +322,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 +939,39 @@ 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)
>>>> +{
>>>> +    void __iomem *base = mhi_cntrl->regs;
>>>> +    void __iomem *edl_db;
>>>> +    int ret;
>>>> +    u32 val;
>>>> +
>>>> +    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);
>>>> +
>>>> +    ret = mhi_get_channel_doorbell(mhi_cntrl, &val);
>>>> +    if (ret)
>>>> +        return ret;
>>> Don't we need error handling part here i.e. calling 
>>> mhi_cntrl->runtime_put() as well mhi_device_put() ?
>>
>> Hi Mayank
>>
>> After soc_reset, device will reboot to EDL mode and MHI state will be 
>> SYSERR. So host will fail to suspend
>> anyway. The "error handling" we need here is actually to enter EDL 
>> mode, this will be done by SYSERR irq.
>> Here, mhi_cntrl->runtime_put() and mhi_device_put() are only to 
>> balance mhi_cntrl->runtime_get() and
>> mhi_device_get_sync().
>>
>> Thanks,
>> Qiang
> I am saying is there possibility that mhi_get_channel_doorbell() 
> returns error ?
> If yes, then why don't we need error handling as part of it. you are 
> exiting if this API return error without doing anything.

I think here mhi_get_channel_doorbell will not return error. But I still
add a error checking because it invoked mhi_read_reg, which is a must check
API. For modem mhi controller, this API eventually does a memory read.
This memory read will return a normal value if link is up and all f's if 
link
is bad.

Thanks,
Qiang
>>>> +    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 +1006,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;
>>> Regards,
>>> Mayank