Re: [PATCH v4 15/18] fpga: region: move device tree support to of-fpga-region.c

From: Alan Tull
Date: Thu Sep 14 2017 - 16:55:37 EST

On Thu, Sep 14, 2017 at 10:50 AM, <matthew.gerlach@xxxxxxxxxxxxxxx> wrote:

Hi Matthew,

Thanks for the review!

>
> Hi Alan,
>
> Just a couple of minor nits.
>
> Matthew Gerlach
>
> On Wed, 13 Sep 2017, Alan Tull wrote:
>
>> Create of-fpga-region.c and ove the following functions without
>
>
> s/ove/move/

OK

>>
>> 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.
>
> s/function/functions/

OK

>
>>
>> * of_fpga_region_init
>> * of_fpga_region_exit
>>
>> Signed-off-by: Alan Tull <atull@xxxxxxxxxx>
>> ---
>> 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
>>
>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
>> index ad5448f..529b729 100644
>> --- a/drivers/fpga/Kconfig
>> +++ b/drivers/fpga/Kconfig
>> @@ -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.
>
>
> Support for loading FPGA images by applying a Device Tree Overlay.

OK

>
>>
>> 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.
>> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
>> index e09895f..8f56766 100644
>> --- a/drivers/fpga/Makefile
>> +++ b/drivers/fpga/Makefile
>> @@ -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
>> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
>> index 5f2fa5f6..afc6188 100644
>> --- a/drivers/fpga/fpga-region.c
>> +++ b/drivers/fpga/fpga-region.c
>> @@ -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(&region->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(&region->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 = &region->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,
>> - &region->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,
>> - &region->bridge_list);
>> -
>> - /* If any of the bridges are in use, give up */
>> - if (ret == -EBUSY) {
>> - fpga_bridges_put(&region->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 = &region->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 = &region->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(&region->bridge_list);
>> - fpga_bridges_put(&region->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(&region->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,
>> &region->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);
>> }
>> diff --git a/drivers/fpga/of-fpga-region.c b/drivers/fpga/of-fpga-region.c
>> new file mode 100644
>> index 0000000..1797057
>> --- /dev/null
>> +++ b/drivers/fpga/of-fpga-region.c
>> @@ -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(&region->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(&region->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 = &region->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,
>> + &region->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,
>> + &region->bridge_list);
>> +
>> + /* If any of the bridges are in use, give up */
>> + if (ret == -EBUSY) {
>> + fpga_bridges_put(&region->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 = &region->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 = &region->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(&region->bridge_list);
>> + fpga_bridges_put(&region->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;
>
>
> 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.

>
>> + }
>> +
>> + 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(&region->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,
>> &region->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 <atull@xxxxxxxxxx>");
>> +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 majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>