<div dir="ltr"><div dir="ltr"><p class="MsoNormal" style="margin:0cm 0cm 0.0001pt"><font face="arial, sans-serif">Hi William,</font></p>

<p class="MsoNormal" style="margin:0cm 0cm 0.0001pt"><font face="arial, sans-serif"> </font></p>

<p class="MsoNormal" style="margin:0cm 0cm 0.0001pt"><font face="arial, sans-serif">Thanks a lot for working on this.</font></p>

<p class="MsoNormal" style="margin:0cm 0cm 0.0001pt"><font face="arial, sans-serif">Thanks for raising issues and sent
great solutions for it. Appreciate it!</font></p><p class="MsoNormal" style="margin:0cm 0cm 0.0001pt"><font face="arial, sans-serif"><br></font></p>

<p class="MsoNormal" style="margin:0cm 0cm 0.0001pt"><font face="arial, sans-serif">I</font><span style="font-family:arial,sans-serif">n general, we prefer to modify the
existing driver and not to do a rework. (I think it can be done in this case), please let us know what do you think.</span></p><p class="MsoNormal" style="margin:0cm 0cm 0.0001pt"><span style="font-family:arial,sans-serif"><br></span></p><p class="MsoNormal" style="margin:0cm 0cm 0.0001pt"></p></div><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, 14 Dec 2019 at 01:19, William A. Kennington III <<a href="mailto:wak@google.com">wak@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">This provides a number of fixes:<br>
 - Multiple file handles are now supported, previously using multiple<br>
   readers would cause a race in the kfifo and spew garbage to the<br>
   readers.<br></blockquote><div><p class="MsoNormal" style="margin:0cm 0cm 0.0001pt"><font face="arial, sans-serif" style="">Indeed, there is not a support for a
multiple readers; I prefer to have only a single reader per channel because</font></p>

<p class="MsoNormal" style="margin:0cm 0cm 0.0001pt"><font face="arial, sans-serif">I don't see too much use from the
OpenBMC/other application multiple readers, so the RCU use (that is coded
great in your rework) is not much needed and cause a little over head.</font></p>

<p class="MsoNormal" style="margin:0cm 0cm 0.0001pt"><font face="arial, sans-serif">Do you need a multiple readers in
the OpenBMC user space?</font></p></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
 - iowrite{8,16,32} are now supported correctly. Previously, the<br>
   dwcapture code would get confused by values added to the fifo for 16<br>
   bit writes<br></blockquote><div><p class="MsoNormal" style="margin:0cm 0cm 0.0001pt"><font face="arial, sans-serif" style="">dwcapture need to be used only with
4 byte writes, of course the host can still write in  {8,16,32}  but
it can cause a some issues.</font></p>

<p class="MsoNormal" style="margin:0cm 0cm 0.0001pt"><font face="arial, sans-serif">for example writing only 16 bit from
the higher address, and not getting bit 8 at byte 0 for separating between the
words.</font></p>

<p class="MsoNormal" style="margin:0cm 0cm 0.0001pt"><font face="arial, sans-serif">I believe the issue is when writing
lower 16 bits from the host</font>(am I correct?)<span style="font-family:arial,sans-serif">, we think it can solved with
padding the same as one 8 bits write.</span></p></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> - Reads from the device now only return a single post code. Previously<br>
   the read call would emit all of the post code data in a single<br>
   syscall. This was broken because it wouldn't account for partial post<br>
   code writes into the fifo, meaning the reader could get partial<br>
   4-byte codes for dwcap.<br></blockquote><div><p class="MsoNormal" style="margin:0cm 0cm 0.0001pt"><font face="arial, sans-serif" style="">But is seems that if the user not
using dwcap each read will be 1 byte only? what if the user like to read more
than one byte at a one read?</font></p>

<p class="MsoNormal" style="margin:0cm 0cm 0.0001pt"><font face="arial, sans-serif">as I mentioned above the dwcap is
tricky (probably we needed to explained the use of it better) so the write of
16 bit can be problematic.</font></p>

<p class="MsoNormal" style="margin:0cm 0cm 0.0001pt"><font face="arial, sans-serif">I think we can solve the 16 bit
write by doing a padding the same as we doing when having 8 bit write, and when
dwcap is enabling</font></p>

<p class="MsoNormal" style="margin:0cm 0cm 0.0001pt"><font face="arial, sans-serif">We can read only 4 byte multiplier.</font></p></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Tested:<br>
    Ran as a module with multiple readers and saw the correct values<br>
    reaching all of the readers. Also tested adding and removing readers<br>
    at runtime and reloading the kernel module and validating the<br>
    register state.<br>
