Enable upper layers using FPGA region w/o device tree
Subsystems (2)
v4 cover Enable upper layers using FPGA region w/o device tree
* Change the fpga-mgr API to have one fpga_mgr_load function instead of three.
* Expose API functions for FPGA region
* Separate common FPGA region code from Device Tree support
* Add API functions for bridges where DT is not used.
This is needed because the current FPGA layer has a couple of problems:
* We now have 3 functions for programming a FPGA, depending on whether the image is in a sg list, a buffer, or firmware. So upper layers have to be written assuming where the image will be or will have to write extra code to maintain flexibility.
* users who aren't using device tree are left to write their own code that is essentially a rewrite of FPGA region.
Mostly this patch set consists of small patches that rework the FPGA region support, followed by one big patch that moves the device tree FPGA region support to a separate file of-fpga-region.c.
v4 adds 3 patches:
* patch 16 supports overlays that don't program the fpga, such as an overlay that adds the devices that are in the static region of the FPGA
* patch 17 that cleans up the drivers/fpga/Kconfig
* patch 18 allows adding attribute groups when registering a fpga manager, bridge, or region.
v4 changes:
* Squashed v3's patch 1 (documentation) and patch 3 (api change)
* Fixes in function that allocates/frees fpga image info.
* Save the device that owns the image info in the image info struct.
These patches have been pushed to the linux-fpga kernel.org repo as branch review-next-20170913-fpga-region-v4 for reviewing convenience.
Alan Tull (18):
fpga: bridge: support getting bridge from device
fpga: mgr: API change to replace fpga load functions with single
function
fpga: mgr: separate getting/locking FPGA manager
fpga: region: use dev_err instead of pr_err
fpga: region: remove unneeded of_node_get and put
fpga: region: get mgr early on
fpga: region: check for child regions before allocing image info
fpga: region: fix slow warning with more than one overlay
fpga: region: use image info as parameter for programming region
fpga: region: separate out code that parses the overlay
fpga: region: add fpga-region.h header
fpga: region: rename some functions prior to moving
fpga: region: add register/unregister functions
fpga: region: add fpga_region_class_find
fpga: region: move device tree support to of-fpga-region.c
fpga: of-fpga-region: accept overlays that don't program FPGA
fpga: clean up fpga Kconfig
fpga: add attribute groups
Documentation/fpga/fpga-mgr.txt | 132 +++++----- Documentation/fpga/fpga-region.txt | 95 +++++++ Documentation/fpga/overview.txt | 23 ++ drivers/fpga/Kconfig | 102 ++++---- drivers/fpga/Makefile | 1 + drivers/fpga/fpga-bridge.c | 111 +++++++-- drivers/fpga/fpga-mgr.c | 121 +++++++-- drivers/fpga/fpga-region.c | 464 ++++------------------------------ drivers/fpga/of-fpga-region.c | 496 +++++++++++++++++++++++++++++++++++++ include/linux/fpga/fpga-bridge.h | 9 +- include/linux/fpga/fpga-mgr.h | 33 ++- include/linux/fpga/fpga-region.h | 40 +++ 12 files changed, 1036 insertions(+), 591 deletions(-) create mode 100644 Documentation/fpga/fpga-region.txt create mode 100644 Documentation/fpga/overview.txt create mode 100644 drivers/fpga/of-fpga-region.c create mode 100644 include/linux/fpga/fpga-region.h
Signature · 1 line
2.7.4
PATCH v4 1/18 fpga: bridge: support getting bridge from device
Add two functions for getting the FPGA bridge from the device rather than device tree node. This is to enable writing code that will support using FPGA bridges without device tree. Rename one old function to make it clear that it is device tree-ish. This leaves us with 3 functions for getting a bridge:
* fpga_bridge_get
Get the bridge given the device.
* fpga_bridges_get_to_list
Given the device, get the bridge and add it to a list.
* of_fpga_bridges_get_to_list
Renamed from priviously existing fpga_bridges_get_to_list. Given the device node, get the bridge and add it to a list.
Signed-off-by Alan Tull <[email protected]>
---
v2: use list_for_each_entry
static the bridge_list_lock
update copyright and author email
v3: no change to this patch in this version of patchset
v4: no change to this patch in this version of patchset
--- drivers/fpga/fpga-bridge.c | 110 +++++++++++++++++++++++++++++++-------- drivers/fpga/fpga-region.c | 11 ++-- include/linux/fpga/fpga-bridge.h | 7 ++- 3 files changed, 100 insertions(+), 28 deletions(-)
drivers/fpga/fpga-bridge.c +87 -23
@@ -2,6 +2,7 @@* FPGA Bridge Framework Driver * * Copyright (C) 2013-2016 Altera Corporation, All Rights Reserved. +* Copyright (C) 2017 Intel Corporation * * This program is free software; you can redistribute it and/or modify it * under the terms and conditions of the GNU General Public License,
@@ -70,29 +71,12 @@ int fpga_bridge_disable(struct fpga_bridge *bridge) }EXPORT_SYMBOL_GPL (fpga_bridge_disable); -/** -* of_fpga_bridge_get - get an exclusive reference to a fpga bridge -* -* @np: node pointer of a FPGA bridge -* @info: fpga image specific information -* -* Return fpga_bridge struct if successful. -* Return -EBUSY if someone already has a reference to the bridge. -* Return -ENODEV if @np is not a FPGA Bridge. -*/ -struct fpga_bridge *of_fpga_bridge_get (struct device_node *np, -struct fpga_image_info *info) - +struct fpga_bridge *__fpga_bridge_get (struct device *dev, +struct fpga_image_info *info) { -struct device *dev;struct fpga_bridge *bridge;int ret = -ENODEV ; - dev =class_find_device (fpga_bridge_class,NULL , np, - fpga_bridge_of_node_match); -if (!dev) - goto err_dev; - bridge =to_fpga_bridge (dev);if (!bridge) goto err_dev;
@@ -117,8 +101,58 @@ struct fpga_bridge *of_fpga_bridge_get(struct device_node *np,put_device (dev);return ERR_PTR (ret); } + +/** +* of_fpga_bridge_get - get an exclusive reference to a fpga bridge +* +* @np: node pointer of a FPGA bridge +* @info: fpga image specific information +* +* Return fpga_bridge struct if successful. +* Return -EBUSY if someone already has a reference to the bridge. +* Return -ENODEV if @np is not a FPGA Bridge. +*/ +struct fpga_bridge *of_fpga_bridge_get (struct device_node *np, +struct fpga_image_info *info) +{ +struct device *dev; + + dev =class_find_device (fpga_bridge_class,NULL , np, + fpga_bridge_of_node_match); +if (!dev) +return ERR_PTR (-ENODEV ); + +return __fpga_bridge_get (dev, info); +}EXPORT_SYMBOL_GPL (of_fpga_bridge_get); +static int fpga_bridge_dev_match (struct device *dev,const void *data) +{ +return dev->parent == data; +} + +/** +* fpga_bridge_get - get an exclusive reference to a fpga bridge +* @dev: parent device that fpga bridge was registered with +* +* Given a device, get an exclusive reference to a fpga bridge. +* +* Return: fpga manager struct or IS_ERR() condition containing error code. +*/ +struct fpga_bridge *fpga_bridge_get (struct device *dev, +struct fpga_image_info *info) +{ +struct device *bridge_dev; + + bridge_dev =class_find_device (fpga_bridge_class,NULL , dev, + fpga_bridge_dev_match); +if (!bridge_dev) +return ERR_PTR (-ENODEV ); + +return __fpga_bridge_get (bridge_dev, info); +} +EXPORT_SYMBOL_GPL (fpga_bridge_get); +/** * fpga_bridge_put - release a reference to a bridge *
@@ -206,7 +240,7 @@ void fpga_bridges_put(struct list_head *bridge_list)EXPORT_SYMBOL_GPL (fpga_bridges_put);/** -* fpga_bridges_get_to_list - get a bridge, add it to a list +* of_fpga_bridge_get_to_list - get a bridge, add it to a list * * @np: node pointer of a FPGA bridge * @info: fpga image specific information
@@ -216,14 +250,44 @@ EXPORT_SYMBOL_GPL(fpga_bridges_put);* * Return 0 for success, error code from of_fpga_bridge_get() othewise. */ -int fpga_bridge_get_to_list (struct device_node *np, +int of_fpga_bridge_get_to_list (struct device_node *np, +struct fpga_image_info *info, +struct list_head *bridge_list) +{ +struct fpga_bridge *bridge; +unsigned long flags; + + bridge =of_fpga_bridge_get (np, info); +if (IS_ERR (bridge)) +return PTR_ERR (bridge); + +spin_lock_irqsave (&bridge_list_lock, flags); +list_add (&bridge->node, bridge_list); +spin_unlock_irqrestore (&bridge_list_lock, flags); + +return 0 ; +} +EXPORT_SYMBOL_GPL (of_fpga_bridge_get_to_list); + +/** +* fpga_bridge_get_to_list - given device, get a bridge, add it to a list +* +* @dev: FPGA bridge device +* @info: fpga image specific information +* @bridge_list: list of FPGA bridges +* +* Get an exclusive reference to the bridge and and it to the list. +* +* Return 0 for success, error code from fpga_bridge_get() othewise. +*/ +int fpga_bridge_get_to_list (struct device *dev,struct fpga_image_info *info,struct list_head *bridge_list) {struct fpga_bridge *bridge;unsigned long flags; - bridge =of_fpga_bridge_get (np, info); + bridge =fpga_bridge_get (dev, info);if (IS_ERR (bridge))return PTR_ERR (bridge);
@@ -381,7 +445,7 @@ static void __exit fpga_bridge_dev_exit(void) }MODULE_DESCRIPTION ("FPGA Bridge Driver" ); -MODULE_AUTHOR ("Alan Tull <[email protected]>" ); +MODULE_AUTHOR ("Alan Tull <[email protected]>" );MODULE_LICENSE ("GPL v2" );subsys_initcall (fpga_bridge_dev_init);
drivers/fpga/fpga-region.c +7 -4
@@ -183,11 +183,14 @@ static int fpga_region_get_bridges(struct fpga_region *region,int i, ret;/* If parent is a bridge, add to list */ - ret =fpga_bridge_get_to_list (region_np->parent, region->info, - ®ion->bridge_list); + ret =of_fpga_bridge_get_to_list (region_np->parent, region->info, + ®ion->bridge_list); + +/* -EBUSY means parent is a bridge that is under use. Give up. */ if (ret == -EBUSY )return ret; +/* Zero return code means parent was a bridge and was added to list. */ if (!ret) parent_br = region_np->parent;
@@ -207,8 +210,8 @@ static int fpga_region_get_bridges(struct fpga_region *region,continue ;/* If node is a bridge, get it and add to list */ - ret =fpga_bridge_get_to_list (br, region->info, - ®ion->bridge_list); + ret =of_fpga_bridge_get_to_list (br, region->info, + ®ion->bridge_list);/* If any of the bridges are in use, give up */ if (ret == -EBUSY ) {
include/linux/fpga/fpga-bridge.h +6 -1
@@ -42,6 +42,8 @@ struct fpga_bridge {struct fpga_bridge *of_fpga_bridge_get (struct device_node *node,struct fpga_image_info *info); +struct fpga_bridge *fpga_bridge_get (struct device *dev, +struct fpga_image_info *info);void fpga_bridge_put (struct fpga_bridge *bridge);int fpga_bridge_enable (struct fpga_bridge *bridge);int fpga_bridge_disable (struct fpga_bridge *bridge);
@@ -49,9 +51,12 @@ int fpga_bridge_disable(struct fpga_bridge *bridge);int fpga_bridges_enable (struct list_head *bridge_list);int fpga_bridges_disable (struct list_head *bridge_list);void fpga_bridges_put (struct list_head *bridge_list); -int fpga_bridge_get_to_list (struct device_node *np, +int fpga_bridge_get_to_list (struct device *dev,struct fpga_image_info *info,struct list_head *bridge_list); +int of_fpga_bridge_get_to_list (struct device_node *np, +struct fpga_image_info *info, +struct list_head *bridge_list);int fpga_bridge_register (struct device *dev,const char *name,const struct fpga_bridge_ops *br_ops,void *priv);
Signature · 1 line
2.7.4
Re: [PATCH v4 01/18] fpga: bridge: support getting bridge from device
Hi Alan,
Two minor nits below.
Matthew Gerlach
Quoted 50 lines · Alan Tull
Add two functions for getting the FPGA bridge from the device rather than device tree node. This is to enable writing code that will support using FPGA bridges without device tree. Rename one old function to make it clear that it is device tree-ish. This leaves us with 3 functions for getting a bridge:
*
fpga_bridge_get
Get the bridge given the device.*
fpga_bridges_get_to_list
Given the device, get the bridge and add it to a list.*
of_fpga_bridges_get_to_list
Renamed from priviously existingfpga_bridges_get_to_list. Given the device node, get the bridge and add it to a list.Signed-off-by Alan Tull <[email protected]>---
v2: uselist_for_each_entry
static thebridge_list_lock
update copyright and author email
v3: no change to this patch in this version of patchset
v4: no change to this patch in this version of patchset
---drivers/fpga/fpga-bridge.c | 110 +++++++++++++++++++++++++++++++-------- drivers/fpga/fpga-region.c | 11 ++-- include/linux/fpga/fpga-bridge.h | 7 ++- 3 files changed, 100 insertions(+), 28 deletions(-)drivers/fpga/fpga-bridge.c +1
-5@@ -2,6 +2,7 @@* FPGA Bridge Framework Driver * * Copyright (C) 2013-2016 Altera Corporation, All Rights Reserved. +* Copyright (C) 2017 Intel Corporation * * This program is free software; you can redistribute it and/or modify it * under the terms and conditions of the GNU General Public License, @@ -70,29 +71,12 @@ int fpga_bridge_disable(struct fpga_bridge *bridge) }EXPORT_SYMBOL_GPL (fpga_bridge_disable); -/** -* of_fpga_bridge_get - get an exclusive reference to a fpga bridge -* -* @np: node pointer of a FPGA bridge -* @info: fpga image specific information
drivers/fpga/fpga-bridge.c int fpga_bridge_disable(struct fpga_bridge *bridge) · line 75
Quoted 10 lines
drivers/fpga/fpga-bridge.c +2
-8-* -* Return fpga_bridge struct if successful. -* Return -EBUSY if someone already has a reference to the bridge. -* Return -ENODEV if @np is not a FPGA Bridge. -*/ -struct fpga_bridge *of_fpga_bridge_get (struct device_node *np, -struct fpga_image_info *info) - +struct fpga_bridge *__fpga_bridge_get (struct device *dev, +struct fpga_image_info *info)
Should this be a static function?
I was recently told by mtd maintainers that function names prefixed with __ should be avoided.
Quoted 204 lines
drivers/fpga/fpga-bridge.c +84
-10{ -struct device *dev;struct fpga_bridge *bridge;int ret = -ENODEV ; - dev =class_find_device (fpga_bridge_class,NULL , np, - fpga_bridge_of_node_match); -if (!dev) - goto err_dev; - bridge =to_fpga_bridge (dev);if (!bridge) goto err_dev;@@ -117,8 +101,58 @@ struct fpga_bridge *of_fpga_bridge_get(struct device_node *np,put_device (dev);return ERR_PTR (ret); } + +/** +* of_fpga_bridge_get - get an exclusive reference to a fpga bridge +* +* @np: node pointer of a FPGA bridge +* @info: fpga image specific information +* +* Return fpga_bridge struct if successful. +* Return -EBUSY if someone already has a reference to the bridge. +* Return -ENODEV if @np is not a FPGA Bridge. +*/ +struct fpga_bridge *of_fpga_bridge_get (struct device_node *np, +struct fpga_image_info *info) +{ +struct device *dev; + + dev =class_find_device (fpga_bridge_class,NULL , np, + fpga_bridge_of_node_match); +if (!dev) +return ERR_PTR (-ENODEV ); + +return __fpga_bridge_get (dev, info); +}EXPORT_SYMBOL_GPL (of_fpga_bridge_get); +static int fpga_bridge_dev_match (struct device *dev,const void *data) +{ +return dev->parent == data; +} + +/** +* fpga_bridge_get - get an exclusive reference to a fpga bridge +* @dev: parent device that fpga bridge was registered with +* +* Given a device, get an exclusive reference to a fpga bridge. +* +* Return: fpga manager struct or IS_ERR() condition containing error code. +*/ +struct fpga_bridge *fpga_bridge_get (struct device *dev, +struct fpga_image_info *info) +{ +struct device *bridge_dev; + + bridge_dev =class_find_device (fpga_bridge_class,NULL , dev, + fpga_bridge_dev_match); +if (!bridge_dev) +return ERR_PTR (-ENODEV ); + +return __fpga_bridge_get (bridge_dev, info); +} +EXPORT_SYMBOL_GPL (fpga_bridge_get); +/** * fpga_bridge_put - release a reference to a bridge * @@ -206,7 +240,7 @@ void fpga_bridges_put(struct list_head *bridge_list)EXPORT_SYMBOL_GPL (fpga_bridges_put);/** -* fpga_bridges_get_to_list - get a bridge, add it to a list +* of_fpga_bridge_get_to_list - get a bridge, add it to a list * * @np: node pointer of a FPGA bridge * @info: fpga image specific information @@ -216,14 +250,44 @@ EXPORT_SYMBOL_GPL(fpga_bridges_put);* * Return 0 for success, error code from of_fpga_bridge_get() othewise. */ -int fpga_bridge_get_to_list (struct device_node *np, +int of_fpga_bridge_get_to_list (struct device_node *np, +struct fpga_image_info *info, +struct list_head *bridge_list) +{ +struct fpga_bridge *bridge; +unsigned long flags; + + bridge =of_fpga_bridge_get (np, info); +if (IS_ERR (bridge)) +return PTR_ERR (bridge); + +spin_lock_irqsave (&bridge_list_lock, flags); +list_add (&bridge->node, bridge_list); +spin_unlock_irqrestore (&bridge_list_lock, flags); + +return 0 ; +} +EXPORT_SYMBOL_GPL (of_fpga_bridge_get_to_list); + +/** +* fpga_bridge_get_to_list - given device, get a bridge, add it to a list +* +* @dev: FPGA bridge device +* @info: fpga image specific information +* @bridge_list: list of FPGA bridges +* +* Get an exclusive reference to the bridge and and it to the list. +* +* Return 0 for success, error code from fpga_bridge_get() othewise. +*/ +int fpga_bridge_get_to_list (struct device *dev,struct fpga_image_info *info,struct list_head *bridge_list) {struct fpga_bridge *bridge;unsigned long flags; - bridge =of_fpga_bridge_get (np, info); + bridge =fpga_bridge_get (dev, info);if (IS_ERR (bridge))return PTR_ERR (bridge);@@ -381,7 +445,7 @@ static void __exit fpga_bridge_dev_exit(void) }MODULE_DESCRIPTION ("FPGA Bridge Driver" ); -MODULE_AUTHOR ("Alan Tull <[email protected]>" ); +MODULE_AUTHOR ("Alan Tull <[email protected]>" );MODULE_LICENSE ("GPL v2" );subsys_initcall (fpga_bridge_dev_init);drivers/fpga/fpga-region.c +7
-4@@ -183,11 +183,14 @@ static int fpga_region_get_bridges(struct fpga_region *region,int i, ret;/* If parent is a bridge, add to list */ - ret =fpga_bridge_get_to_list (region_np->parent, region->info, - ®ion->bridge_list); + ret =of_fpga_bridge_get_to_list (region_np->parent, region->info, + ®ion->bridge_list); + +/* -EBUSY means parent is a bridge that is under use. Give up. */ if (ret == -EBUSY )return ret; +/* Zero return code means parent was a bridge and was added to list. */ if (!ret) parent_br = region_np->parent;@@ -207,8 +210,8 @@ static int fpga_region_get_bridges(struct fpga_region *region,continue ;/* If node is a bridge, get it and add to list */ - ret =fpga_bridge_get_to_list (br, region->info, - ®ion->bridge_list); + ret =of_fpga_bridge_get_to_list (br, region->info, + ®ion->bridge_list);/* If any of the bridges are in use, give up */ if (ret == -EBUSY ) {include/linux/fpga/fpga-bridge.h +6
-1@@ -42,6 +42,8 @@ struct fpga_bridge {struct fpga_bridge *of_fpga_bridge_get (struct device_node *node,struct fpga_image_info *info); +struct fpga_bridge *fpga_bridge_get (struct device *dev, +struct fpga_image_info *info);void fpga_bridge_put (struct fpga_bridge *bridge);int fpga_bridge_enable (struct fpga_bridge *bridge);int fpga_bridge_disable (struct fpga_bridge *bridge);@@ -49,9 +51,12 @@ int fpga_bridge_disable(struct fpga_bridge *bridge);int fpga_bridges_enable (struct list_head *bridge_list);int fpga_bridges_disable (struct list_head *bridge_list);void fpga_bridges_put (struct list_head *bridge_list); -int fpga_bridge_get_to_list (struct device_node *np, +int fpga_bridge_get_to_list (struct device *dev,struct fpga_image_info *info,struct list_head *bridge_list); +int of_fpga_bridge_get_to_list (struct device_node *np, +struct fpga_image_info *info, +struct list_head *bridge_list);int fpga_bridge_register (struct device *dev,const char *name,const struct fpga_bridge_ops *br_ops,void *priv);Signature · 6 lines
2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-fpga" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 01/18] fpga: bridge: support getting bridge from device
On Wed, Sep 13, 2017 at 6:38 PM, <[email protected]> wrote:
Hi Matthew,
Quoted 65 lines
Hi Alan,
Two minor nits below.
Matthew Gerlach
Quoted 56 lines · Alan Tull
Add two functions for getting the FPGA bridge from the device rather than device tree node. This is to enable writing code that will support using FPGA bridges without device tree. Rename one old function to make it clear that it is device tree-ish. This leaves us with 3 functions for getting a bridge:
*
fpga_bridge_get
Get the bridge given the device.*
fpga_bridges_get_to_list
Given the device, get the bridge and add it to a list.*
of_fpga_bridges_get_to_list
Renamed from priviously existingfpga_bridges_get_to_list. Given the device node, get the bridge and add it to a list.Signed-off-by Alan Tull <[email protected]>---
v2: uselist_for_each_entry
static thebridge_list_lock
update copyright and author email
v3: no change to this patch in this version of patchset
v4: no change to this patch in this version of patchset
---drivers/fpga/fpga-bridge.c| 110
+++++++++++++++++++++++++++++++--------drivers/fpga/fpga-region.c| 11 ++--include/linux/fpga/fpga-bridge.h| 7 ++-3 files changed, 100 insertions(+), 28 deletions(-)drivers/fpga/fpga-bridge.c +1
-10@@ -2,6 +2,7 @@* FPGA Bridge Framework Driver * * Copyright (C) 2013-2016 Altera Corporation, All Rights Reserved. +* Copyright (C) 2017 Intel Corporation * * This program is free software; you can redistribute it and/or modify it * under the terms and conditions of the GNU General Public License, @@ -70,29 +71,12 @@ int fpga_bridge_disable(struct fpga_bridge *bridge) }EXPORT_SYMBOL_GPL (fpga_bridge_disable); -/** -* of_fpga_bridge_get - get an exclusive reference to a fpga bridge -* -* @np: node pointer of a FPGA bridge -* @info: fpga image specific information -* -* Return fpga_bridge struct if successful. -* Return -EBUSY if someone already has a reference to the bridge. -* Return -ENODEV if @np is not a FPGA Bridge. -*/
Quoted 8 lines
drivers/fpga/fpga-bridge.c int fpga_bridge_disable(struct fpga_bridge *bridge) · line 75
Quoted 5 lines
drivers/fpga/fpga-bridge.c +2
-3-struct fpga_bridge *of_fpga_bridge_get (struct device_node *np, -struct fpga_image_info *info) - +struct fpga_bridge *__fpga_bridge_get (struct device *dev, +struct fpga_image_info *info)Should this be a static function?
You are right. Will fix in v5.
Quoted 3 lines
I was recently told by mtd maintainers that function names prefixed with __ should be avoided.
I see functions named thusly around in the kernel. Can you point me
to that thread or let me know what their thinking was about this? I am open for suggestions for a new function name.
Alan
Re: [PATCH v4 01/18] fpga: bridge: support getting bridge from device
Quoted 81 lines · Alan Tull
On Wed, Sep 13, 2017 at 6:38 PM, <[email protected]> wrote:
Hi Matthew,
Quoted 65 lines
Hi Alan,
Two minor nits below.
Matthew Gerlach
Quoted 56 lines · Alan Tull
Add two functions for getting the FPGA bridge from the device rather than device tree node. This is to enable writing code that will support using FPGA bridges without device tree. Rename one old function to make it clear that it is device tree-ish. This leaves us with 3 functions for getting a bridge:
*
fpga_bridge_get
Get the bridge given the device.*
fpga_bridges_get_to_list
Given the device, get the bridge and add it to a list.*
of_fpga_bridges_get_to_list
Renamed from priviously existingfpga_bridges_get_to_list. Given the device node, get the bridge and add it to a list.Signed-off-by Alan Tull <[email protected]>---
v2: uselist_for_each_entry
static thebridge_list_lock
update copyright and author email
v3: no change to this patch in this version of patchset
v4: no change to this patch in this version of patchset
---drivers/fpga/fpga-bridge.c| 110
+++++++++++++++++++++++++++++++--------drivers/fpga/fpga-region.c| 11 ++--include/linux/fpga/fpga-bridge.h| 7 ++-3 files changed, 100 insertions(+), 28 deletions(-)drivers/fpga/fpga-bridge.c +1
-10@@ -2,6 +2,7 @@* FPGA Bridge Framework Driver * * Copyright (C) 2013-2016 Altera Corporation, All Rights Reserved. +* Copyright (C) 2017 Intel Corporation * * This program is free software; you can redistribute it and/or modify it * under the terms and conditions of the GNU General Public License, @@ -70,29 +71,12 @@ int fpga_bridge_disable(struct fpga_bridge *bridge) }EXPORT_SYMBOL_GPL (fpga_bridge_disable); -/** -* of_fpga_bridge_get - get an exclusive reference to a fpga bridge -* -* @np: node pointer of a FPGA bridge -* @info: fpga image specific information -* -* Return fpga_bridge struct if successful. -* Return -EBUSY if someone already has a reference to the bridge. -* Return -ENODEV if @np is not a FPGA Bridge. -*/ Quoted 8 lines
drivers/fpga/fpga-bridge.c int fpga_bridge_disable(struct fpga_bridge *bridge) · line 75
Quoted 5 lines
drivers/fpga/fpga-bridge.c +2
-3-struct fpga_bridge *of_fpga_bridge_get (struct device_node *np, -struct fpga_image_info *info) - +struct fpga_bridge *__fpga_bridge_get (struct device *dev, +struct fpga_image_info *info)Should this be a static function?
You are right. Will fix in v5.
Quoted 1 line
Quoted 6 lines
Quoted 2 lines
I was recently told by mtd maintainers that function names prefixed with __ should be avoided.
I see functions named thusly around in the kernel. Can you point me
to that thread or let me know what their thinking was about this? I am open for suggestions for a new function name.
Marek Vasut just told me to "Avoid function names with __ prefix" in
https://patchwork.kernel.org/patch/9883977/
I tend to agree with you that the __ prefix seems to be around, but this could be a case of "evolution" of coding style. Historically, __ seems to mean an internal version of an external function. If __ is to be avoided, we might have to rename to fpga_bridge_get_internal().
Quoted 7 lines
Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Moritz Fischer
maintainerRe: [PATCH v4 01/18] fpga: bridge: support getting bridge from device
Quoted 92 lines · On Thu, Sep 14, 2017 at 03:29:09PM -0700, [email protected] wrote:
Quoted 81 lines · Alan Tull
On Wed, Sep 13, 2017 at 6:38 PM, <[email protected]> wrote:
Hi Matthew,
Quoted 65 lines
Hi Alan,
Two minor nits below.
Matthew Gerlach
Quoted 56 lines · Alan Tull
Add two functions for getting the FPGA bridge from the device rather than device tree node. This is to enable writing code that will support using FPGA bridges without device tree. Rename one old function to make it clear that it is device tree-ish. This leaves us with 3 functions for getting a bridge:
*
fpga_bridge_get
Get the bridge given the device.*
fpga_bridges_get_to_list
Given the device, get the bridge and add it to a list.*
of_fpga_bridges_get_to_list
Renamed from priviously existingfpga_bridges_get_to_list. Given the device node, get the bridge and add it to a list.Signed-off-by Alan Tull <[email protected]>---
v2: uselist_for_each_entry
static thebridge_list_lock
update copyright and author email
v3: no change to this patch in this version of patchset
v4: no change to this patch in this version of patchset
---drivers/fpga/fpga-bridge.c| 110
+++++++++++++++++++++++++++++++--------drivers/fpga/fpga-region.c| 11 ++--include/linux/fpga/fpga-bridge.h| 7 ++-3 files changed, 100 insertions(+), 28 deletions(-)drivers/fpga/fpga-bridge.c +1
-10@@ -2,6 +2,7 @@* FPGA Bridge Framework Driver * * Copyright (C) 2013-2016 Altera Corporation, All Rights Reserved. +* Copyright (C) 2017 Intel Corporation * * This program is free software; you can redistribute it and/or modify it * under the terms and conditions of the GNU General Public License, @@ -70,29 +71,12 @@ int fpga_bridge_disable(struct fpga_bridge *bridge) }EXPORT_SYMBOL_GPL (fpga_bridge_disable); -/** -* of_fpga_bridge_get - get an exclusive reference to a fpga bridge -* -* @np: node pointer of a FPGA bridge -* @info: fpga image specific information -* -* Return fpga_bridge struct if successful. -* Return -EBUSY if someone already has a reference to the bridge. -* Return -ENODEV if @np is not a FPGA Bridge. -*/ Quoted 8 lines
drivers/fpga/fpga-bridge.c int fpga_bridge_disable(struct fpga_bridge *bridge) · line 75
Quoted 5 lines
drivers/fpga/fpga-bridge.c +2
-3-struct fpga_bridge *of_fpga_bridge_get (struct device_node *np, -struct fpga_image_info *info) - +struct fpga_bridge *__fpga_bridge_get (struct device *dev, +struct fpga_image_info *info)Should this be a static function?
You are right. Will fix in v5.
Quoted 1 line
Quoted 6 lines
Quoted 2 lines
I was recently told by mtd maintainers that function names prefixed with __ should be avoided.
I see functions named thusly around in the kernel. Can you point me
to that thread or let me know what their thinking was about this? I am open for suggestions for a new function name.
Quoted 8 lines
Marek Vasut just told me to "Avoid function names with __ prefix" in
https://patchwork.kernel.org/patch/9883977/
I tend to agree with you that the __ prefix seems to be around, but this could be a case of "evolution" of coding style. Historically, __ seems to mean an internal version of an external function. If __ is to be avoided, we might have to rename to
fpga_bridge_get_internal().
Bear in mind that coding style can be subsystem specific. What applies to MTD doesn't necessarily have to apply here.
Quoted 9 lines
Quoted 8 lines
Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks,
Moritz
PS: I'm travelling this week, will get around to reviews next week
Re: [PATCH v4 01/18] fpga: bridge: support getting bridge from device
Quoted 97 lines · Moritz Fischer
Quoted 96 lines · On Thu, Sep 14, 2017 at 03:29:09PM -0700, [email protected] wrote:
Quoted 81 lines · Alan Tull
On Wed, Sep 13, 2017 at 6:38 PM, <[email protected]> wrote:
Hi Matthew,
Quoted 65 lines
Hi Alan,
Two minor nits below.
Matthew Gerlach
Quoted 56 lines · Alan Tull
Add two functions for getting the FPGA bridge from the device rather than device tree node. This is to enable writing code that will support using FPGA bridges without device tree. Rename one old function to make it clear that it is device tree-ish. This leaves us with 3 functions for getting a bridge:
*
fpga_bridge_get
Get the bridge given the device.*
fpga_bridges_get_to_list
Given the device, get the bridge and add it to a list.*
of_fpga_bridges_get_to_list
Renamed from priviously existingfpga_bridges_get_to_list. Given the device node, get the bridge and add it to a list.Signed-off-by Alan Tull <[email protected]>---
v2: uselist_for_each_entry
static thebridge_list_lock
update copyright and author email
v3: no change to this patch in this version of patchset
v4: no change to this patch in this version of patchset
---drivers/fpga/fpga-bridge.c| 110
+++++++++++++++++++++++++++++++--------drivers/fpga/fpga-region.c| 11 ++--include/linux/fpga/fpga-bridge.h| 7 ++-3 files changed, 100 insertions(+), 28 deletions(-)drivers/fpga/fpga-bridge.c +1
-10@@ -2,6 +2,7 @@* FPGA Bridge Framework Driver * * Copyright (C) 2013-2016 Altera Corporation, All Rights Reserved. +* Copyright (C) 2017 Intel Corporation * * This program is free software; you can redistribute it and/or modify it * under the terms and conditions of the GNU General Public License, @@ -70,29 +71,12 @@ int fpga_bridge_disable(struct fpga_bridge *bridge) }EXPORT_SYMBOL_GPL (fpga_bridge_disable); -/** -* of_fpga_bridge_get - get an exclusive reference to a fpga bridge -* -* @np: node pointer of a FPGA bridge -* @info: fpga image specific information -* -* Return fpga_bridge struct if successful. -* Return -EBUSY if someone already has a reference to the bridge. -* Return -ENODEV if @np is not a FPGA Bridge. -*/ Quoted 8 lines
drivers/fpga/fpga-bridge.c int fpga_bridge_disable(struct fpga_bridge *bridge) · line 75
Quoted 5 lines
drivers/fpga/fpga-bridge.c +2
-3-struct fpga_bridge *of_fpga_bridge_get (struct device_node *np, -struct fpga_image_info *info) - +struct fpga_bridge *__fpga_bridge_get (struct device *dev, +struct fpga_image_info *info)Should this be a static function?
You are right. Will fix in v5.
Quoted 1 line
Quoted 6 lines
Quoted 2 lines
I was recently told by mtd maintainers that function names prefixed with __ should be avoided.
I see functions named thusly around in the kernel. Can you point me
to that thread or let me know what their thinking was about this? I am open for suggestions for a new function name.Marek Vasut just told me to "Avoid function names with __ prefix" in
Quoted 7 lines
Quoted 4 lines
I tend to agree with you that the __ prefix seems to be around, but this could be a case of "evolution" of coding style. Historically, __ seems to mean an internal version of an external function. If __ is to be avoided, we might have to rename to
fpga_bridge_get_internal().Bear in mind that coding style can be subsystem specific. What applies to MTD doesn't necessarily have to apply here.
Yes I agree. I'll keep the __ name unless someone gives us good explanation of the naming practice they are recommending. But yes, it should have been static.
Alan
Moritz Fischer
maintainerRe: [PATCH v4 01/18] fpga: bridge: support getting bridge from device
In reply to Alan TullQuoted 150 lines · Alan Tull
Add two functions for getting the FPGA bridge from the device rather than device tree node. This is to enable writing code that will support using FPGA bridges without device tree. Rename one old function to make it clear that it is device tree-ish. This leaves us with 3 functions for getting a bridge:
*
fpga_bridge_get
Get the bridge given the device.*
fpga_bridges_get_to_list
Given the device, get the bridge and add it to a list.*
of_fpga_bridges_get_to_list
Renamed from priviously existingfpga_bridges_get_to_list. Given the device node, get the bridge and add it to a list.Signed-off-by Alan Tull <[email protected]>---
v2: uselist_for_each_entry
static thebridge_list_lock
update copyright and author email
v3: no change to this patch in this version of patchset
v4: no change to this patch in this version of patchset--- drivers/fpga/fpga-bridge.c | 110 +++++++++++++++++++++++++++++++-------- drivers/fpga/fpga-region.c | 11 ++-- include/linux/fpga/fpga-bridge.h | 7 ++- 3 files changed, 100 insertions(+), 28 deletions(-)drivers/fpga/fpga-bridge.c +58
-21@@ -2,6 +2,7 @@* FPGA Bridge Framework Driver * * Copyright (C) 2013-2016 Altera Corporation, All Rights Reserved. +* Copyright (C) 2017 Intel Corporation * * This program is free software; you can redistribute it and/or modify it * under the terms and conditions of the GNU General Public License, @@ -70,29 +71,12 @@ int fpga_bridge_disable(struct fpga_bridge *bridge) }EXPORT_SYMBOL_GPL (fpga_bridge_disable); -/** -* of_fpga_bridge_get - get an exclusive reference to a fpga bridge -* -* @np: node pointer of a FPGA bridge -* @info: fpga image specific information -* -* Return fpga_bridge struct if successful. -* Return -EBUSY if someone already has a reference to the bridge. -* Return -ENODEV if @np is not a FPGA Bridge. -*/ -struct fpga_bridge *of_fpga_bridge_get (struct device_node *np, -struct fpga_image_info *info) - +struct fpga_bridge *__fpga_bridge_get (struct device *dev, +struct fpga_image_info *info) { -struct device *dev;struct fpga_bridge *bridge;int ret = -ENODEV ; - dev =class_find_device (fpga_bridge_class,NULL , np, - fpga_bridge_of_node_match); -if (!dev) - goto err_dev; - bridge =to_fpga_bridge (dev);if (!bridge) goto err_dev;@@ -117,8 +101,58 @@ struct fpga_bridge *of_fpga_bridge_get(struct device_node *np,put_device (dev);return ERR_PTR (ret); } + +/** +* of_fpga_bridge_get - get an exclusive reference to a fpga bridge +* +* @np: node pointer of a FPGA bridge +* @info: fpga image specific information +* +* Return fpga_bridge struct if successful. +* Return -EBUSY if someone already has a reference to the bridge. +* Return -ENODEV if @np is not a FPGA Bridge. +*/ +struct fpga_bridge *of_fpga_bridge_get (struct device_node *np, +struct fpga_image_info *info) +{ +struct device *dev; + + dev =class_find_device (fpga_bridge_class,NULL , np, + fpga_bridge_of_node_match); +if (!dev) +return ERR_PTR (-ENODEV ); + +return __fpga_bridge_get (dev, info); +}EXPORT_SYMBOL_GPL (of_fpga_bridge_get); +static int fpga_bridge_dev_match (struct device *dev,const void *data) +{ +return dev->parent == data; +} + +/** +* fpga_bridge_get - get an exclusive reference to a fpga bridge +* @dev: parent device that fpga bridge was registered with +* +* Given a device, get an exclusive reference to a fpga bridge. +* +* Return: fpga manager struct or IS_ERR() condition containing error code. +*/ +struct fpga_bridge *fpga_bridge_get (struct device *dev, +struct fpga_image_info *info) +{ +struct device *bridge_dev; + + bridge_dev =class_find_device (fpga_bridge_class,NULL , dev, + fpga_bridge_dev_match); +if (!bridge_dev) +return ERR_PTR (-ENODEV ); + +return __fpga_bridge_get (bridge_dev, info); +} +EXPORT_SYMBOL_GPL (fpga_bridge_get); +/** * fpga_bridge_put - release a reference to a bridge * @@ -206,7 +240,7 @@ void fpga_bridges_put(struct list_head *bridge_list)EXPORT_SYMBOL_GPL (fpga_bridges_put);/** -* fpga_bridges_get_to_list - get a bridge, add it to a list +* of_fpga_bridge_get_to_list - get a bridge, add it to a list * * @np: node pointer of a FPGA bridge * @info: fpga image specific information @@ -216,14 +250,44 @@ EXPORT_SYMBOL_GPL(fpga_bridges_put);* * Return 0 for success, error code from of_fpga_bridge_get() othewise. */ -int fpga_bridge_get_to_list(struct device_node *np, +int of_fpga_bridge_get_to_list (struct device_node *np, +struct fpga_image_info *info, +struct list_head *bridge_list) +{
drivers/fpga/fpga-bridge.c EXPORT_SYMBOL_GPL(fpga_bridges_put); · line 266
Quoted 10 lines
drivers/fpga/fpga-bridge.c +10
-0+struct fpga_bridge *bridge; +unsigned long flags; + + bridge =of_fpga_bridge_get (np, info); +if (IS_ERR (bridge)) +return PTR_ERR (bridge); + +spin_lock_irqsave (&bridge_list_lock, flags); +list_add (&bridge->node, bridge_list); +spin_unlock_irqrestore (&bridge_list_lock, flags);
I know this is not new code, but I was wondering the other day:
Why are we using a single spinlock to protect all lists that are being passed in as parameters here?
I have a patch that moves the spinlock into the region containing the list that uses the bridges which (unless I misunderstand something here), makes more sense:
drivers/fpga/fpga-bridge.c +2 -3
@@ -214,6 +214,8 @@ EXPORT_SYMBOL_GPL(fpga_bridges_put);* * Get an exclusive reference to the bridge and and it to the list. * +* Must be called with list lock held. +* * Return 0 for success, error code from of_fpga_bridge_get() othewise. */ int fpga_bridge_get_to_list(struct device_node *np,
@@ -221,15 +223,12 @@ int fpga_bridge_get_to_list(struct device_node *np, struct list_head *bridge_list) {struct fpga_bridge *bridge; -unsigned long flags; bridge =of_fpga_bridge_get (np, info);if (IS_ERR (bridge))return PTR_ERR (bridge); -spin_lock_irqsave (&bridge_list_lock, flags);list_add (&bridge->node, bridge_list); -spin_unlock_irqrestore (&bridge_list_lock, flags);return 0 ; }
drivers/fpga/fpga-region.c +6 -0
@@ -37,6 +37,7 @@ struct fpga_region {struct device dev;struct mutex mutex;/* for exclusive reference to region */ struct list_head bridge_list; +spinlock_t bridge_list_lock;/* protects access to bridge list */ struct fpga_image_info *info; };
@@ -180,11 +181,15 @@ static int fpga_region_get_bridges(struct fpga_region *region,struct device *dev = ®ion->dev;struct device_node *region_np = dev->of_node;struct device_node *br, *np, *parent_br =NULL ; +unsigned long flags;int i, ret;/* If parent is a bridge, add to list */ +spin_lock_irqsave (®ion->bridge_list_lock, flags); ret =fpga_bridge_get_to_list (region_np->parent, region->info, ®ion->bridge_list); +spin_unlock_irqrestore (®ion->bridge_list_lock, flags); +if (ret == -EBUSY )return ret;
@@ -508,6 +513,7 @@ static int fpga_region_probe(struct platform_device *pdev)mutex_init (®ion->mutex);INIT_LIST_HEAD (®ion->bridge_list); +spin_lock_init (®ion->bridge_list_lock);device_initialize (®ion->dev); region->dev.class = fpga_region_class;
Am I missing something here? If not I"ll send out my patch separately.
Quoted 99 lines
drivers/fpga/fpga-bridge.c +19
-2+ +return 0 ; +} +EXPORT_SYMBOL_GPL (of_fpga_bridge_get_to_list); + +/** +* fpga_bridge_get_to_list - given device, get a bridge, add it to a list +* +* @dev: FPGA bridge device +* @info: fpga image specific information +* @bridge_list: list of FPGA bridges +* +* Get an exclusive reference to the bridge and and it to the list. +* +* Return 0 for success, error code from fpga_bridge_get() othewise. +*/ +int fpga_bridge_get_to_list (struct device *dev,struct fpga_image_info *info,struct list_head *bridge_list) {struct fpga_bridge *bridge;unsigned long flags; - bridge =of_fpga_bridge_get (np, info); + bridge =fpga_bridge_get (dev, info);if (IS_ERR (bridge))return PTR_ERR (bridge);@@ -381,7 +445,7 @@ static void __exit fpga_bridge_dev_exit(void) }MODULE_DESCRIPTION ("FPGA Bridge Driver" ); -MODULE_AUTHOR ("Alan Tull <[email protected]>" ); +MODULE_AUTHOR ("Alan Tull <[email protected]>" );MODULE_LICENSE ("GPL v2" );subsys_initcall (fpga_bridge_dev_init);drivers/fpga/fpga-region.c +7
-4@@ -183,11 +183,14 @@ static int fpga_region_get_bridges(struct fpga_region *region,int i, ret;/* If parent is a bridge, add to list */ - ret =fpga_bridge_get_to_list (region_np->parent, region->info, - ®ion->bridge_list); + ret =of_fpga_bridge_get_to_list (region_np->parent, region->info, + ®ion->bridge_list); + +/* -EBUSY means parent is a bridge that is under use. Give up. */ if (ret == -EBUSY )return ret; +/* Zero return code means parent was a bridge and was added to list. */ if (!ret) parent_br = region_np->parent;@@ -207,8 +210,8 @@ static int fpga_region_get_bridges(struct fpga_region *region,continue ;/* If node is a bridge, get it and add to list */ - ret =fpga_bridge_get_to_list (br, region->info, - ®ion->bridge_list); + ret =of_fpga_bridge_get_to_list (br, region->info, + ®ion->bridge_list);/* If any of the bridges are in use, give up */ if (ret == -EBUSY ) {include/linux/fpga/fpga-bridge.h +6
-1@@ -42,6 +42,8 @@ struct fpga_bridge {struct fpga_bridge *of_fpga_bridge_get (struct device_node *node,struct fpga_image_info *info); +struct fpga_bridge *fpga_bridge_get (struct device *dev, +struct fpga_image_info *info);void fpga_bridge_put (struct fpga_bridge *bridge);int fpga_bridge_enable (struct fpga_bridge *bridge);int fpga_bridge_disable (struct fpga_bridge *bridge);@@ -49,9 +51,12 @@ int fpga_bridge_disable(struct fpga_bridge *bridge);int fpga_bridges_enable (struct list_head *bridge_list);int fpga_bridges_disable (struct list_head *bridge_list);void fpga_bridges_put (struct list_head *bridge_list); -int fpga_bridge_get_to_list (struct device_node *np, +int fpga_bridge_get_to_list (struct device *dev,struct fpga_image_info *info,struct list_head *bridge_list); +int of_fpga_bridge_get_to_list (struct device_node *np, +struct fpga_image_info *info, +struct list_head *bridge_list);int fpga_bridge_register (struct device *dev,const char *name,const struct fpga_bridge_ops *br_ops,void *priv);Signature · 1 line
2.7.4
Thanks,
Moritz
Re: [PATCH v4 01/18] fpga: bridge: support getting bridge from device
Quoted 237 lines · Moritz Fischer
Quoted 150 lines · Alan Tull
Add two functions for getting the FPGA bridge from the device rather than device tree node. This is to enable writing code that will support using FPGA bridges without device tree. Rename one old function to make it clear that it is device tree-ish. This leaves us with 3 functions for getting a bridge:
*
fpga_bridge_get
Get the bridge given the device.*
fpga_bridges_get_to_list
Given the device, get the bridge and add it to a list.*
of_fpga_bridges_get_to_list
Renamed from priviously existingfpga_bridges_get_to_list. Given the device node, get the bridge and add it to a list.Signed-off-by Alan Tull <[email protected]>---
v2: uselist_for_each_entry
static thebridge_list_lock
update copyright and author email
v3: no change to this patch in this version of patchset
v4: no change to this patch in this version of patchset--- drivers/fpga/fpga-bridge.c | 110 +++++++++++++++++++++++++++++++-------- drivers/fpga/fpga-region.c | 11 ++-- include/linux/fpga/fpga-bridge.h | 7 ++- 3 files changed, 100 insertions(+), 28 deletions(-)drivers/fpga/fpga-bridge.c +58
-21@@ -2,6 +2,7 @@* FPGA Bridge Framework Driver * * Copyright (C) 2013-2016 Altera Corporation, All Rights Reserved. +* Copyright (C) 2017 Intel Corporation * * This program is free software; you can redistribute it and/or modify it * under the terms and conditions of the GNU General Public License, @@ -70,29 +71,12 @@ int fpga_bridge_disable(struct fpga_bridge *bridge) }EXPORT_SYMBOL_GPL (fpga_bridge_disable); -/** -* of_fpga_bridge_get - get an exclusive reference to a fpga bridge -* -* @np: node pointer of a FPGA bridge -* @info: fpga image specific information -* -* Return fpga_bridge struct if successful. -* Return -EBUSY if someone already has a reference to the bridge. -* Return -ENODEV if @np is not a FPGA Bridge. -*/ -struct fpga_bridge *of_fpga_bridge_get (struct device_node *np, -struct fpga_image_info *info) - +struct fpga_bridge *__fpga_bridge_get (struct device *dev, +struct fpga_image_info *info) { -struct device *dev;struct fpga_bridge *bridge;int ret = -ENODEV ; - dev =class_find_device (fpga_bridge_class,NULL , np, - fpga_bridge_of_node_match); -if (!dev) - goto err_dev; - bridge =to_fpga_bridge (dev);if (!bridge) goto err_dev;@@ -117,8 +101,58 @@ struct fpga_bridge *of_fpga_bridge_get(struct device_node *np,put_device (dev);return ERR_PTR (ret); } + +/** +* of_fpga_bridge_get - get an exclusive reference to a fpga bridge +* +* @np: node pointer of a FPGA bridge +* @info: fpga image specific information +* +* Return fpga_bridge struct if successful. +* Return -EBUSY if someone already has a reference to the bridge. +* Return -ENODEV if @np is not a FPGA Bridge. +*/ +struct fpga_bridge *of_fpga_bridge_get (struct device_node *np, +struct fpga_image_info *info) +{ +struct device *dev; + + dev =class_find_device (fpga_bridge_class,NULL , np, + fpga_bridge_of_node_match); +if (!dev) +return ERR_PTR (-ENODEV ); + +return __fpga_bridge_get (dev, info); +}EXPORT_SYMBOL_GPL (of_fpga_bridge_get); +static int fpga_bridge_dev_match (struct device *dev,const void *data) +{ +return dev->parent == data; +} + +/** +* fpga_bridge_get - get an exclusive reference to a fpga bridge +* @dev: parent device that fpga bridge was registered with +* +* Given a device, get an exclusive reference to a fpga bridge. +* +* Return: fpga manager struct or IS_ERR() condition containing error code. +*/ +struct fpga_bridge *fpga_bridge_get (struct device *dev, +struct fpga_image_info *info) +{ +struct device *bridge_dev; + + bridge_dev =class_find_device (fpga_bridge_class,NULL , dev, + fpga_bridge_dev_match); +if (!bridge_dev) +return ERR_PTR (-ENODEV ); + +return __fpga_bridge_get (bridge_dev, info); +} +EXPORT_SYMBOL_GPL (fpga_bridge_get); +/** * fpga_bridge_put - release a reference to a bridge * @@ -206,7 +240,7 @@ void fpga_bridges_put(struct list_head *bridge_list)EXPORT_SYMBOL_GPL (fpga_bridges_put);/** -* fpga_bridges_get_to_list - get a bridge, add it to a list +* of_fpga_bridge_get_to_list - get a bridge, add it to a list * * @np: node pointer of a FPGA bridge * @info: fpga image specific information @@ -216,14 +250,44 @@ EXPORT_SYMBOL_GPL(fpga_bridges_put);* * Return 0 for success, error code from of_fpga_bridge_get() othewise. */ -int fpga_bridge_get_to_list(struct device_node *np, +int of_fpga_bridge_get_to_list (struct device_node *np, +struct fpga_image_info *info, +struct list_head *bridge_list) +{drivers/fpga/fpga-bridge.c EXPORT_SYMBOL_GPL(fpga_bridges_put); · line 266
Quoted 10 lines
drivers/fpga/fpga-bridge.c +10
-0+struct fpga_bridge *bridge; +unsigned long flags; + + bridge =of_fpga_bridge_get (np, info); +if (IS_ERR (bridge)) +return PTR_ERR (bridge); + +spin_lock_irqsave (&bridge_list_lock, flags); +list_add (&bridge->node, bridge_list); +spin_unlock_irqrestore (&bridge_list_lock, flags);I know this is not new code, but I was wondering the other day:
Why are we using a single spinlock to protect all lists that are being passed in as parameters here?
I have a patch that moves the spinlock into the region containing the list that uses the bridges which (unless I misunderstand something here), makes more sense:
drivers/fpga/fpga-bridge.c +2
-3@@ -214,6 +214,8 @@ EXPORT_SYMBOL_GPL(fpga_bridges_put);* * Get an exclusive reference to the bridge and and it to the list. * +* Must be called with list lock held. +* * Return 0 for success, error code from of_fpga_bridge_get() othewise. */ int fpga_bridge_get_to_list(struct device_node *np,@@ -221,15 +223,12 @@ int fpga_bridge_get_to_list(struct device_node *np, struct list_head *bridge_list) {struct fpga_bridge *bridge; -unsigned long flags; bridge =of_fpga_bridge_get (np, info);if (IS_ERR (bridge))return PTR_ERR (bridge); -spin_lock_irqsave (&bridge_list_lock, flags);list_add (&bridge->node, bridge_list); -spin_unlock_irqrestore (&bridge_list_lock, flags);return 0 ; }drivers/fpga/fpga-region.c +6
-0@@ -37,6 +37,7 @@ struct fpga_region {struct device dev;struct mutex mutex;/* for exclusive reference to region */ struct list_head bridge_list; +spinlock_t bridge_list_lock;/* protects access to bridge list */ struct fpga_image_info *info; };@@ -180,11 +181,15 @@ static int fpga_region_get_bridges(struct fpga_region *region,struct device *dev = ®ion->dev;struct device_node *region_np = dev->of_node;struct device_node *br, *np, *parent_br =NULL ; +unsigned long flags;int i, ret;/* If parent is a bridge, add to list */ +spin_lock_irqsave (®ion->bridge_list_lock, flags); ret =fpga_bridge_get_to_list (region_np->parent, region->info, ®ion->bridge_list); +spin_unlock_irqrestore (®ion->bridge_list_lock, flags); +if (ret == -EBUSY )return ret;@@ -508,6 +513,7 @@ static int fpga_region_probe(struct platform_device *pdev)mutex_init (®ion->mutex);INIT_LIST_HEAD (®ion->bridge_list); +spin_lock_init (®ion->bridge_list_lock);device_initialize (®ion->dev); region->dev.class = fpga_region_class;
Quoted 1 line
Am I missing something here? If not I"ll send out my patch separately.
Hi Moritz,
You are right! I'll look at your patch.
Alan
Quoted 104 lines
Quoted 99 lines
drivers/fpga/fpga-bridge.c +19
-2+ +return 0 ; +} +EXPORT_SYMBOL_GPL (of_fpga_bridge_get_to_list); + +/** +* fpga_bridge_get_to_list - given device, get a bridge, add it to a list +* +* @dev: FPGA bridge device +* @info: fpga image specific information +* @bridge_list: list of FPGA bridges +* +* Get an exclusive reference to the bridge and and it to the list. +* +* Return 0 for success, error code from fpga_bridge_get() othewise. +*/ +int fpga_bridge_get_to_list (struct device *dev,struct fpga_image_info *info,struct list_head *bridge_list) {struct fpga_bridge *bridge;unsigned long flags; - bridge =of_fpga_bridge_get (np, info); + bridge =fpga_bridge_get (dev, info);if (IS_ERR (bridge))return PTR_ERR (bridge);@@ -381,7 +445,7 @@ static void __exit fpga_bridge_dev_exit(void) }MODULE_DESCRIPTION ("FPGA Bridge Driver" ); -MODULE_AUTHOR ("Alan Tull <[email protected]>" ); +MODULE_AUTHOR ("Alan Tull <[email protected]>" );MODULE_LICENSE ("GPL v2" );subsys_initcall (fpga_bridge_dev_init);drivers/fpga/fpga-region.c +7
-4@@ -183,11 +183,14 @@ static int fpga_region_get_bridges(struct fpga_region *region,int i, ret;/* If parent is a bridge, add to list */ - ret =fpga_bridge_get_to_list (region_np->parent, region->info, - ®ion->bridge_list); + ret =of_fpga_bridge_get_to_list (region_np->parent, region->info, + ®ion->bridge_list); + +/* -EBUSY means parent is a bridge that is under use. Give up. */ if (ret == -EBUSY )return ret; +/* Zero return code means parent was a bridge and was added to list. */ if (!ret) parent_br = region_np->parent;@@ -207,8 +210,8 @@ static int fpga_region_get_bridges(struct fpga_region *region,continue ;/* If node is a bridge, get it and add to list */ - ret =fpga_bridge_get_to_list (br, region->info, - ®ion->bridge_list); + ret =of_fpga_bridge_get_to_list (br, region->info, + ®ion->bridge_list);/* If any of the bridges are in use, give up */ if (ret == -EBUSY ) {include/linux/fpga/fpga-bridge.h +6
-2@@ -42,6 +42,8 @@ struct fpga_bridge {struct fpga_bridge *of_fpga_bridge_get (struct device_node *node,struct fpga_image_info *info); +struct fpga_bridge *fpga_bridge_get (struct device *dev, +struct fpga_image_info *info);void fpga_bridge_put (struct fpga_bridge *bridge);int fpga_bridge_enable (struct fpga_bridge *bridge);int fpga_bridge_disable (struct fpga_bridge *bridge);@@ -49,9 +51,12 @@ int fpga_bridge_disable(struct fpga_bridge *bridge);int fpga_bridges_enable (struct list_head *bridge_list);int fpga_bridges_disable (struct list_head *bridge_list);void fpga_bridges_put (struct list_head *bridge_list); -int fpga_bridge_get_to_list (struct device_node *np, +int fpga_bridge_get_to_list (struct device *dev,struct fpga_image_info *info,struct list_head *bridge_list); +int of_fpga_bridge_get_to_list (struct device_node *np, +struct fpga_image_info *info, +struct list_head *bridge_list);int fpga_bridge_register (struct device *dev,const char *name,const struct fpga_bridge_ops *br_ops,void *priv); --Quoted 2 lines
2.7.4
Thanks,
Moritz
Moritz Fischer
maintainerRe: [PATCH v4 01/18] fpga: bridge: support getting bridge from device
In reply to Alan TullQuoted 118 lines · Alan Tull
Add two functions for getting the FPGA bridge from the device rather than device tree node. This is to enable writing code that will support using FPGA bridges without device tree. Rename one old function to make it clear that it is device tree-ish. This leaves us with 3 functions for getting a bridge:
*
fpga_bridge_get
Get the bridge given the device.*
fpga_bridges_get_to_list
Given the device, get the bridge and add it to a list.*
of_fpga_bridges_get_to_list
Renamed from priviously existingfpga_bridges_get_to_list. Given the device node, get the bridge and add it to a list.Signed-off-by Alan Tull <[email protected]>---
v2: uselist_for_each_entry
static thebridge_list_lock
update copyright and author email
v3: no change to this patch in this version of patchset
v4: no change to this patch in this version of patchset--- drivers/fpga/fpga-bridge.c | 110 +++++++++++++++++++++++++++++++-------- drivers/fpga/fpga-region.c | 11 ++-- include/linux/fpga/fpga-bridge.h | 7 ++- 3 files changed, 100 insertions(+), 28 deletions(-)drivers/fpga/fpga-bridge.c +42
-19@@ -2,6 +2,7 @@* FPGA Bridge Framework Driver * * Copyright (C) 2013-2016 Altera Corporation, All Rights Reserved. +* Copyright (C) 2017 Intel Corporation * * This program is free software; you can redistribute it and/or modify it * under the terms and conditions of the GNU General Public License, @@ -70,29 +71,12 @@ int fpga_bridge_disable(struct fpga_bridge *bridge) }EXPORT_SYMBOL_GPL (fpga_bridge_disable); -/** -* of_fpga_bridge_get - get an exclusive reference to a fpga bridge -* -* @np: node pointer of a FPGA bridge -* @info: fpga image specific information -* -* Return fpga_bridge struct if successful. -* Return -EBUSY if someone already has a reference to the bridge. -* Return -ENODEV if @np is not a FPGA Bridge. -*/ -struct fpga_bridge *of_fpga_bridge_get (struct device_node *np, -struct fpga_image_info *info) - +struct fpga_bridge *__fpga_bridge_get (struct device *dev, +struct fpga_image_info *info) { -struct device *dev;struct fpga_bridge *bridge;int ret = -ENODEV ; - dev =class_find_device (fpga_bridge_class,NULL , np, - fpga_bridge_of_node_match); -if (!dev) - goto err_dev; - bridge =to_fpga_bridge (dev);if (!bridge) goto err_dev;@@ -117,8 +101,58 @@ struct fpga_bridge *of_fpga_bridge_get(struct device_node *np,put_device (dev);return ERR_PTR (ret); } + +/** +* of_fpga_bridge_get - get an exclusive reference to a fpga bridge +* +* @np: node pointer of a FPGA bridge +* @info: fpga image specific information +* +* Return fpga_bridge struct if successful. +* Return -EBUSY if someone already has a reference to the bridge. +* Return -ENODEV if @np is not a FPGA Bridge. +*/ +struct fpga_bridge *of_fpga_bridge_get (struct device_node *np, +struct fpga_image_info *info) +{ +struct device *dev; + + dev =class_find_device (fpga_bridge_class,NULL , np, + fpga_bridge_of_node_match); +if (!dev) +return ERR_PTR (-ENODEV ); + +return __fpga_bridge_get (dev, info); +}EXPORT_SYMBOL_GPL (of_fpga_bridge_get); +static int fpga_bridge_dev_match (struct device *dev,const void *data) +{ +return dev->parent == data; +} + +/** +* fpga_bridge_get - get an exclusive reference to a fpga bridge +* @dev: parent device that fpga bridge was registered with +* +* Given a device, get an exclusive reference to a fpga bridge. +* +* Return: fpga manager struct or IS_ERR() condition containing error code. +*/ +struct fpga_bridge *fpga_bridge_get (struct device *dev, +struct fpga_image_info *info) +{
drivers/fpga/fpga-bridge.c struct fpga_bridge *of_fpga_bridge_get(struct device_node *np, · line 154
Quoted 10 lines
drivers/fpga/fpga-bridge.c +10
-0+struct device *bridge_dev; + + bridge_dev =class_find_device (fpga_bridge_class,NULL , dev, + fpga_bridge_dev_match); +if (!bridge_dev) +return ERR_PTR (-ENODEV ); + +return __fpga_bridge_get (bridge_dev, info); +} +EXPORT_SYMBOL_GPL (fpga_bridge_get);
Do we really need two functions here? Can't we just do a:
if (dev->of_node) dev = class_find_device(fpga_bridge_class, NULL, np, fpga_bridge_of_node_match); else dev = class_find_device(fpga_bridge_class, NULL, dev, fpga_bridge_dev_match);
instead? Maybe you could even move the check into the match function, so you can reuse all the code here?
Quoted 131 lines
drivers/fpga/fpga-bridge.c +35
-4+/** * fpga_bridge_put - release a reference to a bridge * @@ -206,7 +240,7 @@ void fpga_bridges_put(struct list_head *bridge_list)EXPORT_SYMBOL_GPL (fpga_bridges_put);/** -* fpga_bridges_get_to_list - get a bridge, add it to a list +* of_fpga_bridge_get_to_list - get a bridge, add it to a list * * @np: node pointer of a FPGA bridge * @info: fpga image specific information @@ -216,14 +250,44 @@ EXPORT_SYMBOL_GPL(fpga_bridges_put);* * Return 0 for success, error code from of_fpga_bridge_get() othewise. */ -int fpga_bridge_get_to_list (struct device_node *np, +int of_fpga_bridge_get_to_list (struct device_node *np, +struct fpga_image_info *info, +struct list_head *bridge_list) +{ +struct fpga_bridge *bridge; +unsigned long flags; + + bridge =of_fpga_bridge_get (np, info); +if (IS_ERR (bridge)) +return PTR_ERR (bridge); + +spin_lock_irqsave (&bridge_list_lock, flags); +list_add (&bridge->node, bridge_list); +spin_unlock_irqrestore (&bridge_list_lock, flags); + +return 0 ; +} +EXPORT_SYMBOL_GPL (of_fpga_bridge_get_to_list); + +/** +* fpga_bridge_get_to_list - given device, get a bridge, add it to a list +* +* @dev: FPGA bridge device +* @info: fpga image specific information +* @bridge_list: list of FPGA bridges +* +* Get an exclusive reference to the bridge and and it to the list. +* +* Return 0 for success, error code from fpga_bridge_get() othewise. +*/ +int fpga_bridge_get_to_list (struct device *dev,struct fpga_image_info *info,struct list_head *bridge_list) {struct fpga_bridge *bridge;unsigned long flags; - bridge =of_fpga_bridge_get (np, info); + bridge =fpga_bridge_get (dev, info);if (IS_ERR (bridge))return PTR_ERR (bridge);@@ -381,7 +445,7 @@ static void __exit fpga_bridge_dev_exit(void) }MODULE_DESCRIPTION ("FPGA Bridge Driver" ); -MODULE_AUTHOR ("Alan Tull <[email protected]>" ); +MODULE_AUTHOR ("Alan Tull <[email protected]>" );MODULE_LICENSE ("GPL v2" );subsys_initcall (fpga_bridge_dev_init);drivers/fpga/fpga-region.c +7
-4@@ -183,11 +183,14 @@ static int fpga_region_get_bridges(struct fpga_region *region,int i, ret;/* If parent is a bridge, add to list */ - ret =fpga_bridge_get_to_list (region_np->parent, region->info, - ®ion->bridge_list); + ret =of_fpga_bridge_get_to_list (region_np->parent, region->info, + ®ion->bridge_list); + +/* -EBUSY means parent is a bridge that is under use. Give up. */ if (ret == -EBUSY )return ret; +/* Zero return code means parent was a bridge and was added to list. */ if (!ret) parent_br = region_np->parent;@@ -207,8 +210,8 @@ static int fpga_region_get_bridges(struct fpga_region *region,continue ;/* If node is a bridge, get it and add to list */ - ret =fpga_bridge_get_to_list (br, region->info, - ®ion->bridge_list); + ret =of_fpga_bridge_get_to_list (br, region->info, + ®ion->bridge_list);/* If any of the bridges are in use, give up */ if (ret == -EBUSY ) {include/linux/fpga/fpga-bridge.h +6
-1@@ -42,6 +42,8 @@ struct fpga_bridge {struct fpga_bridge *of_fpga_bridge_get (struct device_node *node,struct fpga_image_info *info); +struct fpga_bridge *fpga_bridge_get (struct device *dev, +struct fpga_image_info *info);void fpga_bridge_put (struct fpga_bridge *bridge);int fpga_bridge_enable (struct fpga_bridge *bridge);int fpga_bridge_disable (struct fpga_bridge *bridge);@@ -49,9 +51,12 @@ int fpga_bridge_disable(struct fpga_bridge *bridge);int fpga_bridges_enable (struct list_head *bridge_list);int fpga_bridges_disable (struct list_head *bridge_list);void fpga_bridges_put (struct list_head *bridge_list); -int fpga_bridge_get_to_list (struct device_node *np, +int fpga_bridge_get_to_list (struct device *dev,struct fpga_image_info *info,struct list_head *bridge_list); +int of_fpga_bridge_get_to_list (struct device_node *np, +struct fpga_image_info *info, +struct list_head *bridge_list);int fpga_bridge_register (struct device *dev,const char *name,const struct fpga_bridge_ops *br_ops,void *priv);Signature · 1 line
2.7.4
Re: [PATCH v4 01/18] fpga: bridge: support getting bridge from device
On Mon, Sep 18, 2017 at 5:53 PM, Moritz Fischer <[email protected]> wrote:
Hi Moritz,
Quoted 132 lines · Alan Tull
Quoted 118 lines
Add two functions for getting the FPGA bridge from the device rather than device tree node. This is to enable writing code that will support using FPGA bridges without device tree. Rename one old function to make it clear that it is device tree-ish. This leaves us with 3 functions for getting a bridge:
*
fpga_bridge_get
Get the bridge given the device.*
fpga_bridges_get_to_list
Given the device, get the bridge and add it to a list.*
of_fpga_bridges_get_to_list
Renamed from priviously existingfpga_bridges_get_to_list. Given the device node, get the bridge and add it to a list.Signed-off-by Alan Tull <[email protected]>---
v2: uselist_for_each_entry
static thebridge_list_lock
update copyright and author email
v3: no change to this patch in this version of patchset
v4: no change to this patch in this version of patchset--- drivers/fpga/fpga-bridge.c | 110 +++++++++++++++++++++++++++++++-------- drivers/fpga/fpga-region.c | 11 ++-- include/linux/fpga/fpga-bridge.h | 7 ++- 3 files changed, 100 insertions(+), 28 deletions(-)drivers/fpga/fpga-bridge.c +42
-19@@ -2,6 +2,7 @@* FPGA Bridge Framework Driver * * Copyright (C) 2013-2016 Altera Corporation, All Rights Reserved. +* Copyright (C) 2017 Intel Corporation * * This program is free software; you can redistribute it and/or modify it * under the terms and conditions of the GNU General Public License, @@ -70,29 +71,12 @@ int fpga_bridge_disable(struct fpga_bridge *bridge) }EXPORT_SYMBOL_GPL (fpga_bridge_disable); -/** -* of_fpga_bridge_get - get an exclusive reference to a fpga bridge -* -* @np: node pointer of a FPGA bridge -* @info: fpga image specific information -* -* Return fpga_bridge struct if successful. -* Return -EBUSY if someone already has a reference to the bridge. -* Return -ENODEV if @np is not a FPGA Bridge. -*/ -struct fpga_bridge *of_fpga_bridge_get (struct device_node *np, -struct fpga_image_info *info) - +struct fpga_bridge *__fpga_bridge_get (struct device *dev, +struct fpga_image_info *info) { -struct device *dev;struct fpga_bridge *bridge;int ret = -ENODEV ; - dev =class_find_device (fpga_bridge_class,NULL , np, - fpga_bridge_of_node_match); -if (!dev) - goto err_dev; - bridge =to_fpga_bridge (dev);if (!bridge) goto err_dev;@@ -117,8 +101,58 @@ struct fpga_bridge *of_fpga_bridge_get(struct device_node *np,put_device (dev);return ERR_PTR (ret); } + +/** +* of_fpga_bridge_get - get an exclusive reference to a fpga bridge +* +* @np: node pointer of a FPGA bridge +* @info: fpga image specific information +* +* Return fpga_bridge struct if successful. +* Return -EBUSY if someone already has a reference to the bridge. +* Return -ENODEV if @np is not a FPGA Bridge. +*/ +struct fpga_bridge *of_fpga_bridge_get (struct device_node *np, +struct fpga_image_info *info) +{ +struct device *dev; + + dev =class_find_device (fpga_bridge_class,NULL , np, + fpga_bridge_of_node_match); +if (!dev) +return ERR_PTR (-ENODEV ); + +return __fpga_bridge_get (dev, info); +}EXPORT_SYMBOL_GPL (of_fpga_bridge_get); +static int fpga_bridge_dev_match (struct device *dev,const void *data) +{ +return dev->parent == data; +} + +/** +* fpga_bridge_get - get an exclusive reference to a fpga bridge +* @dev: parent device that fpga bridge was registered with +* +* Given a device, get an exclusive reference to a fpga bridge. +* +* Return: fpga manager struct or IS_ERR() condition containing error code. +*/ +struct fpga_bridge *fpga_bridge_get (struct device *dev, +struct fpga_image_info *info) +{drivers/fpga/fpga-bridge.c struct fpga_bridge *of_fpga_bridge_get(struct device_node *np, · line 154
Quoted 10 lines
drivers/fpga/fpga-bridge.c +10
-0+struct device *bridge_dev; + + bridge_dev =class_find_device (fpga_bridge_class,NULL , dev, + fpga_bridge_dev_match); +if (!bridge_dev) +return ERR_PTR (-ENODEV ); + +return __fpga_bridge_get (bridge_dev, info); +} +EXPORT_SYMBOL_GPL (fpga_bridge_get);Do we really need two functions here? Can't we just do a:
Quoted 1 line
if (dev->of_node)
If we have the device, we might as well find the bridge using the device. But if we have the of node, but not the device, we need some way to get to the device from the of node (can't use container_of). Such as in the loop in fpga_region_get_bridges() which is going through the device tree.
Quoted 8 lines
dev = class_find_device(fpga_bridge_class, NULL, np,
fpga_bridge_of_node_match);
else
dev = class_find_device(fpga_bridge_class, NULL, dev,
fpga_bridge_dev_match);instead? Maybe you could even move the check into the match function, so you can reuse all the code here?
But I agree it would be nice to have one function and if we can make it work, we should. We could have one function that takes both a device node and a device. If the device is non-null, look it up, else if the device node is non-node, look it up and return. If that doesn't seem ugly and ungainly.
Alan
Quoted 132 lines
Quoted 132 lines
drivers/fpga/fpga-bridge.c +35
-4+/** * fpga_bridge_put - release a reference to a bridge * @@ -206,7 +240,7 @@ void fpga_bridges_put(struct list_head *bridge_list)EXPORT_SYMBOL_GPL (fpga_bridges_put);/** -* fpga_bridges_get_to_list - get a bridge, add it to a list +* of_fpga_bridge_get_to_list - get a bridge, add it to a list * * @np: node pointer of a FPGA bridge * @info: fpga image specific information @@ -216,14 +250,44 @@ EXPORT_SYMBOL_GPL(fpga_bridges_put);* * Return 0 for success, error code from of_fpga_bridge_get() othewise. */ -int fpga_bridge_get_to_list (struct device_node *np, +int of_fpga_bridge_get_to_list (struct device_node *np, +struct fpga_image_info *info, +struct list_head *bridge_list) +{ +struct fpga_bridge *bridge; +unsigned long flags; + + bridge =of_fpga_bridge_get (np, info); +if (IS_ERR (bridge)) +return PTR_ERR (bridge); + +spin_lock_irqsave (&bridge_list_lock, flags); +list_add (&bridge->node, bridge_list); +spin_unlock_irqrestore (&bridge_list_lock, flags); + +return 0 ; +} +EXPORT_SYMBOL_GPL (of_fpga_bridge_get_to_list); + +/** +* fpga_bridge_get_to_list - given device, get a bridge, add it to a list +* +* @dev: FPGA bridge device +* @info: fpga image specific information +* @bridge_list: list of FPGA bridges +* +* Get an exclusive reference to the bridge and and it to the list. +* +* Return 0 for success, error code from fpga_bridge_get() othewise. +*/ +int fpga_bridge_get_to_list (struct device *dev,struct fpga_image_info *info,struct list_head *bridge_list) {struct fpga_bridge *bridge;unsigned long flags; - bridge =of_fpga_bridge_get (np, info); + bridge =fpga_bridge_get (dev, info);if (IS_ERR (bridge))return PTR_ERR (bridge);@@ -381,7 +445,7 @@ static void __exit fpga_bridge_dev_exit(void) }MODULE_DESCRIPTION ("FPGA Bridge Driver" ); -MODULE_AUTHOR ("Alan Tull <[email protected]>" ); +MODULE_AUTHOR ("Alan Tull <[email protected]>" );MODULE_LICENSE ("GPL v2" );subsys_initcall (fpga_bridge_dev_init);drivers/fpga/fpga-region.c +7
-4@@ -183,11 +183,14 @@ static int fpga_region_get_bridges(struct fpga_region *region,int i, ret;/* If parent is a bridge, add to list */ - ret =fpga_bridge_get_to_list (region_np->parent, region->info, - ®ion->bridge_list); + ret =of_fpga_bridge_get_to_list (region_np->parent, region->info, + ®ion->bridge_list); + +/* -EBUSY means parent is a bridge that is under use. Give up. */ if (ret == -EBUSY )return ret; +/* Zero return code means parent was a bridge and was added to list. */ if (!ret) parent_br = region_np->parent;@@ -207,8 +210,8 @@ static int fpga_region_get_bridges(struct fpga_region *region,continue ;/* If node is a bridge, get it and add to list */ - ret =fpga_bridge_get_to_list (br, region->info, - ®ion->bridge_list); + ret =of_fpga_bridge_get_to_list (br, region->info, + ®ion->bridge_list);/* If any of the bridges are in use, give up */ if (ret == -EBUSY ) {include/linux/fpga/fpga-bridge.h +6
-2@@ -42,6 +42,8 @@ struct fpga_bridge {struct fpga_bridge *of_fpga_bridge_get (struct device_node *node,struct fpga_image_info *info); +struct fpga_bridge *fpga_bridge_get (struct device *dev, +struct fpga_image_info *info);void fpga_bridge_put (struct fpga_bridge *bridge);int fpga_bridge_enable (struct fpga_bridge *bridge);int fpga_bridge_disable (struct fpga_bridge *bridge);@@ -49,9 +51,12 @@ int fpga_bridge_disable(struct fpga_bridge *bridge);int fpga_bridges_enable (struct list_head *bridge_list);int fpga_bridges_disable (struct list_head *bridge_list);void fpga_bridges_put (struct list_head *bridge_list); -int fpga_bridge_get_to_list (struct device_node *np, +int fpga_bridge_get_to_list (struct device *dev,struct fpga_image_info *info,struct list_head *bridge_list); +int of_fpga_bridge_get_to_list (struct device_node *np, +struct fpga_image_info *info, +struct list_head *bridge_list);int fpga_bridge_register (struct device *dev,const char *name,const struct fpga_bridge_ops *br_ops,void *priv); --2.7.4
PATCH v4 2/18 fpga: mgr: API change to replace fpga load functions with single function
In reply to Alan Tullfpga-mgr has three methods for programming FPGAs, depending on whether the image is in a scatter gather list, a contiguous buffer, or a firmware file. This makes it difficult to write upper layers as the caller has to assume whether the FPGA image is in a sg table, as a single buffer, or a firmware file. This commit moves these parameters to struct fpga_image_info and adds a single function for programming fpgas.
New functions:
* fpga_mgr_load - given fpga manager and struct fpga_image_info, program the fpga.
* fpga_image_info_alloc - alloc a struct fpga_image_info.
* fpga_image_info_free - free a struct fpga_image_info.
These three functions are unexported:
* fpga_mgr_buf_load_sg
* fpga_mgr_buf_load
* fpga_mgr_firmware_load
Also use devm_kstrdup to copy firmware_name so we aren't making assumptions about where it comes from when allocing/freeing the struct fpga_image_info.
API documentation has been updated and a new document for FPGA region has been added.
Signed-off-by Alan Tull <[email protected]>
---
v2: add fpga_image_info_alloc/free
update copyright and author email
v3: fix bisectibility
v4: fix return value of fpga_image_info_alloc()
save device in struct fpga_image_info
remove device param from fpga_image_info_free() squash in the documentation patch minus stuff about locking which is included in the next patch
--- Documentation/fpga/fpga-mgr.txt | 119 ++++++++++++++++--------------------- Documentation/fpga/fpga-region.txt | 95 +++++++++++++++++++++++++++++ Documentation/fpga/overview.txt | 23 +++++++ drivers/fpga/fpga-mgr.c | 68 +++++++++++++++++---- drivers/fpga/fpga-region.c | 43 +++++++++----- include/linux/fpga/fpga-mgr.h | 24 +++++--- 6 files changed, 270 insertions(+), 102 deletions(-) create mode 100644 Documentation/fpga/fpga-region.txt create mode 100644 Documentation/fpga/overview.txt
Documentation/fpga/fpga-mgr.txt +51 -68
@@ -11,61 +11,53 @@ hidden away in a low level driver which registers a set of ops with the core. The FPGA image data itself is very manufacturer specific, but for our purposes it's just binary data. The FPGA manager core won't parse it. +The FPGA image to be programmed can be in a scatter gather list, a single +contiguous buffer, or a firmware file. Because allocating contiguous kernel +memory for the buffer should be avoided, users are encouraged to use a scatter +gather list instead if possible. + +The particulars for programming the image are presented in a structure (struct +fpga_image_info). This struct contains parameters such as pointers to the +FPGA image as well as image-specific particulars such as whether the image was +built for full or partial reconfiguration. API Functions: ============== -To program the FPGA from a file or from a buffer: -------------------------------------------------- - - int fpga_mgr_buf_load(struct fpga_manager *mgr, - struct fpga_image_info *info, - const char *buf, size_t count); - -Load the FPGA from an image which exists as a contiguous buffer in -memory. Allocating contiguous kernel memory for the buffer should be avoided, -users are encouraged to use the _sg interface instead of this. - - int fpga_mgr_buf_load_sg(struct fpga_manager *mgr, - struct fpga_image_info *info, - struct sg_table *sgt); +To program the FPGA: +-------------------- -Load the FPGA from an image from non-contiguous in memory. Callers can -construct a sg_table using alloc_page backed memory. + int fpga_mgr_load(struct fpga_manager *mgr, + struct fpga_image_info *info); - int fpga_mgr_firmware_load(struct fpga_manager *mgr, - struct fpga_image_info *info, - const char *image_name); - -Load the FPGA from an image which exists as a file. The image file must be on -the firmware search path (see the firmware class documentation). If successful, +Load the FPGA from an image which is indicated in the info. If successful, the FPGA ends up in operating mode. Return 0 on success or a negative error code. -A FPGA design contained in a FPGA image file will likely have particulars that -affect how the image is programmed to the FPGA. These are contained in struct -fpga_image_info. Currently the only such particular is a single flag bit -indicating whether the image is for full or partial reconfiguration. +To allocate or free a struct fpga_image_info: +--------------------------------------------- + + struct fpga_image_info *fpga_image_info_alloc(struct device *dev); + + void fpga_image_info_free(struct fpga_image_info *info); To get/put a reference to a FPGA manager: ----------------------------------------- struct fpga_manager *of_fpga_mgr_get(struct device_node *node); struct fpga_manager *fpga_mgr_get(struct device *dev); - -Given a DT node or device, get an exclusive reference to a FPGA manager. - void fpga_mgr_put(struct fpga_manager *mgr); -Release the reference. +Given a DT node or device, get an exclusive reference to a FPGA manager. +fpga_mgr_put releases the reference. To register or unregister the low level FPGA-specific driver: ------------------------------------------------------------- int fpga_mgr_register(struct device *dev, const char *name, - const struct fpga_manager_ops *mops, - void *priv); + const struct fpga_manager_ops *mops, + void *priv); void fpga_mgr_unregister(struct device *dev);
@@ -78,59 +70,50 @@ How to write an image buffer to a supported FPGA /* Include to get the API */ #include <linux/fpga/fpga-mgr.h> -/* device node that specifies the FPGA manager to use */ -struct device_node *mgr_node = ... - -/* FPGA image is in this buffer. count is size of the buffer. */ -char *buf = ... -int count = ... +struct fpga_manager *mgr; +struct fpga_image_info *info; +int ret; /* struct with information about the FPGA image to program. */ -struct fpga_image_info info; +info = fpga_image_info_alloc(dev); /* flags indicates whether to do full or partial reconfiguration */ -info.flags = 0; - -int ret; +info->flags = FPGA_MGR_PARTIAL_RECONFIG; -/* Get exclusive control of FPGA manager */ -struct fpga_manager *mgr = of_fpga_mgr_get(mgr_node); +/* + * At this point, indicate where the image is. This is pseudo-code; you're + * going to use one of these three. + */ +if (image is in a scatter gather table) { -/* Load the buffer to the FPGA */ -ret = fpga_mgr_buf_load(mgr, &info, buf, count); - -/* Release the FPGA manager */ -fpga_mgr_put(mgr); + info->sgt = [your scatter gather table] +} else if (image is in a buffer) { -How to write an image file to a supported FPGA -============================================== -/* Include to get the API */ -#include <linux/fpga/fpga-mgr.h> + info->buf = [your image buffer] + info->count = [image buffer size] -/* device node that specifies the FPGA manager to use */ -struct device_node *mgr_node = ... +} else if (image is in a firmware file) { -/* FPGA image is in this file which is in the firmware search path */ -const char *path = "fpga-image-9.rbf" + info->firmware_name = devm_kstrdup(dev, firmware_name, GFP_KERNEL); -/* struct with information about the FPGA image to program. */ -struct fpga_image_info info; - -/* flags indicates whether to do full or partial reconfiguration */ -info.flags = 0; - -int ret; +} -/* Get exclusive control of FPGA manager */ -struct fpga_manager *mgr = of_fpga_mgr_get(mgr_node); +/* + * Get a reference to FPGA manager. This example uses the device node of the + * manager. You could use fpga_mgr_get() instead if you have the device instead + * of the device node. + */ +mgr = of_fpga_mgr_get(mgr_node); -/* Get the firmware image (path) and load it to the FPGA */ -ret = fpga_mgr_firmware_load(mgr, &info, path); +/* Load the buffer to the FPGA */ +ret = fpga_mgr_buf_load(mgr, &info, buf, count); /* Release the FPGA manager */ fpga_mgr_put(mgr); +/* Deallocate the image info if you're done with it */ +fpga_image_info_free(info); How to support a new FPGA device ================================
Documentation/fpga/fpga-region.txt new file +95 -0
95 lines not on this page · raw message
Documentation/fpga/overview.txt new file +23 -0
@@ -0,0 +1,23 @@ +Linux kernel FPGA support + +Alan Tull 2017 + +The main point of this project has been to separate the out the upper layers +that know when to reprogram a FPGA from the lower layers that know how to +reprogram a specific FPGA device. The intention is to make this manufacturer +agnostic, understanding that of course the FPGA images are very device specific +themselves. + +The framework in the kernel includes: +* low level FPGA manager drivers that know how to program a specific device +* the fpga-mgr framework they are registered with +* low level FPGA bridge drivers for hard/soft bridges which are intended to + be disable during FPGA programming +* the fpga-bridge framework they are registered with +* the fpga-region framework which associates and controls managers and bridges + as reconfigurable regions +* the of-fpga-region support for reprogramming FPGAs when device tree overlays + are applied. + +I would encourage you the user to add code that creates FPGA regions rather +that trying to control managers and bridges separately.
drivers/fpga/fpga-mgr.c +57 -11
122 lines not on this page · raw message
drivers/fpga/fpga-region.c +28 -15
101 lines not on this page · raw message
include/linux/fpga/fpga-mgr.h +16 -8
@@ -1,7 +1,8 @@/* * FPGA Framework * -* Copyright (C) 2013-2015 Altera Corporation +* Copyright (C) 2013-2016 Altera Corporation +* Copyright (C) 2017 Intel Corporation * * This program is free software; you can redistribute it and/or modify it * under the terms and conditions of the GNU General Public License,
@@ -83,12 +84,22 @@ enum fpga_mgr_states {* @disable_timeout_us: maximum time to disable traffic through bridge (uSec) * @config_complete_timeout_us: maximum time for FPGA to switch to operating * status in the write_complete op. +* @firmware_name: name of FPGA image firmware file +* @sgt: scatter/gather table containing FPGA image +* @buf: contiguous buffer containing FPGA image +* @count: size of buf +* @dev: device that owns this */ struct fpga_image_info {u32 flags;u32 enable_timeout_us;u32 disable_timeout_us;u32 config_complete_timeout_us; +char *firmware_name; +struct sg_table *sgt; +const char *buf; +size_t count; +struct device *dev; };/**
@@ -138,14 +149,11 @@ struct fpga_manager {#define to_fpga_manager (d) container_of(d, struct fpga_manager, dev) -int fpga_mgr_buf_load (struct fpga_manager *mgr,struct fpga_image_info *info, -const char *buf,size_t count); -int fpga_mgr_buf_load_sg (struct fpga_manager *mgr,struct fpga_image_info *info, -struct sg_table *sgt); +struct fpga_image_info *fpga_image_info_alloc (struct device *dev); -int fpga_mgr_firmware_load (struct fpga_manager *mgr, -struct fpga_image_info *info, -const char *image_name); +void fpga_image_info_free (struct fpga_image_info *info); + +int fpga_mgr_load (struct fpga_manager *mgr,struct fpga_image_info *info);struct fpga_manager *of_fpga_mgr_get (struct device_node *node);
Signature · 1 line
2.7.4
PATCH v4 3/18 fpga: mgr: separate getting/locking FPGA manager
In reply to Alan TullPreviously when the user gets a FPGA manager, it was locked and nobody else could use it for programming.
This commit makes it straightforward to save a reference to an FPGA manager and only lock it when programming the FPGA.
Add functions that get an FPGA manager's mutex for exclusive use:
* fpga_mgr_lock
* fpga_mgr_unlock
The following functions no longer lock an FPGA manager's mutex:
* of_fpga_mgr_get
* fpga_mgr_get
* fpga_mgr_put
Signed-off-by Alan Tull <[email protected]>
---
v2: add static for __fpga_mgr_get
function documentation corrections
use dev_err
v3: bisectibility fix
v4: add API doc updates that were originally in a separate patch
--- Documentation/fpga/fpga-mgr.txt | 35 ++++++++++++++++++++------- drivers/fpga/fpga-mgr.c | 52 ++++++++++++++++++++++++++++------------- drivers/fpga/fpga-region.c | 14 +++++++++-- include/linux/fpga/fpga-mgr.h | 3 +++ 4 files changed, 77 insertions(+), 27 deletions(-)
Documentation/fpga/fpga-mgr.txt +26 -9
@@ -48,8 +48,20 @@ To get/put a reference to a FPGA manager: struct fpga_manager *fpga_mgr_get(struct device *dev); void fpga_mgr_put(struct fpga_manager *mgr); -Given a DT node or device, get an exclusive reference to a FPGA manager. -fpga_mgr_put releases the reference. +Given a DT node or device, get a reference to a FPGA manager. This pointer +can be saved until you are ready to program the FPGA. fpga_mgr_put releases +the reference. + + +To get exclusive control of a FPGA manager: +------------------------------------------- + + int fpga_mgr_lock(struct fpga_manager *mgr); + void fpga_mgr_unlock(struct fpga_manager *mgr); + +The user should call fpga_mgr_lock and verify that it returns 0 before +attempting to program the FPGA. Likewise, the user should call +fpga_mgr_unlock when done programming the FPGA. To register or unregister the low level FPGA-specific driver:
@@ -67,13 +79,21 @@ device." How to write an image buffer to a supported FPGA ================================================ -/* Include to get the API */ #include <linux/fpga/fpga-mgr.h> struct fpga_manager *mgr; struct fpga_image_info *info; int ret; +/* + * Get a reference to FPGA manager. The manager is not locked, so you can + * hold onto this reference without it preventing programming. + * + * This example uses the device node of the manager. Alternatively, use + * fpga_mgr_get(dev) instead if you have the device. + */ +mgr = of_fpga_mgr_get(mgr_node); + /* struct with information about the FPGA image to program. */ info = fpga_image_info_alloc(dev);
@@ -99,17 +119,14 @@ if (image is in a scatter gather table) { } -/* - * Get a reference to FPGA manager. This example uses the device node of the - * manager. You could use fpga_mgr_get() instead if you have the device instead - * of the device node. - */ -mgr = of_fpga_mgr_get(mgr_node); +/* Get exclusive control of FPGA manager */ +ret = fpga_mgr_lock(mgr); /* Load the buffer to the FPGA */ ret = fpga_mgr_buf_load(mgr, &info, buf, count); /* Release the FPGA manager */ +fpga_mgr_unlock(mgr); fpga_mgr_put(mgr); /* Deallocate the image info if you're done with it */
drivers/fpga/fpga-mgr.c +36 -16
96 lines not on this page · raw message
drivers/fpga/fpga-region.c +12 -2
@@ -125,7 +125,7 @@ static void fpga_region_put(struct fpga_region *region) }/** -* fpga_region_get_manager - get exclusive reference for FPGA manager +* fpga_region_get_manager - get reference for FPGA manager * @region: FPGA region * * Get FPGA Manager from "fpga-mgr" property or from ancestor region.
@@ -233,6 +233,7 @@ static int fpga_region_get_bridges(struct fpga_region *region,static int fpga_region_program_fpga (struct fpga_region *region,struct device_node *overlay) { +struct device *dev = ®ion->dev;struct fpga_manager *mgr;int ret;
@@ -249,10 +250,16 @@ static int fpga_region_program_fpga(struct fpga_region *region, goto err_put_region; } + ret =fpga_mgr_lock (mgr); +if (ret) { +dev_err (dev,"FPGA manager is busy\n" ); + goto err_put_mgr; + } + ret =fpga_region_get_bridges (region, overlay);if (ret) {pr_err ("failed to get fpga region bridges\n" ); - goto err_put_mgr; + goto err_unlock_mgr; } ret =fpga_bridges_disable (®ion->bridge_list);
@@ -273,6 +280,7 @@ static int fpga_region_program_fpga(struct fpga_region *region, goto err_put_br; } +fpga_mgr_unlock (mgr);fpga_mgr_put (mgr);fpga_region_put (region);
@@ -280,6 +288,8 @@ static int fpga_region_program_fpga(struct fpga_region *region, err_put_br:fpga_bridges_put (®ion->bridge_list); +err_unlock_mgr: +fpga_mgr_unlock (mgr); err_put_mgr:fpga_mgr_put (mgr); err_put_region:
include/linux/fpga/fpga-mgr.h +3 -0
@@ -155,6 +155,9 @@ void fpga_image_info_free(struct fpga_image_info *info);int fpga_mgr_load (struct fpga_manager *mgr,struct fpga_image_info *info); +int fpga_mgr_lock (struct fpga_manager *mgr); +void fpga_mgr_unlock (struct fpga_manager *mgr); +struct fpga_manager *of_fpga_mgr_get (struct device_node *node);struct fpga_manager *fpga_mgr_get (struct device *dev);
Signature · 1 line
2.7.4
PATCH v4 4/18 fpga: region: use dev_err instead of pr_err
In reply to Alan TullUse dev_err messages instead of pr_err.
Also s/®ion->dev/dev/ in two places where we already have dev = ®ion->dev.
Signed-off-by Alan Tull <[email protected]>
---
v2: new in this version of the patchset
v3: for bisectability some changes moved to earlier patches
v4: no change to this patch in this version of patchset
--- drivers/fpga/fpga-region.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)
drivers/fpga/fpga-region.c +10 -10
@@ -102,7 +102,7 @@ static struct fpga_region *fpga_region_get(struct fpga_region *region)return ERR_PTR (-ENODEV ); } -dev_dbg (®ion->dev,"get\n" ); +dev_dbg (dev,"get\n" );return region; }
@@ -116,7 +116,7 @@ static void fpga_region_put(struct fpga_region *region) {struct device *dev = ®ion->dev; -dev_dbg (®ion->dev,"put\n" ); +dev_dbg (dev,"put\n" );module_put (dev->parent->driver->owner);of_node_put (dev->of_node);
@@ -239,13 +239,13 @@ static int fpga_region_program_fpga(struct fpga_region *region, region =fpga_region_get (region);if (IS_ERR (region)) { -pr_err ("failed to get fpga region\n" ); +dev_err (dev,"failed to get FPGA region\n" );return PTR_ERR (region); } mgr =fpga_region_get_manager (region);if (IS_ERR (mgr)) { -pr_err ("failed to get fpga region manager\n" ); +dev_err (dev,"failed to get FPGA manager\n" ); ret =PTR_ERR (mgr); goto err_put_region; }
@@ -258,25 +258,25 @@ static int fpga_region_program_fpga(struct fpga_region *region, ret =fpga_region_get_bridges (region, overlay);if (ret) { -pr_err ("failed to get fpga region bridges\n" ); +dev_err (dev,"failed to get FPGA bridges\n" ); goto err_unlock_mgr; } ret =fpga_bridges_disable (®ion->bridge_list);if (ret) { -pr_err ("failed to disable region bridges\n" ); +dev_err (dev,"failed to disable bridges\n" ); goto err_put_br; } ret =fpga_mgr_load (mgr, region->info);if (ret) { -pr_err ("failed to load fpga image\n" ); +dev_err (dev,"failed to load FPGA image\n" ); goto err_put_br; } ret =fpga_bridges_enable (®ion->bridge_list);if (ret) { -pr_err ("failed to enable region bridges\n" ); +dev_err (dev,"failed to enable region bridges\n" ); goto err_put_br; }
@@ -407,7 +407,7 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,/* If FPGA was externally programmed, don't specify firmware */ if ((info->flags &FPGA_MGR_EXTERNAL_CONFIG ) && info->firmware_name) { -pr_err ("error: specified firmware and external-fpga-config" ); +dev_err (dev,"error: specified firmware and external-fpga-config" );fpga_image_info_free (info);return -EINVAL ; }
@@ -420,7 +420,7 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,/* If we got this far, we should be programming the FPGA */ if (!info->firmware_name) { -pr_err ("should specify firmware-name or external-fpga-config\n" ); +dev_err (dev,"should specify firmware-name or external-fpga-config\n" );fpga_image_info_free (info);return -EINVAL ; }
Signature · 1 line
2.7.4
Re: [PATCH v4 04/18] fpga: region: use dev_err instead of pr_err
Quoted 6 lines · Alan Tull
Use
dev_errmessages instead ofpr_err.Also s/®ion->dev/dev/ in two places where we already have dev =
®ion->dev.Signed-off-by Alan Tull <[email protected]>
Acked-by Moritz Fischer <[email protected]>
Quoted 97 lines
---
v2: new in this version of the patchset
v3: for bisectability some changes moved to earlier patches
v4: no change to this patch in this version of patchset--- drivers/fpga/fpga-region.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)drivers/fpga/fpga-region.c +10
-10@@ -102,7 +102,7 @@ static struct fpga_region *fpga_region_get(struct fpga_region *region)return ERR_PTR (-ENODEV ); } -dev_dbg (®ion->dev,"get\n" ); +dev_dbg (dev,"get\n" );return region; }@@ -116,7 +116,7 @@ static void fpga_region_put(struct fpga_region *region) {struct device *dev = ®ion->dev; -dev_dbg (®ion->dev,"put\n" ); +dev_dbg (dev,"put\n" );module_put (dev->parent->driver->owner);of_node_put (dev->of_node);@@ -239,13 +239,13 @@ static int fpga_region_program_fpga(struct fpga_region *region, region =fpga_region_get (region);if (IS_ERR (region)) { -pr_err ("failed to get fpga region\n" ); +dev_err (dev,"failed to get FPGA region\n" );return PTR_ERR (region); } mgr =fpga_region_get_manager (region);if (IS_ERR (mgr)) { -pr_err ("failed to get fpga region manager\n" ); +dev_err (dev,"failed to get FPGA manager\n" ); ret =PTR_ERR (mgr); goto err_put_region; }@@ -258,25 +258,25 @@ static int fpga_region_program_fpga(struct fpga_region *region, ret =fpga_region_get_bridges (region, overlay);if (ret) { -pr_err ("failed to get fpga region bridges\n" ); +dev_err (dev,"failed to get FPGA bridges\n" ); goto err_unlock_mgr; } ret =fpga_bridges_disable (®ion->bridge_list);if (ret) { -pr_err ("failed to disable region bridges\n" ); +dev_err (dev,"failed to disable bridges\n" ); goto err_put_br; } ret =fpga_mgr_load (mgr, region->info);if (ret) { -pr_err ("failed to load fpga image\n" ); +dev_err (dev,"failed to load FPGA image\n" ); goto err_put_br; } ret =fpga_bridges_enable (®ion->bridge_list);if (ret) { -pr_err ("failed to enable region bridges\n" ); +dev_err (dev,"failed to enable region bridges\n" ); goto err_put_br; }@@ -407,7 +407,7 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,/* If FPGA was externally programmed, don't specify firmware */ if ((info->flags &FPGA_MGR_EXTERNAL_CONFIG ) && info->firmware_name) { -pr_err ("error: specified firmware and external-fpga-config" ); +dev_err (dev,"error: specified firmware and external-fpga-config" );fpga_image_info_free (info);return -EINVAL ; }@@ -420,7 +420,7 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,/* If we got this far, we should be programming the FPGA */ if (!info->firmware_name) { -pr_err ("should specify firmware-name or external-fpga-config\n" ); +dev_err (dev,"should specify firmware-name or external-fpga-config\n" );fpga_image_info_free (info);return -EINVAL ; }Signature · 1 line
2.7.4
Re: [PATCH v4 04/18] fpga: region: use dev_err instead of pr_err
Quoted 8 lines · Moritz Fischer
Quoted 6 lines · Alan Tull
Use
dev_errmessages instead ofpr_err.Also s/®ion->dev/dev/ in two places where we already have dev =
®ion->dev.Signed-off-by Alan Tull <[email protected]>Acked-by Moritz Fischer <[email protected]>
Hi Moritz,
Thanks!
Alan
Quoted 97 lines
Quoted 97 lines
---
v2: new in this version of the patchset
v3: for bisectability some changes moved to earlier patches
v4: no change to this patch in this version of patchset--- drivers/fpga/fpga-region.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)drivers/fpga/fpga-region.c +10
-11@@ -102,7 +102,7 @@ static struct fpga_region *fpga_region_get(struct fpga_region *region)return ERR_PTR (-ENODEV ); } -dev_dbg (®ion->dev,"get\n" ); +dev_dbg (dev,"get\n" );return region; }@@ -116,7 +116,7 @@ static void fpga_region_put(struct fpga_region *region) {struct device *dev = ®ion->dev; -dev_dbg (®ion->dev,"put\n" ); +dev_dbg (dev,"put\n" );module_put (dev->parent->driver->owner);of_node_put (dev->of_node);@@ -239,13 +239,13 @@ static int fpga_region_program_fpga(struct fpga_region *region, region =fpga_region_get (region);if (IS_ERR (region)) { -pr_err ("failed to get fpga region\n" ); +dev_err (dev,"failed to get FPGA region\n" );return PTR_ERR (region); } mgr =fpga_region_get_manager (region);if (IS_ERR (mgr)) { -pr_err ("failed to get fpga region manager\n" ); +dev_err (dev,"failed to get FPGA manager\n" ); ret =PTR_ERR (mgr); goto err_put_region; }@@ -258,25 +258,25 @@ static int fpga_region_program_fpga(struct fpga_region *region, ret =fpga_region_get_bridges (region, overlay);if (ret) { -pr_err ("failed to get fpga region bridges\n" ); +dev_err (dev,"failed to get FPGA bridges\n" ); goto err_unlock_mgr; } ret =fpga_bridges_disable (®ion->bridge_list);if (ret) { -pr_err ("failed to disable region bridges\n" ); +dev_err (dev,"failed to disable bridges\n" ); goto err_put_br; } ret =fpga_mgr_load (mgr, region->info);if (ret) { -pr_err ("failed to load fpga image\n" ); +dev_err (dev,"failed to load FPGA image\n" ); goto err_put_br; } ret =fpga_bridges_enable (®ion->bridge_list);if (ret) { -pr_err ("failed to enable region bridges\n" ); +dev_err (dev,"failed to enable region bridges\n" ); goto err_put_br; }@@ -407,7 +407,7 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,/* If FPGA was externally programmed, don't specify firmware */ if ((info->flags &FPGA_MGR_EXTERNAL_CONFIG ) && info->firmware_name) { -pr_err ("error: specified firmware and external-fpga-config" ); +dev_err (dev,"error: specified firmware and external-fpga-config" );fpga_image_info_free (info);return -EINVAL ; }@@ -420,7 +420,7 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,/* If we got this far, we should be programming the FPGA */ if (!info->firmware_name) { -pr_err ("should specify firmware-name or external-fpga-config\n" ); +dev_err (dev,"should specify firmware-name or external-fpga-config\n" );fpga_image_info_free (info);return -EINVAL ; } --2.7.4
PATCH v4 5/18 fpga: region: remove unneeded of_node_get and put
In reply to Alan TullRemove of_node_get/put in fpga_region_get/put. Not needed and will get in the way when I separate out the common FPGA region code from Device Tree support code.
Signed-off-by Alan Tull <[email protected]>
Acked-by Moritz Fischer <[email protected]>
---
v2: split out from another patch
v3: Add Moritz' ack
v4: no change to this patch in this version of patchset
--- drivers/fpga/fpga-region.c | 3 --- 1 file changed, 3 deletions(-)
drivers/fpga/fpga-region.c +0 -3
@@ -94,9 +94,7 @@ static struct fpga_region *fpga_region_get(struct fpga_region *region) }get_device (dev); -of_node_get (dev->of_node);if (!try_module_get (dev->parent->driver->owner)) { -of_node_put (dev->of_node);put_device (dev);mutex_unlock (®ion->mutex);return ERR_PTR (-ENODEV );
@@ -119,7 +117,6 @@ static void fpga_region_put(struct fpga_region *region)dev_dbg (dev,"put\n" );module_put (dev->parent->driver->owner); -of_node_put (dev->of_node);put_device (dev);mutex_unlock (®ion->mutex); }
Signature · 1 line
2.7.4
PATCH v4 6/18 fpga: region: get mgr early on
In reply to Alan TullGet the FPGA manager during region creation.
This is a baby step in refactoring the FPGA region code to separate out common FPGA region code from FPGA region Device Tree overlay support.
Signed-off-by Alan Tull <[email protected]>
---
v2: split out from another patch
v3: no change to this patch in this version of patchset
v4: no change to this patch in this version of patchset
--- drivers/fpga/fpga-region.c | 45 +++++++++++++++++++++++---------------------- 1 file changed, 23 insertions(+), 22 deletions(-)
drivers/fpga/fpga-region.c +23 -22
@@ -31,12 +31,14 @@* @dev: FPGA Region device * @mutex: enforces exclusive reference to region * @bridge_list: list of FPGA bridges specified in region +* @mgr: FPGA manager * @info: fpga image specific information */ struct fpga_region {struct device dev;struct mutex mutex;/* for exclusive reference to region */ struct list_head bridge_list; +struct fpga_manager *mgr;struct fpga_image_info *info; };
@@ -123,7 +125,7 @@ static void fpga_region_put(struct fpga_region *region)/** * fpga_region_get_manager - get reference for FPGA manager -* @region: FPGA region +* @np: device node of FPGA region * * Get FPGA Manager from "fpga-mgr" property or from ancestor region. *
@@ -131,10 +133,8 @@ static void fpga_region_put(struct fpga_region *region)* * Return: fpga manager struct or IS_ERR() condition containing error code. */ -static struct fpga_manager *fpga_region_get_manager (struct fpga_region *region) +static struct fpga_manager *fpga_region_get_manager (struct device_node *np) { -struct device *dev = ®ion->dev; -struct device_node *np = dev->of_node;struct device_node *mgr_node;struct fpga_manager *mgr;
@@ -231,7 +231,6 @@ static int fpga_region_program_fpga(struct fpga_region *region, struct device_node *overlay) {struct device *dev = ®ion->dev; -struct fpga_manager *mgr;int ret; region =fpga_region_get (region);
@@ -240,17 +239,10 @@ static int fpga_region_program_fpga(struct fpga_region *region,return PTR_ERR (region); } - mgr =fpga_region_get_manager (region); -if (IS_ERR (mgr)) { -dev_err (dev,"failed to get FPGA manager\n" ); - ret =PTR_ERR (mgr); - goto err_put_region; - } - - ret =fpga_mgr_lock (mgr); + ret =fpga_mgr_lock (region->mgr);if (ret) {dev_err (dev,"FPGA manager is busy\n" ); - goto err_put_mgr; + goto err_put_region; } ret =fpga_region_get_bridges (region, overlay);
@@ -265,7 +257,7 @@ static int fpga_region_program_fpga(struct fpga_region *region, goto err_put_br; } - ret =fpga_mgr_load (mgr, region->info); + ret =fpga_mgr_load (region->mgr, region->info);if (ret) {dev_err (dev,"failed to load FPGA image\n" ); goto err_put_br;
@@ -277,8 +269,7 @@ static int fpga_region_program_fpga(struct fpga_region *region, goto err_put_br; } -fpga_mgr_unlock (mgr); -fpga_mgr_put (mgr); +fpga_mgr_unlock (region->mgr);fpga_region_put (region);return 0 ;
@@ -286,9 +277,7 @@ static int fpga_region_program_fpga(struct fpga_region *region, err_put_br:fpga_bridges_put (®ion->bridge_list); err_unlock_mgr: -fpga_mgr_unlock (mgr); -err_put_mgr: -fpga_mgr_put (mgr); +fpga_mgr_unlock (region->mgr); err_put_region:fpga_region_put (region);
@@ -517,11 +506,20 @@ static int fpga_region_probe(struct platform_device *pdev)struct device *dev = &pdev->dev;struct device_node *np = dev->of_node;struct fpga_region *region; +struct fpga_manager *mgr;int id, ret =0 ; + mgr =fpga_region_get_manager (np); +if (IS_ERR (mgr)) +return -EPROBE_DEFER ; + region =kzalloc (sizeof (*region),GFP_KERNEL ); -if (!region) -return -ENOMEM ; +if (!region) { + ret = -ENOMEM ; + goto err_put_mgr; + } + + region->mgr = mgr; id =ida_simple_get (&fpga_region_ida,0 ,0 ,GFP_KERNEL );if (id <0 ) {
@@ -557,6 +555,8 @@ static int fpga_region_probe(struct platform_device *pdev)ida_simple_remove (&fpga_region_ida, id); err_kfree:kfree (region); +err_put_mgr: +fpga_mgr_put (mgr);return ret; }
@@ -566,6 +566,7 @@ static int fpga_region_remove(struct platform_device *pdev)struct fpga_region *region =platform_get_drvdata (pdev);device_unregister (®ion->dev); +fpga_mgr_put (region->mgr);return 0 ; }
Signature · 1 line
2.7.4
PATCH v4 7/18 fpga: region: check for child regions before allocing image info
In reply to Alan TullDuring a device tree overlay pre-apply notification, the check for child FPGA regions can happen slightly earlier. This saves us from allocating the FPGA image info that just gets thrown away.
This is a baby step in refactoring the FPGA region code to separate out common FPGA region code from FPGA region Device Tree overlay support.
Signed-off-by Alan Tull <[email protected]>
Acked-by Moritz Fischer <[email protected]>
---
v2: split out from another patch
v3: s/dev/®ion->dev/ in one place
add Moritz' ack
v4: no change to this patch in this version of patchset
--- drivers/fpga/fpga-region.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
drivers/fpga/fpga-region.c +9 -5
@@ -355,15 +355,19 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,const char *firmware_name;int ret; - info =fpga_image_info_alloc (dev); -if (!info) -return -ENOMEM ; - -/* Reject overlay if child FPGA Regions have firmware-name property */ +/* +* Reject overlay if child FPGA Regions added in the overlay have +* firmware-name property (would mean that an FPGA region that has +* not been added to the live tree yet is doing FPGA programming). +*/ ret =child_regions_with_firmware (nd->overlay);if (ret)return ret; + info =fpga_image_info_alloc (dev); +if (!info) +return -ENOMEM ; +/* Read FPGA region properties from the overlay */ if (of_property_read_bool (nd->overlay,"partial-fpga-config" )) info->flags |=FPGA_MGR_PARTIAL_RECONFIG ;
Signature · 1 line
2.7.4
PATCH v4 8/18 fpga: region: fix slow warning with more than one overlay
In reply to Alan TullWhen DT overlays are applied, each FPGA region keeps track of the fpga image info as region->info. This pointer is assigned only if an overlay causes the FPGA to be programmed. As it stands, this pointer can be overwritten, causing a slow warning later when overlays are removed.
This patch fixes this by changing the allowed behaviour. If a region has received an overlay that programmed the FPGA, reject other overlays that try to program the FPGA. To reprogram the FPGA, first remove the overlay. This makes sense as removing the overlay also removes the devices cleanly. Note that overlays that make DT changes without reprogramming the FPGA are exempt from this restriction.
Signed-off-by Alan Tull <[email protected]>
---
v2: split out from another patch
v3: better explanation in header
v4: no change to this patch in this version of patchset
--- drivers/fpga/fpga-region.c | 5 +++++ 1 file changed, 5 insertions(+)
drivers/fpga/fpga-region.c +5 -0
@@ -355,6 +355,11 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,const char *firmware_name;int ret; +if (region->info) { +dev_err (dev,"Region already has overlay applied.\n" ); +return -EINVAL ; + } +/* * Reject overlay if child FPGA Regions added in the overlay have * firmware-name property (would mean that an FPGA region that has
Signature · 1 line
2.7.4
PATCH v4 9/18 fpga: region: use image info as parameter for programming region
In reply to Alan TullUse FPGA image info (region->info) when region code is programming the FPGA to pass in multiple parameters.
This is a baby step in refactoring the FPGA region code to separate out common FPGA region code from FPGA region Device Tree overlay support.
Signed-off-by Alan Tull <[email protected]>
---
v2: split out from another patch
v3: change API to use the region->info, remove info param
remove check for region->info, not needed v4: no change to this patch in this version of patchset --- drivers/fpga/fpga-region.c | 16 +++++++++------- include/linux/fpga/fpga-mgr.h | 4 ++++ 2 files changed, 13 insertions(+), 7 deletions(-)
drivers/fpga/fpga-region.c +9 -7
@@ -223,14 +223,13 @@ static int fpga_region_get_bridges(struct fpga_region *region,/** * fpga_region_program_fpga - program FPGA * @region: FPGA region -* @overlay: device node of the overlay -* Program an FPGA using information in the region's fpga image info. +* Program an FPGA using fpga image info (region->info). * Return 0 for success or negative error code. */ -static int fpga_region_program_fpga (struct fpga_region *region, -struct device_node *overlay) +static int fpga_region_program_fpga (struct fpga_region *region) {struct device *dev = ®ion->dev; +struct fpga_image_info *info = region->info;int ret; region =fpga_region_get (region);
@@ -245,7 +244,7 @@ static int fpga_region_program_fpga(struct fpga_region *region, goto err_put_region; } - ret =fpga_region_get_bridges (region, overlay); + ret =fpga_region_get_bridges (region, info->overlay);if (ret) {dev_err (dev,"failed to get FPGA bridges\n" ); goto err_unlock_mgr;
@@ -257,7 +256,7 @@ static int fpga_region_program_fpga(struct fpga_region *region, goto err_put_br; } - ret =fpga_mgr_load (region->mgr, region->info); + ret =fpga_mgr_load (region->mgr, info);if (ret) {dev_err (dev,"failed to load FPGA image\n" ); goto err_put_br;
@@ -373,6 +372,8 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,if (!info)return -ENOMEM ; + info->overlay = nd->overlay; +/* Read FPGA region properties from the overlay */ if (of_property_read_bool (nd->overlay,"partial-fpga-config" )) info->flags |=FPGA_MGR_PARTIAL_RECONFIG ;
@@ -421,7 +422,8 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region, } region->info = info; - ret =fpga_region_program_fpga (region, nd->overlay); + + ret =fpga_region_program_fpga (region);if (ret) {fpga_image_info_free (info); region->info =NULL ;
include/linux/fpga/fpga-mgr.h +4 -0
@@ -89,6 +89,7 @@ enum fpga_mgr_states {* @buf: contiguous buffer containing FPGA image * @count: size of buf * @dev: device that owns this +* @overlay: Device Tree overlay */ struct fpga_image_info {u32 flags;
@@ -100,6 +101,9 @@ struct fpga_image_info {const char *buf;size_t count;struct device *dev; +#ifdef CONFIG_OF +struct device_node *overlay; +#endif };/**
Signature · 1 line
2.7.4
PATCH v4 10/18 fpga: region: separate out code that parses the overlay
In reply to Alan TullNew function of_fpga_region_parse_ov added, moving code from fpga_region_notify_pre_apply. This function gets the FPGA image info from the overlay and is able to simplify some of the logic involved.
This is a baby step in refactoring the FPGA region code to separate out common code from Device Tree overlay support.
Signed-off-by Alan Tull <[email protected]>
---
v2: split out from another patch
v3: update comments
minor changes in flow
v4: no change to this patch in this version of patchset
--- drivers/fpga/fpga-region.c | 122 +++++++++++++++++++++++++++------------------ 1 file changed, 73 insertions(+), 49 deletions(-)
drivers/fpga/fpga-region.c +73 -49
@@ -321,33 +321,22 @@ static int child_regions_with_firmware(struct device_node *overlay) }/** -* fpga_region_notify_pre_apply - pre-apply overlay notification -* -* @region: FPGA region that the overlay was applied to -* @nd: overlay notification data -* -* Called after when an overlay targeted to a FPGA Region is about to be -* applied. Function will check the properties that will be added to the FPGA -* region. If the checks pass, it will program the FPGA. -* -* The checks are: -* The overlay must add either firmware-name or external-fpga-config property -* to the FPGA Region. -* -* firmware-name : program the FPGA -* external-fpga-config : FPGA is already programmed -* encrypted-fpga-config : FPGA bitstream is encrypted +* of_fpga_region_parse_ov - parse and check overlay applied to region * -* The overlay can add other FPGA regions, but child FPGA regions cannot have a -* firmware-name property since those regions don't exist yet. +* @region: FPGA region +* @overlay: overlay applied to the FPGA region * -* If the overlay that breaks the rules, notifier returns an error and the -* overlay is rejected before it goes into the main tree. +* Given an overlay applied to a FPGA region, parse the FPGA image specific +* info in the overlay and do some checking. * -* Returns 0 for success or negative error code for failure. +* Returns: +* NULL if overlay doesn't direct us to program the FPGA. +* fpga_image_info struct if there is an image to program. +* error code for invalid overlay. */ -static int fpga_region_notify_pre_apply (struct fpga_region *region, -struct of_overlay_notify_data *nd) +static struct fpga_image_info *of_fpga_region_parse_ov ( +struct fpga_region *region, +struct device_node *overlay) {struct device *dev = ®ion->dev;struct fpga_image_info *info;
@@ -356,7 +345,7 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,if (region->info) {dev_err (dev,"Region already has overlay applied.\n" ); -return -EINVAL ; +return ERR_PTR (-EINVAL ); }/*
@@ -364,67 +353,102 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,* firmware-name property (would mean that an FPGA region that has * not been added to the live tree yet is doing FPGA programming). */ - ret =child_regions_with_firmware (nd->overlay); + ret =child_regions_with_firmware (overlay);if (ret) -return ret; +return ERR_PTR (ret); info =fpga_image_info_alloc (dev);if (!info) -return -ENOMEM ; +return ERR_PTR (-ENOMEM ); - info->overlay = nd->overlay; + info->overlay = overlay;/* Read FPGA region properties from the overlay */ -if (of_property_read_bool (nd->overlay,"partial-fpga-config" )) +if (of_property_read_bool (overlay,"partial-fpga-config" )) info->flags |=FPGA_MGR_PARTIAL_RECONFIG ; -if (of_property_read_bool (nd->overlay,"external-fpga-config" )) +if (of_property_read_bool (overlay,"external-fpga-config" )) info->flags |=FPGA_MGR_EXTERNAL_CONFIG ; -if (of_property_read_bool (nd->overlay,"encrypted-fpga-config" )) +if (of_property_read_bool (overlay,"encrypted-fpga-config" )) info->flags |=FPGA_MGR_ENCRYPTED_BITSTREAM ; -if (!of_property_read_string (nd->overlay,"firmware-name" , +if (!of_property_read_string (overlay,"firmware-name" , &firmware_name)) { info->firmware_name =devm_kstrdup (dev, firmware_name,GFP_KERNEL );if (!info->firmware_name) -return -ENOMEM ; +return ERR_PTR (-ENOMEM ); } -of_property_read_u32 (nd->overlay,"region-unfreeze-timeout-us" , +of_property_read_u32 (overlay,"region-unfreeze-timeout-us" , &info->enable_timeout_us); -of_property_read_u32 (nd->overlay,"region-freeze-timeout-us" , +of_property_read_u32 (overlay,"region-freeze-timeout-us" , &info->disable_timeout_us); -of_property_read_u32 (nd->overlay,"config-complete-timeout-us" , +of_property_read_u32 (overlay,"config-complete-timeout-us" , &info->config_complete_timeout_us); -/* If FPGA was externally programmed, don't specify firmware */ -if ((info->flags &FPGA_MGR_EXTERNAL_CONFIG ) && info->firmware_name) { -dev_err (dev,"error: specified firmware and external-fpga-config" ); -fpga_image_info_free (info); -return -EINVAL ; +/* If overlay is not programming the FPGA, don't need FPGA image info */ +if (!info->firmware_name) { + ret =0 ; + goto ret_no_info; } -/* FPGA is already configured externally. We're done. */ +/* +* If overlay informs us FPGA was externally programmed, specifying +* firmware here would be ambiguous. +*/ if (info->flags &FPGA_MGR_EXTERNAL_CONFIG ) { -fpga_image_info_free (info); -return 0 ; +dev_err (dev,"error: specified firmware and external-fpga-config" ); + ret = -EINVAL ; + goto ret_no_info; } -/* If we got this far, we should be programming the FPGA */ -if (!info->firmware_name) { -dev_err (dev,"should specify firmware-name or external-fpga-config\n" ); -fpga_image_info_free (info); +return info; +ret_no_info: +fpga_image_info_free (info); +return ERR_PTR (ret); +} + +/** +* fpga_region_notify_pre_apply - pre-apply overlay notification +* +* @region: FPGA region that the overlay was applied to +* @nd: overlay notification data +* +* Called when an overlay targeted to a FPGA Region is about to be applied. +* Parses the overlay for properties that influence how the FPGA will be +* programmed and does some checking. If the checks pass, programs the FPGA. +* If the checks fail, overlay is rejected and does not get added to the +* live tree. +* +* Returns 0 for success or negative error code for failure. +*/ +static int fpga_region_notify_pre_apply (struct fpga_region *region, +struct of_overlay_notify_data *nd) +{ +struct device *dev = ®ion->dev; +struct fpga_image_info *info; +int ret; + +if (region->info) { +dev_err (dev,"Region already has overlay applied.\n" );return -EINVAL ; } - region->info = info; + info =of_fpga_region_parse_ov (region, nd->overlay); +if (IS_ERR (info)) +return PTR_ERR (info); + +if (!info) +return 0 ; + region->info = info; ret =fpga_region_program_fpga (region);if (ret) { +/* error; reject overlay */ fpga_image_info_free (info); region->info =NULL ; }
Signature · 1 line
2.7.4
PATCH v4 11/18 fpga: region: add fpga-region.h header
In reply to Alan Tull* Create fpga-region.h.
* Export fpga_region_program_fpga.
* Move struct fpga_region and other things to the header.
This is a step in separating FPGA region common code from Device Tree support.
Signed-off-by Alan Tull <[email protected]>
---
v2: split out from another patch
update author email
v3: changes due to fpga_region_program_fpga() removed info param
v4: no change to this patch in this version of patchset
--- drivers/fpga/fpga-region.c | 24 ++++-------------------- include/linux/fpga/fpga-region.h | 28 ++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 20 deletions(-) create mode 100644 include/linux/fpga/fpga-region.h
drivers/fpga/fpga-region.c +4 -20
@@ -18,6 +18,7 @@#include <linux/fpga/fpga-bridge.h> #include <linux/fpga/fpga-mgr.h> +#include <linux/fpga/fpga-region.h> #include <linux/idr.h> #include <linux/kernel.h> #include <linux/list.h>
@@ -26,24 +27,6 @@#include <linux/slab.h> #include <linux/spinlock.h> -/** -* struct fpga_region - FPGA Region structure -* @dev: FPGA Region device -* @mutex: enforces exclusive reference to region -* @bridge_list: list of FPGA bridges specified in region -* @mgr: FPGA manager -* @info: fpga image specific information -*/ -struct fpga_region { -struct device dev; -struct mutex mutex;/* for exclusive reference to region */ -struct list_head bridge_list; -struct fpga_manager *mgr; -struct fpga_image_info *info; -}; - -#define to_fpga_region (d) container_of(d, struct fpga_region, dev) -static DEFINE_IDA (fpga_region_ida);static struct class *fpga_region_class;
@@ -226,7 +209,7 @@ static int fpga_region_get_bridges(struct fpga_region *region,* Program an FPGA using fpga image info (region->info). * Return 0 for success or negative error code. */ -static int fpga_region_program_fpga (struct fpga_region *region) +int fpga_region_program_fpga (struct fpga_region *region) {struct device *dev = ®ion->dev;struct fpga_image_info *info = region->info;
@@ -282,6 +265,7 @@ static int fpga_region_program_fpga(struct fpga_region *region)return ret; } +EXPORT_SYMBOL_GPL (fpga_region_program_fpga);/** * child_regions_with_firmware
@@ -667,5 +651,5 @@ subsys_initcall(fpga_region_init);module_exit (fpga_region_exit);MODULE_DESCRIPTION ("FPGA Region" ); -MODULE_AUTHOR ("Alan Tull <[email protected]>" ); +MODULE_AUTHOR ("Alan Tull <[email protected]>" );MODULE_LICENSE ("GPL v2" );
include/linux/fpga/fpga-region.h new file +28 -0
@@ -0,0 +1,28 @@ +#include <linux/device.h> +#include <linux/fpga/fpga-mgr.h> +#include <linux/fpga/fpga-bridge.h> + +#ifndef _FPGA_REGION_H +#define _FPGA_REGION_H + +/** +* struct fpga_region - FPGA Region structure +* @dev: FPGA Region device +* @mutex: enforces exclusive reference to region +* @bridge_list: list of FPGA bridges specified in region +* @mgr: FPGA manager +* @info: FPGA image info +*/ +struct fpga_region { +struct device dev; +struct mutex mutex;/* for exclusive reference to region */ +struct list_head bridge_list; +struct fpga_manager *mgr; +struct fpga_image_info *info; +}; + +#define to_fpga_region (d) container_of(d, struct fpga_region, dev) + +int fpga_region_program_fpga (struct fpga_region *region); + +#endif /* _FPGA_REGION_H */
Signature · 1 line
2.7.4
Re: [PATCH v4 11/18] fpga: region: add fpga-region.h header
Quoted 86 lines · Alan Tull
* Create
fpga-region.h.
* Exportfpga_region_program_fpga.
* Move structfpga_regionand other things to the header.This is a step in separating FPGA region common code from Device Tree support.
Signed-off-by Alan Tull <[email protected]>---
v2: split out from another patch
update author email
v3: changes due tofpga_region_program_fpga()removed info param
v4: no change to this patch in this version of patchset--- drivers/fpga/fpga-region.c | 24 ++++-------------------- include/linux/fpga/fpga-region.h | 28 ++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 20 deletions(-) create mode 100644 include/linux/fpga/fpga-region.hdrivers/fpga/fpga-region.c +4
-20@@ -18,6 +18,7 @@#include <linux/fpga/fpga-bridge.h> #include <linux/fpga/fpga-mgr.h> +#include <linux/fpga/fpga-region.h> #include <linux/idr.h> #include <linux/kernel.h> #include <linux/list.h> @@ -26,24 +27,6 @@#include <linux/slab.h> #include <linux/spinlock.h> -/** -* struct fpga_region - FPGA Region structure -* @dev: FPGA Region device -* @mutex: enforces exclusive reference to region -* @bridge_list: list of FPGA bridges specified in region -* @mgr: FPGA manager -* @info: fpga image specific information -*/ -struct fpga_region { -struct device dev; -struct mutex mutex;/* for exclusive reference to region */ -struct list_head bridge_list; -struct fpga_manager *mgr; -struct fpga_image_info *info; -}; - -#define to_fpga_region (d) container_of(d, struct fpga_region, dev) -static DEFINE_IDA (fpga_region_ida);static struct class *fpga_region_class;@@ -226,7 +209,7 @@ static int fpga_region_get_bridges(struct fpga_region *region,* Program an FPGA using fpga image info (region->info). * Return 0 for success or negative error code. */ -static int fpga_region_program_fpga (struct fpga_region *region) +int fpga_region_program_fpga (struct fpga_region *region) {struct device *dev = ®ion->dev;struct fpga_image_info *info = region->info;@@ -282,6 +265,7 @@ static int fpga_region_program_fpga(struct fpga_region *region)return ret; } +EXPORT_SYMBOL_GPL (fpga_region_program_fpga);/** * child_regions_with_firmware @@ -667,5 +651,5 @@ subsys_initcall(fpga_region_init);module_exit (fpga_region_exit);MODULE_DESCRIPTION ("FPGA Region" ); -MODULE_AUTHOR ("Alan Tull <[email protected]>" ); +MODULE_AUTHOR ("Alan Tull <[email protected]>" );MODULE_LICENSE ("GPL v2" );include/linux/fpga/fpga-region.h new file +0
-0
include/linux/fpga/fpga-region.h line 6
Quoted 6 lines
include/linux/fpga/fpga-region.h new file +6
-0@@ -0,0 +1,28 @@ +#include <linux/device.h> +#include <linux/fpga/fpga-mgr.h> +#include <linux/fpga/fpga-bridge.h> + +#ifndef _FPGA_REGION_H +#define _FPGA_REGION_H
Hi Alan
I think it may be better to move #ifndef check to the beginning of this header files (before include other header files). :)
Thanks
Hao
Quoted 29 lines
include/linux/fpga/fpga-region.h new file +22
-0+ +/** +* struct fpga_region - FPGA Region structure +* @dev: FPGA Region device +* @mutex: enforces exclusive reference to region +* @bridge_list: list of FPGA bridges specified in region +* @mgr: FPGA manager +* @info: FPGA image info +*/ +struct fpga_region { +struct device dev; +struct mutex mutex;/* for exclusive reference to region */ +struct list_head bridge_list; +struct fpga_manager *mgr; +struct fpga_image_info *info; +}; + +#define to_fpga_region (d) container_of(d, struct fpga_region, dev) + +int fpga_region_program_fpga (struct fpga_region *region); + +#endif /* _FPGA_REGION_H */ Signature · 6 lines
2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-fpga" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 11/18] fpga: region: add fpga-region.h header
On Thu, Sep 14, 2017 at 4:50 AM, Wu Hao <[email protected]> wrote:
Hi Hao,
Quoted 96 lines · Alan Tull
Quoted 86 lines
* Create
fpga-region.h.
* Exportfpga_region_program_fpga.
* Move structfpga_regionand other things to the header.This is a step in separating FPGA region common code from Device Tree support.
Signed-off-by Alan Tull <[email protected]>---
v2: split out from another patch
update author email
v3: changes due tofpga_region_program_fpga()removed info param
v4: no change to this patch in this version of patchset--- drivers/fpga/fpga-region.c | 24 ++++-------------------- include/linux/fpga/fpga-region.h | 28 ++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 20 deletions(-) create mode 100644 include/linux/fpga/fpga-region.hdrivers/fpga/fpga-region.c +4
-20@@ -18,6 +18,7 @@#include <linux/fpga/fpga-bridge.h> #include <linux/fpga/fpga-mgr.h> +#include <linux/fpga/fpga-region.h> #include <linux/idr.h> #include <linux/kernel.h> #include <linux/list.h> @@ -26,24 +27,6 @@#include <linux/slab.h> #include <linux/spinlock.h> -/** -* struct fpga_region - FPGA Region structure -* @dev: FPGA Region device -* @mutex: enforces exclusive reference to region -* @bridge_list: list of FPGA bridges specified in region -* @mgr: FPGA manager -* @info: fpga image specific information -*/ -struct fpga_region { -struct device dev; -struct mutex mutex;/* for exclusive reference to region */ -struct list_head bridge_list; -struct fpga_manager *mgr; -struct fpga_image_info *info; -}; - -#define to_fpga_region (d) container_of(d, struct fpga_region, dev) -static DEFINE_IDA (fpga_region_ida);static struct class *fpga_region_class;@@ -226,7 +209,7 @@ static int fpga_region_get_bridges(struct fpga_region *region,* Program an FPGA using fpga image info (region->info). * Return 0 for success or negative error code. */ -static int fpga_region_program_fpga (struct fpga_region *region) +int fpga_region_program_fpga (struct fpga_region *region) {struct device *dev = ®ion->dev;struct fpga_image_info *info = region->info;@@ -282,6 +265,7 @@ static int fpga_region_program_fpga(struct fpga_region *region)return ret; } +EXPORT_SYMBOL_GPL (fpga_region_program_fpga);/** * child_regions_with_firmware @@ -667,5 +651,5 @@ subsys_initcall(fpga_region_init);module_exit (fpga_region_exit);MODULE_DESCRIPTION ("FPGA Region" ); -MODULE_AUTHOR ("Alan Tull <[email protected]>" ); +MODULE_AUTHOR ("Alan Tull <[email protected]>" );MODULE_LICENSE ("GPL v2" );include/linux/fpga/fpga-region.h new file +0
-0include/linux/fpga/fpga-region.h line 6
Quoted 6 lines
include/linux/fpga/fpga-region.h new file +6
-0@@ -0,0 +1,28 @@ +#include <linux/device.h> +#include <linux/fpga/fpga-mgr.h> +#include <linux/fpga/fpga-bridge.h> + +#ifndef _FPGA_REGION_H +#define _FPGA_REGION_HHi Alan
Quoted 5 lines
I think it may be better to move #ifndef check to the beginning of this header files (before include other header files). :)
Thanks
Hao
That's reasonable. Will do!
Thanks,
Alan
Quoted 34 lines
Quoted 26 lines
include/linux/fpga/fpga-region.h new file +22
-1+ +/** +* struct fpga_region - FPGA Region structure +* @dev: FPGA Region device +* @mutex: enforces exclusive reference to region +* @bridge_list: list of FPGA bridges specified in region +* @mgr: FPGA manager +* @info: FPGA image info +*/ +struct fpga_region { +struct device dev; +struct mutex mutex;/* for exclusive reference to region */ +struct list_head bridge_list; +struct fpga_manager *mgr; +struct fpga_image_info *info; +}; + +#define to_fpga_region (d) container_of(d, struct fpga_region, dev) + +int fpga_region_program_fpga (struct fpga_region *region); + +#endif /* _FPGA_REGION_H */ --2.7.4
Quoted 4 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v4 12/18 fpga: region: rename some functions prior to moving
In reply to Alan TullRename some functions that will be moved to of-fpga-region.c. Also change some parameters and export a function to help with refactoring.
This is a step towards the larger goal of separating device tree support from FPGA region common code.
* fpga_region_get_manager -> of_fpga_region_get_mgr
* add 'of_' prefix to the following:
* fpga_region_find
* fpga_region_get_bridges
* fpga_region_notify_pre_apply
* fpga_region_notify_post_remove),
* fpga_region_probe/remove
Parameter changes:
* of_fpga_region_find
change parameter to be the device node of the region.
* of_fpga_region_get_bridges
change second parameter to FPGA image info.
Export of_fpga_region_find as well.
Signed-off-by Alan Tull <[email protected]>
---
v2: split out from another patch
v3: no changes in patch for this version of patchset
v4: no change to this patch in this version of patchset
--- drivers/fpga/fpga-region.c | 60 ++++++++++++++++++++++++---------------------- 1 file changed, 31 insertions(+), 29 deletions(-)
drivers/fpga/fpga-region.c +31 -29
@@ -42,12 +42,14 @@ static int fpga_region_of_node_match(struct device *dev, const void *data) }/** -* fpga_region_find - find FPGA region +* of_fpga_region_find - find FPGA region * @np: device node of FPGA Region +* * Caller will need to put_device(®ion->dev) when done. +* * Returns FPGA Region struct or NULL */ -static struct fpga_region *fpga_region_find (struct device_node *np) +static struct fpga_region *of_fpga_region_find (struct device_node *np) {struct device *dev;
@@ -107,7 +109,7 @@ static void fpga_region_put(struct fpga_region *region) }/** -* fpga_region_get_manager - get reference for FPGA manager +* of_fpga_region_get_mgr - get reference for FPGA manager * @np: device node of FPGA region * * Get FPGA Manager from "fpga-mgr" property or from ancestor region.
@@ -116,7 +118,7 @@ static void fpga_region_put(struct fpga_region *region)* * Return: fpga manager struct or IS_ERR() condition containing error code. */ -static struct fpga_manager *fpga_region_get_manager (struct device_node *np) +static struct fpga_manager *of_fpga_region_get_mgr (struct device_node *np) {struct device_node *mgr_node;struct fpga_manager *mgr;
@@ -139,9 +141,9 @@ static struct fpga_manager *fpga_region_get_manager(struct device_node *np) }/** -* fpga_region_get_bridges - create a list of bridges +* of_fpga_region_get_bridges - create a list of bridges * @region: FPGA region -* @overlay: device node of the overlay +* @info: FPGA image info * * Create a list of bridges including the parent bridge and the bridges * specified by "fpga-bridges" property. Note that the
@@ -154,8 +156,8 @@ static struct fpga_manager *fpga_region_get_manager(struct device_node *np)* Return 0 for success (even if there are no bridges specified) * or -EBUSY if any of the bridges are in use. */ -static int fpga_region_get_bridges (struct fpga_region *region, -struct device_node *overlay) +static int of_fpga_region_get_bridges (struct fpga_region *region, +struct fpga_image_info *info) {struct device *dev = ®ion->dev;struct device_node *region_np = dev->of_node;
@@ -163,7 +165,7 @@ static int fpga_region_get_bridges(struct fpga_region *region,int i, ret;/* If parent is a bridge, add to list */ - ret =of_fpga_bridge_get_to_list (region_np->parent, region->info, + ret =of_fpga_bridge_get_to_list (region_np->parent, info, ®ion->bridge_list);/* -EBUSY means parent is a bridge that is under use. Give up. */
@@ -175,8 +177,8 @@ static int fpga_region_get_bridges(struct fpga_region *region, parent_br = region_np->parent;/* If overlay has a list of bridges, use it. */ -if (of_parse_phandle (overlay,"fpga-bridges" ,0 )) - np = overlay; +if (of_parse_phandle (info->overlay,"fpga-bridges" ,0 )) + np = info->overlay;else np = region_np;
@@ -227,7 +229,7 @@ int fpga_region_program_fpga(struct fpga_region *region) goto err_put_region; } - ret =fpga_region_get_bridges (region, info->overlay); + ret =of_fpga_region_get_bridges (region, info);if (ret) {dev_err (dev,"failed to get FPGA bridges\n" ); goto err_unlock_mgr;
@@ -397,7 +399,7 @@ static struct fpga_image_info *of_fpga_region_parse_ov( }/** -* fpga_region_notify_pre_apply - pre-apply overlay notification +* of_fpga_region_notify_pre_apply - pre-apply overlay notification * * @region: FPGA region that the overlay was applied to * @nd: overlay notification data
@@ -410,8 +412,8 @@ static struct fpga_image_info *of_fpga_region_parse_ov(* * Returns 0 for success or negative error code for failure. */ -static int fpga_region_notify_pre_apply (struct fpga_region *region, -struct of_overlay_notify_data *nd) +static int of_fpga_region_notify_pre_apply (struct fpga_region *region, +struct of_overlay_notify_data *nd) {struct device *dev = ®ion->dev;struct fpga_image_info *info;
@@ -441,7 +443,7 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region, }/** -* fpga_region_notify_post_remove - post-remove overlay notification +* of_fpga_region_notify_post_remove - post-remove overlay notification * * @region: FPGA region that was targeted by the overlay that was removed * @nd: overlay notification data
@@ -449,8 +451,8 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,* Called after an overlay has been removed if the overlay's target was a * FPGA region. */ -static void fpga_region_notify_post_remove (struct fpga_region *region, -struct of_overlay_notify_data *nd) +static void of_fpga_region_notify_post_remove (struct fpga_region *region, +struct of_overlay_notify_data *nd) {fpga_bridges_disable (®ion->bridge_list);fpga_bridges_put (®ion->bridge_list);
@@ -493,18 +495,18 @@ static int of_fpga_region_notify(struct notifier_block *nb,return NOTIFY_OK ; } - region =fpga_region_find (nd->target); + region =of_fpga_region_find (nd->target);if (!region)return NOTIFY_OK ; ret =0 ;switch (action) {case OF_OVERLAY_PRE_APPLY : - ret =fpga_region_notify_pre_apply (region, nd); + ret =of_fpga_region_notify_pre_apply (region, nd);break ;case OF_OVERLAY_POST_REMOVE : -fpga_region_notify_post_remove (region, nd); +of_fpga_region_notify_post_remove (region, nd);break ; }
@@ -520,7 +522,7 @@ static struct notifier_block fpga_region_of_nb = { .notifier_call = of_fpga_region_notify, }; -static int fpga_region_probe (struct platform_device *pdev) +static int of_fpga_region_probe (struct platform_device *pdev) {struct device *dev = &pdev->dev;struct device_node *np = dev->of_node;
@@ -528,7 +530,7 @@ static int fpga_region_probe(struct platform_device *pdev)struct fpga_manager *mgr;int id, ret =0 ; - mgr =fpga_region_get_manager (np); + mgr =of_fpga_region_get_mgr (np);if (IS_ERR (mgr))return -EPROBE_DEFER ;
@@ -580,7 +582,7 @@ static int fpga_region_probe(struct platform_device *pdev)return ret; } -static int fpga_region_remove (struct platform_device *pdev) +static int of_fpga_region_remove (struct platform_device *pdev) {struct fpga_region *region =platform_get_drvdata (pdev);
@@ -590,9 +592,9 @@ static int fpga_region_remove(struct platform_device *pdev)return 0 ; } -static struct platform_driver fpga_region_driver = { - .probe = fpga_region_probe, - .remove = fpga_region_remove, +static struct platform_driver of_fpga_region_driver = { + .probe = of_fpga_region_probe, + .remove = of_fpga_region_remove, .driver = { .name ="fpga-region" , .of_match_table =of_match_ptr (fpga_region_of_match),
@@ -625,7 +627,7 @@ static int __init fpga_region_init(void)if (ret) goto err_class; - ret =platform_driver_register (&fpga_region_driver); + ret =platform_driver_register (&of_fpga_region_driver);if (ret) goto err_plat;
@@ -641,7 +643,7 @@ static int __init fpga_region_init(void) static void __exit fpga_region_exit(void) { -platform_driver_unregister (&fpga_region_driver); +platform_driver_unregister (&of_fpga_region_driver);of_overlay_notifier_unregister (&fpga_region_of_nb);class_destroy (fpga_region_class);ida_destroy (&fpga_region_ida);
Signature · 1 line
2.7.4
PATCH v4 13/18 fpga: region: add register/unregister functions
In reply to Alan TullAnother step in separating common code from device tree specific code for FPGA regions.
* add FPGA region register/unregister functions.
* add the register/unregister functions to the header
* use devm_kzalloc to alloc the region.
* add a method for getting bridges to the region struct
* add priv to the region struct
* use region->info in of_fpga_region_get_bridges
Signed-off-by Alan Tull <[email protected]>
---
v2: split out from another patch
v3: use region->info, remove info param where applicable
v4: no change to this patch in this version of patchset
--- drivers/fpga/fpga-region.c | 106 ++++++++++++++++++++++++--------------- include/linux/fpga/fpga-region.h | 7 +++ 2 files changed, 72 insertions(+), 41 deletions(-)
drivers/fpga/fpga-region.c +65 -41
@@ -143,7 +143,6 @@ static struct fpga_manager *of_fpga_region_get_mgr(struct device_node *np)/** * of_fpga_region_get_bridges - create a list of bridges * @region: FPGA region -* @info: FPGA image info * * Create a list of bridges including the parent bridge and the bridges * specified by "fpga-bridges" property. Note that the
@@ -156,11 +155,11 @@ static struct fpga_manager *of_fpga_region_get_mgr(struct device_node *np)* Return 0 for success (even if there are no bridges specified) * or -EBUSY if any of the bridges are in use. */ -static int of_fpga_region_get_bridges (struct fpga_region *region, -struct fpga_image_info *info) +static int of_fpga_region_get_bridges (struct fpga_region *region) {struct device *dev = ®ion->dev;struct device_node *region_np = dev->of_node; +struct fpga_image_info *info = region->info;struct device_node *br, *np, *parent_br =NULL ;int i, ret;
@@ -192,7 +191,7 @@ static int of_fpga_region_get_bridges(struct fpga_region *region,continue ;/* If node is a bridge, get it and add to list */ - ret =of_fpga_bridge_get_to_list (br, region->info, + ret =of_fpga_bridge_get_to_list (br, info, ®ion->bridge_list);/* If any of the bridges are in use, give up */
@@ -229,10 +228,16 @@ int fpga_region_program_fpga(struct fpga_region *region) goto err_put_region; } - ret =of_fpga_region_get_bridges (region, info); -if (ret) { -dev_err (dev,"failed to get FPGA bridges\n" ); - goto err_unlock_mgr; +/* +* In some cases, we already have a list of bridges in the +* fpga region struct. Or we don't have any bridges. +*/ +if (region->get_bridges) { + ret = region->get_bridges (region); +if (ret) { +dev_err (dev,"failed to get fpga region bridges\n" ); + goto err_unlock_mgr; + } } ret =fpga_bridges_disable (®ion->bridge_list);
@@ -259,7 +264,8 @@ int fpga_region_program_fpga(struct fpga_region *region)return 0 ; err_put_br: -fpga_bridges_put (®ion->bridge_list); +if (region->get_bridges) +fpga_bridges_put (®ion->bridge_list); err_unlock_mgr:fpga_mgr_unlock (region->mgr); err_put_region:
@@ -522,39 +528,20 @@ static struct notifier_block fpga_region_of_nb = { .notifier_call = of_fpga_region_notify, }; -static int of_fpga_region_probe (struct platform_device *pdev) +int fpga_region_register (struct device *dev,struct fpga_region *region) { -struct device *dev = &pdev->dev; -struct device_node *np = dev->of_node; -struct fpga_region *region; -struct fpga_manager *mgr;int id, ret =0 ; - mgr =of_fpga_region_get_mgr (np); -if (IS_ERR (mgr)) -return -EPROBE_DEFER ; - - region =kzalloc (sizeof (*region),GFP_KERNEL ); -if (!region) { - ret = -ENOMEM ; - goto err_put_mgr; - } - - region->mgr = mgr; - id =ida_simple_get (&fpga_region_ida,0 ,0 ,GFP_KERNEL ); -if (id <0 ) { - ret = id; - goto err_kfree; - } +if (id <0 ) +return id;mutex_init (®ion->mutex);INIT_LIST_HEAD (®ion->bridge_list); -device_initialize (®ion->dev); region->dev.class = fpga_region_class; region->dev.parent = dev; - region->dev.of_node = np; + region->dev.of_node = dev->of_node; region->dev.id = id;dev_set_drvdata (dev, region);
@@ -566,19 +553,58 @@ static int of_fpga_region_probe(struct platform_device *pdev)if (ret) goto err_remove; +return 0 ; + +err_remove: +ida_simple_remove (&fpga_region_ida, id); +return ret; +} +EXPORT_SYMBOL_GPL (fpga_region_register); + +int fpga_region_unregister (struct fpga_region *region) +{ +device_unregister (®ion->dev); + +return 0 ; +} +EXPORT_SYMBOL_GPL (fpga_region_unregister); + +static int of_fpga_region_probe (struct platform_device *pdev) +{ +struct device *dev = &pdev->dev; +struct device_node *np = dev->of_node; +struct fpga_region *region; +struct fpga_manager *mgr; +int ret; + +/* Find the FPGA mgr specified by region or parent region. */ + mgr =of_fpga_region_get_mgr (np); +if (IS_ERR (mgr)) +return -EPROBE_DEFER ; + + region =devm_kzalloc (dev,sizeof (*region),GFP_KERNEL ); +if (!region) { + ret = -ENOMEM ; + goto eprobe_mgr_put; + } + + region->mgr = mgr; + +/* Specify how to get bridges for this type of region. */ + region->get_bridges = of_fpga_region_get_bridges; + + ret =fpga_region_register (dev, region); +if (ret) + goto eprobe_mgr_put; +of_platform_populate (np, fpga_region_of_match,NULL , ®ion->dev);dev_info (dev,"FPGA Region probed\n" );return 0 ; -err_remove: -ida_simple_remove (&fpga_region_ida, id); -err_kfree: -kfree (region); -err_put_mgr: +eprobe_mgr_put:fpga_mgr_put (mgr); -return ret; }
@@ -586,8 +612,7 @@ static int of_fpga_region_remove(struct platform_device *pdev) {struct fpga_region *region =platform_get_drvdata (pdev); -device_unregister (®ion->dev); -fpga_mgr_put (region->mgr); +fpga_region_unregister (region);return 0 ; }
@@ -606,7 +631,6 @@ static void fpga_region_dev_release(struct device *dev)struct fpga_region *region =to_fpga_region (dev);ida_simple_remove (&fpga_region_ida, region->dev.id); -kfree (region); }/**
include/linux/fpga/fpga-region.h +7 -0
@@ -12,6 +12,8 @@* @bridge_list: list of FPGA bridges specified in region * @mgr: FPGA manager * @info: FPGA image info +* @priv: private data +* @get_bridges: optional function to get bridges to a list */ struct fpga_region {struct device dev;
@@ -19,10 +21,15 @@ struct fpga_region {struct list_head bridge_list;struct fpga_manager *mgr;struct fpga_image_info *info; +void *priv; + int (*get_bridges)(struct fpga_region *region); };#define to_fpga_region (d) container_of(d, struct fpga_region, dev)int fpga_region_program_fpga (struct fpga_region *region); +int fpga_region_register (struct device *dev,struct fpga_region *region); +int fpga_region_unregister (struct fpga_region *region); +#endif /* _FPGA_REGION_H */
Signature · 1 line
2.7.4
Re: [PATCH v4 13/18] fpga: region: add register/unregister functions
Quoted 196 lines · Alan Tull
Another step in separating common code from device tree specific code for FPGA regions.
* add FPGA region register/unregister functions.
* add the register/unregister functions to the header
* usedevm_kzallocto alloc the region.
* add a method for getting bridges to the region struct
* add priv to the region struct
* useregion->infoinof_fpga_region_get_bridgesSigned-off-by Alan Tull <[email protected]>---
v2: split out from another patch
v3: useregion->info, remove info param where applicable
v4: no change to this patch in this version of patchset--- drivers/fpga/fpga-region.c | 106 ++++++++++++++++++++++++--------------- include/linux/fpga/fpga-region.h | 7 +++ 2 files changed, 72 insertions(+), 41 deletions(-)drivers/fpga/fpga-region.c +64
-38@@ -143,7 +143,6 @@ static struct fpga_manager *of_fpga_region_get_mgr(struct device_node *np)/** * of_fpga_region_get_bridges - create a list of bridges * @region: FPGA region -* @info: FPGA image info * * Create a list of bridges including the parent bridge and the bridges * specified by "fpga-bridges" property. Note that the @@ -156,11 +155,11 @@ static struct fpga_manager *of_fpga_region_get_mgr(struct device_node *np)* Return 0 for success (even if there are no bridges specified) * or -EBUSY if any of the bridges are in use. */ -static int of_fpga_region_get_bridges (struct fpga_region *region, -struct fpga_image_info *info) +static int of_fpga_region_get_bridges (struct fpga_region *region) {struct device *dev = ®ion->dev;struct device_node *region_np = dev->of_node; +struct fpga_image_info *info = region->info;struct device_node *br, *np, *parent_br =NULL ;int i, ret;@@ -192,7 +191,7 @@ static int of_fpga_region_get_bridges(struct fpga_region *region,continue ;/* If node is a bridge, get it and add to list */ - ret =of_fpga_bridge_get_to_list (br, region->info, + ret =of_fpga_bridge_get_to_list (br, info, ®ion->bridge_list);/* If any of the bridges are in use, give up */ @@ -229,10 +228,16 @@ int fpga_region_program_fpga(struct fpga_region *region) goto err_put_region; } - ret =of_fpga_region_get_bridges (region, info); -if (ret) { -dev_err (dev,"failed to get FPGA bridges\n" ); - goto err_unlock_mgr; +/* +* In some cases, we already have a list of bridges in the +* fpga region struct. Or we don't have any bridges. +*/ +if (region->get_bridges) { + ret = region->get_bridges (region); +if (ret) { +dev_err (dev,"failed to get fpga region bridges\n" ); + goto err_unlock_mgr; + } } ret =fpga_bridges_disable (®ion->bridge_list);@@ -259,7 +264,8 @@ int fpga_region_program_fpga(struct fpga_region *region)return 0 ; err_put_br: -fpga_bridges_put (®ion->bridge_list); +if (region->get_bridges) +fpga_bridges_put (®ion->bridge_list); err_unlock_mgr:fpga_mgr_unlock (region->mgr); err_put_region:@@ -522,39 +528,20 @@ static struct notifier_block fpga_region_of_nb = { .notifier_call = of_fpga_region_notify, }; -static int of_fpga_region_probe (struct platform_device *pdev) +int fpga_region_register (struct device *dev,struct fpga_region *region) { -struct device *dev = &pdev->dev; -struct device_node *np = dev->of_node; -struct fpga_region *region; -struct fpga_manager *mgr;int id, ret =0 ; - mgr =of_fpga_region_get_mgr (np); -if (IS_ERR (mgr)) -return -EPROBE_DEFER ; - - region =kzalloc (sizeof (*region),GFP_KERNEL ); -if (!region) { - ret = -ENOMEM ; - goto err_put_mgr; - } - - region->mgr = mgr; - id =ida_simple_get (&fpga_region_ida,0 ,0 ,GFP_KERNEL ); -if (id <0 ) { - ret = id; - goto err_kfree; - } +if (id <0 ) +return id;mutex_init (®ion->mutex);INIT_LIST_HEAD (®ion->bridge_list); -device_initialize (®ion->dev); region->dev.class = fpga_region_class; region->dev.parent = dev; - region->dev.of_node = np; + region->dev.of_node = dev->of_node; region->dev.id = id;dev_set_drvdata (dev, region);@@ -566,19 +553,58 @@ static int of_fpga_region_probe(struct platform_device *pdev)if (ret) goto err_remove; +return 0 ; + +err_remove: +ida_simple_remove (&fpga_region_ida, id); +return ret; +} +EXPORT_SYMBOL_GPL (fpga_region_register); + +int fpga_region_unregister (struct fpga_region *region) +{ +device_unregister (®ion->dev); + +return 0 ; +} +EXPORT_SYMBOL_GPL (fpga_region_unregister); + +static int of_fpga_region_probe (struct platform_device *pdev) +{ +struct device *dev = &pdev->dev; +struct device_node *np = dev->of_node; +struct fpga_region *region; +struct fpga_manager *mgr; +int ret; + +/* Find the FPGA mgr specified by region or parent region. */ + mgr =of_fpga_region_get_mgr (np); +if (IS_ERR (mgr)) +return -EPROBE_DEFER ; + + region =devm_kzalloc (dev,sizeof (*region),GFP_KERNEL ); +if (!region) { + ret = -ENOMEM ; + goto eprobe_mgr_put; + } + + region->mgr = mgr; + +/* Specify how to get bridges for this type of region. */ + region->get_bridges = of_fpga_region_get_bridges; + + ret =fpga_region_register (dev, region); +if (ret) + goto eprobe_mgr_put; +of_platform_populate (np, fpga_region_of_match,NULL , ®ion->dev);dev_info (dev,"FPGA Region probed\n" );return 0 ; -err_remove: -ida_simple_remove (&fpga_region_ida, id); -err_kfree: -kfree (region); -err_put_mgr: +eprobe_mgr_put:fpga_mgr_put (mgr); -return ret; }
drivers/fpga/fpga-region.c static int of_fpga_region_remove(struct platform_device *pdev) · line 615
Quoted 6 lines
drivers/fpga/fpga-region.c +1
-2@@ -586,8 +612,7 @@ static int of_fpga_region_remove(struct platform_device *pdev) {struct fpga_region *region =platform_get_drvdata (pdev); -device_unregister (®ion->dev); -fpga_mgr_put (region->mgr); +fpga_region_unregister (region);
Hi Alan
Do you think if we need to keep fpga_mgr_put(region->mgr) in this remove function? :)
Thanks
Hao
Quoted 47 lines
drivers/fpga/fpga-region.c +0
-1return 0 ; }@@ -606,7 +631,6 @@ static void fpga_region_dev_release(struct device *dev)struct fpga_region *region =to_fpga_region (dev);ida_simple_remove (&fpga_region_ida, region->dev.id); -kfree (region); }/** include/linux/fpga/fpga-region.h +7
-0@@ -12,6 +12,8 @@* @bridge_list: list of FPGA bridges specified in region * @mgr: FPGA manager * @info: FPGA image info +* @priv: private data +* @get_bridges: optional function to get bridges to a list */ struct fpga_region {struct device dev;@@ -19,10 +21,15 @@ struct fpga_region {struct list_head bridge_list;struct fpga_manager *mgr;struct fpga_image_info *info; +void *priv; + int (*get_bridges)(struct fpga_region *region); };#define to_fpga_region (d) container_of(d, struct fpga_region, dev)int fpga_region_program_fpga (struct fpga_region *region); +int fpga_region_register (struct device *dev,struct fpga_region *region); +int fpga_region_unregister (struct fpga_region *region); +#endif /* _FPGA_REGION_H */ Signature · 6 lines
2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-fpga" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 13/18] fpga: region: add register/unregister functions
Quoted 200 lines · Wu Hao
Quoted 199 lines · Alan Tull
Another step in separating common code from device tree specific code for FPGA regions.
* add FPGA region register/unregister functions.
* add the register/unregister functions to the header
* usedevm_kzallocto alloc the region.
* add a method for getting bridges to the region struct
* add priv to the region struct
* useregion->infoinof_fpga_region_get_bridgesSigned-off-by Alan Tull <[email protected]>---
v2: split out from another patch
v3: useregion->info, remove info param where applicable
v4: no change to this patch in this version of patchset--- drivers/fpga/fpga-region.c | 106 ++++++++++++++++++++++++--------------- include/linux/fpga/fpga-region.h | 7 +++ 2 files changed, 72 insertions(+), 41 deletions(-)drivers/fpga/fpga-region.c +64
-38@@ -143,7 +143,6 @@ static struct fpga_manager *of_fpga_region_get_mgr(struct device_node *np)/** * of_fpga_region_get_bridges - create a list of bridges * @region: FPGA region -* @info: FPGA image info * * Create a list of bridges including the parent bridge and the bridges * specified by "fpga-bridges" property. Note that the @@ -156,11 +155,11 @@ static struct fpga_manager *of_fpga_region_get_mgr(struct device_node *np)* Return 0 for success (even if there are no bridges specified) * or -EBUSY if any of the bridges are in use. */ -static int of_fpga_region_get_bridges (struct fpga_region *region, -struct fpga_image_info *info) +static int of_fpga_region_get_bridges (struct fpga_region *region) {struct device *dev = ®ion->dev;struct device_node *region_np = dev->of_node; +struct fpga_image_info *info = region->info;struct device_node *br, *np, *parent_br =NULL ;int i, ret;@@ -192,7 +191,7 @@ static int of_fpga_region_get_bridges(struct fpga_region *region,continue ;/* If node is a bridge, get it and add to list */ - ret =of_fpga_bridge_get_to_list (br, region->info, + ret =of_fpga_bridge_get_to_list (br, info, ®ion->bridge_list);/* If any of the bridges are in use, give up */ @@ -229,10 +228,16 @@ int fpga_region_program_fpga(struct fpga_region *region) goto err_put_region; } - ret =of_fpga_region_get_bridges (region, info); -if (ret) { -dev_err (dev,"failed to get FPGA bridges\n" ); - goto err_unlock_mgr; +/* +* In some cases, we already have a list of bridges in the +* fpga region struct. Or we don't have any bridges. +*/ +if (region->get_bridges) { + ret = region->get_bridges (region); +if (ret) { +dev_err (dev,"failed to get fpga region bridges\n" ); + goto err_unlock_mgr; + } } ret =fpga_bridges_disable (®ion->bridge_list);@@ -259,7 +264,8 @@ int fpga_region_program_fpga(struct fpga_region *region)return 0 ; err_put_br: -fpga_bridges_put (®ion->bridge_list); +if (region->get_bridges) +fpga_bridges_put (®ion->bridge_list); err_unlock_mgr:fpga_mgr_unlock (region->mgr); err_put_region:@@ -522,39 +528,20 @@ static struct notifier_block fpga_region_of_nb = { .notifier_call = of_fpga_region_notify, }; -static int of_fpga_region_probe (struct platform_device *pdev) +int fpga_region_register (struct device *dev,struct fpga_region *region) { -struct device *dev = &pdev->dev; -struct device_node *np = dev->of_node; -struct fpga_region *region; -struct fpga_manager *mgr;int id, ret =0 ; - mgr =of_fpga_region_get_mgr (np); -if (IS_ERR (mgr)) -return -EPROBE_DEFER ; - - region =kzalloc (sizeof (*region),GFP_KERNEL ); -if (!region) { - ret = -ENOMEM ; - goto err_put_mgr; - } - - region->mgr = mgr; - id =ida_simple_get (&fpga_region_ida,0 ,0 ,GFP_KERNEL ); -if (id <0 ) { - ret = id; - goto err_kfree; - } +if (id <0 ) +return id;mutex_init (®ion->mutex);INIT_LIST_HEAD (®ion->bridge_list); -device_initialize (®ion->dev); region->dev.class = fpga_region_class; region->dev.parent = dev; - region->dev.of_node = np; + region->dev.of_node = dev->of_node; region->dev.id = id;dev_set_drvdata (dev, region);@@ -566,19 +553,58 @@ static int of_fpga_region_probe(struct platform_device *pdev)if (ret) goto err_remove; +return 0 ; + +err_remove: +ida_simple_remove (&fpga_region_ida, id); +return ret; +} +EXPORT_SYMBOL_GPL (fpga_region_register); + +int fpga_region_unregister (struct fpga_region *region) +{ +device_unregister (®ion->dev); + +return 0 ; +} +EXPORT_SYMBOL_GPL (fpga_region_unregister); + +static int of_fpga_region_probe (struct platform_device *pdev) +{ +struct device *dev = &pdev->dev; +struct device_node *np = dev->of_node; +struct fpga_region *region; +struct fpga_manager *mgr; +int ret; + +/* Find the FPGA mgr specified by region or parent region. */ + mgr =of_fpga_region_get_mgr (np); +if (IS_ERR (mgr)) +return -EPROBE_DEFER ; + + region =devm_kzalloc (dev,sizeof (*region),GFP_KERNEL ); +if (!region) { + ret = -ENOMEM ; + goto eprobe_mgr_put; + } + + region->mgr = mgr; + +/* Specify how to get bridges for this type of region. */ + region->get_bridges = of_fpga_region_get_bridges; + + ret =fpga_region_register (dev, region); +if (ret) + goto eprobe_mgr_put; +of_platform_populate (np, fpga_region_of_match,NULL , ®ion->dev);dev_info (dev,"FPGA Region probed\n" );return 0 ; -err_remove: -ida_simple_remove (&fpga_region_ida, id); -err_kfree: -kfree (region); -err_put_mgr: +eprobe_mgr_put:fpga_mgr_put (mgr); -return ret; }@@ -586,8 +612,7 @@ static int of_fpga_region_remove(struct platform_device *pdev) {struct fpga_region *region =platform_get_drvdata (pdev);
Quoted 8 lines
drivers/fpga/fpga-region.c static int of_fpga_region_remove(struct platform_device *pdev) · line 615
Quoted 3 lines
drivers/fpga/fpga-region.c +1
-2-device_unregister (®ion->dev); -fpga_mgr_put (region->mgr); +fpga_region_unregister (region);Hi Alan
Do you think if we need to keep
fpga_mgr_put(region->mgr)in this remove function? :)
Hi Hao,
Thanks for catching that! I'll add it back in.
Alan
Quoted 3 lines
Thanks
Hao
PATCH v4 14/18 fpga: region: add fpga_region_class_find
In reply to Alan TullAdd a function for searching the fpga-region class. This will be useful when device tree code is no longer in the same file that declares the fpga-region class. Another step in separating common FPGA region code from device tree support.
Signed-off-by Alan Tull <[email protected]>
Acked-by Moritz Fischer <[email protected]>
---
v2: split out from another patch
v3: add Moritz' ack
v4: no change to this patch in this version of patchset
--- drivers/fpga/fpga-region.c | 23 +++++++++++++++-------- include/linux/fpga/fpga-region.h | 5 ++++- 2 files changed, 19 insertions(+), 9 deletions(-)
drivers/fpga/fpga-region.c +15 -8
@@ -30,6 +30,20 @@static DEFINE_IDA (fpga_region_ida);static struct class *fpga_region_class; +struct fpga_region *fpga_region_class_find ( +struct device *start,const void *data, +int (*match)(struct device *,const void *)) +{ +struct device *dev; + + dev =class_find_device (fpga_region_class, start, data, match); +if (!dev) +return NULL ; + +return to_fpga_region (dev); +} +EXPORT_SYMBOL_GPL (fpga_region_class_find); +static const struct of_device_id fpga_region_of_match[] = { { .compatible ="fpga-region" , }, {},
@@ -51,14 +65,7 @@ static int fpga_region_of_node_match(struct device *dev, const void *data)*/ static struct fpga_region *of_fpga_region_find (struct device_node *np) { -struct device *dev; - - dev =class_find_device (fpga_region_class,NULL , np, - fpga_region_of_node_match); -if (!dev) -return NULL ; - -return to_fpga_region (dev); +return fpga_region_class_find (NULL , np, fpga_region_of_node_match); }/**
include/linux/fpga/fpga-region.h +4 -1
@@ -27,8 +27,11 @@ struct fpga_region {#define to_fpga_region (d) container_of(d, struct fpga_region, dev) -int fpga_region_program_fpga (struct fpga_region *region); +struct fpga_region *fpga_region_class_find ( +struct device *start,const void *data, +int (*match)(struct device *,const void *)); +int fpga_region_program_fpga (struct fpga_region *region);int fpga_region_register (struct device *dev,struct fpga_region *region);int fpga_region_unregister (struct fpga_region *region);
Signature · 1 line
2.7.4
PATCH v4 15/18 fpga: region: move device tree support to of-fpga-region.c
In reply to Alan TullCreate of-fpga-region.c and ove the following functions without modification from fpga-region.c.
* of_fpga_region_find
* of_fpga_region_get_mgr
* of_fpga_region_get_bridges
* child_regions_with_firmware
* of_fpga_region_parse_ov
* of_fpga_region_notify_pre_apply
* of_fpga_region_notify_post_remove
* of_fpga_region_notify
* of_fpga_region_probe
* of_fpga_region_remove
Create two new function with some code from fpga_region_init/exit.
* of_fpga_region_init
* of_fpga_region_exit
Signed-off-by Alan Tull <[email protected]>
---
v2: split out code changes into other patches, only move code here
v3: updated to move changed code to of-fpga-region.c
v4: rebase on current for-next
remove fpga-bridge dependency on CONFIG_OF
--- drivers/fpga/Kconfig | 14 +- drivers/fpga/Makefile | 1 + drivers/fpga/fpga-region.c | 451 +------------------------------------- drivers/fpga/of-fpga-region.c | 495 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 507 insertions(+), 454 deletions(-) create mode 100644 drivers/fpga/of-fpga-region.c
drivers/fpga/Kconfig +10 -4
@@ -13,10 +13,17 @@ if FPGA config FPGA_REGION tristate "FPGA Region" - depends on OF && FPGA_BRIDGE + depends on FPGA_BRIDGE + help + FPGA Region common code. A FPGA Region controls a FPGA Manager + and the FPGA Bridges associated with either a reconfigurable + region of an FPGA or a whole FPGA. + +config OF_FPGA_REGION + tristate "FPGA Region Device Tree Overlay Support" + depends on OF && FPGA_REGION help - FPGA Regions allow loading FPGA images under control of - the Device Tree. + Support for loading FPGA images under control of Device Tree. config FPGA_MGR_ICE40_SPI tristate "Lattice iCE40 SPI"
@@ -74,7 +81,6 @@ config FPGA_MGR_ZYNQ_FPGA config FPGA_BRIDGE tristate "FPGA Bridge Framework" - depends on OF help Say Y here if you want to support bridges connected between host processors and FPGAs or between FPGAs.
drivers/fpga/Makefile +1 -0
@@ -25,3 +25,4 @@ obj-$(CONFIG_XILINX_PR_DECOUPLER) += xilinx-pr-decoupler.o # High Level Interfaces obj-$(CONFIG_FPGA_REGION) += fpga-region.o +obj-$(CONFIG_OF_FPGA_REGION) += of-fpga-region.o
drivers/fpga/fpga-region.c +1 -450
504 lines not on this page · raw message
drivers/fpga/of-fpga-region.c new file +495 -0
495 lines not on this page · raw message
Signature · 1 line
2.7.4
Re: [PATCH v4 15/18] fpga: region: move device tree support to of-fpga-region.c
Hi Alan,
Just a couple of minor nits.
Matthew Gerlach
Quoted 1 line · Alan Tull
Create
of-fpga-region.cand ove the following functions without
s/ove/move/
Quoted 13 lines
modification from
fpga-region.c.*
of_fpga_region_find
*of_fpga_region_get_mgr
*of_fpga_region_get_bridges
*child_regions_with_firmware
*of_fpga_region_parse_ov
*of_fpga_region_notify_pre_apply
*of_fpga_region_notify_post_remove
*of_fpga_region_notify
*of_fpga_region_probe
*of_fpga_region_remove
Quoted 1 line
Create two new function with some code from fpga_region_init/exit.
s/function/functions/
Quoted 37 lines
*
of_fpga_region_init
*of_fpga_region_exitSigned-off-by Alan Tull <[email protected]>---
v2: split out code changes into other patches, only move code here
v3: updated to move changed code toof-fpga-region.c
v4: rebase on current for-next
remove fpga-bridge dependency onCONFIG_OF
---drivers/fpga/Kconfig | 14 +- drivers/fpga/Makefile | 1 + drivers/fpga/fpga-region.c | 451 +------------------------------------- drivers/fpga/of-fpga-region.c | 495 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 507 insertions(+), 454 deletions(-)create mode 100644
drivers/fpga/of-fpga-region.cdrivers/fpga/Kconfig +0
-0@@ -13,10 +13,17 @@ if FPGAconfig
FPGA_REGION
tristate "FPGA Region"drivers/fpga/Kconfig +9
-1- depends on OF && FPGA_BRIDGE + depends on FPGA_BRIDGE + help + FPGA Region common code. A FPGA Region controls a FPGA Manager + and the FPGA Bridges associated with either a reconfigurable + region of an FPGA or a whole FPGA. + +config OF_FPGA_REGION + tristate "FPGA Region Device Tree Overlay Support" + depends on OF && FPGA_REGIONhelp
drivers/fpga/Kconfig if FPGA · line 26
Quoted 3 lines
drivers/fpga/Kconfig +1
-2- FPGA Regions allow loading FPGA images under control of - the Device Tree. + Support for loading FPGA images under control of Device Tree.
Support for loading FPGA images by applying a Device Tree Overlay.
Quoted 905 lines
config
FPGA_MGR_ICE40_SPI
tristate "Lattice iCE40 SPI"drivers/fpga/Kconfig +0
-0@@ -74,7 +81,6 @@ config FPGA_MGR_ZYNQ_FPGAconfig
FPGA_BRIDGE
tristate "FPGA Bridge Framework"drivers/fpga/Kconfig +0
-1- depends on OFhelp
Say Y here if you want to support bridges connected between host processors and FPGAs or between FPGAs.drivers/fpga/Makefile +0
-0@@ -25,3 +25,4 @@ obj-$(CONFIG_XILINX_PR_DECOUPLER) += xilinx-pr-decoupler.o # High Level Interfacesobj-$(CONFIG_FPGA_REGION) += fpga-region.o
drivers/fpga/Makefile +1
-0+obj-$(CONFIG_OF_FPGA_REGION) += of-fpga-region.odrivers/fpga/fpga-region.c +1
-450@@ -2,6 +2,7 @@* FPGA Region - Device Tree support for FPGA programming under Linux * * Copyright (C) 2013-2016 Altera Corporation +* Copyright (C) 2017 Intel Corporation * * This program is free software; you can redistribute it and/or modify it * under the terms and conditions of the GNU General Public License, @@ -23,7 +24,6 @@#include <linux/kernel.h> #include <linux/list.h> #include <linux/module.h> -#include <linux/of_platform.h> #include <linux/slab.h> #include <linux/spinlock.h> @@ -44,30 +44,6 @@ struct fpga_region *fpga_region_class_find( }EXPORT_SYMBOL_GPL (fpga_region_class_find); -static const struct of_device_id fpga_region_of_match[] = { - { .compatible ="fpga-region" , }, - {}, -}; -MODULE_DEVICE_TABLE (of, fpga_region_of_match); - -static int fpga_region_of_node_match (struct device *dev,const void *data) -{ -return dev->of_node == data; -} - -/** -* of_fpga_region_find - find FPGA region -* @np: device node of FPGA Region -* -* Caller will need to put_device(®ion->dev) when done. -* -* Returns FPGA Region struct or NULL -*/ -static struct fpga_region *of_fpga_region_find (struct device_node *np) -{ -return fpga_region_class_find (NULL , np, fpga_region_of_node_match); -} -/** * fpga_region_get - get an exclusive reference to a fpga region * @region: FPGA Region struct @@ -116,102 +92,6 @@ static void fpga_region_put(struct fpga_region *region) }/** -* of_fpga_region_get_mgr - get reference for FPGA manager -* @np: device node of FPGA region -* -* Get FPGA Manager from "fpga-mgr" property or from ancestor region. -* -* Caller should call fpga_mgr_put() when done with manager. -* -* Return: fpga manager struct or IS_ERR() condition containing error code. -*/ -static struct fpga_manager *of_fpga_region_get_mgr (struct device_node *np) -{ -struct device_node *mgr_node; -struct fpga_manager *mgr; - -of_node_get (np); -while (np) { -if (of_device_is_compatible (np,"fpga-region" )) { - mgr_node =of_parse_phandle (np,"fpga-mgr" ,0 ); -if (mgr_node) { - mgr =of_fpga_mgr_get (mgr_node); -of_node_put (np); -return mgr; - } - } - np =of_get_next_parent (np); - } -of_node_put (np); - -return ERR_PTR (-EINVAL ); -} - -/** -* of_fpga_region_get_bridges - create a list of bridges -* @region: FPGA region -* -* Create a list of bridges including the parent bridge and the bridges -* specified by "fpga-bridges" property. Note that the -* fpga_bridges_enable/disable/put functions are all fine with an empty list -* if that happens. -* -* Caller should call fpga_bridges_put(®ion->bridge_list) when -* done with the bridges. -* -* Return 0 for success (even if there are no bridges specified) -* or -EBUSY if any of the bridges are in use. -*/ -static int of_fpga_region_get_bridges (struct fpga_region *region) -{ -struct device *dev = ®ion->dev; -struct device_node *region_np = dev->of_node; -struct fpga_image_info *info = region->info; -struct device_node *br, *np, *parent_br =NULL ; -int i, ret; - -/* If parent is a bridge, add to list */ - ret =of_fpga_bridge_get_to_list (region_np->parent, info, - ®ion->bridge_list); - -/* -EBUSY means parent is a bridge that is under use. Give up. */ -if (ret == -EBUSY ) -return ret; - -/* Zero return code means parent was a bridge and was added to list. */ -if (!ret) - parent_br = region_np->parent; - -/* If overlay has a list of bridges, use it. */ -if (of_parse_phandle (info->overlay,"fpga-bridges" ,0 )) - np = info->overlay; -else - np = region_np; - -for (i =0 ; ; i++) { - br =of_parse_phandle (np,"fpga-bridges" , i); -if (!br) -break ; - -/* If parent bridge is in list, skip it. */ -if (br == parent_br) -continue ; - -/* If node is a bridge, get it and add to list */ - ret =of_fpga_bridge_get_to_list (br, info, - ®ion->bridge_list); - -/* If any of the bridges are in use, give up */ -if (ret == -EBUSY ) { -fpga_bridges_put (®ion->bridge_list); -return -EBUSY ; - } - } - -return 0 ; -} - -/** * fpga_region_program_fpga - program FPGA * @region: FPGA region * Program an FPGA using fpga image info (region->info). @@ -282,259 +162,6 @@ int fpga_region_program_fpga(struct fpga_region *region) }EXPORT_SYMBOL_GPL (fpga_region_program_fpga); -/** -* child_regions_with_firmware -* @overlay: device node of the overlay -* -* If the overlay adds child FPGA regions, they are not allowed to have -* firmware-name property. -* -* Return 0 for OK or -EINVAL if child FPGA region adds firmware-name. -*/ -static int child_regions_with_firmware (struct device_node *overlay) -{ -struct device_node *child_region; -const char *child_firmware_name; -int ret =0 ; - -of_node_get (overlay); - - child_region =of_find_matching_node (overlay, fpga_region_of_match); -while (child_region) { -if (!of_property_read_string (child_region,"firmware-name" , - &child_firmware_name)) { - ret = -EINVAL ; -break ; - } - child_region =of_find_matching_node (child_region, - fpga_region_of_match); - } - -of_node_put (child_region); - -if (ret) -pr_err ("firmware-name not allowed in child FPGA region: %pOF" , - child_region); - -return ret; -} - -/** -* of_fpga_region_parse_ov - parse and check overlay applied to region -* -* @region: FPGA region -* @overlay: overlay applied to the FPGA region -* -* Given an overlay applied to a FPGA region, parse the FPGA image specific -* info in the overlay and do some checking. -* -* Returns: -* NULL if overlay doesn't direct us to program the FPGA. -* fpga_image_info struct if there is an image to program. -* error code for invalid overlay. -*/ -static struct fpga_image_info *of_fpga_region_parse_ov ( -struct fpga_region *region, -struct device_node *overlay) -{ -struct device *dev = ®ion->dev; -struct fpga_image_info *info; -const char *firmware_name; -int ret; - -if (region->info) { -dev_err (dev,"Region already has overlay applied.\n" ); -return ERR_PTR (-EINVAL ); - } - -/* -* Reject overlay if child FPGA Regions added in the overlay have -* firmware-name property (would mean that an FPGA region that has -* not been added to the live tree yet is doing FPGA programming). -*/ - ret =child_regions_with_firmware (overlay); -if (ret) -return ERR_PTR (ret); - - info =fpga_image_info_alloc (dev); -if (!info) -return ERR_PTR (-ENOMEM ); - - info->overlay = overlay; - -/* Read FPGA region properties from the overlay */ -if (of_property_read_bool (overlay,"partial-fpga-config" )) - info->flags |=FPGA_MGR_PARTIAL_RECONFIG ; - -if (of_property_read_bool (overlay,"external-fpga-config" )) - info->flags |=FPGA_MGR_EXTERNAL_CONFIG ; - -if (of_property_read_bool (overlay,"encrypted-fpga-config" )) - info->flags |=FPGA_MGR_ENCRYPTED_BITSTREAM ; - -if (!of_property_read_string (overlay,"firmware-name" , - &firmware_name)) { - info->firmware_name =devm_kstrdup (dev, firmware_name, -GFP_KERNEL ); -if (!info->firmware_name) -return ERR_PTR (-ENOMEM ); - } - -of_property_read_u32 (overlay,"region-unfreeze-timeout-us" , - &info->enable_timeout_us); - -of_property_read_u32 (overlay,"region-freeze-timeout-us" , - &info->disable_timeout_us); - -of_property_read_u32 (overlay,"config-complete-timeout-us" , - &info->config_complete_timeout_us); - -/* If overlay is not programming the FPGA, don't need FPGA image info */ -if (!info->firmware_name) { - ret =0 ; - goto ret_no_info; - } - -/* -* If overlay informs us FPGA was externally programmed, specifying -* firmware here would be ambiguous. -*/ -if (info->flags &FPGA_MGR_EXTERNAL_CONFIG ) { -dev_err (dev,"error: specified firmware and external-fpga-config" ); - ret = -EINVAL ; - goto ret_no_info; - } - -return info; -ret_no_info: -fpga_image_info_free (info); -return ERR_PTR (ret); -} - -/** -* of_fpga_region_notify_pre_apply - pre-apply overlay notification -* -* @region: FPGA region that the overlay was applied to -* @nd: overlay notification data -* -* Called when an overlay targeted to a FPGA Region is about to be applied. -* Parses the overlay for properties that influence how the FPGA will be -* programmed and does some checking. If the checks pass, programs the FPGA. -* If the checks fail, overlay is rejected and does not get added to the -* live tree. -* -* Returns 0 for success or negative error code for failure. -*/ -static int of_fpga_region_notify_pre_apply (struct fpga_region *region, -struct of_overlay_notify_data *nd) -{ -struct device *dev = ®ion->dev; -struct fpga_image_info *info; -int ret; - -if (region->info) { -dev_err (dev,"Region already has overlay applied.\n" ); -return -EINVAL ; - } - - info =of_fpga_region_parse_ov (region, nd->overlay); -if (IS_ERR (info)) -return PTR_ERR (info); - -if (!info) -return 0 ; - - region->info = info; - ret =fpga_region_program_fpga (region); -if (ret) { -/* error; reject overlay */ -fpga_image_info_free (info); - region->info =NULL ; - } - -return ret; -} - -/** -* of_fpga_region_notify_post_remove - post-remove overlay notification -* -* @region: FPGA region that was targeted by the overlay that was removed -* @nd: overlay notification data -* -* Called after an overlay has been removed if the overlay's target was a -* FPGA region. -*/ -static void of_fpga_region_notify_post_remove (struct fpga_region *region, -struct of_overlay_notify_data *nd) -{ -fpga_bridges_disable (®ion->bridge_list); -fpga_bridges_put (®ion->bridge_list); -fpga_image_info_free (region->info); - region->info =NULL ; -} - -/** -* of_fpga_region_notify - reconfig notifier for dynamic DT changes -* @nb: notifier block -* @action: notifier action -* @arg: reconfig data -* -* This notifier handles programming a FPGA when a "firmware-name" property is -* added to a fpga-region. -* -* Returns NOTIFY_OK or error if FPGA programming fails. -*/ -static int of_fpga_region_notify (struct notifier_block *nb, -unsigned long action,void *arg) -{ -struct of_overlay_notify_data *nd = arg; -struct fpga_region *region; -int ret; - -switch (action) { -case OF_OVERLAY_PRE_APPLY : -pr_debug ("%s OF_OVERLAY_PRE_APPLY\n" , __func__); -break ; -case OF_OVERLAY_POST_APPLY : -pr_debug ("%s OF_OVERLAY_POST_APPLY\n" , __func__); -return NOTIFY_OK ;/* not for us */ -case OF_OVERLAY_PRE_REMOVE : -pr_debug ("%s OF_OVERLAY_PRE_REMOVE\n" , __func__); -return NOTIFY_OK ;/* not for us */ -case OF_OVERLAY_POST_REMOVE : -pr_debug ("%s OF_OVERLAY_POST_REMOVE\n" , __func__); -break ; -default :/* should not happen */ -return NOTIFY_OK ; - } - - region =of_fpga_region_find (nd->target); -if (!region) -return NOTIFY_OK ; - - ret =0 ; -switch (action) { -case OF_OVERLAY_PRE_APPLY : - ret =of_fpga_region_notify_pre_apply (region, nd); -break ; - -case OF_OVERLAY_POST_REMOVE : -of_fpga_region_notify_post_remove (region, nd); -break ; - } - -put_device (®ion->dev); - -if (ret) -return notifier_from_errno (ret); - -return NOTIFY_OK ; -} - -static struct notifier_block fpga_region_of_nb = { - .notifier_call = of_fpga_region_notify, -}; -int fpga_region_register (struct device *dev,struct fpga_region *region) {int id, ret =0 ;@@ -576,63 +203,6 @@ int fpga_region_unregister(struct fpga_region *region) }EXPORT_SYMBOL_GPL (fpga_region_unregister); -static int of_fpga_region_probe (struct platform_device *pdev) -{ -struct device *dev = &pdev->dev; -struct device_node *np = dev->of_node; -struct fpga_region *region; -struct fpga_manager *mgr; -int ret; - -/* Find the FPGA mgr specified by region or parent region. */ - mgr =of_fpga_region_get_mgr (np); -if (IS_ERR (mgr)) -return -EPROBE_DEFER ; - - region =devm_kzalloc (dev,sizeof (*region),GFP_KERNEL ); -if (!region) { - ret = -ENOMEM ; - goto eprobe_mgr_put; - } - - region->mgr = mgr; - -/* Specify how to get bridges for this type of region. */ - region->get_bridges = of_fpga_region_get_bridges; - - ret =fpga_region_register (dev, region); -if (ret) - goto eprobe_mgr_put; - -of_platform_populate (np, fpga_region_of_match,NULL , ®ion->dev); - -dev_info (dev,"FPGA Region probed\n" ); - -return 0 ; - -eprobe_mgr_put: -fpga_mgr_put (mgr); -return ret; -} - -static int of_fpga_region_remove (struct platform_device *pdev) -{ -struct fpga_region *region =platform_get_drvdata (pdev); - -fpga_region_unregister (region); - -return 0 ; -} - -static struct platform_driver of_fpga_region_driver = { - .probe = of_fpga_region_probe, - .remove = of_fpga_region_remove, - .driver = { - .name ="fpga-region" , - .of_match_table =of_match_ptr (fpga_region_of_match), - }, -}; -static void fpga_region_dev_release (struct device *dev) {struct fpga_region *region =to_fpga_region (dev);@@ -646,36 +216,17 @@ static void fpga_region_dev_release(struct device *dev)*/ static int __init fpga_region_init (void ) { -int ret; - fpga_region_class =class_create (THIS_MODULE ,"fpga_region" );if (IS_ERR (fpga_region_class))return PTR_ERR (fpga_region_class); fpga_region_class->dev_release = fpga_region_dev_release; - ret =of_overlay_notifier_register (&fpga_region_of_nb); -if (ret) - goto err_class; - - ret =platform_driver_register (&of_fpga_region_driver); -if (ret) - goto err_plat; -return 0 ; - -err_plat: -of_overlay_notifier_unregister (&fpga_region_of_nb); -err_class: -class_destroy (fpga_region_class); -ida_destroy (&fpga_region_ida); -return ret; } static void __exit fpga_region_exit(void) { -platform_driver_unregister (&of_fpga_region_driver); -of_overlay_notifier_unregister (&fpga_region_of_nb);class_destroy (fpga_region_class);ida_destroy (&fpga_region_ida); }drivers/fpga/of-fpga-region.c new file +364
-0@@ -0,0 +1,495 @@ +/* +* FPGA Region - Device Tree support for FPGA programming under Linux +* +* Copyright (C) 2013-2016 Altera Corporation +* Copyright (C) 2017 Intel Corporation +* +* This program is free software; you can redistribute it and/or modify it +* under the terms and conditions of the GNU General Public License, +* version 2, as published by the Free Software Foundation. +* +* This program is distributed in the hope it will be useful, but WITHOUT +* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +* more details. +* +* You should have received a copy of the GNU General Public License along with +* this program. If not, see <http://www.gnu.org/licenses/>. +*/ + +#include <linux/fpga/fpga-bridge.h> +#include <linux/fpga/fpga-mgr.h> +#include <linux/fpga/fpga-region.h> +#include <linux/idr.h> +#include <linux/kernel.h> +#include <linux/list.h> +#include <linux/module.h> +#include <linux/of_platform.h> +#include <linux/slab.h> +#include <linux/spinlock.h> + +static const struct of_device_id fpga_region_of_match[] = { + { .compatible ="fpga-region" , }, + {}, +}; +MODULE_DEVICE_TABLE (of, fpga_region_of_match); + +static int fpga_region_of_node_match (struct device *dev,const void *data) +{ +return dev->of_node == data; +} + +/** +* of_fpga_region_find - find FPGA region +* @np: device node of FPGA Region +* +* Caller will need to put_device(®ion->dev) when done. +* +* Returns FPGA Region struct or NULL +*/ +static struct fpga_region *of_fpga_region_find (struct device_node *np) +{ +return fpga_region_class_find (NULL , np, fpga_region_of_node_match); +} + +/** +* of_fpga_region_get_mgr - get reference for FPGA manager +* @np: device node of FPGA region +* +* Get FPGA Manager from "fpga-mgr" property or from ancestor region. +* +* Caller should call fpga_mgr_put() when done with manager. +* +* Return: fpga manager struct or IS_ERR() condition containing error code. +*/ +static struct fpga_manager *of_fpga_region_get_mgr (struct device_node *np) +{ +struct device_node *mgr_node; +struct fpga_manager *mgr; + +of_node_get (np); +while (np) { +if (of_device_is_compatible (np,"fpga-region" )) { + mgr_node =of_parse_phandle (np,"fpga-mgr" ,0 ); +if (mgr_node) { + mgr =of_fpga_mgr_get (mgr_node); +of_node_put (np); +return mgr; + } + } + np =of_get_next_parent (np); + } +of_node_put (np); + +return ERR_PTR (-EINVAL ); +} + +/** +* of_fpga_region_get_bridges - create a list of bridges +* @region: FPGA region +* +* Create a list of bridges including the parent bridge and the bridges +* specified by "fpga-bridges" property. Note that the +* fpga_bridges_enable/disable/put functions are all fine with an empty list +* if that happens. +* +* Caller should call fpga_bridges_put(®ion->bridge_list) when +* done with the bridges. +* +* Return 0 for success (even if there are no bridges specified) +* or -EBUSY if any of the bridges are in use. +*/ +static int of_fpga_region_get_bridges (struct fpga_region *region) +{ +struct device *dev = ®ion->dev; +struct device_node *region_np = dev->of_node; +struct fpga_image_info *info = region->info; +struct device_node *br, *np, *parent_br =NULL ; +int i, ret; + +/* If parent is a bridge, add to list */ + ret =of_fpga_bridge_get_to_list (region_np->parent, info, + ®ion->bridge_list); + +/* -EBUSY means parent is a bridge that is under use. Give up. */ +if (ret == -EBUSY ) +return ret; + +/* Zero return code means parent was a bridge and was added to list. */ +if (!ret) + parent_br = region_np->parent; + +/* If overlay has a list of bridges, use it. */ +if (of_parse_phandle (info->overlay,"fpga-bridges" ,0 )) + np = info->overlay; +else + np = region_np; + +for (i =0 ; ; i++) { + br =of_parse_phandle (np,"fpga-bridges" , i); +if (!br) +break ; + +/* If parent bridge is in list, skip it. */ +if (br == parent_br) +continue ; + +/* If node is a bridge, get it and add to list */ + ret =of_fpga_bridge_get_to_list (br, info, + ®ion->bridge_list); + +/* If any of the bridges are in use, give up */ +if (ret == -EBUSY ) { +fpga_bridges_put (®ion->bridge_list); +return -EBUSY ; + } + } + +return 0 ; +} + +/** +* child_regions_with_firmware +* @overlay: device node of the overlay +* +* If the overlay adds child FPGA regions, they are not allowed to have +* firmware-name property. +* +* Return 0 for OK or -EINVAL if child FPGA region adds firmware-name. +*/ +static int child_regions_with_firmware (struct device_node *overlay) +{ +struct device_node *child_region; +const char *child_firmware_name; +int ret =0 ; + +of_node_get (overlay); + + child_region =of_find_matching_node (overlay, fpga_region_of_match); +while (child_region) { +if (!of_property_read_string (child_region,"firmware-name" , + &child_firmware_name)) { + ret = -EINVAL ; +break ; + } + child_region =of_find_matching_node (child_region, + fpga_region_of_match); + } + +of_node_put (child_region); + +if (ret) +pr_err ("firmware-name not allowed in child FPGA region: %pOF" , + child_region); + +return ret; +} + +/** +* of_fpga_region_parse_ov - parse and check overlay applied to region +* +* @region: FPGA region +* @overlay: overlay applied to the FPGA region +* +* Given an overlay applied to a FPGA region, parse the FPGA image specific +* info in the overlay and do some checking. +* +* Returns: +* NULL if overlay doesn't direct us to program the FPGA. +* fpga_image_info struct if there is an image to program. +* error code for invalid overlay. +*/ +static struct fpga_image_info *of_fpga_region_parse_ov ( +struct fpga_region *region, +struct device_node *overlay) +{ +struct device *dev = ®ion->dev; +struct fpga_image_info *info; +const char *firmware_name; +int ret; + +if (region->info) { +dev_err (dev,"Region already has overlay applied.\n" ); +return ERR_PTR (-EINVAL ); + } + +/* +* Reject overlay if child FPGA Regions added in the overlay have +* firmware-name property (would mean that an FPGA region that has +* not been added to the live tree yet is doing FPGA programming). +*/ + ret =child_regions_with_firmware (overlay); +if (ret) +return ERR_PTR (ret); + + info =fpga_image_info_alloc (dev); +if (!info) +return ERR_PTR (-ENOMEM ); + + info->overlay = overlay; + +/* Read FPGA region properties from the overlay */ +if (of_property_read_bool (overlay,"partial-fpga-config" )) + info->flags |=FPGA_MGR_PARTIAL_RECONFIG ; + +if (of_property_read_bool (overlay,"external-fpga-config" )) + info->flags |=FPGA_MGR_EXTERNAL_CONFIG ; + +if (of_property_read_bool (overlay,"encrypted-fpga-config" )) + info->flags |=FPGA_MGR_ENCRYPTED_BITSTREAM ; + +if (!of_property_read_string (overlay,"firmware-name" , + &firmware_name)) { + info->firmware_name =devm_kstrdup (dev, firmware_name, +GFP_KERNEL ); +if (!info->firmware_name) +return ERR_PTR (-ENOMEM ); + } + +of_property_read_u32 (overlay,"region-unfreeze-timeout-us" , + &info->enable_timeout_us); + +of_property_read_u32 (overlay,"region-freeze-timeout-us" , + &info->disable_timeout_us); + +of_property_read_u32 (overlay,"config-complete-timeout-us" , + &info->config_complete_timeout_us); + +/* If overlay is not programming the FPGA, don't need FPGA image info */ +if (!info->firmware_name) { + ret =0 ; + goto ret_no_info; + } + +/* +* If overlay informs us FPGA was externally programmed, specifying +* firmware here would be ambiguous. +*/ +if (info->flags &FPGA_MGR_EXTERNAL_CONFIG ) { +dev_err (dev,"error: specified firmware and external-fpga-config" ); + ret = -EINVAL ; + goto ret_no_info; + } + +return info; +ret_no_info: +fpga_image_info_free (info); +return ERR_PTR (ret); +} + +/** +* of_fpga_region_notify_pre_apply - pre-apply overlay notification +* +* @region: FPGA region that the overlay was applied to +* @nd: overlay notification data +* +* Called when an overlay targeted to a FPGA Region is about to be applied. +* Parses the overlay for properties that influence how the FPGA will be +* programmed and does some checking. If the checks pass, programs the FPGA. +* If the checks fail, overlay is rejected and does not get added to the +* live tree. +* +* Returns 0 for success or negative error code for failure. +*/ +static int of_fpga_region_notify_pre_apply (struct fpga_region *region, +struct of_overlay_notify_data *nd) +{ +struct device *dev = ®ion->dev; +struct fpga_image_info *info; +int ret; + +if (region->info) { +dev_err (dev,"Region already has overlay applied.\n" ); +return -EINVAL ; + } + + info =of_fpga_region_parse_ov (region, nd->overlay); +if (IS_ERR (info)) +return PTR_ERR (info); + +if (!info) +return 0 ; + + region->info = info; + ret =fpga_region_program_fpga (region); +if (ret) { +/* error; reject overlay */ +fpga_image_info_free (info); + region->info =NULL ; + } + +return ret; +} + +/** +* of_fpga_region_notify_post_remove - post-remove overlay notification +* +* @region: FPGA region that was targeted by the overlay that was removed +* @nd: overlay notification data +* +* Called after an overlay has been removed if the overlay's target was a +* FPGA region. +*/ +static void of_fpga_region_notify_post_remove (struct fpga_region *region, +struct of_overlay_notify_data *nd) +{ +fpga_bridges_disable (®ion->bridge_list); +fpga_bridges_put (®ion->bridge_list); +fpga_image_info_free (region->info); + region->info =NULL ; +} + +/** +* of_fpga_region_notify - reconfig notifier for dynamic DT changes +* @nb: notifier block +* @action: notifier action +* @arg: reconfig data +* +* This notifier handles programming a FPGA when a "firmware-name" property is +* added to a fpga-region. +* +* Returns NOTIFY_OK or error if FPGA programming fails. +*/ +static int of_fpga_region_notify (struct notifier_block *nb, +unsigned long action,void *arg) +{ +struct of_overlay_notify_data *nd = arg; +struct fpga_region *region; +int ret; + +switch (action) { +case OF_OVERLAY_PRE_APPLY : +pr_debug ("%s OF_OVERLAY_PRE_APPLY\n" , __func__); +break ; +case OF_OVERLAY_POST_APPLY :
drivers/fpga/of-fpga-region.c line 374
Quoted 10 lines
drivers/fpga/of-fpga-region.c new file +10
-0+pr_debug ("%s OF_OVERLAY_POST_APPLY\n" , __func__); +return NOTIFY_OK ;/* not for us */ +case OF_OVERLAY_PRE_REMOVE : +pr_debug ("%s OF_OVERLAY_PRE_REMOVE\n" , __func__); +return NOTIFY_OK ;/* not for us */ +case OF_OVERLAY_POST_REMOVE : +pr_debug ("%s OF_OVERLAY_POST_REMOVE\n" , __func__); +break ; +default :/* should not happen */ +return NOTIFY_OK ;
If this should not happen, then shouldn't an error be returned?
Quoted 129 lines
drivers/fpga/of-fpga-region.c new file +121
-0+ } + + region =of_fpga_region_find (nd->target); +if (!region) +return NOTIFY_OK ; + + ret =0 ; +switch (action) { +case OF_OVERLAY_PRE_APPLY : + ret =of_fpga_region_notify_pre_apply (region, nd); +break ; + +case OF_OVERLAY_POST_REMOVE : +of_fpga_region_notify_post_remove (region, nd); +break ; + } + +put_device (®ion->dev); + +if (ret) +return notifier_from_errno (ret); + +return NOTIFY_OK ; +} + +static struct notifier_block fpga_region_of_nb = { + .notifier_call = of_fpga_region_notify, +}; + +static int of_fpga_region_probe (struct platform_device *pdev) +{ +struct device *dev = &pdev->dev; +struct device_node *np = dev->of_node; +struct fpga_region *region; +struct fpga_manager *mgr; +int ret; + +/* Find the FPGA mgr specified by region or parent region. */ + mgr =of_fpga_region_get_mgr (np); +if (IS_ERR (mgr)) +return -EPROBE_DEFER ; + + region =devm_kzalloc (dev,sizeof (*region),GFP_KERNEL ); +if (!region) { + ret = -ENOMEM ; + goto eprobe_mgr_put; + } + + region->mgr = mgr; + +/* Specify how to get bridges for this type of region. */ + region->get_bridges = of_fpga_region_get_bridges; + + ret =fpga_region_register (dev, region); +if (ret) + goto eprobe_mgr_put; + +of_platform_populate (np, fpga_region_of_match,NULL , ®ion->dev); + +dev_info (dev,"FPGA Region probed\n" ); + +return 0 ; + +eprobe_mgr_put: +fpga_mgr_put (mgr); +return ret; +} + +static int of_fpga_region_remove (struct platform_device *pdev) +{ +struct fpga_region *region =platform_get_drvdata (pdev); + +fpga_region_unregister (region); + +return 0 ; +} + +static struct platform_driver of_fpga_region_driver = { + .probe = of_fpga_region_probe, + .remove = of_fpga_region_remove, + .driver = { + .name ="of-fpga-region" , + .of_match_table =of_match_ptr (fpga_region_of_match), + }, +}; + +/** +* fpga_region_init - init function for fpga_region class +* Creates the fpga_region class and registers a reconfig notifier. +*/ +static int __init of_fpga_region_init (void ) +{ +int ret; + + ret =of_overlay_notifier_register (&fpga_region_of_nb); +if (ret) +return ret; + + ret =platform_driver_register (&of_fpga_region_driver); +if (ret) + goto err_plat; + +return 0 ; + +err_plat: +of_overlay_notifier_unregister (&fpga_region_of_nb); +return ret; +} + +static void __exit of_fpga_region_exit(void) +{ +platform_driver_unregister (&of_fpga_region_driver); +of_overlay_notifier_unregister (&fpga_region_of_nb); +} + +subsys_initcall (of_fpga_region_init); +module_exit (of_fpga_region_exit); + +MODULE_DESCRIPTION ("FPGA Region" ); +MODULE_AUTHOR ("Alan Tull <[email protected]>" ); +MODULE_LICENSE ("GPL v2" );Signature · 6 lines
2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-fpga" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 15/18] fpga: region: move device tree support to of-fpga-region.c
On Thu, Sep 14, 2017 at 10:50 AM, <[email protected]> wrote:
Hi Matthew,
Thanks for the review!
Quoted 5 lines
Hi Alan,
Just a couple of minor nits.
Quoted 8 lines
Matthew Gerlach
Quoted 1 line · Alan Tull
Create
of-fpga-region.cand ove the following functions withouts/ove/move/
OK
Quoted 14 lines
Quoted 14 lines
modification from
fpga-region.c.*
of_fpga_region_find
*of_fpga_region_get_mgr
*of_fpga_region_get_bridges
*child_regions_with_firmware
*of_fpga_region_parse_ov
*of_fpga_region_notify_pre_apply
*of_fpga_region_notify_post_remove
*of_fpga_region_notify
*of_fpga_region_probe
*of_fpga_region_remove
Quoted 3 lines
Quoted 1 line
Create two new function with some code from fpga_region_init/exit.
s/function/functions/
OK
Quoted 37 lines
Quoted 37 lines
*
of_fpga_region_init
*of_fpga_region_exitSigned-off-by Alan Tull <[email protected]>---
v2: split out code changes into other patches, only move code here
v3: updated to move changed code toof-fpga-region.c
v4: rebase on current for-next
remove fpga-bridge dependency onCONFIG_OF
---drivers/fpga/Kconfig | 14 +- drivers/fpga/Makefile | 1 + drivers/fpga/fpga-region.c | 451 +------------------------------------- drivers/fpga/of-fpga-region.c | 495 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 507 insertions(+), 454 deletions(-)create mode 100644
drivers/fpga/of-fpga-region.cdrivers/fpga/Kconfig +0
-0@@ -13,10 +13,17 @@ if FPGAconfig
FPGA_REGION
tristate "FPGA Region"drivers/fpga/Kconfig +8
-1- depends on OF && FPGA_BRIDGE + depends on FPGA_BRIDGE + help + FPGA Region common code. A FPGA Region controls a FPGA Manager + and the FPGA Bridges associated with either a reconfigurable + region of an FPGA or a whole FPGA. + +config OF_FPGA_REGION + tristate "FPGA Region Device Tree Overlay Support"
Quoted 8 lines
drivers/fpga/Kconfig if FPGA · line 26
Quoted 5 lines
drivers/fpga/Kconfig +2
-2+ depends on OF && FPGA_REGION help - FPGA Regions allow loading FPGA images under control of - the Device Tree. + Support for loading FPGA images under control of Device Tree.Support for loading FPGA images by applying a Device Tree Overlay.
OK
Quoted 962 lines
Quoted 962 lines
config
FPGA_MGR_ICE40_SPI
tristate "Lattice iCE40 SPI"drivers/fpga/Kconfig +0
-0@@ -74,7 +81,6 @@ config FPGA_MGR_ZYNQ_FPGAconfig
FPGA_BRIDGE
tristate "FPGA Bridge Framework"drivers/fpga/Kconfig +0
-1- depends on OF help Say Y here if you want to support bridges connected between host processors and FPGAs or between FPGAs.drivers/fpga/Makefile +0
-0@@ -25,3 +25,4 @@ obj-$(CONFIG_XILINX_PR_DECOUPLER) +=xilinx-pr-decoupler.o
# High Level Interfaces
obj-$(CONFIG_FPGA_REGION) += fpga-region.odrivers/fpga/Makefile +1
-0+obj-$(CONFIG_OF_FPGA_REGION) += of-fpga-region.odrivers/fpga/fpga-region.c +1
-25@@ -2,6 +2,7 @@* FPGA Region - Device Tree support for FPGA programming under Linux * * Copyright (C) 2013-2016 Altera Corporation +* Copyright (C) 2017 Intel Corporation * * This program is free software; you can redistribute it and/or modify it * under the terms and conditions of the GNU General Public License, @@ -23,7 +24,6 @@#include <linux/kernel.h> #include <linux/list.h> #include <linux/module.h> -#include <linux/of_platform.h> #include <linux/slab.h> #include <linux/spinlock.h> @@ -44,30 +44,6 @@ struct fpga_region *fpga_region_class_find( }EXPORT_SYMBOL_GPL (fpga_region_class_find); -static const struct of_device_id fpga_region_of_match[] = { - { .compatible ="fpga-region" , }, - {}, -}; -MODULE_DEVICE_TABLE (of, fpga_region_of_match); - -static int fpga_region_of_node_match (struct device *dev,const void *data) -{ -return dev->of_node == data; -} - -/** -* of_fpga_region_find - find FPGA region -* @np: device node of FPGA Region -* -* Caller will need to put_device(®ion->dev) when done. -* -* Returns FPGA Region struct or NULL -*/ -static struct fpga_region *of_fpga_region_find (struct device_node *np) -{ -return fpga_region_class_find (NULL , np, fpga_region_of_node_match); -} -/** *
fpga_region_get- get an exclusive reference to a fpga region
* @region: FPGA Region structdrivers/fpga/fpga-region.c +0
-8@@ -116,102 +92,6 @@ static void fpga_region_put(struct fpga_region *region) }/** -* of_fpga_region_get_mgr - get reference for FPGA manager -* @np: device node of FPGA region -* -* Get FPGA Manager from "fpga-mgr" property or from ancestor region. -* -* Caller should call fpga_mgr_put() when done with manager. -* -* Return: fpga manager struct or IS_ERR() condition containing error code.
- */
-static structfpga_manager*of_fpga_region_get_mgr(structdevice_node*np)drivers/fpga/fpga-region.c +0
-28-{ -struct device_node *mgr_node; -struct fpga_manager *mgr; - -of_node_get (np); -while (np) { -if (of_device_is_compatible (np,"fpga-region" )) { - mgr_node =of_parse_phandle (np,"fpga-mgr" ,0 ); -if (mgr_node) { - mgr =of_fpga_mgr_get (mgr_node); -of_node_put (np); -return mgr; - } - } - np =of_get_next_parent (np); - } -of_node_put (np); - -return ERR_PTR (-EINVAL ); -} - -/** -* of_fpga_region_get_bridges - create a list of bridges -* @region: FPGA region -* -* Create a list of bridges including the parent bridge and the bridges -* specified by "fpga-bridges" property. Note that the -* fpga_bridges_enable/disable/put functions are all fine with an empty list
- * if that happens.
- *
- * Caller should callfpga_bridges_put(®ion->bridge_list)when
- * done with the bridges.
- *
- * Return 0 for success (even if there are no bridges specified)
- * or-EBUSYif any of the bridges are in use.
- */drivers/fpga/fpga-region.c +0
-17-static int of_fpga_region_get_bridges (struct fpga_region *region) -{ -struct device *dev = ®ion->dev; -struct device_node *region_np = dev->of_node; -struct fpga_image_info *info = region->info; -struct device_node *br, *np, *parent_br =NULL ; -int i, ret; - -/* If parent is a bridge, add to list */ - ret =of_fpga_bridge_get_to_list (region_np->parent, info, - ®ion->bridge_list); - -/* -EBUSY means parent is a bridge that is under use. Give up. */ -if (ret == -EBUSY ) -return ret; - -/* Zero return code means parent was a bridge and was added to list. */
drivers/fpga/fpga-region.c +0
-59-if (!ret) - parent_br = region_np->parent; - -/* If overlay has a list of bridges, use it. */ -if (of_parse_phandle (info->overlay,"fpga-bridges" ,0 )) - np = info->overlay; -else - np = region_np; - -for (i =0 ; ; i++) { - br =of_parse_phandle (np,"fpga-bridges" , i); -if (!br) -break ; - -/* If parent bridge is in list, skip it. */ -if (br == parent_br) -continue ; - -/* If node is a bridge, get it and add to list */ - ret =of_fpga_bridge_get_to_list (br, info, - ®ion->bridge_list); - -/* If any of the bridges are in use, give up */ -if (ret == -EBUSY ) { -fpga_bridges_put (®ion->bridge_list); -return -EBUSY ; - } - } - -return 0 ; -} - -/** * fpga_region_program_fpga - program FPGA * @region: FPGA region * Program an FPGA using fpga image info (region->info). @@ -282,259 +162,6 @@ int fpga_region_program_fpga(struct fpga_region *region) }EXPORT_SYMBOL_GPL (fpga_region_program_fpga); -/** -* child_regions_with_firmware -* @overlay: device node of the overlay -* -* If the overlay adds child FPGA regions, they are not allowed to have -* firmware-name property. -* -* Return 0 for OK or -EINVAL if child FPGA region adds firmware-name. -*/ -static int child_regions_with_firmware (struct device_node *overlay) -{ -struct device_node *child_region; -const char *child_firmware_name; -int ret =0 ; - -of_node_get (overlay); - - child_region =of_find_matching_node (overlay, fpga_region_of_match); -while (child_region) { -if (!of_property_read_string (child_region, "firmware-name", - &child_firmware_name)) { - ret = -EINVAL ; -break ; - } - child_region = of_find_matching_node(child_region, -
fpga_region_of_match);drivers/fpga/fpga-region.c +0
-6- } - -of_node_put (child_region); - -if (ret) - pr_err("firmware-name not allowed in child FPGA region:%pOF",
drivers/fpga/fpga-region.c +0
-12- child_region); - -return ret; -} - -/** -* of_fpga_region_parse_ov - parse and check overlay applied to region -* -* @region: FPGA region -* @overlay: overlay applied to the FPGA region -* -* Given an overlay applied to a FPGA region, parse the FPGA image specific
- * info in the overlay and do some checking.
- *
- * Returns:- * NULL if overlay doesn't direct us to program the FPGA. - * fpga_image_info struct if there is an image to program. - * error code for invalid overlay.- */
drivers/fpga/fpga-region.c +0
-2-static struct fpga_image_info *of_fpga_region_parse_ov ( - struct fpga_region*region,
- structdevice_node
*overlay)drivers/fpga/fpga-region.c +0
-54-{ -struct device *dev = ®ion->dev; -struct fpga_image_info *info; -const char *firmware_name; -int ret; - -if (region->info) { -dev_err (dev,"Region already has overlay applied.\n" ); -return ERR_PTR (-EINVAL ); - } - -/* -* Reject overlay if child FPGA Regions added in the overlay have -* firmware-name property (would mean that an FPGA region that has -* not been added to the live tree yet is doing FPGA programming). -*/ - ret =child_regions_with_firmware (overlay); -if (ret) -return ERR_PTR (ret); - - info =fpga_image_info_alloc (dev); -if (!info) -return ERR_PTR (-ENOMEM ); - - info->overlay = overlay; - -/* Read FPGA region properties from the overlay */ -if (of_property_read_bool (overlay,"partial-fpga-config" )) - info->flags |=FPGA_MGR_PARTIAL_RECONFIG ; - -if (of_property_read_bool (overlay,"external-fpga-config" )) - info->flags |=FPGA_MGR_EXTERNAL_CONFIG ; - -if (of_property_read_bool (overlay,"encrypted-fpga-config" )) - info->flags |=FPGA_MGR_ENCRYPTED_BITSTREAM ; - -if (!of_property_read_string (overlay,"firmware-name" , - &firmware_name)) { - info->firmware_name =devm_kstrdup (dev, firmware_name, -GFP_KERNEL ); -if (!info->firmware_name) -return ERR_PTR (-ENOMEM ); - } - -of_property_read_u32 (overlay,"region-unfreeze-timeout-us" , - &info->enable_timeout_us); - -of_property_read_u32 (overlay,"region-freeze-timeout-us" , - &info->disable_timeout_us); - -of_property_read_u32 (overlay,"config-complete-timeout-us" , - &info->config_complete_timeout_us); - -/* If overlay is not programming the FPGA, don't need FPGA image info */
drivers/fpga/fpga-region.c +0
-7-if (!info->firmware_name) { - ret =0 ; - goto ret_no_info; - } - -/* -* If overlay informs us FPGA was externally programmed, specifying
drivers/fpga/fpga-region.c +0
-4-* firmware here would be ambiguous. -*/ -if (info->flags &FPGA_MGR_EXTERNAL_CONFIG ) { - dev_err(dev, "error: specified firmware andexternal-fpga-config");
drivers/fpga/fpga-region.c +0
-17- ret = -EINVAL ; - goto ret_no_info; - } - -return info; -ret_no_info: -fpga_image_info_free (info); -return ERR_PTR (ret); -} - -/** -* of_fpga_region_notify_pre_apply - pre-apply overlay notification -* -* @region: FPGA region that the overlay was applied to -* @nd: overlay notification data -* -* Called when an overlay targeted to a FPGA Region is about to be applied.
- * Parses the overlay for properties that influence how the FPGA will be
- * programmed and does some checking. If the checks pass, programs the FPGA.
- * If the checks fail, overlay is rejected and does not get added to the
- * live tree.
- *
- * Returns 0 for success or negative error code for failure.
- */drivers/fpga/fpga-region.c +0
-2-static int of_fpga_region_notify_pre_apply (struct fpga_region *region, - struct of_overlay_notify_data*nd)
drivers/fpga/fpga-region.c +0
-39-{ -struct device *dev = ®ion->dev; -struct fpga_image_info *info; -int ret; - -if (region->info) { -dev_err (dev,"Region already has overlay applied.\n" ); -return -EINVAL ; - } - - info =of_fpga_region_parse_ov (region, nd->overlay); -if (IS_ERR (info)) -return PTR_ERR (info); - -if (!info) -return 0 ; - - region->info = info; - ret =fpga_region_program_fpga (region); -if (ret) { -/* error; reject overlay */ -fpga_image_info_free (info); - region->info =NULL ; - } - -return ret; -} - -/** -* of_fpga_region_notify_post_remove - post-remove overlay notification -* -* @region: FPGA region that was targeted by the overlay that was removed -* @nd: overlay notification data -* -* Called after an overlay has been removed if the overlay's target was a -* FPGA region. -*/ -static void of_fpga_region_notify_post_remove(struct fpga_region *region, - struct
of_overlay_notify_data*nd)drivers/fpga/fpga-region.c +0
-14-{ -fpga_bridges_disable (®ion->bridge_list); -fpga_bridges_put (®ion->bridge_list); -fpga_image_info_free (region->info); - region->info =NULL ; -} - -/** -* of_fpga_region_notify - reconfig notifier for dynamic DT changes -* @nb: notifier block -* @action: notifier action -* @arg: reconfig data -* -* This notifier handles programming a FPGA when a "firmware-name" property is
- * added to a fpga-region.
- *
- * ReturnsNOTIFY_OKor error if FPGA programming fails.
- */drivers/fpga/fpga-region.c +0
-51-static int of_fpga_region_notify (struct notifier_block *nb, -unsigned long action,void *arg) -{ -struct of_overlay_notify_data *nd = arg; -struct fpga_region *region; -int ret; - -switch (action) { -case OF_OVERLAY_PRE_APPLY : -pr_debug ("%s OF_OVERLAY_PRE_APPLY\n" , __func__); -break ; -case OF_OVERLAY_POST_APPLY : -pr_debug ("%s OF_OVERLAY_POST_APPLY\n" , __func__); -return NOTIFY_OK ;/* not for us */ -case OF_OVERLAY_PRE_REMOVE : -pr_debug ("%s OF_OVERLAY_PRE_REMOVE\n" , __func__); -return NOTIFY_OK ;/* not for us */ -case OF_OVERLAY_POST_REMOVE : -pr_debug ("%s OF_OVERLAY_POST_REMOVE\n" , __func__); -break ; -default :/* should not happen */ -return NOTIFY_OK ; - } - - region =of_fpga_region_find (nd->target); -if (!region) -return NOTIFY_OK ; - - ret =0 ; -switch (action) { -case OF_OVERLAY_PRE_APPLY : - ret =of_fpga_region_notify_pre_apply (region, nd); -break ; - -case OF_OVERLAY_POST_REMOVE : -of_fpga_region_notify_post_remove (region, nd); -break ; - } - -put_device (®ion->dev); - -if (ret) -return notifier_from_errno (ret); - -return NOTIFY_OK ; -} - -static struct notifier_block fpga_region_of_nb = { - .notifier_call = of_fpga_region_notify, -}; -int
fpga_region_register(struct device *dev, struct fpga_region *region){ int id, ret = 0;drivers/fpga/fpga-region.c +0
-57@@ -576,63 +203,6 @@ int fpga_region_unregister(struct fpga_region *region) }EXPORT_SYMBOL_GPL (fpga_region_unregister); -static int of_fpga_region_probe (struct platform_device *pdev) -{ -struct device *dev = &pdev->dev; -struct device_node *np = dev->of_node; -struct fpga_region *region; -struct fpga_manager *mgr; -int ret; - -/* Find the FPGA mgr specified by region or parent region. */ - mgr =of_fpga_region_get_mgr (np); -if (IS_ERR (mgr)) -return -EPROBE_DEFER ; - - region =devm_kzalloc (dev,sizeof (*region),GFP_KERNEL ); -if (!region) { - ret = -ENOMEM ; - goto eprobe_mgr_put; - } - - region->mgr = mgr; - -/* Specify how to get bridges for this type of region. */ - region->get_bridges = of_fpga_region_get_bridges; - - ret =fpga_region_register (dev, region); -if (ret) - goto eprobe_mgr_put; - -of_platform_populate (np, fpga_region_of_match,NULL , ®ion->dev); - -dev_info (dev,"FPGA Region probed\n" ); - -return 0 ; - -eprobe_mgr_put: -fpga_mgr_put (mgr); -return ret; -} - -static int of_fpga_region_remove (struct platform_device *pdev) -{ -struct fpga_region *region =platform_get_drvdata (pdev); - -fpga_region_unregister (region); - -return 0 ; -} - -static struct platform_driver of_fpga_region_driver = { - .probe = of_fpga_region_probe, - .remove = of_fpga_region_remove, - .driver = { - .name ="fpga-region" , - .of_match_table =of_match_ptr (fpga_region_of_match), - }, -}; -static void fpga_region_dev_release (struct device *dev){ struct fpga_region *region = to_fpga_region(dev);drivers/fpga/fpga-region.c +0
-19@@ -646,36 +216,17 @@ static void fpga_region_dev_release(struct device*dev) */ static int __init fpga_region_init (void ) { -int ret; - fpga_region_class =class_create (THIS_MODULE ,"fpga_region" );if (IS_ERR (fpga_region_class))return PTR_ERR (fpga_region_class); fpga_region_class->dev_release = fpga_region_dev_release; - ret =of_overlay_notifier_register (&fpga_region_of_nb); -if (ret) - goto err_class; - - ret =platform_driver_register (&of_fpga_region_driver); -if (ret) - goto err_plat; -return 0 ; - -err_plat: -of_overlay_notifier_unregister (&fpga_region_of_nb); -err_class: -class_destroy (fpga_region_class); -ida_destroy (&fpga_region_ida); -return ret; } static void __exit fpga_region_exit(void) { -platform_driver_unregister (&of_fpga_region_driver); -of_overlay_notifier_unregister (&fpga_region_of_nb);class_destroy (fpga_region_class);ida_destroy (&fpga_region_ida);}
drivers/fpga/of-fpga-region.c new file +7
-0@@ -0,0 +1,495 @@ +/* +* FPGA Region - Device Tree support for FPGA programming under Linux +* +* Copyright (C) 2013-2016 Altera Corporation +* Copyright (C) 2017 Intel Corporation +* +* This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program. If not, see <http://www.gnu.org/licenses/>.
+ */drivers/fpga/of-fpga-region.c new file +19
-0+ +#include <linux/fpga/fpga-bridge.h> +#include <linux/fpga/fpga-mgr.h> +#include <linux/fpga/fpga-region.h> +#include <linux/idr.h> +#include <linux/kernel.h> +#include <linux/list.h> +#include <linux/module.h> +#include <linux/of_platform.h> +#include <linux/slab.h> +#include <linux/spinlock.h> + +static const struct of_device_id fpga_region_of_match[] = { + { .compatible ="fpga-region" , }, + {}, +}; +MODULE_DEVICE_TABLE (of, fpga_region_of_match); + +static int fpga_region_of_node_match(struct device *dev, const void*data)
drivers/fpga/of-fpga-region.c new file +15
-0+{ +return dev->of_node == data; +} + +/** +* of_fpga_region_find - find FPGA region +* @np: device node of FPGA Region +* +* Caller will need to put_device(®ion->dev) when done. +* +* Returns FPGA Region struct or NULL +*/ +static struct fpga_region *of_fpga_region_find (struct device_node *np) +{ + return fpga_region_class_find(NULL, np,
fpga_region_of_node_match);drivers/fpga/of-fpga-region.c new file +11
-0+} + +/** +* of_fpga_region_get_mgr - get reference for FPGA manager +* @np: device node of FPGA region +* +* Get FPGA Manager from "fpga-mgr" property or from ancestor region. +* +* Caller should call fpga_mgr_put() when done with manager. +* +* Return: fpga manager struct or IS_ERR() condition containing error code.
+ */
+static structfpga_manager*of_fpga_region_get_mgr(structdevice_node*np)drivers/fpga/of-fpga-region.c new file +28
-0+{ +struct device_node *mgr_node; +struct fpga_manager *mgr; + +of_node_get (np); +while (np) { +if (of_device_is_compatible (np,"fpga-region" )) { + mgr_node =of_parse_phandle (np,"fpga-mgr" ,0 ); +if (mgr_node) { + mgr =of_fpga_mgr_get (mgr_node); +of_node_put (np); +return mgr; + } + } + np =of_get_next_parent (np); + } +of_node_put (np); + +return ERR_PTR (-EINVAL ); +} + +/** +* of_fpga_region_get_bridges - create a list of bridges +* @region: FPGA region +* +* Create a list of bridges including the parent bridge and the bridges +* specified by "fpga-bridges" property. Note that the +* fpga_bridges_enable/disable/put functions are all fine with an empty list
+ * if that happens.
+ *
+ * Caller should callfpga_bridges_put(®ion->bridge_list)when
+ * done with the bridges.
+ *
+ * Return 0 for success (even if there are no bridges specified)
+ * or-EBUSYif any of the bridges are in use.
+ */drivers/fpga/of-fpga-region.c new file +17
-0+static int of_fpga_region_get_bridges (struct fpga_region *region) +{ +struct device *dev = ®ion->dev; +struct device_node *region_np = dev->of_node; +struct fpga_image_info *info = region->info; +struct device_node *br, *np, *parent_br =NULL ; +int i, ret; + +/* If parent is a bridge, add to list */ + ret =of_fpga_bridge_get_to_list (region_np->parent, info, + ®ion->bridge_list); + +/* -EBUSY means parent is a bridge that is under use. Give up. */ +if (ret == -EBUSY ) +return ret; + +/* Zero return code means parent was a bridge and was added to list. */
drivers/fpga/of-fpga-region.c new file +50
-0+if (!ret) + parent_br = region_np->parent; + +/* If overlay has a list of bridges, use it. */ +if (of_parse_phandle (info->overlay,"fpga-bridges" ,0 )) + np = info->overlay; +else + np = region_np; + +for (i =0 ; ; i++) { + br =of_parse_phandle (np,"fpga-bridges" , i); +if (!br) +break ; + +/* If parent bridge is in list, skip it. */ +if (br == parent_br) +continue ; + +/* If node is a bridge, get it and add to list */ + ret =of_fpga_bridge_get_to_list (br, info, + ®ion->bridge_list); + +/* If any of the bridges are in use, give up */ +if (ret == -EBUSY ) { +fpga_bridges_put (®ion->bridge_list); +return -EBUSY ; + } + } + +return 0 ; +} + +/** +* child_regions_with_firmware +* @overlay: device node of the overlay +* +* If the overlay adds child FPGA regions, they are not allowed to have +* firmware-name property. +* +* Return 0 for OK or -EINVAL if child FPGA region adds firmware-name. +*/ +static int child_regions_with_firmware (struct device_node *overlay) +{ +struct device_node *child_region; +const char *child_firmware_name; +int ret =0 ; + +of_node_get (overlay); + + child_region = of_find_matching_node(overlay,
fpga_region_of_match);drivers/fpga/of-fpga-region.c new file +2
-0+ while (child_region) { + if (!of_property_read_string(child_region,"firmware-name",
drivers/fpga/of-fpga-region.c new file +6
-0+ &child_firmware_name)) { + ret = -EINVAL ; +break ; + } + child_region = of_find_matching_node(child_region, +
fpga_region_of_match);drivers/fpga/of-fpga-region.c new file +6
-0+ } + +of_node_put (child_region); + +if (ret) + pr_err("firmware-name not allowed in child FPGA region:%pOF",
drivers/fpga/of-fpga-region.c new file +12
-0+ child_region); + +return ret; +} + +/** +* of_fpga_region_parse_ov - parse and check overlay applied to region +* +* @region: FPGA region +* @overlay: overlay applied to the FPGA region +* +* Given an overlay applied to a FPGA region, parse the FPGA image specific
+ * info in the overlay and do some checking.
+ *
+ * Returns:+ * NULL if overlay doesn't direct us to program the FPGA. + * fpga_image_info struct if there is an image to program. + * error code for invalid overlay.+ */
drivers/fpga/of-fpga-region.c new file +2
-0+static struct fpga_image_info *of_fpga_region_parse_ov ( + struct fpga_region*region,
+ structdevice_node
*overlay)drivers/fpga/of-fpga-region.c new file +54
-0+{ +struct device *dev = ®ion->dev; +struct fpga_image_info *info; +const char *firmware_name; +int ret; + +if (region->info) { +dev_err (dev,"Region already has overlay applied.\n" ); +return ERR_PTR (-EINVAL ); + } + +/* +* Reject overlay if child FPGA Regions added in the overlay have +* firmware-name property (would mean that an FPGA region that has +* not been added to the live tree yet is doing FPGA programming). +*/ + ret =child_regions_with_firmware (overlay); +if (ret) +return ERR_PTR (ret); + + info =fpga_image_info_alloc (dev); +if (!info) +return ERR_PTR (-ENOMEM ); + + info->overlay = overlay; + +/* Read FPGA region properties from the overlay */ +if (of_property_read_bool (overlay,"partial-fpga-config" )) + info->flags |=FPGA_MGR_PARTIAL_RECONFIG ; + +if (of_property_read_bool (overlay,"external-fpga-config" )) + info->flags |=FPGA_MGR_EXTERNAL_CONFIG ; + +if (of_property_read_bool (overlay,"encrypted-fpga-config" )) + info->flags |=FPGA_MGR_ENCRYPTED_BITSTREAM ; + +if (!of_property_read_string (overlay,"firmware-name" , + &firmware_name)) { + info->firmware_name =devm_kstrdup (dev, firmware_name, +GFP_KERNEL ); +if (!info->firmware_name) +return ERR_PTR (-ENOMEM ); + } + +of_property_read_u32 (overlay,"region-unfreeze-timeout-us" , + &info->enable_timeout_us); + +of_property_read_u32 (overlay,"region-freeze-timeout-us" , + &info->disable_timeout_us); + +of_property_read_u32 (overlay,"config-complete-timeout-us" , + &info->config_complete_timeout_us); + +/* If overlay is not programming the FPGA, don't need FPGA image info */
drivers/fpga/of-fpga-region.c new file +7
-0+if (!info->firmware_name) { + ret =0 ; + goto ret_no_info; + } + +/* +* If overlay informs us FPGA was externally programmed, specifying
drivers/fpga/of-fpga-region.c new file +4
-0+* firmware here would be ambiguous. +*/ +if (info->flags &FPGA_MGR_EXTERNAL_CONFIG ) { + dev_err(dev, "error: specified firmware andexternal-fpga-config");
drivers/fpga/of-fpga-region.c new file +17
-0+ ret = -EINVAL ; + goto ret_no_info; + } + +return info; +ret_no_info: +fpga_image_info_free (info); +return ERR_PTR (ret); +} + +/** +* of_fpga_region_notify_pre_apply - pre-apply overlay notification +* +* @region: FPGA region that the overlay was applied to +* @nd: overlay notification data +* +* Called when an overlay targeted to a FPGA Region is about to be applied.
+ * Parses the overlay for properties that influence how the FPGA will be
+ * programmed and does some checking. If the checks pass, programs the FPGA.
+ * If the checks fail, overlay is rejected and does not get added to the
+ * live tree.
+ *
+ * Returns 0 for success or negative error code for failure.
+ */drivers/fpga/of-fpga-region.c new file +2
-0+static int of_fpga_region_notify_pre_apply (struct fpga_region *region, + struct of_overlay_notify_data*nd)
drivers/fpga/of-fpga-region.c new file +39
-0+{ +struct device *dev = ®ion->dev; +struct fpga_image_info *info; +int ret; + +if (region->info) { +dev_err (dev,"Region already has overlay applied.\n" ); +return -EINVAL ; + } + + info =of_fpga_region_parse_ov (region, nd->overlay); +if (IS_ERR (info)) +return PTR_ERR (info); + +if (!info) +return 0 ; + + region->info = info; + ret =fpga_region_program_fpga (region); +if (ret) { +/* error; reject overlay */ +fpga_image_info_free (info); + region->info =NULL ; + } + +return ret; +} + +/** +* of_fpga_region_notify_post_remove - post-remove overlay notification +* +* @region: FPGA region that was targeted by the overlay that was removed +* @nd: overlay notification data +* +* Called after an overlay has been removed if the overlay's target was a +* FPGA region. +*/ +static void of_fpga_region_notify_post_remove(struct fpga_region *region, + struct
of_overlay_notify_data*nd)drivers/fpga/of-fpga-region.c new file +14
-0+{ +fpga_bridges_disable (®ion->bridge_list); +fpga_bridges_put (®ion->bridge_list); +fpga_image_info_free (region->info); + region->info =NULL ; +} + +/** +* of_fpga_region_notify - reconfig notifier for dynamic DT changes +* @nb: notifier block +* @action: notifier action +* @arg: reconfig data +* +* This notifier handles programming a FPGA when a "firmware-name" property is
+ * added to a fpga-region.
+ *
+ * ReturnsNOTIFY_OKor error if FPGA programming fails.
+ */drivers/fpga/of-fpga-region.c new file +17
-0+static int of_fpga_region_notify (struct notifier_block *nb, +unsigned long action,void *arg) +{ +struct of_overlay_notify_data *nd = arg; +struct fpga_region *region; +int ret; + +switch (action) { +case OF_OVERLAY_PRE_APPLY : +pr_debug ("%s OF_OVERLAY_PRE_APPLY\n" , __func__); +break ; +case OF_OVERLAY_POST_APPLY : +pr_debug ("%s OF_OVERLAY_POST_APPLY\n" , __func__); +return NOTIFY_OK ;/* not for us */ +case OF_OVERLAY_PRE_REMOVE : +pr_debug ("%s OF_OVERLAY_PRE_REMOVE\n" , __func__); +return NOTIFY_OK ;/* not for us */
Quoted 8 lines
drivers/fpga/of-fpga-region.c line 374
Quoted 5 lines
drivers/fpga/of-fpga-region.c new file +5
-0+case OF_OVERLAY_POST_REMOVE : +pr_debug ("%s OF_OVERLAY_POST_REMOVE\n" , __func__); +break ; +default :/* should not happen */ +return NOTIFY_OK ;If this should not happen, then shouldn't an error be returned?
That's not necessary in this case. This function listens only for the notifications we care about here, which are pre-apply and post-remove notifications. The only way the default case would get hit is if other notifications get added to the device tree notifications code (other than the current pre/post + apply/remove). And even if other notifications get added without this code being updated, it won't create an bug for me here. Adding an error for a non-error condition doesn't seem useful.
Quoted 132 lines
Quoted 131 lines
drivers/fpga/of-fpga-region.c new file +58
-0+ } + + region =of_fpga_region_find (nd->target); +if (!region) +return NOTIFY_OK ; + + ret =0 ; +switch (action) { +case OF_OVERLAY_PRE_APPLY : + ret =of_fpga_region_notify_pre_apply (region, nd); +break ; + +case OF_OVERLAY_POST_REMOVE : +of_fpga_region_notify_post_remove (region, nd); +break ; + } + +put_device (®ion->dev); + +if (ret) +return notifier_from_errno (ret); + +return NOTIFY_OK ; +} + +static struct notifier_block fpga_region_of_nb = { + .notifier_call = of_fpga_region_notify, +}; + +static int of_fpga_region_probe (struct platform_device *pdev) +{ +struct device *dev = &pdev->dev; +struct device_node *np = dev->of_node; +struct fpga_region *region; +struct fpga_manager *mgr; +int ret; + +/* Find the FPGA mgr specified by region or parent region. */ + mgr =of_fpga_region_get_mgr (np); +if (IS_ERR (mgr)) +return -EPROBE_DEFER ; + + region =devm_kzalloc (dev,sizeof (*region),GFP_KERNEL ); +if (!region) { + ret = -ENOMEM ; + goto eprobe_mgr_put; + } + + region->mgr = mgr; + +/* Specify how to get bridges for this type of region. */ + region->get_bridges = of_fpga_region_get_bridges; + + ret =fpga_region_register (dev, region); +if (ret) + goto eprobe_mgr_put; + + of_platform_populate(np, fpga_region_of_match, NULL,
®ion->dev);drivers/fpga/of-fpga-region.c new file +63
-1+ +dev_info (dev,"FPGA Region probed\n" ); + +return 0 ; + +eprobe_mgr_put: +fpga_mgr_put (mgr); +return ret; +} + +static int of_fpga_region_remove (struct platform_device *pdev) +{ +struct fpga_region *region =platform_get_drvdata (pdev); + +fpga_region_unregister (region); + +return 0 ; +} + +static struct platform_driver of_fpga_region_driver = { + .probe = of_fpga_region_probe, + .remove = of_fpga_region_remove, + .driver = { + .name ="of-fpga-region" , + .of_match_table =of_match_ptr (fpga_region_of_match), + }, +}; + +/** +* fpga_region_init - init function for fpga_region class +* Creates the fpga_region class and registers a reconfig notifier. +*/ +static int __init of_fpga_region_init (void ) +{ +int ret; + + ret =of_overlay_notifier_register (&fpga_region_of_nb); +if (ret) +return ret; + + ret =platform_driver_register (&of_fpga_region_driver); +if (ret) + goto err_plat; + +return 0 ; + +err_plat: +of_overlay_notifier_unregister (&fpga_region_of_nb); +return ret; +} + +static void __exit of_fpga_region_exit(void) +{ +platform_driver_unregister (&of_fpga_region_driver); +of_overlay_notifier_unregister (&fpga_region_of_nb); +} + +subsys_initcall (of_fpga_region_init); +module_exit (of_fpga_region_exit); + +MODULE_DESCRIPTION ("FPGA Region" ); +MODULE_AUTHOR ("Alan Tull <[email protected]>" ); +MODULE_LICENSE ("GPL v2" ); --2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v4 16/18 fpga: of-fpga-region: accept overlays that don't program FPGA
In reply to Alan TullThe FPGA may already have a static image programmed when Linux boots. In that case a DT overlay may be used to add the devices that already exist. This commit allows that by shuffling the order of some checks.
Signed-off-by Alan Tull <[email protected]>
---
v4: Patch added to patchset in v4
--- drivers/fpga/of-fpga-region.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
drivers/fpga/of-fpga-region.c +6 -5
@@ -298,18 +298,19 @@ static int of_fpga_region_notify_pre_apply(struct fpga_region *region,struct fpga_image_info *info;int ret; -if (region->info) { -dev_err (dev,"Region already has overlay applied.\n" ); -return -EINVAL ; - } - info =of_fpga_region_parse_ov (region, nd->overlay);if (IS_ERR (info))return PTR_ERR (info); +/* If overlay doesn't program the FPGA, accept it anyway. */ if (!info)return 0 ; +if (region->info) { +dev_err (dev,"Region already has overlay applied.\n" ); +return -EINVAL ; + } + region->info = info; ret =fpga_region_program_fpga (region);if (ret) {
Signature · 1 line
2.7.4
PATCH v4 17/18 fpga: clean up fpga Kconfig
In reply to Alan TullThe fpga menuconfig has gotten messy. The bridges and managers are mixxed together.
* Separate the bridges and things dependent on CONFIG_FPGA_BRIDGE from the managers.
* Group the managers by vendor in order that they were added to the kernel.
The following is what the menuconfig ends up looking like more or less (platform dependencies are hiding some of these on any given platform).
--- FPGA Configuration Framework
<*> Altera SOCFPGA FPGA Manager
<*> Altera SoCFPGA Arria10
<*> Altera Partial Reconfiguration IP Core
<*> Platform support of Altera Partial Reconfiguration IP Core
<*> Altera FPGA Passive Serial over SPI
<*> Altera Arria-V/Cyclone-V/Stratix-V CvP FPGA Manager
<*> Xilinx Zynq FPGA
<*> Xilinx Configuration over Slave Serial (SPI)
<*> Lattice iCE40 SPI
<*> Technologic Systems TS-73xx SBC FPGA Manager
<*> FPGA Bridge Framework
<*> Altera SoCFPGA FPGA Bridges
<*> Altera FPGA Freeze Bridge
<*> Xilinx LogiCORE PR Decoupler
<*> FPGA Region
<*> FPGA Region Device Tree Overlay Support
Signed-off-by Alan Tull <[email protected]>
---
v4: Patch added to patchset in v4
--- drivers/fpga/Kconfig | 106 +++++++++++++++++++++++++-------------------------- 1 file changed, 53 insertions(+), 53 deletions(-)
drivers/fpga/Kconfig +53 -53
@@ -11,32 +11,30 @@ menuconfig FPGA if FPGA -config FPGA_REGION - tristate "FPGA Region" - depends on FPGA_BRIDGE +config FPGA_MGR_SOCFPGA + tristate "Altera SOCFPGA FPGA Manager" + depends on ARCH_SOCFPGA || COMPILE_TEST help - FPGA Region common code. A FPGA Region controls a FPGA Manager - and the FPGA Bridges associated with either a reconfigurable - region of an FPGA or a whole FPGA. + FPGA manager driver support for Altera SOCFPGA. -config OF_FPGA_REGION - tristate "FPGA Region Device Tree Overlay Support" - depends on OF && FPGA_REGION +config FPGA_MGR_SOCFPGA_A10 + tristate "Altera SoCFPGA Arria10" + depends on ARCH_SOCFPGA || COMPILE_TEST + select REGMAP_MMIO help - Support for loading FPGA images under control of Device Tree. + FPGA manager driver support for Altera Arria10 SoCFPGA. -config FPGA_MGR_ICE40_SPI - tristate "Lattice iCE40 SPI" - depends on OF && SPI - help - FPGA manager driver support for Lattice iCE40 FPGAs over SPI. +config ALTERA_PR_IP_CORE + tristate "Altera Partial Reconfiguration IP Core" + help + Core driver support for Altera Partial Reconfiguration IP component -config FPGA_MGR_ALTERA_CVP - tristate "Altera Arria-V/Cyclone-V/Stratix-V CvP FPGA Manager" - depends on PCI +config ALTERA_PR_IP_CORE_PLAT + tristate "Platform support of Altera Partial Reconfiguration IP Core" + depends on ALTERA_PR_IP_CORE && OF && HAS_IOMEM help - FPGA manager driver support for Arria-V, Cyclone-V, Stratix-V - and Arria 10 Altera FPGAs using the CvP interface over PCIe. + Platform driver support for Altera Partial Reconfiguration IP + component config FPGA_MGR_ALTERA_PS_SPI tristate "Altera FPGA Passive Serial over SPI"
@@ -45,25 +43,19 @@ config FPGA_MGR_ALTERA_PS_SPI FPGA manager driver support for Altera Arria/Cyclone/Stratix using the passive serial interface over SPI. -config FPGA_MGR_SOCFPGA - tristate "Altera SOCFPGA FPGA Manager" - depends on ARCH_SOCFPGA || COMPILE_TEST - help - FPGA manager driver support for Altera SOCFPGA. - -config FPGA_MGR_SOCFPGA_A10 - tristate "Altera SoCFPGA Arria10" - depends on ARCH_SOCFPGA || COMPILE_TEST - select REGMAP_MMIO +config FPGA_MGR_ALTERA_CVP + tristate "Altera Arria-V/Cyclone-V/Stratix-V CvP FPGA Manager" + depends on PCI help - FPGA manager driver support for Altera Arria10 SoCFPGA. + FPGA manager driver support for Arria-V, Cyclone-V, Stratix-V + and Arria 10 Altera FPGAs using the CvP interface over PCIe. -config FPGA_MGR_TS73XX - tristate "Technologic Systems TS-73xx SBC FPGA Manager" - depends on ARCH_EP93XX && MACH_TS72XX +config FPGA_MGR_ZYNQ_FPGA + tristate "Xilinx Zynq FPGA" + depends on ARCH_ZYNQ || COMPILE_TEST + depends on HAS_DMA help - FPGA manager driver support for the Altera Cyclone II FPGA - present on the TS-73xx SBC boards. + FPGA manager driver support for Xilinx Zynq FPGAs. config FPGA_MGR_XILINX_SPI tristate "Xilinx Configuration over Slave Serial (SPI)"
@@ -72,12 +64,18 @@ config FPGA_MGR_XILINX_SPI FPGA manager driver support for Xilinx FPGA configuration over slave serial interface. -config FPGA_MGR_ZYNQ_FPGA - tristate "Xilinx Zynq FPGA" - depends on ARCH_ZYNQ || COMPILE_TEST - depends on HAS_DMA +config FPGA_MGR_ICE40_SPI + tristate "Lattice iCE40 SPI" + depends on OF && SPI help - FPGA manager driver support for Xilinx Zynq FPGAs. + FPGA manager driver support for Lattice iCE40 FPGAs over SPI. + +config FPGA_MGR_TS73XX + tristate "Technologic Systems TS-73xx SBC FPGA Manager" + depends on ARCH_EP93XX && MACH_TS72XX + help + FPGA manager driver support for the Altera Cyclone II FPGA + present on the TS-73xx SBC boards. config FPGA_BRIDGE tristate "FPGA Bridge Framework"
@@ -101,18 +99,6 @@ config ALTERA_FREEZE_BRIDGE isolate one region of the FPGA from the busses while that region is being reprogrammed. -config ALTERA_PR_IP_CORE - tristate "Altera Partial Reconfiguration IP Core" - help - Core driver support for Altera Partial Reconfiguration IP component - -config ALTERA_PR_IP_CORE_PLAT - tristate "Platform support of Altera Partial Reconfiguration IP Core" - depends on ALTERA_PR_IP_CORE && OF && HAS_IOMEM - help - Platform driver support for Altera Partial Reconfiguration IP - component - config XILINX_PR_DECOUPLER tristate "Xilinx LogiCORE PR Decoupler" depends on FPGA_BRIDGE
@@ -123,4 +109,18 @@ config XILINX_PR_DECOUPLER region of the FPGA from the busses while that region is being reprogrammed during partial reconfig. +config FPGA_REGION + tristate "FPGA Region" + depends on FPGA_BRIDGE + help + FPGA Region common code. A FPGA Region controls a FPGA Manager + and the FPGA Bridges associated with either a reconfigurable + region of an FPGA or a whole FPGA. + +config OF_FPGA_REGION + tristate "FPGA Region Device Tree Overlay Support" + depends on OF && FPGA_REGION + help + Support for loading FPGA images under control of Device Tree. + endif # FPGA
Signature · 1 line
2.7.4
Re: [PATCH v4 17/18] fpga: clean up fpga Kconfig
Hi Alan,
s/mixxed/mixed/
Quoted 198 lines · Alan Tull
The fpga menuconfig has gotten messy. The bridges and managers are mixxed together.
* Separate the bridges and things dependent on
CONFIG_FPGA_BRIDGEfrom the managers.
* Group the managers by vendor in order that they were added to the kernel.The following is what the menuconfig ends up looking like more or less (platform dependencies are hiding some of these on any given platform).
--- FPGA Configuration Framework
<*> Altera SOCFPGA FPGA Manager <*> Altera SoCFPGA Arria10 <*> Altera Partial Reconfiguration IP Core <*> Platform support of Altera Partial Reconfiguration IP Core <*> Altera FPGA Passive Serial over SPI <*> Altera Arria-V/Cyclone-V/Stratix-V CvP FPGA Manager <*> Xilinx Zynq FPGA <*> Xilinx Configuration over Slave Serial (SPI) <*> Lattice iCE40 SPI <*> Technologic Systems TS-73xx SBC FPGA Manager <*> FPGA Bridge Framework <*> Altera SoCFPGA FPGA Bridges <*> Altera FPGA Freeze Bridge <*> Xilinx LogiCORE PR Decoupler <*> FPGA Region<*> FPGA Region Device Tree Overlay Support
Signed-off-by Alan Tull <[email protected]>---
v4: Patch added to patchset in v4
---drivers/fpga/Kconfig| 106 +++++++++++++++++++++++++--------------------------1 file changed, 53 insertions(+), 53 deletions(-)drivers/fpga/Kconfig +0
-0@@ -11,32 +11,30 @@ menuconfig FPGAif FPGA
drivers/fpga/Kconfig +3
-3-config FPGA_REGION - tristate "FPGA Region" - depends on FPGA_BRIDGE +config FPGA_MGR_SOCFPGA + tristate "Altera SOCFPGA FPGA Manager" + depends on ARCH_SOCFPGA || COMPILE_TESThelp
drivers/fpga/Kconfig +5
-6- FPGA Region common code. A FPGA Region controls a FPGA Manager - and the FPGA Bridges associated with either a reconfigurable - region of an FPGA or a whole FPGA. + FPGA manager driver support for Altera SOCFPGA. -config OF_FPGA_REGION - tristate "FPGA Region Device Tree Overlay Support" - depends on OF && FPGA_REGION +config FPGA_MGR_SOCFPGA_A10 + tristate "Altera SoCFPGA Arria10" + depends on ARCH_SOCFPGA || COMPILE_TEST + select REGMAP_MMIOhelp
drivers/fpga/Kconfig +8
-9- Support for loading FPGA images under control of Device Tree. + FPGA manager driver support for Altera Arria10 SoCFPGA. -config FPGA_MGR_ICE40_SPI - tristate "Lattice iCE40 SPI" - depends on OF && SPI - help - FPGA manager driver support for Lattice iCE40 FPGAs over SPI. +config ALTERA_PR_IP_CORE + tristate "Altera Partial Reconfiguration IP Core" + help + Core driver support for Altera Partial Reconfiguration IP component -config FPGA_MGR_ALTERA_CVP - tristate "Altera Arria-V/Cyclone-V/Stratix-V CvP FPGA Manager" - depends on PCI +config ALTERA_PR_IP_CORE_PLAT + tristate "Platform support of Altera Partial Reconfiguration IP Core" + depends on ALTERA_PR_IP_CORE && OF && HAS_IOMEMhelp
drivers/fpga/Kconfig +2
-2- FPGA manager driver support for Arria-V, Cyclone-V, Stratix-V - and Arria 10 Altera FPGAs using the CvP interface over PCIe. + Platform driver support for Altera Partial Reconfiguration IP + componentconfig
FPGA_MGR_ALTERA_PS_SPI
tristate "Altera FPGA Passive Serial over SPI"drivers/fpga/Kconfig +10
-16@@ -45,25 +43,19 @@ config FPGA_MGR_ALTERA_PS_SPI FPGA manager driver support for Altera Arria/Cyclone/Stratix using the passive serial interface over SPI. -config FPGA_MGR_SOCFPGA - tristate "Altera SOCFPGA FPGA Manager" - depends on ARCH_SOCFPGA || COMPILE_TEST - help - FPGA manager driver support for Altera SOCFPGA. - -config FPGA_MGR_SOCFPGA_A10 - tristate "Altera SoCFPGA Arria10" - depends on ARCH_SOCFPGA || COMPILE_TEST - select REGMAP_MMIO +config FPGA_MGR_ALTERA_CVP + tristate "Altera Arria-V/Cyclone-V/Stratix-V CvP FPGA Manager" + depends on PCI help - FPGA manager driver support for Altera Arria10 SoCFPGA. + FPGA manager driver support for Arria-V, Cyclone-V, Stratix-V + and Arria 10 Altera FPGAs using the CvP interface over PCIe. -config FPGA_MGR_TS73XX - tristate "Technologic Systems TS-73xx SBC FPGA Manager" - depends on ARCH_EP93XX && MACH_TS72XX +config FPGA_MGR_ZYNQ_FPGA + tristate "Xilinx Zynq FPGA" + depends on ARCH_ZYNQ || COMPILE_TEST + depends on HAS_DMA help - FPGA manager driver support for the Altera Cyclone II FPGA - present on the TS-73xx SBC boards. + FPGA manager driver support for Xilinx Zynq FPGAs.config
FPGA_MGR_XILINX_SPI
tristate "Xilinx Configuration over Slave Serial (SPI)"drivers/fpga/Kconfig +11
-5@@ -72,12 +64,18 @@ config FPGA_MGR_XILINX_SPI FPGA manager driver support for Xilinx FPGA configuration over slave serial interface. -config FPGA_MGR_ZYNQ_FPGA - tristate "Xilinx Zynq FPGA" - depends on ARCH_ZYNQ || COMPILE_TEST - depends on HAS_DMA +config FPGA_MGR_ICE40_SPI + tristate "Lattice iCE40 SPI" + depends on OF && SPI help - FPGA manager driver support for Xilinx Zynq FPGAs. + FPGA manager driver support for Lattice iCE40 FPGAs over SPI. + +config FPGA_MGR_TS73XX + tristate "Technologic Systems TS-73xx SBC FPGA Manager" + depends on ARCH_EP93XX && MACH_TS72XX + help + FPGA manager driver support for the Altera Cyclone II FPGA + present on the TS-73xx SBC boards.config
FPGA_BRIDGE
tristate "FPGA Bridge Framework"drivers/fpga/Kconfig +0
-12@@ -101,18 +99,6 @@ config ALTERA_FREEZE_BRIDGE isolate one region of the FPGA from the busses while that region is being reprogrammed. -config ALTERA_PR_IP_CORE - tristate "Altera Partial Reconfiguration IP Core" - help - Core driver support for Altera Partial Reconfiguration IP component - -config ALTERA_PR_IP_CORE_PLAT - tristate "Platform support of Altera Partial Reconfiguration IP Core" - depends on ALTERA_PR_IP_CORE && OF && HAS_IOMEM - help - Platform driver support for Altera Partial Reconfiguration IP - component -config
XILINX_PR_DECOUPLER
tristate "Xilinx LogiCORE PR Decoupler"
depends onFPGA_BRIDGEdrivers/fpga/Kconfig +14
-0@@ -123,4 +109,18 @@ config XILINX_PR_DECOUPLER region of the FPGA from the busses while that region is being reprogrammed during partial reconfig. +config FPGA_REGION + tristate "FPGA Region" + depends on FPGA_BRIDGE + help + FPGA Region common code. A FPGA Region controls a FPGA Manager + and the FPGA Bridges associated with either a reconfigurable + region of an FPGA or a whole FPGA. + +config OF_FPGA_REGION + tristate "FPGA Region Device Tree Overlay Support" + depends on OF && FPGA_REGION + help + Support for loading FPGA images under control of Device Tree. +endif # FPGA
Signature · 6 lines
2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-fpga" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 17/18] fpga: clean up fpga Kconfig
On Thu, Sep 14, 2017 at 10:56 AM, <[email protected]> wrote:
Hi Matthew,
Quoted 5 lines
Hi Alan,
s/mixxed/mixed/
OK, I'll fix that.
Alan
Quoted 35 lines · Alan Tull
Quoted 34 lines
The fpga menuconfig has gotten messy. The bridges and managers are mixxed together.
* Separate the bridges and things dependent on
CONFIG_FPGA_BRIDGEfrom the managers.
* Group the managers by vendor in order that they were added to the kernel.The following is what the menuconfig ends up looking like more or less (platform dependencies are hiding some of these on any given platform).
--- FPGA Configuration Framework
<*> Altera SOCFPGA FPGA Manager <*> Altera SoCFPGA Arria10 <*> Altera Partial Reconfiguration IP Core <*> Platform support of Altera Partial Reconfiguration IP Core <*> Altera FPGA Passive Serial over SPI <*> Altera Arria-V/Cyclone-V/Stratix-V CvP FPGA Manager <*> Xilinx Zynq FPGA <*> Xilinx Configuration over Slave Serial (SPI) <*> Lattice iCE40 SPI <*> Technologic Systems TS-73xx SBC FPGA Manager <*> FPGA Bridge Framework <*> Altera SoCFPGA FPGA Bridges <*> Altera FPGA Freeze Bridge <*> Xilinx LogiCORE PR Decoupler <*> FPGA Region<*> FPGA Region Device Tree Overlay Support
Signed-off-by Alan Tull <[email protected]>---
v4: Patch added to patchset in v4
PATCH v4 18/18 fpga: add attribute groups
In reply to Alan TullMake it easy to add attributes to low level FPGA drivers the right way. Add attribute groups pointers to structures that are used when registering a manager, bridge, or group. When the low level driver registers, set the device attribute group. The attributes are created in device_add.
Signed-off-by Alan Tull <[email protected]>
---
v4: Patch added to patchset in v4
--- drivers/fpga/fpga-bridge.c | 1 + drivers/fpga/fpga-mgr.c | 1 + drivers/fpga/fpga-region.c | 1 + include/linux/fpga/fpga-bridge.h | 2 ++ include/linux/fpga/fpga-mgr.h | 2 ++ include/linux/fpga/fpga-region.h | 2 ++ 6 files changed, 9 insertions(+)
drivers/fpga/fpga-bridge.c +1 -0
@@ -367,6 +367,7 @@ int fpga_bridge_register(struct device *dev, const char *name, bridge->priv = priv;device_initialize (&bridge->dev); + bridge->dev.groups = br_ops->groups; bridge->dev.class = fpga_bridge_class; bridge->dev.parent = dev; bridge->dev.of_node = dev->of_node;
drivers/fpga/fpga-mgr.c +1 -0
@@ -569,6 +569,7 @@ int fpga_mgr_register(struct device *dev, const char *name,device_initialize (&mgr->dev); mgr->dev.class = fpga_mgr_class; + mgr->dev.groups = mops->groups; mgr->dev.parent = dev; mgr->dev.of_node = dev->of_node; mgr->dev.id = id;
drivers/fpga/fpga-region.c +1 -0
@@ -173,6 +173,7 @@ int fpga_region_register(struct device *dev, struct fpga_region *region)mutex_init (®ion->mutex);INIT_LIST_HEAD (®ion->bridge_list);device_initialize (®ion->dev); + region->dev.groups = region->groups; region->dev.class = fpga_region_class; region->dev.parent = dev; region->dev.of_node = dev->of_node;
include/linux/fpga/fpga-bridge.h +2 -0
@@ -11,11 +11,13 @@ struct fpga_bridge;* @enable_show: returns the FPGA bridge's status * @enable_set: set a FPGA bridge as enabled or disabled * @fpga_bridge_remove: set FPGA into a specific state during driver remove +* @groups: optional attribute groups. */ struct fpga_bridge_ops {int (*enable_show)(struct fpga_bridge *bridge);int (*enable_set)(struct fpga_bridge *bridge,bool enable);void (*fpga_bridge_remove)(struct fpga_bridge *bridge); +const struct attribute_group **groups; };/**
include/linux/fpga/fpga-mgr.h +2 -0
@@ -115,6 +115,7 @@ struct fpga_image_info {* @write_sg: write the scatter list of configuration data to the FPGA * @write_complete: set FPGA to operating state after writing is done * @fpga_remove: optional: Set FPGA into a specific state during driver remove +* @groups: optional attribute groups. * * fpga_manager_ops are the low level functions implemented by a specific * fpga manager driver. The optional ones are tested for NULL before being
@@ -131,6 +132,7 @@ struct fpga_manager_ops {int (*write_complete)(struct fpga_manager *mgr,struct fpga_image_info *info);void (*fpga_remove)(struct fpga_manager *mgr); +const struct attribute_group **groups; };/**
include/linux/fpga/fpga-region.h +2 -0
@@ -14,6 +14,7 @@* @info: FPGA image info * @priv: private data * @get_bridges: optional function to get bridges to a list +* @groups: optional attribute groups. */ struct fpga_region {struct device dev;
@@ -23,6 +24,7 @@ struct fpga_region {struct fpga_image_info *info;void *priv;int (*get_bridges)(struct fpga_region *region); +const struct attribute_group **groups; };#define to_fpga_region (d) container_of(d, struct fpga_region, dev)
Signature · 1 line
2.7.4