2022-06-16 07:42:18

by Peng Fan (OSS)

Subject: [PATCH V2 8/9] interconnect: imx: configure NoC mode/prioriry/ext_control

From: Peng Fan <[email protected]>

Introduce imx_icc_noc_setting structure to describe a master port setting
Pass imx_icc_noc_setting as a parameter from specific driver
Set priority level, mode, ext control in imx_icc_node_set

Signed-off-by: Peng Fan <[email protected]>
---
drivers/interconnect/imx/imx.c | 43 ++++++++++++++++++++++++++----
drivers/interconnect/imx/imx.h | 44 ++++++++++++++++++++++++++++++-
drivers/interconnect/imx/imx8mm.c | 2 +-
drivers/interconnect/imx/imx8mn.c | 2 +-
drivers/interconnect/imx/imx8mq.c | 2 +-
5 files changed, 84 insertions(+), 9 deletions(-)

diff --git a/drivers/interconnect/imx/imx.c b/drivers/interconnect/imx/imx.c
index 78557fe6da2c..bd728caf2b85 100644
--- a/drivers/interconnect/imx/imx.c
+++ b/drivers/interconnect/imx/imx.c
@@ -10,6 +10,7 @@

#include <linux/device.h>
#include <linux/interconnect-provider.h>
+#include <linux/io.h>
#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_platform.h>
@@ -21,8 +22,10 @@
/* private icc_node data */
struct imx_icc_node {
const struct imx_icc_node_desc *desc;
+ const struct imx_icc_noc_setting *setting;
struct device *qos_dev;
struct dev_pm_qos_request qos_req;
+ struct imx_icc_provider *imx_provider;
};