<br>
Change-Id: Ic979f523ccc7cda76a2328c5f8c869aa25d7204d<br>
Signed-off-by: William A. Kennington III <<a href="mailto:wak@google.com" target="_blank">wak@google.com</a>><br>
---<br>
 drivers/misc/npcm7xx-lpc-bpc.c | 388 ++++++++++++++++++++-------------<br>
 1 file changed, 235 insertions(+), 153 deletions(-)<br>
<br>
diff --git a/drivers/misc/npcm7xx-lpc-bpc.c b/drivers/misc/npcm7xx-lpc-bpc.c<br>
index e014e07cd4a46..b04323c4f932d 100644<br>
--- a/drivers/misc/npcm7xx-lpc-bpc.c<br>
+++ b/drivers/misc/npcm7xx-lpc-bpc.c<br>
@@ -10,6 +10,7 @@<br>
 #include <linux/of.h><br>
 #include <linux/platform_device.h><br>
 #include <linux/regmap.h><br>
+#include <linux/slab.h><br>
 #include <linux/miscdevice.h><br>
 #include <linux/poll.h><br>
<br>
@@ -28,270 +29,348 @@<br>
 #define NPCM7XX_BPCFA1L_REG    0x10 //BIOS POST Code FIFO Address 1 LSB<br>
 #define NPCM7XX_BPCFA1M_REG    0x12 //BIOS POST Code FIFO Address 1 MSB<br>
<br>
-/*BIOS regiser data*/<br>
+/* BIOS regiser data */<br>
 #define FIFO_IOADDR1_ENABLE    0x80<br>
 #define FIFO_IOADDR2_ENABLE    0x40<br>
<br>
 /* BPC interface package and structure definition */<br>
-#define BPC_KFIFO_SIZE         0x400<br>
+#define BPC_KFIFO_SIZE         0x100<br>
<br>
-/*BPC regiser data*/<br>
+/* BPC regiser data */<br>
 #define FIFO_DATA_VALID                0x80<br>
 #define FIFO_OVERFLOW          0x20<br>
 #define FIFO_READY_INT_ENABLE  0x8<br>
 #define FIFO_DWCAPTURE         0x4<br>
 #define FIFO_ADDR_DECODE       0x1<br>
<br>
-/*Host Reset*/<br>
+/* Host Reset */<br>
 #define HOST_RESET_INT_ENABLE  0x10<br>
 #define HOST_RESET_CHANGED     0x40<br>
<br>
+struct npcm7xx_code {<br>
+       u32 data;<br>
+       u8 len;<br>
+};<br>
+<br>
+struct npcm7xx_bpc_file_data {<br>
+       struct list_head                list;<br>
+       struct npcm7xx_bpc_channel      *ch;<br>
+       DECLARE_KFIFO(codes, struct npcm7xx_code, BPC_KFIFO_SIZE);<br>
+       bool                            host_reset;<br>
+};<br>
+<br>
 struct npcm7xx_bpc_channel {<br>
-       struct npcm7xx_bpc      *data;<br>
-       struct kfifo            fifo;<br>
+       struct npcm7xx_bpc      *drv;<br>
        wait_queue_head_t       wq;<br>
-       bool                    host_reset;<br>
+       struct list_head        files;<br>
        struct miscdevice       miscdev;<br>
 };<br>
<br>
 struct npcm7xx_bpc {<br>
        void __iomem                    *base;<br>
+       struct npcm7xx_bpc_channel      chs[NUM_BPC_CHANNELS];<br>
        int                             irq;<br>
        bool                            en_dwcap;<br>
-       struct npcm7xx_bpc_channel      ch[NUM_BPC_CHANNELS];<br>
 };<br>
<br>
-static struct npcm7xx_bpc_channel *npcm7xx_file_to_ch(struct file *file)<br>
+static int npcm7xx_bpc_open(struct inode *inode, struct file *file)<br>
 {<br>
-       return container_of(file->private_data, struct npcm7xx_bpc_channel,<br>
-                           miscdev);<br>
+       struct npcm7xx_bpc_file_data *data;<br>
+<br>
+       data = kmalloc(sizeof(*data), GFP_KERNEL);<br>
+       if (!data)<br>
+               return -ENOMEM;<br>
+<br>
+       INIT_KFIFO(data->codes);<br>
+       data->ch = container_of(file->private_data,<br>
+                               struct npcm7xx_bpc_channel, miscdev);<br>
+       data->host_reset = false;<br>
+<br>
+       file->private_data = data;<br>
+       list_add_rcu(&data->list, &data->ch->files);<br>
+       return 0;<br>
+}<br>
+<br>
+static int npcm7xx_bpc_release(struct inode *inode, struct file *file)<br>
+{<br>
+       struct npcm7xx_bpc_file_data *data = file->private_data;<br>
+<br>
+       if (!data)<br>
+               return -EIO;<br>
+<br>
+       list_del_rcu(&data->list);<br>
+       synchronize_rcu();<br>
+<br>
+       file->private_data = NULL;<br>
+       kfree(data);<br>
+       return 0;<br>
 }<br>
