[PATCH linux 2/3] drivers/misc: Add aspeed mbox driver
Cyril Bur
cyrilbur at gmail.com
Thu Dec 15 18:59:27 AEDT 2016
On Tue, 2016-12-13 at 12:26 +1100, Joel Stanley wrote:
> On Fri, Dec 9, 2016 at 4:43 PM, Cyril Bur <cyrilbur at gmail.com> wrote:
> > Signed-off-by: Cyril Bur <cyrilbur at gmail.com>
>
> Explain why we need this chardev, why it needs ioctls, etc.
>
Yeah, we should probably also work on the name and stuff
> > ---
> > drivers/misc/Kconfig | 7 +
> > drivers/misc/Makefile | 2 +
> > drivers/misc/mbox-host.c | 326
> > +++++++++++++++++++++++++++++++++++++++++
> > include/uapi/linux/mbox-host.h | 18 +++
> > 4 files changed, 353 insertions(+)
> > create mode 100644 drivers/misc/mbox-host.c
> > create mode 100644 include/uapi/linux/mbox-host.h
> >
> > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> > index a216b46..44f191d 100644
> > --- a/drivers/misc/Kconfig
> > +++ b/drivers/misc/Kconfig
> > @@ -804,6 +804,13 @@ 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 MBOX_HOST
> > + depends on ARCH_ASPEED
>
> or COMPILE_TEST
Is both a good solution?
>
> > + bool "Build MBOX driver for BMC to HOST communication"
> > + default "n"
>
> If it's depending on ARCH_ASPEED I think default y is ok.
>
Yes
> > + ---help---
> > + Build the MBOX driver
>
> More text here please.
>
> > +
> > 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 b2fb6dbf..7bcaf30 100644
> > --- a/drivers/misc/Makefile
> > +++ b/drivers/misc/Makefile
>
> "misc seems a bit too misc" - Andrew Jeffery.
>
> How about drivers/soc? Or drivers/mfd?
I think we settled on mailbox?
>
> > +#define DEVICE_NAME "mbox-host"
> > +
> > +#define MBOX_NUM_REGS 16
> > +#define MBOX_NUM_DATA_REGS 14
> > +
> > +#define MBOX_DATA_0 0x00
> > +#define MBOX_STATUS_0 0x40
> > +#define MBOX_STATUS_1 0x44
> > +#define MBOX_BMC_CTRL 0x48
> > + #define MBOX_CTRL_RECV 0x80
> > + #define MBOX_CTRL_MASK 0x02
> > + #define MBOX_CTRL_SEND 0x01
>
> If you want to do show these are bits in the CTRL register, do this.
> I
> think it's more readable to represent the bits as shifts, or even
> better, using the BIT() macro.
>
> #define MBOX_BMC_CTRL 0x48
> #define MBOX_CTRL_RECV BIT(7)
> #define MBOX_CTRL_MASK BIT(1)
>
> etc.
>
Ah yes looks much better now, thanks
> > +#define MBOX_HOST_CTRL 0x4c
> > +#define MBOX_INTERRUPT_0 0x50
> > +#define MBOX_INTERRUPT_1 0x54
> > +
> > +
> > +static u8 mbox_inb(struct mbox_host *mbox_host, int reg)
> > +{
> > + return ioread8(mbox_host->base + reg);
> > +}
> > +
> > +static void mbox_outb(struct mbox_host *mbox_host, u8 data, int
> > reg)
> > +{
> > + iowrite8(data, mbox_host->base + reg);
> > +}
>
> As mentioned in the other email, all access to the LPC address space
> is through regmap.
Yes.
>
> > +
> > +static struct mbox_host *file_mbox_host(struct file *file)
> > +{
> > + return container_of(file->private_data, struct mbox_host,
> > miscdev);
> > +}
> > +
> > +static int mbox_host_open(struct inode *inode, struct file *file)
> > +{
> > + return 0;
> > +}
>
> This means userspace can open this device an unlimited number of
> times. I think we only want one user at a time?
>
That would be wise yes
>
> > +static int mbox_host_release(struct inode *inode, struct file
> > *file)
> > +{
> > + return 0;
> > +}
>
> Why not omit the callback?
>
> > +
> > +static unsigned int mbox_host_poll(struct file *file, poll_table
> > *wait)
> > +{
> > + struct mbox_host *mbox_host = file_mbox_host(file);
> > + unsigned int mask = 0;
> > +
> > + poll_wait(file, &mbox_host->queue, wait);
> > +
> > + if (mbox_inb(mbox_host, MBOX_BMC_CTRL) & MBOX_CTRL_RECV)
> > + mask |= POLLIN;
> > +
> > + return mask;
> > +}
> > +
> > +static const struct file_operations mbox_host_fops = {
> > + .owner = THIS_MODULE,
> > + .open = mbox_host_open,
> > + .read = mbox_host_read,
> > + .write = mbox_host_write,
> > + .release = mbox_host_release,
> > + .poll = mbox_host_poll,
> > + .unlocked_ioctl = mbox_host_ioctl,
> > +};
> > +
> > +static void poll_timer(unsigned long data)
> > +{
> > + struct mbox_host *mbox_host = (void *)data;
> > +
> > + mbox_host->poll_timer.expires += msecs_to_jiffies(500);
>
> Why 500?
Why not? I feel like I should test and justify a number, I honestly
have no idea whats good but 500msecs feels decent... perhaps I'll
determine a better number, or a good reason for 500 in my infinite
spare time ;)
Thanks for the review,
Cyril
>
> > + wake_up(&mbox_host->queue);
> > + add_timer(&mbox_host->poll_timer);
> > +}
> > +
More information about the openbmc
mailing list