[PATCH] fpga: mgr: Use devicetree /alias to assign FPGA IDs

[PATCH] fpga: mgr: Use devicetree /alias to assign FPGA IDs

[Brandon Maier]

The fpga-mgr assigns device IDs based on the order each driver calls
fpga_mgr_register(). This is fine for systems with one FPGA, but if
multiple FPGAs are in use, the device ID can change as probe order
changes across reboots or configuration changes.

This makes it difficult to use the sysfs attributes, as the device path
may change. Instead allow a static ID to be set by adding fpgaX props
to the devicetree /aliases. Falling back to incrementing IDs if none are
provided.

This is based on the function rtc_device_get_id() in drivers/rtc/class.c
(9d2b7e532da8 rtc: honor device tree /alias entries when assigning IDs).

Signed-off-by: Brandon Maier <brandon.maier@rockwellcollins.com>
---
 drivers/fpga/fpga-mgr.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index c3866816456a..d8b7e7a61117 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -551,6 +551,26 @@ void fpga_mgr_unlock(struct fpga_manager *mgr)
 }
 EXPORT_SYMBOL_GPL(fpga_mgr_unlock);
 
+static int fpga_mgr_get_id(struct device *dev)
+{
+	int of_id = -1, id = -1;
+
+	if (dev->of_node)
+		of_id = of_alias_get_id(dev->of_node, "fpga");
+
+	if (of_id >= 0) {
+		id = ida_simple_get(&fpga_mgr_ida, of_id, of_id + 1,
+				    GFP_KERNEL);
+		if (id < 0)
+			dev_warn(dev, "/aliases ID %d not available\n", of_id);
+	}
+
+	if (id < 0)
+		id = ida_simple_get(&fpga_mgr_ida, 0, 0, GFP_KERNEL);
+
+	return id;
+}
+
 /**
  * fpga_mgr_create - create and initialize a FPGA manager struct
  * @dev:	fpga manager device from pdev
@@ -586,7 +606,7 @@ struct fpga_manager *fpga_mgr_create(struct device *dev, const char *name,
 	if (!mgr)
 		return NULL;
 
-	id = ida_simple_get(&fpga_mgr_ida, 0, 0, GFP_KERNEL);
+	id = fpga_mgr_get_id(dev);
 	if (id < 0) {
 		ret = id;
 		goto error_kfree;
-- 
2.20.1

Re: [PATCH] fpga: mgr: Use devicetree /alias to assign FPGA IDs

[Alan Tull]

On Thu, Apr 11, 2019 at 5:12 PM Brandon Maier
<brandon.maier@rockwellcollins.com> wrote:

Hi Brandon,

Thanks!  This is useful.

>
> The fpga-mgr assigns device IDs based on the order each driver calls
> fpga_mgr_register(). This is fine for systems with one FPGA, but if
> multiple FPGAs are in use, the device ID can change as probe order
> changes across reboots or configuration changes.
>
> This makes it difficult to use the sysfs attributes, as the device path
> may change. Instead allow a static ID to be set by adding fpgaX props
> to the devicetree /aliases. Falling back to incrementing IDs if none are
> provided.
>
> This is based on the function rtc_device_get_id() in drivers/rtc/class.c
> (9d2b7e532da8 rtc: honor device tree /alias entries when assigning IDs).
>
> Signed-off-by: Brandon Maier <brandon.maier@rockwellcollins.com>
Acked-by: Alan Tull <atull@kernel.org>

Alan

> ---
>  drivers/fpga/fpga-mgr.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index c3866816456a..d8b7e7a61117 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -551,6 +551,26 @@ void fpga_mgr_unlock(struct fpga_manager *mgr)
>  }
>  EXPORT_SYMBOL_GPL(fpga_mgr_unlock);
>
> +static int fpga_mgr_get_id(struct device *dev)
> +{
> +       int of_id = -1, id = -1;
> +
> +       if (dev->of_node)
> +               of_id = of_alias_get_id(dev->of_node, "fpga");
> +
> +       if (of_id >= 0) {
> +               id = ida_simple_get(&fpga_mgr_ida, of_id, of_id + 1,
> +                                   GFP_KERNEL);
> +               if (id < 0)
> +                       dev_warn(dev, "/aliases ID %d not available\n", of_id);
> +       }
> +
> +       if (id < 0)
> +               id = ida_simple_get(&fpga_mgr_ida, 0, 0, GFP_KERNEL);
> +
> +       return id;
> +}
> +
>  /**
>   * fpga_mgr_create - create and initialize a FPGA manager struct
>   * @dev:       fpga manager device from pdev
> @@ -586,7 +606,7 @@ struct fpga_manager *fpga_mgr_create(struct device *dev, const char *name,
>         if (!mgr)
>                 return NULL;
>
> -       id = ida_simple_get(&fpga_mgr_ida, 0, 0, GFP_KERNEL);
> +       id = fpga_mgr_get_id(dev);
>         if (id < 0) {
>                 ret = id;
>                 goto error_kfree;
> --
> 2.20.1
>

Re: [PATCH] fpga: mgr: Use devicetree /alias to assign FPGA IDs

[Michal Simek]

[-- Attachment #1.1: Type: text/plain, Size: 2366 bytes --]

On 12. 04. 19 0:11, Brandon Maier wrote:
> The fpga-mgr assigns device IDs based on the order each driver calls
> fpga_mgr_register(). This is fine for systems with one FPGA, but if
> multiple FPGAs are in use, the device ID can change as probe order
> changes across reboots or configuration changes.
> 
> This makes it difficult to use the sysfs attributes, as the device path
> may change. Instead allow a static ID to be set by adding fpgaX props
> to the devicetree /aliases. Falling back to incrementing IDs if none are
> provided.
> 
> This is based on the function rtc_device_get_id() in drivers/rtc/class.c
> (9d2b7e532da8 rtc: honor device tree /alias entries when assigning IDs).
> 
> Signed-off-by: Brandon Maier <brandon.maier@rockwellcollins.com>
> ---
>  drivers/fpga/fpga-mgr.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index c3866816456a..d8b7e7a61117 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -551,6 +551,26 @@ void fpga_mgr_unlock(struct fpga_manager *mgr)
>  }
>  EXPORT_SYMBOL_GPL(fpga_mgr_unlock);
>  
> +static int fpga_mgr_get_id(struct device *dev)
> +{
> +	int of_id = -1, id = -1;
> +
> +	if (dev->of_node)
> +		of_id = of_alias_get_id(dev->of_node, "fpga");
> +
> +	if (of_id >= 0) {
> +		id = ida_simple_get(&fpga_mgr_ida, of_id, of_id + 1,
> +				    GFP_KERNEL);
> +		if (id < 0)
> +			dev_warn(dev, "/aliases ID %d not available\n", of_id);
> +	}
> +
> +	if (id < 0)
> +		id = ida_simple_get(&fpga_mgr_ida, 0, 0, GFP_KERNEL);
> +
> +	return id;
> +}

From the first look this is not proper implementation. It is not
handling cases where you have mix of devices with and without aliases.
If first mgr has no alias it gets 0, then second devices with fpga0
alias can't take it even it is assign to it.

For exactly this reason I have introduced of_alias_get_alias_list()
and take a look at cdns_get_id() in serial driver how I am using it.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] fpga: mgr: Use devicetree /alias to assign FPGA IDs

[Brandon Maier]

On Tue, Apr 16, 2019 at 7:06 AM Michal Simek <monstr@monstr.eu> wrote:
>
> From the first look this is not proper implementation. It is not
> handling cases where you have mix of devices with and without aliases.
> If first mgr has no alias it gets 0, then second devices with fpga0
> alias can't take it even it is assign to it.

From what I can tell, most of the drivers that use of_alias_get_id()
work this way. They assume the devicetree will either contain aliases
for all devices, or none at all; and prints a dev_warn() if there's a
mismatch. Not to say this is the best solution, but it's the
precedent.

>
> For exactly this reason I have introduced of_alias_get_alias_list()
> and take a look at cdns_get_id() in serial driver how I am using it.

It looks like cdns_get_id() uses bitmaps instead of IDA to handle
this, which complicates this. I'd prefer to not pull in those changes
for now if that's ok with the maintainers.

I could see value in adding an of.h helper like of_alias_ida_get()
that handles the boiler plate stuff of requesting the alias ID and
searching for a valid ID, though.

>
> Thanks,
> Michal
>
> --
> Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
> w: www.monstr.eu p: +42-0-721842854
> Maintainer of Linux kernel - Xilinx Microblaze
> Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
> U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs
>
>

Thanks,
Brandon