<br>
 static ssize_t npcm7xx_bpc_read(struct file *file, char __user *buffer,<br>
                                size_t count, loff_t *ppos)<br>
 {<br>
-       struct npcm7xx_bpc_channel *chan = npcm7xx_file_to_ch(file);<br>
-       struct npcm7xx_bpc *lpc_bpc = chan->data;<br>
-       unsigned int copied;<br>
+       struct npcm7xx_bpc_file_data *data = file->private_data;<br>
+       struct npcm7xx_code code;<br>
        int ret = 0;<br>
-       int cond_size = 1;<br>
-<br>
-       if (lpc_bpc->en_dwcap)<br>
-               cond_size = 3;<br>
<br>
-       if (kfifo_len(&chan->fifo) < cond_size) {<br>
+       while (!kfifo_get(&data->codes, &code)) {<br>
                if (file->f_flags & O_NONBLOCK)<br>
                        return -EAGAIN;<br>
<br>
                ret = wait_event_interruptible<br>
-                       (chan->wq, kfifo_len(&chan->fifo) > cond_size);<br>
+                       (data->ch->wq, kfifo_len(&data->codes) > 0);<br>
                if (ret == -ERESTARTSYS)<br>
                        return -EINTR;<br>
        }<br>
<br>
-       ret = kfifo_to_user(&chan->fifo, buffer, count, &copied);<br>
+       if (code.len < count)<br>
+               count = code.len;<br>
<br>
-       return ret ? ret : copied;<br>
+       ret = copy_to_user(buffer, &code.data, count);<br>
+       if (ret != 0)<br>
+               return -EFAULT;<br>
+<br>
+       return count;<br>
 }<br>
<br>
 static __poll_t npcm7xx_bpc_poll(struct file *file,<br>
                                 struct poll_table_struct *pt)<br>
 {<br>
-       struct npcm7xx_bpc_channel *chan = npcm7xx_file_to_ch(file);<br>
+       struct npcm7xx_bpc_file_data *data = file->private_data;<br>
        __poll_t mask = 0;<br>
<br>
-       poll_wait(file, &chan->wq, pt);<br>
-       if (!kfifo_is_empty(&chan->fifo))<br>
+       poll_wait(file, &data->ch->wq, pt);<br>
+       if (!kfifo_is_empty(&data->codes))<br>
                mask |= POLLIN;<br>
<br>
-       if (chan->host_reset) {<br>
+       if (data->host_reset) {<br>
                mask |= POLLHUP;<br>
-               chan->host_reset = false;<br>
+               data->host_reset = false;<br>
        }<br>
<br>
        return mask;<br>
 }<br>
<br>
-static const struct file_operations npcm7xx_bpc_fops = {<br>
+static const struct file_operations npcm7xx_bpc_channel_fops = {<br>
        .owner          = THIS_MODULE,<br>
+       .open           = npcm7xx_bpc_open,<br>
+       .release        = npcm7xx_bpc_release,<br>
        .read           = npcm7xx_bpc_read,<br>
        .poll           = npcm7xx_bpc_poll,<br>
        .llseek         = noop_llseek,<br>
 };<br>
<br>
-static irqreturn_t npcm7xx_bpc_irq(int irq, void *arg)<br>
+static void npcm7xx_bpc_channel_update(struct npcm7xx_bpc_channel *ch,<br>
+                                      const struct npcm7xx_code *code)<br>
 {<br>
-       struct npcm7xx_bpc *lpc_bpc = arg;<br>
-       u8 fifo_st;<br>
-       u8 host_st;<br>
-       u8 addr_index = 0;<br>
-       u8 Data;<br>
-       u8 padzero[3] = {0};<br>
-       u8 last_addr_bit = 0;<br>
-       bool isr_flag = false;<br>
-<br>
-       fifo_st = ioread8(lpc_bpc->base + NPCM7XX_BPCFSTAT_REG);<br>
-       while (FIFO_DATA_VALID & fifo_st) {<br>
-                /* If dwcapture enabled only channel 0 (FIFO 0) used */<br>
-               if (!lpc_bpc->en_dwcap)<br>
-                       addr_index = fifo_st & FIFO_ADDR_DECODE;<br>
-               else<br>
-                       last_addr_bit = fifo_st & FIFO_ADDR_DECODE;<br>
-<br>
-               /*Read data from FIFO to clear interrupt*/<br>
-               Data = ioread8(lpc_bpc->base + NPCM7XX_BPCFDATA_REG);<br>
-               if (kfifo_is_full(&lpc_bpc->ch[addr_index].fifo))<br>
-                       kfifo_skip(&lpc_bpc->ch[addr_index].fifo);<br>
-               kfifo_put(&lpc_bpc->ch[addr_index].fifo, Data);<br>
-               if (fifo_st & FIFO_OVERFLOW)<br>
-                       pr_info("BIOS Post Codes FIFO Overflow!!!\n");<br>
+       struct npcm7xx_bpc_file_data *data;<br>
<br>
-               fifo_st = ioread8(lpc_bpc->base + NPCM7XX_BPCFSTAT_REG);<br>
-               if (lpc_bpc->en_dwcap && last_addr_bit) {<br>
-                       if ((fifo_st & FIFO_ADDR_DECODE) ||<br>
-                           ((FIFO_DATA_VALID & fifo_st) == 0)) {<br>
-                               while (kfifo_avail(&lpc_bpc->ch[addr_index].fifo) < DW_PAD_SIZE)<br>
-                                       kfifo_skip(&lpc_bpc->ch[addr_index].fifo);<br>
-                               kfifo_in(&lpc_bpc->ch[addr_index].fifo,<br>
-                                        padzero, DW_PAD_SIZE);<br>
-                       }<br>
+       if (!ch->drv) {<br>
+               pr_warn("BIOS Post Code Update for unconfigured channel\n");<br>
+               return;<br>
+       }<br>
+<br>
+       list_for_each_entry_rcu(data, &ch->files, list) {<br>
+               if (kfifo_is_full(&data->codes))<br>
+                       kfifo_skip(&data->codes);<br>
+               kfifo_put(&data->codes, *code);<br>
+       }<br>
+}<br>
+<br>
+static void npcm7xx_bpc_channel_wake(struct npcm7xx_bpc_channel *ch)<br>
+{<br>
+       if (!ch->drv)<br>
+               return;<br>
+<br>
+       wake_up_interruptible(&ch->wq);<br>
+}<br>
+<br>
+static void npcm7xx_bpc_host_reset(struct npcm7xx_bpc *bpc)<br>
+{<br>
+       struct npcm7xx_bpc_file_data *data;<br>
+       u8 i;<br>
+<br>
+       for (i = 0; i < NUM_BPC_CHANNELS; ++i) {<br>
+               if (!bpc->chs[i].drv)<br>
+                       continue;<br>
+               list_for_each_entry_rcu(data, &bpc->chs[i].files, list) {<br>
+                       data->host_reset = true;<br>
                }<br>
-               isr_flag = true;<br>
        }<br>
+}<br>
+<br>
+static irqreturn_t npcm7xx_bpc_irq(int irq, void *arg)<br>
+{<br>
+       struct npcm7xx_bpc *bpc = arg;<br>
+       struct npcm7xx_code code = {<br>
+               .len = 0,<br>
+               .data = 0,<br>
+       };<br>
+       bool ch_wake[NUM_BPC_CHANNELS] = {};<br>
+       u8 read_byte;<br>
+       u8 status;<br>
+       u8 ch_i;<br>
+       bool reg_valid;<br>
+       irqreturn_t ret = IRQ_NONE;<br>
+<br>
+       rcu_read_lock();<br>
+<br>
+       while (true) {<br>
+               status = ioread8(bpc->base + NPCM7XX_BPCFSTAT_REG);<br>
+               reg_valid = status & FIFO_DATA_VALID;<br>
+               if (code.len > 0 && (!reg_valid || !bpc->en_dwcap ||<br>
+                                    status & FIFO_ADDR_DECODE)) {<br>
+                       npcm7xx_bpc_channel_update(&bpc->chs[ch_i], &code);<br>
+                       ch_wake[ch_i] = true;<br>
+                       code.len = 0;<br>
+                       code.data = 0;<br>
+               }<br>
+               if (!reg_valid)<br>
+                       break;<br>
<br>
-       host_st = ioread8(lpc_bpc->base + NPCM7XX_BPCFMSTAT_REG);<br>
-       if (host_st & HOST_RESET_CHANGED) {<br>
-               iowrite8(HOST_RESET_CHANGED,<br>
-                        lpc_bpc->base + NPCM7XX_BPCFMSTAT_REG);<br>
-               lpc_bpc->ch[addr_index].host_reset = true;<br>
-               isr_flag = true;<br>
+               if (status & FIFO_OVERFLOW)<br>
+                       pr_info("BIOS Post Codes FIFO Overflow!!!\n");<br>
+<br>
+               ch_i = bpc->en_dwcap ? 0 : status & FIFO_ADDR_DECODE;<br>
+               read_byte = ioread8(bpc->base + NPCM7XX_BPCFDATA_REG);<br>
+               code.data |= read_byte << (code.len++ << 3);<br>
        }<br>
<br>
-       if (isr_flag) {<br>
-               wake_up_interruptible(&lpc_bpc->ch[addr_index].wq);<br>
-               return IRQ_HANDLED;<br>
+       status = ioread8(bpc->base + NPCM7XX_BPCFMSTAT_REG);<br>
+       if (status & HOST_RESET_CHANGED) {<br>
+               iowrite8(HOST_RESET_CHANGED, bpc->base + NPCM7XX_BPCFMSTAT_REG);<br>
+               npcm7xx_bpc_host_reset(bpc);<br>
+               for (ch_i = 0; ch_i < NUM_BPC_CHANNELS; ++ch_i)<br>
+                       ch_wake[ch_i] = true;<br>
        }<br>
<br>
-       return IRQ_NONE;<br>
+       rcu_read_unlock();<br>
+<br>
+       for (ch_i = 0; ch_i < NUM_BPC_CHANNELS; ++ch_i)<br>
+               if (ch_wake[ch_i]) {<br>
+                       npcm7xx_bpc_channel_wake(&bpc->chs[ch_i]);<br>
+                       ret = IRQ_HANDLED;<br>
+               }<br>
+<br>
+       return ret;<br>
 }<br>
<br>
-static int npcm7xx_bpc_config_irq(struct npcm7xx_bpc *lpc_bpc,<br>
+static int npcm7xx_bpc_config_irq(struct npcm7xx_bpc *bpc,<br>
                                  struct platform_device *pdev)<br>
 {<br>
        struct device *dev = &pdev->dev;<br>
        int rc;<br>
<br>
-       lpc_bpc->irq = platform_get_irq(pdev, 0);<br>
-       if (lpc_bpc->irq < 0) {<br>
+       bpc->irq = platform_get_irq(pdev, 0);<br>
+       if (bpc->irq < 0) {<br>
                dev_err(dev, "get IRQ failed\n");<br>
-               return lpc_bpc->irq;<br>
+               return bpc->irq;<br>
        }<br>
<br>
-       rc = devm_request_irq(dev, lpc_bpc->irq,<br>
+       rc = devm_request_irq(dev, bpc->irq,<br>
                              npcm7xx_bpc_irq, IRQF_SHARED,<br>
-                             DEVICE_NAME, lpc_bpc);<br>
+                             DEVICE_NAME, bpc);<br>
        if (rc < 0) {<br>
-               dev_warn(dev, "Unable to request IRQ %d\n", lpc_bpc->irq);<br>
+               dev_err(dev, "Unable to request IRQ %d\n", bpc->irq);<br>
                return rc;<br>
        }<br>
<br>
        return 0;<br>
 }<br>
<br>
-static int npcm7xx_enable_bpc(struct npcm7xx_bpc *lpc_bpc, struct device *dev,<br>
-                             int channel, u16 lpc_port)<br>
+static int npcm7xx_bpc_channel_enable(struct npcm7xx_bpc *bpc, struct device *dev,<br>
+                                     int channel, u16 lpc_port)<br>
 {<br>
+       struct npcm7xx_bpc_channel *ch = &bpc->chs[channel];<br>
        int rc;<br>
        u8 addr_en, reg_en;<br>
<br>
-       init_waitqueue_head(&lpc_bpc->ch[channel].wq);<br>
-<br>
-       rc = kfifo_alloc(&lpc_bpc->ch[channel].fifo,<br>
-                        BPC_KFIFO_SIZE, GFP_KERNEL);<br>
-       if (rc)<br>
-               return rc;<br>
+       init_waitqueue_head(&ch->wq);<br>
+       INIT_LIST_HEAD(&ch->files);<br>
<br>
-       lpc_bpc->ch[channel].miscdev.minor = MISC_DYNAMIC_MINOR;<br>
-       lpc_bpc->ch[channel].<a href="http://miscdev.name" rel="noreferrer" target="_blank">miscdev.name</a> =<br>
+       ch->miscdev.minor = MISC_DYNAMIC_MINOR;<br>
+       ch-><a href="http://miscdev.name" rel="noreferrer" target="_blank">miscdev.name</a> =<br>
                devm_kasprintf(dev, GFP_KERNEL, "%s%d", DEVICE_NAME, channel);<br>
-       lpc_bpc->ch[channel].miscdev.fops = &npcm7xx_bpc_fops;<br>
-       lpc_bpc->ch[channel].miscdev.parent = dev;<br>
-       rc = misc_register(&lpc_bpc->ch[channel].miscdev);<br>
+       ch->miscdev.fops = &npcm7xx_bpc_channel_fops;<br>
+       ch->miscdev.parent = dev;<br>
+       rc = misc_register(&ch->miscdev);<br>
        if (rc)<br>
                return rc;<br>
<br>
-       lpc_bpc->ch[channel].data = lpc_bpc;<br>
-       lpc_bpc->ch[channel].host_reset = false;<br>
-<br>
-       /* Enable LPC snoop channel at requested port */<br>
        switch (channel) {<br>
        case 0:<br>
                addr_en = FIFO_IOADDR1_ENABLE;<br>
                iowrite8((u8)lpc_port & 0xFF,<br>
-                        lpc_bpc->base + NPCM7XX_BPCFA1L_REG);<br>
+                        bpc->base + NPCM7XX_BPCFA1L_REG);<br>
                iowrite8((u8)(lpc_port >> 8),<br>
-                        lpc_bpc->base + NPCM7XX_BPCFA1M_REG);<br>
+                        bpc->base + NPCM7XX_BPCFA1M_REG);<br>
                break;<br>
        case 1:<br>
                addr_en = FIFO_IOADDR2_ENABLE;<br>
                iowrite8((u8)lpc_port & 0xFF,<br>
-                        lpc_bpc->base + NPCM7XX_BPCFA2L_REG);<br>
+                        bpc->base + NPCM7XX_BPCFA2L_REG);<br>
                iowrite8((u8)(lpc_port >> 8),<br>
-                        lpc_bpc->base + NPCM7XX_BPCFA2M_REG);<br>
+                        bpc->base + NPCM7XX_BPCFA2M_REG);<br>
                break;<br>
        default:<br>
+               misc_deregister(&ch->miscdev);<br>
                return -EINVAL;<br>
        }<br>
<br>
-       if (lpc_bpc->en_dwcap)<br>
+       if (bpc->en_dwcap)<br>
                addr_en = FIFO_DWCAPTURE;<br>
<br>
-       /*<br>
-        * Enable FIFO Ready Interrupt, FIFO Capture of I/O addr,<br>
-        * and Host Reset<br>
-        */<br>
-       reg_en = ioread8(lpc_bpc->base + NPCM7XX_BPCFEN_REG);<br>
-       iowrite8(reg_en | addr_en | FIFO_READY_INT_ENABLE |<br>
-                HOST_RESET_INT_ENABLE, lpc_bpc->base + NPCM7XX_BPCFEN_REG);<br>
+       reg_en = ioread8(bpc->base + NPCM7XX_BPCFEN_REG);<br>
+       iowrite8(reg_en | addr_en, bpc->base + NPCM7XX_BPCFEN_REG);<br>
<br>
+       smp_mb();<br>
+       ch->drv = bpc;<br>
        return 0;<br>
 }<br>
<br>
-static void npcm7xx_disable_bpc(struct npcm7xx_bpc *lpc_bpc, int channel)<br>
+static void npcm7xx_bpc_channel_disable(struct npcm7xx_bpc *bpc, int channel)<br>
 {<br>
-       u8 reg_en;<br>
+       struct npcm7xx_bpc_channel *ch = &bpc->chs[channel];<br>
+       u8 reg_en = ioread8(bpc->base + NPCM7XX_BPCFEN_REG);<br>
+<br>
+       if (!ch->drv)<br>
+               return;<br>
+       ch->drv = NULL;<br>
<br>
        switch (channel) {<br>
        case 0:<br>
-               reg_en = ioread8(lpc_bpc->base + NPCM7XX_BPCFEN_REG);<br>
-               if (lpc_bpc->en_dwcap)<br>
-                       iowrite8(reg_en & ~FIFO_DWCAPTURE,<br>
-                                lpc_bpc->base + NPCM7XX_BPCFEN_REG);<br>
-               else<br>
-                       iowrite8(reg_en & ~FIFO_IOADDR1_ENABLE,<br>
-                                lpc_bpc->base + NPCM7XX_BPCFEN_REG);<br>
+               iowrite8(reg_en & ~(FIFO_DWCAPTURE | FIFO_IOADDR1_ENABLE),<br>
+                        bpc->base + NPCM7XX_BPCFEN_REG);<br>
                break;<br>
        case 1:<br>
-               reg_en = ioread8(lpc_bpc->base + NPCM7XX_BPCFEN_REG);<br>
                iowrite8(reg_en & ~FIFO_IOADDR2_ENABLE,<br>
-                        lpc_bpc->base + NPCM7XX_BPCFEN_REG);<br>
+                        bpc->base + NPCM7XX_BPCFEN_REG);<br>
                break;<br>
        default:<br>
                return;<br>
        }<br>
<br>
-       if (!(reg_en & (FIFO_IOADDR1_ENABLE | FIFO_IOADDR2_ENABLE)))<br>
-               iowrite8(reg_en &<br>
-                        ~(FIFO_READY_INT_ENABLE | HOST_RESET_INT_ENABLE),<br>
-                        lpc_bpc->base + NPCM7XX_BPCFEN_REG);<br>
+       misc_deregister(&ch->miscdev);<br>
+}<br>
<br>
-       kfifo_free(&lpc_bpc->ch[channel].fifo);<br>
-       misc_deregister(&lpc_bpc->ch[channel].miscdev);<br>
+static void npcm7xx_bpc_reset(struct npcm7xx_bpc *bpc)<br>
+{<br>
+       u8 reg_en = ioread8(bpc->base + NPCM7XX_BPCFEN_REG);<br>
+       reg_en &= ~(FIFO_IOADDR1_ENABLE | FIFO_IOADDR2_ENABLE | FIFO_DWCAPTURE |<br>
+                       FIFO_READY_INT_ENABLE | HOST_RESET_INT_ENABLE);<br>
+       iowrite8(reg_en, bpc->base + NPCM7XX_BPCFEN_REG);<br>
+}<br>
+<br>
+static void npcm7xx_bpc_enable_irq(struct npcm7xx_bpc *bpc)<br>
+{<br>
+       u8 reg_en = ioread8(bpc->base + NPCM7XX_BPCFEN_REG);<br>
+       reg_en |= FIFO_READY_INT_ENABLE | HOST_RESET_INT_ENABLE;<br>
+       iowrite8(reg_en, bpc->base + NPCM7XX_BPCFEN_REG);<br>
 }<br>
<br>
 static int npcm7xx_bpc_probe(struct platform_device *pdev)<br>
 {<br>
-       struct npcm7xx_bpc *lpc_bpc;<br>
+       struct npcm7xx_bpc *bpc;<br>
        struct resource *res;<br>
        struct device *dev;<br>
        u32 port;<br>
@@ -299,8 +378,8 @@ static int npcm7xx_bpc_probe(struct platform_device *pdev)<br>
<br>
        dev = &pdev->dev;<br>
<br>
-       lpc_bpc = devm_kzalloc(dev, sizeof(*lpc_bpc), GFP_KERNEL);<br>
-       if (!lpc_bpc)<br>
+       bpc = devm_kzalloc(dev, sizeof(*bpc), GFP_KERNEL);<br>
+       if (!bpc)<br>
                return -ENOMEM;<br>
<br>
        res = platform_get_resource(pdev, IORESOURCE_MEM, 0);<br>
@@ -310,11 +389,11 @@ static int npcm7xx_bpc_probe(struct platform_device *pdev)<br>
        }<br>
<br>
        dev_dbg(dev, "BIOS post code base resource is %pR\n", res);<br>
-       lpc_bpc->base = devm_ioremap_resource(dev, res);<br>
-       if (IS_ERR(lpc_bpc->base))<br>
-               return PTR_ERR(lpc_bpc->base);<br>
+       bpc->base = devm_ioremap_resource(dev, res);<br>
+       if (IS_ERR(bpc->base))<br>
+               return PTR_ERR(bpc->base);<br>
<br>
-       dev_set_drvdata(&pdev->dev, lpc_bpc);<br>
+       dev_set_drvdata(&pdev->dev, bpc);<br>
<br>
        rc = of_property_read_u32_index(dev->of_node, "monitor-ports", 0,<br>
                                        &port);<br>
@@ -323,14 +402,16 @@ static int npcm7xx_bpc_probe(struct platform_device *pdev)<br>
                return -ENODEV;<br>
        }<br>
<br>
-       lpc_bpc->en_dwcap =<br>
+       bpc->en_dwcap =<br>
                of_property_read_bool(dev->of_node, "bpc-en-dwcapture");<br>
<br>
-       rc = npcm7xx_bpc_config_irq(lpc_bpc, pdev);<br>
+       npcm7xx_bpc_reset(bpc);<br>
+       rc = npcm7xx_bpc_config_irq(bpc, pdev);<br>
        if (rc)<br>
                return rc;<br>
+       npcm7xx_bpc_enable_irq(bpc);<br>
<br>
-       rc = npcm7xx_enable_bpc(lpc_bpc, dev, 0, port);<br>
+       rc = npcm7xx_bpc_channel_enable(bpc, dev, 0, port);<br>
        if (rc) {<br>
                dev_err(dev, "Enable BIOS post code I/O port 0 failed\n");<br>
                return rc;<br>
@@ -340,35 +421,36 @@ static int npcm7xx_bpc_probe(struct platform_device *pdev)<br>
         * Configuration of second BPC channel port is optional<br>
         * Double-Word Capture ignoring address 2<br>
         */<br>
-       if (!lpc_bpc->en_dwcap) {<br>
-               if (of_property_read_u32_index(dev->of_node, "monitor-ports",<br>
-                                              1, &port) == 0) {<br>
-                       rc = npcm7xx_enable_bpc(lpc_bpc, dev, 1, port);<br>
+       rc = of_property_read_u32_index(dev->of_node, "monitor-ports", 1,<br>
+                                       &port);<br>
+       if (rc == 0) {<br>
+               if (!bpc->en_dwcap) {<br>
+                       rc = npcm7xx_bpc_channel_enable(bpc, dev, 1, port);<br>
                        if (rc) {<br>
-                               dev_err(dev, "Enable BIOS post code I/O port 1 failed, disable I/O port 0\n");<br>
-                               npcm7xx_disable_bpc(lpc_bpc, 0);<br>
+                               dev_err(dev, "Enable BIOS post code I/O port 1 failed\n");<br>
+                               npcm7xx_bpc_channel_disable(bpc, 0);<br>
+                               npcm7xx_bpc_reset(bpc);<br>
                                return rc;<br>
                        }<br>
+               } else {<br>
+                       dev_warn(dev, "Ignoring monitor port 1 with DWCAP\n");<br>
                }<br>
        }<br>
<br>
-       pr_info("npcm7xx BIOS post code probe\n");<br>
-<br>
-       return rc;<br>
+       return 0;<br>
 }<br>
<br>
 static int npcm7xx_bpc_remove(struct platform_device *pdev)<br>
 {<br>
-       struct npcm7xx_bpc *lpc_bpc = dev_get_drvdata(&pdev->dev);<br>
-       u8 reg_en;<br>
-<br>
-       reg_en = ioread8(lpc_bpc->base + NPCM7XX_BPCFEN_REG);<br>
+       struct npcm7xx_bpc *bpc = dev_get_drvdata(&pdev->dev);<br>
+       u8 i;<br>
<br>
-       if (reg_en & FIFO_IOADDR1_ENABLE)<br>
-               npcm7xx_disable_bpc(lpc_bpc, 0);<br>
-       if (reg_en & FIFO_IOADDR2_ENABLE)<br>
-               npcm7xx_disable_bpc(lpc_bpc, 1);<br>
+       if (!bpc)<br>
+               return 0;<br>
<br>
+       for (i = 0; i < NUM_BPC_CHANNELS; ++i)<br>
+               npcm7xx_bpc_channel_disable(bpc, i);<br>
+       npcm7xx_bpc_reset(bpc);<br>
        return 0;<br>
 }<br>
<br>
-- <br>
2.24.1<br>
<br></blockquote><div><br></div><div><p class="MsoNormal" style="margin:0cm 0cm 0.0001pt"><span style="font-family:arial,sans-serif">on the upstream side,</span><br></p></div><div><font face="arial, sans-serif"><br></font></div><div><font face="arial, sans-serif">We didn’t succeed to upstream BPC
driver because Linux community wants to create a BMC driver framework in
the </font></div>

<p class="MsoNormal" style="margin:0cm 0cm 0.0001pt"><font face="arial, sans-serif">Linux kernel driver folder for all
the BMC unique modules and not using misc framework.</font></p>

<p class="MsoNormal" style="margin:0cm 0cm 0.0001pt"><font face="arial, sans-serif">(Joel is leading this :-))</font></p>

<p class="MsoNormal" style="margin:0cm 0cm 0.0001pt"><font face="arial, sans-serif"> </font></p>

<p class="MsoNormal" style="margin:0cm 0cm 0.0001pt"><font face="arial, sans-serif">Probably the driver will modify
once we will have the BMC framework.</font></p><p class="MsoNormal" style="margin:0cm 0cm 0.0001pt"><font face="arial, sans-serif"><br></font></p><p class="MsoNormal" style="margin:0cm 0cm 0.0001pt"><font face="arial, sans-serif">Thanks,</font></p><p class="MsoNormal" style="margin:0cm 0cm 0.0001pt"><font face="arial, sans-serif"><br></font></p><p class="MsoNormal" style="margin:0cm 0cm 0.0001pt"><font face="arial, sans-serif">Tomer</font></p></div></div>