static int imx_icc_get_bw(struct icc_node *node, u32 *avg, u32 *peak)
@@ -37,8 +40,24 @@ static int imx_icc_node_set(struct icc_node *node)
{
struct device *dev = node->provider->dev;
struct imx_icc_node *node_data = node->data;
+ void __iomem *base;
+ u32 prio;
u64 freq;

+ if (node_data->setting && !node_data->setting->ignore && node->peak_bw) {
+ base = node_data->setting->reg + node_data->imx_provider->noc_base;
+ if (node_data->setting->mode == IMX_NOC_MODE_FIXED) {
+ prio = node_data->setting->prio_level;
+ prio = PRIORITY_COMP_MARK | (prio << 8) | prio;
+ writel(prio, base + IMX_NOC_PRIO_REG);
+ writel(node_data->setting->mode, base + IMX_NOC_MODE_REG);
+ writel(node_data->setting->ext_control, base + IMX_NOC_EXT_CTL_REG);
+ } else {
+ dev_info(dev, "mode: %d not supported\n", node_data->setting->mode);
+ return -ENOTSUPP;
+ }
+ }
+
if (!node_data->qos_dev)
return 0;

@@ -135,7 +154,8 @@ static int imx_icc_node_init_qos(struct icc_provider *provider,
}

static struct icc_node *imx_icc_node_add(struct imx_icc_provider *imx_provider,
- const struct imx_icc_node_desc *node_desc)
+ const struct imx_icc_node_desc *node_desc,
+ const struct imx_icc_noc_setting *setting)
{
struct icc_provider *provider = &imx_provider->provider;
struct device *dev = provider->dev;
@@ -164,6 +184,8 @@ static struct icc_node *imx_icc_node_add(struct imx_icc_provider *imx_provider,
node->name = node_desc->name;
node->data = node_data;
node_data->desc = node_desc;
+ node_data->setting = setting;
+ node_data->imx_provider = imx_provider;
icc_node_add(node, provider);

if (node_desc->adj) {
@@ -187,7 +209,8 @@ static void imx_icc_unregister_nodes(struct icc_provider *provider)

static int imx_icc_register_nodes(struct imx_icc_provider *imx_provider,
const struct imx_icc_node_desc *descs,
- int count)
+ int count,
+ const struct imx_icc_noc_setting *settings)
{
struct icc_provider *provider = &imx_provider->provider;
struct icc_onecell_data *provider_data = provider->data;
@@ -199,7 +222,10 @@ static int imx_icc_register_nodes(struct imx_icc_provider *imx_provider,
const struct imx_icc_node_desc *node_desc = &descs[i];
size_t j;

- node = imx_icc_node_add(imx_provider, node_desc);
+ if (settings)
+ node = imx_icc_node_add(imx_provider, node_desc, &settings[node_desc->id]);
+ else
+ node = imx_icc_node_add(imx_provider, node_desc, NULL);
if (IS_ERR(node)) {
ret = dev_err_probe(provider->dev, PTR_ERR(node),
"failed to add %s\n", node_desc->name);
@@ -237,7 +263,8 @@ static int get_max_node_id(struct imx_icc_node_desc *nodes, int nodes_count)
}

int imx_icc_register(struct platform_device *pdev,
- struct imx_icc_node_desc *nodes, int nodes_count)
+ struct imx_icc_node_desc *nodes, int nodes_count,
+ struct imx_icc_noc_setting *settings)
{
struct device *dev = &pdev->dev;
struct icc_onecell_data *data;
@@ -267,13 +294,19 @@ int imx_icc_register(struct platform_device *pdev,
provider->dev->of_node = dev->parent->of_node;
platform_set_drvdata(pdev, imx_provider);

+ if (settings) {
+ imx_provider->noc_base = devm_of_iomap(dev, provider->dev->of_node, 0, NULL);
+ if (!imx_provider->noc_base)
+ return PTR_ERR(imx_provider->noc_base);
+ }
+
ret = icc_provider_add(provider);
if (ret) {
dev_err(dev, "error adding interconnect provider: %d\n", ret);
return ret;
}

- ret = imx_icc_register_nodes(imx_provider, nodes, nodes_count);
+ ret = imx_icc_register_nodes(imx_provider, nodes, nodes_count, settings);
if (ret)
goto provider_del;

diff --git a/drivers/interconnect/imx/imx.h b/drivers/interconnect/imx/imx.h
index 0ad2c654c222..1da87cfe27da 100644
--- a/drivers/interconnect/imx/imx.h
+++ b/drivers/interconnect/imx/imx.h
@@ -15,6 +15,31 @@

#define IMX_ICC_MAX_LINKS 4

+/*
+ * High throughput priority level in Regulator mode
+ * Read Priority in Fixed/Limiter mode
+ */
+#define PRIORITY0_SHIFT 0
+/*
+ * Low throughput priority level in Regulator mode
+ * Write Priority in Fixed/Limiter mode
+ */
+#define PRIORITY1_SHIFT 8
+#define PRIORITY_MASK 0x7
+
+#define PRIORITY_COMP_MARK BIT(31) /* Must set */
+
+#define IMX_NOC_MODE_FIXED 0
+#define IMX_NOC_MODE_LIMITER 1
+#define IMX_NOC_MODE_BYPASS 2
+#define IMX_NOC_MODE_REGULATOR 3
+
+#define IMX_NOC_PRIO_REG 0x8
+#define IMX_NOC_MODE_REG 0xC
+#define IMX_NOC_BANDWIDTH_REG 0x10
+#define IMX_NOC_SATURATION 0x14
+#define IMX_NOC_EXT_CTL_REG 0x18
+
struct imx_icc_provider {
void __iomem *noc_base;
struct icc_provider provider;
@@ -44,6 +69,22 @@ struct imx_icc_node_desc {
const struct imx_icc_node_adj_desc *adj;
};

+/*
+ * struct imx_icc_noc_setting - Describe an interconnect node setting
+ * @ignore: indicate whether need apply this setting
+ * @reg: register offset inside the NoC
+ * @prio_level: priority level
+ * @mode: functional mode
+ * @ext_control: external input control
+ */
+struct imx_icc_noc_setting {
+ bool ignore;
+ u32 reg;
+ u32 prio_level;
+ u32 mode;
+ u32 ext_control;
+};
+
#define DEFINE_BUS_INTERCONNECT(_name, _id, _adj, ...) \
{ \
.id = _id, \
@@ -61,7 +102,8 @@ struct imx_icc_node_desc {

int imx_icc_register(struct platform_device *pdev,
struct imx_icc_node_desc *nodes,
- int nodes_count);
+ int nodes_count,
+ struct imx_icc_noc_setting *noc_settings);
int imx_icc_unregister(struct platform_device *pdev);

#endif /* __DRIVERS_INTERCONNECT_IMX_H */
diff --git a/drivers/interconnect/imx/imx8mm.c b/drivers/interconnect/imx/imx8mm.c
index 1083490bb391..ae797412db96 100644
--- a/drivers/interconnect/imx/imx8mm.c
+++ b/drivers/interconnect/imx/imx8mm.c
@@ -83,7 +83,7 @@ static struct imx_icc_node_desc nodes[] = {

static int imx8mm_icc_probe(struct platform_device *pdev)
{
- return imx_icc_register(pdev, nodes, ARRAY_SIZE(nodes));
+ return imx_icc_register(pdev, nodes, ARRAY_SIZE(nodes), NULL);
}

static int imx8mm_icc_remove(struct platform_device *pdev)
diff --git a/drivers/interconnect/imx/imx8mn.c b/drivers/interconnect/imx/imx8mn.c
index ad97e55fd4e5..1ce94c5bdd8c 100644
--- a/drivers/interconnect/imx/imx8mn.c
+++ b/drivers/interconnect/imx/imx8mn.c
@@ -72,7 +72,7 @@ static struct imx_icc_node_desc nodes[] = {

static int imx8mn_icc_probe(struct platform_device *pdev)
{
- return imx_icc_register(pdev, nodes, ARRAY_SIZE(nodes));
+ return imx_icc_register(pdev, nodes, ARRAY_SIZE(nodes), NULL);
}

static int imx8mn_icc_remove(struct platform_device *pdev)
diff --git a/drivers/interconnect/imx/imx8mq.c b/drivers/interconnect/imx/imx8mq.c
index d7768d3c6d8a..7f00a0511c6e 100644
--- a/drivers/interconnect/imx/imx8mq.c
+++ b/drivers/interconnect/imx/imx8mq.c
@@ -82,7 +82,7 @@ static struct imx_icc_node_desc nodes[] = {

static int imx8mq_icc_probe(struct platform_device *pdev)
{
- return imx_icc_register(pdev, nodes, ARRAY_SIZE(nodes));
+ return imx_icc_register(pdev, nodes, ARRAY_SIZE(nodes), NULL);
}

static int imx8mq_icc_remove(struct platform_device *pdev)
--
2.25.1

2022-06-28 18:36:45

by Georgi Djakov

Subject: Re: [PATCH V2 8/9] interconnect: imx: configure NoC mode/prioriry/ext_control

Hi Peng,

On 16.06.22 10:33, Peng Fan (OSS) wrote:
> From: Peng Fan <[email protected]>
>
> Introduce imx_icc_noc_setting structure to describe a master port setting
> Pass imx_icc_noc_setting as a parameter from specific driver
> Set priority level, mode, ext control in imx_icc_node_set
>
> Signed-off-by: Peng Fan <[email protected]>
> ---
> drivers/interconnect/imx/imx.c | 43 ++++++++++++++++++++++++++----
> drivers/interconnect/imx/imx.h | 44 ++++++++++++++++++++++++++++++-
> drivers/interconnect/imx/imx8mm.c | 2 +-
> drivers/interconnect/imx/imx8mn.c | 2 +-
> drivers/interconnect/imx/imx8mq.c | 2 +-
> 5 files changed, 84 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/interconnect/imx/imx.c b/drivers/interconnect/imx/imx.c
> index 78557fe6da2c..bd728caf2b85 100644

[..]

> @@ -37,8 +40,24 @@ static int imx_icc_node_set(struct icc_node *node)
> {
> struct device *dev = node->provider->dev;
> struct imx_icc_node *node_data = node->data;
> + void __iomem *base;
> + u32 prio;
> u64 freq;
>
> + if (node_data->setting && !node_data->setting->ignore && node->peak_bw) {
> + base = node_data->setting->reg + node_data->imx_provider->noc_base;
> + if (node_data->setting->mode == IMX_NOC_MODE_FIXED) {
> + prio = node_data->setting->prio_level;
> + prio = PRIORITY_COMP_MARK | (prio << 8) | prio;
> + writel(prio, base + IMX_NOC_PRIO_REG);
> + writel(node_data->setting->mode, base + IMX_NOC_MODE_REG);
> + writel(node_data->setting->ext_control, base + IMX_NOC_EXT_CTL_REG);
> + } else {
> + dev_info(dev, "mode: %d not supported\n", node_data->setting->mode);
> + return -ENOTSUPP;

Nit: I believe that -EOPNOTSUPP is the preferred error code.

> + }
> + }
> +
> if (!node_data->qos_dev)
> return 0;
>

[..]

> @@ -237,7 +263,8 @@ static int get_max_node_id(struct imx_icc_node_desc *nodes, int nodes_count)
> }
>
> int imx_icc_register(struct platform_device *pdev,
> - struct imx_icc_node_desc *nodes, int nodes_count)
> + struct imx_icc_node_desc *nodes, int nodes_count,
> + struct imx_icc_noc_setting *settings)
> {
> struct device *dev = &pdev->dev;
> struct icc_onecell_data *data;
> @@ -267,13 +294,19 @@ int imx_icc_register(struct platform_device *pdev,
> provider->dev->of_node = dev->parent->of_node;
> platform_set_drvdata(pdev, imx_provider);
>
> + if (settings) {
> + imx_provider->noc_base = devm_of_iomap(dev, provider->dev->of_node, 0, NULL);
> + if (!imx_provider->noc_base)

devm_of_iomap() returns ERR_PTR(). So we should check it with IS_ERR().

Thanks,
Georgi

> + return PTR_ERR(imx_provider->noc_base);
> + }
> +
> ret = icc_provider_add(provider);
> if (ret) {
> dev_err(dev, "error adding interconnect provider: %d\n", ret);
> return ret;
> }
>
> - ret = imx_icc_register_nodes(imx_provider, nodes, nodes_count);
> + ret = imx_icc_register_nodes(imx_provider, nodes, nodes_count, settings);
> if (ret)
> goto provider_del;
>

2022-06-29 15:25:25

by Lucas Stach

Subject: Re: [PATCH V2 8/9] interconnect: imx: configure NoC mode/prioriry/ext_control

Am Donnerstag, dem 16.06.2022 um 15:33 +0800 schrieb Peng Fan (OSS):
> From: Peng Fan <[email protected]>
>
> Introduce imx_icc_noc_setting structure to describe a master port setting
> Pass imx_icc_noc_setting as a parameter from specific driver
> Set priority level, mode, ext control in imx_icc_node_set
>
> Signed-off-by: Peng Fan <[email protected]>
> ---
> drivers/interconnect/imx/imx.c | 43 ++++++++++++++++++++++++++----
> drivers/interconnect/imx/imx.h | 44 ++++++++++++++++++++++++++++++-
> drivers/interconnect/imx/imx8mm.c | 2 +-
> drivers/interconnect/imx/imx8mn.c | 2 +-
> drivers/interconnect/imx/imx8mq.c | 2 +-
> 5 files changed, 84 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/interconnect/imx/imx.c b/drivers/interconnect/imx/imx.c
> index 78557fe6da2c..bd728caf2b85 100644
> --- a/drivers/interconnect/imx/imx.c
> +++ b/drivers/interconnect/imx/imx.c
> @@ -10,6 +10,7 @@
>
> #include <linux/device.h>
> #include <linux/interconnect-provider.h>
> +#include <linux/io.h>
> #include <linux/module.h>
> #include <linux/of.h>
> #include <linux/of_platform.h>
> @@ -21,8 +22,10 @@
> /* private icc_node data */
> struct imx_icc_node {
> const struct imx_icc_node_desc *desc;
> + const struct imx_icc_noc_setting *setting;
> struct device *qos_dev;
> struct dev_pm_qos_request qos_req;
> + struct imx_icc_provider *imx_provider;
> };
>
> static int imx_icc_get_bw(struct icc_node *node, u32 *avg, u32 *peak)
> @@ -37,8 +40,24 @@ static int imx_icc_node_set(struct icc_node *node)
> {
> struct device *dev = node->provider->dev;
> struct imx_icc_node *node_data = node->data;
> + void __iomem *base;
> + u32 prio;
> u64 freq;
>
> + if (node_data->setting && !node_data->setting->ignore && node->peak_bw) {
> + base = node_data->setting->reg + node_data->imx_provider->noc_base;
> + if (node_data->setting->mode == IMX_NOC_MODE_FIXED) {
> + prio = node_data->setting->prio_level;
> + prio = PRIORITY_COMP_MARK | (prio << 8) | prio;
> + writel(prio, base + IMX_NOC_PRIO_REG);
> + writel(node_data->setting->mode, base + IMX_NOC_MODE_REG);
> + writel(node_data->setting->ext_control, base + IMX_NOC_EXT_CTL_REG);
> + } else {
> + dev_info(dev, "mode: %d not supported\n", node_data->setting->mode);
> + return -ENOTSUPP;
> + }
> + }
> +
> if (!node_data->qos_dev)
> return 0;
>
> @@ -135,7 +154,8 @@ static int imx_icc_node_init_qos(struct icc_provider *provider,
> }
>
> static struct icc_node *imx_icc_node_add(struct imx_icc_provider *imx_provider,
> - const struct imx_icc_node_desc *node_desc)
> + const struct imx_icc_node_desc *node_desc,
> + const struct imx_icc_noc_setting *setting)
> {
> struct icc_provider *provider = &imx_provider->provider;
> struct device *dev = provider->dev;
> @@ -164,6 +184,8 @@ static struct icc_node *imx_icc_node_add(struct imx_icc_provider *imx_provider,
> node->name = node_desc->name;
> node->data = node_data;
> node_data->desc = node_desc;
> + node_data->setting = setting;
> + node_data->imx_provider = imx_provider;
> icc_node_add(node, provider);
>
> if (node_desc->adj) {
> @@ -187,7 +209,8 @@ static void imx_icc_unregister_nodes(struct icc_provider *provider)
>
> static int imx_icc_register_nodes(struct imx_icc_provider *imx_provider,
> const struct imx_icc_node_desc *descs,
> - int count)
> + int count,
> + const struct imx_icc_noc_setting *settings)
> {
> struct icc_provider *provider = &imx_provider->provider;
> struct icc_onecell_data *provider_data = provider->data;
> @@ -199,7 +222,10 @@ static int imx_icc_register_nodes(struct imx_icc_provider *imx_provider,
> const struct imx_icc_node_desc *node_desc = &descs[i];
> size_t j;
>
> - node = imx_icc_node_add(imx_provider, node_desc);
> + if (settings)
> + node = imx_icc_node_add(imx_provider, node_desc, &settings[node_desc->id]);
> + else
> + node = imx_icc_node_add(imx_provider, node_desc, NULL);

Maybe just write as
node = imx_icc_node_add(imx_provider, node_desc, settings ? &settings[node_desc->id] : NULL);

But I don't really care, so feel free to ignore this suggestion if you
don't like it.

> if (IS_ERR(node)) {
> ret = dev_err_probe(provider->dev,
> PTR_ERR(node),
> "failed to add %s\n",
> node_desc->name);
> @@ -237,7 +263,8 @@ static int get_max_node_id(struct
> imx_icc_node_desc *nodes, int nodes_count)
> }
>
> int imx_icc_register(struct platform_device *pdev,
> - struct imx_icc_node_desc *nodes, int
> nodes_count)
> + struct imx_icc_node_desc *nodes, int
> nodes_count,
> + struct imx_icc_noc_setting *settings)
> {
> struct device *dev = &pdev->dev;
> struct icc_onecell_data *data;
> @@ -267,13 +294,19 @@ int imx_icc_register(struct platform_device
> *pdev,
> provider->dev->of_node = dev->parent->of_node;
> platform_set_drvdata(pdev, imx_provider);
>
> + if (settings) {
> + imx_provider->noc_base = devm_of_iomap(dev,
> provider->dev->of_node, 0, NULL);
> + if (!imx_provider->noc_base)
> + return PTR_ERR(imx_provider->noc_base);
> + }
> +
> ret = icc_provider_add(provider);
> if (ret) {
> dev_err(dev, "error adding interconnect provider:
> %d\n", ret);
> return ret;
> }
>
> - ret = imx_icc_register_nodes(imx_provider, nodes,
> nodes_count);
> + ret = imx_icc_register_nodes(imx_provider, nodes,
> nodes_count, settings);
> if (ret)
> goto provider_del;
>
> diff --git a/drivers/interconnect/imx/imx.h
> b/drivers/interconnect/imx/imx.h
> index 0ad2c654c222..1da87cfe27da 100644
> --- a/drivers/interconnect/imx/imx.h
> +++ b/drivers/interconnect/imx/imx.h
> @@ -15,6 +15,31 @@
>
> #define IMX_ICC_MAX_LINKS 4
>
> +/*
> + * High throughput priority level in Regulator mode
> + * Read Priority in Fixed/Limiter mode
> + */
> +#define PRIORITY0_SHIFT 0
> +/*
> + * Low throughput priority level in Regulator mode
> + * Write Priority in Fixed/Limiter mode
> + */
> +#define PRIORITY1_SHIFT 8
> +#define PRIORITY_MASK 0x7
> +
> +#define PRIORITY_COMP_MARK BIT(31) /* Must set */
> +
> +#define IMX_NOC_MODE_FIXED 0
> +#define IMX_NOC_MODE_LIMITER 1
> +#define IMX_NOC_MODE_BYPASS 2
> +#define IMX_NOC_MODE_REGULATOR 3
> +
> +#define IMX_NOC_PRIO_REG 0x8
> +#define IMX_NOC_MODE_REG 0xC
> +#define IMX_NOC_BANDWIDTH_REG 0x10
> +#define IMX_NOC_SATURATION 0x14
> +#define IMX_NOC_EXT_CTL_REG 0x18
> +
> struct imx_icc_provider {
> void __iomem *noc_base;
> struct icc_provider provider;
> @@ -44,6 +69,22 @@ struct imx_icc_node_desc {
> const struct imx_icc_node_adj_desc *adj;
> };
>
> +/*
> + * struct imx_icc_noc_setting - Describe an interconnect node
> setting
> + * @ignore: indicate whether need apply this setting
> + * @reg: register offset inside the NoC
> + * @prio_level: priority level
> + * @mode: functional mode
> + * @ext_control: external input control
> + */
> +struct imx_icc_noc_setting {
> + bool ignore;

I don't like this ignore member. Can we get rid of this and make the
"don't touch" a special mode, like IMX_NOC_MODE_UNCONFIGURED?

Other than those two nitpicks, the patch looks good.

Regards,
Lucas

> + u32 reg;
> + u32 prio_level;
> + u32 mode;
> + u32 ext_control;
> +};
> +
> #define DEFINE_BUS_INTERCONNECT(_name, _id, _adj, ...) \
> { \
> .id = _id, \
> @@ -61,7 +102,8 @@ struct imx_icc_node_desc {
>
> int imx_icc_register(struct platform_device *pdev,
> struct imx_icc_node_desc *nodes,
> - int nodes_count);
> + int nodes_count,
> + struct imx_icc_noc_setting *noc_settings);
> int imx_icc_unregister(struct platform_device *pdev);
>
> #endif /* __DRIVERS_INTERCONNECT_IMX_H */
> diff --git a/drivers/interconnect/imx/imx8mm.c b/drivers/interconnect/imx/imx8mm.c
> index 1083490bb391..ae797412db96 100644
> --- a/drivers/interconnect/imx/imx8mm.c
> +++ b/drivers/interconnect/imx/imx8mm.c
> @@ -83,7 +83,7 @@ static struct imx_icc_node_desc nodes[] = {
>
> static int imx8mm_icc_probe(struct platform_device *pdev)
> {
> - return imx_icc_register(pdev, nodes, ARRAY_SIZE(nodes));
> + return imx_icc_register(pdev, nodes, ARRAY_SIZE(nodes), NULL);
> }
>
> static int imx8mm_icc_remove(struct platform_device *pdev)
> diff --git a/drivers/interconnect/imx/imx8mn.c b/drivers/interconnect/imx/imx8mn.c
> index ad97e55fd4e5..1ce94c5bdd8c 100644
> --- a/drivers/interconnect/imx/imx8mn.c
> +++ b/drivers/interconnect/imx/imx8mn.c
> @@ -72,7 +72,7 @@ static struct imx_icc_node_desc nodes[] = {
>
> static int imx8mn_icc_probe(struct platform_device *pdev)
> {
> - return imx_icc_register(pdev, nodes, ARRAY_SIZE(nodes));
> + return imx_icc_register(pdev, nodes, ARRAY_SIZE(nodes), NULL);
> }
>
> static int imx8mn_icc_remove(struct platform_device *pdev)
> diff --git a/drivers/interconnect/imx/imx8mq.c b/drivers/interconnect/imx/imx8mq.c
> index d7768d3c6d8a..7f00a0511c6e 100644
> --- a/drivers/interconnect/imx/imx8mq.c
> +++ b/drivers/interconnect/imx/imx8mq.c
> @@ -82,7 +82,7 @@ static struct imx_icc_node_desc nodes[] = {
>
> static int imx8mq_icc_probe(struct platform_device *pdev)
> {
> - return imx_icc_register(pdev, nodes, ARRAY_SIZE(nodes));
> + return imx_icc_register(pdev, nodes, ARRAY_SIZE(nodes), NULL);
> }
>
> static int imx8mq_icc_remove(struct platform_device *pdev)

2022-07-02 13:13:21

by Peng Fan

Subject: RE: [PATCH V2 8/9] interconnect: imx: configure NoC mode/prioriry/ext_control

> Subject: Re: [PATCH V2 8/9] interconnect: imx: configure NoC
> mode/prioriry/ext_control
>
> Am Donnerstag, dem 16.06.2022 um 15:33 +0800 schrieb Peng Fan (OSS):
> > From: Peng Fan <[email protected]>
> >
> > Introduce imx_icc_noc_setting structure to describe a master port
> > setting Pass imx_icc_noc_setting as a parameter from specific driver
> > Set priority level, mode, ext control in imx_icc_node_set
> >
> > Signed-off-by: Peng Fan <[email protected]>
> > ---
> > drivers/interconnect/imx/imx.c | 43 ++++++++++++++++++++++++++----
> > drivers/interconnect/imx/imx.h | 44
> ++++++++++++++++++++++++++++++-
> > drivers/interconnect/imx/imx8mm.c | 2 +-
> > drivers/interconnect/imx/imx8mn.c | 2 +-
> > drivers/interconnect/imx/imx8mq.c | 2 +-
> > 5 files changed, 84 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/interconnect/imx/imx.c
> > b/drivers/interconnect/imx/imx.c index 78557fe6da2c..bd728caf2b85
> > 100644
> > --- a/drivers/interconnect/imx/imx.c
> > +++ b/drivers/interconnect/imx/imx.c
> > @@ -10,6 +10,7 @@
> >
> > #include <linux/device.h>
> > #include <linux/interconnect-provider.h>
> > +#include <linux/io.h>
> > #include <linux/module.h>
> > #include <linux/of.h>
> > #include <linux/of_platform.h>
> > @@ -21,8 +22,10 @@
> > /* private icc_node data */
> > struct imx_icc_node {
> > const struct imx_icc_node_desc *desc;
> > + const struct imx_icc_noc_setting *setting;
> > struct device *qos_dev;
> > struct dev_pm_qos_request qos_req;
> > + struct imx_icc_provider *imx_provider;
> > };
> >
> > static int imx_icc_get_bw(struct icc_node *node, u32 *avg, u32 *peak)
> > @@ -37,8 +40,24 @@ static int imx_icc_node_set(struct icc_node *node)
> > {
> > struct device *dev = node->provider->dev;
> > struct imx_icc_node *node_data = node->data;
> > + void __iomem *base;
> > + u32 prio;
> > u64 freq;
> >
> > + if (node_data->setting && !node_data->setting->ignore && node-
> >peak_bw) {
> > + base = node_data->setting->reg + node_data-
> >imx_provider->noc_base;
> > + if (node_data->setting->mode == IMX_NOC_MODE_FIXED) {
> > + prio = node_data->setting->prio_level;
> > + prio = PRIORITY_COMP_MARK | (prio << 8) | prio;
> > + writel(prio, base + IMX_NOC_PRIO_REG);
> > + writel(node_data->setting->mode, base +
> IMX_NOC_MODE_REG);
> > + writel(node_data->setting->ext_control, base +
> IMX_NOC_EXT_CTL_REG);
> > + } else {
> > + dev_info(dev, "mode: %d not supported\n",
> node_data->setting->mode);
> > + return -ENOTSUPP;
> > + }
> > + }
> > +
> > if (!node_data->qos_dev)
> > return 0;
> >
> > @@ -135,7 +154,8 @@ static int imx_icc_node_init_qos(struct
> > icc_provider *provider, }
> >
> > static struct icc_node *imx_icc_node_add(struct imx_icc_provider
> *imx_provider,
> > - const struct imx_icc_node_desc
> *node_desc)
> > + const struct imx_icc_node_desc
> *node_desc,
> > + const struct imx_icc_noc_setting
> *setting)
> > {
> > struct icc_provider *provider = &imx_provider->provider;
> > struct device *dev = provider->dev;
> > @@ -164,6 +184,8 @@ static struct icc_node *imx_icc_node_add(struct
> imx_icc_provider *imx_provider,
> > node->name = node_desc->name;
> > node->data = node_data;
> > node_data->desc = node_desc;
> > + node_data->setting = setting;
> > + node_data->imx_provider = imx_provider;
> > icc_node_add(node, provider);
> >
> > if (node_desc->adj) {
> > @@ -187,7 +209,8 @@ static void imx_icc_unregister_nodes(struct
> > icc_provider *provider)
> >
> > static int imx_icc_register_nodes(struct imx_icc_provider *imx_provider,
> > const struct imx_icc_node_desc *descs,
> > - int count)
> > + int count,
> > + const struct imx_icc_noc_setting *settings)
> > {
> > struct icc_provider *provider = &imx_provider->provider;
> > struct icc_onecell_data *provider_data = provider->data; @@ -
> 199,7
> > +222,10 @@ static int imx_icc_register_nodes(struct imx_icc_provider
> *imx_provider,
> > const struct imx_icc_node_desc *node_desc = &descs[i];
> > size_t j;
> >
> > - node = imx_icc_node_add(imx_provider, node_desc);
> > + if (settings)
> > + node = imx_icc_node_add(imx_provider, node_desc,
> &settings[node_desc->id]);
> > + else
> > + node = imx_icc_node_add(imx_provider, node_desc,
> NULL);
>
> Maybe just write as
> node = imx_icc_node_add(imx_provider, node_desc, settings ?
> &settings[node_desc->id] : NULL);

Sure, cleaner.

>
> But I don't really care, so feel free to ignore this suggestion if you don't like it.
>
> > if (IS_ERR(node)) {
> > ret = dev_err_probe(provider->dev, PTR_ERR(node),
> > "failed to add %s\n",
> > node_desc->name);
> > @@ -237,7 +263,8 @@ static int get_max_node_id(struct
> > imx_icc_node_desc *nodes, int nodes_count) }
> >
> > int imx_icc_register(struct platform_device *pdev,
> > - struct imx_icc_node_desc *nodes, int
> > nodes_count)
> > + struct imx_icc_node_desc *nodes, int
> > nodes_count,
> > + struct imx_icc_noc_setting *settings)
> > {
> > struct device *dev = &pdev->dev;
> > struct icc_onecell_data *data;
> > @@ -267,13 +294,19 @@ int imx_icc_register(struct platform_device
> > *pdev,
> > provider->dev->of_node = dev->parent->of_node;
> > platform_set_drvdata(pdev, imx_provider);
> >
> > + if (settings) {
> > + imx_provider->noc_base = devm_of_iomap(dev,
> > provider->dev->of_node, 0, NULL);
> > + if (!imx_provider->noc_base)
> > + return PTR_ERR(imx_provider->noc_base);
> > + }
> > +
> > ret = icc_provider_add(provider);
> > if (ret) {
> > dev_err(dev, "error adding interconnect provider:
> > %d\n", ret);
> > return ret;
> > }
> >
> > - ret = imx_icc_register_nodes(imx_provider, nodes,
> > nodes_count);
> > + ret = imx_icc_register_nodes(imx_provider, nodes,
> > nodes_count, settings);
> > if (ret)
> > goto provider_del;
> >
> > diff --git a/drivers/interconnect/imx/imx.h
> > b/drivers/interconnect/imx/imx.h index 0ad2c654c222..1da87cfe27da
> > 100644
> > --- a/drivers/interconnect/imx/imx.h
> > +++ b/drivers/interconnect/imx/imx.h
> > @@ -15,6 +15,31 @@
> >
> > #define IMX_ICC_MAX_LINKS 4
> >
> > +/*
> > + * High throughput priority level in Regulator mode
> > + * Read Priority in Fixed/Limiter mode */
> > +#define PRIORITY0_SHIFT 0
> > +/*
> > + * Low throughput priority level in Regulator mode
> > + * Write Priority in Fixed/Limiter mode */
> > +#define PRIORITY1_SHIFT 8
> > +#define PRIORITY_MASK 0x7
> > +
> > +#define PRIORITY_COMP_MARK BIT(31) /* Must set */
> > +
> > +#define IMX_NOC_MODE_FIXED 0
> > +#define IMX_NOC_MODE_LIMITER 1
> > +#define IMX_NOC_MODE_BYPASS 2
> > +#define IMX_NOC_MODE_REGULATOR 3
> > +
> > +#define IMX_NOC_PRIO_REG 0x8
> > +#define IMX_NOC_MODE_REG 0xC
> > +#define IMX_NOC_BANDWIDTH_REG 0x10
> > +#define IMX_NOC_SATURATION 0x14
> > +#define IMX_NOC_EXT_CTL_REG 0x18
> > +
> > struct imx_icc_provider {
> > void __iomem *noc_base;
> > struct icc_provider provider;
> > @@ -44,6 +69,22 @@ struct imx_icc_node_desc {
> > const struct imx_icc_node_adj_desc *adj; };
> >
> > +/*
> > + * struct imx_icc_noc_setting - Describe an interconnect node
> > setting
> > + * @ignore: indicate whether need apply this setting
> > + * @reg: register offset inside the NoC
> > + * @prio_level: priority level
> > + * @mode: functional mode
> > + * @ext_control: external input control */ struct
> > +imx_icc_noc_setting {
> > + bool ignore;
>
> I don't like this ignore member. Can we get rid of this and make the "don't
> touch" a special mode, like IMX_NOC_MODE_UNCONFIGURED?

I'll address in V3.

Thanks,
Peng.

>
> Other than those two nitpicks, the patch looks good.
>
> Regards,
> Lucas
>
> > + u32 reg;
> > + u32 prio_level;
> > + u32 mode;
> > + u32 ext_control;
> > +};
> > +
> > #define DEFINE_BUS_INTERCONNECT(_name, _id, _adj, ...)
> \
> > { \
> > .id = _id, \
> > @@ -61,7 +102,8 @@ struct imx_icc_node_desc {
> >
> > int imx_icc_register(struct platform_device *pdev,
> > struct imx_icc_node_desc *nodes,
> > - int nodes_count);
> > + int nodes_count,
> > + struct imx_icc_noc_setting *noc_settings);
> > int imx_icc_unregister(struct platform_device *pdev);
> >
> > #endif /* __DRIVERS_INTERCONNECT_IMX_H */ diff --git
> > a/drivers/interconnect/imx/imx8mm.c
> > b/drivers/interconnect/imx/imx8mm.c
> > index 1083490bb391..ae797412db96 100644
> > --- a/drivers/interconnect/imx/imx8mm.c
> > +++ b/drivers/interconnect/imx/imx8mm.c
> > @@ -83,7 +83,7 @@ static struct imx_icc_node_desc nodes[] = {
> >
> > static int imx8mm_icc_probe(struct platform_device *pdev) {
> > - return imx_icc_register(pdev, nodes, ARRAY_SIZE(nodes));
> > + return imx_icc_register(pdev, nodes, ARRAY_SIZE(nodes), NULL);
> > }
> >
> > static int imx8mm_icc_remove(struct platform_device *pdev) diff --git
> > a/drivers/interconnect/imx/imx8mn.c
> > b/drivers/interconnect/imx/imx8mn.c
> > index ad97e55fd4e5..1ce94c5bdd8c 100644
> > --- a/drivers/interconnect/imx/imx8mn.c
> > +++ b/drivers/interconnect/imx/imx8mn.c
> > @@ -72,7 +72,7 @@ static struct imx_icc_node_desc nodes[] = {
> >
> > static int imx8mn_icc_probe(struct platform_device *pdev) {
> > - return imx_icc_register(pdev, nodes, ARRAY_SIZE(nodes));
> > + return imx_icc_register(pdev, nodes, ARRAY_SIZE(nodes), NULL);
> > }
> >
> > static int imx8mn_icc_remove(struct platform_device *pdev) diff --git
> > a/drivers/interconnect/imx/imx8mq.c
> > b/drivers/interconnect/imx/imx8mq.c
> > index d7768d3c6d8a..7f00a0511c6e 100644
> > --- a/drivers/interconnect/imx/imx8mq.c
> > +++ b/drivers/interconnect/imx/imx8mq.c
> > @@ -82,7 +82,7 @@ static struct imx_icc_node_desc nodes[] = {
> >
> > static int imx8mq_icc_probe(struct platform_device *pdev) {
> > - return imx_icc_register(pdev, nodes, ARRAY_SIZE(nodes));
> > + return imx_icc_register(pdev, nodes, ARRAY_SIZE(nodes), NULL);
> > }
> >
> > static int imx8mq_icc_remove(struct platform_device *pdev)
>

2022-07-02 13:13:49

by Peng Fan

Subject: RE: [PATCH V2 8/9] interconnect: imx: configure NoC mode/prioriry/ext_control

> Subject: Re: [PATCH V2 8/9] interconnect: imx: configure NoC
> mode/prioriry/ext_control
>
> Hi Peng,
>
> On 16.06.22 10:33, Peng Fan (OSS) wrote:
> > From: Peng Fan <[email protected]>
> >
> > Introduce imx_icc_noc_setting structure to describe a master port
> > setting Pass imx_icc_noc_setting as a parameter from specific driver
> > Set priority level, mode, ext control in imx_icc_node_set
> >
> > Signed-off-by: Peng Fan <[email protected]>
> > ---
> > drivers/interconnect/imx/imx.c | 43 ++++++++++++++++++++++++++----
> > drivers/interconnect/imx/imx.h | 44
> ++++++++++++++++++++++++++++++-
> > drivers/interconnect/imx/imx8mm.c | 2 +-
> > drivers/interconnect/imx/imx8mn.c | 2 +-
> > drivers/interconnect/imx/imx8mq.c | 2 +-
> > 5 files changed, 84 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/interconnect/imx/imx.c
> > b/drivers/interconnect/imx/imx.c index 78557fe6da2c..bd728caf2b85
> > 100644
>
> [..]
>
> > @@ -37,8 +40,24 @@ static int imx_icc_node_set(struct icc_node *node)
> > {
> > struct device *dev = node->provider->dev;
> > struct imx_icc_node *node_data = node->data;
> > + void __iomem *base;
> > + u32 prio;
> > u64 freq;
> >
> > + if (node_data->setting && !node_data->setting->ignore && node-
> >peak_bw) {
> > + base = node_data->setting->reg + node_data-
> >imx_provider->noc_base;
> > + if (node_data->setting->mode == IMX_NOC_MODE_FIXED) {
> > + prio = node_data->setting->prio_level;
> > + prio = PRIORITY_COMP_MARK | (prio << 8) | prio;
> > + writel(prio, base + IMX_NOC_PRIO_REG);
> > + writel(node_data->setting->mode, base +
> IMX_NOC_MODE_REG);
> > + writel(node_data->setting->ext_control, base +
> IMX_NOC_EXT_CTL_REG);
> > + } else {
> > + dev_info(dev, "mode: %d not supported\n",
> node_data->setting->mode);
> > + return -ENOTSUPP;
>
> Nit: I believe that -EOPNOTSUPP is the preferred error code.

Fix in V3.

>
> > + }
> > + }
> > +
> > if (!node_data->qos_dev)
> > return 0;
> >
>
> [..]
>
> > @@ -237,7 +263,8 @@ static int get_max_node_id(struct
> imx_icc_node_desc *nodes, int nodes_count)
> > }
> >
> > int imx_icc_register(struct platform_device *pdev,
> > - struct imx_icc_node_desc *nodes, int nodes_count)
> > + struct imx_icc_node_desc *nodes, int nodes_count,
> > + struct imx_icc_noc_setting *settings)
> > {
> > struct device *dev = &pdev->dev;
> > struct icc_onecell_data *data;
> > @@ -267,13 +294,19 @@ int imx_icc_register(struct platform_device
> *pdev,
> > provider->dev->of_node = dev->parent->of_node;
> > platform_set_drvdata(pdev, imx_provider);
> >
> > + if (settings) {
> > + imx_provider->noc_base = devm_of_iomap(dev, provider-
> >dev->of_node, 0, NULL);
> > + if (!imx_provider->noc_base)
>
> devm_of_iomap() returns ERR_PTR(). So we should check it with IS_ERR().

Oops, fix in V3.

Thanks,
Peng.

>
> Thanks,
> Georgi
>
> > + return PTR_ERR(imx_provider->noc_base);
> > + }
> > +
> > ret = icc_provider_add(provider);
> > if (ret) {
> > dev_err(dev, "error adding interconnect provider: %d\n",
> ret);
> > return ret;
> > }
> >
> > - ret = imx_icc_register_nodes(imx_provider, nodes, nodes_count);
> > + ret = imx_icc_register_nodes(imx_provider, nodes, nodes_count,
> > +settings);
> > if (ret)
> > goto provider_del;
> >