On Tue, Oct 17, 2017 at 04:20:23PM -0500, Alan Tull wrote:
> New 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 <at...@kernel.org>
Acked-by: Moritz Fischer <m...@kernel.org>
> ---
> v2: split out from another patch
> v3: update comments
> minor changes in flow
> v4: no change to this patch in this version of patchset
> v5: no change to this patch in this version of patchset
> ---
> drivers/fpga/fpga-region.c | 122
> +++++++++++++++++++++++++++------------------
> 1 file changed, 73 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
> index eaacf50..2a8621d 100644
> --- a/drivers/fpga/fpga-region.c
> +++ b/drivers/fpga/fpga-region.c
> @@ -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;
> }
> --
> 2.7.4
>
