diff mbox series

[v6,02/14] of: property: Add fw_devlink support for msi-parent

Message ID 20230719113542.2293295-3-apatel@ventanamicro.com
State New
Headers
Series Linux RISC-V AIA Support |

Commit Message

Anup Patel July 19, 2023, 11:35 a.m. UTC 
  This allows fw_devlink to create device links between consumers of
a MSI and the supplier of the MSI.

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 drivers/of/property.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

Comments

Saravana Kannan July 19, 2023, 10:25 p.m. UTC | #1 
On Wed, Jul 19, 2023 at 4:36 AM Anup Patel <apatel@ventanamicro.com> wrote:
>
> This allows fw_devlink to create device links between consumers of
> a MSI and the supplier of the MSI.
>
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
>  drivers/of/property.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index ddc75cd50825..e4096b79a872 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1325,6 +1325,37 @@ static struct device_node *parse_interrupts(struct device_node *np,
>         return of_irq_parse_one(np, index, &sup_args) ? NULL : sup_args.np;
>  }
>
> +static struct device_node *parse_msi_parent(struct device_node *np,
> +                                           const char *prop_name, int index)
> +{
> +       struct of_phandle_args sup_args;
> +       struct device_node *msi_np;
> +
> +       if (!IS_ENABLED(CONFIG_OF_IRQ))
> +               return NULL;
> +
> +       if (strcmp(prop_name, "msi-parent"))
> +               return NULL;
> +
> +       msi_np = of_parse_phandle(np, prop_name, 0);
> +       if (msi_np) {
> +               if (!of_property_read_bool(msi_np, "#msi-cells")) {
> +                       if (index) {
> +                               of_node_put(msi_np);
> +                               return NULL;
> +                       }
> +                       return msi_np;
> +               }
> +               of_node_put(msi_np);
> +       }
> +
> +       if (of_parse_phandle_with_args(np, prop_name, "#msi-cells", index,
> +                                      &sup_args))
> +               return NULL;
> +
> +       return sup_args.np;
> +}
> +

I'm amazed by the different ways you choose to waste people's time.
Did you even scroll up to see how the other properties are handled?

Why can't this be handled using DEFINE_SIMPLE_PROP macro?

-Saravana

