| Message ID | 1476918586-13475-18-git-send-email-christopher.lee.bostic@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
Hi Chris, > diff > +static int scom_probe(struct device *); > + > +struct scom_op { > + uint64_t data; > + uint32_t addr; > +}; You're making this a userspace interface, so this should go into the uapi headers (and then will form part of the unchangeable Linux ABI). However, more on the ABI below... > +struct scom_device { > + struct fsi_device *fsi_dev; > + struct cdev *cdev; > +}; > + > +struct scom_device scom_dev; > + > +struct fsi_device_id scom_ids[] = { > + { > + .engine_type = FSI_ENGID_SCOM, > + .version = FSI_VERSION_ANY, > + }, > + { 0 } > +}; > + > +struct fsi_driver scom_drv = { > + .id_table = scom_ids, > + .drv = { > + .name = "scom", > + .bus = &fsi_bus_type, > + .probe = scom_probe, > + } > +}; Make these static, and put after the code, just before the module_fsi_driver. Then you won't need the forward definition of scom_probe. > > +static ssize_t scom_read(struct file *filep, char __user *buf, size_t len, > + loff_t *offset) > +{ > + int rc = 0; > + struct scom_op op; > + struct scom_device *scom = &scom_dev; > + struct device *dev = &scom->fsi_dev->dev; > + > + /* todo: Retrieve device to operate on from filep->private_data */ Yep, otherwise you only get one scom interface, and crazy results if you get probed twice. I'd suggest not leaving this until later. > + > + if (len != sizeof(struct scom_op)) > + return -EINVAL; > + > + rc = copy_from_user(&op, buf, len); > + if (rc) { > + dev_dbg(dev, "copy from user failed:%d\n", rc); > + return -EINVAL; > + } > + > + rc = get_scom(&op.data, op.addr); I'm not a huge fan of that ABI to userspace. You already have the address in the offset argument, so you can do this without needing new uapi headers, using something like: static ssize_t scom_read(struct file *filep, char __user *buf, size_t len, loff_t *offset) { uint64_t val; /* todo: can we do 1/2/4/8 byte operations too? */ if (len != sizeof(uint64_t)) return -EINVAL; rc = get_scom(&val, offset); if (rc) return rc; rc = copy_to_user(&val, buf, len); return rc ? rc : len; } Also, have you thought about endianness? > +static int scom_probe(struct device *dev) > +{ > + struct fsi_device *fsi_dev = to_fsi_dev(dev); > + struct scom_device *scom = &scom_dev; > + int rc; > + > + scom->fsi_dev = fsi_dev; > + scom->cdev = cdev_alloc(); > + if (!scom->cdev) > + return -ENOMEM; > + > + scom->cdev->owner = THIS_MODULE; > + > + rc = register_chrdev(0, "scom", &scom_fops); > + if (rc < 0) { > + dev_dbg(dev, "scom major allocation failed:%d\n", rc); > + return rc; > + } > + dev_dbg(dev, "scom major allocated:%d\n", rc); You need to register the actual device too, right? We'll want scom0, scom1, etc. Cheers, Jeremy
On Wed, Oct 19, 2016 at 10:27 PM, Jeremy Kerr <jk@ozlabs.org> wrote: > Hi Chris, > >> diff > +static int scom_probe(struct device *); >> + >> +struct scom_op { >> + uint64_t data; >> + uint32_t addr; >> +}; > > You're making this a userspace interface, so this should go into the > uapi headers (and then will form part of the unchangeable Linux ABI). > However, more on the ABI below... > >> +struct scom_device { >> + struct fsi_device *fsi_dev; >> + struct cdev *cdev; >> +}; >> + >> +struct scom_device scom_dev; >> + >> +struct fsi_device_id scom_ids[] = { >> + { >> + .engine_type = FSI_ENGID_SCOM, >> + .version = FSI_VERSION_ANY, >> + }, >> + { 0 } >> +}; >> + >> +struct fsi_driver scom_drv = { >> + .id_table = scom_ids, >> + .drv = { >> + .name = "scom", >> + .bus = &fsi_bus_type, >> + .probe = scom_probe, >> + } >> +}; > > Make these static, and put after the code, just before the > module_fsi_driver. Then you won't need the forward definition of > scom_probe. > Hi Jeremy, OK will change. >> > +static ssize_t scom_read(struct file *filep, char __user *buf, size_t len, >> + loff_t *offset) >> +{ >> + int rc = 0; >> + struct scom_op op; >> + struct scom_device *scom = &scom_dev; >> + struct device *dev = &scom->fsi_dev->dev; >> + >> + /* todo: Retrieve device to operate on from filep->private_data */ > > Yep, otherwise you only get one scom interface, and crazy results if you > get probed twice. I'd suggest not leaving this until later. > Will add. >> + >> + if (len != sizeof(struct scom_op)) >> + return -EINVAL; >> + >> + rc = copy_from_user(&op, buf, len); >> + if (rc) { >> + dev_dbg(dev, "copy from user failed:%d\n", rc); >> + return -EINVAL; >> + } >> + >> + rc = get_scom(&op.data, op.addr); > > I'm not a huge fan of that ABI to userspace. You already have the > address in the offset argument, so you can do this without needing new > uapi headers, using something like: > > static ssize_t scom_read(struct file *filep, char __user *buf, size_t len, > loff_t *offset) > { > uint64_t val; > > /* todo: can we do 1/2/4/8 byte operations too? */ > if (len != sizeof(uint64_t)) > return -EINVAL; > > rc = get_scom(&val, offset); > if (rc) > return rc; > > rc = copy_to_user(&val, buf, len); > > return rc ? rc : len; > } > OK, will change that. > > Also, have you thought about endianness? > > The plan is for endianness is to be dealt with in the fsi-core so the other engines don't need to worry about it. Yes there is likely to be some additions there. Will need to evaluate. >> +static int scom_probe(struct device *dev) >> +{ >> + struct fsi_device *fsi_dev = to_fsi_dev(dev); >> + struct scom_device *scom = &scom_dev; >> + int rc; >> + >> + scom->fsi_dev = fsi_dev; >> + scom->cdev = cdev_alloc(); >> + if (!scom->cdev) >> + return -ENOMEM; >> + >> + scom->cdev->owner = THIS_MODULE; >> + >> + rc = register_chrdev(0, "scom", &scom_fops); >> + if (rc < 0) { >> + dev_dbg(dev, "scom major allocation failed:%d\n", rc); >> + return rc; >> + } >> + dev_dbg(dev, "scom major allocated:%d\n", rc); > > You need to register the actual device too, right? We'll want scom0, > scom1, etc. Don't understand your meaning here. Also scom0 and scom1 refer to what exactly? For a first pass of core code I'd like to be able to deal with a single engine if possible and expand later. Thanks, Chris > > Cheers, > > > Jeremy
> >> +static int scom_probe(struct device *dev) >> +{ >> + struct fsi_device *fsi_dev = to_fsi_dev(dev); >> + struct scom_device *scom = &scom_dev; >> + int rc; >> + >> + scom->fsi_dev = fsi_dev; >> + scom->cdev = cdev_alloc(); >> + if (!scom->cdev) >> + return -ENOMEM; >> + >> + scom->cdev->owner = THIS_MODULE; >> + >> + rc = register_chrdev(0, "scom", &scom_fops); >> + if (rc < 0) { >> + dev_dbg(dev, "scom major allocation failed:%d\n", rc); >> + return rc; >> + } >> + dev_dbg(dev, "scom major allocated:%d\n", rc); > > You need to register the actual device too, right? We'll want scom0, > scom1, etc. Hi Jeremy, Is there any value in appending an FSI device path to scom? Like cat'ing the fsi device name on end. That's what is done in existing product and it makes it easy to tell what device is where in the topology. Thanks, Chris > > Cheers, > > > Jeremy
Hi Chris, >> Also, have you thought about endianness? > > The plan is for endianness is to be dealt with in the fsi-core > so the other engines don't need to worry about it. Yes there > is likely to be some additions there. Will need to evaluate. OK. >>> +static int scom_probe(struct device *dev) >>> +{ >>> + struct fsi_device *fsi_dev = to_fsi_dev(dev); >>> + struct scom_device *scom = &scom_dev; >>> + int rc; >>> + >>> + scom->fsi_dev = fsi_dev; >>> + scom->cdev = cdev_alloc(); >>> + if (!scom->cdev) >>> + return -ENOMEM; >>> + >>> + scom->cdev->owner = THIS_MODULE; >>> + >>> + rc = register_chrdev(0, "scom", &scom_fops); >>> + if (rc < 0) { >>> + dev_dbg(dev, "scom major allocation failed:%d\n", rc); >>> + return rc; >>> + } >>> + dev_dbg(dev, "scom major allocated:%d\n", rc); >> >> You need to register the actual device too, right? We'll want scom0, >> scom1, etc. > > Don't understand your meaning here. Also scom0 and scom1 refer to > what exactly? The actual devices. You're only registering a chardev major number there, not an actual character device, and so this doesn't result in any /dev/scom node being created. You'll want to do a device_create there, to get the minor allocated and have the device appear. However, that means you'll also need to create a class too. If you want to skip all that, just create a miscdev instead. That will take care of the major/minor numbers for you, and will go into the 'misc' device class. > For a first pass of core code I'd like to be able to deal with a > single engine if possible and expand later. There's a right and wrong way to do that though. Currently, it looks like you're intending to register a /dev/scom. Then, when you want to add multiple interfaces in future, that will have to be removed to create /dev/scom0, /dev/scom1, etc. That's a userspace ABI change, which could break old code. So, even if your driver only handles one device for now, name that in a way that doesn't conflict with that future interface. However, handling multiple devices is pretty trivial. Just allocate your device in the probe, rather than using one static instance, and name it accordingly. Here's what I'd suggest for your probe function: struct scom_device { struct fsi_device *fsi_dev; struct miscdevice mdev; char name[32]; }; static atomic_t scom_idx = ATOMIC_INIT(0); static int scom_probe(struct device *dev) { struct fsi_device *fsi_dev = to_fsi_dev(dev); struct scom_device *scom; int rc; scom = devm_kzalloc(dev, sizeof(*scom), GFP_KERNEL); if (!scom) return -ENOMEM; snprintf(scom->name, sizeof(scom->name), "scom%d", atomic_inc_return(&scom_idx)); scom->fsi_dev = fsi_dev; scom->mdev.minor = MISC_DYNAMIC_MINOR; scom->mdev.fops = &scom_fops; scom->mdev.name = scom->name; scom->mdev.parent = dev; return misc_register(&scom->mdev); } Then, in your read/write functions, file->private_data is your mdev, and you can just do a container_of to get the struct scom_device *. From your next email: > Is there any value in appending an FSI device path to scom? Like > cat'ing the fsi device name on end. That's what is done in existing > product and it makes it easy to tell what device is where in the > topology. No, that's not normal convention for providing that sort of information. If we set the device parent (ie, scom->mdev.parent), the device will be referenced correctly through sysfs, so we have the correct hardware bus layout available there. For example: # cat /sys/bus/fsi/devices/00:00:00:00/misc/scom1/dev 10:62 # ls -l /dev/scom1 crw------- 1 root root 10, 62 Jan 1 00:00 /dev/scom1 Then, if we want symlinks in dev, we add the appropriate udev rules to create those according to system policy. The names in dev should be fairly dynamic (eg, /dev/sdXN, /dev/ipmiN, etc) [One advantage of using a chardev with a proper class is that 'misc' class directory in the sysfs path could be named something more appropriate...] Cheers, Jeremy
On Fri, Oct 21, 2016 at 12:24 AM, Jeremy Kerr <jk@ozlabs.org> wrote: > Hi Chris, > >>> Also, have you thought about endianness? >> >> The plan is for endianness is to be dealt with in the fsi-core >> so the other engines don't need to worry about it. Yes there >> is likely to be some additions there. Will need to evaluate. > > OK. > >>>> +static int scom_probe(struct device *dev) >>>> +{ >>>> + struct fsi_device *fsi_dev = to_fsi_dev(dev); >>>> + struct scom_device *scom = &scom_dev; >>>> + int rc; >>>> + >>>> + scom->fsi_dev = fsi_dev; >>>> + scom->cdev = cdev_alloc(); >>>> + if (!scom->cdev) >>>> + return -ENOMEM; >>>> + >>>> + scom->cdev->owner = THIS_MODULE; >>>> + >>>> + rc = register_chrdev(0, "scom", &scom_fops); >>>> + if (rc < 0) { >>>> + dev_dbg(dev, "scom major allocation failed:%d\n", rc); >>>> + return rc; >>>> + } >>>> + dev_dbg(dev, "scom major allocated:%d\n", rc); >>> >>> You need to register the actual device too, right? We'll want scom0, >>> scom1, etc. >> >> Don't understand your meaning here. Also scom0 and scom1 refer to >> what exactly? > > The actual devices. You're only registering a chardev major number > there, not an actual character device, and so this doesn't result in any > /dev/scom node being created. > > You'll want to do a device_create there, to get the minor allocated and > have the device appear. However, that means you'll also need to create a > class too. > > If you want to skip all that, just create a miscdev instead. That will > take care of the major/minor numbers for you, and will go into the > 'misc' device class. > Hi Jeremy, Ok, I've added this change for the next set to review. >> For a first pass of core code I'd like to be able to deal with a >> single engine if possible and expand later. > > There's a right and wrong way to do that though. Currently, it looks > like you're intending to register a /dev/scom. Then, when you want to > add multiple interfaces in future, that will have to be removed to > create /dev/scom0, /dev/scom1, etc. That's a userspace ABI change, which > could break old code. > > So, even if your driver only handles one device for now, name that in a > way that doesn't conflict with that future interface. > > However, handling multiple devices is pretty trivial. Just allocate your > device in the probe, rather than using one static instance, and name it > accordingly. Here's what I'd suggest for your probe function: > > struct scom_device { > struct fsi_device *fsi_dev; > struct miscdevice mdev; > char name[32]; > }; > > static atomic_t scom_idx = ATOMIC_INIT(0); > > static int scom_probe(struct device *dev) > { > struct fsi_device *fsi_dev = to_fsi_dev(dev); > struct scom_device *scom; > int rc; > > scom = devm_kzalloc(dev, sizeof(*scom), GFP_KERNEL); > if (!scom) > return -ENOMEM; > > snprintf(scom->name, sizeof(scom->name), > "scom%d", atomic_inc_return(&scom_idx)); > scom->fsi_dev = fsi_dev; > scom->mdev.minor = MISC_DYNAMIC_MINOR; > scom->mdev.fops = &scom_fops; > scom->mdev.name = scom->name; > scom->mdev.parent = dev; > > return misc_register(&scom->mdev); > } > > > Then, in your read/write functions, file->private_data is your mdev, and > you can just do a container_of to get the struct scom_device *. > Added these changes as well to the next patch set. > From your next email: > >> Is there any value in appending an FSI device path to scom? Like >> cat'ing the fsi device name on end. That's what is done in existing >> product and it makes it easy to tell what device is where in the >> topology. > > No, that's not normal convention for providing that sort of information. > If we set the device parent (ie, scom->mdev.parent), the device will be > referenced correctly through sysfs, so we have the correct hardware bus > layout available there. For example: > > # cat /sys/bus/fsi/devices/00:00:00:00/misc/scom1/dev > 10:62 > # ls -l /dev/scom1 > crw------- 1 root root 10, 62 Jan 1 00:00 /dev/scom1 > > Then, if we want symlinks in dev, we add the appropriate udev rules to > create those according to system policy. The names in dev should be > fairly dynamic (eg, /dev/sdXN, /dev/ipmiN, etc) > > [One advantage of using a chardev with a proper class is that 'misc' > class directory in the sysfs path could be named something more > appropriate...] > Thanks for the clarification. Chris. > Cheers, > > > Jeremy
diff --git a/drivers/fsi/Kconfig b/drivers/fsi/Kconfig index 69e7ee8..719a217 100644 --- a/drivers/fsi/Kconfig +++ b/drivers/fsi/Kconfig @@ -24,6 +24,12 @@ config FSI_MASTER_GPIO select GPIO_DEVRES ---help--- This option enables a FSI master driver, using GPIO lines directly. + +config FSI_SCOM + tristate "SCOM FSI client" + depends on FSI + ---help--- + This option enables the SCOM FSI client device driver. endif endmenu diff --git a/drivers/fsi/Makefile b/drivers/fsi/Makefile index 2021ce5..3e31d9a 100644 --- a/drivers/fsi/Makefile +++ b/drivers/fsi/Makefile @@ -2,3 +2,4 @@ obj-$(CONFIG_FSI) += fsi-core.o obj-$(CONFIG_FSI_MASTER_FAKE) += fsi-master-fake.o obj-$(CONFIG_FSI_MASTER_GPIO) += fsi-master-gpio.o +obj-$(CONFIG_FSI_SCOM) += fsi-scom.o diff --git a/drivers/fsi/fsi-scom.c b/drivers/fsi/fsi-scom.c new file mode 100644 index 0000000..5ee12a9 --- /dev/null +++ b/drivers/fsi/fsi-scom.c @@ -0,0 +1,217 @@ +/* + * SCOM FSI Client device driver + * + * Copyright (C) IBM Corporation 2016 + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERGCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/fsi.h> +#include <linux/module.h> +#include <linux/cdev.h> +#include <linux/delay.h> +#include <linux/fs.h> +#include <linux/uaccess.h> + +#define FSI_ENGID_SCOM 0x5 + +#define SCOM_FSI2PIB_DELAY 50 + +/* SCOM engine register set */ +#define SCOM_DATA0_REG 0x00 +#define SCOM_DATA1_REG 0x04 +#define SCOM_CMD_REG 0x08 +#define SCOM_RESET_REG 0x1C + +#define SCOM_RESET_CMD 0x80000000 +#define SCOM_WRITE_CMD 0x80000000 + +static int scom_probe(struct device *); + +struct scom_op { + uint64_t data; + uint32_t addr; +}; + +struct scom_device { + struct fsi_device *fsi_dev; + struct cdev *cdev; +}; + +struct scom_device scom_dev; + +struct fsi_device_id scom_ids[] = { + { + .engine_type = FSI_ENGID_SCOM, + .version = FSI_VERSION_ANY, + }, + { 0 } +}; + +struct fsi_driver scom_drv = { + .id_table = scom_ids, + .drv = { + .name = "scom", + .bus = &fsi_bus_type, + .probe = scom_probe, + } +}; + +static int put_scom(uint64_t value, uint32_t addr) +{ + int rc; + uint32_t data = SCOM_RESET_CMD; + struct scom_device *scom = &scom_dev; + + rc = fsi_device_write(scom->fsi_dev, SCOM_RESET_REG, &data, + sizeof(uint32_t)); + if (rc) + return rc; + + data = (value >> 32) & 0xffffffff; + rc = fsi_device_write(scom->fsi_dev, SCOM_DATA0_REG, &data, + sizeof(uint32_t)); + if (rc) + return rc; + + data = value & 0xffffffff; + rc = fsi_device_write(scom->fsi_dev, SCOM_DATA1_REG, &data, + sizeof(uint32_t)); + if (rc) + return rc; + + data = SCOM_WRITE_CMD | addr; + return fsi_device_write(scom->fsi_dev, SCOM_CMD_REG, &data, + sizeof(uint32_t)); +} + + +static int get_scom(uint64_t *value, uint32_t addr) +{ + uint32_t result, data; + struct scom_device *scom = &scom_dev; + int rc; + + udelay(SCOM_FSI2PIB_DELAY); + + data = addr; + rc = fsi_device_write(scom->fsi_dev, SCOM_CMD_REG, &data, + sizeof(uint32_t)); + if (rc) + return rc; + + rc = fsi_device_read(scom->fsi_dev, SCOM_DATA0_REG, &result, + sizeof(uint32_t)); + if (rc) + return rc; + + *value |= (uint64_t) result << 32; + rc = fsi_device_read(scom->fsi_dev, SCOM_DATA1_REG, &result, + sizeof(uint32_t)); + if (rc) + return rc; + + *value |= result; + + return 0; +} + +static ssize_t scom_read(struct file *filep, char __user *buf, size_t len, + loff_t *offset) +{ + int rc = 0; + struct scom_op op; + struct scom_device *scom = &scom_dev; + struct device *dev = &scom->fsi_dev->dev; + + /* todo: Retrieve device to operate on from filep->private_data */ + + if (len != sizeof(struct scom_op)) + return -EINVAL; + + rc = copy_from_user(&op, buf, len); + if (rc) { + dev_dbg(dev, "copy from user failed:%d\n", rc); + return -EINVAL; + } + + rc = get_scom(&op.data, op.addr); + if (rc) { + dev_dbg(dev, "get_scom failed with:%d\n", rc); + return -EINVAL; + } + + rc = copy_to_user(buf, &op, sizeof(struct scom_op)); + if (rc) { + dev_dbg(dev, "copy to user failed:%d\n", rc); + return -EIO; + } + + return len; +} + +static ssize_t scom_write(struct file *filep, const char __user *buf, + size_t len, loff_t *offset) +{ + int rc = 0; + struct scom_op op; + struct scom_device *scom = &scom_dev; + struct device *dev = &scom->fsi_dev->dev; + + /* todo: Retrieve device to operate on from filep->private_data */ + + if (len != sizeof(struct scom_op)) + return -EINVAL; + + rc = copy_from_user(&op, buf, sizeof(uint64_t)); + if (rc) { + dev_dbg(dev, "copy from user failed:%d\n", rc); + return -EINVAL; + } + + rc = put_scom(op.data, op.addr); + if (rc) { + dev_dbg(dev, "put_scom failed with:%d\n", rc); + return -EIO; + } + + return len; +} + +static const struct file_operations scom_fops = { + .owner = THIS_MODULE, + .read = scom_read, + .write = scom_write, +}; + +static int scom_probe(struct device *dev) +{ + struct fsi_device *fsi_dev = to_fsi_dev(dev); + struct scom_device *scom = &scom_dev; + int rc; + + scom->fsi_dev = fsi_dev; + scom->cdev = cdev_alloc(); + if (!scom->cdev) + return -ENOMEM; + + scom->cdev->owner = THIS_MODULE; + + rc = register_chrdev(0, "scom", &scom_fops); + if (rc < 0) { + dev_dbg(dev, "scom major allocation failed:%d\n", rc); + return rc; + } + dev_dbg(dev, "scom major allocated:%d\n", rc); + + return 0; +} + +module_fsi_driver(scom_drv);