On 1/31/08, Stephen Neuendorffer <[EMAIL PROTECTED]> wrote:
> This includes code for new fifo-based xps_hwicap in addition to the
> older opb_hwicap, which has a significantly different interface. The
> common code between the two drivers is largely shared.
>
> Significant differences exists between this driver and what is
> supported in the EDK drivers. In particular, most of the
> architecture-specific code for reconfiguring individual FPGA resources
> has been removed. This functionality is likely better provided in a
> user-space support library. In addition, read and write access is
> supported. In addition, although the xps_hwicap cores support
> interrupt-driver mode, this driver only supports polled operation, in
> order to make the code simpler, and since the interrupt processing
> overhead is likely to slow down the throughput under Linux.
>
> Signed-off-by: Stephen Neuendorffer <[EMAIL PROTECTED]>
Comments below...
Also, I'm not sure if drivers/char/xilinx_hwicap is the best place for
this driver. drivers/misc/ might be better (but I'm not sure; poll
other people on this)
Finally, buffer_icap and fifo_icap are not huge blocks of code. Will
they be used by any other drivers? Can they be rolled into the
xilinx_hwicap.c file?
> +static int buffer_icap_device_read(struct hwicap_drvdata *drvdata,
> + u32 offset, u32 count)
> +{
> +
> + s32 retries = 0;
> + void __iomem *base_address = drvdata->base_address;
> +
> + if (buffer_icap_busy(base_address))
> + return -EBUSY;
> +
> + if ((offset + count) <= XHI_MAX_BUFFER_INTS) {
> + /* setSize count*4 to get bytes. */
> + buffer_icap_set_size(base_address, (count << 2));
> + buffer_icap_set_offset(base_address, offset);
> + buffer_icap_set_rnc(base_address, XHI_READBACK);
> +
> + while (buffer_icap_busy(base_address)) {
> + retries++;
> + if (retries > XHI_MAX_RETRIES)
> + return -EBUSY;
> + }
> + } else {
> + return -EINVAL;
> + }
Style comment: reverse the condition and bail with return -EINVAL at
the test point. It will simplify the code and better match with
common practice in the kernel.
> + return 0;
> +
> +};
> +
> +/**
> + * buffer_icap_device_write: Transfer bytes from ICAP to the storage buffer.
> + * @parameter drvdata: a pointer to the drvdata.
> + * @parameter offset: The storage buffer start address.
> + * @parameter count: The number of words (32 bit) to read from the
> + * device (ICAP).
> + **/
> +static int buffer_icap_device_write(struct hwicap_drvdata *drvdata,
> + u32 offset, u32 count)
> +{
> +
> + s32 retries = 0;
> + void __iomem *base_address = drvdata->base_address;
> +
> + if (buffer_icap_busy(base_address))
> + return -EBUSY;
> +
> + if ((offset + count) <= XHI_MAX_BUFFER_INTS) {
> + /* setSize count*4 to get bytes. */
> + buffer_icap_set_size(base_address, count << 2);
> + buffer_icap_set_offset(base_address, offset);
> + buffer_icap_set_rnc(base_address, XHI_CONFIGURE);
> +
> + while (buffer_icap_busy(base_address)) {
> + retries++;
> + if (retries > XHI_MAX_RETRIES)
> + return -EBUSY;
> + }
> + } else {
> + return -EINVAL;
> + }
Ditto
> +/**
> + * buffer_icap_set_configuration: Load a partial bitstream from system
> memory.
> + * @parameter drvdata: a pointer to the drvdata.
> + * @parameter data: Kernel address of the partial bitstream.
> + * @parameter size: the size of the partial bitstream in 32 bit words.
> + **/
> +int buffer_icap_set_configuration(struct hwicap_drvdata *drvdata, u32 *data,
> + u32 size)
> +{
> + int status;
> + s32 buffer_count = 0;
> + s32 num_writes = 0;
> + bool dirty = 0;
> + u32 i;
> + void __iomem *base_address = drvdata->base_address;
> +
> + /* Loop through all the data */
> + for (i = 0, buffer_count = 0; i < size; i++) {
> +
> + /* Copy data to bram */
> + buffer_icap_set_bram(base_address, buffer_count, data[i]);
> + dirty = 1;
> +
> + if (buffer_count == XHI_MAX_BUFFER_INTS - 1) {
> + /* Write data to ICAP */
> + status = buffer_icap_device_write(
> + drvdata,
> + XHI_BUFFER_START,
> + XHI_MAX_BUFFER_INTS);
> + if (status != 0) {
> + /* abort. */
> + buffer_icap_reset(drvdata);
> + return status;
> + }
> +
> + buffer_count = 0;
> + num_writes++;
> + dirty = 0;
> + } else {
> + buffer_count++;
> + }
Nit: Consider reversing the test condition and using 'buffer_count++; continue'
> +static ssize_t
> +hwicap_read(struct file *file, char *buf, size_t count, loff_t *ppos)
> +{
> + struct hwicap_drvdata *drvdata = file->private_data;
> + ssize_t bytes_to_read = 0;
> + u32 *kbuf;
> + u32 words;
> + u32 bytes_remaining;
> + int status;
> +
> + if (drvdata->is_accessing)
> + return -EBUSY;
> +
> + drvdata->is_accessing = 1;
Race condition, you need to use a semaphore. Otherwise it is possible
to get two processes (or even two threads of the same process) in the
read() hook.
> +static int hwicap_open(struct inode *inode, struct file *file)
> +{
> + struct hwicap_drvdata *drvdata;
> + int status;
> +
> + drvdata = container_of(inode->i_cdev, struct hwicap_drvdata, cdev);
> + if (drvdata->is_open)
> + return -EBUSY;
> +
> + drvdata->is_open = 1;
Also a race condition. Use a semaphore.
Also, this isn't sufficient to prevent 2 processes from having the
device open. If a process has already opened it and then calls
fork(), then *both* processes will have it opened even though the open
fop was only called once. (It might not matter for this particular
driver as the use case is limited; but it is something you should be
aware of.)
> +static int __devinit hwicap_setup(struct device *dev, int id,
> + const struct resource *regs_res,
> + const struct hwicap_driver_config *config,
> + const struct config_registers *config_regs)
> +{
> + dev_t devt;
> + struct hwicap_drvdata *drvdata = NULL;
> + int retval = 0;
> +
> + dev_info(dev, "Xilinx icap port driver\n");
> +
> + if (id < 0) {
> + for (id = 0; id < HWICAP_DEVICES; id++)
> + if (!probed_devices[id])
> + break;
> + }
> + if (id < 0 || id >= HWICAP_DEVICES) {
> + dev_err(dev, "%s%i too large\n", DRIVER_NAME, id);
> + return -EINVAL;
> + }
> + if (probed_devices[id]) {
> + dev_err(dev, "cannot assign to %s%i; it is already in use\n",
> + DRIVER_NAME, id);
> + return -EBUSY;
> + }
> +
> + probed_devices[id] = 1;
Hmmm, I'm not thrilled with the fixed array of HWICAP devices. That
sort of thing just ends up causing headaches down the road. Can the
driver just allocate a new structure and the next available ID at
probe time? (I hold my nose every time I write something like the
above because I know I'm going to regret it later). sysfs/udev can
provide details about which one is which.
> +
> +static int __devexit hwicap_remove(struct device *dev)
> +{
> + struct hwicap_drvdata *drvdata;
> +
> + drvdata = (struct hwicap_drvdata *)dev_get_drvdata(dev);
> +
> + if (drvdata) {
if (!drvdata)
return 0;
Cheers,
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
