On Tue, Sep 22, 2015 at 10:21:10AM -0500, at...@opensource.altera.com wrote: > From: Alan Tull <at...@opensource.altera.com> > > API to support programming FPGA's. > > The following functions are exported as GPL: > * fpga_mgr_buf_load > Load fpga from image in buffer > > * fpga_mgr_firmware_load > Request firmware and load it to the FPGA. > > * fpga_mgr_register > * fpga_mgr_unregister > FPGA device drivers can be added by calling > fpga_mgr_register() to register a set of > fpga_manager_ops to do device specific stuff. > > * of_fpga_mgr_get > * fpga_mgr_put > Get/put a reference to a fpga manager. > > The following sysfs files are created: > * /sys/class/fpga_manager/<fpga>/name > Name of low level driver.
Don't 'struct device's already have a name? Why do you need another name
attribute?
>
> * /sys/class/fpga_manager/<fpga>/state
> State of fpga manager
>
> Signed-off-by: Alan Tull <at...@opensource.altera.com>
> Acked-by: Michal Simek <michal.si...@xilinx.com>
[..]
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -0,0 +1,382 @@
[..]
> +int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags, const char *buf,
> + size_t count)
> +{
> + struct device *dev = &mgr->dev;
> + int ret;
> +
> + if (!mgr)
> + return -ENODEV;
This seems overly defensive.
[..]
> +int fpga_mgr_firmware_load(struct fpga_manager *mgr, u32 flags,
> + const char *image_name)
> +{
> + struct device *dev = &mgr->dev;
> + const struct firmware *fw;
> + int ret;
> +
> + if (!mgr)
> + return -ENODEV;
Again; I'm of the opinion this is needlessly defensive.
> +
> + dev_info(dev, "writing %s to %s\n", image_name, mgr->name);
> +
> + mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ;
> +
> + ret = request_firmware(&fw, image_name, dev);
> + if (ret) {
> + mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ_ERR;
> + dev_err(dev, "Error requesting firmware %s\n", image_name);
> + return ret;
> + }
> +
> + ret = fpga_mgr_buf_load(mgr, flags, fw->data, fw->size);
> + if (ret)
> + return ret;
> +
> + release_firmware(fw);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(fpga_mgr_firmware_load);
> +
> +static const char * const state_str[] = {
> + [FPGA_MGR_STATE_UNKNOWN] = "unknown",
> + [FPGA_MGR_STATE_POWER_OFF] = "power off",
> + [FPGA_MGR_STATE_POWER_UP] = "power up",
> + [FPGA_MGR_STATE_RESET] = "reset",
> +
> + /* requesting FPGA image from firmware */
> + [FPGA_MGR_STATE_FIRMWARE_REQ] = "firmware request",
> + [FPGA_MGR_STATE_FIRMWARE_REQ_ERR] = "firmware request error",
> +
> + /* Preparing FPGA to receive image */
> + [FPGA_MGR_STATE_WRITE_INIT] = "write init",
> + [FPGA_MGR_STATE_WRITE_INIT_ERR] = "write init error",
> +
> + /* Writing image to FPGA */
> + [FPGA_MGR_STATE_WRITE] = "write",
> + [FPGA_MGR_STATE_WRITE_ERR] = "write error",
> +
> + /* Finishing configuration after image has been written */
> + [FPGA_MGR_STATE_WRITE_COMPLETE] = "write complete",
> + [FPGA_MGR_STATE_WRITE_COMPLETE_ERR] = "write complete error",
> +
> + /* FPGA reports to be in normal operating mode */
> + [FPGA_MGR_STATE_OPERATING] = "operating",
> +};
Is it really necessary to expose the whole FPGA manager state machine to
userspace? Is the only justification "for debugging"?
If so, that seems pretty short-sighted, as it then makes the state
machine part of the stable usermode API. It even makes less sense given
this patchsets current state: configuration of the FPGA is not something
that userspace is directly triggering.
> +
> +static ssize_t name_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct fpga_manager *mgr = to_fpga_manager(dev);
> +
> + return sprintf(buf, "%s\n", mgr->name);
> +}
> +
> +static ssize_t state_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct fpga_manager *mgr = to_fpga_manager(dev);
> +
> + return sprintf(buf, "%s\n", state_str[mgr->state]);
> +}
Is it possible that the state of the FPGA has changed since the last
time we've done an update? Should the lower-level drivers' state()
callback be invoked here?
> +
> +static DEVICE_ATTR_RO(name);
> +static DEVICE_ATTR_RO(state);
> +
> +static struct attribute *fpga_mgr_attrs[] = {
> + &dev_attr_name.attr,
> + &dev_attr_state.attr,
> + NULL,
> +};
> +ATTRIBUTE_GROUPS(fpga_mgr);
> +
> +static int fpga_mgr_of_node_match(struct device *dev, const void *data)
> +{
> + return dev->of_node == data;
> +}
> +
> +/**
> + * of_fpga_mgr_get - get an exclusive reference to a fpga mgr
> + * @node: device node
> + *
> + * Given a device node, get an exclusive reference to a fpga mgr.
> + *
> + * Return: fpga manager struct or IS_ERR() condition containing error code.
> + */
> +struct fpga_manager *of_fpga_mgr_get(struct device_node *node)
> +{
> + struct fpga_manager *mgr;
> + struct device *dev;
> +
> + if (!node)
> + return ERR_PTR(-EINVAL);
> +
> + dev = class_find_device(fpga_mgr_class, NULL, node,
> + fpga_mgr_of_node_match);
> + if (!dev)
> + return ERR_PTR(-ENODEV);
> +
> + mgr = to_fpga_manager(dev);
> + put_device(dev);
Who's ensuring the FPGA manager device's lifetime between _get and _put?
> + if (!mgr)
> + return ERR_PTR(-ENODEV);
> +
> + /* Get exclusive use of fpga manager */
> + if (!mutex_trylock(&mgr->ref_mutex))
> + return ERR_PTR(-EBUSY);
> +
> + if (!try_module_get(THIS_MODULE)) {
> + mutex_unlock(&mgr->ref_mutex);
> + return ERR_PTR(-ENODEV);
> + }
> +
> + return mgr;
> +}
[..]
> +int fpga_mgr_register(struct device *dev, const char *name,
> + const struct fpga_manager_ops *mops,
> + void *priv)
> +{
> + struct fpga_manager *mgr;
> + const char *dt_label;
> + int id, ret;
> +
> + if (!mops || !mops->write_init || !mops->write ||
> + !mops->write_complete || !mops->state) {
> + dev_err(dev, "Attempt to register without fpga_manager_ops\n");
> + return -EINVAL;
> + }
> +
> + if (!name || !strlen(name)) {
> + dev_err(dev, "Attempt to register with no name!\n");
> + return -EINVAL;
> + }
> +
> + mgr = kzalloc(sizeof(*mgr), GFP_KERNEL);
> + if (!mgr)
> + return -ENOMEM;
> +
> + id = ida_simple_get(&fpga_mgr_ida, 0, 0, GFP_KERNEL);
> + if (id < 0) {
> + ret = id;
> + goto error_kfree;
> + }
> +
> + mutex_init(&mgr->ref_mutex);
> +
> + mgr->name = name;
> + mgr->mops = mops;
> + mgr->priv = priv;
> +
> + /*
> + * Initialize framework state by requesting low level driver read state
> + * from device. FPGA may be in reset mode or may have been programmed
> + * by bootloader or EEPROM.
> + */
> + mgr->state = mgr->mops->state(mgr);
> +
> + device_initialize(&mgr->dev);
> + mgr->dev.class = fpga_mgr_class;
> + mgr->dev.parent = dev;
> + mgr->dev.of_node = dev->of_node;
> + mgr->dev.id = id;
> + dev_set_drvdata(dev, mgr);
> +
> + dt_label = of_get_property(mgr->dev.of_node, "label", NULL);
> + if (dt_label)
> + ret = dev_set_name(&mgr->dev, "%s", dt_label);
> + else
> + ret = dev_set_name(&mgr->dev, "fpga%d", id);
I'm wondering if an alias {} node is better for this.
[..]
> +++ b/include/linux/fpga/fpga-mgr.h
[..]
> +/*
> + * FPGA Manager flags
> + * FPGA_MGR_PARTIAL_RECONFIG: do partial reconfiguration if supported
> + */
> +#define FPGA_MGR_PARTIAL_RECONFIG BIT(0)
> +
> +/**
> + * struct fpga_manager_ops - ops for low level fpga manager drivers
> + * @state: returns an enum value of the FPGA's state
> + * @write_init: prepare the FPGA to receive confuration data
^configuration
> + * @write: write count bytes 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
Any specific state? State in the FPGA-manager-state-machine case? Or a
basic 'reset' state?
Josh
signature.asc
Description: PGP signature
