From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752844AbcLGALJ (ORCPT ); Tue, 6 Dec 2016 19:11:09 -0500 Received: from out2-smtp.messagingengine.com ([66.111.4.26]:60748 "EHLO out2-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752475AbcLGALG (ORCPT ); Tue, 6 Dec 2016 19:11:06 -0500 X-ME-Sender: X-Sasl-enc: wlClnnqRxIITtvZ9uSo65bs03i/3C/V7Cy6jnnZHeLZW 1481069089 Message-ID: <1481069081.7840.3.camel@aj.id.au> Subject: Re: [PATCH] ipmi: bt-bmc: Use a regmap for register access From: Andrew Jeffery To: =?ISO-8859-1?Q?C=E9dric?= Le Goater , Corey Minyard Cc: Joel Stanley , openipmi-developer@lists.sourceforge.net, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Date: Wed, 07 Dec 2016 11:04:41 +1100 In-Reply-To: <36014e0c-82f8-eeaf-ee2a-6c1e413b957d@kaod.org> References: <20161206025715.2002-1-andrew@aj.id.au> <36014e0c-82f8-eeaf-ee2a-6c1e413b957d@kaod.org> Content-Type: multipart/signed; micalg="pgp-sha512"; protocol="application/pgp-signature"; boundary="=-WKkhe6UTknK0psrUYfuM" X-Mailer: Evolution 3.22.1-0ubuntu2 Mime-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --=-WKkhe6UTknK0psrUYfuM Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, 2016-12-06 at 16:02 +0100, C=C3=A9dric Le Goater wrote: > [ this is a resend bc of some mailing list issues]=C2=A0 Thanks for resending. >=20 > On 12/06/2016 03:57 AM, Andrew Jeffery 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. > >=20 > > 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. > >=20 > > The patch has been tested on an OpenPOWER Palmetto machine, successfull= y > > booting, rebooting and powering down the host. > >=20 > > Signed-off-by: Andrew Jeffery >=20 > It would be nice to have an example of the associated binding.=C2=A0 > I did not see it. Essentially because the approach of the patch means there's no required change to the bindings documentation. So, pulling together the various patches I've sent out, a partial devicetree using the new bindings might look something like: lpc: lpc@1e789000 { compatible =3D "aspeed,ast2500-lpc", "simple-mfd"; reg =3D <0x1e789000 0x1000>; #address-cells =3D <1>; #size-cells =3D <1>; ranges =3D <0 0x1e789000 0x1000>; lpc_bmc: lpc-bmc@0 { compatible =3D "aspeed,ast2500-lpc-bmc"; reg =3D <0x0 0x80>; reg-io-width =3D <1>; }; lpc_host: lpc-host@80 { compatible =3D "aspeed,ast2500-lpc-host", "simple-mfd", "syscon"; reg =3D <0x80 0x1e0>; #address-cells =3D <1>; #size-cells =3D <1>; ranges =3D <0 0x80 0x1e0>; reg-io-width =3D <4>; ibt: ibt@c0 { compatible =3D "aspeed,ast2400-bt-bmc"; reg =3D <0xc0 0x18>; interrupts =3D <8>; }; }; }; It's a bit tedious, but as mentioned in the commit message we need a way to arbitrate access to other registers in the "host" portion of the LPC controller amongst other complications, and this layout gives us that capability without breaking any existing bindings. Andrew > A part from that : >=20 > > Reviewed-by: C=C3=A9dric Le Goater >=20 > Thanks, >=20 > C. >=20 > > --- > > =C2=A0drivers/char/ipmi/Kconfig=C2=A0=C2=A0|=C2=A0=C2=A01 + > > =C2=A0drivers/char/ipmi/bt-bmc.c | 82 +++++++++++++++++++++++++++++++++= +------------ > > =C2=A02 files changed, 62 insertions(+), 21 deletions(-) > >=20 > > 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 > > =C2=A0 > > =C2=A0config ASPEED_BT_IPMI_BMC > > > > =C2=A0 depends on ARCH_ASPEED > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0depends on REGMAP && R= EGMAP_MMIO && MFD_SYSCON > > > > =C2=A0 tristate "BT IPMI bmc driver" > > > > =C2=A0 help > > > > =C2=A0 =C2=A0=C2=A0Provides a driver for the BT (Block Transfer) IP= MI 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 @@ > > =C2=A0#include > > =C2=A0#include > > =C2=A0#include > > +#include > > =C2=A0#include > > =C2=A0#include > > +#include > > =C2=A0#include > > =C2=A0#include > > +#include > > =C2=A0#include > > =C2=A0#include > > =C2=A0 > > @@ -60,7 +63,8 @@ > > =C2=A0struct bt_bmc { > > > > > > =C2=A0 struct device dev; > > > > > > =C2=A0 struct miscdevice miscdev; > > > > > > - void __iomem *base; > > > > > > + struct regmap *map; > > > > > > + int offset; > > > > > > =C2=A0 int irq; > > > > > > =C2=A0 wait_queue_head_t queue; > > > > > > =C2=A0 struct timer_list poll_timer; > > @@ -69,14 +73,31 @@ struct bt_bmc { > > =C2=A0 > > =C2=A0static atomic_t open_count =3D ATOMIC_INIT(0); > > =C2=A0 > > +static struct regmap_config bt_regmap_cfg =3D { > > > > + .reg_bits =3D 32, > > > > + .val_bits =3D 32, > > > > + .reg_stride =3D 4, > > +}; > > + > > =C2=A0static u8 bt_inb(struct bt_bmc *bt_bmc, int reg) > > =C2=A0{ > > > > - return ioread8(bt_bmc->base + reg); > > > > + uint32_t val =3D 0; > > > > + int rc; > > + > > > > + rc =3D regmap_read(bt_bmc->map, bt_bmc->offset + reg, &val); > > > > + WARN(rc !=3D 0, "%s:%d: regmap_read() failed: %d\n", > > > > + __FILE__, __LINE__, rc); > > + > > > > + return rc =3D=3D 0 ? (u8) val : 0; > > =C2=A0} > > =C2=A0 > > =C2=A0static void bt_outb(struct bt_bmc *bt_bmc, u8 data, int reg) > > =C2=A0{ > > > > - iowrite8(data, bt_bmc->base + reg); > > > > + int rc; > > + > > > > + rc =3D regmap_write(bt_bmc->map, bt_bmc->offset + reg, data); > > > > + WARN(rc !=3D 0, "%s:%d: regmap_write() failed: %d\n", > > > > + __FILE__, __LINE__, rc); > > =C2=A0} > > =C2=A0 > > =C2=A0static void clr_rd_ptr(struct bt_bmc *bt_bmc) > > @@ -367,14 +388,18 @@ static irqreturn_t bt_bmc_irq(int irq, void *arg) > > =C2=A0{ > > > > =C2=A0 struct bt_bmc *bt_bmc =3D arg; > > > > =C2=A0 u32 reg; > > > > + int rc; > > + > > > > + rc =3D regmap_read(bt_bmc->map, bt_bmc->offset + BT_CR2, ®); > > > > + if (rc) > > > > + return IRQ_NONE; > > =C2=A0 > > > > - reg =3D ioread32(bt_bmc->base + BT_CR2); > > > > =C2=A0 reg &=3D BT_CR2_IRQ_H2B | BT_CR2_IRQ_HBUSY; > > > > =C2=A0 if (!reg) > > > > =C2=A0 return IRQ_NONE; > > =C2=A0 > > > > =C2=A0 /* ack pending IRQs */ > > > > - iowrite32(reg, bt_bmc->base + BT_CR2); > > > > + regmap_write(bt_bmc->map, bt_bmc->offset + BT_CR2, reg); > > =C2=A0 > > > > =C2=A0 wake_up(&bt_bmc->queue); > > > > =C2=A0 return IRQ_HANDLED; > > @@ -384,7 +409,6 @@ static int bt_bmc_config_irq(struct bt_bmc *bt_bmc, > > > > =C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct platform_device *pdev= ) > > =C2=A0{ > > > > =C2=A0 struct device *dev =3D &pdev->dev; > > > > - u32 reg; > > > > =C2=A0 int rc; > > =C2=A0 > > > > =C2=A0 bt_bmc->irq =3D platform_get_irq(pdev, 0); > > @@ -405,18 +429,17 @@ static int bt_bmc_config_irq(struct bt_bmc *bt_bm= c, > > > > =C2=A0 =C2=A0* will be cleared (along with B2H) when we can write t= he next > > > > =C2=A0 =C2=A0* message to the BT buffer > > > > =C2=A0 =C2=A0*/ > > > > - reg =3D ioread32(bt_bmc->base + BT_CR1); > > > > - reg |=3D BT_CR1_IRQ_H2B | BT_CR1_IRQ_HBUSY; > > > > - iowrite32(reg, bt_bmc->base + BT_CR1); > > > > + rc =3D 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)); > > =C2=A0 > > > > - return 0; > > > > + return rc; > > =C2=A0} > > =C2=A0 > > =C2=A0static int bt_bmc_probe(struct platform_device *pdev) > > =C2=A0{ > > > > =C2=A0 struct bt_bmc *bt_bmc; > > > > =C2=A0 struct device *dev; > > > > - struct resource *res; > > > > =C2=A0 int rc; > > =C2=A0 > > > > =C2=A0 if (!pdev || !pdev->dev.of_node) > > @@ -431,10 +454,27 @@ static int bt_bmc_probe(struct platform_device *p= dev) > > =C2=A0 > > > > =C2=A0 dev_set_drvdata(&pdev->dev, bt_bmc); > > =C2=A0 > > > > - res =3D platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > > - bt_bmc->base =3D devm_ioremap_resource(&pdev->dev, res); > > > > - if (IS_ERR(bt_bmc->base)) > > > > - return PTR_ERR(bt_bmc->base); > > > > + bt_bmc->map =3D syscon_node_to_regmap(pdev->dev.parent->of_node); > > > > + if (IS_ERR(bt_bmc->map)) { > > > > + struct resource *res; > > > > + void __iomem *base; > > + > > > > + /* > > > > + =C2=A0* Assume it's not the MFD-based devicetree description, in > > > > + =C2=A0* which case generate a regmap ourselves > > > > + =C2=A0*/ > > > > + res =3D platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > > + base =3D devm_ioremap_resource(&pdev->dev, res); > > > > + if (IS_ERR(base)) > > > > + return PTR_ERR(base); > > + > > > > + bt_bmc->map =3D devm_regmap_init_mmio(dev, base, &bt_regmap_cfg)= ; > > > > + bt_bmc->offset =3D 0; > > > > + } else { > > > > + rc =3D of_property_read_u32(dev->of_node, "reg", &bt_bmc->offset= ); > > > > + if (rc) > > > > + return rc; > > > > + } > > =C2=A0 > > > > =C2=A0 mutex_init(&bt_bmc->mutex); > > > > =C2=A0 init_waitqueue_head(&bt_bmc->queue); > > @@ -461,12 +501,12 @@ static int bt_bmc_probe(struct platform_device *p= dev) > > > > =C2=A0 add_timer(&bt_bmc->poll_timer); > > > > =C2=A0 } > > =C2=A0 > > > > - iowrite32((BT_IO_BASE << BT_CR0_IO_BASE) | > > > > - =C2=A0=C2=A0(BT_IRQ << BT_CR0_IRQ) | > > > > - =C2=A0=C2=A0BT_CR0_EN_CLR_SLV_RDP | > > > > - =C2=A0=C2=A0BT_CR0_EN_CLR_SLV_WRP | > > > > - =C2=A0=C2=A0BT_CR0_ENABLE_IBT, > > > > - =C2=A0=C2=A0bt_bmc->base + BT_CR0); > > > > + regmap_write(bt_bmc->map, bt_bmc->offset + BT_CR0, > > > > + =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0(BT_IO_BASE << BT_CR0_IO_BASE) | > > > > + =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0(BT_IRQ << BT_CR0_IRQ) | > > > > + =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0BT_CR0_EN_CLR_SLV_RDP | > > > > + =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0BT_CR0_EN_CLR_SLV_WRP | > > > > + =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0BT_CR0_ENABLE_IBT); > > =C2=A0 > > > > =C2=A0 clr_b_busy(bt_bmc); > > =C2=A0 > >=20 >=20 >=20 --=-WKkhe6UTknK0psrUYfuM Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- iQIcBAABCgAGBQJYR1IZAAoJEJ0dnzgO5LT5FEgP/R4DwNs/P67UZftn2wdYgzT+ zQ0/qxM9ajgTAcCmDIftp8bGzmk6vPSMtfXcWcqtQT0RmbM2UhHAJ9GUYV2nY8n0 rXOS8RvaWK2zNJmMRaFXGFAYz1D/GhsSWD+9/VVyeJ4DTDTTqE4YLJqSv7mTiAGH FG5F58aqUB3ltb8M8HLEyThlEwCy9x9jSHch9CFvBdXMJG2860EN3qWoVozUJmEO XpYd9vpkQKMIl1n7EdvWgCuodfwygkoLS0afiwFjfs5yVfVqpzhGNxGw8jPDdMNr yzPH7pHQkff6IQZi0Z/bfTwRI9qHbTw5GMLjlk1ZsJnn2jCUn8kgeJishiH0Q9Cm 4YyzZW6bvLKCgM4+s8fIr6eJmq+KmaXTgHC05ZwUquxuU3FTPbI8CMtI1gjd/3PE GgjmktecutW8J3YxbW0dgZjqZOPsHntvIvrPMFzPlGw/mPwWMVA8tur8HyZAwKEP VI361OjukVMBxDXUTgKtfjUdH+yvysrUmQTlhZEvXQdmTOWe8isweEyfsP2U2dEJ aW5qoK6cHMgmo31Vey8+NXuqZwBpG4E0KG0NfTFyRQpcfhzQRQsPH+haq9B/GgkB UZVngo3oqMtbJ7aHmxpcjYq5XYacyk734o/izOOkbh7IW2Lq3scBReacYP1SPcS/ t6VMqkqpu9AsOE5LOlL0 =eWQm -----END PGP SIGNATURE----- --=-WKkhe6UTknK0psrUYfuM--