* 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 = {
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 1/5] clk: scmi: Allocate CLK operations dynamically
From: Cristian Marussi @ 2024-04-08 18:23 UTC (permalink / raw)
To: Stephen Boyd
Cc: linux-arm-kernel, linux-clk, linux-kernel, sudeep.holla,
james.quinlan, f.fainelli, vincent.guittot, peng.fan,
michal.simek, quic_sibis, quic_nkela, souvik.chakravarty,
mturquette
In-Reply-To: <7027a28723d2597d9f620f4e0e1da97e.sboyd@kernel.org>
On Sun, Apr 07, 2024 at 09:38:46PM -0700, Stephen Boyd wrote:
> Quoting Cristian Marussi (2024-03-25 14:00:21)
> > diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
> > index 8cbe24789c24..d5d369b052bd 100644
> > --- a/drivers/clk/clk-scmi.c
> > +++ b/drivers/clk/clk-scmi.c
> > @@ -16,6 +16,14 @@
> > #define NOT_ATOMIC false
> > #define ATOMIC true
> >
Hi,
thanks for the review.
> > +enum scmi_clk_feats {
> > + SCMI_CLK_ATOMIC_SUPPORTED,
> > + SCMI_CLK_MAX_FEATS
> > +};
> > +
> > +#define SCMI_MAX_CLK_OPS (1 << SCMI_CLK_MAX_FEATS)
> > +
> > +static const struct clk_ops *clk_ops_db[SCMI_MAX_CLK_OPS];
>
> Can it be 'scmi_clk_ops_db' for some name spacing?
>
Yes.
> > static const struct scmi_clk_proto_ops *scmi_proto_clk_ops;
> >
> > struct scmi_clk {
> > @@ -230,6 +202,106 @@ static int scmi_clk_ops_init(struct device *dev, struct scmi_clk *sclk,
> > return ret;
> > }
> >
> > +/**
> > + * scmi_clk_ops_alloc() - Alloc and configure clock operations
> > + * @dev: A device reference for devres
> > + * @feats_key: A bitmap representing the desired clk_ops capabilities.
>
> Drop the period please because it's not consistent with the previous
> argument descriptor.
>
Ok.
> > + *
> > + * Allocate and configure a proper set of clock operations depending on the
> > + * specifically required SCMI clock features.
> > + *
> > + * Return: A pointer to the allocated and configured clk_ops on Success,
>
> Lowercase 'Success'.
>
Ok.
> > +
> > +/**
> > + * scmi_clk_ops_select() - Select a proper set of clock operations
> > + * @sclk: A reference to an SCMI clock descriptor
> > + * @atomic_capable: A flag to indicate if atomic mode is supported by the
> > + * transport
> > + * @atomic_threshold: Platform atomic threshold value
>
> Is this in nanoseconds, microseconds, or ??? Maybe a better description is
> "clk_ops are atomic when clk enable_latency is less than X [time unit]"
>
It is micro, I will comment better.
> > + *
> > + * After having built a bitmap descriptor to represent the set of features
> > + * needed by this SCMI clock, at first use it to lookup into the set of
> > + * previously allocated clk_ops to check if a suitable combination of clock
> > + * operations was already created; when no match is found allocate a brand new
> > + * set of clk_ops satisfying the required combination of features and save it
> > + * for future references.
> > + *
> > + * In this way only one set of clk_ops is ever created for each different
> > + * combination that is effectively needed.
> > + *
> > + * Return: A pointer to the allocated and configured clk_ops on Success, or
>
> Lowercase 'Success'.
>
Ok.
> > + * NULL otherwise.
> > + */
> > +static const struct clk_ops *
> > +scmi_clk_ops_select(struct scmi_clk *sclk, bool atomic_capable,
> > + unsigned int atomic_threshold)
> > +{
> > + const struct scmi_clock_info *ci = sclk->info;
> > + unsigned int feats_key = 0;
> > + const struct clk_ops *ops;
> > +
> > + /*
> > + * Note that when transport is atomic but SCMI protocol did not
> > + * specify (or support) an enable_latency associated with a
> > + * clock, we default to use atomic operations mode.
> > + */
> > + if (atomic_capable && ci->enable_latency <= atomic_threshold)
> > + feats_key |= BIT(SCMI_CLK_ATOMIC_SUPPORTED);
> > +
>
> Can we have a static_assert() here that makes sure 'feats_key' isn't
> larger than the size of clk_ops_db?
>
> static_assert(ARRAY_SIZE(clk_ops_db) >= feats_key);
>
Ok.
> > + /* Lookup previously allocated ops */
> > + ops = clk_ops_db[feats_key];
> > + if (!ops) {
> > + ops = scmi_clk_ops_alloc(sclk->dev, feats_key);
> > + if (!ops)
> > + return NULL;
>
> This could be less nested if the first lookup is put in
> scmi_clk_ops_alloc() and the store below is folded in. Or an early
> return if found.
>
> ops = clk_ops_db[feats_key];
> if (ops)
> return ops;
>
> /* Didn't find one */
> ops = scmi_clk_ops_alloc(...)
> if (!ops)
> return NULL;
>
> clk_ops_db[feats_key] = ops;
> return ops;
>
ok.
> > +
> > + /* Store new ops combinations */
> > + clk_ops_db[feats_key] = ops;
> > + }
> > +
> > + return ops;
> > +}
> > +
> > static int scmi_clocks_probe(struct scmi_device *sdev)
> > {
> > int idx, count, err;
> > @@ -285,16 +357,10 @@ static int scmi_clocks_probe(struct scmi_device *sdev)
> > sclk->ph = ph;
> > sclk->dev = dev;
> >
> > - /*
> > - * Note that when transport is atomic but SCMI protocol did not
> > - * specify (or support) an enable_latency associated with a
> > - * clock, we default to use atomic operations mode.
> > - */
> > - if (is_atomic &&
> > - sclk->info->enable_latency <= atomic_threshold)
> > - scmi_ops = &scmi_atomic_clk_ops;
> > - else
> > - scmi_ops = &scmi_clk_ops;
> > + scmi_ops = scmi_clk_ops_select(sclk, is_atomic,
>
> 'is_atomic' should probably be 'transport_is_atomic' so this reads
> easier.
>
Ok.
Thanks,
Cristian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 2/5] clk: scmi: Add support for state control restricted clocks
From: Cristian Marussi @ 2024-04-08 18:26 UTC (permalink / raw)
To: Stephen Boyd
Cc: linux-arm-kernel, linux-clk, linux-kernel, sudeep.holla,
james.quinlan, f.fainelli, vincent.guittot, peng.fan,
michal.simek, quic_sibis, quic_nkela, souvik.chakravarty,
mturquette
In-Reply-To: <90ac6d2dce33e8078db7cb5681fb94d7.sboyd@kernel.org>
On Sun, Apr 07, 2024 at 09:48:59PM -0700, Stephen Boyd wrote:
> Quoting Cristian Marussi (2024-03-25 14:00:22)
> > Some exposed SCMI Clocks could be marked as non-supporting state changes.
> > Configure a clk_ops descriptor which does not provide the state change
> > callbacks for such clocks when registering with CLK framework.
> >
> > CC: Michael Turquette <mturquette@baylibre.com>
> > CC: Stephen Boyd <sboyd@kernel.org>
> > CC: linux-clk@vger.kernel.org
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > ---
> > drivers/clk/clk-scmi.c | 22 +++++++++++++++-------
> > 1 file changed, 15 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
> > index d5d369b052bd..fc9603988d91 100644
> > --- a/drivers/clk/clk-scmi.c
> > +++ b/drivers/clk/clk-scmi.c
> > @@ -18,6 +18,7 @@
> >
> > enum scmi_clk_feats {
> > SCMI_CLK_ATOMIC_SUPPORTED,
> > + SCMI_CLK_STATE_CTRL_FORBIDDEN,
>
> Can it be positive, i.e. SCMI_CLK_STATE_CTRL_SUPPORTED?
Yes of course.
>
> > SCMI_CLK_MAX_FEATS
> > };
> >
> > @@ -230,15 +231,19 @@ scmi_clk_ops_alloc(struct device *dev, unsigned long feats_key)
> > * only the prepare/unprepare API, as allowed by the clock framework
> > * when atomic calls are not available.
> > */
> > - if (feats_key & BIT(SCMI_CLK_ATOMIC_SUPPORTED)) {
> > - ops->enable = scmi_clk_atomic_enable;
> > - ops->disable = scmi_clk_atomic_disable;
> > - ops->is_enabled = scmi_clk_atomic_is_enabled;
> > - } else {
> > - ops->prepare = scmi_clk_enable;
> > - ops->unprepare = scmi_clk_disable;
> > + if (!(feats_key & BIT(SCMI_CLK_STATE_CTRL_FORBIDDEN))) {
> > + if (feats_key & BIT(SCMI_CLK_ATOMIC_SUPPORTED)) {
> > + ops->enable = scmi_clk_atomic_enable;
> > + ops->disable = scmi_clk_atomic_disable;
> > + } else {
> > + ops->prepare = scmi_clk_enable;
> > + ops->unprepare = scmi_clk_disable;
> > + }
> > }
> >
> > + if (feats_key & BIT(SCMI_CLK_ATOMIC_SUPPORTED))
> > + ops->is_enabled = scmi_clk_atomic_is_enabled;
> > +
> > /* Rate ops */
> > ops->recalc_rate = scmi_clk_recalc_rate;
> > ops->round_rate = scmi_clk_round_rate;
> > @@ -288,6 +293,9 @@ scmi_clk_ops_select(struct scmi_clk *sclk, bool atomic_capable,
> > if (atomic_capable && ci->enable_latency <= atomic_threshold)
> > feats_key |= BIT(SCMI_CLK_ATOMIC_SUPPORTED);
> >
> > + if (ci->state_ctrl_forbidden)
>
> Then this is negated.
>
I will rework accordingly
Thanks,
Cristian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ 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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ 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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 2/2] thermal/drivers/mediatek/lvts_thermal: Improve some memory allocation
From: Christophe JAILLET @ 2024-04-08 18:41 UTC (permalink / raw)
To: Dan Carpenter
Cc: Rafael J. Wysocki, Daniel Lezcano, Zhang Rui, Lukasz Luba,
Matthias Brugger, AngeloGioacchino Del Regno, linux-kernel,
kernel-janitors, linux-pm, linux-arm-kernel, linux-mediatek
In-Reply-To: <d97f2a57-d318-455b-a860-8bd7972c8aaf@moroto.mountain>
Le 08/04/2024 à 10:09, Dan Carpenter a écrit :
> On Sun, Apr 07, 2024 at 10:01:49PM +0200, Christophe JAILLET wrote:
>> diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c
>> index 3003dc350766..b133f731c5ba 100644
>> --- a/drivers/thermal/mediatek/lvts_thermal.c
>> +++ b/drivers/thermal/mediatek/lvts_thermal.c
>> @@ -204,7 +204,7 @@ static const struct debugfs_reg32 lvts_regs[] = {
>>
>> static int lvts_debugfs_init(struct device *dev, struct lvts_domain *lvts_td)
>> {
>> - struct debugfs_regset32 *regset;
>> + struct debugfs_regset32 *regsets;
>> struct lvts_ctrl *lvts_ctrl;
>> struct dentry *dentry;
>> char name[64];
>> @@ -214,8 +214,14 @@ static int lvts_debugfs_init(struct device *dev, struct lvts_domain *lvts_td)
>> if (IS_ERR(lvts_td->dom_dentry))
>> return 0;
>>
>> + regsets = devm_kcalloc(dev, lvts_td->num_lvts_ctrl,
>> + sizeof(*regsets), GFP_KERNEL);
>> + if (!regsets)
>> + return 0;
>
> I understand that this preserved the behavior from the original code,
> but the original code was wrong. This should return -ENOMEM.
Hi Dan,
I don't agree.
For me, this memory allocation is of the same type as all debugfs
functions that we ignore the error code.
If it fails, it is not a reason good enough to have the probe fail.
(anyway, if we are short on memory at this point other errors will
likely occur)
>
>> +
>> for (i = 0; i < lvts_td->num_lvts_ctrl; i++) {
>>
>> + struct debugfs_regset32 *regset = ®sets[i];
>> lvts_ctrl = &lvts_td->lvts_ctrl[i];
>
> The blank line should come after the declaration.
The blank line was already there, and in this file, it looks like the
preferred style (even if not completely consistent)
Let see if there is some comment about 0 or -ENOMEM in case of memory
allocation error, and if needed, I'll repost without the blank line.
This patch being a really tiny tiny tiny improvement (IMHO), so it may
also just be ignored.
CJ
>
> regards,
> dan carpenter
>
>
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ 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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ 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.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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v4 7/8] cpuidle/poll_state: replace cpu_relax with smp_cond_load_relaxed
From: Ankur Arora @ 2024-04-08 18:46 UTC (permalink / raw)
To: Okanovic, Haris
Cc: ankur.a.arora@oracle.com, joao.m.martins@oracle.com,
kvm@vger.kernel.org, dianders@chromium.org,
linux-arm-kernel@lists.infradead.org, pmladek@suse.com,
wanpengli@tencent.com, akpm@linux-foundation.org,
linux-kernel@vger.kernel.org, catalin.marinas@arm.com,
mingo@redhat.com, pbonzini@redhat.com, tglx@linutronix.de,
daniel.lezcano@linaro.org, mihai.carabas@oracle.com,
arnd@arndb.de, will@kernel.org, hpa@zytor.com,
peterz@infradead.org, mic@digikod.net, vkuznets@redhat.com,
bp@alien8.de, npiggin@gmail.com, linux-pm@vger.kernel.org,
rafael@kernel.org, juerg.haefliger@canonical.com, x86@kernel.org,
rick.p.edgecombe@intel.com
In-Reply-To: <aada0beae0b3479bfa311eea94a3b595bb8e5835.camel@amazon.com>
Okanovic, Haris <harisokn@amazon.com> writes:
> On Fri, 2024-04-05 at 16:14 -0700, Ankur Arora wrote:
>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>>
>>
>>
>> Okanovic, Haris <harisokn@amazon.com> writes:
>>
>> > On Thu, 2024-02-15 at 09:41 +0200, Mihai Carabas wrote:
>> > > cpu_relax on ARM64 does a simple "yield". Thus we replace it with
>> > > smp_cond_load_relaxed which basically does a "wfe".
>> > >
>> > > Suggested-by: Peter Zijlstra <peterz@infradead.org>
>> > > Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com>
>> > > ---
>> > > drivers/cpuidle/poll_state.c | 15 ++++++++++-----
>> > > 1 file changed, 10 insertions(+), 5 deletions(-)
>> > >
>> > > diff --git a/drivers/cpuidle/poll_state.c b/drivers/cpuidle/poll_state.c
>> > > index 9b6d90a72601..1e45be906e72 100644
>> > > --- a/drivers/cpuidle/poll_state.c
>> > > +++ b/drivers/cpuidle/poll_state.c
>> > > @@ -13,6 +13,7 @@
>> > > static int __cpuidle poll_idle(struct cpuidle_device *dev,
>> > > struct cpuidle_driver *drv, int index)
>> > > {
>> > > + unsigned long ret;
>> > > u64 time_start;
>> > >
>> > > time_start = local_clock_noinstr();
>> > > @@ -26,12 +27,16 @@ static int __cpuidle poll_idle(struct cpuidle_device *dev,
>> > >
>> > > limit = cpuidle_poll_time(drv, dev);
>> > >
>> > > - while (!need_resched()) {
>> > > - cpu_relax();
>> > > - if (loop_count++ < POLL_IDLE_RELAX_COUNT)
>> > > - continue;
>> > > -
>> > > + for (;;) {
>> > > loop_count = 0;
>> > > +
>> > > + ret = smp_cond_load_relaxed(¤t_thread_info()->flags,
>> > > + VAL & _TIF_NEED_RESCHED ||
>> > > + loop_count++ >= POLL_IDLE_RELAX_COUNT);
>> >
>> > Is it necessary to repeat this 200 times with a wfe poll?
>>
>> The POLL_IDLE_RELAX_COUNT is there because on x86 each cpu_relax()
>> iteration is much shorter.
>>
>> With WFE, it makes less sense.
>>
>> > Does kvm not implement a timeout period?
>>
>> Not yet, but it does become more useful after a WFE haltpoll is
>> available on ARM64.
>
> Note that kvm conditionally traps WFE and WFI based on number of host
> CPU tasks. VMs will sometimes see hardware behavior - potentially
> polling for a long time before entering WFI.
>
> https://elixir.bootlin.com/linux/latest/source/arch/arm64/kvm/arm.c#L459
Yeah. There was a discussion on this
https://lore.kernel.org/lkml/871qc6qufy.fsf@oracle.com/.
>> Haltpoll does have a timeout, which you should be able to tune via
>> /sys/module/haltpoll/parameters/ but that, of course, won't help here.
>>
>> > Could you make it configurable? This patch improves certain workloads
>> > on AWS Graviton instances as well, but blocks up to 6ms in 200 * 30us
>> > increments before going to wfi, which is a bit excessive.
>>
>> Yeah, this looks like a problem. We could solve it by making it an
>> architectural parameter. Though I worry about ARM platforms with
>> much smaller default timeouts.
>> The other possibility is using WFET in the primitive, but then we
>> have that dependency and that's a bigger change.
>
> See arm64's delay() for inspiration:
>
> https://elixir.bootlin.com/linux/v6.9-rc2/source/arch/arm64/lib/delay.c#L26
Sure, that part is straight-forward enough. However, this will need a fallback
the case when WFET is not available. And, because this path is used on x86,
so we need a cross platform smp_cond*timeout(). Though given that the x86
version is based on cpu_relax() then that could just fold the sched_clock()
check in.
Maybe another place to do this would be by KVM forcing a WFE timeout. Arguably
that is needed regardless of whether we use a smp_cond*timeout() or not.
--
ankur
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [WIP 0/3] Memory model and atomic API in Rust
From: Paul E. McKenney @ 2024-04-08 18:47 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Linus Torvalds, Philipp Stanner, Kent Overstreet, Boqun Feng,
rust-for-linux, linux-kernel, linux-arch, llvm, Miguel Ojeda,
Alex Gaynor, Wedson Almeida Filho, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Alan Stern,
Andrea Parri, Will Deacon, Peter Zijlstra, Nicholas Piggin,
David Howells, Jade Alglave, Luc Maranget, Akira Yokosawa,
Daniel Lustig, Joel Fernandes, Nathan Chancellor,
Nick Desaulniers, kent.overstreet, Greg Kroah-Hartman, elver,
Mark Rutland, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, H. Peter Anvin, Catalin Marinas,
linux-arm-kernel, linux-fsdevel
In-Reply-To: <ZhQjT4xdS3h-GbtC@casper.infradead.org>
On Mon, Apr 08, 2024 at 06:03:11PM +0100, Matthew Wilcox wrote:
> On Mon, Apr 08, 2024 at 09:55:23AM -0700, Paul E. McKenney wrote:
> > On Mon, Apr 08, 2024 at 05:02:37PM +0100, Matthew Wilcox wrote:
> > > In my ideal world, the compiler would turn this into:
> > >
> > > newfolio->flags |= folio->flags & MIGRATE_MASK;
> >
> > Why not accumulate the changes in a mask, and then apply the mask the
> > one time? (In situations where __folio_set_foo() need not apply.)
>
> Yes, absolutely, we can, should and probably eventually will do this
> when it gets to the top of somebody's todo list. But it irks me that
> we can't tell the compiler this is a safe transformation for it to make.
> There are a number of places where similar things happen.
>
> $ git grep folio_test.*folio_test
>
> will find you 82 of them (where they happen to be on the same line)
>
> if (folio_test_dirty(folio) || folio_test_locked(folio) ||
> folio_test_writeback(folio))
> break;
>
> turns into:
>
> 1f41: 48 8b 29 mov (%rcx),%rbp
> 1f44: 48 c1 ed 04 shr $0x4,%rbp
> 1f48: 83 e5 01 and $0x1,%ebp
> 1f4b: 0f 85 d5 00 00 00 jne 2026 <filemap_range_has_writeback+0x1a6>
> 1f51: 48 8b 29 mov (%rcx),%rbp
> 1f54: 83 e5 01 and $0x1,%ebp
> 1f57: 0f 85 c9 00 00 00 jne 2026 <filemap_range_has_writeback+0x1a6>
> 1f5d: 48 8b 29 mov (%rcx),%rbp
> 1f60: 48 d1 ed shr $1,%rbp
> 1f63: 83 e5 01 and $0x1,%ebp
> 1f66: 0f 85 ba 00 00 00 jne 2026 <filemap_range_has_writeback+0x1a6>
>
> rather than _one_ load from rcx and a test against a mask.
Agreed, it would be nice if we could convince the compiler to do this
for us, preferably without breaking anything.
> > If it turns out that we really do need a not-quite-volatile, what exactly
> > does it do? You clearly want it to be able to be optimized so as to merge
> > similar accesses. Is there a limit to the number of accesses that can
> > be merged or to the region of code over which such merging is permitted?
> > Either way, how is the compiler informed of these limits?
>
> Right, like I said, it's not going to be easy to define exactly what we
> want.
Or to convince the usual suspects that any definition we might come up
with is useful/implementable/teacheable/... :-/
Thanx, Paul
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ 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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ 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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ 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.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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH 9/9] tools/include: Sync arm64 asm/cputype.h with the kernel sources
From: Namhyung Kim @ 2024-04-08 18:55 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Ian Rogers, Kan Liang
Cc: Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
linux-perf-users, Catalin Marinas, Will Deacon, linux-arm-kernel
In-Reply-To: <20240408185520.1550865-1-namhyung@kernel.org>
To pick up the changes from:
fb091ff39479 ("arm64: Subscribe Microsoft Azure Cobalt 100 to ARM Neoverse N2 errata")
This should address these tools/perf build warnings:
Warning: Kernel ABI header differences:
diff -u tools/arch/arm64/include/asm/cputype.h arch/arm64/include/asm/cputype.h
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
tools/arch/arm64/include/asm/cputype.h | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/tools/arch/arm64/include/asm/cputype.h b/tools/arch/arm64/include/asm/cputype.h
index 7c7493cb571f..52f076afeb96 100644
--- a/tools/arch/arm64/include/asm/cputype.h
+++ b/tools/arch/arm64/include/asm/cputype.h
@@ -61,6 +61,7 @@
#define ARM_CPU_IMP_HISI 0x48
#define ARM_CPU_IMP_APPLE 0x61
#define ARM_CPU_IMP_AMPERE 0xC0
+#define ARM_CPU_IMP_MICROSOFT 0x6D
#define ARM_CPU_PART_AEM_V8 0xD0F
#define ARM_CPU_PART_FOUNDATION 0xD00
@@ -135,6 +136,8 @@
#define AMPERE_CPU_PART_AMPERE1 0xAC3
+#define MICROSOFT_CPU_PART_AZURE_COBALT_100 0xD49 /* Based on r0p0 of ARM Neoverse N2 */
+
#define MIDR_CORTEX_A53 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_A53)
#define MIDR_CORTEX_A57 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_A57)
#define MIDR_CORTEX_A72 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_A72)
@@ -193,6 +196,7 @@
#define MIDR_APPLE_M2_BLIZZARD_MAX MIDR_CPU_MODEL(ARM_CPU_IMP_APPLE, APPLE_CPU_PART_M2_BLIZZARD_MAX)
#define MIDR_APPLE_M2_AVALANCHE_MAX MIDR_CPU_MODEL(ARM_CPU_IMP_APPLE, APPLE_CPU_PART_M2_AVALANCHE_MAX)
#define MIDR_AMPERE1 MIDR_CPU_MODEL(ARM_CPU_IMP_AMPERE, AMPERE_CPU_PART_AMPERE1)
+#define MIDR_MICROSOFT_AZURE_COBALT_100 MIDR_CPU_MODEL(ARM_CPU_IMP_MICROSOFT, MICROSOFT_CPU_PART_AZURE_COBALT_100)
/* Fujitsu Erratum 010001 affects A64FX 1.0 and 1.1, (v0r0 and v1r0) */
#define MIDR_FUJITSU_ERRATUM_010001 MIDR_FUJITSU_A64FX
--
2.44.0.478.gd926399ef9-goog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* 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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ 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>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ 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>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ 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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ 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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH v4 7/8] cpuidle/poll_state: replace cpu_relax with smp_cond_load_relaxed
From: Okanovic, Haris @ 2024-04-08 20:04 UTC (permalink / raw)
To: ankur.a.arora@oracle.com
Cc: joao.m.martins@oracle.com, kvm@vger.kernel.org,
dianders@chromium.org, rick.p.edgecombe@intel.com,
rafael@kernel.org, pmladek@suse.com, wanpengli@tencent.com,
akpm@linux-foundation.org, linux-kernel@vger.kernel.org,
catalin.marinas@arm.com, mingo@redhat.com, pbonzini@redhat.com,
tglx@linutronix.de, daniel.lezcano@linaro.org, arnd@arndb.de,
mihai.carabas@oracle.com, will@kernel.org, hpa@zytor.com,
peterz@infradead.org, mic@digikod.net, vkuznets@redhat.com,
linux-arm-kernel@lists.infradead.org, Okanovic, Haris,
linux-pm@vger.kernel.org, bp@alien8.de,
juerg.haefliger@canonical.com, npiggin@gmail.com, x86@kernel.org
In-Reply-To: <87il0rsnf0.fsf@oracle.com>
On Mon, 2024-04-08 at 11:46 -0700, Ankur Arora wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>
>
>
> Okanovic, Haris <harisokn@amazon.com> writes:
>
> > On Fri, 2024-04-05 at 16:14 -0700, Ankur Arora wrote:
> > > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> > >
> > >
> > >
> > > Okanovic, Haris <harisokn@amazon.com> writes:
> > >
> > > > On Thu, 2024-02-15 at 09:41 +0200, Mihai Carabas wrote:
> > > > > cpu_relax on ARM64 does a simple "yield". Thus we replace it with
> > > > > smp_cond_load_relaxed which basically does a "wfe".
> > > > >
> > > > > Suggested-by: Peter Zijlstra <peterz@infradead.org>
> > > > > Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com>
> > > > > ---
> > > > > drivers/cpuidle/poll_state.c | 15 ++++++++++-----
> > > > > 1 file changed, 10 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/drivers/cpuidle/poll_state.c b/drivers/cpuidle/poll_state.c
> > > > > index 9b6d90a72601..1e45be906e72 100644
> > > > > --- a/drivers/cpuidle/poll_state.c
> > > > > +++ b/drivers/cpuidle/poll_state.c
> > > > > @@ -13,6 +13,7 @@
> > > > > static int __cpuidle poll_idle(struct cpuidle_device *dev,
> > > > > struct cpuidle_driver *drv, int index)
> > > > > {
> > > > > + unsigned long ret;
> > > > > u64 time_start;
> > > > >
> > > > > time_start = local_clock_noinstr();
> > > > > @@ -26,12 +27,16 @@ static int __cpuidle poll_idle(struct cpuidle_device *dev,
> > > > >
> > > > > limit = cpuidle_poll_time(drv, dev);
> > > > >
> > > > > - while (!need_resched()) {
> > > > > - cpu_relax();
> > > > > - if (loop_count++ < POLL_IDLE_RELAX_COUNT)
> > > > > - continue;
> > > > > -
> > > > > + for (;;) {
> > > > > loop_count = 0;
> > > > > +
> > > > > + ret = smp_cond_load_relaxed(¤t_thread_info()->flags,
> > > > > + VAL & _TIF_NEED_RESCHED ||
> > > > > + loop_count++ >= POLL_IDLE_RELAX_COUNT);
> > > >
> > > > Is it necessary to repeat this 200 times with a wfe poll?
> > >
> > > The POLL_IDLE_RELAX_COUNT is there because on x86 each cpu_relax()
> > > iteration is much shorter.
> > >
> > > With WFE, it makes less sense.
> > >
> > > > Does kvm not implement a timeout period?
> > >
> > > Not yet, but it does become more useful after a WFE haltpoll is
> > > available on ARM64.
> >
> > Note that kvm conditionally traps WFE and WFI based on number of host
> > CPU tasks. VMs will sometimes see hardware behavior - potentially
> > polling for a long time before entering WFI.
> >
> > https://elixir.bootlin.com/linux/latest/source/arch/arm64/kvm/arm.c#L459
>
> Yeah. There was a discussion on this
> https://lore.kernel.org/lkml/871qc6qufy.fsf@oracle.com/.
>
> > > Haltpoll does have a timeout, which you should be able to tune via
> > > /sys/module/haltpoll/parameters/ but that, of course, won't help here.
> > >
> > > > Could you make it configurable? This patch improves certain workloads
> > > > on AWS Graviton instances as well, but blocks up to 6ms in 200 * 30us
> > > > increments before going to wfi, which is a bit excessive.
> > >
> > > Yeah, this looks like a problem. We could solve it by making it an
> > > architectural parameter. Though I worry about ARM platforms with
> > > much smaller default timeouts.
> > > The other possibility is using WFET in the primitive, but then we
> > > have that dependency and that's a bigger change.
> >
> > See arm64's delay() for inspiration:
> >
> > https://elixir.bootlin.com/linux/v6.9-rc2/source/arch/arm64/lib/delay.c#L26
>
> Sure, that part is straight-forward enough. However, this will need a fallback
> the case when WFET is not available. And, because this path is used on x86,
> so we need a cross platform smp_cond*timeout(). Though given that the x86
> version is based on cpu_relax() then that could just fold the sched_clock()
> check in.
I was trying to point out how delay() handles different configurations:
It prefers WFET when available, falls back to WFE when event stream is
available, and finally falls back to cpu_relax() as last resort. Same
logic can apply here. The x86 case can always use cpu_relax() fallback,
for same behavior as smp_cond_load_relaxed().
Re your concern about "ARM platforms with much smaller default
timeouts": You could do something different when arch_timer_get_rate()
is too small. Although I'm not sure this is a huge concern, given that
delay() doesn't seem to care in the WFE case.
-- Haris Okanovic
>
> Maybe another place to do this would be by KVM forcing a WFE timeout. Arguably
> that is needed regardless of whether we use a smp_cond*timeout() or not.
>
> --
> ankur
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [WIP 0/3] Memory model and atomic API in Rust
From: Linus Torvalds @ 2024-04-08 20:05 UTC (permalink / raw)
To: Al Viro
Cc: Matthew Wilcox, Philipp Stanner, Kent Overstreet, Boqun Feng,
rust-for-linux, linux-kernel, linux-arch, llvm, Miguel Ojeda,
Alex Gaynor, Wedson Almeida Filho, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Alan Stern,
Andrea Parri, Will Deacon, Peter Zijlstra, Nicholas Piggin,
David Howells, Jade Alglave, Luc Maranget, Paul E. McKenney,
Akira Yokosawa, Daniel Lustig, Joel Fernandes, Nathan Chancellor,
Nick Desaulniers, kent.overstreet, Greg Kroah-Hartman, elver,
Mark Rutland, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, H. Peter Anvin, Catalin Marinas,
linux-arm-kernel, linux-fsdevel
In-Reply-To: <20240408181436.GO538574@ZenIV>
On Mon, 8 Apr 2024 at 11:14, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> FWIW, PA-RISC is no better - the same "fetch and replace with constant"
> kind of primitive as for sparc32, only the constant is (u32)0 instead
> of (u8)~0. And unlike sparc64, 64bit variant didn't get better.
Heh. The thing about PA-RISC is that it is actually *so* much worse
that it was never useful for an arithmetic type.
IOW, the fact that sparc used just a byte meant that the aotmic_t
hackery on sparc still gave us 24 useful bits in a 32-bit atomic_t.
So long ago, we used to have an arithmetic atomic_t that was 32-bit on
all sane architectures, but only had a 24-bit range on sparc.
And I know you know all this, I'm just explaining the horror for the audience.
On PA-RISC you couldn't do that horrendous trick, so parist just used
the "we use a hashed spinlock for all atomics", and "atomic_t" was a
regular full-sized integer type.
Anyway, the sparc 24-bit atomics were actually replaced by the PA-RISC
version back twenty years ago (almost to the day):
https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/?id=373f1583c5c5
and while we still had some left-over of that horror in the git tree
up until 2011 (until commit 348738afe530: "sparc32: drop unused
atomic24 support") we probably should have made the
"arch_atomic_xyz()" ops work on generic types rather than "atomic_t"
for a long long time, so that you could use them on other things than
"atomic_t" and friends.
You can see the casting horror here, for example:
include/asm-generic/bitops/atomic.h
where we do that cast from "volatile unsigned long *p" to
"atomic_long_t *" just to use the raw_atomic_long_xyz() operations.
It would make more sense if the raw atomics took that "native"
volatile unsigned long pointer directly.
(And here that "volatile" is not because it's necessary used as a
volatile - it is - but simply because it's the most permissive type of
pointer. You can see other places using "const volatile unsigned long"
pointers for the same reason: passing in a non-const or non-volatile
pointer is perfectly fine).
Linus
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v1 3/4] usb: dwc3: exynos: Use devm_regulator_bulk_get_enable() helper function
From: Christophe JAILLET @ 2024-04-08 20:26 UTC (permalink / raw)
To: Anand Moon
Cc: Thinh Nguyen, Greg Kroah-Hartman, Krzysztof Kozlowski,
Alim Akhtar, linux-usb, linux-arm-kernel, linux-samsung-soc,
linux-kernel
In-Reply-To: <CANAwSgQ6_bxpKmanPwXvKEQJ0p7AvwTs3on9ubOegaDn3nkdxw@mail.gmail.com>
Le 08/04/2024 à 12:02, Anand Moon a écrit :
> Hi Christophe,
>
> On Fri, 5 Apr 2024 at 21:42, Christophe JAILLET
> <christophe.jaillet@wanadoo.fr> wrote:
>>
>> Le 05/04/2024 à 08:10, Anand Moon a écrit :
>>> Hi Christophe, Krzysztof,
>>>
>>> On Mon, 4 Mar 2024 at 17:16, Anand Moon <linux.amoon@gmail.com> wrote:
>>>>
>>>> Hi Christophe,
>>>>
>>>> On Sun, 3 Mar 2024 at 00:07, Christophe JAILLET
>>>> <christophe.jaillet@wanadoo.fr> wrote:
>>>>>
>>>>> Le 02/03/2024 à 17:48, Anand Moon a écrit :
>>>>>> Hi Christophe,
>>>>>>
>>>>>> On Sat, 2 Mar 2024 at 21:20, Christophe JAILLET
>>>>>> <christophe.jaillet@wanadoo.fr> wrote:
>>>>>>>
>>>>>>> Le 01/03/2024 à 20:38, Anand Moon a écrit :
>>>>>>>> Use devm_regulator_bulk_get_enable() instead of open coded
>>>>>>>> 'devm_regulator_get(), regulator_enable(), regulator_disable().
>>>>>>>>
>>>>>>>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>>>>>>>> ---
>>>>>>>> drivers/usb/dwc3/dwc3-exynos.c | 49 +++-------------------------------
>>>>>>>> 1 file changed, 4 insertions(+), 45 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
>>>>>>>> index 5d365ca51771..7c77f3c69825 100644
>>>>>>>> --- a/drivers/usb/dwc3/dwc3-exynos.c
>>>>>>>> +++ b/drivers/usb/dwc3/dwc3-exynos.c
>>>>>>>> @@ -32,9 +32,6 @@ struct dwc3_exynos {
>>>>>>>> struct clk *clks[DWC3_EXYNOS_MAX_CLOCKS];
>>>>>>>> int num_clks;
>>>>>>>> int suspend_clk_idx;
>>>>>>>> -
>>>>>>>> - struct regulator *vdd33;
>>>>>>>> - struct regulator *vdd10;
>>>>>>>> };
>>>>>>>>
>>>>>>>> static int dwc3_exynos_probe(struct platform_device *pdev)
>>>>>>>> @@ -44,6 +41,7 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
>>>>>>>> struct device_node *node = dev->of_node;
>>>>>>>> const struct dwc3_exynos_driverdata *driver_data;
>>>>>>>> int i, ret;
>>>>>>>> + static const char * const regulators[] = { "vdd33", "vdd10" };
>>>>>>>>
>>>>>>>> exynos = devm_kzalloc(dev, sizeof(*exynos), GFP_KERNEL);
>>>>>>>> if (!exynos)
>>>>>>>> @@ -78,27 +76,9 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
>>>>>>>> if (exynos->suspend_clk_idx >= 0)
>>>>>>>> clk_prepare_enable(exynos->clks[exynos->suspend_clk_idx]);
>>>>>>>>
>>>>>>>> - exynos->vdd33 = devm_regulator_get(dev, "vdd33");
>>>>>>>> - if (IS_ERR(exynos->vdd33)) {
>>>>>>>> - ret = PTR_ERR(exynos->vdd33);
>>>>>>>> - goto vdd33_err;
>>>>>>>> - }
>>>>>>>> - ret = regulator_enable(exynos->vdd33);
>>>>>>>> - if (ret) {
>>>>>>>> - dev_err(dev, "Failed to enable VDD33 supply\n");
>>>>>>>> - goto vdd33_err;
>>>>>>>> - }
>>>>>>>> -
>>>>>>>> - exynos->vdd10 = devm_regulator_get(dev, "vdd10");
>>>>>>>> - if (IS_ERR(exynos->vdd10)) {
>>>>>>>> - ret = PTR_ERR(exynos->vdd10);
>>>>>>>> - goto vdd10_err;
>>>>>>>> - }
>>>>>>>> - ret = regulator_enable(exynos->vdd10);
>>>>>>>> - if (ret) {
>>>>>>>> - dev_err(dev, "Failed to enable VDD10 supply\n");
>>>>>>>> - goto vdd10_err;
>>>>>>>> - }
>>>>>>>> + ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(regulators), regulators);
>>>>>>>> + if (ret)
>>>>>>>> + return dev_err_probe(dev, ret, "Failed to enable regulators\n");
>>>>>>>>
>>>>>>>> if (node) {
>>>>>>>> ret = of_platform_populate(node, NULL, NULL, dev);
>>>>>>>> @@ -115,10 +95,6 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
>>>>>>>> return 0;
>>>>>>>>
>>>>>>>> populate_err:
>>>>>>>> - regulator_disable(exynos->vdd10);
>>>>>>>> -vdd10_err:
>>>>>>>> - regulator_disable(exynos->vdd33);
>>>>>>>> -vdd33_err:
>>>>>>>> for (i = exynos->num_clks - 1; i >= 0; i--)
>>>>>>>> clk_disable_unprepare(exynos->clks[i]);
>>>>>>>>
>>>>>>>> @@ -140,9 +116,6 @@ static void dwc3_exynos_remove(struct platform_device *pdev)
>>>>>>>>
>>>>>>>> if (exynos->suspend_clk_idx >= 0)
>>>>>>>> clk_disable_unprepare(exynos->clks[exynos->suspend_clk_idx]);
>>>>>>>> -
>>>>>>>> - regulator_disable(exynos->vdd33);
>>>>>>>> - regulator_disable(exynos->vdd10);
>>>>>>>> }
>>>>>>>>
>>>>>>>> static const struct dwc3_exynos_driverdata exynos5250_drvdata = {
>>>>>>>> @@ -196,9 +169,6 @@ static int dwc3_exynos_suspend(struct device *dev)
>>>>>>>> for (i = exynos->num_clks - 1; i >= 0; i--)
>>>>>>>> clk_disable_unprepare(exynos->clks[i]);
>>>>>>>>
>>>>>>>> - regulator_disable(exynos->vdd33);
>>>>>>>> - regulator_disable(exynos->vdd10);
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> Same here, I don't think that removing regulator_[en|dis]able from the
>>>>>>> suspend and resume function is correct.
>>>>>>>
>>>>>>> The goal is to stop some hardware when the system is suspended, in order
>>>>>>> to save some power.
>>>>>> Ok,
>>>>>>>
>>>>>>> Why did you removed it?
>>>>>>
>>>>>> As per the description of the function devm_regulator_bulk_get_enable
>>>>>>
>>>>>> * This helper function allows drivers to get several regulator
>>>>>> * consumers in one operation with management, the regulators will
>>>>>> * automatically be freed when the device is unbound. If any of the
>>>>>> * regulators cannot be acquired then any regulators that were
>>>>>> * allocated will be freed before returning to the caller.
>>>>>
>>>>> The code in suspend/resume is not about freeing some resources. It is
>>>>> about enabling/disabling some hardware to save some power.
>>>>>
>>>>> Think to the probe/remove functions as the software in the kernel that
>>>>> knows how to handle some hardawre, and the suspend/resume as the on/off
>>>>> button to power-on and off the electrical chips.
>>>>>
>>>>> When the system is suspended, the software is still around. But some
>>>>> hardware can be set in a low consumption mode to save some power.
>>>>>
>>>>> IMHO, part of the code you removed changed this behaviour and increase
>>>>> the power consumption when the system is suspended.
>>>>>
>>>>
>>>> You are correct, I have changed the regulator API from
>>>> devm_regulator_get_enable to devm_regulator_bulk_get_enable
>>>> which changes this behavior.
>>>> I will fix it in the next version.
>>>>
>>>>> CJ
>>>
>>> I could not find any example in the kernel to support
>>> devm_regulator_bulk_disable
>>> but here is my modified file.
>>>
>>> If you have any suggestions for this plz let me know.
>>
>> I don't think that your approach is correct, and I don't think that the
>> proposed patch does what you expect it to do.
>>
>> Calling a devm_ function in suspend/resume functions looks really
>> strange to me and is likely broken.
>>
>> Especially here, devm_regulator_bulk_get_enable() in the resume function
>> allocates some memory that is not freed in
>> devm_regulator_bulk_disable(), because the API is not designed to work
>> like that. So this could generate a kind of memory leak.
>>
>>
>> *I think that the code is good enough as-is*, but if you really want to
>> change something, maybe:
>> - devm_regulator_get()+regulator_enable() in the probe could be
>> changed to devm_regulator_get_enable()
>> - the resume/suspend function should be left as-is with
>> regulator_disable()/regulator_ensable()
>> - remove regulator_disable() from the error handling path of the
>> probe and from the remove function.
>>
>> I *think* it would work.
>>
> No devm_regulator_get_enable use the same logic as
> devm_regulator_bulk_get_enable
> to enable the regulator.
Yes, the logic is the same, but you get a pointer to the "struct
regulator" which can be used to disable/enable in the suspend/resume
functions.
With the bulk version, you can not do that.
See my first reply on your 3/4 patch.
>
> [0] https://elixir.bootlin.com/linux/latest/source/drivers/regulator/devres.c#L126
>
> So as of now I am dropping the changes on the regulator in this patch series.
I do agree that it is certainly the way to go here.
CJ
>
>> CJ
>>
> Thanks for your inputs.
>
> Thanks
>
> -Anand
>
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ 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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ 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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ 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