* Re: [PATCH] ipmi: bt-bmc: Use registers directly
[not found] <20210903051039.307991-1-joel@jms.id.au>
@ 2021-10-06 2:33 ` Joel Stanley
2021-10-06 11:59 ` Corey Minyard
0 siblings, 1 reply; 2+ messages in thread
From: Joel Stanley @ 2021-10-06 2:33 UTC (permalink / raw)
To: Corey Minyard
Cc: openipmi-developer, Linux ARM, Andrew Jeffery, Alistar Popple
Hi Corey,
On Fri, 3 Sept 2021 at 05:10, Joel Stanley <joel@jms.id.au> wrote:
>
> This driver was originally written to use the regmap abstraction with no
> clear benefit. As the registers are always mmio and there is no sharing
> of the region with other devices, we can safely read and write without
> the locking that regmap provides.
>
> This reduces the code size of the driver by about 25%.
Do you have any feedback on this one?
Cheers,
Joel
>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
> drivers/char/ipmi/bt-bmc.c | 68 +++++++++-----------------------------
> 1 file changed, 16 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/char/ipmi/bt-bmc.c b/drivers/char/ipmi/bt-bmc.c
> index 6e3d247b55d1..fb771ce6f7bf 100644
> --- a/drivers/char/ipmi/bt-bmc.c
> +++ b/drivers/char/ipmi/bt-bmc.c
> @@ -8,13 +8,11 @@
> #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>
>
> @@ -59,8 +57,7 @@
> struct bt_bmc {
> struct device dev;
> struct miscdevice miscdev;
> - struct regmap *map;
> - int offset;
> + void __iomem *base;
> int irq;
> wait_queue_head_t queue;
> struct timer_list poll_timer;
> @@ -69,29 +66,14 @@ struct bt_bmc {
>
> static atomic_t open_count = ATOMIC_INIT(0);
>
> -static const struct regmap_config bt_regmap_cfg = {
> - .reg_bits = 32,
> - .val_bits = 32,
> - .reg_stride = 4,
> -};
> -
> static u8 bt_inb(struct bt_bmc *bt_bmc, int reg)
> {
> - uint32_t val = 0;
> - int rc;
> -
> - rc = regmap_read(bt_bmc->map, bt_bmc->offset + reg, &val);
> - WARN(rc != 0, "regmap_read() failed: %d\n", rc);
> -
> - return rc == 0 ? (u8) val : 0;
> + return readb(bt_bmc->base + reg);
> }
>
> static void bt_outb(struct bt_bmc *bt_bmc, u8 data, int reg)
> {
> - int rc;
> -
> - rc = regmap_write(bt_bmc->map, bt_bmc->offset + reg, data);
> - WARN(rc != 0, "regmap_write() failed: %d\n", rc);
> + writeb(data, bt_bmc->base + reg);
> }
>
> static void clr_rd_ptr(struct bt_bmc *bt_bmc)
> @@ -376,18 +358,15 @@ 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, ®);
> - if (rc)
> - return IRQ_NONE;
> + reg = readl(bt_bmc->base + BT_CR2);
>
> reg &= BT_CR2_IRQ_H2B | BT_CR2_IRQ_HBUSY;
> if (!reg)
> return IRQ_NONE;
>
> /* ack pending IRQs */
> - regmap_write(bt_bmc->map, bt_bmc->offset + BT_CR2, reg);
> + writel(reg, bt_bmc->base + BT_CR2);
>
> wake_up(&bt_bmc->queue);
> return IRQ_HANDLED;
> @@ -398,6 +377,7 @@ static int bt_bmc_config_irq(struct bt_bmc *bt_bmc,
> {
> struct device *dev = &pdev->dev;
> int rc;
> + u32 reg;
>
> bt_bmc->irq = platform_get_irq_optional(pdev, 0);
> if (bt_bmc->irq < 0)
> @@ -417,11 +397,11 @@ 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
> */
> - 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));
> + reg = readl(bt_bmc->base + BT_CR1);
> + reg |= BT_CR1_IRQ_H2B | BT_CR1_IRQ_HBUSY;
> + writel(reg, bt_bmc->base + BT_CR1);
>
> - return rc;
> + return 0;
> }
>
> static int bt_bmc_probe(struct platform_device *pdev)
> @@ -439,25 +419,9 @@ static int bt_bmc_probe(struct platform_device *pdev)
>
> dev_set_drvdata(&pdev->dev, bt_bmc);
>
> - bt_bmc->map = syscon_node_to_regmap(pdev->dev.parent->of_node);
> - if (IS_ERR(bt_bmc->map)) {
> - void __iomem *base;
> -
> - /*
> - * Assume it's not the MFD-based devicetree description, in
> - * which case generate a regmap ourselves
> - */
> - base = devm_platform_ioremap_resource(pdev, 0);
> - 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;
> - }
> + bt_bmc->base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(bt_bmc->base))
> + return PTR_ERR(bt_bmc->base);
>
> mutex_init(&bt_bmc->mutex);
> init_waitqueue_head(&bt_bmc->queue);
> @@ -483,12 +447,12 @@ static int bt_bmc_probe(struct platform_device *pdev)
> add_timer(&bt_bmc->poll_timer);
> }
>
> - regmap_write(bt_bmc->map, bt_bmc->offset + BT_CR0,
> - (BT_IO_BASE << BT_CR0_IO_BASE) |
> + writel((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_CR0_ENABLE_IBT,
> + bt_bmc->base + BT_CR0);
>
> clr_b_busy(bt_bmc);
>
> --
> 2.33.0
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH] ipmi: bt-bmc: Use registers directly
2021-10-06 2:33 ` [PATCH] ipmi: bt-bmc: Use registers directly Joel Stanley
@ 2021-10-06 11:59 ` Corey Minyard
0 siblings, 0 replies; 2+ messages in thread
From: Corey Minyard @ 2021-10-06 11:59 UTC (permalink / raw)
To: Joel Stanley
Cc: openipmi-developer, Linux ARM, Andrew Jeffery, Alistar Popple
On Wed, Oct 06, 2021 at 02:33:06AM +0000, Joel Stanley wrote:
> Hi Corey,
>
> On Fri, 3 Sept 2021 at 05:10, Joel Stanley <joel@jms.id.au> wrote:
> >
> > This driver was originally written to use the regmap abstraction with no
> > clear benefit. As the registers are always mmio and there is no sharing
> > of the region with other devices, we can safely read and write without
> > the locking that regmap provides.
> >
> > This reduces the code size of the driver by about 25%.
>
> Do you have any feedback on this one?
Ah, sorry, this looks ok to me. I was unable to find much useful
information about the benefits of regmap, but it seems that if you had
registers layed out in various ways, with different caching, etc, that
regmap would be useful. It might be useful for the host side driver
because it deals with various register layouts. However, for this
application, it doesn't seem useful. I didn't see any other comments on
it, but it looks clean. Applied to my next tree.
-corey
>
> Cheers,
>
> Joel
>
>
> >
> > Signed-off-by: Joel Stanley <joel@jms.id.au>
> > ---
> > drivers/char/ipmi/bt-bmc.c | 68 +++++++++-----------------------------
> > 1 file changed, 16 insertions(+), 52 deletions(-)
> >
> > diff --git a/drivers/char/ipmi/bt-bmc.c b/drivers/char/ipmi/bt-bmc.c
> > index 6e3d247b55d1..fb771ce6f7bf 100644
> > --- a/drivers/char/ipmi/bt-bmc.c
> > +++ b/drivers/char/ipmi/bt-bmc.c
> > @@ -8,13 +8,11 @@
> > #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>
> >
> > @@ -59,8 +57,7 @@
> > struct bt_bmc {
> > struct device dev;
> > struct miscdevice miscdev;
> > - struct regmap *map;
> > - int offset;
> > + void __iomem *base;
> > int irq;
> > wait_queue_head_t queue;
> > struct timer_list poll_timer;
> > @@ -69,29 +66,14 @@ struct bt_bmc {
> >
> > static atomic_t open_count = ATOMIC_INIT(0);
> >
> > -static const struct regmap_config bt_regmap_cfg = {
> > - .reg_bits = 32,
> > - .val_bits = 32,
> > - .reg_stride = 4,
> > -};
> > -
> > static u8 bt_inb(struct bt_bmc *bt_bmc, int reg)
> > {
> > - uint32_t val = 0;
> > - int rc;
> > -
> > - rc = regmap_read(bt_bmc->map, bt_bmc->offset + reg, &val);
> > - WARN(rc != 0, "regmap_read() failed: %d\n", rc);
> > -
> > - return rc == 0 ? (u8) val : 0;
> > + return readb(bt_bmc->base + reg);
> > }
> >
> > static void bt_outb(struct bt_bmc *bt_bmc, u8 data, int reg)
> > {
> > - int rc;
> > -
> > - rc = regmap_write(bt_bmc->map, bt_bmc->offset + reg, data);
> > - WARN(rc != 0, "regmap_write() failed: %d\n", rc);
> > + writeb(data, bt_bmc->base + reg);
> > }
> >
> > static void clr_rd_ptr(struct bt_bmc *bt_bmc)
> > @@ -376,18 +358,15 @@ 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, ®);
> > - if (rc)
> > - return IRQ_NONE;
> > + reg = readl(bt_bmc->base + BT_CR2);
> >
> > reg &= BT_CR2_IRQ_H2B | BT_CR2_IRQ_HBUSY;
> > if (!reg)
> > return IRQ_NONE;
> >
> > /* ack pending IRQs */
> > - regmap_write(bt_bmc->map, bt_bmc->offset + BT_CR2, reg);
> > + writel(reg, bt_bmc->base + BT_CR2);
> >
> > wake_up(&bt_bmc->queue);
> > return IRQ_HANDLED;
> > @@ -398,6 +377,7 @@ static int bt_bmc_config_irq(struct bt_bmc *bt_bmc,
> > {
> > struct device *dev = &pdev->dev;
> > int rc;
> > + u32 reg;
> >
> > bt_bmc->irq = platform_get_irq_optional(pdev, 0);
> > if (bt_bmc->irq < 0)
> > @@ -417,11 +397,11 @@ 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
> > */
> > - 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));
> > + reg = readl(bt_bmc->base + BT_CR1);
> > + reg |= BT_CR1_IRQ_H2B | BT_CR1_IRQ_HBUSY;
> > + writel(reg, bt_bmc->base + BT_CR1);
> >
> > - return rc;
> > + return 0;
> > }
> >
> > static int bt_bmc_probe(struct platform_device *pdev)
> > @@ -439,25 +419,9 @@ static int bt_bmc_probe(struct platform_device *pdev)
> >
> > dev_set_drvdata(&pdev->dev, bt_bmc);
> >
> > - bt_bmc->map = syscon_node_to_regmap(pdev->dev.parent->of_node);
> > - if (IS_ERR(bt_bmc->map)) {
> > - void __iomem *base;
> > -
> > - /*
> > - * Assume it's not the MFD-based devicetree description, in
> > - * which case generate a regmap ourselves
> > - */
> > - base = devm_platform_ioremap_resource(pdev, 0);
> > - 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;
> > - }
> > + bt_bmc->base = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(bt_bmc->base))
> > + return PTR_ERR(bt_bmc->base);
> >
> > mutex_init(&bt_bmc->mutex);
> > init_waitqueue_head(&bt_bmc->queue);
> > @@ -483,12 +447,12 @@ static int bt_bmc_probe(struct platform_device *pdev)
> > add_timer(&bt_bmc->poll_timer);
> > }
> >
> > - regmap_write(bt_bmc->map, bt_bmc->offset + BT_CR0,
> > - (BT_IO_BASE << BT_CR0_IO_BASE) |
> > + writel((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_CR0_ENABLE_IBT,
> > + bt_bmc->base + BT_CR0);
> >
> > clr_b_busy(bt_bmc);
> >
> > --
> > 2.33.0
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-10-06 12:01 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20210903051039.307991-1-joel@jms.id.au>
2021-10-06 2:33 ` [PATCH] ipmi: bt-bmc: Use registers directly Joel Stanley
2021-10-06 11:59 ` Corey Minyard
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).