From: Sibi Sankar <quic_sibis@quicinc.com>
To: Konrad Dybcio <konrad.dybcio@linaro.org>, <sudeep.holla@arm.com>,
<cristian.marussi@arm.com>
Cc: <linux-kernel@vger.kernel.org>, <linux-arm-msm@vger.kernel.org>,
<devicetree@vger.kernel.org>, <quic_rgottimu@quicinc.com>,
<quic_kshivnan@quicinc.com>
Subject: Re: [PATCH V4 2/5] mailbox: Add support for QTI CPUCP mailbox controller
Date: Tue, 14 May 2024 16:49:06 +0530 [thread overview]
Message-ID: <d113adc9-2192-44af-a5df-7bbaa6907ac8@quicinc.com> (raw)
In-Reply-To: <d5581614-71ad-5f7f-e948-bdbab3ef854e@quicinc.com>
On 4/23/24 22:40, Sibi Sankar wrote:
>
>
> On 4/23/24 04:47, Konrad Dybcio wrote:
>>
>>
>> On 4/22/24 18:40, Sibi Sankar wrote:
>>> Add support for CPUSS Control Processor (CPUCP) mailbox controller,
>>> this driver enables communication between AP and CPUCP by acting as
>>> a doorbell between them.
>>>
>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
>>> ---
>>
>> [...]
>>
>>> +
>>> +static int qcom_cpucp_mbox_send_data(struct mbox_chan *chan, void
>>> *data)
>>> +{
>>> + struct qcom_cpucp_mbox *cpucp = container_of(chan->mbox, struct
>>> qcom_cpucp_mbox, mbox);
>>> + unsigned long chan_id = channel_number(chan);
>>> + u32 *val = data;
>>> +
>>> + writel(*val, cpucp->tx_base + APSS_CPUCP_TX_MBOX_CMD(chan_id) +
>>> APSS_CPUCP_MBOX_CMD_OFF);
>>
>
> Hey Konrad,
>
> Thanks for taking time to review the series.
>
>> Just checking in, is *this access only* supposed to be 32b instead of
>> 64 like others?
>
> yeah, the readl and writely in the driver were used intentionally.
>
>>
>> [...]
>>
>>> +
>>> + writeq(0, cpucp->rx_base + APSS_CPUCP_RX_MBOX_EN);
>>> + writeq(0, cpucp->rx_base + APSS_CPUCP_RX_MBOX_CLEAR);
>>> + writeq(0, cpucp->rx_base + APSS_CPUCP_RX_MBOX_MAP);
>>
>> If these writes are here to prevent a possible interrupt storm type
>> tragedy,
>> you need to read back these registers to ensure the writes have left
>> the CPU
>> complex and reached the observer at the other end of the bus (not to be
>> confused with barriers which only ensure that such accesses are ordered
>> *when still possibly within the CPU complex*).
>
> I couldn't find anything alluding to ^^. This sequence was just
> meant to reset the mailbox. Looks like we do need to preserve the
> ordering so relaxed read/writes aren't an option.
>
> -Sibi
>
>>
>> Moreover, if the order of them arriving (en/clear/mask) doesn't
>> matter, you
>> can add _relaxed for a possible nanosecond-order perf gain
>>
>>> +
>>> + irq = platform_get_irq(pdev, 0);
>>> + if (irq < 0)
>>> + return irq;
>>> +
>>> + ret = devm_request_irq(dev, irq, qcom_cpucp_mbox_irq_fn,
>>> + IRQF_TRIGGER_HIGH, "apss_cpucp_mbox", cpucp);
>>> + if (ret < 0)
>>> + return dev_err_probe(dev, ret, "Failed to register irq:
>>> %d\n", irq);
>>> +
>>> + writeq(APSS_CPUCP_RX_MBOX_CMD_MASK, cpucp->rx_base +
>>> APSS_CPUCP_RX_MBOX_MAP);
>>
>> Similarly here, unless read back, we may potentially miss some
>> interrupts if
>> e.g. a channel is opened and that write "is decided" (by the silicon)
>> to leave
>> the internal buffer first
>
> At this point in time we don't expect any interrupts. They are expected
> only after channel activation. Also there were no recommendations for
> reading it back here as well.
>
> -Sibi
>
>>
>>
>>> +
>>> + mbox = &cpucp->mbox;
>>> + mbox->dev = dev;
>>> + mbox->num_chans = APSS_CPUCP_IPC_CHAN_SUPPORTED;
>>> + mbox->chans = cpucp->chans;
>>> + mbox->ops = &qcom_cpucp_mbox_chan_ops;
>>> + mbox->txdone_irq = false;
>>> + mbox->txdone_poll = false;
>>
>> "false" == 0 is the default value (as you're using k*z*alloc)
>>
>>
>>> +
>>> + ret = devm_mbox_controller_register(dev, mbox);
>>> + if (ret)
>>> + return dev_err_probe(dev, ret, "Failed to create mailbox\n");
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const struct of_device_id qcom_cpucp_mbox_of_match[] = {
>>> + { .compatible = "qcom,x1e80100-cpucp-mbox" },
>>> + {}
>>> +};
>>> +MODULE_DEVICE_TABLE(of, qcom_cpucp_mbox_of_match);
>>> +
>>> +static struct platform_driver qcom_cpucp_mbox_driver = {
>>> + .probe = qcom_cpucp_mbox_probe,
>>> + .driver = {
>>> + .name = "qcom_cpucp_mbox",
>>> + .of_match_table = qcom_cpucp_mbox_of_match,
>>> + },
>>> +};
>>> +module_platform_driver(qcom_cpucp_mbox_driver);
>>
>> That's turbo late. Go core_initcall.
Christian/Sudeep,
Looks like making the cpucp mbox as part of the core initcall and having
the vendor protocol as a module_scmi_driver causes a race as follows:
scmi_core: SCMI protocol bus registered
scmi_core: Requesting SCMI device (clocks) for protocol 14
scmi_core: Registered new scmi driver scmi-clocks
scmi_core: Requesting SCMI device (qcom_scmi_vendor_protocol) for
protocol 80
scmi_core: Registered new scmi driver qcom-scmi-driver
scmi_core: Requesting SCMI device (perf) for protocol 13
scmi_core: Registered new scmi driver scmi-perf-domain
scmi_core: Requesting SCMI device (genpd) for protocol 11
scmi_core: Registered new scmi driver scmi-power-domain
scmi_core: Requesting SCMI device (reset) for protocol 16
scmi_core: Registered new scmi driver scmi-reset
scmi_core: Requesting SCMI device (hwmon) for protocol 15
scmi_core: Registered new scmi driver scmi-hwmon
scmi_core: Requesting SCMI device (cpufreq) for protocol 13
scmi_core: Registered new scmi driver scmi-cpufreq
scmi_module: Registered SCMI Protocol 0x10
scmi_module: Registered SCMI Protocol 0x14
scmi_module: Registered SCMI Protocol 0x13
scmi_module: Registered SCMI Protocol 0x11
scmi_module: Registered SCMI Protocol 0x16
scmi_module: Registered SCMI Protocol 0x15
scmi_module: Registered SCMI Protocol 0x17
scmi_module: Registered SCMI Protocol 0x12
scmi_module: Registered SCMI Protocol 0x18
scmi_module: Registered SCMI Protocol 0x19
scmi_core: (scmi) Created SCMI device 'scmi_dev.1' for protocol 0x10
(__scmi_transport_device_tx_10)
scmi_core: (scmi) Created SCMI device 'scmi_dev.2' for protocol 0x10
(__scmi_transport_device_rx_10)
arm-scmi firmware:scmi: SCMI Notifications - Core Enabled.
scmi_module: Found SCMI Protocol 0x10
arm-scmi firmware:scmi: SCMI Protocol v2.0 'Qualcomm:' Firmware version
0x20000
scmi_module: Found SCMI Protocol 0x13
scmi_core: (scmi) Created SCMI device 'scmi_dev.3' for protocol 0x13
(cpufreq)
scmi-perf-domain scmi_dev.4: Initialized 3 performance domains
scmi_core: (scmi) Created SCMI device 'scmi_dev.4' for protocol 0x13 (perf)
scmi_module: SCMI Protocol 0x80 not found!
scmi_core: (scmi) Created SCMI device 'scmi_dev.5' for protocol 0x80
(qcom_scmi_vendor_protocol)
scmi_module: Registered SCMI Protocol 0x80
By the time the vendor protocol get's registered it's already reported
as not found.
-Sibi
>>
>> Konrad
>
next prev parent reply other threads:[~2024-05-14 11:20 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-22 16:40 [PATCH V4 0/5] qcom: x1e80100: Enable CPUFreq Sibi Sankar
2024-04-22 16:40 ` [PATCH V4 1/5] dt-bindings: mailbox: qcom: Add CPUCP mailbox controller bindings Sibi Sankar
2024-04-22 16:40 ` [PATCH V4 2/5] mailbox: Add support for QTI CPUCP mailbox controller Sibi Sankar
2024-04-22 23:17 ` Konrad Dybcio
2024-04-23 17:10 ` Sibi Sankar
2024-05-14 11:19 ` Sibi Sankar [this message]
2024-05-01 2:14 ` Jassi Brar
2024-05-14 9:36 ` Sibi Sankar
2024-05-03 12:48 ` Cristian Marussi
2024-05-14 10:54 ` Sibi Sankar
2024-04-22 16:40 ` [PATCH V4 3/5] arm64: dts: qcom: x1e80100: Resize GIC Redistributor register region Sibi Sankar
2024-04-22 16:40 ` [PATCH V4 4/5] arm64: dts: qcom: x1e80100: Add cpucp mailbox and sram nodes Sibi Sankar
2024-04-22 16:40 ` [PATCH V4 5/5] arm64: dts: qcom: x1e80100: Enable cpufreq Sibi Sankar
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=d113adc9-2192-44af-a5df-7bbaa6907ac8@quicinc.com \
--to=quic_sibis@quicinc.com \
--cc=cristian.marussi@arm.com \
--cc=devicetree@vger.kernel.org \
--cc=konrad.dybcio@linaro.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=quic_kshivnan@quicinc.com \
--cc=quic_rgottimu@quicinc.com \
--cc=sudeep.holla@arm.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).