[PATCH v2 5/5] drivers/misc: Add aspeed ast2400/ast2500 lpc controlling driver
Cyril Bur
cyrilbur at gmail.com
Fri Dec 23 19:04:26 AEDT 2016
On Fri, 2016-12-23 at 13:25 +1030, Andrew Jeffery wrote:
> On Thu, 2016-12-22 at 17:06 +1100, Cyril Bur wrote:
> > This driver exposes a reserved chunk of BMC ram to userspace as well as
> > an ioctl interface to control the BMC<->HOST mapping of the LPC bus.
> > This allows for a communication channel between the BMC and the host
> >
> > > Signed-off-by: Cyril Bur <cyrilbur at gmail.com>
> >
> > ---
> > drivers/misc/Kconfig | 9 ++
> > drivers/misc/Makefile | 1 +
> > drivers/misc/aspeed-lpc-ctrl.c | 292 +++++++++++++++++++++++++++++++++++
> > include/uapi/linux/aspeed-lpc-ctrl.h | 25 +++
> > 4 files changed, 327 insertions(+)
> > create mode 100644 drivers/misc/aspeed-lpc-ctrl.c
> > create mode 100644 include/uapi/linux/aspeed-lpc-ctrl.h
> >
> > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> > index a216b4667742..f1e1c043d91c 100644
> > --- a/drivers/misc/Kconfig
> > +++ b/drivers/misc/Kconfig
> > @@ -804,6 +804,15 @@ config PANEL_BOOT_MESSAGE
> > > An empty message will only clear the display at driver init time. Any other
> > > printf()-formatted message is valid with newline and escape codes.
> >
> >
> > +config ASPEED_LPC_CTRL
> > + depends on ARCH_ASPEED || COMPILE_TEST
>
> Something that I should have mentioned on 4/5 as well is that we now
> need to say:
>
> depends on REGMAP && MFD_SYSCON
>
Yeah my bad should have figured that out.
Thanks
> > + bool "Build a driver to control the BMC to HOST LPC bus"
> > > + default "y"
> > > + ---help---
> > > + Provides a driver to control BMC to HOST LPC mappings through
> > > + ioctl()s, the driver aso provides a read/write interface to a BMC ram
> > > + region where host LPC can be buffered.
> >
> > +
> > source "drivers/misc/c2port/Kconfig"
> > source "drivers/misc/eeprom/Kconfig"
> > source "drivers/misc/cb710/Kconfig"
> > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> > index b2fb6dbffcef..cdcd1af48971 100644
> > --- a/drivers/misc/Makefile
> > +++ b/drivers/misc/Makefile
> > > @@ -57,3 +57,4 @@ obj-$(CONFIG_ECHO) += echo/
> > > obj-$(CONFIG_VEXPRESS_SYSCFG) += vexpress-syscfg.o
> > > obj-$(CONFIG_CXL_BASE) += cxl/
> >
> > obj-$(CONFIG_PANEL) += panel.o
> > > +obj-$(CONFIG_ASPEED_LPC_CTRL) += aspeed-lpc-ctrl.o
> >
> > diff --git a/drivers/misc/aspeed-lpc-ctrl.c b/drivers/misc/aspeed-lpc-ctrl.c
> > new file mode 100644
> > index 000000000000..4698d8fa5a4c
> > --- /dev/null
> > +++ b/drivers/misc/aspeed-lpc-ctrl.c
> > @@ -0,0 +1,292 @@
> > +/*
> > + * Copyright 2016 IBM Corporation
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version
> > + * 2 of the License, or (at your option) any later version.
> > + */
> > +
> > +#include <linux/mm.h>
> > +#include <linux/module.h>
> > +#include <linux/moduleparam.h>
> > +#include <linux/errno.h>
> > +#include <linux/poll.h>
> > +#include <linux/sched.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/slab.h>
> > +#include <linux/init.h>
> > +#include <linux/device.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/io.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/delay.h>
> > +#include <linux/miscdevice.h>
> > +#include <linux/mtd/mtd.h>
> > +#include <linux/timer.h>
> > +#include <linux/jiffies.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/regmap.h>
> > +#include <linux/aspeed-lpc-ctrl.h>
> > +
> > > +#define DEVICE_NAME "aspeed-lpc-ctrl"
> >
> > +
> > +#define HICR7 0x8
> > +#define HICR8 0xc
> > +
> > +struct lpc_ctrl {
> > > > + struct miscdevice miscdev;
> > > > + struct regmap *regmap;
> > > > + phys_addr_t base;
> > > > + resource_size_t size;
> > > > + uint32_t pnor_size;
> > > > + uint32_t pnor_base;
> >
> > +};
> > +
> > +static atomic_t lpc_ctrl_open_count = ATOMIC_INIT(0);
> > +
> > +static struct lpc_ctrl *file_lpc_ctrl(struct file *file)
> > +{
> > > + return container_of(file->private_data, struct lpc_ctrl, miscdev);
> >
> > +}
> > +
> > +static int lpc_ctrl_mmap(struct file *file, struct vm_area_struct *vma)
> > +{
> > > + struct lpc_ctrl *lpc_ctrl = file_lpc_ctrl(file);
> > > + unsigned long vsize = vma->vm_end - vma->vm_start;
> >
> > +
> > > + if (vma->vm_pgoff + vsize > lpc_ctrl->base + lpc_ctrl->size)
> > > + return -EINVAL;
> >
> > +
> > > + /* Other checks? */
> >
> > +
> > > + if (remap_pfn_range(vma, vma->vm_start,
> > > + (lpc_ctrl->base >> PAGE_SHIFT) + vma->vm_pgoff,
> > > + vsize, vma->vm_page_prot))
> > > + return -EAGAIN;
> >
> > +
> > > + return 0;
> >
> > +}
> > +
> > +static int lpc_ctrl_open(struct inode *inode, struct file *file)
> > +{
> > > + if (atomic_inc_return(&lpc_ctrl_open_count) == 1)
> > > + return 0;
> >
> > +
> > > + atomic_dec(&lpc_ctrl_open_count);
> > > + return -EBUSY;
> >
> > +}
> > +
> > +static ssize_t lpc_ctrl_read(struct file *file, char __user *buf,
> > > + size_t count, loff_t *ppos)
> >
> > +{
> > > + if (!access_ok(VERIFY_WRITE, buf, count))
> > > + return -EFAULT;
> >
> > +
> > > + WARN_ON(*ppos);
> >
> > +
> > > + return -EPERM;
> >
> > +}
> > +
> > +static ssize_t lpc_ctrl_write(struct file *file, const char __user *buf,
> > > + size_t count, loff_t *ppos)
> >
> > +{
> > > + if (!access_ok(VERIFY_READ, buf, count))
> > > + return -EFAULT;
> >
> > +
> > > + WARN_ON(*ppos);
> >
> > +
> > > + return -EPERM;
> >
> > +}
> > +
> > +static long lpc_ctrl_ioctl(struct file *file, unsigned int cmd,
> > > + unsigned long param)
> >
> > +{
> > > + long rc;
> > > + struct lpc_mapping map;
> > > + struct lpc_ctrl *lpc_ctrl = file_lpc_ctrl(file);
> > > + void __user *p = (void __user *)param;
> >
> > +
> > > + switch (cmd) {
> > > + case LPC_CTRL_IOCTL_SIZE:
> > > + return copy_to_user(p, &lpc_ctrl->size,
> > > + sizeof(lpc_ctrl->size)) ? -EFAULT : 0;
> > > + case LPC_CTRL_IOCTL_MAP:
> > > + if (copy_from_user(&map, p, sizeof(map)))
> > > + return -EFAULT;
> >
> > +
> > +
>
> Whitespace (double line break).
>
Thanks
> > + /*
> > > + * The top half of HICR7 is the MSB of the BMC address of the
> > > + * mapping.
> > > + * The bottom half of HICR7 is the MSB of the HOST LPC
> > > + * firmware space address of the mapping.
> > > + *
> > > + * The 1 bits in the top of half of HICR8 represent the bits
> > > + * (in the requested address) that should be ignored and
> > > + * replaced with those from the top half of HICR7.
> > > + * The 1 bits in the bottom half of HICR8 represent the bits
> > > + * (in the requested address) that should be kept and pass
> > > + * into the BMC address space.
> >
> > + */
>
> If only we could put this comment into the datasheet and re-publish it.
>
Glad it makes sense!
> > +
> > > + regmap_write(lpc_ctrl->regmap, HICR7,
> > > + (lpc_ctrl->base | (map.hostaddr >> 16)));
> > > + regmap_write(lpc_ctrl->regmap, HICR8,
> >
> > + (~(map.size - 1)) | ((map.size >> 16) - 1));
>
> What are your thoughts on error handling
>
I'll do the same as unmap.
> > + return 0;
> > > + case LPC_CTRL_IOCTL_UNMAP:
> > > + /*
> > > + * The top nibble in host lpc addresses references which
> > > + * firmware space, use space zero hence the & 0x0fff
> > > + */
> >
> > +
> > > + dev_info(lpc_ctrl->miscdev.parent, "Setting HICR7 to 0x%08x\n",
> >
> > + lpc_ctrl->pnor_base | (((-lpc_ctrl->pnor_size) >> 16) & 0x0fff));
>
> Lets drop the dev_info()s, we've confirmed this works with the fixes to
> the mbox driver.
Yep.
>
> > + rc = regmap_write(lpc_ctrl->regmap, HICR7,
> > > + lpc_ctrl->pnor_base | (((-lpc_ctrl->pnor_size) >> 16) & 0x0fff));
> > > + if (rc)
> > > + return rc;
> > > + dev_info(lpc_ctrl->miscdev.parent, "Setting HICR8 to 0x%08x\n",
> > > + (~(lpc_ctrl->pnor_size - 1)) | ((lpc_ctrl->pnor_size >> 16) - 1));
> > > + rc = regmap_write(lpc_ctrl->regmap, HICR8,
> > > + (~(lpc_ctrl->pnor_size - 1)) | ((lpc_ctrl->pnor_size >> 16) - 1));
> > > + return rc;
> > > + }
> >
> > +
> > > + return -EINVAL;
> >
> > +}
> > +
> > +static int lpc_ctrl_release(struct inode *inode, struct file *file)
> > +{
> > > + atomic_dec(&lpc_ctrl_open_count);
> > > + return 0;
> >
> > +}
> > +
> > +static const struct file_operations lpc_ctrl_fops = {
> > > > + .owner = THIS_MODULE,
> > > > + .mmap = lpc_ctrl_mmap,
> > > > + .open = lpc_ctrl_open,
> > > > + .read = lpc_ctrl_read,
> > > > + .write = lpc_ctrl_write,
> > > > + .release = lpc_ctrl_release,
> > > > + .unlocked_ioctl = lpc_ctrl_ioctl,
> >
> > +};
> > +
> > +static int lpc_ctrl_probe(struct platform_device *pdev)
> > +{
> > > + struct lpc_ctrl *lpc_ctrl;
> > > + struct device *dev;
> > > + struct device_node *node;
> > > + struct resource resm;
> > > + struct mtd_info *mtd;
> > > + int rc;
> >
> > +
> > > + if (!pdev || !pdev->dev.of_node)
> > > + return -ENODEV;
> >
> > +
> > > > + mtd = get_mtd_device_nm("1e630000.spi:pnor at 0");
> > >
> > > + if (IS_ERR(mtd)) {
> > > + dev_err(&pdev->dev, "Couldn't find pnor\n");
> > > + return -EPROBE_DEFER;
> > > + }
> >
> > +
> > > + dev = &pdev->dev;
> >
> > +
> > > + lpc_ctrl = devm_kzalloc(dev, sizeof(*lpc_ctrl), GFP_KERNEL);
> > > + if (!lpc_ctrl)
> > > + return -ENOMEM;
> >
> > +
> > > + dev_set_drvdata(&pdev->dev, lpc_ctrl);
> >
> > +
> > > + node = of_get_parent(mtd_get_of_node(mtd));
> > > + if (!node) {
> > > + dev_err(dev, "Couldn't get MTD parent OF node\n");
> > > + return -ENODEV;
> > > + }
> > > + lpc_ctrl->pnor_size = mtd->size;
> > > + rc = of_property_read_u32_index(node, "reg", 2,
> > > + &lpc_ctrl->pnor_base);
> > > + if (rc)
> > > + return rc;
> >
> > +
> > > + dev_info(dev, "Host PNOR base: 0x%08x size: 0x%08x\n",
> > > + lpc_ctrl->pnor_base, lpc_ctrl->pnor_size);
> >
> > +
> > > + node = of_parse_phandle(dev->of_node, "memory-region", 0);
> > > + if (!node) {
> > > + /*
> > > + * Should probaby handle this by allocating 4-64k now and
> > > + * using that
> > > + */
> >
> > + dev_err(dev, "Didn't find reserved memory\n");
>
> I think this is good enough for the moment?
>
Yep, would you prefer I remove the comment? My userspace daemon
wouldn't handle a buffer smaller than flash so even if we wrote it we
couldn't use it.
> > + rc = -EINVAL;
> > > + goto out;
> > > + }
> >
> > +
> > > + rc = of_address_to_resource(node, 0, &resm);
> > > + of_node_put(node);
> > > + if (rc) {
> > > + dev_err(dev, "Could address to resource\n");
> > > + rc = -ENOMEM;
> > > + goto out;
> > > + }
> >
> > +
> > > + lpc_ctrl->size = resource_size(&resm);
> > > + lpc_ctrl->base = resm.start;
> >
> > +
> > > + lpc_ctrl->regmap = syscon_node_to_regmap(
> > > + pdev->dev.parent->of_node);
> > > + if (IS_ERR(lpc_ctrl->regmap)) {
> > > + dev_err(dev, "Couldn't get regmap\n");
> > > + rc = -ENODEV;
> > > + goto out;
> > > + }
> >
> > +
> > > + lpc_ctrl->miscdev.minor = MISC_DYNAMIC_MINOR;
> > > + lpc_ctrl->miscdev.name = DEVICE_NAME;
> > > + lpc_ctrl->miscdev.fops = &lpc_ctrl_fops;
> > > + lpc_ctrl->miscdev.parent = dev;
> > > + rc = misc_register(&lpc_ctrl->miscdev);
> > > + if (rc)
> > > + dev_err(dev, "Unable to register device\n");
> > > + else
> > > + dev_info(dev, "Loaded at 0x%08x (0x%08x)\n",
> > > + lpc_ctrl->base, lpc_ctrl->size);
> >
> > +
> > +out:
> > > + return rc;
> >
> > +}
> > +
> > +static int lpc_ctrl_remove(struct platform_device *pdev)
> > +{
> > > + struct lpc_ctrl *lpc_ctrl = dev_get_drvdata(&pdev->dev);
> >
> > +
> > + misc_deregister(&lpc_ctrl->miscdev);
>
> It feels like there should be a devm_misc_register()...
>
Uh sorry?
> > + lpc_ctrl = NULL;
> > +
> > > + return 0;
> >
> > +}
> > +
> > +static const struct of_device_id lpc_ctrl_match[] = {
> > + { .compatible = "aspeed,ast2400-lpc-ctrl" },
>
> Another issue I meant to mention on 4/5 was we'll need bindings
> documentation for these compatible strings.
>
> It's probably also worth adding the AST2500.
>
True,
Thanks,
Cyril
> > + { },
> > +};
> > +
> > +static struct platform_driver lpc_ctrl_driver = {
> > > + .driver = {
> > > > + .name = DEVICE_NAME,
> > >
> > > + .of_match_table = lpc_ctrl_match,
> > > + },
> > > + .probe = lpc_ctrl_probe,
> > > + .remove = lpc_ctrl_remove,
> >
> > +};
> > +
> > +module_platform_driver(lpc_ctrl_driver);
> > +
> > +MODULE_DEVICE_TABLE(of, lpc_ctrl_match);
> > +MODULE_LICENSE("GPL");
> > > +MODULE_AUTHOR("Cyril Bur <cyrilbur at gmail.com>");
> >
> > +MODULE_DESCRIPTION("Linux device interface to control LPC bus");
> > diff --git a/include/uapi/linux/aspeed-lpc-ctrl.h b/include/uapi/linux/aspeed-lpc-ctrl.h
> > new file mode 100644
> > index 000000000000..c5f1caf827ac
> > --- /dev/null
> > +++ b/include/uapi/linux/aspeed-lpc-ctrl.h
> > @@ -0,0 +1,25 @@
> > +/*
> > + * Copyright 2016 IBM Corp.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version
> > + * 2 of the License, or (at your option) any later version.
> > + */
> > +
> > +#ifndef _UAPI_LINUX_LPC_CTRL_H
> > +#define _UAPI_LINUX_LPC_CTRL_H
> > +
> > +#include <linux/ioctl.h>
> > +
> > +struct lpc_mapping {
> > > + uint32_t hostaddr;
> > > + uint32_t size;
> >
> > +};
> > +
> > > +#define __LPC_CTRL_IOCTL_MAGIC 0xb2
> > > +#define LPC_CTRL_IOCTL_SIZE _IOR(__LPC_CTRL_IOCTL_MAGIC, 0x00, uint32_t)
> > > +#define LPC_CTRL_IOCTL_MAP _IOW(__LPC_CTRL_IOCTL_MAGIC, 0x01, struct lpc_mapping)
> > > +#define LPC_CTRL_IOCTL_UNMAP _IO(__LPC_CTRL_IOCTL_MAGIC, 0x02)
> >
> > +
> > +#endif /* _UAPI_LINUX_LPC_CTRL_H */
>
> Cheers,
>
> Andrew
More information about the openbmc
mailing list