From: Qiang Yu <quic_qianyu@quicinc.com>
To: Manivannan Sadhasivam <mani@kernel.org>,
Jeffrey Hugo <quic_jhugo@quicinc.com>
Cc: <mhi@lists.linux.dev>, <linux-arm-msm@vger.kernel.org>,
<linux-kernel@vger.kernel.org>, <quic_cang@quicinc.com>,
<quic_mrana@quicinc.com>
Subject: Re: [PATCH v2 2/2] bus: mhi: host: pci_generic: Add edl callback to enter EDL
Date: Mon, 15 Apr 2024 13:48:54 +0800 [thread overview]
Message-ID: <a43f394e-9f6a-4ec0-aff7-b43de1e3035e@quicinc.com> (raw)
In-Reply-To: <20240412170910.GA21555@thinkpad>
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;
>>
prev parent reply other threads:[~2024-04-15 5:49 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-11 3:15 [PATCH v2 0/2] Add sysfs entry to EDL mode Qiang Yu
2024-04-11 3:15 ` [PATCH v2 1/2] bus: mhi: host: Add sysfs entry to force device to enter EDL Qiang Yu
2024-04-11 14:41 ` Jeffrey Hugo
2024-04-11 3:15 ` [PATCH v2 2/2] bus: mhi: host: pci_generic: Add edl callback " Qiang Yu
2024-04-11 14:46 ` Jeffrey Hugo
2024-04-12 7:13 ` Qiang Yu
2024-04-12 14:16 ` Jeffrey Hugo
2024-04-12 17:09 ` Manivannan Sadhasivam
2024-04-15 5:48 ` Qiang Yu [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a43f394e-9f6a-4ec0-aff7-b43de1e3035e@quicinc.com \
--to=quic_qianyu@quicinc.com \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mani@kernel.org \
--cc=mhi@lists.linux.dev \
--cc=quic_cang@quicinc.com \
--cc=quic_jhugo@quicinc.com \
--cc=quic_mrana@quicinc.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).