| Message ID | 20240117173458.2312669-1-quic_sibis@quicinc.com |
|---|---|
| Headers | show |
| Series | firmware: arm_scmi: Qualcomm Vendor Protocol | expand |
On 1/17/24 18:34, Sibi Sankar wrote: > Add devicetree binding for CPUSS Control Processor (CPUCP) mailbox > controller. > > Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> > --- [...] > + - | > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + > + mailbox@17430000 { > + compatible = "qcom,x1e80100-cpucp-mbox", "qcom,cpucp-mbox"; > + reg = <0x17430000 0x10000>, <0x18830000 0x300>; These reg spaces are quite far apart.. On 7280-8550, a similar mailbox exists, although it's dubbed RIMPS-mbox instead. In that case, I separated the mbox into tx (via qcom-apcs-ipc-mailbox.c) and rx (with a simple driver). Still haven't pushed or posted that anywhere, I'd need to access another machine.. On (some of) these SoCs, one of the channels (rx[1], iirc?) clearly bleeds into the CPUFREQ_HW/OSM register region, which gives an impression of misrepresenting the hardware. X1E doesn't have a node for cpufreq_hw defined, so I can't tell whether it's also the case here. Konrad
On 1/17/24 18:34, Sibi Sankar wrote: > From: Shivnandan Kumar <quic_kshivnan@quicinc.com> > > SCMI QCOM vendor protocol provides interface to communicate with SCMI > controller and enable vendor specific features like bus scaling capable > of running on it. "QCOM protocol" sounds overly generic, especially given how many different vendor protocols have historically been present in QC firmware.. > > Signed-off-by: Shivnandan Kumar <quic_kshivnan@quicinc.com> > Co-developed-by: Ramakrishna Gottimukkula <quic_rgottimu@quicinc.com> > Signed-off-by: Ramakrishna Gottimukkula <quic_rgottimu@quicinc.com> > Co-developed-by: Amir Vajid <avajid@quicinc.com> > Signed-off-by: Amir Vajid <avajid@quicinc.com> > Co-developed-by: Sibi Sankar <quic_sibis@quicinc.com> > Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> > --- So, this is another 0x80 protocol, different to the one that has been shipping on devices that got released with msm-5.4, msm-5.10 and msm-5.15 [1][2]. They're totally incompatible (judging by the msg format), use the same protocol ID and they are (at a glance) providing access to the same HW/FW/tunables. I'm not sure if this can be trusted not to change again.. Unless we get a strong commitment that all platforms (compute, mobile, auto, iot, whatever) stick to this one.. That said, the spec (DEN0056C) says that protocol IDs 0x80-0xff are: "Reserved for vendor or platform-specific extensions to this interface.". So if perhaps there's a will to maintain multiple versions of this, with a way to discern between them.. Konrad [1] https://git.codelinaro.org/clo/la/kernel/msm-5.15/-/blob/KERNEL.PLATFORM.2.1.r5-05400-kernel.0/drivers/firmware/arm_scmi/memlat_vendor.c?ref_type=tags [2] https://git.codelinaro.org/clo/la/kernel/msm-5.15/-/blob/KERNEL.PLATFORM.2.1.r5-05400-kernel.0/include/linux/scmi_memlat.h#L16
On Wed, Jan 17, 2024 at 09:15:40PM +0100, Konrad Dybcio wrote: > > > On 1/17/24 18:34, Sibi Sankar wrote: > > From: Shivnandan Kumar <quic_kshivnan@quicinc.com> > > Hi, a few early remarks, I am gonna look at this better next week. > > SCMI QCOM vendor protocol provides interface to communicate with SCMI > > controller and enable vendor specific features like bus scaling capable > > of running on it. > > "QCOM protocol" sounds overly generic, especially given how many > different vendor protocols have historically been present in > QC firmware.. > I was going to raise the same point :D, usually the name identifies the aim of the protocol (and the vendor also in this case) > > > > Signed-off-by: Shivnandan Kumar <quic_kshivnan@quicinc.com> > > Co-developed-by: Ramakrishna Gottimukkula <quic_rgottimu@quicinc.com> > > Signed-off-by: Ramakrishna Gottimukkula <quic_rgottimu@quicinc.com> > > Co-developed-by: Amir Vajid <avajid@quicinc.com> > > Signed-off-by: Amir Vajid <avajid@quicinc.com> > > Co-developed-by: Sibi Sankar <quic_sibis@quicinc.com> > > Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> > > --- > > So, this is another 0x80 protocol, different to the one that has > been shipping on devices that got released with msm-5.4, msm-5.10 > and msm-5.15 [1][2]. They're totally incompatible (judging by the > msg format), use the same protocol ID and they are (at a glance) > providing access to the same HW/FW/tunables. > > I'm not sure if this can be trusted not to change again.. Unless > we get a strong commitment that all platforms (compute, mobile, > auto, iot, whatever) stick to this one.. > > That said, the spec (DEN0056C) says that protocol IDs 0x80-0xff > are: "Reserved for vendor or platform-specific extensions to > this interface.". So if perhaps there's a will to maintain > multiple versions of this, with a way to discern between them.. > Just recently we had a discussion with some other vendor about this possible clashing of vendor protocols numbers between different vendors/platforms, especially if aiming to just push all in defconfig. The basic idea to solve this, which I am going to post shortly in the next weeks, was to add a way to define and register your protocol number associated with a vendor identifier(s) of some kind, since vendor/subvendor/firmware versions are advertised by the Platform and are retrieved via Base protocol at probe time upfront; this way it 'should' be feasible to compile in any existent vendor protocol but allow at run-time only the registration with the SCMI core of the protocols whose vendor identity matches that of the identity advertised by the running firmware.... ...still not sure which of the IDs to use vendor/subvendor and still not have really experimented with this...so any feedback welcome. This would rule out, anyway, the capability of solving number clashes within the same vendor. Thanks, Cristian
On 1/17/24 18:34, Sibi Sankar wrote: > Enable LLCC/DDR dvfs through the Qualcomm's SCMI vendor protocol. > > Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> > --- > arch/arm64/boot/dts/qcom/x1e80100.dtsi | 48 ++++++++++++++++++++++++++ > 1 file changed, 48 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi > index 6856a206f7fc..3dc6f32fbb4c 100644 > --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi > +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi > @@ -329,6 +329,54 @@ scmi_dvfs: protocol@13 { > reg = <0x13>; > #clock-cells = <1>; > }; > + > + scmi_vendor: protocol@80 { > + reg = <0x80>; > + > + memlat { > + #address-cells = <1>; > + #size-cells = <0>; > + > + memory@0 { > + reg = <0x0>; /* Memory Type DDR */ I'm not sure reg is the best property to (ab)use.. You could very well define a new one, like qcom,memory type, then the subnodes could look like: memory-0 { qcom,memory-type = <QCOM_MEM_TYPE_DDR>; [...] }; > + freq-table-khz = <200000 4224000>; > + > + monitor-0 { > + qcom,cpulist = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7 &CPU8 &CPU9 &CPU10 &CPU11>; I fail to see the usefulness in checking which CPUs make use of the same DRAM or LLC pool. If that's something that may not be obvious in future designs like on dual-socket x86 servers, I think it can be deferred until then and for now, AFAIU you can just unconditionally assume all CPUs count. > + qcom,cpufreq-memfreq-tbl = < 999000 547000 >, > + < 1440000 768000 >, > + < 1671000 1555000 >, > + < 2189000 2092000 >, > + < 2156000 3187000 >, > + < 3860000 4224000 >; I.. can't seem to think of a future where this doesn't explode. When you release a different bin/SKU/fuse config of this SoC where the CPU frequencies are different, this will likely also need to be updated. We don't want that manual cruft in the devicetree. Since both previously cpufreq-hw and now cpufreq-scmi generally operate on levels that map to some frequencies in the firmware, could it be bound to that instead? Konrad
On Wed, 17 Jan 2024 at 19:37, Sibi Sankar <quic_sibis@quicinc.com> wrote: > > Enable LLCC/DDR dvfs through the Qualcomm's SCMI vendor protocol. Could you please post DT bindings? > > Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> > --- > arch/arm64/boot/dts/qcom/x1e80100.dtsi | 48 ++++++++++++++++++++++++++ > 1 file changed, 48 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi > index 6856a206f7fc..3dc6f32fbb4c 100644 > --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi > +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi > @@ -329,6 +329,54 @@ scmi_dvfs: protocol@13 { > reg = <0x13>; > #clock-cells = <1>; > }; > + > + scmi_vendor: protocol@80 { > + reg = <0x80>; > + > + memlat { This doesn't look like a generic node name. > + #address-cells = <1>; > + #size-cells = <0>; > + > + memory@0 { > + reg = <0x0>; /* Memory Type DDR */ > + freq-table-khz = <200000 4224000>; > + > + monitor-0 { > + qcom,cpulist = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7 &CPU8 &CPU9 &CPU10 &CPU11>; > + qcom,cpufreq-memfreq-tbl = < 999000 547000 >, > + < 1440000 768000 >, > + < 1671000 1555000 >, > + < 2189000 2092000 >, > + < 2156000 3187000 >, > + < 3860000 4224000 >; These tables should be rewritten as OPP tables. > + }; > + > + monitor-1 { > + qcom,compute-mon; > + qcom,cpulist = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7 &CPU8 &CPU9 &CPU10 &CPU11>; > + qcom,cpufreq-memfreq-tbl = < 1440000 200000 >, > + < 2189000 768000 >, > + < 2156000 1555000 >, > + < 3860000 2092000 >; > + }; > + }; > + > + memory@1 { > + reg = <0x1>; /* Memory Type LLCC */ > + freq-table-khz = <300000 1067000>; > + > + monitor-0 { > + qcom,cpulist = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7 &CPU8 &CPU9 &CPU10 &CPU11>; > + qcom,cpufreq-memfreq-tbl = < 999000 300000 >, > + < 1440000 466000 >, > + < 1671000 600000 >, > + < 2189000 806000 >, > + < 2156000 933000 >, > + < 3860000 1066000 >; > + }; > + }; > + }; > + }; > }; > }; > > -- > 2.34.1 > > -- With best wishes Dmitry
On Wed, Jan 17, 2024 at 11:04:54PM +0530, Sibi Sankar wrote: > From: Shivnandan Kumar <quic_kshivnan@quicinc.com> > > SCMI QCOM vendor protocol provides interface to communicate with SCMI > controller and enable vendor specific features like bus scaling capable > of running on it. > > Signed-off-by: Shivnandan Kumar <quic_kshivnan@quicinc.com> > Co-developed-by: Ramakrishna Gottimukkula <quic_rgottimu@quicinc.com> > Signed-off-by: Ramakrishna Gottimukkula <quic_rgottimu@quicinc.com> > Co-developed-by: Amir Vajid <avajid@quicinc.com> > Signed-off-by: Amir Vajid <avajid@quicinc.com> > Co-developed-by: Sibi Sankar <quic_sibis@quicinc.com> > Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> > --- > drivers/firmware/arm_scmi/Kconfig | 11 ++ > drivers/firmware/arm_scmi/Makefile | 1 + > drivers/firmware/arm_scmi/qcom_scmi_vendor.c | 160 +++++++++++++++++++ > include/linux/qcom_scmi_vendor.h | 36 +++++ > 4 files changed, 208 insertions(+) > create mode 100644 drivers/firmware/arm_scmi/qcom_scmi_vendor.c > create mode 100644 include/linux/qcom_scmi_vendor.h > > diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig > index aa5842be19b2..86b5d6c18ec4 100644 > --- a/drivers/firmware/arm_scmi/Kconfig > +++ b/drivers/firmware/arm_scmi/Kconfig > @@ -180,4 +180,15 @@ config ARM_SCMI_POWER_CONTROL > called scmi_power_control. Note this may needed early in boot to catch > early shutdown/reboot SCMI requests. > > +config QCOM_SCMI_VENDOR_PROTOCOL > + tristate "Qualcomm Technologies, Inc. Qcom SCMI vendor Protocol" > + depends on ARM || ARM64 || COMPILE_TEST > + depends on ARM_SCMI_PROTOCOL > + help > + The SCMI QCOM vendor protocol provides interface to communicate with SCMI > + controller and enable vendor specific features like bus scaling. > + I assume it will include all the Qualcomm specific vendor protocol handling here. Not sure how it it implemented across different platforms and but I already assume different platforms will use same protocol id for different things and this implementation will abstract all those details. > diff --git a/drivers/firmware/arm_scmi/qcom_scmi_vendor.c b/drivers/firmware/arm_scmi/qcom_scmi_vendor.c > new file mode 100644 > index 000000000000..878b99f0d1ef > --- /dev/null > +++ b/drivers/firmware/arm_scmi/qcom_scmi_vendor.c > @@ -0,0 +1,160 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2024, The Linux Foundation. All rights reserved. > + */ > + > +#include <linux/qcom_scmi_vendor.h> > + > +#include "common.h" > + > +#define EXTENDED_MSG_ID 0 This gives me no clue what this means ? > +#define SCMI_MAX_TX_RX_SIZE 128 > +#define PROTOCOL_PAYLOAD_SIZE 16 > +#define SET_PARAM 0x10 I assume these are the actual message IDs ? Any idea why 0x0-0xF is skipped ? I assume atleast the required 0x0-0x2 are implemented. > +#define GET_PARAM 0x11 > +#define START_ACTIVITY 0x12 > +#define STOP_ACTIVITY 0x13 In general, good to add description of these in the implementation here or under Documentation or a pointer to the url where I can get the info. If documenting within the kernel, please use SCMI spec format as it may be easy to follow the same pattern even in the vendor protocols. > + > +static int qcom_scmi_set_param(const struct scmi_protocol_handle *ph, void *buf, u64 algo_str, > + u32 param_id, size_t size) > +{ > + int ret = -EINVAL; > + struct scmi_xfer *t; > + u32 *msg; > + > + if (!ph || !ph->xops) > + return ret; > + > + ret = ph->xops->xfer_get_init(ph, SET_PARAM, size + PROTOCOL_PAYLOAD_SIZE, > + SCMI_MAX_TX_RX_SIZE, &t); > + if (ret) > + return ret; > + > + msg = t->tx.buf; > + *msg++ = cpu_to_le32(EXTENDED_MSG_ID); > + *msg++ = cpu_to_le32(algo_str & GENMASK(31, 0)); > + *msg++ = cpu_to_le32((algo_str & GENMASK(63, 32)) >> 32); > + *msg++ = cpu_to_le32(param_id); > + memcpy(msg, buf, size); > + ret = ph->xops->do_xfer(ph, t); > + ph->xops->xfer_put(ph, t); > + > + return ret; > +} > + > +static int qcom_scmi_get_param(const struct scmi_protocol_handle *ph, void *buf, u64 algo_str, > + u32 param_id, size_t tx_size, size_t rx_size) > +{ > + int ret = -EINVAL; > + struct scmi_xfer *t; > + u32 *msg; > + > + if (!ph || !ph->xops || !buf) > + return ret; > + > + ret = ph->xops->xfer_get_init(ph, GET_PARAM, tx_size + PROTOCOL_PAYLOAD_SIZE, > + SCMI_MAX_TX_RX_SIZE, &t); > + if (ret) > + return ret; > + > + msg = t->tx.buf; > + *msg++ = cpu_to_le32(EXTENDED_MSG_ID); > + *msg++ = cpu_to_le32(algo_str & GENMASK(31, 0)); > + *msg++ = cpu_to_le32((algo_str & GENMASK(63, 32)) >> 32); > + *msg++ = cpu_to_le32(param_id); > + memcpy(msg, buf, tx_size); > + ret = ph->xops->do_xfer(ph, t); > + if (t->rx.len > rx_size) { > + pr_err("SCMI received buffer size %zu is more than expected size %zu\n", > + t->rx.len, rx_size); > + return -EMSGSIZE; > + } > + memcpy(buf, t->rx.buf, t->rx.len); > + ph->xops->xfer_put(ph, t); > + > + return ret; > +} > + > +static int qcom_scmi_start_activity(const struct scmi_protocol_handle *ph, > + void *buf, u64 algo_str, u32 param_id, size_t size) > +{ > + int ret = -EINVAL; > + struct scmi_xfer *t; > + u32 *msg; > + > + if (!ph || !ph->xops) > + return ret; > + > + ret = ph->xops->xfer_get_init(ph, START_ACTIVITY, size + PROTOCOL_PAYLOAD_SIZE, > + SCMI_MAX_TX_RX_SIZE, &t); > + if (ret) > + return ret; > + > + msg = t->tx.buf; > + *msg++ = cpu_to_le32(EXTENDED_MSG_ID); > + *msg++ = cpu_to_le32(algo_str & GENMASK(31, 0)); > + *msg++ = cpu_to_le32((algo_str & GENMASK(63, 32)) >> 32); > + *msg++ = cpu_to_le32(param_id); > + memcpy(msg, buf, size); > + ret = ph->xops->do_xfer(ph, t); > + ph->xops->xfer_put(ph, t); > + > + return ret; > +} > + > +static int qcom_scmi_stop_activity(const struct scmi_protocol_handle *ph, void *buf, u64 algo_str, > + u32 param_id, size_t size) > +{ > + int ret = -EINVAL; > + struct scmi_xfer *t; > + u32 *msg; > + > + if (!ph || !ph->xops) > + return ret; > + > + ret = ph->xops->xfer_get_init(ph, STOP_ACTIVITY, size + PROTOCOL_PAYLOAD_SIZE, > + SCMI_MAX_TX_RX_SIZE, &t); > + if (ret) > + return ret; > + > + msg = t->tx.buf; > + *msg++ = cpu_to_le32(EXTENDED_MSG_ID); > + *msg++ = cpu_to_le32(algo_str & GENMASK(31, 0)); > + *msg++ = cpu_to_le32((algo_str & GENMASK(63, 32)) >> 32); > + *msg++ = cpu_to_le32(param_id); > + memcpy(msg, buf, size); > + ret = ph->xops->do_xfer(ph, t); > + ph->xops->xfer_put(ph, t); > + > + return ret; > +} > + > +static struct qcom_scmi_vendor_ops qcom_proto_ops = { > + .set_param = qcom_scmi_set_param, > + .get_param = qcom_scmi_get_param, > + .start_activity = qcom_scmi_start_activity, > + .stop_activity = qcom_scmi_stop_activity, > +}; > + > +static int qcom_scmi_vendor_protocol_init(const struct scmi_protocol_handle *ph) > +{ > + u32 version; > + > + ph->xops->version_get(ph, &version); > + > + dev_info(ph->dev, "qcom scmi version %d.%d\n", > + PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version)); > + > + return 0; > +} > + > +static const struct scmi_protocol qcom_scmi_vendor = { > + .id = QCOM_SCMI_VENDOR_PROTOCOL, As Cristian might have pointed out, this will conflict and we need better matching to ensure each vendor and protocols with each implementation has unique matching mechanism so that only one match occurs per protocol on any platform.
On 1/18/24 01:23, Konrad Dybcio wrote: > > > On 1/17/24 18:34, Sibi Sankar wrote: >> Add devicetree binding for CPUSS Control Processor (CPUCP) mailbox >> controller. >> >> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> >> --- > Hey Konrad, Thanks for taking time to review the series. > [...] > >> + - | >> + #include <dt-bindings/interrupt-controller/arm-gic.h> >> + >> + mailbox@17430000 { >> + compatible = "qcom,x1e80100-cpucp-mbox", "qcom,cpucp-mbox"; >> + reg = <0x17430000 0x10000>, <0x18830000 0x300>; > > These reg spaces are quite far apart.. On 7280-8550, a similar > mailbox exists, although it's dubbed RIMPS-mbox instead. In > that case, I separated the mbox into tx (via > qcom-apcs-ipc-mailbox.c) and rx (with a simple driver). Still > haven't pushed or posted that anywhere, I'd need to access > another machine.. > > On (some of) these SoCs, one of the channels (rx[1], iirc?) clearly > bleeds into the CPUFREQ_HW/OSM register region, which gives an > impression of misrepresenting the hardware. X1E doesn't have a > node for cpufreq_hw defined, so I can't tell whether it's also the > case here. I am aware of ^^ discussion and the X1E doesn't have this problem. Both the regions described are only used for mailbox communication. X1E uses the scmi perf protocol for cpu dvfs. -Sibi > > Konrad
On 1/18/24 01:45, Konrad Dybcio wrote: > > > On 1/17/24 18:34, Sibi Sankar wrote: >> From: Shivnandan Kumar <quic_kshivnan@quicinc.com> >> >> SCMI QCOM vendor protocol provides interface to communicate with SCMI >> controller and enable vendor specific features like bus scaling capable >> of running on it. > Hey Konrad, > "QCOM protocol" sounds overly generic, especially given how many > different vendor protocols have historically been present in > QC firmware.. Here it is specifically mentioned that way to communicate that this is the only vendor protocol exposed by Qualcomm. It handles all the other protocols which were usually handled separately on older SoCs. > >> >> Signed-off-by: Shivnandan Kumar <quic_kshivnan@quicinc.com> >> Co-developed-by: Ramakrishna Gottimukkula <quic_rgottimu@quicinc.com> >> Signed-off-by: Ramakrishna Gottimukkula <quic_rgottimu@quicinc.com> >> Co-developed-by: Amir Vajid <avajid@quicinc.com> >> Signed-off-by: Amir Vajid <avajid@quicinc.com> >> Co-developed-by: Sibi Sankar <quic_sibis@quicinc.com> >> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> >> --- > > So, this is another 0x80 protocol, different to the one that has > been shipping on devices that got released with msm-5.4, msm-5.10 > and msm-5.15 [1][2]. They're totally incompatible (judging by the > msg format), use the same protocol ID and they are (at a glance) > providing access to the same HW/FW/tunables. Thanks for bringing this up but like I already explained the only SoC that was actually shipped with ^^ protocol was SC7180 and we already have an alternative arrangement for memory dvfs upstreamed on it. Further more it handles only L3 dvfs so it makes zero sense to try to upstream the older protocol given that working dvfs solution already exists upstream. All other SoCs don't have the 0x80 protocol enabled for memory dvfs in production. > > I'm not sure if this can be trusted not to change again.. Unless > we get a strong commitment that all platforms (compute, mobile, > auto, iot, whatever) stick to this one.. This is exactly that consolidation effort from Qualcomm. Here they expose just one vendor protocol and implement all the algorithms just through it. > > That said, the spec (DEN0056C) says that protocol IDs 0x80-0xff > are: "Reserved for vendor or platform-specific extensions to > this interface.". So if perhaps there's a will to maintain > multiple versions of this, with a way to discern between them.. > > Konrad > > [1] > https://git.codelinaro.org/clo/la/kernel/msm-5.15/-/blob/KERNEL.PLATFORM.2.1.r5-05400-kernel.0/drivers/firmware/arm_scmi/memlat_vendor.c?ref_type=tags > [2] > https://git.codelinaro.org/clo/la/kernel/msm-5.15/-/blob/KERNEL.PLATFORM.2.1.r5-05400-kernel.0/include/linux/scmi_memlat.h#L16
On 8.02.2024 11:22, Sibi Sankar wrote: > > > On 1/18/24 01:23, Konrad Dybcio wrote: >> >> >> On 1/17/24 18:34, Sibi Sankar wrote: >>> Add devicetree binding for CPUSS Control Processor (CPUCP) mailbox >>> controller. >>> >>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> >>> --- >> > > Hey Konrad, > > Thanks for taking time to review the series. > >> [...] >> >>> + - | >>> + #include <dt-bindings/interrupt-controller/arm-gic.h> >>> + >>> + mailbox@17430000 { >>> + compatible = "qcom,x1e80100-cpucp-mbox", "qcom,cpucp-mbox"; >>> + reg = <0x17430000 0x10000>, <0x18830000 0x300>; >> >> These reg spaces are quite far apart.. On 7280-8550, a similar >> mailbox exists, although it's dubbed RIMPS-mbox instead. In >> that case, I separated the mbox into tx (via >> qcom-apcs-ipc-mailbox.c) and rx (with a simple driver). Still >> haven't pushed or posted that anywhere, I'd need to access >> another machine.. >> >> On (some of) these SoCs, one of the channels (rx[1], iirc?) clearly >> bleeds into the CPUFREQ_HW/OSM register region, which gives an >> impression of misrepresenting the hardware. X1E doesn't have a >> node for cpufreq_hw defined, so I can't tell whether it's also the >> case here. > > I am aware of ^^ discussion and the X1E doesn't have this problem. > Both the regions described are only used for mailbox communication. > X1E uses the scmi perf protocol for cpu dvfs. Yes, that's clear. I am however asking for something different: I presume the CPUSS IP hasn't changed too much on this SoC, other than having new cores and OSM now being controlled through a different firmware interface, and I'd like to keep the hardware description in our DT as close to the metal as possible. In other words, if the good ol' OSM hardware is indeed there under however many layers of firmware, and if RX does indeed bleed into its register space, I'd prefer there be at least a syscon node describing the actual block, and not a magic hwio entry that's many zeroes away. Konrad
On 8.02.2024 12:44, Sibi Sankar wrote: > > > On 1/18/24 01:45, Konrad Dybcio wrote: >> >> >> On 1/17/24 18:34, Sibi Sankar wrote: >>> From: Shivnandan Kumar <quic_kshivnan@quicinc.com> >>> >>> SCMI QCOM vendor protocol provides interface to communicate with SCMI >>> controller and enable vendor specific features like bus scaling capable >>> of running on it. >> > > Hey Konrad, > >> "QCOM protocol" sounds overly generic, especially given how many >> different vendor protocols have historically been present in >> QC firmware.. > > Here it is specifically mentioned that way to communicate that > this is the only vendor protocol exposed by Qualcomm. It handles > all the other protocols which were usually handled separately on > older SoCs. I'm no SCMI specialist but that's a rather.. peculiar design decision, I guess > >> >>> >>> Signed-off-by: Shivnandan Kumar <quic_kshivnan@quicinc.com> >>> Co-developed-by: Ramakrishna Gottimukkula <quic_rgottimu@quicinc.com> >>> Signed-off-by: Ramakrishna Gottimukkula <quic_rgottimu@quicinc.com> >>> Co-developed-by: Amir Vajid <avajid@quicinc.com> >>> Signed-off-by: Amir Vajid <avajid@quicinc.com> >>> Co-developed-by: Sibi Sankar <quic_sibis@quicinc.com> >>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> >>> --- >> >> So, this is another 0x80 protocol, different to the one that has >> been shipping on devices that got released with msm-5.4, msm-5.10 >> and msm-5.15 [1][2]. They're totally incompatible (judging by the >> msg format), use the same protocol ID and they are (at a glance) >> providing access to the same HW/FW/tunables. > > Thanks for bringing this up but like I already explained the only > SoC that was actually shipped with ^^ protocol was SC7180 and we > already have an alternative arrangement for memory dvfs upstreamed > on it. Ok, that makes sense. I took my 8550 phone, enabled some debug prints and it looks like the only SCMI protocol exposed is 0x19 (which doesn't seem to be defined). Not sure what other devices would spit out, but I assume what you said is true. For completeness, the reported rev is: arm-scmi firmware:scmi: SCMI Protocol v2.0 'Qualcomm:' Firmware version 0x10000 > Further more it handles only L3 dvfs so it makes zero sense > to try to upstream the older protocol given that working dvfs solution > already exists upstream. We don't have any sort of governor for it though, so I wouldn't go as far as calling it working :P > All other SoCs don't have the 0x80 protocol > enabled for memory dvfs in production. > >> >> I'm not sure if this can be trusted not to change again.. Unless >> we get a strong commitment that all platforms (compute, mobile, >> auto, iot, whatever) stick to this one.. > > This is exactly that consolidation effort from Qualcomm. Here they > expose just one vendor protocol and implement all the algorithms just > through it. And I'm very glad you're taking such consolidation steps.. Just a little worried that in case this protocol's extensibility is exhausted, the next one would need to be called.. "Qualcomm2"? Konrad
On 2/9/24 04:44, Konrad Dybcio wrote: > On 8.02.2024 11:22, Sibi Sankar wrote: >> >> >> On 1/18/24 01:23, Konrad Dybcio wrote: >>> >>> >>> On 1/17/24 18:34, Sibi Sankar wrote: >>>> Add devicetree binding for CPUSS Control Processor (CPUCP) mailbox >>>> controller. >>>> >>>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> >>>> --- >>> >> >> Hey Konrad, >> >> Thanks for taking time to review the series. >> >>> [...] >>> >>>> + - | >>>> + #include <dt-bindings/interrupt-controller/arm-gic.h> >>>> + >>>> + mailbox@17430000 { >>>> + compatible = "qcom,x1e80100-cpucp-mbox", "qcom,cpucp-mbox"; >>>> + reg = <0x17430000 0x10000>, <0x18830000 0x300>; >>> >>> These reg spaces are quite far apart.. On 7280-8550, a similar >>> mailbox exists, although it's dubbed RIMPS-mbox instead. In >>> that case, I separated the mbox into tx (via >>> qcom-apcs-ipc-mailbox.c) and rx (with a simple driver). Still >>> haven't pushed or posted that anywhere, I'd need to access >>> another machine.. >>> >>> On (some of) these SoCs, one of the channels (rx[1], iirc?) clearly >>> bleeds into the CPUFREQ_HW/OSM register region, which gives an >>> impression of misrepresenting the hardware. X1E doesn't have a >>> node for cpufreq_hw defined, so I can't tell whether it's also the >>> case here. >> >> I am aware of ^^ discussion and the X1E doesn't have this problem. >> Both the regions described are only used for mailbox communication. >> X1E uses the scmi perf protocol for cpu dvfs. > > Yes, that's clear. > > I am however asking for something different: I presume the CPUSS > IP hasn't changed too much on this SoC, other than having new cores and > OSM now being controlled through a different firmware interface, and I'd > like to keep the hardware description in our DT as close to the metal as > possible. > > In other words, if the good ol' OSM hardware is indeed there under however > many layers of firmware, and if RX does indeed bleed into its register > space, I'd prefer there be at least a syscon node describing the actual > block, and not a magic hwio entry that's many zeroes away. > With the new cores X1E does not have any artifacts from the legacy OSM way that Qualcomm has followed till now. If it indeed existed it would make zero sense to vote for CPU frequencies through a mailbox than vote for it directly. -Sibi > Konrad >
On 2/10/24 04:15, Konrad Dybcio wrote: > On 8.02.2024 12:44, Sibi Sankar wrote: >> >> >> On 1/18/24 01:45, Konrad Dybcio wrote: >>> >>> >>> On 1/17/24 18:34, Sibi Sankar wrote: >>>> From: Shivnandan Kumar <quic_kshivnan@quicinc.com> >>>> >>>> SCMI QCOM vendor protocol provides interface to communicate with SCMI >>>> controller and enable vendor specific features like bus scaling capable >>>> of running on it. >>> >> >> Hey Konrad, >> >>> "QCOM protocol" sounds overly generic, especially given how many >>> different vendor protocols have historically been present in >>> QC firmware.. >> >> Here it is specifically mentioned that way to communicate that >> this is the only vendor protocol exposed by Qualcomm. It handles >> all the other protocols which were usually handled separately on >> older SoCs. > > I'm no SCMI specialist but that's a rather.. peculiar design decision, > I guess > > >> >>> >>>> >>>> Signed-off-by: Shivnandan Kumar <quic_kshivnan@quicinc.com> >>>> Co-developed-by: Ramakrishna Gottimukkula <quic_rgottimu@quicinc.com> >>>> Signed-off-by: Ramakrishna Gottimukkula <quic_rgottimu@quicinc.com> >>>> Co-developed-by: Amir Vajid <avajid@quicinc.com> >>>> Signed-off-by: Amir Vajid <avajid@quicinc.com> >>>> Co-developed-by: Sibi Sankar <quic_sibis@quicinc.com> >>>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> >>>> --- >>> >>> So, this is another 0x80 protocol, different to the one that has >>> been shipping on devices that got released with msm-5.4, msm-5.10 >>> and msm-5.15 [1][2]. They're totally incompatible (judging by the >>> msg format), use the same protocol ID and they are (at a glance) >>> providing access to the same HW/FW/tunables. >> >> Thanks for bringing this up but like I already explained the only >> SoC that was actually shipped with ^^ protocol was SC7180 and we >> already have an alternative arrangement for memory dvfs upstreamed >> on it. > > Ok, that makes sense. > > I took my 8550 phone, enabled some debug prints and it looks like the > only SCMI protocol exposed is 0x19 (which doesn't seem to be defined). > > Not sure what other devices would spit out, but I assume what you said > is true. > > For completeness, the reported rev is: > > arm-scmi firmware:scmi: SCMI Protocol v2.0 'Qualcomm:' Firmware version 0x10000 > >> Further more it handles only L3 dvfs so it makes zero sense >> to try to upstream the older protocol given that working dvfs solution >> already exists upstream. > > We don't have any sort of governor for it though, so I wouldn't go as > far as calling it working :P It is a working solution (it is equivalent to the compute mon mapping in downstream implementation) but isn't feature complete ¯\_(ツ)_/¯. > >> All other SoCs don't have the 0x80 protocol >> enabled for memory dvfs in production. >> >>> >>> I'm not sure if this can be trusted not to change again.. Unless >>> we get a strong commitment that all platforms (compute, mobile, >>> auto, iot, whatever) stick to this one.. >> >> This is exactly that consolidation effort from Qualcomm. Here they >> expose just one vendor protocol and implement all the algorithms just >> through it. > > And I'm very glad you're taking such consolidation steps.. Just a little > worried that in case this protocol's extensibility is exhausted, the next > one would need to be called.. "Qualcomm2"? We don't see ^^ happening in the near future (meaning this doesn't apply to just X1E). The consolidation would still be better than spinning out n number of protocols per SoC. -Sibi > > Konrad
On 1/18/24 22:52, Sudeep Holla wrote: > On Wed, Jan 17, 2024 at 11:04:54PM +0530, Sibi Sankar wrote: >> From: Shivnandan Kumar <quic_kshivnan@quicinc.com> >> >> SCMI QCOM vendor protocol provides interface to communicate with SCMI >> controller and enable vendor specific features like bus scaling capable >> of running on it. Hey Sudeep, Thanks for taking time to review the series! >> >> Signed-off-by: Shivnandan Kumar <quic_kshivnan@quicinc.com> >> Co-developed-by: Ramakrishna Gottimukkula <quic_rgottimu@quicinc.com> >> Signed-off-by: Ramakrishna Gottimukkula <quic_rgottimu@quicinc.com> >> Co-developed-by: Amir Vajid <avajid@quicinc.com> >> Signed-off-by: Amir Vajid <avajid@quicinc.com> >> Co-developed-by: Sibi Sankar <quic_sibis@quicinc.com> >> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> >> --- >> drivers/firmware/arm_scmi/Kconfig | 11 ++ >> drivers/firmware/arm_scmi/Makefile | 1 + >> drivers/firmware/arm_scmi/qcom_scmi_vendor.c | 160 +++++++++++++++++++ >> include/linux/qcom_scmi_vendor.h | 36 +++++ >> 4 files changed, 208 insertions(+) >> create mode 100644 drivers/firmware/arm_scmi/qcom_scmi_vendor.c >> create mode 100644 include/linux/qcom_scmi_vendor.h >> >> diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig >> index aa5842be19b2..86b5d6c18ec4 100644 >> --- a/drivers/firmware/arm_scmi/Kconfig >> +++ b/drivers/firmware/arm_scmi/Kconfig >> @@ -180,4 +180,15 @@ config ARM_SCMI_POWER_CONTROL >> called scmi_power_control. Note this may needed early in boot to catch >> early shutdown/reboot SCMI requests. >> >> +config QCOM_SCMI_VENDOR_PROTOCOL >> + tristate "Qualcomm Technologies, Inc. Qcom SCMI vendor Protocol" >> + depends on ARM || ARM64 || COMPILE_TEST >> + depends on ARM_SCMI_PROTOCOL >> + help >> + The SCMI QCOM vendor protocol provides interface to communicate with SCMI >> + controller and enable vendor specific features like bus scaling. >> + > > I assume it will include all the Qualcomm specific vendor protocol > handling here. Not sure how it it implemented across different platforms > and but I already assume different platforms will use same protocol id > for different things and this implementation will abstract all those > details. Yes, that's what we are going for. > >> diff --git a/drivers/firmware/arm_scmi/qcom_scmi_vendor.c b/drivers/firmware/arm_scmi/qcom_scmi_vendor.c >> new file mode 100644 >> index 000000000000..878b99f0d1ef >> --- /dev/null >> +++ b/drivers/firmware/arm_scmi/qcom_scmi_vendor.c >> @@ -0,0 +1,160 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Copyright (c) 2024, The Linux Foundation. All rights reserved. >> + */ >> + >> +#include <linux/qcom_scmi_vendor.h> >> + >> +#include "common.h" >> + >> +#define EXTENDED_MSG_ID 0 > > This gives me no clue what this means ? > >> +#define SCMI_MAX_TX_RX_SIZE 128 >> +#define PROTOCOL_PAYLOAD_SIZE 16 >> +#define SET_PARAM 0x10 > > I assume these are the actual message IDs ? Any idea why 0x0-0xF is skipped ? > I assume atleast the required 0x0-0x2 are implemented. Yup 0x0-0x2 should be implemented. I'll have to get info on why the rest were skipped. Will add comments detailing the extended msg id as well. > >> +#define GET_PARAM 0x11 >> +#define START_ACTIVITY 0x12 >> +#define STOP_ACTIVITY 0x13 > > In general, good to add description of these in the implementation here > or under Documentation or a pointer to the url where I can get the info. > If documenting within the kernel, please use SCMI spec format as it may > be easy to follow the same pattern even in the vendor protocols. > ack >> + >> +static int qcom_scmi_set_param(const struct scmi_protocol_handle *ph, void *buf, u64 algo_str, >> + u32 param_id, size_t size) >> +{ >> + int ret = -EINVAL; >> + struct scmi_xfer *t; >> + u32 *msg; >> + >> + if (!ph || !ph->xops) >> + return ret; >> + >> + ret = ph->xops->xfer_get_init(ph, SET_PARAM, size + PROTOCOL_PAYLOAD_SIZE, >> + SCMI_MAX_TX_RX_SIZE, &t); >> + if (ret) >> + return ret; >> + >> + msg = t->tx.buf; >> + *msg++ = cpu_to_le32(EXTENDED_MSG_ID); >> + *msg++ = cpu_to_le32(algo_str & GENMASK(31, 0)); >> + *msg++ = cpu_to_le32((algo_str & GENMASK(63, 32)) >> 32); >> + *msg++ = cpu_to_le32(param_id); >> + memcpy(msg, buf, size); >> + ret = ph->xops->do_xfer(ph, t); >> + ph->xops->xfer_put(ph, t); >> + >> + return ret; >> +} >> + >> +static int qcom_scmi_get_param(const struct scmi_protocol_handle *ph, void *buf, u64 algo_str, >> + u32 param_id, size_t tx_size, size_t rx_size) >> +{ >> + int ret = -EINVAL; >> + struct scmi_xfer *t; >> + u32 *msg; >> + >> + if (!ph || !ph->xops || !buf) >> + return ret; >> + >> + ret = ph->xops->xfer_get_init(ph, GET_PARAM, tx_size + PROTOCOL_PAYLOAD_SIZE, >> + SCMI_MAX_TX_RX_SIZE, &t); >> + if (ret) >> + return ret; >> + >> + msg = t->tx.buf; >> + *msg++ = cpu_to_le32(EXTENDED_MSG_ID); >> + *msg++ = cpu_to_le32(algo_str & GENMASK(31, 0)); >> + *msg++ = cpu_to_le32((algo_str & GENMASK(63, 32)) >> 32); >> + *msg++ = cpu_to_le32(param_id); >> + memcpy(msg, buf, tx_size); >> + ret = ph->xops->do_xfer(ph, t); >> + if (t->rx.len > rx_size) { >> + pr_err("SCMI received buffer size %zu is more than expected size %zu\n", >> + t->rx.len, rx_size); >> + return -EMSGSIZE; >> + } >> + memcpy(buf, t->rx.buf, t->rx.len); >> + ph->xops->xfer_put(ph, t); >> + >> + return ret; >> +} >> + >> +static int qcom_scmi_start_activity(const struct scmi_protocol_handle *ph, >> + void *buf, u64 algo_str, u32 param_id, size_t size) >> +{ >> + int ret = -EINVAL; >> + struct scmi_xfer *t; >> + u32 *msg; >> + >> + if (!ph || !ph->xops) >> + return ret; >> + >> + ret = ph->xops->xfer_get_init(ph, START_ACTIVITY, size + PROTOCOL_PAYLOAD_SIZE, >> + SCMI_MAX_TX_RX_SIZE, &t); >> + if (ret) >> + return ret; >> + >> + msg = t->tx.buf; >> + *msg++ = cpu_to_le32(EXTENDED_MSG_ID); >> + *msg++ = cpu_to_le32(algo_str & GENMASK(31, 0)); >> + *msg++ = cpu_to_le32((algo_str & GENMASK(63, 32)) >> 32); >> + *msg++ = cpu_to_le32(param_id); >> + memcpy(msg, buf, size); >> + ret = ph->xops->do_xfer(ph, t); >> + ph->xops->xfer_put(ph, t); >> + >> + return ret; >> +} >> + >> +static int qcom_scmi_stop_activity(const struct scmi_protocol_handle *ph, void *buf, u64 algo_str, >> + u32 param_id, size_t size) >> +{ >> + int ret = -EINVAL; >> + struct scmi_xfer *t; >> + u32 *msg; >> + >> + if (!ph || !ph->xops) >> + return ret; >> + >> + ret = ph->xops->xfer_get_init(ph, STOP_ACTIVITY, size + PROTOCOL_PAYLOAD_SIZE, >> + SCMI_MAX_TX_RX_SIZE, &t); >> + if (ret) >> + return ret; >> + >> + msg = t->tx.buf; >> + *msg++ = cpu_to_le32(EXTENDED_MSG_ID); >> + *msg++ = cpu_to_le32(algo_str & GENMASK(31, 0)); >> + *msg++ = cpu_to_le32((algo_str & GENMASK(63, 32)) >> 32); >> + *msg++ = cpu_to_le32(param_id); >> + memcpy(msg, buf, size); >> + ret = ph->xops->do_xfer(ph, t); >> + ph->xops->xfer_put(ph, t); >> + >> + return ret; >> +} >> + >> +static struct qcom_scmi_vendor_ops qcom_proto_ops = { >> + .set_param = qcom_scmi_set_param, >> + .get_param = qcom_scmi_get_param, >> + .start_activity = qcom_scmi_start_activity, >> + .stop_activity = qcom_scmi_stop_activity, >> +}; >> + >> +static int qcom_scmi_vendor_protocol_init(const struct scmi_protocol_handle *ph) >> +{ >> + u32 version; >> + >> + ph->xops->version_get(ph, &version); >> + >> + dev_info(ph->dev, "qcom scmi version %d.%d\n", >> + PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version)); >> + >> + return 0; >> +} >> + >> +static const struct scmi_protocol qcom_scmi_vendor = { >> + .id = QCOM_SCMI_VENDOR_PROTOCOL, > > As Cristian might have pointed out, this will conflict and we need better > matching to ensure each vendor and protocols with each implementation has > unique matching mechanism so that only one match occurs per protocol on > any platform. Ack. Also as mentioned in another thread this will be the only implementation of the 0x80 vendor protocol upstream given that no other SoC actually shipped with it enabled (expect for sc7180 which already has an alternative dvfs solution upstream). -Sibi >
On 1/18/24 02:17, Dmitry Baryshkov wrote: > On Wed, 17 Jan 2024 at 19:37, Sibi Sankar <quic_sibis@quicinc.com> wrote: >> >> Enable LLCC/DDR dvfs through the Qualcomm's SCMI vendor protocol. > > Could you please post DT bindings? ack, will include it in the next re-spin. > >> >> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> >> --- >> arch/arm64/boot/dts/qcom/x1e80100.dtsi | 48 ++++++++++++++++++++++++++ >> 1 file changed, 48 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi >> index 6856a206f7fc..3dc6f32fbb4c 100644 >> --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi >> +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi >> @@ -329,6 +329,54 @@ scmi_dvfs: protocol@13 { >> reg = <0x13>; >> #clock-cells = <1>; >> }; >> + >> + scmi_vendor: protocol@80 { >> + reg = <0x80>; >> + >> + memlat { > > This doesn't look like a generic node name. > >> + #address-cells = <1>; >> + #size-cells = <0>; > >> + >> + memory@0 { >> + reg = <0x0>; /* Memory Type DDR */ >> + freq-table-khz = <200000 4224000>; >> + >> + monitor-0 { >> + qcom,cpulist = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7 &CPU8 &CPU9 &CPU10 &CPU11>; > > > >> + qcom,cpufreq-memfreq-tbl = < 999000 547000 >, >> + < 1440000 768000 >, >> + < 1671000 1555000 >, >> + < 2189000 2092000 >, >> + < 2156000 3187000 >, >> + < 3860000 4224000 >; > > These tables should be rewritten as OPP tables. > > >> + }; >> + >> + monitor-1 { >> + qcom,compute-mon; >> + qcom,cpulist = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7 &CPU8 &CPU9 &CPU10 &CPU11>; >> + qcom,cpufreq-memfreq-tbl = < 1440000 200000 >, >> + < 2189000 768000 >, >> + < 2156000 1555000 >, >> + < 3860000 2092000 >; >> + }; >> + }; >> + >> + memory@1 { >> + reg = <0x1>; /* Memory Type LLCC */ >> + freq-table-khz = <300000 1067000>; >> + >> + monitor-0 { >> + qcom,cpulist = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7 &CPU8 &CPU9 &CPU10 &CPU11>; >> + qcom,cpufreq-memfreq-tbl = < 999000 300000 >, >> + < 1440000 466000 >, >> + < 1671000 600000 >, >> + < 2189000 806000 >, >> + < 2156000 933000 >, >> + < 3860000 1066000 >; >> + }; >> + }; >> + }; >> + }; >> }; >> }; >> >> -- >> 2.34.1 >> >> > > > -- > With best wishes > Dmitry
On 1/18/24 02:08, Konrad Dybcio wrote: > > > On 1/17/24 18:34, Sibi Sankar wrote: >> Enable LLCC/DDR dvfs through the Qualcomm's SCMI vendor protocol. >> >> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> >> --- >> arch/arm64/boot/dts/qcom/x1e80100.dtsi | 48 ++++++++++++++++++++++++++ >> 1 file changed, 48 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi >> b/arch/arm64/boot/dts/qcom/x1e80100.dtsi >> index 6856a206f7fc..3dc6f32fbb4c 100644 >> --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi >> +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi >> @@ -329,6 +329,54 @@ scmi_dvfs: protocol@13 { >> reg = <0x13>; >> #clock-cells = <1>; >> }; >> + >> + scmi_vendor: protocol@80 { >> + reg = <0x80>; >> + >> + memlat { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + memory@0 { >> + reg = <0x0>; /* Memory Type DDR */ > > I'm not sure reg is the best property to (ab)use.. I'm ok with introducing a custom property as well. I went ahead with reg mainly because the overall structure looked similar to audio apr. > > You could very well define a new one, like qcom,memory type, > then the subnodes could look like: > > memory-0 { > qcom,memory-type = <QCOM_MEM_TYPE_DDR>; > [...] > }; > >> + freq-table-khz = <200000 4224000>; >> + >> + monitor-0 { >> + qcom,cpulist = <&CPU0 &CPU1 &CPU2 &CPU3 >> &CPU4 &CPU5 &CPU6 &CPU7 &CPU8 &CPU9 &CPU10 &CPU11>; > > I fail to see the usefulness in checking which CPUs make use of > the same DRAM or LLC pool. If that's something that may not be > obvious in future designs like on dual-socket x86 servers, > I think it can be deferred until then and for now, AFAIU you > can just unconditionally assume all CPUs count. we list all the cpus here because on X1E they are identical and have the same cpu frequency to memory frequency mapping. But doesn't really apply to other SoCs in general. But dropping this would mean that driver assumes a table applies to all cpus by default. > >> + qcom,cpufreq-memfreq-tbl = < 999000 >> 547000 >, >> + < 1440000 768000 >, >> + < 1671000 1555000 >, >> + < 2189000 2092000 >, >> + < 2156000 3187000 >, >> + < 3860000 4224000 >; > > I.. can't seem to think of a future where this doesn't explode. Not really ... You can already see a more or less standard table being used across various skus on older SoCs that uses memlat running from the kernel downstream. So that should count for something. > > When you release a different bin/SKU/fuse config of this SoC where > the CPU frequencies are different, this will likely also need to be > updated. We don't want that manual cruft in the devicetree. Also unlike cpufreq map, if you notice this table doesn't list all possible cpu frequencies but list broad ranges instead. This way the table rarely needs updates unless we want to scale to max llcc/ddr at a lower CPU frequency for a particular SKU. > > Since both previously cpufreq-hw and now cpufreq-scmi generally > operate on levels that map to some frequencies in the firmware, > could it be bound to that instead? At this point the only decision is whether the table lies in dt or in the driver. But driver wouldn't even have a way to distinguish between various skus so the dt looks like the only option. -Sibi > > Konrad >
On Wed, Jan 17, 2024 at 11:04:51PM +0530, Sibi Sankar wrote: > This patch series introduces the Qualcomm SCMI Vendor protocol and adds a > client driver that interacts with the vendor protocol and passes on the > required tuneables to start various features running on the SCMI controller. > Hi Sibi, > The series specifically enables (LLCC/DDR) dvfs on X1E80100 SoC by passing > several tuneables including the IPM ratio (Instructions Per Miss), > cpu frequency to memory/bus frequency tables, CPU mapping to the vendor > protocol which in turn will enable the memory latency governor running > on the SCMI controller. > As a side note, before I forget (and I got lost again searching for this thread), next time you post this, please CC also linux-arm-kernel, being this series SCMI-related this way can be seen by other non-MSM/QC SCMI-interested people. (as it is advised by get_maintainer.pl too) Thanks, Cristian