Message ID | 1472664259-23933-2-git-send-email-clg@kaod.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Wednesday, August 31, 2016 7:24:17 PM CEST Cédric Le Goater wrote: > From: Alistair Popple <alistair@popple.id.au> > > This patch adds a simple device driver to expose the iBT interface on > Aspeed chips as a character device (/dev/bt). > > The iBT interface is used to perform in-band IPMI communication from a > BMC to the host. > > Signed-off-by: Alistair Popple <alistair@popple.id.au> > Signed-off-by: Jeremy Kerr <jk@ozlabs.org> > Signed-off-by: Joel Stanley <joel@jms.id.au> > [clg: checkpatch fixes > devicetree binding documentation] > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > .../devicetree/bindings/misc/aspeed,bt-host.txt | 19 + > drivers/misc/Kconfig | 5 + > drivers/misc/Makefile | 1 + > drivers/misc/bt-host.c | 433 +++++++++++++++++++++ > include/uapi/linux/Kbuild | 1 + > include/uapi/linux/bt-host.h | 18 + > 6 files changed, 477 insertions(+) > create mode 100644 Documentation/devicetree/bindings/misc/aspeed,bt-host.txt > create mode 100644 drivers/misc/bt-host.c > create mode 100644 include/uapi/linux/bt-host.h > > diff --git a/Documentation/devicetree/bindings/misc/aspeed,bt-host.txt b/Documentation/devicetree/bindings/misc/aspeed,bt-host.txt > new file mode 100644 > index 000000000000..938c5998c331 > --- /dev/null > +++ b/Documentation/devicetree/bindings/misc/aspeed,bt-host.txt "misc" seems like a bad category here. Does this fit nowhere else? > @@ -0,0 +1,19 @@ > +* Aspeed BT IPMI interface What does "BT" stand for? IPMI is a more commonly known acronym, but maybe list both with their full name as well. > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile > index 7410c6d9a34d..71a7b9feb0f0 100644 > --- a/drivers/misc/Makefile > +++ b/drivers/misc/Makefile > @@ -57,6 +57,7 @@ obj-$(CONFIG_ECHO) += echo/ > obj-$(CONFIG_VEXPRESS_SYSCFG) += vexpress-syscfg.o > obj-$(CONFIG_CXL_BASE) += cxl/ > obj-$(CONFIG_PANEL) += panel.o > +obj-$(CONFIG_ASPEED_BT_IPMI_HOST) += bt-host.o > > lkdtm-$(CONFIG_LKDTM) += lkdtm_core.o > lkdtm-$(CONFIG_LKDTM) += lkdtm_bugs.o Maybe put this in a subdirectory of drivers/char/ipmi? I understand that this is the other end of the protocol, but they are closely related after all. > +#define DEVICE_NAME "bt-host" here maybe "ipmi/bt-host" or "ipmi-bt-host"? > +static ssize_t bt_host_read(struct file *file, char __user *buf, > + size_t count, loff_t *ppos) > +{ > + struct bt_host *bt_host = file_bt_host(file); > + char __user *p = buf; > + u8 len; > + > + if (!access_ok(VERIFY_WRITE, buf, count)) > + return -EFAULT; > + > + WARN_ON(*ppos); > + > + if (wait_event_interruptible(bt_host->queue, > + bt_inb(bt_host, BT_CTRL) & BT_CTRL_H2B_ATN)) > + return -ERESTARTSYS; > + > + set_b_busy(bt_host); > + clr_h2b_atn(bt_host); > + clr_rd_ptr(bt_host); > + > + len = bt_read(bt_host); > + __put_user(len, p++); > + > + /* We pass the length back as well */ > + if (len + 1 > count) > + len = count - 1; > + > + while (len) { > + if (__put_user(bt_read(bt_host), p)) > + return -EFAULT; > + len--; p++; > + } If there are larger chunks of data to be transferred, using a temporary buffer with copy_from_user/copy_to_user would be more efficient. Since the size appears to be limited to 256 bytes anyway, that easily fits on the stack. > + > + clr_b_busy(bt_host); > + > + return p - buf; > +} What is the motivation for only allowing complete messages to be transferred or truncated for short buffers? Have you considered reading the message into a device specific buffer and allowing continued reads? I don't see an obvious reason one way or another, and I suppose you had an idea of what you were doing, so maybe explain it in a comment. > +static long bt_host_ioctl(struct file *file, unsigned int cmd, > + unsigned long param) > +{ > + struct bt_host *bt_host = file_bt_host(file); > + > + switch (cmd) { > + case BT_HOST_IOCTL_SMS_ATN: > + set_sms_atn(bt_host); > + return 0; > + } > + return -EINVAL; > +} Is this ioctl interface defined in a way that makes sense on any IPMI host hardware, or did you just do it like this because it is the easiest way on the hardware. I think it's important for the user interface to be extensible to other implementations if we ever add any. > +static int bt_host_config_irq(struct bt_host *bt_host, > + struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + uint32_t reg; > + int rc; > + > + bt_host->irq = irq_of_parse_and_map(dev->of_node, 0); > + if (!bt_host->irq) > + return -ENODEV; I think platform_get_irq() is the preferred interface here. Arnd
Hello, Adding Corey in cc: . I guess I should have done that in the first place. Arnd is suggesting to put this IPMI BT driver under drivers/char/ipmi which is indeed a better place than drivers/misc. Below some comments, On 08/31/2016 09:57 PM, Arnd Bergmann wrote: > On Wednesday, August 31, 2016 7:24:17 PM CEST Cédric Le Goater wrote: >> From: Alistair Popple <alistair@popple.id.au> >> >> This patch adds a simple device driver to expose the iBT interface on >> Aspeed chips as a character device (/dev/bt). >> >> The iBT interface is used to perform in-band IPMI communication from a >> BMC to the host. >> >> Signed-off-by: Alistair Popple <alistair@popple.id.au> >> Signed-off-by: Jeremy Kerr <jk@ozlabs.org> >> Signed-off-by: Joel Stanley <joel@jms.id.au> >> [clg: checkpatch fixes >> devicetree binding documentation] >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> --- >> .../devicetree/bindings/misc/aspeed,bt-host.txt | 19 + >> drivers/misc/Kconfig | 5 + >> drivers/misc/Makefile | 1 + >> drivers/misc/bt-host.c | 433 +++++++++++++++++++++ >> include/uapi/linux/Kbuild | 1 + >> include/uapi/linux/bt-host.h | 18 + >> 6 files changed, 477 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/misc/aspeed,bt-host.txt >> create mode 100644 drivers/misc/bt-host.c >> create mode 100644 include/uapi/linux/bt-host.h >> >> diff --git a/Documentation/devicetree/bindings/misc/aspeed,bt-host.txt b/Documentation/devicetree/bindings/misc/aspeed,bt-host.txt >> new file mode 100644 >> index 000000000000..938c5998c331 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/misc/aspeed,bt-host.txt > > "misc" seems like a bad category here. Does this fit nowhere else? > >> @@ -0,0 +1,19 @@ >> +* Aspeed BT IPMI interface > > What does "BT" stand for? IPMI is a more commonly known acronym, > but maybe list both with their full name as well. "Block Transfer" which is described in the IPMI specs. yes, I need to rephrase the commit log a bit and put some references to the specs. > >> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile >> index 7410c6d9a34d..71a7b9feb0f0 100644 >> --- a/drivers/misc/Makefile >> +++ b/drivers/misc/Makefile >> @@ -57,6 +57,7 @@ obj-$(CONFIG_ECHO) += echo/ >> obj-$(CONFIG_VEXPRESS_SYSCFG) += vexpress-syscfg.o >> obj-$(CONFIG_CXL_BASE) += cxl/ >> obj-$(CONFIG_PANEL) += panel.o >> +obj-$(CONFIG_ASPEED_BT_IPMI_HOST) += bt-host.o >> >> lkdtm-$(CONFIG_LKDTM) += lkdtm_core.o >> lkdtm-$(CONFIG_LKDTM) += lkdtm_bugs.o > > Maybe put this in a subdirectory of drivers/char/ipmi? > I understand that this is the other end of the protocol, > but they are closely related after all. I agree. There are also some definitions we could make common. Let's see what Corey thinks about it. >> +#define DEVICE_NAME "bt-host" > > here maybe "ipmi/bt-host" or "ipmi-bt-host"? yes. a name containing 'ipmi' is certainly wanted. >> +static ssize_t bt_host_read(struct file *file, char __user *buf, >> + size_t count, loff_t *ppos) >> +{ >> + struct bt_host *bt_host = file_bt_host(file); >> + char __user *p = buf; >> + u8 len; >> + >> + if (!access_ok(VERIFY_WRITE, buf, count)) >> + return -EFAULT; >> + >> + WARN_ON(*ppos); >> + >> + if (wait_event_interruptible(bt_host->queue, >> + bt_inb(bt_host, BT_CTRL) & BT_CTRL_H2B_ATN)) >> + return -ERESTARTSYS; >> + >> + set_b_busy(bt_host); >> + clr_h2b_atn(bt_host); >> + clr_rd_ptr(bt_host); >> + >> + len = bt_read(bt_host); >> + __put_user(len, p++); >> + >> + /* We pass the length back as well */ >> + if (len + 1 > count) >> + len = count - 1; >> + >> + while (len) { >> + if (__put_user(bt_read(bt_host), p)) >> + return -EFAULT; >> + len--; p++; >> + } > > If there are larger chunks of data to be transferred, > using a temporary buffer with copy_from_user/copy_to_user > would be more efficient. Since the size appears to > be limited to 256 bytes anyway, that easily fits on the stack. ok. I will change that. >> + >> + clr_b_busy(bt_host); >> + >> + return p - buf; >> +} > > What is the motivation for only allowing complete messages > to be transferred or truncated for short buffers? > > Have you considered reading the message into a device specific > buffer and allowing continued reads? > > I don't see an obvious reason one way or another, and I suppose > you had an idea of what you were doing, so maybe explain it > in a comment. The interface is not byte oriented. It is called 'Block Transfer' because an entire message is buffered and then the host or the bmc is notified that there is data to be read. I will add a comment on that. >> +static long bt_host_ioctl(struct file *file, unsigned int cmd, >> + unsigned long param) >> +{ >> + struct bt_host *bt_host = file_bt_host(file); >> + >> + switch (cmd) { >> + case BT_HOST_IOCTL_SMS_ATN: >> + set_sms_atn(bt_host); >> + return 0; >> + } >> + return -EINVAL; >> +} > > Is this ioctl interface defined in a way that makes sense on > any IPMI host hardware, or did you just do it like this because > it is the easiest way on the hardware. This platform runs OpenBMC on which a dbus daemon acts as a proxy between the IPMI BT char device and the rest of the system. So yes, the ioctl is relatively easy to use. > I think it's important for the user interface to be extensible > to other implementations if we ever add any. I agree but I don't know of any other BMC side implementations. May be others ? >> +static int bt_host_config_irq(struct bt_host *bt_host, >> + struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + uint32_t reg; >> + int rc; >> + >> + bt_host->irq = irq_of_parse_and_map(dev->of_node, 0); >> + if (!bt_host->irq) >> + return -ENODEV; > > I think platform_get_irq() is the preferred interface here. OK. Thanks, C.
On 09/02/2016 08:22 AM, Cédric Le Goater wrote: > Hello, > > Adding Corey in cc: . I guess I should have done that in the first place. Yes, probably so. I've been travelling and didn't see it on the mailing lists until now. There is already a BT driver in the kernel, in drivers/char/ipmi, why won't that work? -corey > > Arnd is suggesting to put this IPMI BT driver under drivers/char/ipmi > which is indeed a better place than drivers/misc. > > Below some comments, > > On 08/31/2016 09:57 PM, Arnd Bergmann wrote: >> On Wednesday, August 31, 2016 7:24:17 PM CEST Cédric Le Goater wrote: >>> From: Alistair Popple <alistair@popple.id.au> >>> >>> This patch adds a simple device driver to expose the iBT interface on >>> Aspeed chips as a character device (/dev/bt). >>> >>> The iBT interface is used to perform in-band IPMI communication from a >>> BMC to the host. >>> >>> Signed-off-by: Alistair Popple <alistair@popple.id.au> >>> Signed-off-by: Jeremy Kerr <jk@ozlabs.org> >>> Signed-off-by: Joel Stanley <joel@jms.id.au> >>> [clg: checkpatch fixes >>> devicetree binding documentation] >>> Signed-off-by: Cédric Le Goater <clg@kaod.org> >>> --- >>> .../devicetree/bindings/misc/aspeed,bt-host.txt | 19 + >>> drivers/misc/Kconfig | 5 + >>> drivers/misc/Makefile | 1 + >>> drivers/misc/bt-host.c | 433 +++++++++++++++++++++ >>> include/uapi/linux/Kbuild | 1 + >>> include/uapi/linux/bt-host.h | 18 + >>> 6 files changed, 477 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/misc/aspeed,bt-host.txt >>> create mode 100644 drivers/misc/bt-host.c >>> create mode 100644 include/uapi/linux/bt-host.h >>> >>> diff --git a/Documentation/devicetree/bindings/misc/aspeed,bt-host.txt b/Documentation/devicetree/bindings/misc/aspeed,bt-host.txt >>> new file mode 100644 >>> index 000000000000..938c5998c331 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/misc/aspeed,bt-host.txt >> "misc" seems like a bad category here. Does this fit nowhere else? >> >>> @@ -0,0 +1,19 @@ >>> +* Aspeed BT IPMI interface >> What does "BT" stand for? IPMI is a more commonly known acronym, >> but maybe list both with their full name as well. > "Block Transfer" which is described in the IPMI specs. > > yes, I need to rephrase the commit log a bit and put some references > to the specs. > >>> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile >>> index 7410c6d9a34d..71a7b9feb0f0 100644 >>> --- a/drivers/misc/Makefile >>> +++ b/drivers/misc/Makefile >>> @@ -57,6 +57,7 @@ obj-$(CONFIG_ECHO) += echo/ >>> obj-$(CONFIG_VEXPRESS_SYSCFG) += vexpress-syscfg.o >>> obj-$(CONFIG_CXL_BASE) += cxl/ >>> obj-$(CONFIG_PANEL) += panel.o >>> +obj-$(CONFIG_ASPEED_BT_IPMI_HOST) += bt-host.o >>> >>> lkdtm-$(CONFIG_LKDTM) += lkdtm_core.o >>> lkdtm-$(CONFIG_LKDTM) += lkdtm_bugs.o >> Maybe put this in a subdirectory of drivers/char/ipmi? >> I understand that this is the other end of the protocol, >> but they are closely related after all. > I agree. There are also some definitions we could make common. > > Let's see what Corey thinks about it. > >>> +#define DEVICE_NAME "bt-host" >> here maybe "ipmi/bt-host" or "ipmi-bt-host"? > yes. a name containing 'ipmi' is certainly wanted. > >>> +static ssize_t bt_host_read(struct file *file, char __user *buf, >>> + size_t count, loff_t *ppos) >>> +{ >>> + struct bt_host *bt_host = file_bt_host(file); >>> + char __user *p = buf; >>> + u8 len; >>> + >>> + if (!access_ok(VERIFY_WRITE, buf, count)) >>> + return -EFAULT; >>> + >>> + WARN_ON(*ppos); >>> + >>> + if (wait_event_interruptible(bt_host->queue, >>> + bt_inb(bt_host, BT_CTRL) & BT_CTRL_H2B_ATN)) >>> + return -ERESTARTSYS; >>> + >>> + set_b_busy(bt_host); >>> + clr_h2b_atn(bt_host); >>> + clr_rd_ptr(bt_host); >>> + >>> + len = bt_read(bt_host); >>> + __put_user(len, p++); >>> + >>> + /* We pass the length back as well */ >>> + if (len + 1 > count) >>> + len = count - 1; >>> + >>> + while (len) { >>> + if (__put_user(bt_read(bt_host), p)) >>> + return -EFAULT; >>> + len--; p++; >>> + } >> If there are larger chunks of data to be transferred, >> using a temporary buffer with copy_from_user/copy_to_user >> would be more efficient. Since the size appears to >> be limited to 256 bytes anyway, that easily fits on the stack. > ok. I will change that. > >>> + >>> + clr_b_busy(bt_host); >>> + >>> + return p - buf; >>> +} >> What is the motivation for only allowing complete messages >> to be transferred or truncated for short buffers? >> >> Have you considered reading the message into a device specific >> buffer and allowing continued reads? >> >> I don't see an obvious reason one way or another, and I suppose >> you had an idea of what you were doing, so maybe explain it >> in a comment. > The interface is not byte oriented. It is called 'Block Transfer' > because an entire message is buffered and then the host or the bmc > is notified that there is data to be read. > > I will add a comment on that. > >>> +static long bt_host_ioctl(struct file *file, unsigned int cmd, >>> + unsigned long param) >>> +{ >>> + struct bt_host *bt_host = file_bt_host(file); >>> + >>> + switch (cmd) { >>> + case BT_HOST_IOCTL_SMS_ATN: >>> + set_sms_atn(bt_host); >>> + return 0; >>> + } >>> + return -EINVAL; >>> +} >> Is this ioctl interface defined in a way that makes sense on >> any IPMI host hardware, or did you just do it like this because >> it is the easiest way on the hardware. > This platform runs OpenBMC on which a dbus daemon acts as a proxy > between the IPMI BT char device and the rest of the system. So yes, > the ioctl is relatively easy to use. > >> I think it's important for the user interface to be extensible >> to other implementations if we ever add any. > I agree but I don't know of any other BMC side implementations. > May be others ? > >>> +static int bt_host_config_irq(struct bt_host *bt_host, >>> + struct platform_device *pdev) >>> +{ >>> + struct device *dev = &pdev->dev; >>> + uint32_t reg; >>> + int rc; >>> + >>> + bt_host->irq = irq_of_parse_and_map(dev->of_node, 0); >>> + if (!bt_host->irq) >>> + return -ENODEV; >> I think platform_get_irq() is the preferred interface here. > OK. > > Thanks, > > C. > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Monday, September 12, 2016 1:55:40 PM CEST Corey Minyard wrote: > On 09/02/2016 08:22 AM, Cédric Le Goater wrote: > > Hello, > > > > Adding Corey in cc: . I guess I should have done that in the first place. > > Yes, probably so. I've been travelling and didn't see it on the mailing > lists until now. > > There is already a BT driver in the kernel, in drivers/char/ipmi, why > won't that work? The new driver is the host side (running on the BMC), the existing one is the client (running on the PC). Arnd
On 09/12/2016 02:15 PM, Arnd Bergmann wrote: > On Monday, September 12, 2016 1:55:40 PM CEST Corey Minyard wrote: >> On 09/02/2016 08:22 AM, Cédric Le Goater wrote: >>> Hello, >>> >>> Adding Corey in cc: . I guess I should have done that in the first place. >> Yes, probably so. I've been travelling and didn't see it on the mailing >> lists until now. >> >> There is already a BT driver in the kernel, in drivers/char/ipmi, why >> won't that work? > The new driver is the host side (running on the BMC), the existing one > is the client (running on the PC). > > Arnd Ok, that's not really clear from the documentation or the Kconfig. In the IPMI spec the "host" side is the computer side, not the BMC side. Like: 11.6.1 BT Host Interface Registers The Host BT interface provides an independent set of registers and interrupts to allow the Host driver to communicate with the baseboard management controller without conflicting with the O/S ACPI driver. In light of that, this should probably be named the bt-bmc driver. I haven't reviewed this in detail, but I'm ok with putting it in drivers/char/ipmi. The state machine part looks reasonably generic. The configuration part isn't, but that could be split out later if necessary. The biggest thing I don't like is the byte at a time interfaces from userspace. That seems fairly inefficient if the system does extra work for each userspace access. IIRC some systems do and some don't. -corey > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hello, On 09/12/2016 10:33 PM, Corey Minyard wrote: > On 09/12/2016 02:15 PM, Arnd Bergmann wrote: >> On Monday, September 12, 2016 1:55:40 PM CEST Corey Minyard wrote: >>> On 09/02/2016 08:22 AM, Cédric Le Goater wrote: >>>> Hello, >>>> >>>> Adding Corey in cc: . I guess I should have done that in the first place. >>> Yes, probably so. I've been travelling and didn't see it on the mailing >>> lists until now. >>> >>> There is already a BT driver in the kernel, in drivers/char/ipmi, why >>> won't that work? >> The new driver is the host side (running on the BMC), the existing one >> is the client (running on the PC). >> >> Arnd > > Ok, that's not really clear from the documentation or the Kconfig. > In the IPMI spec the "host" side is the computer side, not the BMC > side. Like: > > 11.6.1 BT Host Interface Registers > The Host BT interface provides an independent set of registers and > interrupts to allow the Host driver to > communicate with the baseboard management controller without > conflicting with the O/S ACPI driver. > > In light of that, this should probably be named the bt-bmc driver. > > I haven't reviewed this in detail, but I'm ok with putting it in > drivers/char/ipmi. The state machine part looks reasonably > generic. The configuration part isn't, but that could be split > out later if necessary. > > The biggest thing I don't like is the byte at a time interfaces > from userspace. That seems fairly inefficient if the system > does extra work for each userspace access. IIRC some > systems do and some don't. What about the ioctl to send an SMS ATN event to the host ? Is that ok for you ? Thanks, C.
On 09/12/2016 04:23 PM, Cédric Le Goater wrote: > Hello, > > On 09/12/2016 10:33 PM, Corey Minyard wrote: >> On 09/12/2016 02:15 PM, Arnd Bergmann wrote: >>> On Monday, September 12, 2016 1:55:40 PM CEST Corey Minyard wrote: >>>> On 09/02/2016 08:22 AM, Cédric Le Goater wrote: >>>>> Hello, >>>>> >>>>> Adding Corey in cc: . I guess I should have done that in the first place. >>>> Yes, probably so. I've been travelling and didn't see it on the mailing >>>> lists until now. >>>> >>>> There is already a BT driver in the kernel, in drivers/char/ipmi, why >>>> won't that work? >>> The new driver is the host side (running on the BMC), the existing one >>> is the client (running on the PC). >>> >>> Arnd >> Ok, that's not really clear from the documentation or the Kconfig. >> In the IPMI spec the "host" side is the computer side, not the BMC >> side. Like: >> >> 11.6.1 BT Host Interface Registers >> The Host BT interface provides an independent set of registers and >> interrupts to allow the Host driver to >> communicate with the baseboard management controller without >> conflicting with the O/S ACPI driver. >> >> In light of that, this should probably be named the bt-bmc driver. >> >> I haven't reviewed this in detail, but I'm ok with putting it in >> drivers/char/ipmi. The state machine part looks reasonably >> generic. The configuration part isn't, but that could be split >> out later if necessary. >> >> The biggest thing I don't like is the byte at a time interfaces >> from userspace. That seems fairly inefficient if the system >> does extra work for each userspace access. IIRC some >> systems do and some don't. > What about the ioctl to send an SMS ATN event to the host ? Is > that ok for you ? Yeah, that's necessary, there's no way the low-level driver can know when to do that. I have a similar piece of code in the qemu IPMI emulator, even the external IPMI emulator has a message it sends to set ATTN. I did look at similarities between the two pieces of code, but the qemu one has to be register-read/write driven, so it's completely different in design. -corey > Thanks, > > C.
On 09/12/2016 10:33 PM, Corey Minyard wrote: > On 09/12/2016 02:15 PM, Arnd Bergmann wrote: >> On Monday, September 12, 2016 1:55:40 PM CEST Corey Minyard wrote: >>> On 09/02/2016 08:22 AM, Cédric Le Goater wrote: >>>> Hello, >>>> >>>> Adding Corey in cc: . I guess I should have done that in the first place. >>> Yes, probably so. I've been travelling and didn't see it on the mailing >>> lists until now. >>> >>> There is already a BT driver in the kernel, in drivers/char/ipmi, why >>> won't that work? >> The new driver is the host side (running on the BMC), the existing one >> is the client (running on the PC). >> >> Arnd > > Ok, that's not really clear from the documentation or the Kconfig. > In the IPMI spec the "host" side is the computer side, not the BMC > side. Like: > > 11.6.1 BT Host Interface Registers > The Host BT interface provides an independent set of registers and > interrupts to allow the Host driver to > communicate with the baseboard management controller without > conflicting with the O/S ACPI driver. > > In light of that, this should probably be named the bt-bmc driver. > > I haven't reviewed this in detail, but I'm ok with putting it in > drivers/char/ipmi. The state machine part looks reasonably > generic. The configuration part isn't, but that could be split > out later if necessary. what do you mean by configuration ? I am ready to send a v2. May be I can add a few other things. Thanks, C.
On 09/15/2016 01:51 AM, Cédric Le Goater wrote: > On 09/12/2016 10:33 PM, Corey Minyard wrote: >> On 09/12/2016 02:15 PM, Arnd Bergmann wrote: >>> On Monday, September 12, 2016 1:55:40 PM CEST Corey Minyard wrote: >>>> On 09/02/2016 08:22 AM, Cédric Le Goater wrote: >>>>> Hello, >>>>> >>>>> Adding Corey in cc: . I guess I should have done that in the first place. >>>> Yes, probably so. I've been travelling and didn't see it on the mailing >>>> lists until now. >>>> >>>> There is already a BT driver in the kernel, in drivers/char/ipmi, why >>>> won't that work? >>> The new driver is the host side (running on the BMC), the existing one >>> is the client (running on the PC). >>> >>> Arnd >> Ok, that's not really clear from the documentation or the Kconfig. >> In the IPMI spec the "host" side is the computer side, not the BMC >> side. Like: >> >> 11.6.1 BT Host Interface Registers >> The Host BT interface provides an independent set of registers and >> interrupts to allow the Host driver to >> communicate with the baseboard management controller without >> conflicting with the O/S ACPI driver. >> >> In light of that, this should probably be named the bt-bmc driver. >> >> I haven't reviewed this in detail, but I'm ok with putting it in >> drivers/char/ipmi. The state machine part looks reasonably >> generic. The configuration part isn't, but that could be split >> out later if necessary. > what do you mean by configuration ? I am ready to send a v2. May be > I can add a few other things. The part that sets everything up, basically everything from bt_host_fops down. If you were to use this on another system, that code would have to be made more generic and the machine-specific part split into different files, but that's not necessary now, I don't think. -corey > Thanks, > > C.
diff --git a/Documentation/devicetree/bindings/misc/aspeed,bt-host.txt b/Documentation/devicetree/bindings/misc/aspeed,bt-host.txt new file mode 100644 index 000000000000..938c5998c331 --- /dev/null +++ b/Documentation/devicetree/bindings/misc/aspeed,bt-host.txt @@ -0,0 +1,19 @@ +* Aspeed BT IPMI interface + +Required properties: + +- compatible : should be "aspeed,bt-host" +- reg: physical address and size of the registers + +Optional properties: + +- interrupts: interrupt generated by the BT interface. without an + interrupt, the driver will operate in poll mode. + +Example: + + ibt@1e789140 { + compatible = "aspeed,bt-host"; + reg = <0x1e789140 0x18>; + interrupts = <8>; + }; diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index a216b4667742..47f761861120 100644 --- a/drivers/misc/Kconfig +++ b/drivers/misc/Kconfig @@ -804,6 +804,11 @@ 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_BT_IPMI_HOST + tristate "BT IPMI host driver" + help + Support for the Aspeed BT IPMI host. + 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 7410c6d9a34d..71a7b9feb0f0 100644 --- a/drivers/misc/Makefile +++ b/drivers/misc/Makefile @@ -57,6 +57,7 @@ obj-$(CONFIG_ECHO) += echo/ obj-$(CONFIG_VEXPRESS_SYSCFG) += vexpress-syscfg.o obj-$(CONFIG_CXL_BASE) += cxl/ obj-$(CONFIG_PANEL) += panel.o +obj-$(CONFIG_ASPEED_BT_IPMI_HOST) += bt-host.o lkdtm-$(CONFIG_LKDTM) += lkdtm_core.o lkdtm-$(CONFIG_LKDTM) += lkdtm_bugs.o diff --git a/drivers/misc/bt-host.c b/drivers/misc/bt-host.c new file mode 100644 index 000000000000..3d9bfbaa8940 --- /dev/null +++ b/drivers/misc/bt-host.c @@ -0,0 +1,433 @@ +/* + * Copyright (c) 2015-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/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_irq.h> +#include <linux/platform_device.h> +#include <linux/io.h> +#include <linux/interrupt.h> +#include <linux/delay.h> +#include <linux/miscdevice.h> +#include <linux/timer.h> +#include <linux/jiffies.h> +#include <linux/bt-host.h> + +#define DEVICE_NAME "bt-host" + +#define BT_IO_BASE 0xe4 +#define BT_IRQ 10 + +#define BT_CR0 0x0 +#define BT_CR0_IO_BASE 16 +#define BT_CR0_IRQ 12 +#define BT_CR0_EN_CLR_SLV_RDP 0x8 +#define BT_CR0_EN_CLR_SLV_WRP 0x4 +#define BT_CR0_ENABLE_IBT 0x1 +#define BT_CR1 0x4 +#define BT_CR1_IRQ_H2B 0x01 +#define BT_CR1_IRQ_HBUSY 0x40 +#define BT_CR2 0x8 +#define BT_CR2_IRQ_H2B 0x01 +#define BT_CR2_IRQ_HBUSY 0x40 +#define BT_CR3 0xc +#define BT_CTRL 0x10 +#define BT_CTRL_B_BUSY 0x80 +#define BT_CTRL_H_BUSY 0x40 +#define BT_CTRL_OEM0 0x20 +#define BT_CTRL_SMS_ATN 0x10 +#define BT_CTRL_B2H_ATN 0x08 +#define BT_CTRL_H2B_ATN 0x04 +#define BT_CTRL_CLR_RD_PTR 0x02 +#define BT_CTRL_CLR_WR_PTR 0x01 +#define BT_BMC2HOST 0x14 +#define BT_INTMASK 0x18 +#define BT_INTMASK_B2H_IRQEN 0x01 +#define BT_INTMASK_B2H_IRQ 0x02 +#define BT_INTMASK_BMC_HWRST 0x80 + +struct bt_host { + struct device dev; + struct miscdevice miscdev; + void __iomem *base; + int open_count; + int irq; + wait_queue_head_t queue; + struct timer_list poll_timer; +}; + +static u8 bt_inb(struct bt_host *bt_host, int reg) +{ + return ioread8(bt_host->base + reg); +} + +static void bt_outb(struct bt_host *bt_host, u8 data, int reg) +{ + iowrite8(data, bt_host->base + reg); +} + +static void clr_rd_ptr(struct bt_host *bt_host) +{ + bt_outb(bt_host, BT_CTRL_CLR_RD_PTR, BT_CTRL); +} + +static void clr_wr_ptr(struct bt_host *bt_host) +{ + bt_outb(bt_host, BT_CTRL_CLR_WR_PTR, BT_CTRL); +} + +static void clr_h2b_atn(struct bt_host *bt_host) +{ + bt_outb(bt_host, BT_CTRL_H2B_ATN, BT_CTRL); +} + +static void set_b_busy(struct bt_host *bt_host) +{ + if (!(bt_inb(bt_host, BT_CTRL) & BT_CTRL_B_BUSY)) + bt_outb(bt_host, BT_CTRL_B_BUSY, BT_CTRL); +} + +static void clr_b_busy(struct bt_host *bt_host) +{ + if (bt_inb(bt_host, BT_CTRL) & BT_CTRL_B_BUSY) + bt_outb(bt_host, BT_CTRL_B_BUSY, BT_CTRL); +} + +static void set_b2h_atn(struct bt_host *bt_host) +{ + bt_outb(bt_host, BT_CTRL_B2H_ATN, BT_CTRL); +} + +static u8 bt_read(struct bt_host *bt_host) +{ + return bt_inb(bt_host, BT_BMC2HOST); +} + +static void bt_write(struct bt_host *bt_host, u8 c) +{ + bt_outb(bt_host, c, BT_BMC2HOST); +} + +static void set_sms_atn(struct bt_host *bt_host) +{ + bt_outb(bt_host, BT_CTRL_SMS_ATN, BT_CTRL); +} + +static struct bt_host *file_bt_host(struct file *file) +{ + return container_of(file->private_data, struct bt_host, miscdev); +} + +static int bt_host_open(struct inode *inode, struct file *file) +{ + struct bt_host *bt_host = file_bt_host(file); + + clr_b_busy(bt_host); + + return 0; +} + +static ssize_t bt_host_read(struct file *file, char __user *buf, + size_t count, loff_t *ppos) +{ + struct bt_host *bt_host = file_bt_host(file); + char __user *p = buf; + u8 len; + + if (!access_ok(VERIFY_WRITE, buf, count)) + return -EFAULT; + + WARN_ON(*ppos); + + if (wait_event_interruptible(bt_host->queue, + bt_inb(bt_host, BT_CTRL) & BT_CTRL_H2B_ATN)) + return -ERESTARTSYS; + + set_b_busy(bt_host); + clr_h2b_atn(bt_host); + clr_rd_ptr(bt_host); + + len = bt_read(bt_host); + __put_user(len, p++); + + /* We pass the length back as well */ + if (len + 1 > count) + len = count - 1; + + while (len) { + if (__put_user(bt_read(bt_host), p)) + return -EFAULT; + len--; p++; + } + + clr_b_busy(bt_host); + + return p - buf; +} + +static ssize_t bt_host_write(struct file *file, const char __user *buf, + size_t count, loff_t *ppos) +{ + struct bt_host *bt_host = file_bt_host(file); + const char __user *p = buf; + u8 c; + + if (!access_ok(VERIFY_READ, buf, count)) + return -EFAULT; + + WARN_ON(*ppos); + + /* There's no interrupt for clearing host busy so we have to + * poll + */ + if (wait_event_interruptible(bt_host->queue, + !(bt_inb(bt_host, BT_CTRL) & + (BT_CTRL_H_BUSY | BT_CTRL_B2H_ATN)))) + return -ERESTARTSYS; + + clr_wr_ptr(bt_host); + + while (count) { + if (__get_user(c, p)) + return -EFAULT; + + bt_write(bt_host, c); + count--; p++; + } + + set_b2h_atn(bt_host); + + return p - buf; +} + +static long bt_host_ioctl(struct file *file, unsigned int cmd, + unsigned long param) +{ + struct bt_host *bt_host = file_bt_host(file); + + switch (cmd) { + case BT_HOST_IOCTL_SMS_ATN: + set_sms_atn(bt_host); + return 0; + } + return -EINVAL; +} + +static int bt_host_release(struct inode *inode, struct file *file) +{ + struct bt_host *bt_host = file_bt_host(file); + + set_b_busy(bt_host); + return 0; +} + +static unsigned int bt_host_poll(struct file *file, poll_table *wait) +{ + struct bt_host *bt_host = file_bt_host(file); + unsigned int mask = 0; + uint8_t ctrl; + + poll_wait(file, &bt_host->queue, wait); + + ctrl = bt_inb(bt_host, BT_CTRL); + + if (ctrl & BT_CTRL_H2B_ATN) + mask |= POLLIN; + + if (!(ctrl & (BT_CTRL_H_BUSY | BT_CTRL_B2H_ATN))) + mask |= POLLOUT; + + return mask; +} + +static const struct file_operations bt_host_fops = { + .owner = THIS_MODULE, + .open = bt_host_open, + .read = bt_host_read, + .write = bt_host_write, + .release = bt_host_release, + .poll = bt_host_poll, + .unlocked_ioctl = bt_host_ioctl, +}; + +static void poll_timer(unsigned long data) +{ + struct bt_host *bt_host = (void *)data; + + bt_host->poll_timer.expires += msecs_to_jiffies(500); + wake_up(&bt_host->queue); + add_timer(&bt_host->poll_timer); +} + +static irqreturn_t bt_host_irq(int irq, void *arg) +{ + struct bt_host *bt_host = arg; + uint32_t reg; + + reg = ioread32(bt_host->base + BT_CR2); + reg &= BT_CR2_IRQ_H2B | BT_CR2_IRQ_HBUSY; + if (!reg) + return IRQ_NONE; + + /* ack pending IRQs */ + iowrite32(reg, bt_host->base + BT_CR2); + + wake_up(&bt_host->queue); + return IRQ_HANDLED; +} + +static int bt_host_config_irq(struct bt_host *bt_host, + struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + uint32_t reg; + int rc; + + bt_host->irq = irq_of_parse_and_map(dev->of_node, 0); + if (!bt_host->irq) + return -ENODEV; + + rc = devm_request_irq(dev, bt_host->irq, bt_host_irq, IRQF_SHARED, + DEVICE_NAME, bt_host); + if (rc < 0) { + dev_warn(dev, "Unable to request IRQ %d\n", bt_host->irq); + bt_host->irq = 0; + return rc; + } + + /* Configure IRQs on the host clearing the H2B and HBUSY bits; + * H2B will be asserted when the host has data for us; HBUSY + * will be cleared (along with B2H) when we can write the next + * message to the BT buffer + */ + reg = ioread32(bt_host->base + BT_CR1); + reg |= BT_CR1_IRQ_H2B | BT_CR1_IRQ_HBUSY; + iowrite32(reg, bt_host->base + BT_CR1); + + return 0; +} + +static int bt_host_probe(struct platform_device *pdev) +{ + struct bt_host *bt_host; + struct device *dev; + struct resource *res; + int rc; + + if (!pdev || !pdev->dev.of_node) + return -ENODEV; + + dev = &pdev->dev; + dev_info(dev, "Found bt host device\n"); + + bt_host = devm_kzalloc(dev, sizeof(*bt_host), GFP_KERNEL); + if (!bt_host) + return -ENOMEM; + + dev_set_drvdata(&pdev->dev, bt_host); + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!res) { + dev_err(dev, "Unable to find resources\n"); + rc = -ENXIO; + goto out_free; + } + + bt_host->base = devm_ioremap_resource(&pdev->dev, res); + if (!bt_host->base) { + rc = -ENOMEM; + goto out_free; + } + + init_waitqueue_head(&bt_host->queue); + + bt_host->miscdev.minor = MISC_DYNAMIC_MINOR, + bt_host->miscdev.name = DEVICE_NAME, + bt_host->miscdev.fops = &bt_host_fops, + bt_host->miscdev.parent = dev; + rc = misc_register(&bt_host->miscdev); + if (rc) { + dev_err(dev, "Unable to register device\n"); + goto out_unmap; + } + + bt_host_config_irq(bt_host, pdev); + + if (bt_host->irq) { + dev_info(dev, "Using IRQ %d\n", bt_host->irq); + } else { + dev_info(dev, "No IRQ; using timer\n"); + setup_timer(&bt_host->poll_timer, poll_timer, + (unsigned long)bt_host); + bt_host->poll_timer.expires = jiffies + msecs_to_jiffies(10); + add_timer(&bt_host->poll_timer); + } + + iowrite32((BT_IO_BASE << BT_CR0_IO_BASE) | + (BT_IRQ << BT_CR0_IRQ) | + BT_CR0_EN_CLR_SLV_RDP | + BT_CR0_EN_CLR_SLV_WRP | + BT_CR0_ENABLE_IBT, + bt_host->base + BT_CR0); + + clr_b_busy(bt_host); + + return 0; + +out_unmap: + devm_iounmap(&pdev->dev, bt_host->base); + +out_free: + devm_kfree(dev, bt_host); + return rc; + +} + +static int bt_host_remove(struct platform_device *pdev) +{ + struct bt_host *bt_host = dev_get_drvdata(&pdev->dev); + + misc_deregister(&bt_host->miscdev); + if (!bt_host->irq) + del_timer_sync(&bt_host->poll_timer); + devm_iounmap(&pdev->dev, bt_host->base); + devm_kfree(&pdev->dev, bt_host); + bt_host = NULL; + + return 0; +} + +static const struct of_device_id bt_host_match[] = { + { .compatible = "aspeed,bt-host" }, + { }, +}; + +static struct platform_driver bt_host_driver = { + .driver = { + .name = DEVICE_NAME, + .of_match_table = bt_host_match, + }, + .probe = bt_host_probe, + .remove = bt_host_remove, +}; + +module_platform_driver(bt_host_driver); + +MODULE_DEVICE_TABLE(of, bt_host_match); +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Alistair Popple <alistair@popple.id.au>"); +MODULE_DESCRIPTION("Linux device interface to the BT interface"); diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild index 185f8ea2702f..c763a35c2147 100644 --- a/include/uapi/linux/Kbuild +++ b/include/uapi/linux/Kbuild @@ -74,6 +74,7 @@ header-y += bpf_common.h header-y += bpf.h header-y += bpqether.h header-y += bsg.h +header-y += bt-host.h header-y += btrfs.h header-y += can.h header-y += capability.h diff --git a/include/uapi/linux/bt-host.h b/include/uapi/linux/bt-host.h new file mode 100644 index 000000000000..e2aedddf0bf4 --- /dev/null +++ b/include/uapi/linux/bt-host.h @@ -0,0 +1,18 @@ +/* + * Copyright (c) 2015-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. + */ + +#ifndef _UAPI_LINUX_BT_HOST_H +#define _UAPI_LINUX_BT_HOST_H + +#include <linux/ioctl.h> + +#define __BT_HOST_IOCTL_MAGIC 0xb1 +#define BT_HOST_IOCTL_SMS_ATN _IO(__BT_HOST_IOCTL_MAGIC, 0x00) + +#endif /* _UAPI_LINUX_BT_HOST_H */