>  static const struct supplier_bindings of_supplier_bindings[] = {
>         { .parse_prop = parse_clocks, },
>         { .parse_prop = parse_interconnects, },
> @@ -1359,6 +1390,7 @@ static const struct supplier_bindings of_supplier_bindings[] = {
>         { .parse_prop = parse_regulators, },
>         { .parse_prop = parse_gpio, },
>         { .parse_prop = parse_gpios, },
> +       { .parse_prop = parse_msi_parent, },
>         {}
>  };
>
> --
> 2.34.1
>
Rob Herring July 19, 2023, 10:37 p.m. UTC | #2 
On Wed, Jul 19, 2023 at 05:05:30PM +0530, Anup Patel wrote:
> This allows fw_devlink to create device links between consumers of
> a MSI and the supplier of the MSI.
> 
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
>  drivers/of/property.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index ddc75cd50825..e4096b79a872 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1325,6 +1325,37 @@ static struct device_node *parse_interrupts(struct device_node *np,
>  	return of_irq_parse_one(np, index, &sup_args) ? NULL : sup_args.np;
>  }
>  
> +static struct device_node *parse_msi_parent(struct device_node *np,
> +					    const char *prop_name, int index)
> +{
> +	struct of_phandle_args sup_args;
> +	struct device_node *msi_np;
> +
> +	if (!IS_ENABLED(CONFIG_OF_IRQ))
> +		return NULL;

Why do we need this? Sparc is not going to have MSI properties to begin 
with. I guess it saves a little bit of code. Though Sparc doesn't need 
any of this file. Or maybe there's a better kconfig symbol to use here 
if MSIs are not supported?

> +
> +	if (strcmp(prop_name, "msi-parent"))
> +		return NULL;
> +
> +	msi_np = of_parse_phandle(np, prop_name, 0);
> +	if (msi_np) {
> +		if (!of_property_read_bool(msi_np, "#msi-cells")) {
> +			if (index) {
> +				of_node_put(msi_np);
> +				return NULL;
> +			}
> +			return msi_np;
> +		}
> +		of_node_put(msi_np);
> +	}
> +
> +	if (of_parse_phandle_with_args(np, prop_name, "#msi-cells", index,
> +				       &sup_args))
> +		return NULL;
> +
> +	return sup_args.np;
> +}
> +
>  static const struct supplier_bindings of_supplier_bindings[] = {
>  	{ .parse_prop = parse_clocks, },
>  	{ .parse_prop = parse_interconnects, },
> @@ -1359,6 +1390,7 @@ static const struct supplier_bindings of_supplier_bindings[] = {
>  	{ .parse_prop = parse_regulators, },
>  	{ .parse_prop = parse_gpio, },
>  	{ .parse_prop = parse_gpios, },
> +	{ .parse_prop = parse_msi_parent, },
>  	{}
>  };
>  
> -- 
> 2.34.1
>
Anup Patel July 20, 2023, 5:21 a.m. UTC | #3 
On Thu, Jul 20, 2023 at 3:55 AM Saravana Kannan <saravanak@google.com> wrote:
>
> On Wed, Jul 19, 2023 at 4:36 AM Anup Patel <apatel@ventanamicro.com> wrote:
> >
> > This allows fw_devlink to create device links between consumers of
> > a MSI and the supplier of the MSI.
> >
> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > ---
> >  drivers/of/property.c | 32 ++++++++++++++++++++++++++++++++
> >  1 file changed, 32 insertions(+)
> >
> > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > index ddc75cd50825..e4096b79a872 100644
> > --- a/drivers/of/property.c
> > +++ b/drivers/of/property.c
> > @@ -1325,6 +1325,37 @@ static struct device_node *parse_interrupts(struct device_node *np,
> >         return of_irq_parse_one(np, index, &sup_args) ? NULL : sup_args.np;
> >  }
> >
> > +static struct device_node *parse_msi_parent(struct device_node *np,
> > +                                           const char *prop_name, int index)
> > +{
> > +       struct of_phandle_args sup_args;
> > +       struct device_node *msi_np;
> > +
> > +       if (!IS_ENABLED(CONFIG_OF_IRQ))
> > +               return NULL;
> > +
> > +       if (strcmp(prop_name, "msi-parent"))
> > +               return NULL;
> > +
> > +       msi_np = of_parse_phandle(np, prop_name, 0);
> > +       if (msi_np) {
> > +               if (!of_property_read_bool(msi_np, "#msi-cells")) {
> > +                       if (index) {
> > +                               of_node_put(msi_np);
> > +                               return NULL;
> > +                       }
> > +                       return msi_np;
> > +               }
> > +               of_node_put(msi_np);
> > +       }
> > +
> > +       if (of_parse_phandle_with_args(np, prop_name, "#msi-cells", index,
> > +                                      &sup_args))
> > +               return NULL;
> > +
> > +       return sup_args.np;
> > +}
> > +
>
> I'm amazed by the different ways you choose to waste people's time.
> Did you even scroll up to see how the other properties are handled?
>
> Why can't this be handled using DEFINE_SIMPLE_PROP macro?

DEFINE_SIMPLE_PROP() is not suitable for msi-parent because we
have a special case where for a single MSI parent the "#msi-cells"
property won't be present in the supplier DT node.

The of_msi_get_domain() function also handles this special case
separately.

Regards,
Anup


>
> -Saravana
>
> >  static const struct supplier_bindings of_supplier_bindings[] = {
> >         { .parse_prop = parse_clocks, },
> >         { .parse_prop = parse_interconnects, },
> > @@ -1359,6 +1390,7 @@ static const struct supplier_bindings of_supplier_bindings[] = {
> >         { .parse_prop = parse_regulators, },
> >         { .parse_prop = parse_gpio, },
> >         { .parse_prop = parse_gpios, },
> > +       { .parse_prop = parse_msi_parent, },
> >         {}
> >  };
> >
> > --
> > 2.34.1
> >
Anup Patel July 20, 2023, 11:55 a.m. UTC | #4 
On Thu, Jul 20, 2023 at 4:08 AM Rob Herring <robh@kernel.org> wrote:
>
> On Wed, Jul 19, 2023 at 05:05:30PM +0530, Anup Patel wrote:
> > This allows fw_devlink to create device links between consumers of
> > a MSI and the supplier of the MSI.
> >
> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > ---
> >  drivers/of/property.c | 32 ++++++++++++++++++++++++++++++++
> >  1 file changed, 32 insertions(+)
> >
> > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > index ddc75cd50825..e4096b79a872 100644
> > --- a/drivers/of/property.c
> > +++ b/drivers/of/property.c
> > @@ -1325,6 +1325,37 @@ static struct device_node *parse_interrupts(struct device_node *np,
> >       return of_irq_parse_one(np, index, &sup_args) ? NULL : sup_args.np;
> >  }
> >
> > +static struct device_node *parse_msi_parent(struct device_node *np,
> > +                                         const char *prop_name, int index)
> > +{
> > +     struct of_phandle_args sup_args;
> > +     struct device_node *msi_np;
> > +
> > +     if (!IS_ENABLED(CONFIG_OF_IRQ))
> > +             return NULL;
>
> Why do we need this? Sparc is not going to have MSI properties to begin
> with. I guess it saves a little bit of code. Though Sparc doesn't need
> any of this file. Or maybe there's a better kconfig symbol to use here
> if MSIs are not supported?

I can't think of a better kconfig symbol over here but since Sparc does
not have MSI properties, I think following is better:

if (IS_ENABLED(CONFIG_SPARC))
    return NULL;

Any other suggestions ?

Regards,
Anup

>
> > +
> > +     if (strcmp(prop_name, "msi-parent"))
> > +             return NULL;
> > +
> > +     msi_np = of_parse_phandle(np, prop_name, 0);
> > +     if (msi_np) {
> > +             if (!of_property_read_bool(msi_np, "#msi-cells")) {
> > +                     if (index) {
> > +                             of_node_put(msi_np);
> > +                             return NULL;
> > +                     }
> > +                     return msi_np;
> > +             }
> > +             of_node_put(msi_np);
> > +     }
> > +
> > +     if (of_parse_phandle_with_args(np, prop_name, "#msi-cells", index,
> > +                                    &sup_args))
> > +             return NULL;
> > +
> > +     return sup_args.np;
> > +}
> > +
> >  static const struct supplier_bindings of_supplier_bindings[] = {
> >       { .parse_prop = parse_clocks, },
> >       { .parse_prop = parse_interconnects, },
> > @@ -1359,6 +1390,7 @@ static const struct supplier_bindings of_supplier_bindings[] = {
> >       { .parse_prop = parse_regulators, },
> >       { .parse_prop = parse_gpio, },
> >       { .parse_prop = parse_gpios, },
> > +     { .parse_prop = parse_msi_parent, },
> >       {}
> >  };
> >
> > --
> > 2.34.1
> >
diff mbox series

Patch

diff --git a/drivers/of/property.c b/drivers/of/property.c
index ddc75cd50825..e4096b79a872 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1325,6 +1325,37 @@  static struct device_node *parse_interrupts(struct device_node *np,
 	return of_irq_parse_one(np, index, &sup_args) ? NULL : sup_args.np;
 }
 
+static struct device_node *parse_msi_parent(struct device_node *np,
+					    const char *prop_name, int index)
+{
+	struct of_phandle_args sup_args;
+	struct device_node *msi_np;
+
+	if (!IS_ENABLED(CONFIG_OF_IRQ))
+		return NULL;
+
+	if (strcmp(prop_name, "msi-parent"))
+		return NULL;
+
+	msi_np = of_parse_phandle(np, prop_name, 0);
+	if (msi_np) {
+		if (!of_property_read_bool(msi_np, "#msi-cells")) {
+			if (index) {
+				of_node_put(msi_np);
+				return NULL;
+			}
+			return msi_np;
+		}
+		of_node_put(msi_np);
+	}
+
+	if (of_parse_phandle_with_args(np, prop_name, "#msi-cells", index,
+				       &sup_args))
+		return NULL;
+
+	return sup_args.np;
+}
+
 static const struct supplier_bindings of_supplier_bindings[] = {
 	{ .parse_prop = parse_clocks, },
 	{ .parse_prop = parse_interconnects, },
@@ -1359,6 +1390,7 @@  static const struct supplier_bindings of_supplier_bindings[] = {
 	{ .parse_prop = parse_regulators, },
 	{ .parse_prop = parse_gpio, },
 	{ .parse_prop = parse_gpios, },
+	{ .parse_prop = parse_msi_parent, },
 	{}
 };