Re: [PATCH] ipmi: bt-bmc: Use a regmap for register access

[Andrew Jeffery]

On Wed, 2016-12-14 at 11:59 +1030, Joel Stanley wrote:
> > On Tue, Dec 6, 2016 at 1:27 PM, Andrew Jeffery <andrew@xxxxxxxx> wrote:
> > The registers for the bt-bmc device live under the Aspeed LPC
> > controller. Devicetree bindings have recently been introduced for the
> > LPC controller where the "host" portion of the LPC register space is
> > described as a syscon device. Future devicetrees describing the bt-bmc
> > device should nest its node under the appropriate "simple-mfd", "syscon"
> > compatible node.
> > 
> > This change allows the bt-bmc driver to function with both syscon and
> > non-syscon- based devicetree descriptions by always using a regmap for
> > register access, either retrieved from the parent syscon device or
> > instantiated if none exists.
> > 
> > The patch has been tested on an OpenPOWER Palmetto machine, successfully
> > booting, rebooting and powering down the host.
> > 
> > > > Signed-off-by: Andrew Jeffery <andrew@xxxxxxxx>
> > ---
> > Âdrivers/char/ipmi/KconfigÂÂ|ÂÂ1 +
> > Âdrivers/char/ipmi/bt-bmc.c | 82 ++++++++++++++++++++++++++++++++++------------
> > Â2 files changed, 62 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig
> > index 7f816655cbbf..b5d48d9af124 100644
> > --- a/drivers/char/ipmi/Kconfig
> > +++ b/drivers/char/ipmi/Kconfig
> > @@ -79,6 +79,7 @@ endif # IPMI_HANDLER
> > 
> > Âconfig ASPEED_BT_IPMI_BMC
> > ÂÂÂÂÂÂÂÂdepends on ARCH_ASPEED
> 
> If you do a v2 of this series it would be great to add || COMPILE_TEST here.
> 
> > +ÂÂÂÂÂÂÂÂdepends on REGMAP && REGMAP_MMIO && MFD_SYSCON
> > ÂÂÂÂÂÂÂÂtristate "BT IPMI bmc driver"
> > ÂÂÂÂÂÂÂÂhelp
> > ÂÂÂÂÂÂÂÂÂÂProvides a driver for the BT (Block Transfer) IPMI interface
> > diff --git a/drivers/char/ipmi/bt-bmc.c b/drivers/char/ipmi/bt-bmc.c
> > index fc9e8891eae3..ca1e20f6c6c5 100644
> > --- a/drivers/char/ipmi/bt-bmc.c
> > +++ b/drivers/char/ipmi/bt-bmc.c
> > @@ -12,10 +12,13 @@
> > Â#include <linux/errno.h>
> > Â#include <linux/interrupt.h>
> > Â#include <linux/io.h>
> > +#include <linux/mfd/syscon.h>
> > Â#include <linux/miscdevice.h>
> > Â#include <linux/module.h>
> > +#include <linux/of.h>
> > Â#include <linux/platform_device.h>
> > Â#include <linux/poll.h>
> > +#include <linux/regmap.h>
> > Â#include <linux/sched.h>
> > Â#include <linux/timer.h>
> > 
> > @@ -60,7 +63,8 @@
> > Âstruct bt_bmc {
> > ÂÂÂÂÂÂÂÂstruct deviceÂÂÂÂÂÂÂÂÂÂÂdev;
> > ÂÂÂÂÂÂÂÂstruct miscdeviceÂÂÂÂÂÂÂmiscdev;
> > -ÂÂÂÂÂÂÂvoid __iomemÂÂÂÂÂÂÂÂÂÂÂÂ*base;
> > +ÂÂÂÂÂÂÂstruct regmapÂÂÂÂÂÂÂÂÂÂÂ*map;
> > +ÂÂÂÂÂÂÂintÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂoffset;
> > ÂÂÂÂÂÂÂÂintÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂirq;
> > ÂÂÂÂÂÂÂÂwait_queue_head_tÂÂÂÂÂÂÂqueue;
> > ÂÂÂÂÂÂÂÂstruct timer_listÂÂÂÂÂÂÂpoll_timer;
> > @@ -69,14 +73,31 @@ struct bt_bmc {
> > 
> > Âstatic atomic_t open_count = ATOMIC_INIT(0);
> > 
> > +static struct regmap_config bt_regmap_cfg = {
> 
> const?

Good point. Is it worth a v2?

> 
> > +ÂÂÂÂÂÂÂ.reg_bits = 32,
> > +ÂÂÂÂÂÂÂ.val_bits = 32,
> > +ÂÂÂÂÂÂÂ.reg_stride = 4,
> > +};
> > +
> > Âstatic u8 bt_inb(struct bt_bmc *bt_bmc, int reg)
> > Â{
> > -ÂÂÂÂÂÂÂreturn ioread8(bt_bmc->base + reg);
> > +ÂÂÂÂÂÂÂuint32_t val = 0;
> > +ÂÂÂÂÂÂÂint rc;
> > +
> > +ÂÂÂÂÂÂÂrc = regmap_read(bt_bmc->map, bt_bmc->offset + reg, &val);
> > +ÂÂÂÂÂÂÂWARN(rc != 0, "%s:%d: regmap_read() failed: %d\n",
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ__FILE__, __LINE__, rc);
> 
> Under what circumstances do we expect the read to fail?

By the regmap_read() implementation for MMIO it should never fail. If
it does, then we should tell someone about it.

> 
> This isn't much cleaner, but I prefer it slightly:
> 
> rc = regmap_read(bt_bmc->map, bt_bmc->offset + reg, &val);
> if (rc) {
> ÂÂÂdev_warn(bt_bmc->dev, "read failed %d\n", rc);
> ÂÂÂreturn rc;
> }
> 
> return val;

bt_inb() currently returns a u8 type, and as such no caller performs
error checking. While your suggestion might seem slightly preferable,
it feels likely "slightly preferable" might fail to take into account
the cascading effect of changing the return type and testing for errors
at each of the (transitive) call-sites.

I think the additional code required makes your suggestion less
attractive given that the call should never fail.

If you decide you feel strongly about it I can make the change.

> 
> > +
> > +ÂÂÂÂÂÂÂreturn rc == 0 ? (u8) val : 0;
> > Â}
> > 
> > Âstatic void bt_outb(struct bt_bmc *bt_bmc, u8 data, int reg)
> > Â{
> > -ÂÂÂÂÂÂÂiowrite8(data, bt_bmc->base + reg);
> > +ÂÂÂÂÂÂÂint rc;
> > +
> > +ÂÂÂÂÂÂÂrc = regmap_write(bt_bmc->map, bt_bmc->offset + reg, data);
> > +ÂÂÂÂÂÂÂWARN(rc != 0, "%s:%d: regmap_write() failed: %d\n",
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ__FILE__, __LINE__, rc);
> > Â}
> > 
> > Âstatic void clr_rd_ptr(struct bt_bmc *bt_bmc)
> > @@ -367,14 +388,18 @@ static irqreturn_t bt_bmc_irq(int irq, void *arg)
> > Â{
> > ÂÂÂÂÂÂÂÂstruct bt_bmc *bt_bmc = arg;
> > ÂÂÂÂÂÂÂÂu32 reg;
> > +ÂÂÂÂÂÂÂint rc;
> > +
> > +ÂÂÂÂÂÂÂrc = regmap_read(bt_bmc->map, bt_bmc->offset + BT_CR2, &reg);
> > +ÂÂÂÂÂÂÂif (rc)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn IRQ_NONE;
> > 
> > -ÂÂÂÂÂÂÂreg = ioread32(bt_bmc->base + BT_CR2);
> > ÂÂÂÂÂÂÂÂreg &= BT_CR2_IRQ_H2B | BT_CR2_IRQ_HBUSY;
> > ÂÂÂÂÂÂÂÂif (!reg)
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn IRQ_NONE;
> > 
> > ÂÂÂÂÂÂÂÂ/* ack pending IRQs */
> > -ÂÂÂÂÂÂÂiowrite32(reg, bt_bmc->base + BT_CR2);
> > +ÂÂÂÂÂÂÂregmap_write(bt_bmc->map, bt_bmc->offset + BT_CR2, reg);
> > 
> > ÂÂÂÂÂÂÂÂwake_up(&bt_bmc->queue);
> > ÂÂÂÂÂÂÂÂreturn IRQ_HANDLED;
> > @@ -384,7 +409,6 @@ static int bt_bmc_config_irq(struct bt_bmc *bt_bmc,
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂstruct platform_device *pdev)
> > Â{
> > ÂÂÂÂÂÂÂÂstruct device *dev = &pdev->dev;
> > -ÂÂÂÂÂÂÂu32 reg;
> > ÂÂÂÂÂÂÂÂint rc;
> > 
> > ÂÂÂÂÂÂÂÂbt_bmc->irq = platform_get_irq(pdev, 0);
> > @@ -405,18 +429,17 @@ static int bt_bmc_config_irq(struct bt_bmc *bt_bmc,
> > ÂÂÂÂÂÂÂÂÂ* will be cleared (along with B2H) when we can write the next
> > ÂÂÂÂÂÂÂÂÂ* message to the BT buffer
> > ÂÂÂÂÂÂÂÂÂ*/
> > -ÂÂÂÂÂÂÂreg = ioread32(bt_bmc->base + BT_CR1);
> > -ÂÂÂÂÂÂÂreg |= BT_CR1_IRQ_H2B | BT_CR1_IRQ_HBUSY;
> > -ÂÂÂÂÂÂÂiowrite32(reg, bt_bmc->base + BT_CR1);
> > +ÂÂÂÂÂÂÂrc = regmap_update_bits(bt_bmc->map, bt_bmc->offset + BT_CR1,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ(BT_CR1_IRQ_H2B | BT_CR1_IRQ_HBUSY),
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ(BT_CR1_IRQ_H2B | BT_CR1_IRQ_HBUSY));
> 
> You could drop the ( ) around the flags if you want.

I think I wrote it this way when I was toying with different line-
wraps, but regardless I think they are a nice visual queue.

Andrew

> 
> > 
> > -ÂÂÂÂÂÂÂreturn 0;
> > +ÂÂÂÂÂÂÂreturn rc;
> > Â}
> > 
> > Âstatic int bt_bmc_probe(struct platform_device *pdev)
> > Â{
> > ÂÂÂÂÂÂÂÂstruct bt_bmc *bt_bmc;
> > ÂÂÂÂÂÂÂÂstruct device *dev;
> > -ÂÂÂÂÂÂÂstruct resource *res;
> > ÂÂÂÂÂÂÂÂint rc;
> > 
> > ÂÂÂÂÂÂÂÂif (!pdev || !pdev->dev.of_node)
> > @@ -431,10 +454,27 @@ static int bt_bmc_probe(struct platform_device *pdev)
> > 
> > ÂÂÂÂÂÂÂÂdev_set_drvdata(&pdev->dev, bt_bmc);
> > 
> > -ÂÂÂÂÂÂÂres = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > -ÂÂÂÂÂÂÂbt_bmc->base = devm_ioremap_resource(&pdev->dev, res);
> > -ÂÂÂÂÂÂÂif (IS_ERR(bt_bmc->base))
> > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn PTR_ERR(bt_bmc->base);
> > +ÂÂÂÂÂÂÂbt_bmc->map = syscon_node_to_regmap(pdev->dev.parent->of_node);
> > +ÂÂÂÂÂÂÂif (IS_ERR(bt_bmc->map)) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂstruct resource *res;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂvoid __iomem *base;
> > +
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ/*
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ* Assume it's not the MFD-based devicetree description, in
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ* which case generate a regmap ourselves
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ*/
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂres = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbase = devm_ioremap_resource(&pdev->dev, res);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (IS_ERR(base))
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn PTR_ERR(base);
> > +
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbt_bmc->map = devm_regmap_init_mmio(dev, base, &bt_regmap_cfg);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbt_bmc->offset = 0;
> > +ÂÂÂÂÂÂÂ} else {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂrc = of_property_read_u32(dev->of_node, "reg", &bt_bmc->offset);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (rc)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn rc;
> > +ÂÂÂÂÂÂÂ}
> > 
> > ÂÂÂÂÂÂÂÂmutex_init(&bt_bmc->mutex);
> > ÂÂÂÂÂÂÂÂinit_waitqueue_head(&bt_bmc->queue);
> > @@ -461,12 +501,12 @@ static int bt_bmc_probe(struct platform_device *pdev)
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂadd_timer(&bt_bmc->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_bmc->base + BT_CR0);
> > +ÂÂÂÂÂÂÂregmap_write(bt_bmc->map, bt_bmc->offset + BT_CR0,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ(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);
> > 
> > ÂÂÂÂÂÂÂÂclr_b_busy(bt_bmc);
> > 
> > --
> > 2.9.3
> >

Attachment: signature.asc
Description: This is a digitally signed message part