* Re: [PATCH v2 3/6] firmware: arm_scmi: add initial support for i.MX BBM protocol
From: Cristian Marussi @ 2024-04-08 18:04 UTC (permalink / raw)
To: Peng Fan (OSS)
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Shawn Guo,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
Sudeep Holla, Peng Fan, devicetree, imx, linux-arm-kernel,
linux-kernel
In-Reply-To: <20240405-imx95-bbm-misc-v2-v2-3-9fc9186856c2@nxp.com>
On Fri, Apr 05, 2024 at 08:39:25PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
>
> The i.MX BBM protocol is for managing i.MX BBM module which provides
> RTC and BUTTON feature.
>
Hi,
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> drivers/firmware/arm_scmi/Kconfig | 10 +
> drivers/firmware/arm_scmi/Makefile | 1 +
> drivers/firmware/arm_scmi/imx-sm-bbm.c | 378 +++++++++++++++++++++++++++++++++
> include/linux/scmi_imx_protocol.h | 45 ++++
> 4 files changed, 434 insertions(+)
>
> diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
> index aa5842be19b2..56d11c9d9f47 100644
> --- a/drivers/firmware/arm_scmi/Kconfig
> +++ b/drivers/firmware/arm_scmi/Kconfig
> @@ -181,3 +181,13 @@ config ARM_SCMI_POWER_CONTROL
> early shutdown/reboot SCMI requests.
>
> endmenu
> +
> +config IMX_SCMI_BBM_EXT
> + tristate "i.MX SCMI BBM EXTENSION"
> + depends on ARM_SCMI_PROTOCOL || (COMPILE_TEST && OF)
> + default y if ARCH_MXC
> + help
> + This enables i.MX System BBM control logic which supports RTC
> + and BUTTON.
> +
> + This driver can also be built as a module.
> diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
> index a7bc4796519c..327687acf857 100644
> --- a/drivers/firmware/arm_scmi/Makefile
> +++ b/drivers/firmware/arm_scmi/Makefile
> @@ -11,6 +11,7 @@ scmi-transport-$(CONFIG_ARM_SCMI_HAVE_MSG) += msg.o
> scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO) += virtio.o
> scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o
> scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o
> +scmi-protocols-$(CONFIG_IMX_SCMI_BBM_EXT) += imx-sm-bbm.o
> scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y) $(scmi-transport-y)
>
> obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-core.o
> diff --git a/drivers/firmware/arm_scmi/imx-sm-bbm.c b/drivers/firmware/arm_scmi/imx-sm-bbm.c
> new file mode 100644
> index 000000000000..92c0aedf65cc
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/imx-sm-bbm.c
> @@ -0,0 +1,378 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * System Control and Management Interface (SCMI) NXP BBM Protocol
> + *
> + * Copyright 2024 NXP
> + */
> +
> +#define pr_fmt(fmt) "SCMI Notifications BBM - " fmt
> +
> +#include <linux/bits.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/scmi_protocol.h>
> +#include <linux/scmi_imx_protocol.h>
> +
> +#include "protocols.h"
> +#include "notify.h"
> +
> +#define SCMI_PROTOCOL_SUPPORTED_VERSION 0x10000
> +
I appreciate that you added versioning but I think a bit of documentation
about what the protocol and its comamnds purpose is still lacking, as asked
by Sudeep previously
https://lore.kernel.org/linux-arm-kernel/ZeGtoJ7ztSe8Kg8R@bogus/#t
> +enum scmi_imx_bbm_protocol_cmd {
> + IMX_BBM_GPR_SET = 0x3,
> + IMX_BBM_GPR_GET = 0x4,
> + IMX_BBM_RTC_ATTRIBUTES = 0x5,
> + IMX_BBM_RTC_TIME_SET = 0x6,
> + IMX_BBM_RTC_TIME_GET = 0x7,
> + IMX_BBM_RTC_ALARM_SET = 0x8,
> + IMX_BBM_BUTTON_GET = 0x9,
> + IMX_BBM_RTC_NOTIFY = 0xA,
> + IMX_BBM_BUTTON_NOTIFY = 0xB,
> +};
> +
> +#define GET_RTCS_NR(x) le32_get_bits((x), GENMASK(23, 16))
> +#define GET_GPRS_NR(x) le32_get_bits((x), GENMASK(15, 0))
> +
> +#define SCMI_IMX_BBM_NOTIFY_RTC_UPDATED BIT(2)
> +#define SCMI_IMX_BBM_NOTIFY_RTC_ROLLOVER BIT(1)
> +#define SCMI_IMX_BBM_NOTIFY_RTC_ALARM BIT(0)
> +
> +#define SCMI_IMX_BBM_RTC_ALARM_ENABLE_FLAG BIT(0)
> +
> +#define SCMI_IMX_BBM_NOTIFY_RTC_FLAG \
> + (SCMI_IMX_BBM_NOTIFY_RTC_UPDATED | SCMI_IMX_BBM_NOTIFY_RTC_ROLLOVER | \
> + SCMI_IMX_BBM_NOTIFY_RTC_ALARM)
> +
> +#define SCMI_IMX_BBM_EVENT_RTC_MASK GENMASK(31, 24)
> +
> +struct scmi_imx_bbm_info {
> + u32 version;
> + int nr_rtc;
> + int nr_gpr;
> +};
> +
> +struct scmi_msg_imx_bbm_protocol_attributes {
> + __le32 attributes;
> +};
> +
> +struct scmi_imx_bbm_set_time {
> + __le32 id;
> + __le32 flags;
> + __le32 value_low;
> + __le32 value_high;
> +};
> +
> +struct scmi_imx_bbm_get_time {
> + __le32 id;
> + __le32 flags;
> +};
> +
> +struct scmi_imx_bbm_alarm_time {
> + __le32 id;
> + __le32 flags;
> + __le32 value_low;
> + __le32 value_high;
> +};
> +
> +struct scmi_msg_imx_bbm_rtc_notify {
> + __le32 rtc_id;
> + __le32 flags;
> +};
> +
> +struct scmi_msg_imx_bbm_button_notify {
> + __le32 flags;
> +};
> +
> +struct scmi_imx_bbm_notify_payld {
> + __le32 flags;
> +};
> +
> +static int scmi_imx_bbm_attributes_get(const struct scmi_protocol_handle *ph,
> + struct scmi_imx_bbm_info *pi)
> +{
> + int ret;
> + struct scmi_xfer *t;
> + struct scmi_msg_imx_bbm_protocol_attributes *attr;
> +
> + ret = ph->xops->xfer_get_init(ph, PROTOCOL_ATTRIBUTES, 0, sizeof(*attr), &t);
> + if (ret)
> + return ret;
> +
> + attr = t->rx.buf;
> +
> + ret = ph->xops->do_xfer(ph, t);
> + if (!ret) {
> + pi->nr_rtc = GET_RTCS_NR(attr->attributes);
> + pi->nr_gpr = GET_GPRS_NR(attr->attributes);
> + }
> +
> + ph->xops->xfer_put(ph, t);
> +
> + return ret;
> +}
> +
> +static int scmi_imx_bbm_notify(const struct scmi_protocol_handle *ph,
> + u32 src_id, int message_id, bool enable)
> +{
> + int ret;
> + struct scmi_xfer *t;
> +
> + if (message_id == IMX_BBM_RTC_NOTIFY) {
> + struct scmi_msg_imx_bbm_rtc_notify *rtc_notify;
> +
> + ret = ph->xops->xfer_get_init(ph, message_id,
> + sizeof(*rtc_notify), 0, &t);
> + if (ret)
> + return ret;
> +
> + rtc_notify = t->tx.buf;
> + rtc_notify->rtc_id = cpu_to_le32(0);
> + rtc_notify->flags =
> + cpu_to_le32(enable ? SCMI_IMX_BBM_NOTIFY_RTC_FLAG : 0);
> + } else if (message_id == IMX_BBM_BUTTON_NOTIFY) {
> + struct scmi_msg_imx_bbm_button_notify *button_notify;
> +
> + ret = ph->xops->xfer_get_init(ph, message_id,
> + sizeof(*button_notify), 0, &t);
> + if (ret)
> + return ret;
> +
> + button_notify = t->tx.buf;
> + button_notify->flags = cpu_to_le32(enable ? 1 : 0);
> + } else {
> + return -EINVAL;
> + }
> +
> + ret = ph->xops->do_xfer(ph, t);
> +
> + ph->xops->xfer_put(ph, t);
> + return ret;
> +}
> +
> +static enum scmi_imx_bbm_protocol_cmd evt_2_cmd[] = {
> + IMX_BBM_RTC_NOTIFY,
> + IMX_BBM_BUTTON_NOTIFY
> +};
> +
> +static int scmi_imx_bbm_set_notify_enabled(const struct scmi_protocol_handle *ph,
> + u8 evt_id, u32 src_id, bool enable)
> +{
> + int ret, cmd_id;
> +
> + if (evt_id >= ARRAY_SIZE(evt_2_cmd))
> + return -EINVAL;
> +
> + cmd_id = evt_2_cmd[evt_id];
> + ret = scmi_imx_bbm_notify(ph, src_id, cmd_id, enable);
> + if (ret)
> + pr_debug("FAIL_ENABLED - evt[%X] dom[%d] - ret:%d\n",
> + evt_id, src_id, ret);
> +
> + return ret;
> +}
> +
> +static void *scmi_imx_bbm_fill_custom_report(const struct scmi_protocol_handle *ph,
> + u8 evt_id, ktime_t timestamp,
> + const void *payld, size_t payld_sz,
> + void *report, u32 *src_id)
> +{
> + const struct scmi_imx_bbm_notify_payld *p = payld;
> + struct scmi_imx_bbm_notif_report *r = report;
> +
> + if (sizeof(*p) != payld_sz)
> + return NULL;
> +
> + if (evt_id == SCMI_EVENT_IMX_BBM_RTC) {
> + r->is_rtc = true;
> + r->is_button = false;
> + r->timestamp = timestamp;
> + r->rtc_id = le32_get_bits(p->flags, SCMI_IMX_BBM_EVENT_RTC_MASK);
> + r->rtc_evt = le32_get_bits(p->flags, SCMI_IMX_BBM_NOTIFY_RTC_FLAG);
> + dev_dbg(ph->dev, "RTC: %d evt: %x\n", r->rtc_id, r->rtc_evt);
> + *src_id = r->rtc_evt;
> + } else if (evt_id == SCMI_EVENT_IMX_BBM_BUTTON) {
> + r->is_rtc = false;
> + r->is_button = true;
> + r->timestamp = timestamp;
> + dev_dbg(ph->dev, "BBM Button\n");
> + *src_id = 0;
> + } else {
> + WARN_ON_ONCE(1);
> + return NULL;
> + }
> +
> + return r;
> +}
> +
> +static const struct scmi_event scmi_imx_bbm_events[] = {
> + {
> + .id = SCMI_EVENT_IMX_BBM_RTC,
> + .max_payld_sz = sizeof(struct scmi_imx_bbm_notify_payld),
> + .max_report_sz = sizeof(struct scmi_imx_bbm_notif_report),
> + },
> + {
> + .id = SCMI_EVENT_IMX_BBM_BUTTON,
> + .max_payld_sz = sizeof(struct scmi_imx_bbm_notify_payld),
> + .max_report_sz = sizeof(struct scmi_imx_bbm_notif_report),
> + },
> +};
> +
> +static const struct scmi_event_ops scmi_imx_bbm_event_ops = {
> + .set_notify_enabled = scmi_imx_bbm_set_notify_enabled,
> + .fill_custom_report = scmi_imx_bbm_fill_custom_report,
> +};
> +
> +static const struct scmi_protocol_events scmi_imx_bbm_protocol_events = {
> + .queue_sz = SCMI_PROTO_QUEUE_SZ,
> + .ops = &scmi_imx_bbm_event_ops,
> + .evts = scmi_imx_bbm_events,
> + .num_events = ARRAY_SIZE(scmi_imx_bbm_events),
> + .num_sources = 1,
> +};
> +
> +static int scmi_imx_bbm_protocol_init(const struct scmi_protocol_handle *ph)
> +{
> + u32 version;
> + int ret;
> + struct scmi_imx_bbm_info *binfo;
> +
> + ret = ph->xops->version_get(ph, &version);
> + if (ret)
> + return ret;
> +
> + dev_info(ph->dev, "NXP SM BBM Version %d.%d\n",
> + PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
> +
> + binfo = devm_kzalloc(ph->dev, sizeof(*binfo), GFP_KERNEL);
> + if (!binfo)
> + return -ENOMEM;
> +
> + ret = scmi_imx_bbm_attributes_get(ph, binfo);
> + if (ret)
> + return ret;
> +
> + return ph->set_priv(ph, binfo, version);
> +}
> +
> +static int scmi_imx_bbm_rtc_time_set(const struct scmi_protocol_handle *ph,
> + u32 rtc_id, u64 sec)
> +{
> + struct scmi_imx_bbm_info *pi = ph->get_priv(ph);
> + struct scmi_imx_bbm_set_time *cfg;
> + struct scmi_xfer *t;
> + int ret;
> +
> + if (rtc_id >= pi->nr_rtc)
> + return -EINVAL;
> +
> + ret = ph->xops->xfer_get_init(ph, IMX_BBM_RTC_TIME_SET, sizeof(*cfg), 0, &t);
> + if (ret)
> + return ret;
> +
> + cfg = t->tx.buf;
> + cfg->id = cpu_to_le32(rtc_id);
> + cfg->flags = 0;
> + cfg->value_low = lower_32_bits(sec);
> + cfg->value_high = upper_32_bits(sec);
Sorry I may have not been clear on this, but when I mentioned lower/upper
helpers I did not mean that they solved ALSO the endianity problem, so I
suppose that after having chunked your 64bits value in 2, you still want
to transmit it as 2 LE quantity....this is generally the expectation for
SCMI payloads...in this case any available documentation about the
expected command layout would have helped...
> +
> + ret = ph->xops->do_xfer(ph, t);
> +
> + ph->xops->xfer_put(ph, t);
> +
> + return ret;
> +}
> +
> +static int scmi_imx_bbm_rtc_time_get(const struct scmi_protocol_handle *ph,
> + u32 rtc_id, u64 *value)
> +{
> + struct scmi_imx_bbm_info *pi = ph->get_priv(ph);
> + struct scmi_imx_bbm_get_time *cfg;
> + struct scmi_xfer *t;
> + int ret;
> +
> + if (rtc_id >= pi->nr_rtc)
> + return -EINVAL;
> +
> + ret = ph->xops->xfer_get_init(ph, IMX_BBM_RTC_TIME_GET, sizeof(*cfg),
> + sizeof(u64), &t);
> + if (ret)
> + return ret;
> +
> + cfg = t->tx.buf;
> + cfg->id = cpu_to_le32(rtc_id);
> + cfg->flags = 0;
> +
> + ret = ph->xops->do_xfer(ph, t);
> + if (!ret)
> + *value = get_unaligned_le64(t->rx.buf);
> +
> + ph->xops->xfer_put(ph, t);
> +
> + return ret;
> +}
> +
> +static int scmi_imx_bbm_rtc_alarm_set(const struct scmi_protocol_handle *ph,
> + u32 rtc_id, u64 sec)
> +{
> + struct scmi_imx_bbm_info *pi = ph->get_priv(ph);
> + struct scmi_imx_bbm_alarm_time *cfg;
> + struct scmi_xfer *t;
> + int ret;
> +
> + if (rtc_id >= pi->nr_rtc)
> + return -EINVAL;
> +
> + ret = ph->xops->xfer_get_init(ph, IMX_BBM_RTC_ALARM_SET, sizeof(*cfg), 0, &t);
> + if (ret)
> + return ret;
> +
> + cfg = t->tx.buf;
> + cfg->id = cpu_to_le32(rtc_id);
> + cfg->flags = SCMI_IMX_BBM_RTC_ALARM_ENABLE_FLAG;
> + cfg->value_low = lower_32_bits(sec);
> + cfg->value_high = upper_32_bits(sec);
Same.
Thanks,
Cristian
^ permalink raw reply
* RE: [PATCH v3 2/9] drm: xlnx: zynqmp_dpsub: Update live format defines
From: Klymenko, Anatoliy @ 2024-04-08 18:17 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: dri-devel@lists.freedesktop.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
linux-media@vger.kernel.org, Laurent Pinchart, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Daniel Vetter,
Simek, Michal, Andrzej Hajda, Neil Armstrong, Robert Foss,
Jonas Karlman, Jernej Skrabec, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Mauro Carvalho Chehab
In-Reply-To: <e79ca193-0452-47ab-bbdf-56d7c52130a3@ideasonboard.com>
> -----Original Message-----
> From: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> Sent: Friday, April 5, 2024 5:10 AM
> To: Klymenko, Anatoliy <Anatoliy.Klymenko@amd.com>
> Cc: dri-devel@lists.freedesktop.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-
> media@vger.kernel.org; Laurent Pinchart
> <laurent.pinchart@ideasonboard.com>; Maarten Lankhorst
> <maarten.lankhorst@linux.intel.com>; Maxime Ripard
> <mripard@kernel.org>; Thomas Zimmermann <tzimmermann@suse.de>;
> David Airlie <airlied@gmail.com>; Daniel Vetter <daniel@ffwll.ch>;
> Simek, Michal <michal.simek@amd.com>; Andrzej Hajda
> <andrzej.hajda@intel.com>; Neil Armstrong
> <neil.armstrong@linaro.org>; Robert Foss <rfoss@kernel.org>; Jonas
> Karlman <jonas@kwiboo.se>; Jernej Skrabec
> <jernej.skrabec@gmail.com>; Rob Herring <robh+dt@kernel.org>;
> Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley
> <conor+dt@kernel.org>; Mauro Carvalho Chehab
> <mchehab@kernel.org>
> Subject: Re: [PATCH v3 2/9] drm: xlnx: zynqmp_dpsub: Update live format
> defines
>
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
>
>
> On 21/03/2024 22:43, Anatoliy Klymenko wrote:
> > Update live format defines to match DPSUB AV_BUF_LIVE_VID_CONFIG
> register
> > layout.
>
> I think this description needs a bit more. Mention that the defines are
> not currently used, so we can change them like this without any other
> change.
>
Makes sense. I'll update this.
> Tomi
>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Anatoliy Klymenko <anatoliy.klymenko@amd.com>
> > ---
> > drivers/gpu/drm/xlnx/zynqmp_disp_regs.h | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
> b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
> > index f92a006d5070..fa3935384834 100644
> > --- a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
> > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
> > @@ -165,10 +165,10 @@
> > #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_10 0x2
> > #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_12 0x3
> > #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_MASK
> GENMASK(2, 0)
> > -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB 0x0
> > -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV444 0x1
> > -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422 0x2
> > -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YONLY 0x3
> > +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB (0x0
> << 4)
> > +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV444 (0x1 <<
> 4)
> > +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422 (0x2 <<
> 4)
> > +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YONLY (0x3 <<
> 4)
> > #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_MASK
> GENMASK(5, 4)
> > #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_CB_FIRST BIT(8)
> > #define ZYNQMP_DISP_AV_BUF_PALETTE_MEMORY 0x400
> >
^ permalink raw reply
* RE: [PATCH v3 1/9] drm: xlnx: zynqmp_dpsub: Set layer mode during creation
From: Klymenko, Anatoliy @ 2024-04-08 18:21 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: dri-devel@lists.freedesktop.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
linux-media@vger.kernel.org, Laurent Pinchart, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Daniel Vetter,
Simek, Michal, Andrzej Hajda, Neil Armstrong, Robert Foss,
Jonas Karlman, Jernej Skrabec, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Mauro Carvalho Chehab
In-Reply-To: <0dc8d701-3bc7-4f50-8852-5cce2b405c2e@ideasonboard.com>
> -----Original Message-----
> From: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> Sent: Friday, April 5, 2024 5:31 AM
> To: Klymenko, Anatoliy <Anatoliy.Klymenko@amd.com>
> Cc: dri-devel@lists.freedesktop.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-
> media@vger.kernel.org; Laurent Pinchart
> <laurent.pinchart@ideasonboard.com>; Maarten Lankhorst
> <maarten.lankhorst@linux.intel.com>; Maxime Ripard
> <mripard@kernel.org>; Thomas Zimmermann <tzimmermann@suse.de>;
> David Airlie <airlied@gmail.com>; Daniel Vetter <daniel@ffwll.ch>;
> Simek, Michal <michal.simek@amd.com>; Andrzej Hajda
> <andrzej.hajda@intel.com>; Neil Armstrong
> <neil.armstrong@linaro.org>; Robert Foss <rfoss@kernel.org>; Jonas
> Karlman <jonas@kwiboo.se>; Jernej Skrabec
> <jernej.skrabec@gmail.com>; Rob Herring <robh+dt@kernel.org>;
> Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley
> <conor+dt@kernel.org>; Mauro Carvalho Chehab
> <mchehab@kernel.org>
> Subject: Re: [PATCH v3 1/9] drm: xlnx: zynqmp_dpsub: Set layer mode
> during creation
>
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
>
>
> On 21/03/2024 22:43, Anatoliy Klymenko wrote:
> > Set layer mode of operation (live or dma-based) during layer creation.
> >
> > Each DPSUB layer mode of operation is defined by corresponding DT
> node port
> > connection, so it is possible to assign it during layer object creation.
> > Previously it was set in layer enable functions, although it is too late
> > as setting layer format depends on layer mode, and should be done
> before
> > given layer enabled.
> >
> > Signed-off-by: Anatoliy Klymenko <anatoliy.klymenko@amd.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > drivers/gpu/drm/xlnx/zynqmp_disp.c | 20 ++++++++++++++++----
> > drivers/gpu/drm/xlnx/zynqmp_disp.h | 13 +------------
> > drivers/gpu/drm/xlnx/zynqmp_dp.c | 2 +-
> > drivers/gpu/drm/xlnx/zynqmp_kms.c | 2 +-
> > 4 files changed, 19 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > index 8a39b3accce5..e6d26ef60e89 100644
> > --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > @@ -64,6 +64,16 @@
> >
> > #define ZYNQMP_DISP_MAX_NUM_SUB_PLANES 3
> >
> > +/**
> > + * enum zynqmp_dpsub_layer_mode - Layer mode
> > + * @ZYNQMP_DPSUB_LAYER_NONLIVE: non-live (memory) mode
> > + * @ZYNQMP_DPSUB_LAYER_LIVE: live (stream) mode
> > + */
> > +enum zynqmp_dpsub_layer_mode {
> > + ZYNQMP_DPSUB_LAYER_NONLIVE,
> > + ZYNQMP_DPSUB_LAYER_LIVE,
> > +};
> > +
> > /**
> > * struct zynqmp_disp_format - Display subsystem format information
> > * @drm_fmt: DRM format (4CC)
> > @@ -902,15 +912,12 @@ u32
> *zynqmp_disp_layer_drm_formats(struct zynqmp_disp_layer *layer,
> > /**
> > * zynqmp_disp_layer_enable - Enable a layer
> > * @layer: The layer
> > - * @mode: Operating mode of layer
> > *
> > * Enable the @layer in the audio/video buffer manager and the
> blender. DMA
> > * channels are started separately by zynqmp_disp_layer_update().
> > */
> > -void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer,
> > - enum zynqmp_dpsub_layer_mode mode)
> > +void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer)
> > {
> > - layer->mode = mode;
> > zynqmp_disp_avbuf_enable_video(layer->disp, layer);
> > zynqmp_disp_blend_layer_enable(layer->disp, layer);
> > }
> > @@ -1134,6 +1141,11 @@ static int zynqmp_disp_create_layers(struct
> zynqmp_disp *disp)
> > layer->id = i;
> > layer->disp = disp;
> > layer->info = &layer_info[i];
> > + /* For now assume dpsub works in either live or non-live
> mode for both layers.
> > + * Hybrid mode is not supported yet.
> > + */
>
> This comment style is not according to the style guide, and in fact you
> fix it in the patch 4. So please fix it here instead.
>
Thanks for catching it.
> Tomi
>
> > + layer->mode = disp->dpsub->dma_enabled ?
> ZYNQMP_DPSUB_LAYER_NONLIVE
> > + : ZYNQMP_DPSUB_LAYER_LIVE;
> >
> > ret = zynqmp_disp_layer_request_dma(disp, layer);
> > if (ret)
> > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.h
> b/drivers/gpu/drm/xlnx/zynqmp_disp.h
> > index 123cffac08be..9b8b202224d9 100644
> > --- a/drivers/gpu/drm/xlnx/zynqmp_disp.h
> > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.h
> > @@ -42,16 +42,6 @@ enum zynqmp_dpsub_layer_id {
> > ZYNQMP_DPSUB_LAYER_GFX,
> > };
> >
> > -/**
> > - * enum zynqmp_dpsub_layer_mode - Layer mode
> > - * @ZYNQMP_DPSUB_LAYER_NONLIVE: non-live (memory) mode
> > - * @ZYNQMP_DPSUB_LAYER_LIVE: live (stream) mode
> > - */
> > -enum zynqmp_dpsub_layer_mode {
> > - ZYNQMP_DPSUB_LAYER_NONLIVE,
> > - ZYNQMP_DPSUB_LAYER_LIVE,
> > -};
> > -
> > void zynqmp_disp_enable(struct zynqmp_disp *disp);
> > void zynqmp_disp_disable(struct zynqmp_disp *disp);
> > int zynqmp_disp_setup_clock(struct zynqmp_disp *disp,
> > @@ -62,8 +52,7 @@ void zynqmp_disp_blend_set_global_alpha(struct
> zynqmp_disp *disp,
> >
> > u32 *zynqmp_disp_layer_drm_formats(struct zynqmp_disp_layer
> *layer,
> > unsigned int *num_formats);
> > -void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer,
> > - enum zynqmp_dpsub_layer_mode mode);
> > +void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer);
> > void zynqmp_disp_layer_disable(struct zynqmp_disp_layer *layer);
> > void zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer,
> > const struct drm_format_info *info);
> > diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> > index 1846c4971fd8..04b6bcac3b07 100644
> > --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> > +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> > @@ -1295,7 +1295,7 @@ static void zynqmp_dp_disp_enable(struct
> zynqmp_dp *dp,
> > /* TODO: Make the format configurable. */
> > info = drm_format_info(DRM_FORMAT_YUV422);
> > zynqmp_disp_layer_set_format(layer, info);
> > - zynqmp_disp_layer_enable(layer, ZYNQMP_DPSUB_LAYER_LIVE);
> > + zynqmp_disp_layer_enable(layer);
> >
> > if (layer_id == ZYNQMP_DPSUB_LAYER_GFX)
> > zynqmp_disp_blend_set_global_alpha(dp->dpsub->disp, true,
> 255);
> > diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.c
> b/drivers/gpu/drm/xlnx/zynqmp_kms.c
> > index db3bb4afbfc4..43bf416b33d5 100644
> > --- a/drivers/gpu/drm/xlnx/zynqmp_kms.c
> > +++ b/drivers/gpu/drm/xlnx/zynqmp_kms.c
> > @@ -122,7 +122,7 @@ static void
> zynqmp_dpsub_plane_atomic_update(struct drm_plane *plane,
> >
> > /* Enable or re-enable the plane if the format has changed. */
> > if (format_changed)
> > - zynqmp_disp_layer_enable(layer,
> ZYNQMP_DPSUB_LAYER_NONLIVE);
> > + zynqmp_disp_layer_enable(layer);
> > }
> >
> > static const struct drm_plane_helper_funcs
> zynqmp_dpsub_plane_helper_funcs = {
> >
^ permalink raw reply
* Re: [PATCH v1 2/2] ASoC: meson: implement link-name optional property in meson card utils
From: Jerome Brunet @ 2024-04-08 18:15 UTC (permalink / raw)
To: Dmitry Rokosov
Cc: neil.armstrong, lgirdwood, jbrunet, broonie, conor+dt, robh+dt,
krzysztof.kozlowski+dt, perex, tiwai, khilman,
martin.blumenstingl, kernel, rockosov, linux-amlogic, alsa-devel,
linux-sound, devicetree, linux-kernel, linux-arm-kernel
In-Reply-To: <20240408164947.30717-3-ddrokosov@salutedevices.com>
On Mon 08 Apr 2024 at 19:49, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:
> The 'link-name' property presents an optional DT feature that empowers
> users to customize the name associated with the DAI link and PCM stream.
> This functionality reflects the approach often employed in Qualcomm
> audio cards, providing enhanced flexibility in DAI naming conventions
> for improved system integration and userspace experience.
>
> It allows userspace program to easy determine PCM stream purpose, e.g.:
> ~ # cat /proc/asound/pcm
> 00-00: speaker (*) : : playback 1
> 00-01: mics (*) : : capture 1
> 00-02: loopback (*) : : capture 1
The example above is exactly what you should not do with link names, at
least with the amlogic audio system.
Userspace pcm, otherwise known as DPCM frontend, are merely that:
frontends. What they do is entirely defined by the routing defined by
the userspace (amixer and friends)
So naming the interface in DT (the FW describing the HW) after what the
the userspace SW could possibly set later on is wrong.
Bottom line: I have mixed feeling about this change. It could allow all
sort of bad names to be set.
The only way it could make sense HW wise is if the only allowed names
where (fr|to)ddr_[abcd], which could help maps the interface and the
kcontrol.
Such restriction should be documented in the binding doc.
>
> The previous naming approach using auto-generated fe or be strings
> continues to be utilized as a fallback.
>
> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> ---
> sound/soc/meson/meson-card-utils.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/sound/soc/meson/meson-card-utils.c b/sound/soc/meson/meson-card-utils.c
> index ed6c7e2f609c..7bae72905a9b 100644
> --- a/sound/soc/meson/meson-card-utils.c
> +++ b/sound/soc/meson/meson-card-utils.c
> @@ -94,10 +94,14 @@ static int meson_card_set_link_name(struct snd_soc_card *card,
> struct device_node *node,
> const char *prefix)
> {
> - char *name = devm_kasprintf(card->dev, GFP_KERNEL, "%s.%s",
> - prefix, node->full_name);
> - if (!name)
> - return -ENOMEM;
> + const char *name;
> +
> + if (of_property_read_string(node, "link-name", &name)) {
> + name = devm_kasprintf(card->dev, GFP_KERNEL, "%s.%s",
> + prefix, node->full_name);
> + if (!name)
> + return -ENOMEM;
> + }
>
> link->name = name;
> link->stream_name = name;
--
Jerome
^ permalink raw reply
* Re: [PATCH 1/2] dt-bindings: pinctrl: qcom,pmic-gpio: Allow gpio-hog nodes
From: Luca Weiss @ 2024-04-08 18:36 UTC (permalink / raw)
To: ~postmarketos/upstreaming, phone-devel, Bjorn Andersson,
Linus Walleij, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Konrad Dybcio
Cc: linux-arm-msm, linux-gpio, devicetree, linux-kernel
In-Reply-To: <794f7e3c-6467-4da1-bc9f-3853459bbd78@linaro.org>
On Montag, 8. April 2024 19:26:49 CEST Konrad Dybcio wrote:
>
> On 4/8/24 18:39, Luca Weiss wrote:
> > Allow specifying a GPIO hog, as already used on
> > qcom-msm8974-lge-nexus5-hammerhead.dts.
> >
> > Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> > ---
> > .../devicetree/bindings/pinctrl/qcom,pmic-gpio.yaml | 12 ++++++++++++
> > 1 file changed, 12 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.yaml b/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.yaml
> > index a786357ed1af..510a05369dbb 100644
> > --- a/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.yaml
> > +++ b/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.yaml
> > @@ -424,6 +424,10 @@ patternProperties:
> > $ref: "#/$defs/qcom-pmic-gpio-state"
> > additionalProperties: false
> >
> > + "^(hog-[0-9]+|.+-hog(-[0-9]+)?)$":
>
> I see a couple bindings do this, but I'm not sure if we want two
> allow two styles for no reason.. Rob?
This regex is actually from the gpio-hog.yaml base
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/gpio/gpio-hog.yaml#L23
Why it's made this way I cannot tell you, but I didn't want to 'artifically'
restrict the pattern for qcom,pmic-gpio.
>
> Konrad
>
^ permalink raw reply
* Re: [PATCH v1 2/2] ASoC: meson: implement link-name optional property in meson card utils
From: Dmitry Rokosov @ 2024-04-08 18:40 UTC (permalink / raw)
To: Jerome Brunet
Cc: neil.armstrong, lgirdwood, broonie, conor+dt, robh+dt,
krzysztof.kozlowski+dt, perex, tiwai, khilman,
martin.blumenstingl, kernel, rockosov, linux-amlogic, alsa-devel,
linux-sound, devicetree, linux-kernel, linux-arm-kernel
In-Reply-To: <1j1q7fpv1n.fsf@starbuckisacylon.baylibre.com>
On Mon, Apr 08, 2024 at 08:15:54PM +0200, Jerome Brunet wrote:
>
> On Mon 08 Apr 2024 at 19:49, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:
>
> > The 'link-name' property presents an optional DT feature that empowers
> > users to customize the name associated with the DAI link and PCM stream.
> > This functionality reflects the approach often employed in Qualcomm
> > audio cards, providing enhanced flexibility in DAI naming conventions
> > for improved system integration and userspace experience.
> >
> > It allows userspace program to easy determine PCM stream purpose, e.g.:
> > ~ # cat /proc/asound/pcm
> > 00-00: speaker (*) : : playback 1
> > 00-01: mics (*) : : capture 1
> > 00-02: loopback (*) : : capture 1
>
> The example above is exactly what you should not do with link names, at
> least with the amlogic audio system.
>
> Userspace pcm, otherwise known as DPCM frontend, are merely that:
> frontends. What they do is entirely defined by the routing defined by
> the userspace (amixer and friends)
>
> So naming the interface in DT (the FW describing the HW) after what the
> the userspace SW could possibly set later on is wrong.
>
> Bottom line: I have mixed feeling about this change. It could allow all
> sort of bad names to be set.
>
> The only way it could make sense HW wise is if the only allowed names
> where (fr|to)ddr_[abcd], which could help maps the interface and the
> kcontrol.
>
> Such restriction should be documented in the binding doc.
>
The link-name is an optional parameter. Yes, you are right, it can be
routed in a way that it no longer functions as a speaker in most cases.
However, if you plan to use your board's dt for common purposes, you
should not change the common names for DAI links. But if you know that
you have a static setup for speakers, microphones, loopback, or other
references (you 100% know it, because you are HW developer of this
board), why not help the user understand the PCM device assignment in
the easiest way?
Ultimately, it is the responsibility of the DT board developer to define
specific DAIs and name them based on their own knowledge about HW and
understanding of the board's usage purposes.
> >
> > The previous naming approach using auto-generated fe or be strings
> > continues to be utilized as a fallback.
> >
> > Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> > ---
> > sound/soc/meson/meson-card-utils.c | 12 ++++++++----
> > 1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/sound/soc/meson/meson-card-utils.c b/sound/soc/meson/meson-card-utils.c
> > index ed6c7e2f609c..7bae72905a9b 100644
> > --- a/sound/soc/meson/meson-card-utils.c
> > +++ b/sound/soc/meson/meson-card-utils.c
> > @@ -94,10 +94,14 @@ static int meson_card_set_link_name(struct snd_soc_card *card,
> > struct device_node *node,
> > const char *prefix)
> > {
> > - char *name = devm_kasprintf(card->dev, GFP_KERNEL, "%s.%s",
> > - prefix, node->full_name);
> > - if (!name)
> > - return -ENOMEM;
> > + const char *name;
> > +
> > + if (of_property_read_string(node, "link-name", &name)) {
> > + name = devm_kasprintf(card->dev, GFP_KERNEL, "%s.%s",
> > + prefix, node->full_name);
> > + if (!name)
> > + return -ENOMEM;
> > + }
> >
> > link->name = name;
> > link->stream_name = name;
>
>
> --
> Jerome
--
Thank you,
Dmitry
^ permalink raw reply
* Re: [PATCH v1 2/2] ASoC: meson: implement link-name optional property in meson card utils
From: Jerome Brunet @ 2024-04-08 18:42 UTC (permalink / raw)
To: Jerome Brunet
Cc: Dmitry Rokosov, neil.armstrong, lgirdwood, broonie, conor+dt,
robh+dt, krzysztof.kozlowski+dt, perex, tiwai, khilman,
martin.blumenstingl, kernel, rockosov, linux-amlogic, alsa-devel,
linux-sound, devicetree, linux-kernel, linux-arm-kernel
In-Reply-To: <1j1q7fpv1n.fsf@starbuckisacylon.baylibre.com>
On Mon 08 Apr 2024 at 20:15, Jerome Brunet <jbrunet@baylibre.com> wrote:
> On Mon 08 Apr 2024 at 19:49, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:
>
>> The 'link-name' property presents an optional DT feature that empowers
>> users to customize the name associated with the DAI link and PCM stream.
>> This functionality reflects the approach often employed in Qualcomm
>> audio cards, providing enhanced flexibility in DAI naming conventions
>> for improved system integration and userspace experience.
>>
>> It allows userspace program to easy determine PCM stream purpose, e.g.:
>> ~ # cat /proc/asound/pcm
>> 00-00: speaker (*) : : playback 1
>> 00-01: mics (*) : : capture 1
>> 00-02: loopback (*) : : capture 1
>
> The example above is exactly what you should not do with link names, at
> least with the amlogic audio system.
>
> Userspace pcm, otherwise known as DPCM frontend, are merely that:
> frontends. What they do is entirely defined by the routing defined by
> the userspace (amixer and friends)
>
> So naming the interface in DT (the FW describing the HW) after what the
> the userspace SW could possibly set later on is wrong.
>
> Bottom line: I have mixed feeling about this change. It could allow all
> sort of bad names to be set.
>
> The only way it could make sense HW wise is if the only allowed names
> where (fr|to)ddr_[abcd], which could help maps the interface and the
> kcontrol.
>
> Such restriction should be documented in the binding doc.
>
Thinking about it further, even this does not make a lot of sense.
The information is already available from dai_name, prefixes and all.
Please use that instead if you must rename the userspace pcm, not DT.
>>
>> The previous naming approach using auto-generated fe or be strings
>> continues to be utilized as a fallback.
>>
>> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
>> ---
>> sound/soc/meson/meson-card-utils.c | 12 ++++++++----
>> 1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/sound/soc/meson/meson-card-utils.c b/sound/soc/meson/meson-card-utils.c
>> index ed6c7e2f609c..7bae72905a9b 100644
>> --- a/sound/soc/meson/meson-card-utils.c
>> +++ b/sound/soc/meson/meson-card-utils.c
>> @@ -94,10 +94,14 @@ static int meson_card_set_link_name(struct snd_soc_card *card,
>> struct device_node *node,
>> const char *prefix)
>> {
>> - char *name = devm_kasprintf(card->dev, GFP_KERNEL, "%s.%s",
>> - prefix, node->full_name);
>> - if (!name)
>> - return -ENOMEM;
>> + const char *name;
>> +
>> + if (of_property_read_string(node, "link-name", &name)) {
>> + name = devm_kasprintf(card->dev, GFP_KERNEL, "%s.%s",
>> + prefix, node->full_name);
>> + if (!name)
>> + return -ENOMEM;
>> + }
>>
>> link->name = name;
>> link->stream_name = name;
--
Jerome
^ permalink raw reply
* Re: [PATCH v1 2/2] ASoC: meson: implement link-name optional property in meson card utils
From: Mark Brown @ 2024-04-08 18:45 UTC (permalink / raw)
To: Dmitry Rokosov
Cc: Jerome Brunet, neil.armstrong, lgirdwood, conor+dt, robh+dt,
krzysztof.kozlowski+dt, perex, tiwai, khilman,
martin.blumenstingl, kernel, rockosov, linux-amlogic, alsa-devel,
linux-sound, devicetree, linux-kernel, linux-arm-kernel
In-Reply-To: <20240408184041.3jcav5tabxiblpn4@CAB-WSD-L081021>
[-- Attachment #1: Type: text/plain, Size: 1653 bytes --]
On Mon, Apr 08, 2024 at 09:40:41PM +0300, Dmitry Rokosov wrote:
> On Mon, Apr 08, 2024 at 08:15:54PM +0200, Jerome Brunet wrote:
> > Userspace pcm, otherwise known as DPCM frontend, are merely that:
> > frontends. What they do is entirely defined by the routing defined by
> > the userspace (amixer and friends)
> > So naming the interface in DT (the FW describing the HW) after what the
> > the userspace SW could possibly set later on is wrong.
> > Bottom line: I have mixed feeling about this change. It could allow all
> > sort of bad names to be set.
> > The only way it could make sense HW wise is if the only allowed names
> > where (fr|to)ddr_[abcd], which could help maps the interface and the
> > kcontrol.
> The link-name is an optional parameter. Yes, you are right, it can be
> routed in a way that it no longer functions as a speaker in most cases.
> However, if you plan to use your board's dt for common purposes, you
> should not change the common names for DAI links. But if you know that
> you have a static setup for speakers, microphones, loopback, or other
> references (you 100% know it, because you are HW developer of this
> board), why not help the user understand the PCM device assignment in
> the easiest way?
I would expect that the place to fix names based on the userspace
configuration is in whatever userspace is using to define it's
configurations, like a UCM config.
> Ultimately, it is the responsibility of the DT board developer to define
> specific DAIs and name them based on their own knowledge about HW and
> understanding of the board's usage purposes.
DT seems like the wrong abstraction layer here.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v1 2/2] ASoC: meson: implement link-name optional property in meson card utils
From: Dmitry Rokosov @ 2024-04-08 18:47 UTC (permalink / raw)
To: Mark Brown
Cc: Jerome Brunet, neil.armstrong, lgirdwood, conor+dt, robh+dt,
krzysztof.kozlowski+dt, perex, tiwai, khilman,
martin.blumenstingl, kernel, rockosov, linux-amlogic, alsa-devel,
linux-sound, devicetree, linux-kernel, linux-arm-kernel
In-Reply-To: <51b39153-d1a4-4e7f-9b30-8c77fc4ee46f@sirena.org.uk>
On Mon, Apr 08, 2024 at 07:45:00PM +0100, Mark Brown wrote:
> On Mon, Apr 08, 2024 at 09:40:41PM +0300, Dmitry Rokosov wrote:
> > On Mon, Apr 08, 2024 at 08:15:54PM +0200, Jerome Brunet wrote:
>
> > > Userspace pcm, otherwise known as DPCM frontend, are merely that:
> > > frontends. What they do is entirely defined by the routing defined by
> > > the userspace (amixer and friends)
>
> > > So naming the interface in DT (the FW describing the HW) after what the
> > > the userspace SW could possibly set later on is wrong.
>
> > > Bottom line: I have mixed feeling about this change. It could allow all
> > > sort of bad names to be set.
>
> > > The only way it could make sense HW wise is if the only allowed names
> > > where (fr|to)ddr_[abcd], which could help maps the interface and the
> > > kcontrol.
>
> > The link-name is an optional parameter. Yes, you are right, it can be
> > routed in a way that it no longer functions as a speaker in most cases.
> > However, if you plan to use your board's dt for common purposes, you
> > should not change the common names for DAI links. But if you know that
> > you have a static setup for speakers, microphones, loopback, or other
> > references (you 100% know it, because you are HW developer of this
> > board), why not help the user understand the PCM device assignment in
> > the easiest way?
>
> I would expect that the place to fix names based on the userspace
> configuration is in whatever userspace is using to define it's
> configurations, like a UCM config.
>
Honestly, I have tried to find a way to rename the PCM device name or
mark it in some way (such as using a metainformation tag or any other
method), but unfortunately, my search has been unsuccessful.
> > Ultimately, it is the responsibility of the DT board developer to define
> > specific DAIs and name them based on their own knowledge about HW and
> > understanding of the board's usage purposes.
>
> DT seems like the wrong abstraction layer here.
--
Thank you,
Dmitry
^ permalink raw reply
* Re: [PATCH v1 1/2] ASoC: dt-bindings: meson: introduce link-name optional property
From: Jerome Brunet @ 2024-04-08 18:44 UTC (permalink / raw)
To: Dmitry Rokosov
Cc: neil.armstrong, lgirdwood, jbrunet, broonie, conor+dt, robh+dt,
krzysztof.kozlowski+dt, perex, tiwai, khilman,
martin.blumenstingl, kernel, rockosov, linux-amlogic, alsa-devel,
linux-sound, devicetree, linux-kernel, linux-arm-kernel
In-Reply-To: <20240408164947.30717-2-ddrokosov@salutedevices.com>
On Mon 08 Apr 2024 at 19:49, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:
> The 'link-name' property is an optional DT property that allows for the
> customization of the name associated with the DAI link and PCM stream.
> This functionality mirrors the approach commonly utilized in Qualcomm
> audio cards, providing flexibility in DAI naming conventions for
> improved system integration and userspace experience.
As explained in patch #2, I think this change is wrong.
The names below describe a possible userspace usage of the interface,
not HW. This does not belong in DT.
The only valid name for the interface is one that helps map the PCM
interface and the kcontrols exposed in userspace.
This information is already available through dai_name, prefixes, etc.
Using DT for this is bad, in the context of these particular sound
cards at least.
>
> It allows userspace program to easy determine PCM stream purpose, e.g.:
> ~ # cat /proc/asound/pcm
> 00-00: speaker (*) : : playback 1
> 00-01: mics (*) : : capture 1
> 00-02: loopback (*) : : capture 1
>
> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> ---
> .../devicetree/bindings/sound/amlogic,axg-sound-card.yaml | 6 ++++++
> .../devicetree/bindings/sound/amlogic,gx-sound-card.yaml | 6 ++++++
> 2 files changed, 12 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/sound/amlogic,axg-sound-card.yaml b/Documentation/devicetree/bindings/sound/amlogic,axg-sound-card.yaml
> index 492b41cc8ccd..46774a3e4b1d 100644
> --- a/Documentation/devicetree/bindings/sound/amlogic,axg-sound-card.yaml
> +++ b/Documentation/devicetree/bindings/sound/amlogic,axg-sound-card.yaml
> @@ -66,6 +66,11 @@ patternProperties:
> maxItems: 1
> description: phandle of the CPU DAI
>
> + link-name:
> + description: Indicates dai-link name and PCM stream name.
> + $ref: /schemas/types.yaml#/definitions/string
> + maxItems: 1
> +
> patternProperties:
> "^dai-tdm-slot-(t|r)x-mask-[0-3]$":
> $ref: /schemas/types.yaml#/definitions/uint32-array
> @@ -137,6 +142,7 @@ examples:
>
> dai-link-0 {
> sound-dai = <&frddr_a>;
> + link-name = "speaker";
> };
>
> dai-link-1 {
> diff --git a/Documentation/devicetree/bindings/sound/amlogic,gx-sound-card.yaml b/Documentation/devicetree/bindings/sound/amlogic,gx-sound-card.yaml
> index d4277d342e69..975c148f9712 100644
> --- a/Documentation/devicetree/bindings/sound/amlogic,gx-sound-card.yaml
> +++ b/Documentation/devicetree/bindings/sound/amlogic,gx-sound-card.yaml
> @@ -52,6 +52,11 @@ patternProperties:
> maxItems: 1
> description: phandle of the CPU DAI
>
> + link-name:
> + description: Indicates dai-link name and PCM stream name.
> + $ref: /schemas/types.yaml#/definitions/string
> + maxItems: 1
> +
> patternProperties:
> "^codec(-[0-9]+)?$":
> type: object
> @@ -89,6 +94,7 @@ examples:
>
> dai-link-0 {
> sound-dai = <&i2s_fifo>;
> + link-name = "speaker";
> };
>
> dai-link-1 {
--
Jerome
^ permalink raw reply
* Re: [PATCH v1 2/2] ASoC: meson: implement link-name optional property in meson card utils
From: Mark Brown @ 2024-04-08 18:53 UTC (permalink / raw)
To: Dmitry Rokosov
Cc: Jerome Brunet, neil.armstrong, lgirdwood, conor+dt, robh+dt,
krzysztof.kozlowski+dt, perex, tiwai, khilman,
martin.blumenstingl, kernel, rockosov, linux-amlogic, alsa-devel,
linux-sound, devicetree, linux-kernel, linux-arm-kernel
In-Reply-To: <20240408184744.nfktcppdqewurmgg@CAB-WSD-L081021>
[-- Attachment #1: Type: text/plain, Size: 732 bytes --]
On Mon, Apr 08, 2024 at 09:47:44PM +0300, Dmitry Rokosov wrote:
> On Mon, Apr 08, 2024 at 07:45:00PM +0100, Mark Brown wrote:
> > I would expect that the place to fix names based on the userspace
> > configuration is in whatever userspace is using to define it's
> > configurations, like a UCM config.
> Honestly, I have tried to find a way to rename the PCM device name or
> mark it in some way (such as using a metainformation tag or any other
> method), but unfortunately, my search has been unsuccessful.
I'd not be at all surprised if there's no such facility yet in whatever
userspace you're using and that it would need implementing, I'm just
saying that that seems like a better place to solve the problem you're
seeing.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 2/2] PCI: Add Qualcomm PCIe ECAM root complex driver
From: Mayank Rana @ 2024-04-08 18:57 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: linux-pci, lpieralisi, kw, robh, bhelgaas, andersson,
krzysztof.kozlowski+dt, conor+dt, devicetree, linux-arm-msm,
quic_ramkri, quic_nkela, quic_shazhuss, quic_msarkar,
quic_nitegupt
In-Reply-To: <20240406041717.GD2678@thinkpad>
Hi Mani
On 4/5/2024 9:17 PM, Manivannan Sadhasivam wrote:
> On Fri, Apr 05, 2024 at 10:41:15AM -0700, Mayank Rana wrote:
>> Hi Mani
>>
>> On 4/4/2024 10:30 PM, Manivannan Sadhasivam wrote:
>>> On Thu, Apr 04, 2024 at 12:11:24PM -0700, Mayank Rana wrote:
>>>> On some of Qualcomm platform, firmware configures PCIe controller into
>>>> ECAM mode allowing static memory allocation for configuration space of
>>>> supported bus range. Firmware also takes care of bringing up PCIe PHY
>>>> and performing required operation to bring PCIe link into D0. Firmware
>>>> also manages system resources (e.g. clocks/regulators/resets/ bus voting).
>>>> Hence add Qualcomm PCIe ECAM root complex driver which enumerates PCIe
>>>> root complex and connected PCIe devices. Firmware won't be enumerating
>>>> or powering up PCIe root complex until this driver invokes power domain
>>>> based notification to bring PCIe link into D0/D3cold mode.
>>>>
>>>
>>> Is this an in-house PCIe IP of Qualcomm or the same DWC IP that is used in other
>>> SoCs?
>>>
>>> - Mani
>> Driver is validated on SA8775p-ride platform using PCIe DWC IP for
>> now.Although this driver doesn't need to know used PCIe controller and PHY
>> IP as well programming sequence as that would be taken care by firmware.
>>
>
> Ok, so it is the same IP but firmware is controlling the resources now. This
> information should be present in the commit message.
>
> Btw, there is an existing generic ECAM host controller driver:
> drivers/pci/controller/pci-host-generic.c
>
> This driver is already being used by several vendors as well. So we should try
> to extend it for Qcom usecase also.
I did review pci-host-generic.c driver for usage. although there are
more functionalityneeded for use case purpose as below:
1. MSI functionality
2. Suspend/Resume
3. Wakeup Functionality (not part of current change, but would be added
later)
4. Here this driver provides way to virtualized PCIe controller. So VMs
only talk to a generic ECAM whereas HW is only directed accessed by
service VM.
5. Adding more Auto based safety use cases related implementation
Hence keeping pci-host-generic.c as generic driver where above
functionality may not be needed. Also here we may add more functionality
using PM runtime based GenPD/Power Domain with SCMI communication with
firmware.
>>>> This driver also support MSI functionality using PCIe controller based
>>>> MSI controller as GIC ITS based MSI functionality is not available on
>>>> some of platform.
>>>>
>
> So is this the same internal MSI controller in the DWC IP? If so, then we
> already have the MSI implementation in
> drivers/pci/controller/dwc/pcie-designware-host.c and that should be reused here
> instead of duplicating the code.
If you are referring just MSI implementation as duplication code than I
agree with you.
Although proposed new driver is agnostic to specific PCIe controller
related IP. Currently we are using PCIe DWC controller based MSI
controller for MSI functionality using controller specific SPIs.
Although I am looking into implementation where we can use free SPIs
(there is no free SPIs available on SA877p-ride platform) or extended
SPIs to use for MSI functionality so we don't need to use PCIe
controller based MSI controller. extended SPI based MSI functionality
related work is under progress and eventually will replace current
proposed solution based MSI implementation. With that we would have
generic enough implementation for MSI functionality using free
SPIs/extended SPIs with this new driver.
> - Mani
>
Regards,
Mayank
>>>> Signed-off-by: Mayank Rana <quic_mrana@quicinc.com>
>>>> ---
>>>> drivers/pci/controller/Kconfig | 12 +
>>>> drivers/pci/controller/Makefile | 1 +
>>>> drivers/pci/controller/pcie-qcom-ecam.c | 575 ++++++++++++++++++++++++++++++++
>>>> 3 files changed, 588 insertions(+)
>>>> create mode 100644 drivers/pci/controller/pcie-qcom-ecam.c
>>>>
>>>> diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
>>>> index e534c02..abbd9f2 100644
>>>> --- a/drivers/pci/controller/Kconfig
>>>> +++ b/drivers/pci/controller/Kconfig
>>>> @@ -353,6 +353,18 @@ config PCIE_XILINX_CPM
>>>> Say 'Y' here if you want kernel support for the
>>>> Xilinx Versal CPM host bridge.
>>>> +config PCIE_QCOM_ECAM
>>>> + tristate "QCOM PCIe ECAM host controller"
>>>> + depends on ARCH_QCOM && PCI
>>>> + depends on OF
>>>> + select PCI_MSI
>>>> + select PCI_HOST_COMMON
>>>> + select IRQ_DOMAIN
>>>> + help
>>>> + Say 'Y' here if you want to use ECAM shift mode compatible Qualcomm
>>>> + PCIe root host controller. The controller is programmed using firmware
>>>> + to support ECAM compatible memory address space.
>>>> +
>>>> source "drivers/pci/controller/cadence/Kconfig"
>>>> source "drivers/pci/controller/dwc/Kconfig"
>>>> source "drivers/pci/controller/mobiveil/Kconfig"
>>>> diff --git a/drivers/pci/controller/Makefile b/drivers/pci/controller/Makefile
>>>> index f2b19e6..2f1ee1e 100644
>>>> --- a/drivers/pci/controller/Makefile
>>>> +++ b/drivers/pci/controller/Makefile
>>>> @@ -40,6 +40,7 @@ obj-$(CONFIG_PCI_LOONGSON) += pci-loongson.o
>>>> obj-$(CONFIG_PCIE_HISI_ERR) += pcie-hisi-error.o
>>>> obj-$(CONFIG_PCIE_APPLE) += pcie-apple.o
>>>> obj-$(CONFIG_PCIE_MT7621) += pcie-mt7621.o
>>>> +obj-$(CONFIG_PCIE_QCOM_ECAM) += pcie-qcom-ecam.o
>>>> # pcie-hisi.o quirks are needed even without CONFIG_PCIE_DW
>>>> obj-y += dwc/
>>>> diff --git a/drivers/pci/controller/pcie-qcom-ecam.c b/drivers/pci/controller/pcie-qcom-ecam.c
>>>> new file mode 100644
>>>> index 00000000..5b4c68b
>>>> --- /dev/null
>>>> +++ b/drivers/pci/controller/pcie-qcom-ecam.c
>>>> @@ -0,0 +1,575 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>> +/*
>>>> + * Qualcomm PCIe ECAM root host controller driver
>>>> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
>>>> + */
>>>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>>> +
>>>> +#include <linux/irq.h>
>>>> +#include <linux/irqchip/chained_irq.h>
>>>> +#include <linux/irqdomain.h>
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/msi.h>
>>>> +#include <linux/of_address.h>
>>>> +#include <linux/of_irq.h>
>>>> +#include <linux/of_pci.h>
>>>> +#include <linux/pci.h>
>>>> +#include <linux/pci-ecam.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/pm_domain.h>
>>>> +#include <linux/pm_runtime.h>
>>>> +#include <linux/slab.h>
>>>> +#include <linux/types.h>
>>>> +
>>>> +#define PCIE_MSI_CTRL_BASE (0x820)
>>>> +#define PCIE_MSI_CTRL_SIZE (0x68)
>>>> +#define PCIE_MSI_CTRL_ADDR_OFFS (0x0)
>>>> +#define PCIE_MSI_CTRL_UPPER_ADDR_OFFS (0x4)
>>>> +#define PCIE_MSI_CTRL_INT_N_EN_OFFS(n) (0x8 + 0xc * (n))
>>>> +#define PCIE_MSI_CTRL_INT_N_MASK_OFFS(n) (0xc + 0xc * (n))
>>>> +#define PCIE_MSI_CTRL_INT_N_STATUS_OFFS(n) (0x10 + 0xc * (n))
>>>> +
>>>> +#define MSI_DB_ADDR 0xa0000000
>>>> +#define MSI_IRQ_PER_GRP (32)
>>>> +
>>>> +/**
>>>> + * struct qcom_msi_irq - MSI IRQ information
>>>> + * @client: pointer to MSI client struct
>>>> + * @grp: group the irq belongs to
>>>> + * @grp_index: index in group
>>>> + * @hwirq: hwirq number
>>>> + * @virq: virq number
>>>> + * @pos: position in MSI bitmap
>>>> + */
>>>> +struct qcom_msi_irq {
>>>> + struct qcom_msi_client *client;
>>>> + struct qcom_msi_grp *grp;
>>>> + unsigned int grp_index;
>>>> + unsigned int hwirq;
>>>> + unsigned int virq;
>>>> + u32 pos;
>>>> +};
>>>> +
>>>> +/**
>>>> + * struct qcom_msi_grp - MSI group information
>>>> + * @int_en_reg: memory-mapped interrupt enable register address
>>>> + * @int_mask_reg: memory-mapped interrupt mask register address
>>>> + * @int_status_reg: memory-mapped interrupt status register address
>>>> + * @mask: tracks masked/unmasked MSI
>>>> + * @irqs: structure to MSI IRQ information
>>>> + */
>>>> +struct qcom_msi_grp {
>>>> + void __iomem *int_en_reg;
>>>> + void __iomem *int_mask_reg;
>>>> + void __iomem *int_status_reg;
>>>> + u32 mask;
>>>> + struct qcom_msi_irq irqs[MSI_IRQ_PER_GRP];
>>>> +};
>>>> +
>>>> +/**
>>>> + * struct qcom_msi - PCIe controller based MSI controller information
>>>> + * @clients: list for tracking clients
>>>> + * @dev: platform device node
>>>> + * @nr_hwirqs: total number of hardware IRQs
>>>> + * @nr_virqs: total number of virqs
>>>> + * @nr_grps: total number of groups
>>>> + * @grps: pointer to all groups information
>>>> + * @bitmap: tracks used/unused MSI
>>>> + * @mutex: for modifying MSI client list and bitmap
>>>> + * @inner_domain: parent domain; gen irq related
>>>> + * @msi_domain: child domain; pcie related
>>>> + * @msi_db_addr: MSI doorbell address
>>>> + * @cfg_lock: lock for configuring MSI controller registers
>>>> + * @pcie_msi_cfg: memory-mapped MSI controller register space
>>>> + */
>>>> +struct qcom_msi {
>>>> + struct list_head clients;
>>>> + struct device *dev;
>>>> + int nr_hwirqs;
>>>> + int nr_virqs;
>>>> + int nr_grps;
>>>> + struct qcom_msi_grp *grps;
>>>> + unsigned long *bitmap;
>>>> + struct mutex mutex;
>>>> + struct irq_domain *inner_domain;
>>>> + struct irq_domain *msi_domain;
>>>> + phys_addr_t msi_db_addr;
>>>> + spinlock_t cfg_lock;
>>>> + void __iomem *pcie_msi_cfg;
>>>> +};
>>>> +
>>>> +/**
>>>> + * struct qcom_msi_client - structure for each client of MSI controller
>>>> + * @node: list to track number of MSI clients
>>>> + * @msi: client specific MSI controller based resource pointer
>>>> + * @dev: client's dev of pci_dev
>>>> + * @nr_irqs: number of irqs allocated for client
>>>> + * @msi_addr: MSI doorbell address
>>>> + */
>>>> +struct qcom_msi_client {
>>>> + struct list_head node;
>>>> + struct qcom_msi *msi;
>>>> + struct device *dev;
>>>> + unsigned int nr_irqs;
>>>> + phys_addr_t msi_addr;
>>>> +};
>>>> +
>>>> +static void qcom_msi_handler(struct irq_desc *desc)
>>>> +{
>>>> + struct irq_chip *chip = irq_desc_get_chip(desc);
>>>> + struct qcom_msi_grp *msi_grp;
>>>> + u32 status;
>>>> + int i;
>>>> +
>>>> + chained_irq_enter(chip, desc);
>>>> +
>>>> + msi_grp = irq_desc_get_handler_data(desc);
>>>> + status = readl_relaxed(msi_grp->int_status_reg);
>>>> + status ^= (msi_grp->mask & status);
>>>> + writel(status, msi_grp->int_status_reg);
>>>> +
>>>> + for (i = 0; status; i++, status >>= 1)
>>>> + if (status & 0x1)
>>>> + generic_handle_irq(msi_grp->irqs[i].virq);
>>>> +
>>>> + chained_irq_exit(chip, desc);
>>>> +}
>>>> +
>>>> +static void qcom_msi_mask_irq(struct irq_data *data)
>>>> +{
>>>> + struct irq_data *parent_data;
>>>> + struct qcom_msi_irq *msi_irq;
>>>> + struct qcom_msi_grp *msi_grp;
>>>> + struct qcom_msi *msi;
>>>> + unsigned long flags;
>>>> +
>>>> + parent_data = data->parent_data;
>>>> + if (!parent_data)
>>>> + return;
>>>> +
>>>> + msi_irq = irq_data_get_irq_chip_data(parent_data);
>>>> + msi = msi_irq->client->msi;
>>>> + msi_grp = msi_irq->grp;
>>>> +
>>>> + spin_lock_irqsave(&msi->cfg_lock, flags);
>>>> + pci_msi_mask_irq(data);
>>>> + msi_grp->mask |= BIT(msi_irq->grp_index);
>>>> + writel(msi_grp->mask, msi_grp->int_mask_reg);
>>>> + spin_unlock_irqrestore(&msi->cfg_lock, flags);
>>>> +}
>>>> +
>>>> +static void qcom_msi_unmask_irq(struct irq_data *data)
>>>> +{
>>>> + struct irq_data *parent_data;
>>>> + struct qcom_msi_irq *msi_irq;
>>>> + struct qcom_msi_grp *msi_grp;
>>>> + struct qcom_msi *msi;
>>>> + unsigned long flags;
>>>> +
>>>> + parent_data = data->parent_data;
>>>> + if (!parent_data)
>>>> + return;
>>>> +
>>>> + msi_irq = irq_data_get_irq_chip_data(parent_data);
>>>> + msi = msi_irq->client->msi;
>>>> + msi_grp = msi_irq->grp;
>>>> +
>>>> + spin_lock_irqsave(&msi->cfg_lock, flags);
>>>> + msi_grp->mask &= ~BIT(msi_irq->grp_index);
>>>> + writel(msi_grp->mask, msi_grp->int_mask_reg);
>>>> + pci_msi_unmask_irq(data);
>>>> + spin_unlock_irqrestore(&msi->cfg_lock, flags);
>>>> +}
>>>> +
>>>> +static struct irq_chip qcom_msi_irq_chip = {
>>>> + .name = "qcom_pci_msi",
>>>> + .irq_enable = qcom_msi_unmask_irq,
>>>> + .irq_disable = qcom_msi_mask_irq,
>>>> + .irq_mask = qcom_msi_mask_irq,
>>>> + .irq_unmask = qcom_msi_unmask_irq,
>>>> +};
>>>> +
>>>> +static int qcom_msi_domain_prepare(struct irq_domain *domain, struct device *dev,
>>>> + int nvec, msi_alloc_info_t *arg)
>>>> +{
>>>> + struct qcom_msi *msi = domain->parent->host_data;
>>>> + struct qcom_msi_client *client;
>>>> +
>>>> + client = kzalloc(sizeof(*client), GFP_KERNEL);
>>>> + if (!client)
>>>> + return -ENOMEM;
>>>> +
>>>> + client->msi = msi;
>>>> + client->dev = dev;
>>>> + client->msi_addr = msi->msi_db_addr;
>>>> + mutex_lock(&msi->mutex);
>>>> + list_add_tail(&client->node, &msi->clients);
>>>> + mutex_unlock(&msi->mutex);
>>>> +
>>>> + /* zero out struct for pcie msi framework */
>>>> + memset(arg, 0, sizeof(*arg));
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static struct msi_domain_ops qcom_msi_domain_ops = {
>>>> + .msi_prepare = qcom_msi_domain_prepare,
>>>> +};
>>>> +
>>>> +static struct msi_domain_info qcom_msi_domain_info = {
>>>> + .flags = (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
>>>> + MSI_FLAG_MULTI_PCI_MSI | MSI_FLAG_PCI_MSIX),
>>>> + .ops = &qcom_msi_domain_ops,
>>>> + .chip = &qcom_msi_irq_chip,
>>>> +};
>>>> +
>>>> +static int qcom_msi_irq_set_affinity(struct irq_data *data,
>>>> + const struct cpumask *mask, bool force)
>>>> +{
>>>> + struct irq_data *parent_data = irq_get_irq_data(irqd_to_hwirq(data));
>>>> + int ret = 0;
>>>> +
>>>> + if (!parent_data)
>>>> + return -ENODEV;
>>>> +
>>>> + /* set affinity for MSI HW IRQ */
>>>> + if (parent_data->chip->irq_set_affinity)
>>>> + ret = parent_data->chip->irq_set_affinity(parent_data, mask, force);
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static void qcom_msi_irq_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
>>>> +{
>>>> + struct irq_data *parent_data = irq_get_irq_data(irqd_to_hwirq(data));
>>>> + struct qcom_msi_irq *msi_irq = irq_data_get_irq_chip_data(data);
>>>> + struct qcom_msi_client *client = msi_irq->client;
>>>> +
>>>> + if (!parent_data)
>>>> + return;
>>>> +
>>>> + msg->address_lo = lower_32_bits(client->msi_addr);
>>>> + msg->address_hi = upper_32_bits(client->msi_addr);
>>>> + msg->data = msi_irq->pos;
>>>> +}
>>>> +
>>>> +static struct irq_chip qcom_msi_bottom_irq_chip = {
>>>> + .name = "qcom_msi",
>>>> + .irq_set_affinity = qcom_msi_irq_set_affinity,
>>>> + .irq_compose_msi_msg = qcom_msi_irq_compose_msi_msg,
>>>> +};
>>>> +
>>>> +static int qcom_msi_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
>>>> + unsigned int nr_irqs, void *args)
>>>> +{
>>>> + struct device *dev = ((msi_alloc_info_t *)args)->desc->dev;
>>>> + struct qcom_msi_client *tmp, *client = NULL;
>>>> + struct qcom_msi *msi = domain->host_data;
>>>> + int i, ret = 0;
>>>> + int pos;
>>>> +
>>>> + mutex_lock(&msi->mutex);
>>>> + list_for_each_entry(tmp, &msi->clients, node) {
>>>> + if (tmp->dev == dev) {
>>>> + client = tmp;
>>>> + break;
>>>> + }
>>>> + }
>>>> +
>>>> + if (!client) {
>>>> + dev_err(msi->dev, "failed to find MSI client dev\n");
>>>> + ret = -ENODEV;
>>>> + goto out;
>>>> + }
>>>> +
>>>> + pos = bitmap_find_next_zero_area(msi->bitmap, msi->nr_virqs, 0,
>>>> + nr_irqs, nr_irqs - 1);
>>>> + if (pos > msi->nr_virqs) {
>>>> + ret = -ENOSPC;
>>>> + goto out;
>>>> + }
>>>> +
>>>> + bitmap_set(msi->bitmap, pos, nr_irqs);
>>>> + for (i = 0; i < nr_irqs; i++) {
>>>> + u32 grp = pos / MSI_IRQ_PER_GRP;
>>>> + u32 index = pos % MSI_IRQ_PER_GRP;
>>>> + struct qcom_msi_irq *msi_irq = &msi->grps[grp].irqs[index];
>>>> +
>>>> + msi_irq->virq = virq + i;
>>>> + msi_irq->client = client;
>>>> + irq_domain_set_info(domain, msi_irq->virq,
>>>> + msi_irq->hwirq,
>>>> + &qcom_msi_bottom_irq_chip, msi_irq,
>>>> + handle_simple_irq, NULL, NULL);
>>>> + client->nr_irqs++;
>>>> + pos++;
>>>> + }
>>>> +out:
>>>> + mutex_unlock(&msi->mutex);
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static void qcom_msi_irq_domain_free(struct irq_domain *domain, unsigned int virq,
>>>> + unsigned int nr_irqs)
>>>> +{
>>>> + struct irq_data *data = irq_domain_get_irq_data(domain, virq);
>>>> + struct qcom_msi_client *client;
>>>> + struct qcom_msi_irq *msi_irq;
>>>> + struct qcom_msi *msi;
>>>> +
>>>> + if (!data)
>>>> + return;
>>>> +
>>>> + msi_irq = irq_data_get_irq_chip_data(data);
>>>> + client = msi_irq->client;
>>>> + msi = client->msi;
>>>> +
>>>> + mutex_lock(&msi->mutex);
>>>> + bitmap_clear(msi->bitmap, msi_irq->pos, nr_irqs);
>>>> +
>>>> + client->nr_irqs -= nr_irqs;
>>>> + if (!client->nr_irqs) {
>>>> + list_del(&client->node);
>>>> + kfree(client);
>>>> + }
>>>> + mutex_unlock(&msi->mutex);
>>>> +
>>>> + irq_domain_free_irqs_parent(domain, virq, nr_irqs);
>>>> +}
>>>> +
>>>> +static const struct irq_domain_ops msi_domain_ops = {
>>>> + .alloc = qcom_msi_irq_domain_alloc,
>>>> + .free = qcom_msi_irq_domain_free,
>>>> +};
>>>> +
>>>> +static int qcom_msi_alloc_domains(struct qcom_msi *msi)
>>>> +{
>>>> + msi->inner_domain = irq_domain_add_linear(NULL, msi->nr_virqs,
>>>> + &msi_domain_ops, msi);
>>>> + if (!msi->inner_domain) {
>>>> + dev_err(msi->dev, "failed to create IRQ inner domain\n");
>>>> + return -ENOMEM;
>>>> + }
>>>> +
>>>> + msi->msi_domain = pci_msi_create_irq_domain(of_node_to_fwnode(msi->dev->of_node),
>>>> + &qcom_msi_domain_info, msi->inner_domain);
>>>> + if (!msi->msi_domain) {
>>>> + dev_err(msi->dev, "failed to create MSI domain\n");
>>>> + irq_domain_remove(msi->inner_domain);
>>>> + return -ENOMEM;
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int qcom_msi_irq_setup(struct qcom_msi *msi)
>>>> +{
>>>> + struct qcom_msi_grp *msi_grp;
>>>> + struct qcom_msi_irq *msi_irq;
>>>> + int i, index, ret;
>>>> + unsigned int irq;
>>>> +
>>>> + /* setup each MSI group. nr_hwirqs == nr_grps */
>>>> + for (i = 0; i < msi->nr_hwirqs; i++) {
>>>> + irq = irq_of_parse_and_map(msi->dev->of_node, i);
>>>> + if (!irq) {
>>>> + dev_err(msi->dev,
>>>> + "MSI: failed to parse/map interrupt\n");
>>>> + ret = -ENODEV;
>>>> + goto free_irqs;
>>>> + }
>>>> +
>>>> + msi_grp = &msi->grps[i];
>>>> + msi_grp->int_en_reg = msi->pcie_msi_cfg +
>>>> + PCIE_MSI_CTRL_INT_N_EN_OFFS(i);
>>>> + msi_grp->int_mask_reg = msi->pcie_msi_cfg +
>>>> + PCIE_MSI_CTRL_INT_N_MASK_OFFS(i);
>>>> + msi_grp->int_status_reg = msi->pcie_msi_cfg +
>>>> + PCIE_MSI_CTRL_INT_N_STATUS_OFFS(i);
>>>> +
>>>> + for (index = 0; index < MSI_IRQ_PER_GRP; index++) {
>>>> + msi_irq = &msi_grp->irqs[index];
>>>> +
>>>> + msi_irq->grp = msi_grp;
>>>> + msi_irq->grp_index = index;
>>>> + msi_irq->pos = (i * MSI_IRQ_PER_GRP) + index;
>>>> + msi_irq->hwirq = irq;
>>>> + }
>>>> +
>>>> + irq_set_chained_handler_and_data(irq, qcom_msi_handler, msi_grp);
>>>> + }
>>>> +
>>>> + return 0;
>>>> +
>>>> +free_irqs:
>>>> + for (--i; i >= 0; i--) {
>>>> + irq = msi->grps[i].irqs[0].hwirq;
>>>> +
>>>> + irq_set_chained_handler_and_data(irq, NULL, NULL);
>>>> + irq_dispose_mapping(irq);
>>>> + }
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static void qcom_msi_config(struct irq_domain *domain)
>>>> +{
>>>> + struct qcom_msi *msi;
>>>> + int i;
>>>> +
>>>> + msi = domain->parent->host_data;
>>>> +
>>>> + /* program termination address */
>>>> + writel(msi->msi_db_addr, msi->pcie_msi_cfg + PCIE_MSI_CTRL_ADDR_OFFS);
>>>> + writel(0, msi->pcie_msi_cfg + PCIE_MSI_CTRL_UPPER_ADDR_OFFS);
>>>> +
>>>> + /* restore mask and enable all interrupts for each group */
>>>> + for (i = 0; i < msi->nr_grps; i++) {
>>>> + struct qcom_msi_grp *msi_grp = &msi->grps[i];
>>>> +
>>>> + writel(msi_grp->mask, msi_grp->int_mask_reg);
>>>> + writel(~0, msi_grp->int_en_reg);
>>>> + }
>>>> +}
>>>> +
>>>> +static void qcom_msi_deinit(struct qcom_msi *msi)
>>>> +{
>>>> + irq_domain_remove(msi->msi_domain);
>>>> + irq_domain_remove(msi->inner_domain);
>>>> +}
>>>> +
>>>> +static struct qcom_msi *qcom_msi_init(struct device *dev)
>>>> +{
>>>> + struct qcom_msi *msi;
>>>> + u64 addr;
>>>> + int ret;
>>>> +
>>>> + msi = devm_kzalloc(dev, sizeof(*msi), GFP_KERNEL);
>>>> + if (!msi)
>>>> + return ERR_PTR(-ENOMEM);
>>>> +
>>>> + msi->dev = dev;
>>>> + mutex_init(&msi->mutex);
>>>> + spin_lock_init(&msi->cfg_lock);
>>>> + INIT_LIST_HEAD(&msi->clients);
>>>> +
>>>> + msi->msi_db_addr = MSI_DB_ADDR;
>>>> + msi->nr_hwirqs = of_irq_count(dev->of_node);
>>>> + if (!msi->nr_hwirqs) {
>>>> + dev_err(msi->dev, "no hwirqs found\n");
>>>> + return ERR_PTR(-ENODEV);
>>>> + }
>>>> +
>>>> + if (of_property_read_reg(dev->of_node, 0, &addr, NULL) < 0) {
>>>> + dev_err(msi->dev, "failed to get reg address\n");
>>>> + return ERR_PTR(-ENODEV);
>>>> + }
>>>> +
>>>> + dev_dbg(msi->dev, "hwirq:%d pcie_msi_cfg:%llx\n", msi->nr_hwirqs, addr);
>>>> + msi->pcie_msi_cfg = devm_ioremap(dev, addr + PCIE_MSI_CTRL_BASE, PCIE_MSI_CTRL_SIZE);
>>>> + if (!msi->pcie_msi_cfg)
>>>> + return ERR_PTR(-ENOMEM);
>>>> +
>>>> + msi->nr_virqs = msi->nr_hwirqs * MSI_IRQ_PER_GRP;
>>>> + msi->nr_grps = msi->nr_hwirqs;
>>>> + msi->grps = devm_kcalloc(dev, msi->nr_grps, sizeof(*msi->grps), GFP_KERNEL);
>>>> + if (!msi->grps)
>>>> + return ERR_PTR(-ENOMEM);
>>>> +
>>>> + msi->bitmap = devm_kcalloc(dev, BITS_TO_LONGS(msi->nr_virqs),
>>>> + sizeof(*msi->bitmap), GFP_KERNEL);
>>>> + if (!msi->bitmap)
>>>> + return ERR_PTR(-ENOMEM);
>>>> +
>>>> + ret = qcom_msi_alloc_domains(msi);
>>>> + if (ret)
>>>> + return ERR_PTR(ret);
>>>> +
>>>> + ret = qcom_msi_irq_setup(msi);
>>>> + if (ret) {
>>>> + qcom_msi_deinit(msi);
>>>> + return ERR_PTR(ret);
>>>> + }
>>>> +
>>>> + qcom_msi_config(msi->msi_domain);
>>>> + return msi;
>>>> +}
>>>> +
>>>> +static int qcom_pcie_ecam_suspend_noirq(struct device *dev)
>>>> +{
>>>> + return pm_runtime_put_sync(dev);
>>>> +}
>>>> +
>>>> +static int qcom_pcie_ecam_resume_noirq(struct device *dev)
>>>> +{
>>>> + return pm_runtime_get_sync(dev);
>>>> +}
>>>> +
>>>> +static int qcom_pcie_ecam_probe(struct platform_device *pdev)
>>>> +{
>>>> + struct device *dev = &pdev->dev;
>>>> + struct qcom_msi *msi;
>>>> + int ret;
>>>> +
>>>> + ret = devm_pm_runtime_enable(dev);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + ret = pm_runtime_resume_and_get(dev);
>>>> + if (ret < 0) {
>>>> + dev_err(dev, "fail to enable pcie controller: %d\n", ret);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + msi = qcom_msi_init(dev);
>>>> + if (IS_ERR(msi)) {
>>>> + pm_runtime_put_sync(dev);
>>>> + return PTR_ERR(msi);
>>>> + }
>>>> +
>>>> + ret = pci_host_common_probe(pdev);
>>>> + if (ret) {
>>>> + dev_err(dev, "pci_host_common_probe() failed:%d\n", ret);
>>>> + qcom_msi_deinit(msi);
>>>> + pm_runtime_put_sync(dev);
>>>> + }
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static const struct dev_pm_ops qcom_pcie_ecam_pm_ops = {
>>>> + NOIRQ_SYSTEM_SLEEP_PM_OPS(qcom_pcie_ecam_suspend_noirq,
>>>> + qcom_pcie_ecam_resume_noirq)
>>>> +};
>>>> +
>>>> +static const struct pci_ecam_ops qcom_pcie_ecam_ops = {
>>>> + .pci_ops = {
>>>> + .map_bus = pci_ecam_map_bus,
>>>> + .read = pci_generic_config_read,
>>>> + .write = pci_generic_config_write,
>>>> + }
>>>> +};
>>>> +
>>>> +static const struct of_device_id qcom_pcie_ecam_of_match[] = {
>>>> + {
>>>> + .compatible = "qcom,pcie-ecam-rc",
>>>> + .data = &qcom_pcie_ecam_ops,
>>>> + },
>>>> + { },
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, qcom_pcie_ecam_of_match);
>>>> +
>>>> +static struct platform_driver qcom_pcie_ecam_driver = {
>>>> + .probe = qcom_pcie_ecam_probe,
>>>> + .driver = {
>>>> + .name = "qcom-pcie-ecam-rc",
>>>> + .suppress_bind_attrs = true,
>>>> + .of_match_table = qcom_pcie_ecam_of_match,
>>>> + .probe_type = PROBE_PREFER_ASYNCHRONOUS,
>>>> + .pm = &qcom_pcie_ecam_pm_ops,
>>>> + },
>>>> +};
>>>> +module_platform_driver(qcom_pcie_ecam_driver);
>>>> +
>>>> +MODULE_DESCRIPTION("Qualcomm PCIe ECAM root complex driver");
>>>> +MODULE_LICENSE("GPL");
>>>> --
>>>> 2.7.4
>>>>
>>>
>
^ permalink raw reply
* Re: [PATCH v1 2/2] ASoC: meson: implement link-name optional property in meson card utils
From: Jerome Brunet @ 2024-04-08 18:53 UTC (permalink / raw)
To: Dmitry Rokosov
Cc: Jerome Brunet, neil.armstrong, lgirdwood, broonie, conor+dt,
robh+dt, krzysztof.kozlowski+dt, perex, tiwai, khilman,
martin.blumenstingl, kernel, rockosov, linux-amlogic, alsa-devel,
linux-sound, devicetree, linux-kernel, linux-arm-kernel
In-Reply-To: <20240408184041.3jcav5tabxiblpn4@CAB-WSD-L081021>
On Mon 08 Apr 2024 at 21:40, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:
> On Mon, Apr 08, 2024 at 08:15:54PM +0200, Jerome Brunet wrote:
>>
>> On Mon 08 Apr 2024 at 19:49, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:
>>
>> > The 'link-name' property presents an optional DT feature that empowers
>> > users to customize the name associated with the DAI link and PCM stream.
>> > This functionality reflects the approach often employed in Qualcomm
>> > audio cards, providing enhanced flexibility in DAI naming conventions
>> > for improved system integration and userspace experience.
>> >
>> > It allows userspace program to easy determine PCM stream purpose, e.g.:
>> > ~ # cat /proc/asound/pcm
>> > 00-00: speaker (*) : : playback 1
>> > 00-01: mics (*) : : capture 1
>> > 00-02: loopback (*) : : capture 1
>>
>> The example above is exactly what you should not do with link names, at
>> least with the amlogic audio system.
>>
>> Userspace pcm, otherwise known as DPCM frontend, are merely that:
>> frontends. What they do is entirely defined by the routing defined by
>> the userspace (amixer and friends)
>>
>> So naming the interface in DT (the FW describing the HW) after what the
>> the userspace SW could possibly set later on is wrong.
>>
>> Bottom line: I have mixed feeling about this change. It could allow all
>> sort of bad names to be set.
>>
>> The only way it could make sense HW wise is if the only allowed names
>> where (fr|to)ddr_[abcd], which could help maps the interface and the
>> kcontrol.
>>
>> Such restriction should be documented in the binding doc.
>>
>
> The link-name is an optional parameter. Yes, you are right, it can be
> routed in a way that it no longer functions as a speaker in most cases.
> However, if you plan to use your board's dt for common purposes, you
> should not change the common names for DAI links. But if you know that
> you have a static setup for speakers, microphones, loopback, or other
> references (you 100% know it, because you are HW developer of this
> board), why not help the user understand the PCM device assignment in
> the easiest way?
>
> Ultimately, it is the responsibility of the DT board developer to define
> specific DAIs and name them based on their own knowledge about HW and
> understanding of the board's usage purposes.
Speaker and mics are NOT statically tied to a frontend. They are tied to a
codec (... possibly). The routing from the frontend to the backend is
dynamic, even while streaming.
So defining FW names based on usage in wrong.
As Mark pointed out as well, DT is not the place for this.
>
>> >
>> > The previous naming approach using auto-generated fe or be strings
>> > continues to be utilized as a fallback.
>> >
>> > Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
>> > ---
>> > sound/soc/meson/meson-card-utils.c | 12 ++++++++----
>> > 1 file changed, 8 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/sound/soc/meson/meson-card-utils.c b/sound/soc/meson/meson-card-utils.c
>> > index ed6c7e2f609c..7bae72905a9b 100644
>> > --- a/sound/soc/meson/meson-card-utils.c
>> > +++ b/sound/soc/meson/meson-card-utils.c
>> > @@ -94,10 +94,14 @@ static int meson_card_set_link_name(struct snd_soc_card *card,
>> > struct device_node *node,
>> > const char *prefix)
>> > {
>> > - char *name = devm_kasprintf(card->dev, GFP_KERNEL, "%s.%s",
>> > - prefix, node->full_name);
>> > - if (!name)
>> > - return -ENOMEM;
>> > + const char *name;
>> > +
>> > + if (of_property_read_string(node, "link-name", &name)) {
>> > + name = devm_kasprintf(card->dev, GFP_KERNEL, "%s.%s",
>> > + prefix, node->full_name);
>> > + if (!name)
>> > + return -ENOMEM;
>> > + }
>> >
>> > link->name = name;
>> > link->stream_name = name;
>>
>>
>> --
>> Jerome
--
Jerome
^ permalink raw reply
* Re: [RFC PATCH 1/2] dt-bindings: pcie: Document QCOM PCIE ECAM compatible root complex
From: Mayank Rana @ 2024-04-08 19:09 UTC (permalink / raw)
To: Krzysztof Kozlowski, linux-pci, lpieralisi, kw, robh, bhelgaas,
andersson, manivannan.sadhasivam, krzysztof.kozlowski+dt,
conor+dt, devicetree
Cc: linux-arm-msm, quic_ramkri, quic_nkela, quic_shazhuss,
quic_msarkar, quic_nitegupt
In-Reply-To: <51b02d02-0e20-49df-ad13-e3dbe3c3214f@linaro.org>
Hi Krzysztof
On 4/4/2024 12:30 PM, Krzysztof Kozlowski wrote:
> On 04/04/2024 21:11, Mayank Rana wrote:
>> On some of Qualcomm platform, firmware configures PCIe controller in RC
>
> On which?
>
> Your commit or binding must answer to all such questions.
>
>> mode with static iATU window mappings of configuration space for entire
>> supported bus range in ECAM compatible mode. Firmware also manages PCIe
>> PHY as well required system resources. Here document properties and
>> required configuration to power up QCOM PCIe ECAM compatible root complex
>> and PHY for PCIe functionality.
>>
>> Signed-off-by: Mayank Rana <quic_mrana@quicinc.com>
>> ---
>> .../devicetree/bindings/pci/qcom,pcie-ecam.yaml | 94 ++++++++++++++++++++++
>> 1 file changed, 94 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/pci/qcom,pcie-ecam.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-ecam.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-ecam.yaml
>> new file mode 100644
>> index 00000000..c209f12
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie-ecam.yaml
>> @@ -0,0 +1,94 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/pci/qcom,pcie-ecam.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm ECAM compliant PCI express root complex
>> +
>> +description: |
> Do not need '|' unless you need to preserve formatting.
ACK
>
>> + Qualcomm SOC based ECAM compatible PCIe root complex supporting MSI controller.
>
> Which SoC?
ACK
>> + Firmware configures PCIe controller in RC mode with static iATU window mappings
>> + of configuration space for entire supported bus range in ECAM compatible mode.
>> +
>> +maintainers:
>> + - Mayank Rana <quic_mrana@quicinc.com>
>> +
>> +allOf:
>> + - $ref: /schemas/pci/pci-bus.yaml#
>> + - $ref: /schemas/power-domain/power-domain-consumer.yaml
>> +
>> +properties:
>> + compatible:
>> + const: qcom,pcie-ecam-rc
>
> No, this must have SoC specific compatibles.
This driver is proposed to work with any PCIe controller supported ECAM
functionality on Qualcomm platform
where firmware running on other VM/processor is controlling PCIe PHY and
controller for PCIe link up functionality.
Do you still suggest to have SoC specific compatibles here ?
>> +
>> + reg:
>> + minItems: 1
>
> maxItems instead
>
>> + description: ECAM address space starting from root port till supported bus range
>> +
>> + interrupts:
>> + minItems: 1
>> + maxItems: 8
>
> This is way too unspecific.
will review and update.
>> +
>> + ranges:
>> + minItems: 2
>> + maxItems: 3
>
> Why variable?
It depends on how ECAM configured to support 32-bit and 64-bit based
prefetch address space.
So there are different combination of prefetch (32-bit or 64-bit or
both) and non-prefetch (32-bit), and IO address space available. hence
kept it as variable with based on required use case and address space
availability.
>> +
>> + iommu-map:
>> + minItems: 1
>> + maxItems: 16
>
> Why variable?
>
> Open existing bindings and look how it is done.
ok. will review and update as needed.
>
>> +
>> + power-domains:
>> + maxItems: 1
>> + description: A phandle to node which is able support way to communicate with firmware
>> + for enabling PCIe controller and PHY as well managing all system resources needed to
>> + make both controller and PHY operational for PCIe functionality.
>
> This description does not tell me much. Say something specific. And drop
> redundant parts like phandle.
ok. will rephrase it.
>
>> +
>> + dma-coherent: true
>> +
>> +required:
>> + - compatible
>> + - reg
>> + - interrupts
>> + - ranges
>> + - power-domains
>> + - device_type
>> + - linux,pci-domain
>> + - bus-range
>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> + - |
>> + #include <dt-bindings/interrupt-controller/arm-gic.h>
>> + soc {
>> + #address-cells = <2>;
>> + #size-cells = <2>;
>> + pcie0: pci@1c00000 {
>> + compatible = "qcom,pcie-ecam-rc";
>> + reg = <0x4 0x00000000 0 0x10000000>;
>> + device_type = "pci";
>> + #address-cells = <3>;
>> + #size-cells = <2>;
>> + ranges = <0x01000000 0x0 0x40000000 0x0 0x40000000 0x0 0x100000>,
>> + <0x02000000 0x0 0x40100000 0x0 0x40100000 0x0 0x1ff00000>,
>> + <0x43000000 0x4 0x10100000 0x4 0x10100000 0x0 0x100000>;
>
> Follow DTS coding style about placement and alignment.
>
> Best regards,
> Krzysztof
>
Regards,
Mayank
^ permalink raw reply
* Re: [PATCH V3 5/7] clk: qcom: Add NSS clock Controller driver for IPQ9574
From: mr.nuke.me @ 2024-04-08 19:21 UTC (permalink / raw)
To: Devi Priya, andersson, konrad.dybcio, mturquette, sboyd, robh+dt,
krzysztof.kozlowski+dt, conor+dt, catalin.marinas, will, p.zabel,
richardcochran, geert+renesas, arnd, neil.armstrong, nfraprado,
m.szyprowski
Cc: linux-arm-msm, linux-clk, devicetree, linux-kernel,
linux-arm-kernel, netdev
In-Reply-To: <20240129051104.1855487-6-quic_devipriy@quicinc.com>
On 1/28/24 23:11, Devi Priya wrote:
> Add Networking Sub System Clock Controller(NSSCC) driver for ipq9574 based
> devices.
>
> Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
> ---
> Changes in V3:
> - Moved DT_XO to the beginning of the enum list
> - Hex values changed to lowercase
> - Construct mask for resets which require multiple bits to be set/cleared.
> - Dropped pm_clks as the necessary nss clocks are enabled in the
> probe function of gcc driver.
>
> drivers/clk/qcom/Kconfig | 7 +
> drivers/clk/qcom/Makefile | 1 +
> drivers/clk/qcom/nsscc-ipq9574.c | 3068 ++++++++++++++++++++++++++++++
> 3 files changed, 3076 insertions(+)
> create mode 100644 drivers/clk/qcom/nsscc-ipq9574.c
Tested-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
^ permalink raw reply
* [PATCH 0/2] Fix msm8974 apcs syscon compatible
From: Luca Weiss @ 2024-04-08 19:32 UTC (permalink / raw)
To: ~postmarketos/upstreaming, phone-devel, Jassi Brar, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Matthias Brugger, AngeloGioacchino Del Regno, Jacky Huang,
Shan-Chun Hung
Cc: linux-arm-msm, linux-kernel, devicetree, linux-arm-kernel,
linux-mediatek, Luca Weiss
Finally fix a warning about the apcs-global syscon used on msm8974 that
has been around forever.
Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
---
Luca Weiss (2):
dt-bindings: mailbox: qcom: Add MSM8974 APCS compatible
ARM: dts: qcom: msm8974: Use proper compatible for APCS syscon
.../devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml | 1 +
arch/arm/boot/dts/qcom/qcom-msm8974.dtsi | 6 ++++--
2 files changed, 5 insertions(+), 2 deletions(-)
---
base-commit: 8568bb2ccc278f344e6ac44af6ed010a90aa88dc
change-id: 20240408-msm8974-apcs-b7765f6bab99
Best regards,
--
Luca Weiss <luca@z3ntu.xyz>
^ permalink raw reply
* [PATCH 1/2] dt-bindings: mailbox: qcom: Add MSM8974 APCS compatible
From: Luca Weiss @ 2024-04-08 19:32 UTC (permalink / raw)
To: ~postmarketos/upstreaming, phone-devel, Jassi Brar, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Matthias Brugger, AngeloGioacchino Del Regno, Jacky Huang,
Shan-Chun Hung
Cc: linux-arm-msm, linux-kernel, devicetree, linux-arm-kernel,
linux-mediatek, Luca Weiss
In-Reply-To: <20240408-msm8974-apcs-v1-0-90cb7368836e@z3ntu.xyz>
Add compatible for the Qualcomm MSM8974 APCS block.
Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
---
Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml b/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml
index 79eb523b8436..982c741e6225 100644
--- a/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml
+++ b/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml
@@ -30,6 +30,7 @@ properties:
- const: syscon
- items:
- enum:
+ - qcom,msm8974-apcs-kpss-global
- qcom,msm8976-apcs-kpss-global
- const: qcom,msm8994-apcs-kpss-global
- const: syscon
--
2.44.0
^ permalink raw reply related
* [PATCH 2/2] ARM: dts: qcom: msm8974: Use proper compatible for APCS syscon
From: Luca Weiss @ 2024-04-08 19:32 UTC (permalink / raw)
To: ~postmarketos/upstreaming, phone-devel, Jassi Brar, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Matthias Brugger, AngeloGioacchino Del Regno, Jacky Huang,
Shan-Chun Hung
Cc: linux-arm-msm, linux-kernel, devicetree, linux-arm-kernel,
linux-mediatek, Luca Weiss
In-Reply-To: <20240408-msm8974-apcs-v1-0-90cb7368836e@z3ntu.xyz>
Use the apcs-kpss-global compatible for the APCS global mailbox block
found on this SoC.
This also resolves a dt-binding checker warning:
arch/arm/boot/dts/qcom/qcom-msm8974pro-fairphone-fp2.dtb: syscon@f9011000: compatible: 'anyOf' conditional failed, one must be fixed:
['syscon'] is too short
'syscon' is not one of ['allwinner,sun8i-a83t-system-controller', 'allwinner,sun8i-h3-system-controller', 'allwinner,sun8i-v3s-system-controller', 'allwinner,sun50i-a64-system-controller', 'amd,pensando-elba-syscon', 'brcm,cru-clkset', 'freecom,fsg-cs2-system-controller', 'fsl,imx93-aonmix-ns-syscfg', 'fsl,imx93-wakeupmix-syscfg', 'hisilicon,dsa-subctrl', 'hisilicon,hi6220-sramctrl', 'hisilicon,pcie-sas-subctrl', 'hisilicon,peri-subctrl', 'hpe,gxp-sysreg', 'intel,lgm-syscon', 'loongson,ls1b-syscon', 'loongson,ls1c-syscon', 'marvell,armada-3700-usb2-host-misc', 'mediatek,mt8135-pctl-a-syscfg', 'mediatek,mt8135-pctl-b-syscfg', 'mediatek,mt8365-syscfg', 'microchip,lan966x-cpu-syscon', 'microchip,sparx5-cpu-syscon', 'mstar,msc313-pmsleep', 'nuvoton,ma35d1-sys', 'nuvoton,wpcm450-shm', 'rockchip,px30-qos', 'rockchip,rk3036-qos', 'rockchip,rk3066-qos', 'rockchip,rk3128-qos', 'rockchip,rk3228-qos', 'rockchip,rk3288-qos', 'rockchip,rk3368-qos', 'rockchip,rk3399-qos', 'rockchip,rk356
8-qos', 'rockchip,rk3588-qos', 'rockchip,rv1126-qos', 'starfive,jh7100-sysmain', 'ti,am62-usb-phy-ctrl', 'ti,am654-dss-oldi-io-ctrl', 'ti,am654-serdes-ctrl', 'ti,j784s4-pcie-ctrl']
from schema $id: http://devicetree.org/schemas/mfd/syscon.yaml#
Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
---
arch/arm/boot/dts/qcom/qcom-msm8974.dtsi | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/arch/arm/boot/dts/qcom/qcom-msm8974.dtsi b/arch/arm/boot/dts/qcom/qcom-msm8974.dtsi
index 233d9bf42298..7e0224006b1f 100644
--- a/arch/arm/boot/dts/qcom/qcom-msm8974.dtsi
+++ b/arch/arm/boot/dts/qcom/qcom-msm8974.dtsi
@@ -341,9 +341,11 @@ intc: interrupt-controller@f9000000 {
<0xf9002000 0x1000>;
};
- apcs: syscon@f9011000 {
- compatible = "syscon";
+ apcs: mailbox@f9011000 {
+ compatible = "qcom,msm8974-apcs-kpss-global",
+ "qcom,msm8994-apcs-kpss-global", "syscon";
reg = <0xf9011000 0x1000>;
+ #mbox-cells = <1>;
};
saw_l2: power-manager@f9012000 {
--
2.44.0
^ permalink raw reply related
* Re: [PATCH] dt-bindings: iio: imu: mpu6050: Improve i2c-gate disallow list
From: Jean-Baptiste Maneyrol @ 2024-04-08 19:49 UTC (permalink / raw)
To: Luca Weiss, ~postmarketos/upstreaming@lists.sr.ht,
phone-devel@vger.kernel.org, Jonathan Cameron, Lars-Peter Clausen,
Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20240408-mpu6050-i2c-gate-v1-1-621f051ce7de@z3ntu.xyz>
Hello Luca,
good catch, thanks for the patch!
Acked-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
From: Luca Weiss <luca@z3ntu.xyz>
Sent: Monday, April 8, 2024 18:34
To: ~postmarketos/upstreaming@lists.sr.ht <~postmarketos/upstreaming@lists.sr.ht>; phone-devel@vger.kernel.org <phone-devel@vger.kernel.org>; Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen <lars@metafoo.de>; Rob Herring <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>; Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>
Cc: linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; devicetree@vger.kernel.org <devicetree@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; Luca Weiss <luca@z3ntu.xyz>
Subject: [PATCH] dt-bindings: iio: imu: mpu6050: Improve i2c-gate disallow list
This Message Is From an Untrusted Sender
You have not previously corresponded with this sender.
Before all supported sensors except for MPU{9150,9250,9255} were not
allowed to use i2c-gate in the bindings which excluded quite a few
supported sensors where this functionality is supported.
Switch the list of sensors to ones where the Linux driver explicitly
disallows support for the auxiliary bus ("inv_mpu_i2c_aux_bus"). Since
the driver is also based on "default: return true" this should scale
better into the future.
Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
---
This fixes dt validation error on qcom-msm8974-lge-nexus5-hammerhead
which uses mpu6515
arch/arm/boot/dts/qcom/qcom-msm8974-lge-nexus5-hammerhead.dtb: mpu6515@68: i2c-gate: False schema does not allow {'#address-cells': [[1]], '#size-cells': [[0]], 'ak8963@f': {'compatible': ['asahi-kasei,ak8963'], 'reg': [[15]], 'gpios': [[40, 67, 0]], 'vid-supply': [[50]], 'vdd-supply': [[49]]}, 'bmp280@76': {'compatible': ['bosch,bmp280'], 'reg': [[118]], 'vdda-supply': [[50]], 'vddd-supply': [[49]]}}
from schema $id: https://urldefense.com/v3/__http://devicetree.org/schemas/iio/imu/invensense,mpu6050.yaml*__;Iw!!FtrhtPsWDhZ6tw!Athn1pwCL_LPpZ97exHEFSkirApIqFF2ISY01IuyHtFBxpbPkcPWh_FmzB_TiCzb8uv1HO0AHY4IeIlv1-o$[devicetree[.]org]
---
.../devicetree/bindings/iio/imu/invensense,mpu6050.yaml | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/Documentation/devicetree/bindings/iio/imu/invensense,mpu6050.yaml b/Documentation/devicetree/bindings/iio/imu/invensense,mpu6050.yaml
index 297b8a1a7ffb..587ff2bced2d 100644
--- a/Documentation/devicetree/bindings/iio/imu/invensense,mpu6050.yaml
+++ b/Documentation/devicetree/bindings/iio/imu/invensense,mpu6050.yaml
@@ -62,14 +62,15 @@ properties:
allOf:
- $ref: /schemas/spi/spi-peripheral-props.yaml#
- if:
- not:
- properties:
- compatible:
- contains:
- enum:
- - invensense,mpu9150
- - invensense,mpu9250
- - invensense,mpu9255
+ properties:
+ compatible:
+ contains:
+ enum:
+ - invensense,iam20680
+ - invensense,icm20602
+ - invensense,icm20608
+ - invensense,icm20609
+ - invensense,icm20689
then:
properties:
i2c-gate: false
---
base-commit: 8568bb2ccc278f344e6ac44af6ed010a90aa88dc
change-id: 20240408-mpu6050-i2c-gate-4ea473e492f4
Best regards,
--
Luca Weiss <luca@z3ntu.xyz>
^ permalink raw reply related
* Re: [PATCHv3 1/2] dt-bindings: usb: typec: anx7688: start a binding document
From: Krzysztof Kozlowski @ 2024-04-08 20:12 UTC (permalink / raw)
To: Ondřej Jirman, Pavel Machek, phone-devel, kernel list,
fiona.klute, martijn, samuel, heikki.krogerus, gregkh, linux-usb,
robh+dt, krzysztof.kozlowski+dt, devicetree
In-Reply-To: <vbo7bacecuagu4qzrr6tsdh4qlejrv7ia67yylf6ay4u7qnwge@kqj27bun2m7d>
On 08/04/2024 17:17, Ondřej Jirman wrote:
> On Mon, Apr 08, 2024 at 03:27:00PM GMT, Krzysztof Kozlowski wrote:
>> On 08/04/2024 14:48, Ondřej Jirman wrote:
>>> Yeah, I understand where the confusion is. The driver is not for anx7688 chip
>>> really. The driver is named anx7688, but that's mostly a historical accident at
>>> this point.
>>>
>>> I guess there can be a driver for anx7688 chip that can directly use the chip's
>>> resources from the host by directly manipulating its registers and implementing
>>> type-c functionality via eg. Linux's TCPM or TCPCI stack, etc. (eg. like
>>> fusb302 driver, or various tcpci subdrivers).
>>>
>>> But in this case the chip is driven by an optional on-chip microcontroller's
>>> firmware and *this driver* is specifically for *the Type-C port on Pinephone*
>>
>> We do not talk here about the driver, but bindings, so hardware.
>
> Got it. Bindings should be the same regardless of what driver would be used,
> whether this OCM based one, or some future one based on the above mentioned
> TCPCI in-kernel implementation. Hardware is the same in both cases.
>
> Just trying to imagine how to actually solve the issues...
>
> Basic thing with the I2C regulator thing is that needs to be enabled as long
> as anx7688 needs to communicate over I2C. Other user of this power rail is
> touchscreen controller for its normal power supply, and it needs to be able
> to disable it during system suspend.
This does not look like anything specific to this particular device...
but even without this, please think how you want it to be solved. Same
supply which has to be on always, because your anx7688 can talk over
I2C, and in the same time sometimes off, so touchscreen can be shutdown.
If this is regulator for the I2C bus, then I think we already had this
discussion some time ago. I think it is not a property of the I2C
device, but the controller.
>
> Now for things to not fail during suspend/resume based on PM callbacks
> invocation order, anx7688 driver needs to enable this regulator too, as long
> as it needs it.
No, the I2C bus driver needs to manage it. Not one individual I2C
device. Again, why anx7688 is specific? If you next phone has anx8867,
using different driver, you also add there i2c-supply? And if it is
nxp,ptn5100 as well?
>
> I can put bus-supply to I2C controller node, and read it from the ANX7688 driver
> I guess, by going up a DT node. Whether that's going to be acceptable, I don't
> know.
>
>
> VCONN regulator I don't know where else to put either. It doesn't seem to belong
> anywhere. It's not something directly connected to Type-C connector, so
> not part of connector bindings, and there's nothing else I can see, other
> than anx7688 device which needs it for core functionality.
That sounds like a GPIO, not regulator. anx7688 has GPIOs, right? On
Pinephone they go to regulator, but on FooPhone also using anx7688 they
go somewhere else, so why this anx7688 assumes this is a regulator?
You need to properly represent the hardware, not bend it to your one
use-case for one hardware.
>
> ANX7688 chip desing doesn't have integrated VCONN mosfet switches so it always
> needs external supply + switches that are controlled by the chip itself. There's
> no sensible design where someone would not want this and the driver needs
> to get this regulator reference from somewhere. The switches are sort of an
> extension of the chip.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2] dt-bindings: PCI: altera: Convert to YAML
From: matthew.gerlach @ 2024-04-08 20:34 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: bhelgaas, lpieralisi, kw, robh, krzysztof.kozlowski+dt, conor+dt,
linux-pci, devicetree, linux-kernel
In-Reply-To: <2ece9ac2-899c-4185-b0f3-8ab939afc1e5@linaro.org>
On Sun, 7 Apr 2024, Krzysztof Kozlowski wrote:
> On 05/04/2024 16:53, matthew.gerlach@linux.intel.com wrote:
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> Convert the device tree bindings for the Altera Root Port PCIe controller
>> from text to YAML.
>>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> ---
>> v2:
>> - Move allOf: to bottom of file, just like example-schema is showing
>
> No, just open it and you will see it is placed differently...
I see what you mean. I will match the ordering of example-schema.
>
>> - add constraint for reg and reg-names
>
> Not complete...
I will complete.
>
>> - remove unneeded device_type
>> - drop #address-cells and #size-cells
>> - change minItems to maxItems for interrupts:
>> - change msi-parent to just "msi-parent: true"
>> - cleaned up required:
>> - make subject consistent with other commits coverting to YAML
>> - s/overt/onvert/g
>> ---
>> .../devicetree/bindings/pci/altera-pcie.txt | 50 ---------
>> .../bindings/pci/altr,pcie-root-port.yaml | 106 ++++++++++++++++++
>> 2 files changed, 106 insertions(+), 50 deletions(-)
>> delete mode 100644 Documentation/devicetree/bindings/pci/altera-pcie.txt
>> create mode 100644 Documentation/devicetree/bindings/pci/altr,pcie-root-port.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/pci/altera-pcie.txt b/Documentation/devicetree/bindings/pci/altera-pcie.txt
>> deleted file mode 100644
>> index 816b244a221e..000000000000
>> --- a/Documentation/devicetree/bindings/pci/altera-pcie.txt
>> +++ /dev/null
>> @@ -1,50 +0,0 @@
>> -* Altera PCIe controller
>> -
>> -Required properties:
>> -- compatible : should contain "altr,pcie-root-port-1.0" or "altr,pcie-root-port-2.0"
>> -- reg: a list of physical base address and length for TXS and CRA.
>> - For "altr,pcie-root-port-2.0", additional HIP base address and length.
>> -- reg-names: must include the following entries:
>> - "Txs": TX slave port region
>> - "Cra": Control register access region
>> - "Hip": Hard IP region (if "altr,pcie-root-port-2.0")
>> -- interrupts: specifies the interrupt source of the parent interrupt
>> - controller. The format of the interrupt specifier depends
>> - on the parent interrupt controller.
>> -- device_type: must be "pci"
>> -- #address-cells: set to <3>
>> -- #size-cells: set to <2>
>> -- #interrupt-cells: set to <1>
>> -- ranges: describes the translation of addresses for root ports and
>> - standard PCI regions.
>> -- interrupt-map-mask and interrupt-map: standard PCI properties to define the
>> - mapping of the PCIe interface to interrupt numbers.
>> -
>> -Optional properties:
>> -- msi-parent: Link to the hardware entity that serves as the MSI controller
>> - for this PCIe controller.
>> -- bus-range: PCI bus numbers covered
>> -
>> -Example
>> - pcie_0: pcie@c00000000 {
>> - compatible = "altr,pcie-root-port-1.0";
>> - reg = <0xc0000000 0x20000000>,
>> - <0xff220000 0x00004000>;
>> - reg-names = "Txs", "Cra";
>> - interrupt-parent = <&hps_0_arm_gic_0>;
>> - interrupts = <0 40 4>;
>> - interrupt-controller;
>> - #interrupt-cells = <1>;
>> - bus-range = <0x0 0xFF>;
>> - device_type = "pci";
>> - msi-parent = <&msi_to_gic_gen_0>;
>> - #address-cells = <3>;
>> - #size-cells = <2>;
>> - interrupt-map-mask = <0 0 0 7>;
>> - interrupt-map = <0 0 0 1 &pcie_0 1>,
>> - <0 0 0 2 &pcie_0 2>,
>> - <0 0 0 3 &pcie_0 3>,
>> - <0 0 0 4 &pcie_0 4>;
>> - ranges = <0x82000000 0x00000000 0x00000000 0xc0000000 0x00000000 0x10000000
>> - 0x82000000 0x00000000 0x10000000 0xd0000000 0x00000000 0x10000000>;
>> - };
>> diff --git a/Documentation/devicetree/bindings/pci/altr,pcie-root-port.yaml b/Documentation/devicetree/bindings/pci/altr,pcie-root-port.yaml
>> new file mode 100644
>> index 000000000000..999dcda05f55
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pci/altr,pcie-root-port.yaml
>> @@ -0,0 +1,106 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +# Copyright (C) 2024, Intel Corporation
>
> This is derivative of previous work, which is easily visible by doing
> the same mistakes in DTS as they were before.
This is definitely derivative of previous work, and I want to fix the
DTS mistakes too.
>
> You now added fresh copyrights ignoring all previous work, even though
> you copied it. I don't agree.
>
> If you want to ignore previous copyrights, then at least don't copy
> existing code... although even that would not be sufficient.
Ignoring previous copyrights was not my intent. There is no copyright
statement in the original text version of the device tree bindings. Should
that lack of copyright statement carry forward?
>
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/altr,pcie-root-port.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Altera PCIe Root Port
>> +
>> +maintainers:
>> + - Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> +
>> +properties:
>> + compatible:
>> + items:
>
> Drop items.
I will drop the items.
>
>> + - enum:
>> + - altr,pcie-root-port-1.0
>> + - altr,pcie-root-port-2.0
>> +
>
> Missing reg with constraints.
I will add the following here:
reg:
minItems: 2
maxItems: 3
reg-names:
minItems: 2
maxItems: 3
>
>> + interrupts:
>> + maxItems: 1
>> +
>> + interrupt-map-mask:
>> + items:
>> + - const: 0
>> + - const: 0
>> + - const: 0
>> + - const: 7
>> +
>> + interrupt-map:
>> + maxItems: 4
>> +
>> + "#interrupt-cells":
>> + const: 1
>> +
>> + msi-parent: true
>> +
>> +required:
>> + - compatible
>> + - reg
>> + - reg-names
>> + - device_type
>> + - interrupts
>> + - interrupt-map
>> + - interrupt-map-mask
>> +
>> +unevaluatedProperties: false
>> +
>> +allOf:
>> + - $ref: /schemas/pci/pci-bus.yaml#
>
> That's deprecated, as explained in its description. You should use
> pci-host-bridge.yaml.
I will switch to pci-host-bridge.yaml.
>
>
>
>> + - if:
>> + properties:
>> + compatible:
>> + enum:
>> + - altr,pcie-root-port-1.0
>> + then:
>> + properties:
>> + reg:
>> + items:
>> + - description: TX slave port region
>> + - description: Control register access region
>> +
>> + reg-names:
>> + items:
>> + - const: Txs
>> + - const: Cra
>> +
>> + else:
>> + properties:
>> + reg:
>> + items:
>> + - description: Hard IP region
>> + - description: TX slave port region
>> + - description: Control register access region
>> +
>> + reg-names:
>> + items:
>> + - const: Hip
>> + - const: Txs
>> + - const: Cra
>> +
>
> unevaluated goes here, just like example-schema.
Yes, just like the example-schema
>
>> +examples:
>> + - |
>> + #include <dt-bindings/interrupt-controller/arm-gic.h>
>> + #include <dt-bindings/interrupt-controller/irq.h>
>> + pcie_0: pcie@c00000000 {
>> + compatible = "altr,pcie-root-port-1.0";
>> + reg = <0xc0000000 0x20000000>,
>> + <0xff220000 0x00004000>;
>> + reg-names = "Txs", "Cra";
>> + interrupt-parent = <&hps_0_arm_gic_0>;
>> + interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>;
>> + #interrupt-cells = <1>;
>> + bus-range = <0x0 0xff>;
>> + device_type = "pci";
>> + msi-parent = <&msi_to_gic_gen_0>;
>> + #address-cells = <3>;
>> + #size-cells = <2>;
>> + interrupt-map-mask = <0 0 0 7>;
>> + interrupt-map = <0 0 0 1 &pcie_intc 1>,
>> + <0 0 0 2 &pcie_intc 2>,
>> + <0 0 0 3 &pcie_intc 3>,
>> + <0 0 0 4 &pcie_intc 4>;
>> + ranges = <0x82000000 0x00000000 0x00000000 0xc0000000 0x00000000 0x10000000
>> + 0x82000000 0x00000000 0x10000000 0xd0000000 0x00000000 0x10000000>;
>
> That's two entries.
I will fix the broken DTS as follows:
ranges = <0x82000000 0x00000000 0x00000000 0xc0000000 0x00000000 0x10000000>,
<0x82000000 0x00000000 0x10000000 0xd0000000 0x00000000 0x10000000>;
>
> Best regards,
> Krzysztof
>
>
Thank you for the review,
Matthew Gerlach
^ permalink raw reply
* [PATCH v14 1/4] remoteproc: zynqmp: fix lockstep mode memory region
From: Tanmay Shah @ 2024-04-08 20:53 UTC (permalink / raw)
To: andersson, mathieu.poirier, robh, krzysztof.kozlowski+dt,
conor+dt, michal.simek, ben.levinsky, tanmay.shah
Cc: linux-remoteproc, devicetree, linux-arm-kernel, linux-kernel
In-Reply-To: <20240408205313.3552165-1-tanmay.shah@amd.com>
In lockstep mode, r5 core0 uses TCM of R5 core1. Following is lockstep
mode memory region as per hardware reference manual.
| *TCM* | *R5 View* | *Linux view* |
| R5_0 ATCM (128 KB) | 0x0000_0000 | 0xFFE0_0000 |
| R5_0 BTCM (128 KB) | 0x0002_0000 | 0xFFE2_0000 |
However, driver shouldn't model it as above because R5 core0 TCM and core1
TCM has different power-domains mapped to it.
Hence, TCM address space in lockstep mode should be modeled as 64KB
regions only where each region has its own power-domain as following:
| *TCM* | *R5 View* | *Linux view* |
| R5_0 ATCM0 (64 KB) | 0x0000_0000 | 0xFFE0_0000 |
| R5_0 BTCM0 (64 KB) | 0x0002_0000 | 0xFFE2_0000 |
| R5_0 ATCM1 (64 KB) | 0x0001_0000 | 0xFFE1_0000 |
| R5_0 BTCM1 (64 KB) | 0x0003_0000 | 0xFFE3_0000 |
This makes driver maintanance easy and makes design robust for future
platorms as well.
Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
---
drivers/remoteproc/xlnx_r5_remoteproc.c | 146 ++----------------------
1 file changed, 12 insertions(+), 134 deletions(-)
diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
index cfbd97b89c26..0f942440b4e2 100644
--- a/drivers/remoteproc/xlnx_r5_remoteproc.c
+++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
@@ -84,12 +84,12 @@ static const struct mem_bank_data zynqmp_tcm_banks_split[] = {
{0xffeb0000UL, 0x20000, 0x10000UL, PD_R5_1_BTCM, "btcm1"},
};
-/* In lockstep mode cluster combines each 64KB TCM and makes 128KB TCM */
+/* In lockstep mode cluster uses each 64KB TCM from second core as well */
static const struct mem_bank_data zynqmp_tcm_banks_lockstep[] = {
- {0xffe00000UL, 0x0, 0x20000UL, PD_R5_0_ATCM, "atcm0"}, /* TCM 128KB each */
- {0xffe20000UL, 0x20000, 0x20000UL, PD_R5_0_BTCM, "btcm0"},
- {0, 0, 0, PD_R5_1_ATCM, ""},
- {0, 0, 0, PD_R5_1_BTCM, ""},
+ {0xffe00000UL, 0x0, 0x10000UL, PD_R5_0_ATCM, "atcm0"}, /* TCM 64KB each */
+ {0xffe20000UL, 0x20000, 0x10000UL, PD_R5_0_BTCM, "btcm0"},
+ {0xffe10000UL, 0x10000, 0x10000UL, PD_R5_1_ATCM, "atcm1"},
+ {0xffe30000UL, 0x30000, 0x10000UL, PD_R5_1_BTCM, "btcm1"},
};
/**
@@ -541,14 +541,14 @@ static int tcm_mem_map(struct rproc *rproc,
}
/*
- * add_tcm_carveout_split_mode()
+ * add_tcm_banks()
* @rproc: single R5 core's corresponding rproc instance
*
- * allocate and add remoteproc carveout for TCM memory in split mode
+ * allocate and add remoteproc carveout for TCM memory
*
* return 0 on success, otherwise non-zero value on failure
*/
-static int add_tcm_carveout_split_mode(struct rproc *rproc)
+static int add_tcm_banks(struct rproc *rproc)
{
struct rproc_mem_entry *rproc_mem;
struct zynqmp_r5_core *r5_core;
@@ -581,10 +581,10 @@ static int add_tcm_carveout_split_mode(struct rproc *rproc)
ZYNQMP_PM_REQUEST_ACK_BLOCKING);
if (ret < 0) {
dev_err(dev, "failed to turn on TCM 0x%x", pm_domain_id);
- goto release_tcm_split;
+ goto release_tcm;
}
- dev_dbg(dev, "TCM carveout split mode %s addr=%llx, da=0x%x, size=0x%lx",
+ dev_dbg(dev, "TCM carveout %s addr=%llx, da=0x%x, size=0x%lx",
bank_name, bank_addr, da, bank_size);
rproc_mem = rproc_mem_entry_init(dev, NULL, bank_addr,
@@ -594,99 +594,16 @@ static int add_tcm_carveout_split_mode(struct rproc *rproc)
if (!rproc_mem) {
ret = -ENOMEM;
zynqmp_pm_release_node(pm_domain_id);
- goto release_tcm_split;
- }
-
- rproc_add_carveout(rproc, rproc_mem);
- rproc_coredump_add_segment(rproc, da, bank_size);
- }
-
- return 0;
-
-release_tcm_split:
- /* If failed, Turn off all TCM banks turned on before */
- for (i--; i >= 0; i--) {
- pm_domain_id = r5_core->tcm_banks[i]->pm_domain_id;
- zynqmp_pm_release_node(pm_domain_id);
- }
- return ret;
-}
-
-/*
- * add_tcm_carveout_lockstep_mode()
- * @rproc: single R5 core's corresponding rproc instance
- *
- * allocate and add remoteproc carveout for TCM memory in lockstep mode
- *
- * return 0 on success, otherwise non-zero value on failure
- */
-static int add_tcm_carveout_lockstep_mode(struct rproc *rproc)
-{
- struct rproc_mem_entry *rproc_mem;
- struct zynqmp_r5_core *r5_core;
- int i, num_banks, ret;
- phys_addr_t bank_addr;
- size_t bank_size = 0;
- struct device *dev;
- u32 pm_domain_id;
- char *bank_name;
- u32 da;
-
- r5_core = rproc->priv;
- dev = r5_core->dev;
-
- /* Go through zynqmp banks for r5 node */
- num_banks = r5_core->tcm_bank_count;
-
- /*
- * In lockstep mode, TCM is contiguous memory block
- * However, each TCM block still needs to be enabled individually.
- * So, Enable each TCM block individually.
- * Although ATCM and BTCM is contiguous memory block, add two separate
- * carveouts for both.
- */
- for (i = 0; i < num_banks; i++) {
- pm_domain_id = r5_core->tcm_banks[i]->pm_domain_id;
-
- /* Turn on each TCM bank individually */
- ret = zynqmp_pm_request_node(pm_domain_id,
- ZYNQMP_PM_CAPABILITY_ACCESS, 0,
- ZYNQMP_PM_REQUEST_ACK_BLOCKING);
- if (ret < 0) {
- dev_err(dev, "failed to turn on TCM 0x%x", pm_domain_id);
- goto release_tcm_lockstep;
- }
-
- bank_size = r5_core->tcm_banks[i]->size;
- if (bank_size == 0)
- continue;
-
- bank_addr = r5_core->tcm_banks[i]->addr;
- da = r5_core->tcm_banks[i]->da;
- bank_name = r5_core->tcm_banks[i]->bank_name;
-
- /* Register TCM address range, TCM map and unmap functions */
- rproc_mem = rproc_mem_entry_init(dev, NULL, bank_addr,
- bank_size, da,
- tcm_mem_map, tcm_mem_unmap,
- bank_name);
- if (!rproc_mem) {
- ret = -ENOMEM;
- zynqmp_pm_release_node(pm_domain_id);
- goto release_tcm_lockstep;
+ goto release_tcm;
}
- /* If registration is success, add carveouts */
rproc_add_carveout(rproc, rproc_mem);
rproc_coredump_add_segment(rproc, da, bank_size);
-
- dev_dbg(dev, "TCM carveout lockstep mode %s addr=0x%llx, da=0x%x, size=0x%lx",
- bank_name, bank_addr, da, bank_size);
}
return 0;
-release_tcm_lockstep:
+release_tcm:
/* If failed, Turn off all TCM banks turned on before */
for (i--; i >= 0; i--) {
pm_domain_id = r5_core->tcm_banks[i]->pm_domain_id;
@@ -695,45 +612,6 @@ static int add_tcm_carveout_lockstep_mode(struct rproc *rproc)
return ret;
}
-/*
- * add_tcm_banks()
- * @rproc: single R5 core's corresponding rproc instance
- *
- * allocate and add remoteproc carveouts for TCM memory based on cluster mode
- *
- * return 0 on success, otherwise non-zero value on failure
- */
-static int add_tcm_banks(struct rproc *rproc)
-{
- struct zynqmp_r5_cluster *cluster;
- struct zynqmp_r5_core *r5_core;
- struct device *dev;
-
- r5_core = rproc->priv;
- if (!r5_core)
- return -EINVAL;
-
- dev = r5_core->dev;
-
- cluster = dev_get_drvdata(dev->parent);
- if (!cluster) {
- dev_err(dev->parent, "Invalid driver data\n");
- return -EINVAL;
- }
-
- /*
- * In lockstep mode TCM banks are one contiguous memory region of 256Kb
- * In split mode, each TCM bank is 64Kb and not contiguous.
- * We add memory carveouts accordingly.
- */
- if (cluster->mode == SPLIT_MODE)
- return add_tcm_carveout_split_mode(rproc);
- else if (cluster->mode == LOCKSTEP_MODE)
- return add_tcm_carveout_lockstep_mode(rproc);
-
- return -EINVAL;
-}
-
/*
* zynqmp_r5_parse_fw()
* @rproc: single R5 core's corresponding rproc instance
--
2.25.1
^ permalink raw reply related
* [PATCH v14 0/4] add zynqmp TCM bindings
From: Tanmay Shah @ 2024-04-08 20:53 UTC (permalink / raw)
To: andersson, mathieu.poirier, robh, krzysztof.kozlowski+dt,
conor+dt, michal.simek, ben.levinsky, tanmay.shah
Cc: linux-remoteproc, devicetree, linux-arm-kernel, linux-kernel
Tightly-Coupled Memories(TCMs) are low-latency memory that provides
predictable instruction execution and predictable data load/store
timing. Each Cortex-R5F processor contains exclusive two 64 KB memory
banks on the ATCM and BTCM ports, for a total of 128 KB of memory.
In lockstep mode, both 128KB memory is accessible to the cluster.
As per ZynqMP Ultrascale+ Technical Reference Manual UG1085, following
is address space of TCM memory. The bindings in this patch series
introduces properties to accommodate following address space with
address translation between Linux and Cortex-R5 views.
| | | |
| --- | --- | --- |
| *Mode* | *R5 View* | *Linux view* | Notes |
| *Split Mode* | *start addr*| *start addr* | |
| R5_0 ATCM (64 KB) | 0x0000_0000 | 0xFFE0_0000 | |
| R5_0 BTCM (64 KB) | 0x0002_0000 | 0xFFE2_0000 | |
| R5_1 ATCM (64 KB) | 0x0000_0000 | 0xFFE9_0000 | alias of 0xFFE1_0000 |
| R5_1 BTCM (64 KB) | 0x0002_0000 | 0xFFEB_0000 | alias of 0xFFE3_0000 |
| ___ | ___ | ___ | |
| *Lockstep Mode* | | | |
| R5_0 ATCM (128 KB) | 0x0000_0000 | 0xFFE0_0000 | |
| R5_0 BTCM (128 KB) | 0x0002_0000 | 0xFFE2_0000 | |
References:
UG1085 TCM address space:
https://docs.xilinx.com/r/en-US/ug1085-zynq-ultrascale-trm/Tightly-Coupled-Memory-Address-Map
---
prerequisite-patch-link: https://lore.kernel.org/all/d4556268-8274-4089-949f-3b97d67793c7@gmail.com/
Base Branch: 6.9.rc2
Changes in v14:
- Add xlnx,tcm-mode property and use it for TCM configuration
- Add Versal and Versal-NET platform support
- Maintain backward compatibility for ZynqMP platform and use hardcode
TCM addresses
Changes in v13:
- Have power-domains property for lockstep case instead of
keeping it flexible.
- Add "items:" list in power-domains property
Changes in v12:
- add "reg", "reg-names" and "power-domains" in pattern properties
- add "reg" and "reg-names" in required list
- keep "power-domains" in required list as it was before the change
Changes in v11:
- Fix yamllint warning and reduce indentation as needed
- Remove redundant initialization of the variable
- Return correct error code if memory allocation failed
Changs in v10:
- Add new patch (1/4) to series that changes hardcode TCM addresses in
lockstep mode and removes separate handling of TCM in lockstep and
split mode
- modify number of "reg", "reg-names" and "power-domains" entries
based on cluster mode
- Add extra optional atcm and btcm in "reg" property for lockstep mode
- Add "reg-names" for extra optional atcm and btcm for lockstep mode
- Drop previous Ack as bindings has new change
- Add individual tcm regions via "reg" and "reg-names" for lockstep mode
- Add each tcm's power-domains in lockstep mode
- Drop previous Ack as new change in dts patchset
- Remove redundant changes in driver to handle TCM in lockstep mode
Changes in v9:
- Fix rproc lockstep dts
- Introduce new API to request and release core1 TCM power-domains in
lockstep mode. This will be used during prepare -> add_tcm_banks
callback to enable TCM in lockstep mode.
- Parse TCM from device-tree in lockstep mode and split mode in
uniform way.
- Fix TCM representation in device-tree in lockstep mode.
- Fix comments as suggested
Changes in v8:
- Remove use of pm_domains framework
- Remove checking of pm_domain_id validation to power on/off tcm
- Remove spurious change
- parse power-domains property from device-tree and use EEMI calls
to power on/off TCM instead of using pm domains framework
Changes in v7:
- %s/pm_dev1/pm_dev_core0/r
- %s/pm_dev_link1/pm_dev_core0_link/r
- %s/pm_dev2/pm_dev_core1/r
- %s/pm_dev_link2/pm_dev_core1_link/r
- remove pm_domain_id check to move next patch
- add comment about how 1st entry in pm domain list is used
- fix loop when jump to fail_add_pm_domains loop
- move checking of pm_domain_id from previous patch
- fix mem_bank_data memory allocation
Changes in v6:
- Introduce new node entry for r5f cluster split mode dts and
keep it disabled by default.
- Keep remoteproc lockstep mode enabled by default to maintian
back compatibility.
- Enable split mode only for zcu102 board to demo split mode use
- Remove spurious change
- Handle errors in add_pm_domains function
- Remove redundant code to handle errors from remove_pm_domains
- Missing . at the end of the commit message
- remove redundant initialization of variables
- remove fail_tcm label and relevant code to free memory
acquired using devm_* API. As this will be freed when device free it
- add extra check to see if "reg" property is supported or not
Changes in v5:
- maintain Rob's Ack on bindings patch as no changes in bindings
- split previous patch into multiple patches
- Use pm domain framework to turn on/off TCM
- Add support of parsing TCM information from device-tree
- maintain backward compatibility with previous bindings without
TCM information available in device-tree
This patch series continues previous effort to upstream ZynqMP
TCM bindings:
Previous v4 version link:
https://lore.kernel.org/all/20230829181900.2561194-1-tanmay.shah@amd.com/
Previous v3 version link:
https://lore.kernel.org/all/1689964908-22371-1-git-send-email-radhey.shyam.pandey@amd.com/
Radhey Shyam Pandey (1):
dt-bindings: remoteproc: add Tightly Coupled Memory (TCM) bindings
Tanmay Shah (3):
remoteproc: zynqmp: fix lockstep mode memory region
dts: zynqmp: add properties for TCM in remoteproc
remoteproc: zynqmp: parse TCM from device tree
.../remoteproc/xlnx,zynqmp-r5fss.yaml | 279 +++++++++++++--
.../boot/dts/xilinx/zynqmp-zcu102-rev1.0.dts | 8 +
arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 67 +++-
drivers/remoteproc/xlnx_r5_remoteproc.c | 319 ++++++++----------
4 files changed, 471 insertions(+), 202 deletions(-)
base-commit: 39cd87c4eb2b893354f3b850f916353f2658ae6f
prerequisite-patch-id: f6c4bf78d30a332948d38e5c937f031496cd3b5a
--
2.25.1
^ permalink raw reply
* [PATCH v14 2/4] dt-bindings: remoteproc: add Tightly Coupled Memory (TCM) bindings
From: Tanmay Shah @ 2024-04-08 20:53 UTC (permalink / raw)
To: andersson, mathieu.poirier, robh, krzysztof.kozlowski+dt,
conor+dt, michal.simek, ben.levinsky, tanmay.shah
Cc: linux-remoteproc, devicetree, linux-arm-kernel, linux-kernel,
Radhey Shyam Pandey
In-Reply-To: <20240408205313.3552165-1-tanmay.shah@amd.com>
From: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
Introduce bindings for TCM memory address space on AMD-xilinx Zynq
UltraScale+ platform. It will help in defining TCM in device-tree
and make it's access platform agnostic and data-driven.
Tightly-coupled memories(TCMs) are low-latency memory that provides
predictable instruction execution and predictable data load/store
timing. Each Cortex-R5F processor contains two 64-bit wide 64 KB memory
banks on the ATCM and BTCM ports, for a total of 128 KB of memory.
The TCM resources(reg, reg-names and power-domain) are documented for
each TCM in the R5 node. The reg and reg-names are made as required
properties as we don't want to hardcode TCM addresses for future
platforms and for zu+ legacy implementation will ensure that the
old dts w/o reg/reg-names works and stable ABI is maintained.
It also extends the examples for TCM split and lockstep modes.
Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
---
Changes in v14:
- Remove previous RB tag
- Add xlnx,tcm-mode property
- Add Versal platform support
- Add Versal-NET platform support
.../remoteproc/xlnx,zynqmp-r5fss.yaml | 279 ++++++++++++++++--
1 file changed, 257 insertions(+), 22 deletions(-)
diff --git a/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml b/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
index 78aac69f1060..6f13da11f593 100644
--- a/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
+++ b/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
@@ -18,11 +18,26 @@ description: |
properties:
compatible:
- const: xlnx,zynqmp-r5fss
+ enum:
+ - xlnx,zynqmp-r5fss
+ - xlnx,versal-r5fss
+ - xlnx,versal-net-r52fss
+
+ "#address-cells":
+ const: 2
+
+ "#size-cells":
+ const: 2
+
+ ranges:
+ description: |
+ Standard ranges definition providing address translations for
+ local R5F TCM address spaces to bus addresses.
xlnx,cluster-mode:
$ref: /schemas/types.yaml#/definitions/uint32
enum: [0, 1, 2]
+ default: 1
description: |
The RPU MPCore can operate in split mode (Dual-processor performance), Safety
lock-step mode(Both RPU cores execute the same code in lock-step,
@@ -36,8 +51,16 @@ properties:
1: lockstep mode (default)
2: single cpu mode
+ xlnx,tcm-mode:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ enum: [0, 1]
+ description: |
+ Configure RPU TCM
+ 0: split mode
+ 1: lockstep mode
+
patternProperties:
- "^r5f-[a-f0-9]+$":
+ "^r(.*)@[0-9a-f]+$":
type: object
description: |
The RPU is located in the Low Power Domain of the Processor Subsystem.
@@ -52,10 +75,22 @@ patternProperties:
properties:
compatible:
- const: xlnx,zynqmp-r5f
+ enum:
+ - xlnx,zynqmp-r5f
+ - xlnx,versal-r5f
+ - xlnx,versal-net-r52f
+
+ reg:
+ minItems: 1
+ maxItems: 4
+
+ reg-names:
+ minItems: 1
+ maxItems: 4
power-domains:
- maxItems: 1
+ minItems: 2
+ maxItems: 5
mboxes:
minItems: 1
@@ -101,35 +136,235 @@ patternProperties:
required:
- compatible
+ - reg
+ - reg-names
- power-domains
- unevaluatedProperties: false
-
required:
- compatible
+ - "#address-cells"
+ - "#size-cells"
+ - ranges
+
+allOf:
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - xlnx,versal-net-r52fss
+ then:
+ properties:
+ xlnx,tcm-mode: false
+
+ patternProperties:
+ "^r52f@[0-9a-f]+$":
+ type: object
+
+ properties:
+ reg:
+ minItems: 1
+ items:
+ - description: ATCM internal memory
+ - description: BTCM internal memory
+ - description: CTCM internal memory
+
+ reg-names:
+ minItems: 1
+ items:
+ - const: atcm0
+ - const: btcm0
+ - const: ctcm0
+
+ power-domains:
+ minItems: 2
+ items:
+ - description: RPU core power domain
+ - description: ATCM power domain
+ - description: BTCM power domain
+ - description: CTCM power domain
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - xlnx,zynqmp-r5fss
+ - xlnx,versal-r5fss
+ then:
+ if:
+ properties:
+ xlnx,cluster-mode:
+ enum: [1, 2]
+ then:
+ properties:
+ xlnx,tcm-mode:
+ enum: [1]
+
+ patternProperties:
+ "^r5f@[0-9a-f]+$":
+ type: object
+
+ properties:
+ reg:
+ minItems: 1
+ items:
+ - description: ATCM internal memory
+ - description: BTCM internal memory
+ - description: extra ATCM memory in lockstep mode
+ - description: extra BTCM memory in lockstep mode
+
+ reg-names:
+ minItems: 1
+ items:
+ - const: atcm0
+ - const: btcm0
+ - const: atcm1
+ - const: btcm1
+
+ power-domains:
+ minItems: 2
+ items:
+ - description: RPU core power domain
+ - description: ATCM power domain
+ - description: BTCM power domain
+ - description: second ATCM power domain
+ - description: second BTCM power domain
+
+ required:
+ - xlnx,tcm-mode
+
+ else:
+ properties:
+ xlnx,tcm-mode:
+ enum: [0]
+
+ patternProperties:
+ "^r5f@[0-9a-f]+$":
+ type: object
+
+ properties:
+ reg:
+ minItems: 1
+ items:
+ - description: ATCM internal memory
+ - description: BTCM internal memory
+
+ reg-names:
+ minItems: 1
+ items:
+ - const: atcm0
+ - const: btcm0
+
+ power-domains:
+ minItems: 2
+ items:
+ - description: RPU core power domain
+ - description: ATCM power domain
+ - description: BTCM power domain
+
+ required:
+ - xlnx,tcm-mode
additionalProperties: false
examples:
- |
- remoteproc {
- compatible = "xlnx,zynqmp-r5fss";
- xlnx,cluster-mode = <1>;
-
- r5f-0 {
- compatible = "xlnx,zynqmp-r5f";
- power-domains = <&zynqmp_firmware 0x7>;
- memory-region = <&rproc_0_fw_image>, <&rpu0vdev0buffer>, <&rpu0vdev0vring0>, <&rpu0vdev0vring1>;
- mboxes = <&ipi_mailbox_rpu0 0>, <&ipi_mailbox_rpu0 1>;
- mbox-names = "tx", "rx";
+ #include <dt-bindings/power/xlnx-zynqmp-power.h>
+
+ // Split mode configuration
+ soc {
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ remoteproc@ffe00000 {
+ compatible = "xlnx,zynqmp-r5fss";
+ xlnx,cluster-mode = <0>;
+ xlnx,tcm-mode = <0>;
+
+ #address-cells = <2>;
+ #size-cells = <2>;
+ ranges = <0x0 0x0 0x0 0xffe00000 0x0 0x10000>,
+ <0x0 0x20000 0x0 0xffe20000 0x0 0x10000>,
+ <0x1 0x0 0x0 0xffe90000 0x0 0x10000>,
+ <0x1 0x20000 0x0 0xffeb0000 0x0 0x10000>;
+
+ r5f@0 {
+ compatible = "xlnx,zynqmp-r5f";
+ reg = <0x0 0x0 0x0 0x10000>, <0x0 0x20000 0x0 0x10000>;
+ reg-names = "atcm0", "btcm0";
+ power-domains = <&zynqmp_firmware PD_RPU_0>,
+ <&zynqmp_firmware PD_R5_0_ATCM>,
+ <&zynqmp_firmware PD_R5_0_BTCM>;
+ memory-region = <&rproc_0_fw_image>, <&rpu0vdev0buffer>,
+ <&rpu0vdev0vring0>, <&rpu0vdev0vring1>;
+ mboxes = <&ipi_mailbox_rpu0 0>, <&ipi_mailbox_rpu0 1>;
+ mbox-names = "tx", "rx";
+ };
+
+ r5f@1 {
+ compatible = "xlnx,zynqmp-r5f";
+ reg = <0x1 0x0 0x0 0x10000>, <0x1 0x20000 0x0 0x10000>;
+ reg-names = "atcm0", "btcm0";
+ power-domains = <&zynqmp_firmware PD_RPU_1>,
+ <&zynqmp_firmware PD_R5_1_ATCM>,
+ <&zynqmp_firmware PD_R5_1_BTCM>;
+ memory-region = <&rproc_1_fw_image>, <&rpu1vdev0buffer>,
+ <&rpu1vdev0vring0>, <&rpu1vdev0vring1>;
+ mboxes = <&ipi_mailbox_rpu1 0>, <&ipi_mailbox_rpu1 1>;
+ mbox-names = "tx", "rx";
+ };
};
+ };
+
+ - |
+ //Lockstep configuration
+ soc {
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ remoteproc@ffe00000 {
+ compatible = "xlnx,zynqmp-r5fss";
+ xlnx,cluster-mode = <1>;
+ xlnx,tcm-mode = <1>;
+
+ #address-cells = <2>;
+ #size-cells = <2>;
+ ranges = <0x0 0x0 0x0 0xffe00000 0x0 0x10000>,
+ <0x0 0x20000 0x0 0xffe20000 0x0 0x10000>,
+ <0x0 0x10000 0x0 0xffe10000 0x0 0x10000>,
+ <0x0 0x30000 0x0 0xffe30000 0x0 0x10000>;
+
+ r5f@0 {
+ compatible = "xlnx,zynqmp-r5f";
+ reg = <0x0 0x0 0x0 0x10000>,
+ <0x0 0x20000 0x0 0x10000>,
+ <0x0 0x10000 0x0 0x10000>,
+ <0x0 0x30000 0x0 0x10000>;
+ reg-names = "atcm0", "btcm0", "atcm1", "btcm1";
+ power-domains = <&zynqmp_firmware PD_RPU_0>,
+ <&zynqmp_firmware PD_R5_0_ATCM>,
+ <&zynqmp_firmware PD_R5_0_BTCM>,
+ <&zynqmp_firmware PD_R5_1_ATCM>,
+ <&zynqmp_firmware PD_R5_1_BTCM>;
+ memory-region = <&rproc_0_fw_image>, <&rpu0vdev0buffer>,
+ <&rpu0vdev0vring0>, <&rpu0vdev0vring1>;
+ mboxes = <&ipi_mailbox_rpu0 0>, <&ipi_mailbox_rpu0 1>;
+ mbox-names = "tx", "rx";
+ };
- r5f-1 {
- compatible = "xlnx,zynqmp-r5f";
- power-domains = <&zynqmp_firmware 0x8>;
- memory-region = <&rproc_1_fw_image>, <&rpu1vdev0buffer>, <&rpu1vdev0vring0>, <&rpu1vdev0vring1>;
- mboxes = <&ipi_mailbox_rpu1 0>, <&ipi_mailbox_rpu1 1>;
- mbox-names = "tx", "rx";
+ r5f@1 {
+ compatible = "xlnx,zynqmp-r5f";
+ reg = <0x1 0x0 0x0 0x10000>, <0x1 0x20000 0x0 0x10000>;
+ reg-names = "atcm0", "btcm0";
+ power-domains = <&zynqmp_firmware PD_RPU_1>,
+ <&zynqmp_firmware PD_R5_1_ATCM>,
+ <&zynqmp_firmware PD_R5_1_BTCM>;
+ memory-region = <&rproc_1_fw_image>, <&rpu1vdev0buffer>,
+ <&rpu1vdev0vring0>, <&rpu1vdev0vring1>;
+ mboxes = <&ipi_mailbox_rpu1 0>, <&ipi_mailbox_rpu1 1>;
+ mbox-names = "tx", "rx";
+ };
};
};
...
--
2.25.1
^ permalink raw reply related
* [PATCH v14 3/4] dts: zynqmp: add properties for TCM in remoteproc
From: Tanmay Shah @ 2024-04-08 20:53 UTC (permalink / raw)
To: andersson, mathieu.poirier, robh, krzysztof.kozlowski+dt,
conor+dt, michal.simek, ben.levinsky, tanmay.shah
Cc: linux-remoteproc, devicetree, linux-arm-kernel, linux-kernel
In-Reply-To: <20240408205313.3552165-1-tanmay.shah@amd.com>
Add properties as per new bindings in zynqmp remoteproc node
to represent TCM address and size.
This patch also adds alternative remoteproc node to represent
remoteproc cluster in split mode. By default lockstep mode is
enabled and users should disable it before using split mode
dts. Both device-tree nodes can't be used simultaneously one
of them must be disabled. For zcu102-1.0 and zcu102-1.1 board
remoteproc split mode dts node is enabled and lockstep mode
dts is disabled.
Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
---
Changes in v14:
- Add xlnx,tcm-mode property in remoteproc node
.../boot/dts/xilinx/zynqmp-zcu102-rev1.0.dts | 8 +++
arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 67 +++++++++++++++++--
2 files changed, 70 insertions(+), 5 deletions(-)
diff --git a/arch/arm64/boot/dts/xilinx/zynqmp-zcu102-rev1.0.dts b/arch/arm64/boot/dts/xilinx/zynqmp-zcu102-rev1.0.dts
index c8f71a1aec89..495ca94b45db 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp-zcu102-rev1.0.dts
+++ b/arch/arm64/boot/dts/xilinx/zynqmp-zcu102-rev1.0.dts
@@ -14,6 +14,14 @@ / {
compatible = "xlnx,zynqmp-zcu102-rev1.0", "xlnx,zynqmp-zcu102", "xlnx,zynqmp";
};
+&rproc_split {
+ status = "okay";
+};
+
+&rproc_lockstep {
+ status = "disabled";
+};
+
&eeprom {
#address-cells = <1>;
#size-cells = <1>;
diff --git a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
index 25d20d803230..ef31b0fc73d1 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
+++ b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
@@ -260,19 +260,76 @@ fpga_full: fpga-full {
ranges;
};
- remoteproc {
+ rproc_lockstep: remoteproc@ffe00000 {
compatible = "xlnx,zynqmp-r5fss";
xlnx,cluster-mode = <1>;
+ xlnx,tcm-mode = <1>;
- r5f-0 {
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ ranges = <0x0 0x0 0x0 0xffe00000 0x0 0x10000>,
+ <0x0 0x20000 0x0 0xffe20000 0x0 0x10000>,
+ <0x0 0x10000 0x0 0xffe10000 0x0 0x10000>,
+ <0x0 0x30000 0x0 0xffe30000 0x0 0x10000>;
+
+ r5f@0 {
+ compatible = "xlnx,zynqmp-r5f";
+ reg = <0x0 0x0 0x0 0x10000>,
+ <0x0 0x20000 0x0 0x10000>,
+ <0x0 0x10000 0x0 0x10000>,
+ <0x0 0x30000 0x0 0x10000>;
+ reg-names = "atcm0", "btcm0", "atcm1", "btcm1";
+ power-domains = <&zynqmp_firmware PD_RPU_0>,
+ <&zynqmp_firmware PD_R5_0_ATCM>,
+ <&zynqmp_firmware PD_R5_0_BTCM>,
+ <&zynqmp_firmware PD_R5_1_ATCM>,
+ <&zynqmp_firmware PD_R5_1_BTCM>;
+ memory-region = <&rproc_0_fw_image>;
+ };
+
+ r5f@1 {
+ compatible = "xlnx,zynqmp-r5f";
+ reg = <0x1 0x0 0x0 0x10000>, <0x1 0x20000 0x0 0x10000>;
+ reg-names = "atcm0", "btcm0";
+ power-domains = <&zynqmp_firmware PD_RPU_1>,
+ <&zynqmp_firmware PD_R5_1_ATCM>,
+ <&zynqmp_firmware PD_R5_1_BTCM>;
+ memory-region = <&rproc_1_fw_image>;
+ };
+ };
+
+ rproc_split: remoteproc-split@ffe00000 {
+ status = "disabled";
+ compatible = "xlnx,zynqmp-r5fss";
+ xlnx,cluster-mode = <0>;
+ xlnx,tcm-mode = <0>;
+
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ ranges = <0x0 0x0 0x0 0xffe00000 0x0 0x10000>,
+ <0x0 0x20000 0x0 0xffe20000 0x0 0x10000>,
+ <0x1 0x0 0x0 0xffe90000 0x0 0x10000>,
+ <0x1 0x20000 0x0 0xffeb0000 0x0 0x10000>;
+
+ r5f@0 {
compatible = "xlnx,zynqmp-r5f";
- power-domains = <&zynqmp_firmware PD_RPU_0>;
+ reg = <0x0 0x0 0x0 0x10000>, <0x0 0x20000 0x0 0x10000>;
+ reg-names = "atcm0", "btcm0";
+ power-domains = <&zynqmp_firmware PD_RPU_0>,
+ <&zynqmp_firmware PD_R5_0_ATCM>,
+ <&zynqmp_firmware PD_R5_0_BTCM>;
memory-region = <&rproc_0_fw_image>;
};
- r5f-1 {
+ r5f@1 {
compatible = "xlnx,zynqmp-r5f";
- power-domains = <&zynqmp_firmware PD_RPU_1>;
+ reg = <0x1 0x0 0x0 0x10000>, <0x1 0x20000 0x0 0x10000>;
+ reg-names = "atcm0", "btcm0";
+ power-domains = <&zynqmp_firmware PD_RPU_1>,
+ <&zynqmp_firmware PD_R5_1_ATCM>,
+ <&zynqmp_firmware PD_R5_1_BTCM>;
memory-region = <&rproc_1_fw_image>;
};
};
--
2.25.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox