* [PATCH 1/2] ASoC: fsl-asoc-card: Add WM8524 support
From: Shengjiu Wang @ 2020-06-23 6:52 UTC (permalink / raw)
To: timur, nicoleotsuka, Xiubo.Lee, festevam, lgirdwood, broonie,
perex, tiwai, alsa-devel, linuxppc-dev, linux-kernel
WM8524 only supports playback mode, and only works at
slave mode.
Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
---
sound/soc/fsl/fsl-asoc-card.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/sound/soc/fsl/fsl-asoc-card.c b/sound/soc/fsl/fsl-asoc-card.c
index d0543a53764e..57ea1b072326 100644
--- a/sound/soc/fsl/fsl-asoc-card.c
+++ b/sound/soc/fsl/fsl-asoc-card.c
@@ -611,6 +611,15 @@ static int fsl_asoc_card_probe(struct platform_device *pdev)
priv->dai_link[2].dpcm_capture = 0;
priv->card.dapm_routes = audio_map_tx;
priv->card.num_dapm_routes = ARRAY_SIZE(audio_map_tx);
+ } else if (of_device_is_compatible(np, "fsl,imx-audio-wm8524")) {
+ codec_dai_name = "wm8524-hifi";
+ priv->card.set_bias_level = NULL;
+ priv->dai_fmt |= SND_SOC_DAIFMT_CBS_CFS;
+ priv->dai_link[1].dpcm_capture = 0;
+ priv->dai_link[2].dpcm_capture = 0;
+ priv->cpu_priv.slot_width = 32;
+ priv->card.dapm_routes = audio_map_tx;
+ priv->card.num_dapm_routes = ARRAY_SIZE(audio_map_tx);
} else {
dev_err(&pdev->dev, "unknown Device Tree compatible\n");
ret = -EINVAL;
@@ -760,6 +769,7 @@ static const struct of_device_id fsl_asoc_card_dt_ids[] = {
{ .compatible = "fsl,imx-audio-wm8962", },
{ .compatible = "fsl,imx-audio-wm8960", },
{ .compatible = "fsl,imx-audio-mqs", },
+ { .compatible = "fsl,imx-audio-wm8524", },
{}
};
MODULE_DEVICE_TABLE(of, fsl_asoc_card_dt_ids);
--
2.21.0
^ permalink raw reply related
* [PATCH 2/2] ASoC: bindings: fsl-asoc-card: Add compatible string for wm8524
From: Shengjiu Wang @ 2020-06-23 6:52 UTC (permalink / raw)
To: timur, nicoleotsuka, Xiubo.Lee, festevam, lgirdwood, broonie,
perex, tiwai, alsa-devel, linuxppc-dev, linux-kernel
In-Reply-To: <1592895167-30483-1-git-send-email-shengjiu.wang@nxp.com>
In order to support wm8524 codec with fsl-asoc-card machine
driver, add compatible string "fsl,imx-audio-wm8524".
Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
---
Documentation/devicetree/bindings/sound/fsl-asoc-card.txt | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/sound/fsl-asoc-card.txt b/Documentation/devicetree/bindings/sound/fsl-asoc-card.txt
index ca9a3a43adfd..133d7e14a4d0 100644
--- a/Documentation/devicetree/bindings/sound/fsl-asoc-card.txt
+++ b/Documentation/devicetree/bindings/sound/fsl-asoc-card.txt
@@ -36,6 +36,8 @@ The compatible list for this generic sound card currently:
"fsl,imx-audio-mqs"
+ "fsl,imx-audio-wm8524"
+
Required properties:
- compatible : Contains one of entries in the compatible list.
--
2.21.0
^ permalink raw reply related
* Re: [PATCH v2 2/2] ASoC: fsl_mqs: Fix unchecked return value for clk_prepare_enable
From: Nicolin Chen @ 2020-06-23 6:19 UTC (permalink / raw)
To: Shengjiu Wang
Cc: alsa-devel, timur, Xiubo.Lee, linuxppc-dev, tiwai, perex, broonie,
festevam, linux-kernel
In-Reply-To: <5edd68d03def367d96268f1a9a00bd528ea5aaf2.1592888591.git.shengjiu.wang@nxp.com>
On Tue, Jun 23, 2020 at 02:01:12PM +0800, Shengjiu Wang wrote:
> Fix unchecked return value for clk_prepare_enable, add error
> handler in fsl_mqs_runtime_resume.
>
> Fixes: 9e28f6532c61 ("ASoC: fsl_mqs: Add MQS component driver")
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
Acked-by: Nicolin Chen <nicoleotsuka@gmail.com>
^ permalink raw reply
* Re: [PATCH v2 1/2] ASoC: fsl_mqs: Don't check clock is NULL before calling clk API
From: Nicolin Chen @ 2020-06-23 6:16 UTC (permalink / raw)
To: Shengjiu Wang
Cc: alsa-devel, timur, Xiubo.Lee, linuxppc-dev, tiwai, perex, broonie,
festevam, linux-kernel
In-Reply-To: <743be216bd504c26e8d45d5ce4a84561b67a122b.1592888591.git.shengjiu.wang@nxp.com>
On Tue, Jun 23, 2020 at 02:01:11PM +0800, Shengjiu Wang wrote:
> Because clk_prepare_enable and clk_disable_unprepare should
> check input clock parameter is NULL or not internally, then
> we don't need to check them before calling the function.
>
> Fixes: 9e28f6532c61 ("ASoC: fsl_mqs: Add MQS component driver")
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
Acked-by: Nicolin Chen <nicoleotsuka@gmail.com>
^ permalink raw reply
* [PATCH v2 2/2] ASoC: fsl_mqs: Fix unchecked return value for clk_prepare_enable
From: Shengjiu Wang @ 2020-06-23 6:01 UTC (permalink / raw)
To: timur, nicoleotsuka, Xiubo.Lee, festevam, broonie, perex, tiwai,
alsa-devel
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <cover.1592888591.git.shengjiu.wang@nxp.com>
Fix unchecked return value for clk_prepare_enable, add error
handler in fsl_mqs_runtime_resume.
Fixes: 9e28f6532c61 ("ASoC: fsl_mqs: Add MQS component driver")
Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
---
sound/soc/fsl/fsl_mqs.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/sound/soc/fsl/fsl_mqs.c b/sound/soc/fsl/fsl_mqs.c
index b44b134390a3..69aeb0e71844 100644
--- a/sound/soc/fsl/fsl_mqs.c
+++ b/sound/soc/fsl/fsl_mqs.c
@@ -265,10 +265,20 @@ static int fsl_mqs_remove(struct platform_device *pdev)
static int fsl_mqs_runtime_resume(struct device *dev)
{
struct fsl_mqs *mqs_priv = dev_get_drvdata(dev);
+ int ret;
- clk_prepare_enable(mqs_priv->ipg);
+ ret = clk_prepare_enable(mqs_priv->ipg);
+ if (ret) {
+ dev_err(dev, "failed to enable ipg clock\n");
+ return ret;
+ }
- clk_prepare_enable(mqs_priv->mclk);
+ ret = clk_prepare_enable(mqs_priv->mclk);
+ if (ret) {
+ dev_err(dev, "failed to enable mclk clock\n");
+ clk_disable_unprepare(mqs_priv->ipg);
+ return ret;
+ }
if (mqs_priv->use_gpr)
regmap_write(mqs_priv->regmap, IOMUXC_GPR2,
--
2.21.0
^ permalink raw reply related
* [PATCH v2 1/2] ASoC: fsl_mqs: Don't check clock is NULL before calling clk API
From: Shengjiu Wang @ 2020-06-23 6:01 UTC (permalink / raw)
To: timur, nicoleotsuka, Xiubo.Lee, festevam, broonie, perex, tiwai,
alsa-devel
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <cover.1592888591.git.shengjiu.wang@nxp.com>
Because clk_prepare_enable and clk_disable_unprepare should
check input clock parameter is NULL or not internally, then
we don't need to check them before calling the function.
Fixes: 9e28f6532c61 ("ASoC: fsl_mqs: Add MQS component driver")
Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
---
sound/soc/fsl/fsl_mqs.c | 13 ++++---------
1 file changed, 4 insertions(+), 9 deletions(-)
diff --git a/sound/soc/fsl/fsl_mqs.c b/sound/soc/fsl/fsl_mqs.c
index 0c813a45bba7..b44b134390a3 100644
--- a/sound/soc/fsl/fsl_mqs.c
+++ b/sound/soc/fsl/fsl_mqs.c
@@ -266,11 +266,9 @@ static int fsl_mqs_runtime_resume(struct device *dev)
{
struct fsl_mqs *mqs_priv = dev_get_drvdata(dev);
- if (mqs_priv->ipg)
- clk_prepare_enable(mqs_priv->ipg);
+ clk_prepare_enable(mqs_priv->ipg);
- if (mqs_priv->mclk)
- clk_prepare_enable(mqs_priv->mclk);
+ clk_prepare_enable(mqs_priv->mclk);
if (mqs_priv->use_gpr)
regmap_write(mqs_priv->regmap, IOMUXC_GPR2,
@@ -292,11 +290,8 @@ static int fsl_mqs_runtime_suspend(struct device *dev)
regmap_read(mqs_priv->regmap, REG_MQS_CTRL,
&mqs_priv->reg_mqs_ctrl);
- if (mqs_priv->mclk)
- clk_disable_unprepare(mqs_priv->mclk);
-
- if (mqs_priv->ipg)
- clk_disable_unprepare(mqs_priv->ipg);
+ clk_disable_unprepare(mqs_priv->mclk);
+ clk_disable_unprepare(mqs_priv->ipg);
return 0;
}
--
2.21.0
^ permalink raw reply related
* [PATCH v2 0/2] Fix unchecked return value for clk_prepare_enable
From: Shengjiu Wang @ 2020-06-23 6:01 UTC (permalink / raw)
To: timur, nicoleotsuka, Xiubo.Lee, festevam, broonie, perex, tiwai,
alsa-devel
Cc: linuxppc-dev, linux-kernel
First patch is to remove the check of clock pointer before calling
clk API.
Second patch is to fix the issue that the return value of
clk_prepare_enable is not checked.
changes in v2:
- split the patch to separate patches
Shengjiu Wang (2):
ASoC: fsl_mqs: Don't check clock is NULL before calling clk API
ASoC: fsl_mqs: Fix unchecked return value for clk_prepare_enable
sound/soc/fsl/fsl_mqs.c | 23 ++++++++++++++---------
1 file changed, 14 insertions(+), 9 deletions(-)
--
2.21.0
^ permalink raw reply
* Re: [PATCH 1/2] powerpc/papr_scm: Fetch nvdimm performance stats from PHYP
From: Aneesh Kumar K.V @ 2020-06-23 5:52 UTC (permalink / raw)
To: Vaibhav Jain, linuxppc-dev, linux-nvdimm; +Cc: Vaibhav Jain
In-Reply-To: <871rm649u0.fsf@linux.ibm.com>
Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> writes:
> Vaibhav Jain <vaibhav@linux.ibm.com> writes:
> + */
>> + seq_buf_init(&s, buf, PAGE_SIZE);
>> + for (index = 0, stat = stats->scm_statistic;
>> + index < stats->num_statistics; ++index, ++stat) {
>> + seq_buf_printf(&s, "%.8s = 0x%016llX\n",
>> + stat->statistic_id, stat->statistic_value);
>
>
> That is raw number (statistic_id). Is that useful? Can we map them to user readable
> strings?
Ok i missed that "%.8s" .
>
>> + }
>> + }
>> +
>> + kfree(stats);
>> + return rc ? rc : seq_buf_used(&s);
>> +}
>> +DEVICE_ATTR_RO(perf_stats);
-aneesh
^ permalink raw reply
* Re: [PATCH 1/2] powerpc/papr_scm: Fetch nvdimm performance stats from PHYP
From: Aneesh Kumar K.V @ 2020-06-23 5:42 UTC (permalink / raw)
To: Vaibhav Jain, linuxppc-dev, linux-nvdimm; +Cc: Vaibhav Jain
In-Reply-To: <20200622042451.22448-2-vaibhav@linux.ibm.com>
Vaibhav Jain <vaibhav@linux.ibm.com> writes:
> Update papr_scm.c to query dimm performance statistics from PHYP via
> H_SCM_PERFORMANCE_STATS hcall and export them to user-space as PAPR
> specific NVDIMM attribute 'perf_stats' in sysfs. The patch also
> provide a sysfs ABI documentation for the stats being reported and
> their meanings.
>
> During NVDIMM probe time in papr_scm_nvdimm_init() a special variant
> of H_SCM_PERFORMANCE_STATS hcall is issued to check if collection of
> performance statistics is supported or not. If successful then a PHYP
> returns a maximum possible buffer length needed to read all
> performance stats. This returned value is stored in a per-nvdimm
> attribute 'len_stat_buffer'.
>
> The layout of request buffer for reading NVDIMM performance stats from
> PHYP is defined in 'struct papr_scm_perf_stats' and 'struct
> papr_scm_perf_stat'. These structs are used in newly introduced
> drc_pmem_query_stats() that issues the H_SCM_PERFORMANCE_STATS hcall.
>
> The sysfs access function perf_stats_show() uses value
> 'len_stat_buffer' to allocate a buffer large enough to hold all
> possible NVDIMM performance stats and passes it to
> drc_pmem_query_stats() to populate. Finally statistics reported in the
> buffer are formatted into the sysfs access function output buffer.
>
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> ---
> Documentation/ABI/testing/sysfs-bus-papr-pmem | 27 ++++
> arch/powerpc/platforms/pseries/papr_scm.c | 139 ++++++++++++++++++
> 2 files changed, 166 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-papr-pmem b/Documentation/ABI/testing/sysfs-bus-papr-pmem
> index 5b10d036a8d4..c1a67275c43f 100644
> --- a/Documentation/ABI/testing/sysfs-bus-papr-pmem
> +++ b/Documentation/ABI/testing/sysfs-bus-papr-pmem
> @@ -25,3 +25,30 @@ Description:
> NVDIMM have been scrubbed.
> * "locked" : Indicating that NVDIMM contents cant
> be modified until next power cycle.
> +
> +What: /sys/bus/nd/devices/nmemX/papr/perf_stats
> +Date: May, 2020
> +KernelVersion: v5.9
> +Contact: linuxppc-dev <linuxppc-dev@lists.ozlabs.org>, linux-nvdimm@lists.01.org,
> +Description:
> + (RO) Report various performance stats related to papr-scm NVDIMM
> + device. Each stat is reported on a new line with each line
> + composed of a stat-identifier followed by it value. Below are
> + currently known dimm performance stats which are reported:
> +
> + * "CtlResCt" : Controller Reset Count
> + * "CtlResTm" : Controller Reset Elapsed Time
> + * "PonSecs " : Power-on Seconds
> + * "MemLife " : Life Remaining
> + * "CritRscU" : Critical Resource Utilization
> + * "HostLCnt" : Host Load Count
> + * "HostSCnt" : Host Store Count
> + * "HostSDur" : Host Store Duration
> + * "HostLDur" : Host Load Duration
> + * "MedRCnt " : Media Read Count
> + * "MedWCnt " : Media Write Count
> + * "MedRDur " : Media Read Duration
> + * "MedWDur " : Media Write Duration
> + * "CchRHCnt" : Cache Read Hit Count
> + * "CchWHCnt" : Cache Write Hit Count
> + * "FastWCnt" : Fast Write Count
> \ No newline at end of file
> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> index 9c569078a09f..cb3f9acc325b 100644
> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> @@ -62,6 +62,24 @@
> PAPR_PMEM_HEALTH_FATAL | \
> PAPR_PMEM_HEALTH_UNHEALTHY)
>
> +#define PAPR_SCM_PERF_STATS_EYECATCHER __stringify(SCMSTATS)
> +#define PAPR_SCM_PERF_STATS_VERSION 0x1
> +
> +/* Struct holding a single performance metric */
> +struct papr_scm_perf_stat {
> + u8 statistic_id[8];
> + u64 statistic_value;
May be stat_id, stat_val ?
> +};
> +
> +/* Struct exchanged between kernel and PHYP for fetching drc perf stats */
> +struct papr_scm_perf_stats {
> + u8 eye_catcher[8];
> + u32 stats_version; /* Should be 0x01 */
> + u32 num_statistics; /* Number of stats following */
> + /* zero or more performance matrics */
> + struct papr_scm_perf_stat scm_statistic[];
> +} __packed;
For Phyp interaction these should be big-endian. I see you do
stats[i].statistic_value = be64_to_cpu(stats[i].statistic_value);
Can we avoid that?
> +
> /* private struct associated with each region */
> struct papr_scm_priv {
> struct platform_device *pdev;
> @@ -89,6 +107,9 @@ struct papr_scm_priv {
>
> /* Health information for the dimm */
> u64 health_bitmap;
> +
> + /* length of the stat buffer as expected by phyp */
> + size_t len_stat_buffer;
how about stat_buffer_len?
> };
>
> static int drc_pmem_bind(struct papr_scm_priv *p)
> @@ -194,6 +215,75 @@ static int drc_pmem_query_n_bind(struct papr_scm_priv *p)
> return drc_pmem_bind(p);
> }
>
> +/*
> + * Query the Dimm performance stats from PHYP and copy them (if returned) to
> + * provided struct papr_scm_perf_stats instance 'stats' of 'size' in bytes.
> + * The value of R4 is copied to 'out' if the pointer is provided.
> + */
> +static int drc_pmem_query_stats(struct papr_scm_priv *p,
> + struct papr_scm_perf_stats *buff_stats,
> + size_t size, unsigned int num_stats,
> + uint64_t *out)
> +{
> + unsigned long ret[PLPAR_HCALL_BUFSIZE];
> + struct papr_scm_perf_stat *stats;
> + s64 rc, i;
> +
> + /* Setup the out buffer */
> + if (buff_stats) {
> + memcpy(buff_stats->eye_catcher,
> + PAPR_SCM_PERF_STATS_EYECATCHER, 8);
> + buff_stats->stats_version =
> + cpu_to_be32(PAPR_SCM_PERF_STATS_VERSION);
> + buff_stats->num_statistics =
> + cpu_to_be32(num_stats);
> + } else {
> + /* In case of no out buffer ignore the size */
> + size = 0;
> + }
> +
> + /*
> + * Do the HCALL asking PHYP for info and if R4 was requested
> + * return its value in 'out' variable.
> + */
> + rc = plpar_hcall(H_SCM_PERFORMANCE_STATS, ret, p->drc_index,
> + virt_to_phys(buff_stats), size);
> + if (out)
> + *out = ret[0];
> +
> + if (rc == H_PARTIAL) {
> + dev_err(&p->pdev->dev,
> + "Unknown performance stats, Err:0x%016lX\n", ret[0]);
> + return -ENOENT;
> + } else if (rc != H_SUCCESS) {
> + dev_err(&p->pdev->dev,
> + "Failed to query performance stats, Err:%lld\n", rc);
> + return -ENXIO;
May be just -1? ENXIO is that a suitable error return here?
> + }
> +
> + /* Successfully fetched the requested stats from phyp */
> + if (size != 0) {
> + buff_stats->num_statistics =
> + be32_to_cpu(buff_stats->num_statistics);
> +
> + /* Transform the stats buffer values from BE to cpu native */
> + for (i = 0, stats = buff_stats->scm_statistic;
> + i < buff_stats->num_statistics; ++i) {
> + stats[i].statistic_value =
> + be64_to_cpu(stats[i].statistic_value);
> + }
> + dev_dbg(&p->pdev->dev,
> + "Performance stats returned %d stats\n",
> + buff_stats->num_statistics);
> + } else {
> + /* Handle case where stat buffer size was requested */
> + dev_dbg(&p->pdev->dev,
> + "Performance stats size %ld\n", ret[0]);
> + }
> +
> + return 0;
> +}
> +
> /*
> * Issue hcall to retrieve dimm health info and populate papr_scm_priv with the
> * health information.
> @@ -631,6 +721,45 @@ static int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc,
> return 0;
> }
>
> +static ssize_t perf_stats_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + int index, rc;
> + struct seq_buf s;
> + struct papr_scm_perf_stat *stat;
> + struct papr_scm_perf_stats *stats;
> + struct nvdimm *dimm = to_nvdimm(dev);
> + struct papr_scm_priv *p = nvdimm_provider_data(dimm);
> +
> + if (!p->len_stat_buffer)
> + return -ENOENT;
> +
> + /* Allocate the buffer for phyp where stats are written */
> + stats = kzalloc(p->len_stat_buffer, GFP_KERNEL);
> + if (!stats)
> + return -ENOMEM;
> +
> + /* Ask phyp to return all dimm perf stats */
> + rc = drc_pmem_query_stats(p, stats, p->len_stat_buffer, 0, NULL);
> + if (!rc) {
> + /*
> + * Go through the returned output buffer and print stats and
> + * values. Since statistic_id is essentially a char string of
> + * 8 bytes, simply use the string format specifier to print it.
> + */
> + seq_buf_init(&s, buf, PAGE_SIZE);
> + for (index = 0, stat = stats->scm_statistic;
> + index < stats->num_statistics; ++index, ++stat) {
> + seq_buf_printf(&s, "%.8s = 0x%016llX\n",
> + stat->statistic_id, stat->statistic_value);
That is raw number (statistic_id). Is that useful? Can we map them to user readable
strings?
> + }
> + }
> +
> + kfree(stats);
> + return rc ? rc : seq_buf_used(&s);
> +}
> +DEVICE_ATTR_RO(perf_stats);
> +
> static ssize_t flags_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> @@ -676,6 +805,7 @@ DEVICE_ATTR_RO(flags);
> /* papr_scm specific dimm attributes */
> static struct attribute *papr_nd_attributes[] = {
> &dev_attr_flags.attr,
> + &dev_attr_perf_stats.attr,
> NULL,
> };
>
> @@ -696,6 +826,7 @@ static int papr_scm_nvdimm_init(struct papr_scm_priv *p)
> struct nd_region_desc ndr_desc;
> unsigned long dimm_flags;
> int target_nid, online_nid;
> + u64 stat_size;
>
> p->bus_desc.ndctl = papr_scm_ndctl;
> p->bus_desc.module = THIS_MODULE;
> @@ -759,6 +890,14 @@ static int papr_scm_nvdimm_init(struct papr_scm_priv *p)
> dev_info(dev, "Region registered with target node %d and online node %d",
> target_nid, online_nid);
>
> + /* Try retriving the stat buffer and see if its supported */
> + if (!drc_pmem_query_stats(p, NULL, 0, 0, &stat_size)) {
> + p->len_stat_buffer = (size_t)stat_size;
> + dev_dbg(&p->pdev->dev, "Max perf-stat size %lu-bytes\n",
> + p->len_stat_buffer);
> + } else {
> + dev_info(&p->pdev->dev, "Limited dimm stat info available\n");
> + }
> return 0;
>
> err: nvdimm_bus_unregister(p->bus);
^ permalink raw reply
* Re: [PATCH] ASoC: fsl_easrc: Fix uninitialized scalar variable in fsl_easrc_set_ctx_format
From: Nicolin Chen @ 2020-06-23 5:34 UTC (permalink / raw)
To: Shengjiu Wang
Cc: alsa-devel, timur, Xiubo.Lee, linuxppc-dev, tiwai, lgirdwood,
perex, broonie, festevam, linux-kernel
In-Reply-To: <1592816611-16297-1-git-send-email-shengjiu.wang@nxp.com>
On Mon, Jun 22, 2020 at 05:03:31PM +0800, Shengjiu Wang wrote:
> The "ret" in fsl_easrc_set_ctx_format is not initialized, then
> the unknown value maybe returned by this function.
>
> Fixes: 955ac624058f ("ASoC: fsl_easrc: Add EASRC ASoC CPU DAI drivers")
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
Acked-by: Nicolin Chen <nicoleotsuka@gmail.com>
^ permalink raw reply
* Re: [PATCH] ASoC: fsl_mqs: Fix unchecked return value for clk_prepare_enable
From: Shengjiu Wang @ 2020-06-23 5:13 UTC (permalink / raw)
To: Markus Elfring
Cc: Linux-ALSA, Timur Tabi, Xiubo Li, Fabio Estevam, Shengjiu Wang,
kernel-janitors, Takashi Iwai, linux-kernel, Nicolin Chen,
Mark Brown, linuxppc-dev
In-Reply-To: <3eab889e-75b6-6287-a668-a2eaa509834c@web.de>
On Tue, Jun 23, 2020 at 12:08 AM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> > Fix unchecked return value for clk_prepare_enable.
> >
> > And because clk_prepare_enable and clk_disable_unprepare should
> > check input clock parameter is NULL or not, then we don't need
> > to check it before calling the function.
>
> I propose to split the adjustment of two function implementations
> into separate update steps for a small patch series.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=625d3449788f85569096780592549d0340e9c0c7#n138
>
> I suggest to improve the change descriptions accordingly.
ok, will update the patches in v2.
best regards
wang shengjiu
^ permalink raw reply
* Re: [PATCH 4/4] powerpc/pseries/iommu: Remove default DMA window before creating DDW
From: Alexey Kardashevskiy @ 2020-06-23 3:52 UTC (permalink / raw)
To: Leonardo Bras, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Thiago Jung Bauermann, Ram Pai
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <03e82e1a1bcf516d01ca472546d8b31e468aba8b.camel@gmail.com>
On 23/06/2020 12:43, Leonardo Bras wrote:
> On Tue, 2020-06-23 at 12:35 +1000, Alexey Kardashevskiy wrote:
>>> I am not sure if this is true in general, but in this device (SR-IOV
>>> VF) I am testing it will return 0 windows if the default DMA window is
>>> not deleted, and 1 after it's deleted.
>>
>> Since pHyp can only create windows in "64bit space", now (I did not know
>> until a few month back) I expect that thing to return "1" always no
>> matter what happened to the default window. And removing the default
>> window will only affect the maximum number of TCEs but not the number of
>> possible windows.
>
> Humm, something gone wrong then.
>
> This patchset was developed mostly because on testing, DDW would never
> be created because windows_available would always be 0.
? On phyp, if there is a huge window, then it can be 0 or 1. On KVM, it
is 0 or 1 or 2.
>
> I will take a deeper look in that.
>
> Best regards,
> Leonardo
>
--
Alexey
^ permalink raw reply
* Re: [PATCH 4/4] powerpc/pseries/iommu: Remove default DMA window before creating DDW
From: Leonardo Bras @ 2020-06-23 2:43 UTC (permalink / raw)
To: Alexey Kardashevskiy, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Thiago Jung Bauermann, Ram Pai
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <cfb197e1-c608-71f9-5c98-c120a3496266@ozlabs.ru>
On Tue, 2020-06-23 at 12:35 +1000, Alexey Kardashevskiy wrote:
> > I am not sure if this is true in general, but in this device (SR-IOV
> > VF) I am testing it will return 0 windows if the default DMA window is
> > not deleted, and 1 after it's deleted.
>
> Since pHyp can only create windows in "64bit space", now (I did not know
> until a few month back) I expect that thing to return "1" always no
> matter what happened to the default window. And removing the default
> window will only affect the maximum number of TCEs but not the number of
> possible windows.
Humm, something gone wrong then.
This patchset was developed mostly because on testing, DDW would never
be created because windows_available would always be 0.
I will take a deeper look in that.
Best regards,
Leonardo
^ permalink raw reply
* Re: [PATCH 4/4] powerpc/pseries/iommu: Remove default DMA window before creating DDW
From: Alexey Kardashevskiy @ 2020-06-23 2:35 UTC (permalink / raw)
To: Leonardo Bras, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Thiago Jung Bauermann, Ram Pai
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <205edd45b7bbf39d2fc1d2d75fd7e35336f109ac.camel@gmail.com>
On 23/06/2020 12:31, Leonardo Bras wrote:
> On Tue, 2020-06-23 at 11:11 +1000, Alexey Kardashevskiy wrote:
>>
>> On 23/06/2020 04:59, Leonardo Bras wrote:
>>> Hello Alexey, thanks for the feedback!
>>>
>>> On Mon, 2020-06-22 at 20:02 +1000, Alexey Kardashevskiy wrote:
>>>> On 19/06/2020 15:06, Leonardo Bras wrote:
>>>>> On LoPAR "DMA Window Manipulation Calls", it's recommended to remove the
>>>>> default DMA window for the device, before attempting to configure a DDW,
>>>>> in order to make the maximum resources available for the next DDW to be
>>>>> created.
>>>>>
>>>>> This is a requirement for some devices to use DDW, given they only
>>>>> allow one DMA window.
>>>>>
>>>>> If setting up a new DDW fails anywhere after the removal of this
>>>>> default DMA window, restore it using reset_dma_window.
>>>>
>>>> Nah... If we do it like this, then under pHyp we lose 32bit DMA for good
>>>> as pHyp can only create a single window and it has to map at
>>>> 0x800.0000.0000.0000. They probably do not care though.
>>>>
>>>> Under KVM, this will fail as VFIO allows creating 2 windows and it
>>>> starts from 0 but the existing iommu_bypass_supported_pSeriesLP() treats
>>>> the window address == 0 as a failure. And we want to keep both DMA
>>>> windows for PCI adapters with both 64bit and 32bit PCI functions (I
>>>> heard AMD GPU video + audio are like this) or someone could hotplug
>>>> 32bit DMA device on a vphb with already present 64bit DMA window so we
>>>> do not remove the default window.
>>>
>>> Well, then I would suggest doing something like this:
>>> query_ddw(...);
>>> if (query.windows_available == 0){
>>> remove_dma_window(...,default_win);
>>> query_ddw(...);
>>> }
>>>
>>> This would make sure to cover cases of windows available == 1
>>> and windows available > 1;
>>
>> Is "1" what pHyp returns on query? And was it always like that? Then it
>> is probably ok. I just never really explored the idea of removing the
>> default window as we did not have to.
>
> I am not sure if this is true in general, but in this device (SR-IOV
> VF) I am testing it will return 0 windows if the default DMA window is
> not deleted, and 1 after it's deleted.
Since pHyp can only create windows in "64bit space", now (I did not know
until a few month back) I expect that thing to return "1" always no
matter what happened to the default window. And removing the default
window will only affect the maximum number of TCEs but not the number of
possible windows.
--
Alexey
^ permalink raw reply
* Re: [PATCH 4/4] powerpc/pseries/iommu: Remove default DMA window before creating DDW
From: Leonardo Bras @ 2020-06-23 2:31 UTC (permalink / raw)
To: Alexey Kardashevskiy, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Thiago Jung Bauermann, Ram Pai
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <afd1c5ac-d291-5281-1592-a345ee3c0c8c@ozlabs.ru>
On Tue, 2020-06-23 at 11:11 +1000, Alexey Kardashevskiy wrote:
>
> On 23/06/2020 04:59, Leonardo Bras wrote:
> > Hello Alexey, thanks for the feedback!
> >
> > On Mon, 2020-06-22 at 20:02 +1000, Alexey Kardashevskiy wrote:
> > > On 19/06/2020 15:06, Leonardo Bras wrote:
> > > > On LoPAR "DMA Window Manipulation Calls", it's recommended to remove the
> > > > default DMA window for the device, before attempting to configure a DDW,
> > > > in order to make the maximum resources available for the next DDW to be
> > > > created.
> > > >
> > > > This is a requirement for some devices to use DDW, given they only
> > > > allow one DMA window.
> > > >
> > > > If setting up a new DDW fails anywhere after the removal of this
> > > > default DMA window, restore it using reset_dma_window.
> > >
> > > Nah... If we do it like this, then under pHyp we lose 32bit DMA for good
> > > as pHyp can only create a single window and it has to map at
> > > 0x800.0000.0000.0000. They probably do not care though.
> > >
> > > Under KVM, this will fail as VFIO allows creating 2 windows and it
> > > starts from 0 but the existing iommu_bypass_supported_pSeriesLP() treats
> > > the window address == 0 as a failure. And we want to keep both DMA
> > > windows for PCI adapters with both 64bit and 32bit PCI functions (I
> > > heard AMD GPU video + audio are like this) or someone could hotplug
> > > 32bit DMA device on a vphb with already present 64bit DMA window so we
> > > do not remove the default window.
> >
> > Well, then I would suggest doing something like this:
> > query_ddw(...);
> > if (query.windows_available == 0){
> > remove_dma_window(...,default_win);
> > query_ddw(...);
> > }
> >
> > This would make sure to cover cases of windows available == 1
> > and windows available > 1;
>
> Is "1" what pHyp returns on query? And was it always like that? Then it
> is probably ok. I just never really explored the idea of removing the
> default window as we did not have to.
I am not sure if this is true in general, but in this device (SR-IOV
VF) I am testing it will return 0 windows if the default DMA window is
not deleted, and 1 after it's deleted.
>
>
> > > The last discussed thing I remember was that there was supposed to be a
> > > new bit in "ibm,architecture-vec-5" (forgot the details), we could use
> > > that to decide whether to keep the default window or not, like this.
> >
> > I checked on the latest LoPAR draft (soon to be published), for the
> > ibm,architecture-vec 'option array 5' and this entry was the only
> > recently added one that is related to this patchset:
> >
> > Byte 8 - Bit 0:
> > SRIOV Virtual Functions Support Dynamic DMA Windows (DDW):
> > 0: SRIOV Virtual Functions do not support DDW
> > 1: SRIOV Virtual Functions do support DDW
> >
> > Isn't this equivalent to having a "ibm,ddw-applicable" property?
>
> I am not sure, is there anything else to this new bit?
I copied everything from the LoPAR, and IIRC the ACR for this change
only described this change in the document.
> I'd think if the
> client supports it, then pHyp will create one 64bit window per a PE and
> DDW API won't be needed. Thanks,
That would make sense, and be great.
I will dig some more.
Thank you!
> > > > Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
> > > > ---
> > > > arch/powerpc/platforms/pseries/iommu.c | 20 +++++++++++++++++---
> > > > 1 file changed, 17 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
> > > > index de633f6ae093..68d1ea957ac7 100644
> > > > --- a/arch/powerpc/platforms/pseries/iommu.c
> > > > +++ b/arch/powerpc/platforms/pseries/iommu.c
> > > > @@ -1074,8 +1074,9 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)
> > > > u64 dma_addr, max_addr;
> > > > struct device_node *dn;
> > > > u32 ddw_avail[3];
> > > > +
> > > > struct direct_window *window;
> > > > - struct property *win64;
> > > > + struct property *win64, *dfl_win;
> > >
> > > Make it "default_win" or "def_win", "dfl" hurts to read :)
> >
> > Sure, no problem :)
> >
> > > > struct dynamic_dma_window_prop *ddwprop;
> > > > struct failed_ddw_pdn *fpdn;
> > > >
> > > > @@ -1110,8 +1111,19 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)
> > > > if (ret)
> > > > goto out_failed;
> > > >
> > > > - /*
> > > > - * Query if there is a second window of size to map the
> > > > + /*
> > > > + * First step of setting up DDW is removing the default DMA window,
> > > > + * if it's present. It will make all the resources available to the
> > > > + * new DDW window.
> > > > + * If anything fails after this, we need to restore it.
> > > > + */
> > > > +
> > > > + dfl_win = of_find_property(pdn, "ibm,dma-window", NULL);
> > > > + if (dfl_win)
> > > > + remove_dma_window(pdn, ddw_avail, dfl_win);
> > >
> > > Before doing so, you want to make sure that the "reset" is actually
> > > supported. Thanks,
> >
> > Good catch, I will improve that.
> >
> > >
> > > > +
> > > > + /*
> > > > + * Query if there is a window of size to map the
> > > > * whole partition. Query returns number of windows, largest
> > > > * block assigned to PE (partition endpoint), and two bitmasks
> > > > * of page sizes: supported and supported for migrate-dma.
> > > > @@ -1219,6 +1231,8 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)
> > > > kfree(win64);
> > > >
> > > > out_failed:
> > > > + if (dfl_win)
> > > > + reset_dma_window(dev, pdn);
> > > >
> > > > fpdn = kzalloc(sizeof(*fpdn), GFP_KERNEL);
> > > > if (!fpdn)
> > > >
> >
> > Best regards,
> > Leonardo
> >
^ permalink raw reply
* Re: [PATCH 1/4] powerpc/pseries/iommu: Update call to ibm,query-pe-dma-windows
From: Alexey Kardashevskiy @ 2020-06-23 2:29 UTC (permalink / raw)
To: Leonardo Bras
Cc: Ram Pai, linux-kernel, Paul Mackerras, linuxppc-dev,
Thiago Jung Bauermann
In-Reply-To: <c331742c9f7a3e3ccead5d9db99a66d3f268b95f.camel@gmail.com>
On 23/06/2020 12:14, Leonardo Bras wrote:
> On Tue, 2020-06-23 at 11:12 +1000, Alexey Kardashevskiy wrote:
> [snip]
>>>>> +static int query_ddw_out_sz(struct device_node *par_dn)
>>>>
>>>> Can easily be folded into query_ddw().
>>>
>>> Sure, but it will get inlined by the compiler, and I think it reads
>>> better this way.
>>> I mean, I understand you have a reason to think it's better to fold it
>>> in query_ddw(), and I would like to better understand that to improve
>>> my code in the future.
>>
>> You have numbers 5 and 6 (the number of parameters) twice in the file,
>> this is why I brought it up. query_ddw_out_sz() can potentially return
>> something else than 5 or 6 and you will have to change the callsite(s)
>> then, since these are not macros, this allows to think there may be more
>> places with 5 and 6. Dunno. A single function will simplify things imho.
>
> That's a good point. Thanks!
>
>>
>>
>>>>> +{
>>>>> + int ret;
>>>>> + u32 ddw_ext[3];
>>>>> +
>>>>> + ret = of_property_read_u32_array(par_dn, "ibm,ddw-extensions",
>>>>> + &ddw_ext[0], 3);
>>>>> + if (ret || ddw_ext[0] < 2 || ddw_ext[2] != 1)
>>>>
>>>> Oh that PAPR thing again :-/
>>>>
>>>> ===
>>>> The “ibm,ddw-extensions” property value is a list of integers the first
>>>> integer indicates the number of extensions implemented and subsequent
>>>> integers, one per extension, provide a value associated with that
>>>> extension.
>>>> ===
>>>>
>>>> So ddw_ext[0] is length.
>>>> Listindex==2 is for "reset" says PAPR and
>>>> Listindex==3 is for this new 64bit "largest_available_block".
>>>>
>>>> So I'd expect ddw_ext[2] to have the "reset" token and ddw_ext[3] to
>>>> have "1" for this new feature but indexes are smaller. I am confused.
>>>> Either way these "2" and "3" needs to be defined in macros, "0" probably
>>>> too.
>>>
>>> Remember these indexes are not C-like 0-starting indexes, where the
>>> size would be Listindex==1.
>>
>> Yeah I can see that is the assumption but out of curiosity - is it
>> written anywhere? Across PAPR, they index bytes from 1 but bits from 0 :-/
>
> From LoPAR:
> The “ibm,ddw-extensions” property value is a list of integers the first
> integer indicates the number of extensions implemented and subsequent
> integers, one per extension, provide a value associated with that
> extension.
>
> And the list/table then shows extensions from 2 on onwards:
> List index 2 : Token of the ibm,reset-pe-dma-windows RTAS Call
> (...)
I means a place saying "we number things starting from 1 and not from
zero", this kind of thing. The code implementing the spec uses the C
language so it would make sense to count from zero, otoh the writer
probably did not write any code for ages :)
--
Alexey
^ permalink raw reply
* Re: [PATCH 3/4] powerpc/pseries/iommu: Move window-removing part of remove_ddw into remove_dma_window
From: Leonardo Bras @ 2020-06-23 2:26 UTC (permalink / raw)
To: Oliver O'Halloran, Alexey Kardashevskiy
Cc: Ram Pai, Linux Kernel Mailing List, Paul Mackerras, linuxppc-dev,
Thiago Jung Bauermann
In-Reply-To: <CAOSf1CEC-tYH1so5b4P7dQ7s8v1o4qy_u5CG7LKtKNnRQvC4-w@mail.gmail.com>
On Tue, 2020-06-23 at 11:33 +1000, Oliver O'Halloran wrote:
> On Tue, Jun 23, 2020 at 11:12 AM Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> > On 23/06/2020 04:59, Leonardo Bras wrote:
> > > > Also, despite this particular file, the "pdn" name is usually used for
> > > > struct pci_dn (not device_node), let's keep it that way.
> > >
> > > Sure, I got confused for some time about this, as we have:
> > > static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn).
> > > but on *_ddw() we have "struct pci_dn *pdn".
> >
> > True again, not the cleanest style here.
> >
> >
> > > I will also add a patch that renames those 'struct device_node *pdn' to
> > > something like 'struct device_node *parent_dn'.
>
> I usually go with "np" or "node". In this case I'd use "parent_np" or
> just "parent." As you said pci_dn conventionally uses pdn so that
> should be avoided if at all possible. There's some places that just
> use "dn" for device_node, but I don't think that's something we should
> encourage due to how similar it is to pdn.
Sure, I will try that.
>
> > I would not go that far, we (well, Oliver) are getting rid of many
> > occurrences of pci_dn and Oliver may have a stronger opinion here.
>
> I'm trying to remove the use of pci_dn from non-RTAS platforms which
> doesn't apply to pseries. For RTAS platforms having pci_dn sort of
> makes sense since it's used to cache data from the device_node and
> having it saves you from needing to parse and validate the DT at
> runtime since we're supposed to be relying on the FW provided settings
> in the DT. I want to get rid of it on PowerNV because it's become a
> dumping ground for random bits and pieces of platform specific data.
> It's confusing at best and IMO it duplicates a lot of what's already
> available in the per-PHB structures which the platform specific stuff
> should actually be looking at.
>
> Oliver
Best regards,
Leonardo Bras
^ permalink raw reply
* Re: [PATCH 3/4] powerpc/pseries/iommu: Move window-removing part of remove_ddw into remove_dma_window
From: Leonardo Bras @ 2020-06-23 2:22 UTC (permalink / raw)
To: Alexey Kardashevskiy, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Thiago Jung Bauermann, Ram Pai
Cc: Oliver, linuxppc-dev, linux-kernel
In-Reply-To: <887bf30e-ae9e-b0cb-0388-dc555692ff0a@ozlabs.ru>
On Tue, 2020-06-23 at 11:12 +1000, Alexey Kardashevskiy wrote:
>
> On 23/06/2020 04:59, Leonardo Bras wrote:
> > Hello Alexey, thanks for the feedback!
> >
> > On Mon, 2020-06-22 at 20:02 +1000, Alexey Kardashevskiy wrote:
> > > On 19/06/2020 15:06, Leonardo Bras wrote:
> > > > Move the window-removing part of remove_ddw into a new function
> > > > (remove_dma_window), so it can be used to remove other DMA windows.
> > > >
> > > > It's useful for removing DMA windows that don't create DIRECT64_PROPNAME
> > > > property, like the default DMA window from the device, which uses
> > > > "ibm,dma-window".
> > > >
> > > > Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
> > > > ---
> > > > arch/powerpc/platforms/pseries/iommu.c | 53 +++++++++++++++-----------
> > > > 1 file changed, 31 insertions(+), 22 deletions(-)
> > > >
> > > > diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
> > > > index 5e1fbc176a37..de633f6ae093 100644
> > > > --- a/arch/powerpc/platforms/pseries/iommu.c
> > > > +++ b/arch/powerpc/platforms/pseries/iommu.c
> > > > @@ -767,25 +767,14 @@ static int __init disable_ddw_setup(char *str)
> > > >
> > > > early_param("disable_ddw", disable_ddw_setup);
> > > >
> > > > -static void remove_ddw(struct device_node *np, bool remove_prop)
> > > > +static void remove_dma_window(struct device_node *pdn, u32 *ddw_avail,
> > >
> > > You do not need the entire ddw_avail here, pass just the token you need.
> >
> > Well, I just emulated the behavior of create_ddw() and query_ddw() as
> > both just pass the array instead of the token, even though they only
> > use a single token.
>
> True, there is a pattern.
>
> > I think it's to make the rest of the code independent of the design of
> > the "ibm,ddw-applicable" array, and if it changes, only local changes
> > on the functions will be needed.
>
> The helper removes a window, if you are going to call other operations
> in remove_dma_window(), then you'll have to change its name ;)
Not only doing new stuff, it can change the order for some reason (as
the order of the output of query), and it would need not change the
caller.
>
>
> > > Also, despite this particular file, the "pdn" name is usually used for
> > > struct pci_dn (not device_node), let's keep it that way.
> >
> > Sure, I got confused for some time about this, as we have:
> > static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn).
> > but on *_ddw() we have "struct pci_dn *pdn".
>
> True again, not the cleanest style here.
>
>
> > I will also add a patch that renames those 'struct device_node *pdn' to
> > something like 'struct device_node *parent_dn'.
>
> I would not go that far, we (well, Oliver) are getting rid of many
> occurrences of pci_dn and Oliver may have a stronger opinion here.
>
>
> > > > + struct property *win)
> > > > {
> > > > struct dynamic_dma_window_prop *dwp;
> > > > - struct property *win64;
> > > > - u32 ddw_avail[3];
> > > > u64 liobn;
> > > > - int ret = 0;
> > > > -
> > > > - ret = of_property_read_u32_array(np, "ibm,ddw-applicable",
> > > > - &ddw_avail[0], 3);
> > > > -
> > > > - win64 = of_find_property(np, DIRECT64_PROPNAME, NULL);
> > > > - if (!win64)
> > > > - return;
> > > > -
> > > > - if (ret || win64->length < sizeof(*dwp))
> > > > - goto delprop;
> > > > + int ret;
> > > >
> > > > - dwp = win64->value;
> > > > + dwp = win->value;
> > > > liobn = (u64)be32_to_cpu(dwp->liobn);
> > > >
> > > > /* clear the whole window, note the arg is in kernel pages */
> > > > @@ -793,24 +782,44 @@ static void remove_ddw(struct device_node *np, bool remove_prop)
> > > > 1ULL << (be32_to_cpu(dwp->window_shift) - PAGE_SHIFT), dwp);
> > > > if (ret)
> > > > pr_warn("%pOF failed to clear tces in window.\n",
> > > > - np);
> > > > + pdn);
> > > > else
> > > > pr_debug("%pOF successfully cleared tces in window.\n",
> > > > - np);
> > > > + pdn);
> > > >
> > > > ret = rtas_call(ddw_avail[2], 1, 1, NULL, liobn);
> > > > if (ret)
> > > > pr_warn("%pOF: failed to remove direct window: rtas returned "
> > > > "%d to ibm,remove-pe-dma-window(%x) %llx\n",
> > > > - np, ret, ddw_avail[2], liobn);
> > > > + pdn, ret, ddw_avail[2], liobn);
> > > > else
> > > > pr_debug("%pOF: successfully removed direct window: rtas returned "
> > > > "%d to ibm,remove-pe-dma-window(%x) %llx\n",
> > > > - np, ret, ddw_avail[2], liobn);
> > > > + pdn, ret, ddw_avail[2], liobn);
> > > > +}
> > > > +
> > > > +static void remove_ddw(struct device_node *np, bool remove_prop)
> > > > +{
> > > > + struct property *win;
> > > > + u32 ddw_avail[3];
> > > > + int ret = 0;
> > > > +
> > > > + ret = of_property_read_u32_array(np, "ibm,ddw-applicable",
> > > > + &ddw_avail[0], 3);
> > > > + if (ret)
> > > > + return;
> > > > +
> > > > + win = of_find_property(np, DIRECT64_PROPNAME, NULL);
> > > > + if (!win)
> > > > + return;
> > > > +
> > > > + if (win->length >= sizeof(struct dynamic_dma_window_prop))
> > >
> > > Any good reason not to make it "=="? Is there something optional or we
> > > expect extension (which may not grow from the end but may add cells in
> > > between). Thanks,
> >
> > Well, it comes from the old behavior of remove_ddw():
> > - if (ret || win64->length < sizeof(*dwp))
> > - goto delprop;
> > As I reversed the logic from 'if (test) go out' to 'if (!test) do
> > stuff', I also reversed (a < b) to !(a < b) => (a >= b).
> >
> > I have no problem changing that to '==', but it will produce a
> > different behavior than before.
>
> I missed than, never mind then.
>
>
> > >
> > > > + remove_dma_window(np, ddw_avail, win);
> > > > +
> > > > + if (!remove_prop)
> > > > + return;
> > > >
> > > > -delprop:
> > > > - if (remove_prop)
> > > > - ret = of_remove_property(np, win64);
> > > > + ret = of_remove_property(np, win);
> > > > if (ret)
> > > > pr_warn("%pOF: failed to remove direct window property: %d\n",
> > > > np, ret);
> > > >
> >
> > Best regards,
> > Leonardo
> >
^ permalink raw reply
* Re: [PATCH 2/4] powerpc/pseries/iommu: Implement ibm,reset-pe-dma-windows rtas call
From: Leonardo Bras @ 2020-06-23 2:20 UTC (permalink / raw)
To: Alexey Kardashevskiy, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Thiago Jung Bauermann, Ram Pai
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <c02fbebb-32ed-f328-ff93-ab2201844d61@ozlabs.ru>
On Tue, 2020-06-23 at 11:11 +1000, Alexey Kardashevskiy wrote:
>
> On 23/06/2020 04:58, Leonardo Bras wrote:
> > Hello Alexey, thanks for the feedback!
> >
> > On Mon, 2020-06-22 at 20:02 +1000, Alexey Kardashevskiy wrote:
> > > On 19/06/2020 15:06, Leonardo Bras wrote:
> > > > Platforms supporting the DDW option starting with LoPAR level 2.7 implement
> > > > ibm,ddw-extensions. The first extension available (index 2) carries the
> > > > token for ibm,reset-pe-dma-windows rtas call, which is used to restore
> > > > the default DMA window for a device, if it has been deleted.
> > > >
> > > > It does so by resetting the TCE table allocation for the PE to it's
> > > > boot time value, available in "ibm,dma-window" device tree node.
> > > >
> > > > Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
> > > > ---
> > > > arch/powerpc/platforms/pseries/iommu.c | 33 ++++++++++++++++++++++++++
> > > > 1 file changed, 33 insertions(+)
> > > >
> > > > diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
> > > > index e5a617738c8b..5e1fbc176a37 100644
> > > > --- a/arch/powerpc/platforms/pseries/iommu.c
> > > > +++ b/arch/powerpc/platforms/pseries/iommu.c
> > > > @@ -1012,6 +1012,39 @@ static phys_addr_t ddw_memory_hotplug_max(void)
> > > > return max_addr;
> > > > }
> > > >
> > > > +/*
> > > > + * Platforms supporting the DDW option starting with LoPAR level 2.7 implement
> > > > + * ibm,ddw-extensions, which carries the rtas token for
> > > > + * ibm,reset-pe-dma-windows.
> > > > + * That rtas-call can be used to restore the default DMA window for the device.
> > > > + */
> > > > +static void reset_dma_window(struct pci_dev *dev, struct device_node *par_dn)
> > > > +{
> > > > + int ret;
> > > > + u32 cfg_addr, ddw_ext[3];
> > > > + u64 buid;
> > > > + struct device_node *dn;
> > > > + struct pci_dn *pdn;
> > > > +
> > > > + ret = of_property_read_u32_array(par_dn, "ibm,ddw-extensions",
> > > > + &ddw_ext[0], 3);
> > >
> > > s/3/2/ as for the reset extension you do not need the "64bit largest
> > > block" extension.
> >
> > Sure, I will update this.
> >
> > >
> > > > + if (ret)
> > > > + return;
> > > > +
> > > > + dn = pci_device_to_OF_node(dev);
> > > > + pdn = PCI_DN(dn);
> > > > + buid = pdn->phb->buid;
> > > > + cfg_addr = ((pdn->busno << 16) | (pdn->devfn << 8));
> > > > +
> > > > + ret = rtas_call(ddw_ext[1], 3, 1, NULL, cfg_addr,
> > >
> > > Here the "reset" extention is in ddw_ext[1]. Hm. 1/4 has a bug then.
> >
> > Humm, in 1/4 I used dd_ext[0] (how many extensions) and ddw_ext[2] (64-
> > bit largest window count). I fail to see the bug here.
>
> There is none, my bad :)
>
>
> > > And I am pretty sure it won't compile as reset_dma_window() is not used
> > > and it is static so fold it into one the next patches. Thanks,
> >
> > Sure, I will do that.
> > I was questioning myself about this and thought it would be better to
> > split for easier revision.
>
> People separate things when a patch is really huge but even then I miss
> the point - I'd really like to see a new function _and_ its uses in the
> same patch, otherwise I either need to jump between mails or apply the
> series, either is little but extra work :) Thanks,
Sure, that makes sense.
I will keep that in mind for future patchsets (and v2).
Thank you!
>
>
> > >
> > > > + BUID_HI(buid), BUID_LO(buid));
> > > > + if (ret)
> > > > + dev_info(&dev->dev,
> > > > + "ibm,reset-pe-dma-windows(%x) %x %x %x returned %d ",
> > > > + ddw_ext[1], cfg_addr, BUID_HI(buid), BUID_LO(buid),
> > > > + ret);
> > > > +}
> > > > +
> > > > /*
> > > > * If the PE supports dynamic dma windows, and there is space for a table
> > > > * that can map all pages in a linear offset, then setup such a table,
> > > >
> >
> > Best regards,
> > Leonardo
> >
^ permalink raw reply
* Re: [PATCH 1/4] powerpc/pseries/iommu: Update call to ibm,query-pe-dma-windows
From: Leonardo Bras @ 2020-06-23 2:14 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Ram Pai, linux-kernel, Paul Mackerras, linuxppc-dev,
Thiago Jung Bauermann
In-Reply-To: <4176ea2b-c778-2f59-ba57-7339b873ead5@ozlabs.ru>
On Tue, 2020-06-23 at 11:12 +1000, Alexey Kardashevskiy wrote:
[snip]
> > > > +static int query_ddw_out_sz(struct device_node *par_dn)
> > >
> > > Can easily be folded into query_ddw().
> >
> > Sure, but it will get inlined by the compiler, and I think it reads
> > better this way.
> > I mean, I understand you have a reason to think it's better to fold it
> > in query_ddw(), and I would like to better understand that to improve
> > my code in the future.
>
> You have numbers 5 and 6 (the number of parameters) twice in the file,
> this is why I brought it up. query_ddw_out_sz() can potentially return
> something else than 5 or 6 and you will have to change the callsite(s)
> then, since these are not macros, this allows to think there may be more
> places with 5 and 6. Dunno. A single function will simplify things imho.
That's a good point. Thanks!
>
>
> > > > +{
> > > > + int ret;
> > > > + u32 ddw_ext[3];
> > > > +
> > > > + ret = of_property_read_u32_array(par_dn, "ibm,ddw-extensions",
> > > > + &ddw_ext[0], 3);
> > > > + if (ret || ddw_ext[0] < 2 || ddw_ext[2] != 1)
> > >
> > > Oh that PAPR thing again :-/
> > >
> > > ===
> > > The “ibm,ddw-extensions” property value is a list of integers the first
> > > integer indicates the number of extensions implemented and subsequent
> > > integers, one per extension, provide a value associated with that
> > > extension.
> > > ===
> > >
> > > So ddw_ext[0] is length.
> > > Listindex==2 is for "reset" says PAPR and
> > > Listindex==3 is for this new 64bit "largest_available_block".
> > >
> > > So I'd expect ddw_ext[2] to have the "reset" token and ddw_ext[3] to
> > > have "1" for this new feature but indexes are smaller. I am confused.
> > > Either way these "2" and "3" needs to be defined in macros, "0" probably
> > > too.
> >
> > Remember these indexes are not C-like 0-starting indexes, where the
> > size would be Listindex==1.
>
> Yeah I can see that is the assumption but out of curiosity - is it
> written anywhere? Across PAPR, they index bytes from 1 but bits from 0 :-/
From LoPAR:
The “ibm,ddw-extensions” property value is a list of integers the first
integer indicates the number of extensions implemented and subsequent
integers, one per extension, provide a value associated with that
extension.
And the list/table then shows extensions from 2 on onwards:
List index 2 : Token of the ibm,reset-pe-dma-windows RTAS Call
(...)
> Either way make them macros.
>
>
> > Basically, in C-like array it's :
> > a[0] == size,
> > a[1] == reset_token,
> > a[2] == new 64bit "largest_available_block"
> >
> > > Please post 'lsprop "ibm,ddw-extensions"' here. Thanks,
> >
> > Sure:
> > [root@host pci@800000029004005]# lsprop "ibm,ddw-extensions"
> > ibm,dd
> > w-extensions
> > 00000002 00000056 00000000
>
> Right, good. Thanks,
Thank you for reviewing!
>
> >
> > > > + return 5;
> > > > + return 6;
> > > > +}
> > > > +
> > > > static int query_ddw(struct pci_dev *dev, const u32 *ddw_avail,
> > > > - struct ddw_query_response *query)
> > > > + struct ddw_query_response *query,
> > > > + struct device_node *par_dn)
> > > > {
> > > > struct device_node *dn;
> > > > struct pci_dn *pdn;
> > > > - u32 cfg_addr;
> > > > + u32 cfg_addr, query_out[5];
> > > > u64 buid;
> > > > - int ret;
> > > > + int ret, out_sz;
> > > >
> > > > /*
> > > > * Get the config address and phb buid of the PE window.
> > > > @@ -888,12 +906,29 @@ static int query_ddw(struct pci_dev *dev, const u32 *ddw_avail,
> > > > pdn = PCI_DN(dn);
> > > > buid = pdn->phb->buid;
> > > > cfg_addr = ((pdn->busno << 16) | (pdn->devfn << 8));
> > > > + out_sz = query_ddw_out_sz(par_dn);
> > > > +
> > > > + ret = rtas_call(ddw_avail[0], 3, out_sz, query_out,
> > > > + cfg_addr, BUID_HI(buid), BUID_LO(buid));
> > > > + dev_info(&dev->dev, "ibm,query-pe-dma-windows(%x) %x %x %x returned %d\n",
> > > > + ddw_avail[0], cfg_addr, BUID_HI(buid), BUID_LO(buid), ret);
> > > > +
> > > > + switch (out_sz) {
> > > > + case 5:
> > > > + query->windows_available = query_out[0];
> > > > + query->largest_available_block = query_out[1];
> > > > + query->page_size = query_out[2];
> > > > + query->migration_capable = query_out[3];
> > > > + break;
> > > > + case 6:
> > > > + query->windows_available = query_out[0];
> > > > + query->largest_available_block = ((u64)query_out[1] << 32) |
> > > > + query_out[2];
> > > > + query->page_size = query_out[3];
> > > > + query->migration_capable = query_out[4];
> > > > + break;
> > > > + }
> > > >
> > > > - ret = rtas_call(ddw_avail[0], 3, 5, (u32 *)query,
> > > > - cfg_addr, BUID_HI(buid), BUID_LO(buid));
> > > > - dev_info(&dev->dev, "ibm,query-pe-dma-windows(%x) %x %x %x"
> > > > - " returned %d\n", ddw_avail[0], cfg_addr, BUID_HI(buid),
> > > > - BUID_LO(buid), ret);
> > > > return ret;
> > > > }
> > > >
> > > > @@ -1040,7 +1075,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)
> > > > * of page sizes: supported and supported for migrate-dma.
> > > > */
> > > > dn = pci_device_to_OF_node(dev);
> > > > - ret = query_ddw(dev, ddw_avail, &query);
> > > > + ret = query_ddw(dev, ddw_avail, &query, pdn);
> > > > if (ret != 0)
> > > > goto out_failed;
> > > >
> > > > @@ -1068,7 +1103,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)
> > > > /* check largest block * page size > max memory hotplug addr */
> > > > max_addr = ddw_memory_hotplug_max();
> > > > if (query.largest_available_block < (max_addr >> page_shift)) {
> > > > - dev_dbg(&dev->dev, "can't map partition max 0x%llx with %u "
> > > > + dev_dbg(&dev->dev, "can't map partition max 0x%llx with %llu "
> > > > "%llu-sized pages\n", max_addr, query.largest_available_block,
> > > > 1ULL << page_shift);
> > > > goto out_failed;
> > > >
> >
> > Best regards,
> > Leonardo
> >
Best regards,
Leonardo Bras
^ permalink raw reply
* Re: [PATCH 3/4] powerpc/pseries/iommu: Move window-removing part of remove_ddw into remove_dma_window
From: Oliver O'Halloran @ 2020-06-23 1:33 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Leonardo Bras, Ram Pai, Linux Kernel Mailing List, Paul Mackerras,
linuxppc-dev, Thiago Jung Bauermann
In-Reply-To: <887bf30e-ae9e-b0cb-0388-dc555692ff0a@ozlabs.ru>
On Tue, Jun 23, 2020 at 11:12 AM Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>
> On 23/06/2020 04:59, Leonardo Bras wrote:
> >
> >> Also, despite this particular file, the "pdn" name is usually used for
> >> struct pci_dn (not device_node), let's keep it that way.
> >
> > Sure, I got confused for some time about this, as we have:
> > static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn).
> > but on *_ddw() we have "struct pci_dn *pdn".
>
> True again, not the cleanest style here.
>
>
> > I will also add a patch that renames those 'struct device_node *pdn' to
> > something like 'struct device_node *parent_dn'.
I usually go with "np" or "node". In this case I'd use "parent_np" or
just "parent." As you said pci_dn conventionally uses pdn so that
should be avoided if at all possible. There's some places that just
use "dn" for device_node, but I don't think that's something we should
encourage due to how similar it is to pdn.
> I would not go that far, we (well, Oliver) are getting rid of many
> occurrences of pci_dn and Oliver may have a stronger opinion here.
I'm trying to remove the use of pci_dn from non-RTAS platforms which
doesn't apply to pseries. For RTAS platforms having pci_dn sort of
makes sense since it's used to cache data from the device_node and
having it saves you from needing to parse and validate the DT at
runtime since we're supposed to be relying on the FW provided settings
in the DT. I want to get rid of it on PowerNV because it's become a
dumping ground for random bits and pieces of platform specific data.
It's confusing at best and IMO it duplicates a lot of what's already
available in the per-PHB structures which the platform specific stuff
should actually be looking at.
Oliver
^ permalink raw reply
* Re: [PATCH 3/4] powerpc/pseries/iommu: Move window-removing part of remove_ddw into remove_dma_window
From: Alexey Kardashevskiy @ 2020-06-23 1:12 UTC (permalink / raw)
To: Leonardo Bras, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Thiago Jung Bauermann, Ram Pai
Cc: Oliver, linuxppc-dev, linux-kernel
In-Reply-To: <ccf7b591f2bf61ba4705699b2e2b050c3cf48d99.camel@gmail.com>
On 23/06/2020 04:59, Leonardo Bras wrote:
> Hello Alexey, thanks for the feedback!
>
> On Mon, 2020-06-22 at 20:02 +1000, Alexey Kardashevskiy wrote:
>>
>> On 19/06/2020 15:06, Leonardo Bras wrote:
>>> Move the window-removing part of remove_ddw into a new function
>>> (remove_dma_window), so it can be used to remove other DMA windows.
>>>
>>> It's useful for removing DMA windows that don't create DIRECT64_PROPNAME
>>> property, like the default DMA window from the device, which uses
>>> "ibm,dma-window".
>>>
>>> Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
>>> ---
>>> arch/powerpc/platforms/pseries/iommu.c | 53 +++++++++++++++-----------
>>> 1 file changed, 31 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
>>> index 5e1fbc176a37..de633f6ae093 100644
>>> --- a/arch/powerpc/platforms/pseries/iommu.c
>>> +++ b/arch/powerpc/platforms/pseries/iommu.c
>>> @@ -767,25 +767,14 @@ static int __init disable_ddw_setup(char *str)
>>>
>>> early_param("disable_ddw", disable_ddw_setup);
>>>
>>> -static void remove_ddw(struct device_node *np, bool remove_prop)
>>> +static void remove_dma_window(struct device_node *pdn, u32 *ddw_avail,
>>
>> You do not need the entire ddw_avail here, pass just the token you need.
>
> Well, I just emulated the behavior of create_ddw() and query_ddw() as
> both just pass the array instead of the token, even though they only
> use a single token.
True, there is a pattern.
> I think it's to make the rest of the code independent of the design of
> the "ibm,ddw-applicable" array, and if it changes, only local changes
> on the functions will be needed.
The helper removes a window, if you are going to call other operations
in remove_dma_window(), then you'll have to change its name ;)
>> Also, despite this particular file, the "pdn" name is usually used for
>> struct pci_dn (not device_node), let's keep it that way.
>
> Sure, I got confused for some time about this, as we have:
> static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn).
> but on *_ddw() we have "struct pci_dn *pdn".
True again, not the cleanest style here.
> I will also add a patch that renames those 'struct device_node *pdn' to
> something like 'struct device_node *parent_dn'.
I would not go that far, we (well, Oliver) are getting rid of many
occurrences of pci_dn and Oliver may have a stronger opinion here.
>
>>> + struct property *win)
>>> {
>>> struct dynamic_dma_window_prop *dwp;
>>> - struct property *win64;
>>> - u32 ddw_avail[3];
>>> u64 liobn;
>>> - int ret = 0;
>>> -
>>> - ret = of_property_read_u32_array(np, "ibm,ddw-applicable",
>>> - &ddw_avail[0], 3);
>>> -
>>> - win64 = of_find_property(np, DIRECT64_PROPNAME, NULL);
>>> - if (!win64)
>>> - return;
>>> -
>>> - if (ret || win64->length < sizeof(*dwp))
>>> - goto delprop;
>>> + int ret;
>>>
>>> - dwp = win64->value;
>>> + dwp = win->value;
>>> liobn = (u64)be32_to_cpu(dwp->liobn);
>>>
>>> /* clear the whole window, note the arg is in kernel pages */
>>> @@ -793,24 +782,44 @@ static void remove_ddw(struct device_node *np, bool remove_prop)
>>> 1ULL << (be32_to_cpu(dwp->window_shift) - PAGE_SHIFT), dwp);
>>> if (ret)
>>> pr_warn("%pOF failed to clear tces in window.\n",
>>> - np);
>>> + pdn);
>>> else
>>> pr_debug("%pOF successfully cleared tces in window.\n",
>>> - np);
>>> + pdn);
>>>
>>> ret = rtas_call(ddw_avail[2], 1, 1, NULL, liobn);
>>> if (ret)
>>> pr_warn("%pOF: failed to remove direct window: rtas returned "
>>> "%d to ibm,remove-pe-dma-window(%x) %llx\n",
>>> - np, ret, ddw_avail[2], liobn);
>>> + pdn, ret, ddw_avail[2], liobn);
>>> else
>>> pr_debug("%pOF: successfully removed direct window: rtas returned "
>>> "%d to ibm,remove-pe-dma-window(%x) %llx\n",
>>> - np, ret, ddw_avail[2], liobn);
>>> + pdn, ret, ddw_avail[2], liobn);
>>> +}
>>> +
>>> +static void remove_ddw(struct device_node *np, bool remove_prop)
>>> +{
>>> + struct property *win;
>>> + u32 ddw_avail[3];
>>> + int ret = 0;
>>> +
>>> + ret = of_property_read_u32_array(np, "ibm,ddw-applicable",
>>> + &ddw_avail[0], 3);
>>> + if (ret)
>>> + return;
>>> +
>>> + win = of_find_property(np, DIRECT64_PROPNAME, NULL);
>>> + if (!win)
>>> + return;
>>> +
>>> + if (win->length >= sizeof(struct dynamic_dma_window_prop))
>>
>> Any good reason not to make it "=="? Is there something optional or we
>> expect extension (which may not grow from the end but may add cells in
>> between). Thanks,
>
> Well, it comes from the old behavior of remove_ddw():
> - if (ret || win64->length < sizeof(*dwp))
> - goto delprop;
> As I reversed the logic from 'if (test) go out' to 'if (!test) do
> stuff', I also reversed (a < b) to !(a < b) => (a >= b).
>
> I have no problem changing that to '==', but it will produce a
> different behavior than before.
I missed than, never mind then.
>
>>
>>
>>> + remove_dma_window(np, ddw_avail, win);
>>> +
>>> + if (!remove_prop)
>>> + return;
>>>
>>> -delprop:
>>> - if (remove_prop)
>>> - ret = of_remove_property(np, win64);
>>> + ret = of_remove_property(np, win);
>>> if (ret)
>>> pr_warn("%pOF: failed to remove direct window property: %d\n",
>>> np, ret);
>>>
>
> Best regards,
> Leonardo
>
--
Alexey
^ permalink raw reply
* Re: [PATCH 1/4] powerpc/pseries/iommu: Update call to ibm,query-pe-dma-windows
From: Alexey Kardashevskiy @ 2020-06-23 1:12 UTC (permalink / raw)
To: Leonardo Bras
Cc: Ram Pai, linux-kernel, Paul Mackerras, linuxppc-dev,
Thiago Jung Bauermann
In-Reply-To: <c15189a5c77752ea62022608dab28601965afaaa.camel@gmail.com>
On 23/06/2020 04:58, Leonardo Bras wrote:
> Hello Alexey, thank you for the feedback!
>
> On Mon, 2020-06-22 at 20:02 +1000, Alexey Kardashevskiy wrote:
>>
>> On 19/06/2020 15:06, Leonardo Bras wrote:
>>> From LoPAR level 2.8, "ibm,ddw-extensions" index 3 can make the number of
>>> outputs from "ibm,query-pe-dma-windows" go from 5 to 6.
>>>
>>> This change of output size is meant to expand the address size of
>>> largest_available_block PE TCE from 32-bit to 64-bit, which ends up
>>> shifting page_size and migration_capable.
>>>
>>> This ends up requiring the update of
>>> ddw_query_response->largest_available_block from u32 to u64, and manually
>>> assigning the values from the buffer into this struct, according to
>>> output size.
>>>
>>> Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
>>> ---
>>> arch/powerpc/platforms/pseries/iommu.c | 57 +++++++++++++++++++++-----
>>> 1 file changed, 46 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
>>> index 6d47b4a3ce39..e5a617738c8b 100644
>>> --- a/arch/powerpc/platforms/pseries/iommu.c
>>> +++ b/arch/powerpc/platforms/pseries/iommu.c
>>> @@ -334,7 +334,7 @@ struct direct_window {
>>> /* Dynamic DMA Window support */
>>> struct ddw_query_response {
>>> u32 windows_available;
>>> - u32 largest_available_block;
>>> + u64 largest_available_block;
>>> u32 page_size;
>>> u32 migration_capable;
>>> };
>>> @@ -869,14 +869,32 @@ static int find_existing_ddw_windows(void)
>>> }
>>> machine_arch_initcall(pseries, find_existing_ddw_windows);
>>>
>>> +/*
>>> + * From LoPAR level 2.8, "ibm,ddw-extensions" index 3 can rule how many output
>>> + * parameters ibm,query-pe-dma-windows will have, ranging from 5 to 6.
>>> + */
>>> +
>>> +static int query_ddw_out_sz(struct device_node *par_dn)
>>
>> Can easily be folded into query_ddw().
>
> Sure, but it will get inlined by the compiler, and I think it reads
> better this way.
> I mean, I understand you have a reason to think it's better to fold it
> in query_ddw(), and I would like to better understand that to improve
> my code in the future.
You have numbers 5 and 6 (the number of parameters) twice in the file,
this is why I brought it up. query_ddw_out_sz() can potentially return
something else than 5 or 6 and you will have to change the callsite(s)
then, since these are not macros, this allows to think there may be more
places with 5 and 6. Dunno. A single function will simplify things imho.
>
>>> +{
>>> + int ret;
>>> + u32 ddw_ext[3];
>>> +
>>> + ret = of_property_read_u32_array(par_dn, "ibm,ddw-extensions",
>>> + &ddw_ext[0], 3);
>>> + if (ret || ddw_ext[0] < 2 || ddw_ext[2] != 1)
>>
>> Oh that PAPR thing again :-/
>>
>> ===
>> The “ibm,ddw-extensions” property value is a list of integers the first
>> integer indicates the number of extensions implemented and subsequent
>> integers, one per extension, provide a value associated with that
>> extension.
>> ===
>>
>> So ddw_ext[0] is length.
>> Listindex==2 is for "reset" says PAPR and
>> Listindex==3 is for this new 64bit "largest_available_block".
>>
>> So I'd expect ddw_ext[2] to have the "reset" token and ddw_ext[3] to
>> have "1" for this new feature but indexes are smaller. I am confused.
>> Either way these "2" and "3" needs to be defined in macros, "0" probably
>> too.
>
> Remember these indexes are not C-like 0-starting indexes, where the
> size would be Listindex==1.
Yeah I can see that is the assumption but out of curiosity - is it
written anywhere? Across PAPR, they index bytes from 1 but bits from 0 :-/
Either way make them macros.
> Basically, in C-like array it's :
> a[0] == size,
> a[1] == reset_token,
> a[2] == new 64bit "largest_available_block"
>
>> Please post 'lsprop "ibm,ddw-extensions"' here. Thanks,
>
> Sure:
> [root@host pci@800000029004005]# lsprop "ibm,ddw-extensions"
> ibm,dd
> w-extensions
> 00000002 00000056 00000000
Right, good. Thanks,
>
>
>>
>>> + return 5;
>>> + return 6;
>>> +}
>>> +
>>> static int query_ddw(struct pci_dev *dev, const u32 *ddw_avail,
>>> - struct ddw_query_response *query)
>>> + struct ddw_query_response *query,
>>> + struct device_node *par_dn)
>>> {
>>> struct device_node *dn;
>>> struct pci_dn *pdn;
>>> - u32 cfg_addr;
>>> + u32 cfg_addr, query_out[5];
>>> u64 buid;
>>> - int ret;
>>> + int ret, out_sz;
>>>
>>> /*
>>> * Get the config address and phb buid of the PE window.
>>> @@ -888,12 +906,29 @@ static int query_ddw(struct pci_dev *dev, const u32 *ddw_avail,
>>> pdn = PCI_DN(dn);
>>> buid = pdn->phb->buid;
>>> cfg_addr = ((pdn->busno << 16) | (pdn->devfn << 8));
>>> + out_sz = query_ddw_out_sz(par_dn);
>>> +
>>> + ret = rtas_call(ddw_avail[0], 3, out_sz, query_out,
>>> + cfg_addr, BUID_HI(buid), BUID_LO(buid));
>>> + dev_info(&dev->dev, "ibm,query-pe-dma-windows(%x) %x %x %x returned %d\n",
>>> + ddw_avail[0], cfg_addr, BUID_HI(buid), BUID_LO(buid), ret);
>>> +
>>> + switch (out_sz) {
>>> + case 5:
>>> + query->windows_available = query_out[0];
>>> + query->largest_available_block = query_out[1];
>>> + query->page_size = query_out[2];
>>> + query->migration_capable = query_out[3];
>>> + break;
>>> + case 6:
>>> + query->windows_available = query_out[0];
>>> + query->largest_available_block = ((u64)query_out[1] << 32) |
>>> + query_out[2];
>>> + query->page_size = query_out[3];
>>> + query->migration_capable = query_out[4];
>>> + break;
>>> + }
>>>
>>> - ret = rtas_call(ddw_avail[0], 3, 5, (u32 *)query,
>>> - cfg_addr, BUID_HI(buid), BUID_LO(buid));
>>> - dev_info(&dev->dev, "ibm,query-pe-dma-windows(%x) %x %x %x"
>>> - " returned %d\n", ddw_avail[0], cfg_addr, BUID_HI(buid),
>>> - BUID_LO(buid), ret);
>>> return ret;
>>> }
>>>
>>> @@ -1040,7 +1075,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)
>>> * of page sizes: supported and supported for migrate-dma.
>>> */
>>> dn = pci_device_to_OF_node(dev);
>>> - ret = query_ddw(dev, ddw_avail, &query);
>>> + ret = query_ddw(dev, ddw_avail, &query, pdn);
>>> if (ret != 0)
>>> goto out_failed;
>>>
>>> @@ -1068,7 +1103,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)
>>> /* check largest block * page size > max memory hotplug addr */
>>> max_addr = ddw_memory_hotplug_max();
>>> if (query.largest_available_block < (max_addr >> page_shift)) {
>>> - dev_dbg(&dev->dev, "can't map partition max 0x%llx with %u "
>>> + dev_dbg(&dev->dev, "can't map partition max 0x%llx with %llu "
>>> "%llu-sized pages\n", max_addr, query.largest_available_block,
>>> 1ULL << page_shift);
>>> goto out_failed;
>>>
>
> Best regards,
> Leonardo
>
--
Alexey
^ permalink raw reply
* Re: [PATCH 2/4] powerpc/pseries/iommu: Implement ibm,reset-pe-dma-windows rtas call
From: Alexey Kardashevskiy @ 2020-06-23 1:11 UTC (permalink / raw)
To: Leonardo Bras, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Thiago Jung Bauermann, Ram Pai
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <4180fd9bb0409a9c7009fef3ccae8eb2ad46d0a2.camel@gmail.com>
On 23/06/2020 04:58, Leonardo Bras wrote:
> Hello Alexey, thanks for the feedback!
>
> On Mon, 2020-06-22 at 20:02 +1000, Alexey Kardashevskiy wrote:
>>
>> On 19/06/2020 15:06, Leonardo Bras wrote:
>>> Platforms supporting the DDW option starting with LoPAR level 2.7 implement
>>> ibm,ddw-extensions. The first extension available (index 2) carries the
>>> token for ibm,reset-pe-dma-windows rtas call, which is used to restore
>>> the default DMA window for a device, if it has been deleted.
>>>
>>> It does so by resetting the TCE table allocation for the PE to it's
>>> boot time value, available in "ibm,dma-window" device tree node.
>>>
>>> Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
>>> ---
>>> arch/powerpc/platforms/pseries/iommu.c | 33 ++++++++++++++++++++++++++
>>> 1 file changed, 33 insertions(+)
>>>
>>> diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
>>> index e5a617738c8b..5e1fbc176a37 100644
>>> --- a/arch/powerpc/platforms/pseries/iommu.c
>>> +++ b/arch/powerpc/platforms/pseries/iommu.c
>>> @@ -1012,6 +1012,39 @@ static phys_addr_t ddw_memory_hotplug_max(void)
>>> return max_addr;
>>> }
>>>
>>> +/*
>>> + * Platforms supporting the DDW option starting with LoPAR level 2.7 implement
>>> + * ibm,ddw-extensions, which carries the rtas token for
>>> + * ibm,reset-pe-dma-windows.
>>> + * That rtas-call can be used to restore the default DMA window for the device.
>>> + */
>>> +static void reset_dma_window(struct pci_dev *dev, struct device_node *par_dn)
>>> +{
>>> + int ret;
>>> + u32 cfg_addr, ddw_ext[3];
>>> + u64 buid;
>>> + struct device_node *dn;
>>> + struct pci_dn *pdn;
>>> +
>>> + ret = of_property_read_u32_array(par_dn, "ibm,ddw-extensions",
>>> + &ddw_ext[0], 3);
>>
>> s/3/2/ as for the reset extension you do not need the "64bit largest
>> block" extension.
>
> Sure, I will update this.
>
>>
>>
>>> + if (ret)
>>> + return;
>>> +
>>> + dn = pci_device_to_OF_node(dev);
>>> + pdn = PCI_DN(dn);
>>> + buid = pdn->phb->buid;
>>> + cfg_addr = ((pdn->busno << 16) | (pdn->devfn << 8));
>>> +
>>> + ret = rtas_call(ddw_ext[1], 3, 1, NULL, cfg_addr,
>>
>> Here the "reset" extention is in ddw_ext[1]. Hm. 1/4 has a bug then.
>
> Humm, in 1/4 I used dd_ext[0] (how many extensions) and ddw_ext[2] (64-
> bit largest window count). I fail to see the bug here.
There is none, my bad :)
>> And I am pretty sure it won't compile as reset_dma_window() is not used
>> and it is static so fold it into one the next patches. Thanks,
>
> Sure, I will do that.
> I was questioning myself about this and thought it would be better to
> split for easier revision.
People separate things when a patch is really huge but even then I miss
the point - I'd really like to see a new function _and_ its uses in the
same patch, otherwise I either need to jump between mails or apply the
series, either is little but extra work :) Thanks,
>>
>>
>>> + BUID_HI(buid), BUID_LO(buid));
>>> + if (ret)
>>> + dev_info(&dev->dev,
>>> + "ibm,reset-pe-dma-windows(%x) %x %x %x returned %d ",
>>> + ddw_ext[1], cfg_addr, BUID_HI(buid), BUID_LO(buid),
>>> + ret);
>>> +}
>>> +
>>> /*
>>> * If the PE supports dynamic dma windows, and there is space for a table
>>> * that can map all pages in a linear offset, then setup such a table,
>>>
>
> Best regards,
> Leonardo
>
--
Alexey
^ permalink raw reply
* Re: [PATCH 4/4] powerpc/pseries/iommu: Remove default DMA window before creating DDW
From: Alexey Kardashevskiy @ 2020-06-23 1:11 UTC (permalink / raw)
To: Leonardo Bras, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Thiago Jung Bauermann, Ram Pai
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <4bf1d32da3d13a44e3c2e4b04f369fe52c24a023.camel@gmail.com>
On 23/06/2020 04:59, Leonardo Bras wrote:
> Hello Alexey, thanks for the feedback!
>
> On Mon, 2020-06-22 at 20:02 +1000, Alexey Kardashevskiy wrote:
>>
>> On 19/06/2020 15:06, Leonardo Bras wrote:
>>> On LoPAR "DMA Window Manipulation Calls", it's recommended to remove the
>>> default DMA window for the device, before attempting to configure a DDW,
>>> in order to make the maximum resources available for the next DDW to be
>>> created.
>>>
>>> This is a requirement for some devices to use DDW, given they only
>>> allow one DMA window.
>>>
>>> If setting up a new DDW fails anywhere after the removal of this
>>> default DMA window, restore it using reset_dma_window.
>>
>> Nah... If we do it like this, then under pHyp we lose 32bit DMA for good
>> as pHyp can only create a single window and it has to map at
>> 0x800.0000.0000.0000. They probably do not care though.
>>
>> Under KVM, this will fail as VFIO allows creating 2 windows and it
>> starts from 0 but the existing iommu_bypass_supported_pSeriesLP() treats
>> the window address == 0 as a failure. And we want to keep both DMA
>> windows for PCI adapters with both 64bit and 32bit PCI functions (I
>> heard AMD GPU video + audio are like this) or someone could hotplug
>> 32bit DMA device on a vphb with already present 64bit DMA window so we
>> do not remove the default window.
>
> Well, then I would suggest doing something like this:
> query_ddw(...);
> if (query.windows_available == 0){
> remove_dma_window(...,default_win);
> query_ddw(...);
> }
>
> This would make sure to cover cases of windows available == 1
> and windows available > 1;
Is "1" what pHyp returns on query? And was it always like that? Then it
is probably ok. I just never really explored the idea of removing the
default window as we did not have to.
>> The last discussed thing I remember was that there was supposed to be a
>> new bit in "ibm,architecture-vec-5" (forgot the details), we could use
>> that to decide whether to keep the default window or not, like this.
>
> I checked on the latest LoPAR draft (soon to be published), for the
> ibm,architecture-vec 'option array 5' and this entry was the only
> recently added one that is related to this patchset:
>
> Byte 8 - Bit 0:
> SRIOV Virtual Functions Support Dynamic DMA Windows (DDW):
> 0: SRIOV Virtual Functions do not support DDW
> 1: SRIOV Virtual Functions do support DDW
>
> Isn't this equivalent to having a "ibm,ddw-applicable" property?
I am not sure, is there anything else to this new bit? I'd think if the
client supports it, then pHyp will create one 64bit window per a PE and
DDW API won't be needed. Thanks,
>
>
>>
>>> Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
>>> ---
>>> arch/powerpc/platforms/pseries/iommu.c | 20 +++++++++++++++++---
>>> 1 file changed, 17 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
>>> index de633f6ae093..68d1ea957ac7 100644
>>> --- a/arch/powerpc/platforms/pseries/iommu.c
>>> +++ b/arch/powerpc/platforms/pseries/iommu.c
>>> @@ -1074,8 +1074,9 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)
>>> u64 dma_addr, max_addr;
>>> struct device_node *dn;
>>> u32 ddw_avail[3];
>>> +
>>> struct direct_window *window;
>>> - struct property *win64;
>>> + struct property *win64, *dfl_win;
>>
>> Make it "default_win" or "def_win", "dfl" hurts to read :)
>
> Sure, no problem :)
>
>>
>>> struct dynamic_dma_window_prop *ddwprop;
>>> struct failed_ddw_pdn *fpdn;
>>>
>>> @@ -1110,8 +1111,19 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)
>>> if (ret)
>>> goto out_failed;
>>>
>>> - /*
>>> - * Query if there is a second window of size to map the
>>> + /*
>>> + * First step of setting up DDW is removing the default DMA window,
>>> + * if it's present. It will make all the resources available to the
>>> + * new DDW window.
>>> + * If anything fails after this, we need to restore it.
>>> + */
>>> +
>>> + dfl_win = of_find_property(pdn, "ibm,dma-window", NULL);
>>> + if (dfl_win)
>>> + remove_dma_window(pdn, ddw_avail, dfl_win);
>>
>> Before doing so, you want to make sure that the "reset" is actually
>> supported. Thanks,
>
> Good catch, I will improve that.
>
>>
>>
>>> +
>>> + /*
>>> + * Query if there is a window of size to map the
>>> * whole partition. Query returns number of windows, largest
>>> * block assigned to PE (partition endpoint), and two bitmasks
>>> * of page sizes: supported and supported for migrate-dma.
>>> @@ -1219,6 +1231,8 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)
>>> kfree(win64);
>>>
>>> out_failed:
>>> + if (dfl_win)
>>> + reset_dma_window(dev, pdn);
>>>
>>> fpdn = kzalloc(sizeof(*fpdn), GFP_KERNEL);
>>> if (!fpdn)
>>>
>
> Best regards,
> Leonardo
>
--
Alexey
^ permalink raw reply
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