Message-ID: <fb58c20e-ad2c-5398-ad69-8e01de58fc02@linaro.org>
Date:   Thu, 18 May 2023 14:54:16 +0200
From:   Konrad Dybcio <konrad.dybcio@...aro.org>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Stephan Gerhold <stephan@...hold.net>
Cc:     Rob Herring <robh@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Andy Gross <agross@...nel.org>, linux-kernel@...r.kernel.org,
        Shawn Guo <shawn.guo@...aro.org>,
        Marijn Suijten <marijn.suijten@...ainline.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Marc Zyngier <maz@...nel.org>, linux-arm-msm@...r.kernel.org,
        devicetree@...r.kernel.org
Subject: Re: [PATCH v2 1/2] dt-bindings: interrupt-controller: mpm: Pass MSG
 RAM slice through phandle



On 13.04.2023 10:50, Krzysztof Kozlowski wrote:
> On 12/04/2023 19:06, Konrad Dybcio wrote:
>>
>>
>> On 12.04.2023 18:53, Krzysztof Kozlowski wrote:
>>> On 12/04/2023 14:09, Konrad Dybcio wrote:
>>>>
>>>>
>>>> On 12.04.2023 13:55, Krzysztof Kozlowski wrote:
>>>>> On 12/04/2023 13:47, Konrad Dybcio wrote:
>>>>>>> For unrelated reasons I actually have some patches for this, that switch
>>>>>>> the /smd top-level node to a "remoteproc-like" node dedicated to the
>>>>>>> RPM, similar to how WCNSS/ADSP/Modem/etc are represented. I need this to
>>>>>>> add additional (optional) properties like "resets" and "iommus" for the
>>>>>>> RPM, but it would allow adding arbitrary subnodes as well:
>>>>>>>
>>>>>>> https://github.com/msm8916-mainline/linux/commit/35231ac28703805daa8220f1233847c7df34589e
>>>>>>>
>>>>>>> I could finish those up and post them if that would help...
>>>>>> Krzysztof, what do you think?
>>>>>
>>>>> I don't know what is there in MSM8916 and how it should be represented.
>>>> Similarly to other Qualcomm SoCs, MSM8916 has a RPM (Cortex-M3) core,
>>>> which communicates over the SMD protocol (or G-LINK on >=8996).
>>>>
>>>> The Qualcomm firmware loads the RPM fw blob and sets it up early in
>>>> the boot process, but msm8916-mainline folks managed to get TF-A
>>>> going and due to it being less.. invasive.. than the Qualcomm TZ,
>>>> RPM needs a bit more handling to be accessible.
>>>>
>>>> The M3 core is wired up through the CNoC bus and we communicate
>>>> with it through the MSG RAM and the "APCS mailbox".
>>>
>>> Thanks, that's actually good description. Yet I still do not know what
>>> is exactly the problem and the question. Linking some out of tree
>>> commits does not give me the answer, at least I cannot get that answer
>>> form the link.
>>>
>>> For example what I don't understand is: why additional resources (like
>>> resets) can be provided only in new binding, but not in the old.
>> The old binding dictates that the rpm node (which in turn
>> holds all "devices" that only interface with RPM, like RPMCC) is
>> a child of smd{}, which does not make sense logically, as SMD is
>> a protocol (e.g. we don't place devices connected over i2c under
>> /i2c{}).
> 
> We do. All devices connected over I2C are under i2c node which is the
> controller. The example is different than what you have here...
> 
>>  The rpm node lacks a compatible, as it's representing
>> an "smd channel", so there's no driver so there's no way to assert
>> resets etc.
> 
> You have rpm-requests which has compatible. These are not its resources?
I believe we misrepresented this 10y ago and now we're stuck with that
legacy..

Currently we have:

[1]
smd {
	rpm {
		rpm-requests {
			compatible = "qcom,rpm-msm8916"

or

[2]
rpm-glink {
	rpm-requests {
		compatible = "qcom,rpm-sm6375"


In the case of [1], 'smd' is a communication protocol and the 'rpm'
node describes the RPM's "smd edge" (think a communication channel
assigned to the RPM processor)


In the case of [2], rpm-glink is also just a description of the G-LINK
communication protocol/"bus" (putting bus in quotes, as GLINK is really a
very very fancy set of mailboxes)

So we've really been describing the protocols and not the hardware buses..

What Stephan and I were trying to say, is that there's no great node that
actually represents the Cortex-M3 RPM core itself.

The rpm-requests node is the closest, but it won't fit his purpose, as it 
depends on the communication with the CM3 already being active - it will
only get registered through qcom_glink_rx_open / qcom_channel_state_worker
for GLINK/SMD respectively. These channels will only be open if the core
is up, but for that to happen its reset line must be deasserted.

Stephen proposed restructuring that to be centered around the CM3 core and
not the communication protocol.

I know you're not very fond of downstream tree commits, but looking at his
branch, I think that's it:

https://github.com/msm8916-mainline/linux/commit/e4e90fd3f711295461ee17891567e75e2342e5c8

I'd be in favour of such restructurization - makes things much more clear
and sane.

Stephen, if you're willing to do it, I can test your patches on both GLINK
and SMD platforms.

> 
>>
>> On newer SoCs that still implement SMD RPM (like 8996), we do
>> actually have a driver and a parent node which it binds to
>> (rpm-glink).
> 
> You want to add RPM resets to rpm-glink node? This also does not look right.
No, I was just pointing out that rpm-requests' direct parent node has a
driver bound to it in case of GLINK but not in the case of SMD


> 
>>
>> AFAIU:
>> In both cases, the "final" drivers (rpmcc, rpmpd..) are bound
>> after hitting a SMD/GLINK callback that tells Linux we're ready
>> to rock. That's an issue for Stephan, as these callbacks won't
>> ever happen if the RPM core is not initialized (and TF-A doesn't
>> do that).
> 
> To me half or almost all of Qualcomm remote-proc-related bindings, like
> SMD, GLINK and associated processors, are difficult to read, half-baked
> and developed to match the current Linux/SW need.
Agreed :/

When the Linux drivers
> changed, new bindings were added... If you want to fix it, sure go
> ahead, but design everything to match something rational, not again to
> match one specific SW/FW implementation.
I don't think it's worth the hassle.. we may add it to the "we'll fix it
when we eventually find some gamebreaking issue that requires us to break
the 10yo backwards compatibility for some deep core driver, if that happens"
list..

Konrad
> 
> Best regards,
> Krzysztof
> 

Powered by blists - more mailing lists