| Message ID | 20210319062752.145730-1-andrew@aj.id.au |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,01/21] dt-bindings: aspeed-lpc: Remove LPC partitioning | expand |
On Fri, 19 Mar 2021 16:57:48 +1030, Andrew Jeffery wrote: > Given the deprecated binding, improve the ability to detect issues in > the platform devicetrees. Further, a subsequent patch will introduce a > new interrupts property for specifying SerIRQ behaviour, so convert > before we do any further additions. > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > --- > .../bindings/ipmi/aspeed,ast2400-kcs-bmc.yaml | 92 +++++++++++++++++++ > .../bindings/ipmi/aspeed-kcs-bmc.txt | 33 ------- > 2 files changed, 92 insertions(+), 33 deletions(-) > create mode 100644 Documentation/devicetree/bindings/ipmi/aspeed,ast2400-kcs-bmc.yaml > delete mode 100644 Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt > Reviewed-by: Rob Herring <robh@kernel.org>
On Fri, 19 Mar 2021 16:57:49 +1030, Andrew Jeffery wrote: > Allocating IO and IRQ resources to LPC devices is in-theory an operation > for the host, however ASPEED don't appear to expose this capability > outside the BMC (e.g. SuperIO). Instead, we are left with BMC-internal > registers for managing these resources, so introduce a devicetree > property for KCS devices to describe SerIRQ properties. > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > --- > .../bindings/ipmi/aspeed,ast2400-kcs-bmc.yaml | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > Reviewed-by: Rob Herring <robh@kernel.org>
On Fri, 19 Mar 2021 at 06:28, Andrew Jeffery <andrew@aj.id.au> wrote: > > From: "Chia-Wei, Wang" <chiawei_wang@aspeedtech.com> > > The LPC controller has no concept of the BMC and the Host partitions. > This patch fixes the documentation by removing the description on LPC > partitions. The register offsets illustrated in the DTS node examples > are also fixed to adapt to the LPC DTS change. Is this accurate: The node examples change their reg address to be an offset from the LPC HC to be an offset from the base of the LPC region. Reviewed-by: Joel Stanley <joel@jms.id.au> > > Signed-off-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com> > Reviewed-by: Andrew Jeffery <andrew@aj.id.au> > Acked-by: Rob Herring <robh@kernel.org> > Acked-by: Lee Jones <lee.jones@linaro.org> > --- > .../devicetree/bindings/mfd/aspeed-lpc.txt | 100 +++++------------- > 1 file changed, 25 insertions(+), 75 deletions(-) > > diff --git a/Documentation/devicetree/bindings/mfd/aspeed-lpc.txt b/Documentation/devicetree/bindings/mfd/aspeed-lpc.txt > index d0a38ba8b9ce..936aa108eab4 100644 > --- a/Documentation/devicetree/bindings/mfd/aspeed-lpc.txt > +++ b/Documentation/devicetree/bindings/mfd/aspeed-lpc.txt > @@ -9,13 +9,7 @@ primary use case of the Aspeed LPC controller is as a slave on the bus > conditions it can also take the role of bus master. > > The LPC controller is represented as a multi-function device to account for the > -mix of functionality it provides. The principle split is between the register > -layout at the start of the I/O space which is, to quote the Aspeed datasheet, > -"basically compatible with the [LPC registers from the] popular BMC controller > -H8S/2168[1]", and everything else, where everything else is an eclectic > -collection of functions with a esoteric register layout. "Everything else", > -here labeled the "host" portion of the controller, includes, but is not limited > -to: > +mix of functionality, which includes, but is not limited to: > > * An IPMI Block Transfer[2] Controller > > @@ -44,80 +38,36 @@ Required properties > =================== > > - compatible: One of: > - "aspeed,ast2400-lpc", "simple-mfd" > - "aspeed,ast2500-lpc", "simple-mfd" > - "aspeed,ast2600-lpc", "simple-mfd" > + "aspeed,ast2400-lpc-v2", "simple-mfd", "syscon" > + "aspeed,ast2500-lpc-v2", "simple-mfd", "syscon" > + "aspeed,ast2600-lpc-v2", "simple-mfd", "syscon" > > - reg: contains the physical address and length values of the Aspeed > LPC memory region. > > - #address-cells: <1> > - #size-cells: <1> > -- ranges: Maps 0 to the physical address and length of the LPC memory > - region > - > -Required LPC Child nodes > -======================== > - > -BMC Node > --------- > - > -- compatible: One of: > - "aspeed,ast2400-lpc-bmc" > - "aspeed,ast2500-lpc-bmc" > - "aspeed,ast2600-lpc-bmc" > - > -- reg: contains the physical address and length values of the > - H8S/2168-compatible LPC controller memory region > - > -Host Node > ---------- > - > -- compatible: One of: > - "aspeed,ast2400-lpc-host", "simple-mfd", "syscon" > - "aspeed,ast2500-lpc-host", "simple-mfd", "syscon" > - "aspeed,ast2600-lpc-host", "simple-mfd", "syscon" > - > -- reg: contains the address and length values of the host-related > - register space for the Aspeed LPC controller > - > -- #address-cells: <1> > -- #size-cells: <1> > -- ranges: Maps 0 to the address and length of the host-related LPC memory > +- ranges: Maps 0 to the physical address and length of the LPC memory > region > > Example: > > lpc: lpc@1e789000 { > - compatible = "aspeed,ast2500-lpc", "simple-mfd"; > + compatible = "aspeed,ast2500-lpc-v2", "simple-mfd", "syscon"; > reg = <0x1e789000 0x1000>; > > #address-cells = <1>; > #size-cells = <1>; > ranges = <0x0 0x1e789000 0x1000>; > > - lpc_bmc: lpc-bmc@0 { > - compatible = "aspeed,ast2500-lpc-bmc"; > + lpc_snoop: lpc-snoop@0 { > + compatible = "aspeed,ast2600-lpc-snoop"; > reg = <0x0 0x80>; > - }; > - > - lpc_host: lpc-host@80 { > - compatible = "aspeed,ast2500-lpc-host", "simple-mfd", "syscon"; > - reg = <0x80 0x1e0>; > - reg-io-width = <4>; > - > - #address-cells = <1>; > - #size-cells = <1>; > - ranges = <0x0 0x80 0x1e0>; > + interrupts = <GIC_SPI 144 IRQ_TYPE_LEVEL_HIGH>; > + snoop-ports = <0x80>; > }; > }; > > -BMC Node Children > -================== > - > - > -Host Node Children > -================== > > LPC Host Interface Controller > ------------------- > @@ -149,14 +99,12 @@ Optional properties: > > Example: > > -lpc-host@80 { > - lpc_ctrl: lpc-ctrl@0 { > - compatible = "aspeed,ast2500-lpc-ctrl"; > - reg = <0x0 0x80>; > - clocks = <&syscon ASPEED_CLK_GATE_LCLK>; > - memory-region = <&flash_memory>; > - flash = <&spi>; > - }; > +lpc_ctrl: lpc-ctrl@80 { > + compatible = "aspeed,ast2500-lpc-ctrl"; > + reg = <0x80 0x80>; > + clocks = <&syscon ASPEED_CLK_GATE_LCLK>; > + memory-region = <&flash_memory>; > + flash = <&spi>; > }; > > LPC Host Controller > @@ -179,9 +127,9 @@ Required properties: > > Example: > > -lhc: lhc@20 { > +lhc: lhc@a0 { > compatible = "aspeed,ast2500-lhc"; > - reg = <0x20 0x24 0x48 0x8>; > + reg = <0xa0 0x24 0xc8 0x8>; > }; > > LPC reset control > @@ -192,16 +140,18 @@ state of the LPC bus. Some systems may chose to modify this configuration. > > Required properties: > > - - compatible: "aspeed,ast2600-lpc-reset" or > - "aspeed,ast2500-lpc-reset" > - "aspeed,ast2400-lpc-reset" > + - compatible: One of: > + "aspeed,ast2600-lpc-reset"; > + "aspeed,ast2500-lpc-reset"; > + "aspeed,ast2400-lpc-reset"; > + > - reg: offset and length of the IP in the LHC memory region > - #reset-controller indicates the number of reset cells expected > > Example: > > -lpc_reset: reset-controller@18 { > +lpc_reset: reset-controller@98 { > compatible = "aspeed,ast2500-lpc-reset"; > - reg = <0x18 0x4>; > + reg = <0x98 0x4>; > #reset-cells = <1>; > }; > -- > 2.27.0 >
On Fri, 19 Mar 2021 at 06:28, Andrew Jeffery <andrew@aj.id.au> wrote: > > From: "Chia-Wei, Wang" <chiawei_wang@aspeedtech.com> > > Add check against LPC device v2 compatible string to > ensure that the fixed device tree layout is adopted. > The LPC register offsets are also fixed accordingly. > > Signed-off-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com> > Reviewed-by: Andrew Jeffery <andrew@aj.id.au> > Acked-by: Linus Walleij <linus.walleij@linaro.org> Reviewed-by: Joel Stanley <joel@jms.id.au>
On Fri, 19 Mar 2021 at 06:28, Andrew Jeffery <andrew@aj.id.au> wrote: > > From: "Chia-Wei, Wang" <chiawei_wang@aspeedtech.com> > > Add check against LPC device v2 compatible string to > ensure that the fixed device tree layout is adopted. > The LPC register offsets are also fixed accordingly. > > Signed-off-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com> > Reviewed-by: Andrew Jeffery <andrew@aj.id.au> Reviewed-by: Joel Stanley <joel@jms.id.au>
On Fri, Mar 19, 2021 at 01:27:42AM CDT, Andrew Jeffery wrote: >Strengthen the distinction between code that abstracts the >implementation of the KCS behaviours (device drivers) and code that >exploits KCS behaviours (clients). Neither needs to know about the APIs >required by the other, so provide separate headers. > >Signed-off-by: Andrew Jeffery <andrew@aj.id.au> >--- > drivers/char/ipmi/kcs_bmc.c | 21 ++++++++++----- > drivers/char/ipmi/kcs_bmc.h | 30 ++++++++++----------- > drivers/char/ipmi/kcs_bmc_aspeed.c | 20 +++++++++----- > drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 39 ++++++++++++++++++--------- > drivers/char/ipmi/kcs_bmc_client.h | 29 ++++++++++++++++++++ > drivers/char/ipmi/kcs_bmc_device.h | 19 +++++++++++++ > drivers/char/ipmi/kcs_bmc_npcm7xx.c | 20 +++++++++----- > 7 files changed, 129 insertions(+), 49 deletions(-) > create mode 100644 drivers/char/ipmi/kcs_bmc_client.h > create mode 100644 drivers/char/ipmi/kcs_bmc_device.h > >diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c >index 709b6bdec165..1046ce2bbefc 100644 >--- a/drivers/char/ipmi/kcs_bmc.c >+++ b/drivers/char/ipmi/kcs_bmc.c >@@ -1,46 +1,52 @@ > // SPDX-License-Identifier: GPL-2.0 > /* > * Copyright (c) 2015-2018, Intel Corporation. >+ * Copyright (c) 2021, IBM Corp. > */ > > #include <linux/module.h> > > #include "kcs_bmc.h" > >+/* Implement both the device and client interfaces here */ >+#include "kcs_bmc_device.h" >+#include "kcs_bmc_client.h" >+ >+/* Consumer data access */ >+ > u8 kcs_bmc_read_data(struct kcs_bmc *kcs_bmc) > { >- return kcs_bmc->io_inputb(kcs_bmc, kcs_bmc->ioreg.idr); >+ return kcs_bmc->ops->io_inputb(kcs_bmc, kcs_bmc->ioreg.idr); > } > EXPORT_SYMBOL(kcs_bmc_read_data); > > void kcs_bmc_write_data(struct kcs_bmc *kcs_bmc, u8 data) > { >- kcs_bmc->io_outputb(kcs_bmc, kcs_bmc->ioreg.odr, data); >+ kcs_bmc->ops->io_outputb(kcs_bmc, kcs_bmc->ioreg.odr, data); > } > EXPORT_SYMBOL(kcs_bmc_write_data); > > u8 kcs_bmc_read_status(struct kcs_bmc *kcs_bmc) > { >- return kcs_bmc->io_inputb(kcs_bmc, kcs_bmc->ioreg.str); >+ return kcs_bmc->ops->io_inputb(kcs_bmc, kcs_bmc->ioreg.str); > } > EXPORT_SYMBOL(kcs_bmc_read_status); > > void kcs_bmc_write_status(struct kcs_bmc *kcs_bmc, u8 data) > { >- kcs_bmc->io_outputb(kcs_bmc, kcs_bmc->ioreg.str, data); >+ kcs_bmc->ops->io_outputb(kcs_bmc, kcs_bmc->ioreg.str, data); > } > EXPORT_SYMBOL(kcs_bmc_write_status); > > void kcs_bmc_update_status(struct kcs_bmc *kcs_bmc, u8 mask, u8 val) > { >- kcs_bmc->io_updateb(kcs_bmc, kcs_bmc->ioreg.str, mask, val); >+ kcs_bmc->ops->io_updateb(kcs_bmc, kcs_bmc->ioreg.str, mask, val); > } > EXPORT_SYMBOL(kcs_bmc_update_status); > >-int kcs_bmc_ipmi_event(struct kcs_bmc *kcs_bmc); > int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc) > { >- return kcs_bmc_ipmi_event(kcs_bmc); >+ return kcs_bmc->client.ops->event(&kcs_bmc->client); > } > EXPORT_SYMBOL(kcs_bmc_handle_event); > >@@ -60,4 +66,5 @@ EXPORT_SYMBOL(kcs_bmc_remove_device); > > MODULE_LICENSE("GPL v2"); > MODULE_AUTHOR("Haiyue Wang <haiyue.wang@linux.intel.com>"); >+MODULE_AUTHOR("Andrew Jeffery <andrew@aj.id.au>"); > MODULE_DESCRIPTION("KCS BMC to handle the IPMI request from system software"); >diff --git a/drivers/char/ipmi/kcs_bmc.h b/drivers/char/ipmi/kcs_bmc.h >index bf0ae327997f..a1350e567723 100644 >--- a/drivers/char/ipmi/kcs_bmc.h >+++ b/drivers/char/ipmi/kcs_bmc.h >@@ -8,6 +8,15 @@ > > #include <linux/miscdevice.h> > >+#include "kcs_bmc_client.h" >+ >+#define KCS_BMC_EVENT_NONE 0 >+#define KCS_BMC_EVENT_HANDLED 1 Is there a particular reason we're introducing these macros and using an int return type for kcs_bmc_client_ops.event instead of just having it be irqreturn_t? Other event types or outcomes we're anticipating needing to handle maybe? >+ >+#define KCS_BMC_STR_OBF BIT(0) >+#define KCS_BMC_STR_IBF BIT(1) >+#define KCS_BMC_STR_CMD_DAT BIT(3) The first two of these macros are used later in the series, but the third doesn't end up used at all I think? >+ > /* Different phases of the KCS BMC module. > * KCS_PHASE_IDLE: > * BMC should not be expecting nor sending any data. >@@ -66,19 +75,21 @@ struct kcs_ioreg { > u32 str; > }; > >+struct kcs_bmc_device_ops; >+ > struct kcs_bmc { > struct device *dev; > >+ const struct kcs_bmc_device_ops *ops; >+ >+ struct kcs_bmc_client client; >+ > spinlock_t lock; > > u32 channel; > int running; > >- /* Setup by BMC KCS controller driver */ > struct kcs_ioreg ioreg; >- u8 (*io_inputb)(struct kcs_bmc *kcs_bmc, u32 reg); >- void (*io_outputb)(struct kcs_bmc *kcs_bmc, u32 reg, u8 b); >- void (*io_updateb)(struct kcs_bmc *kcs_bmc, u32 reg, u8 mask, u8 val); > > enum kcs_phases phase; > enum kcs_errors error; >@@ -97,15 +108,4 @@ struct kcs_bmc { > > struct miscdevice miscdev; > }; >- >-int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc); >-int kcs_bmc_add_device(struct kcs_bmc *kcs_bmc); >-int kcs_bmc_remove_device(struct kcs_bmc *kcs_bmc); >- >-u8 kcs_bmc_read_data(struct kcs_bmc *kcs_bmc); >-void kcs_bmc_write_data(struct kcs_bmc *kcs_bmc, u8 data); >-u8 kcs_bmc_read_status(struct kcs_bmc *kcs_bmc); >-void kcs_bmc_write_status(struct kcs_bmc *kcs_bmc, u8 data); >-void kcs_bmc_update_status(struct kcs_bmc *kcs_bmc, u8 mask, u8 val); >- > #endif /* __KCS_BMC_H__ */ >diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c b/drivers/char/ipmi/kcs_bmc_aspeed.c >index 0416ac78ce68..1b313355b1c8 100644 >--- a/drivers/char/ipmi/kcs_bmc_aspeed.c >+++ b/drivers/char/ipmi/kcs_bmc_aspeed.c >@@ -21,7 +21,7 @@ > #include <linux/slab.h> > #include <linux/timer.h> > >-#include "kcs_bmc.h" >+#include "kcs_bmc_device.h" > > > #define DEVICE_NAME "ast-kcs-bmc" >@@ -220,14 +220,22 @@ static void aspeed_kcs_enable_channel(struct kcs_bmc *kcs_bmc, bool enable) > } > } > >+static const struct kcs_bmc_device_ops aspeed_kcs_ops = { >+ .io_inputb = aspeed_kcs_inb, >+ .io_outputb = aspeed_kcs_outb, >+ .io_updateb = aspeed_kcs_updateb, >+}; >+ > static irqreturn_t aspeed_kcs_irq(int irq, void *arg) > { > struct kcs_bmc *kcs_bmc = arg; >+ int rc; > >- if (!kcs_bmc_handle_event(kcs_bmc)) >- return IRQ_HANDLED; >+ rc = kcs_bmc_handle_event(kcs_bmc); >+ if (rc < 0) >+ dev_warn(kcs_bmc->dev, "Failed to service irq: %d\n", rc); > >- return IRQ_NONE; >+ return rc == KCS_BMC_EVENT_HANDLED ? IRQ_HANDLED : IRQ_NONE; > } > > static int aspeed_kcs_config_irq(struct kcs_bmc *kcs_bmc, >@@ -362,9 +370,7 @@ static int aspeed_kcs_probe(struct platform_device *pdev) > kcs_bmc->dev = &pdev->dev; > kcs_bmc->channel = channel; > kcs_bmc->ioreg = ast_kcs_bmc_ioregs[channel - 1]; >- kcs_bmc->io_inputb = aspeed_kcs_inb; >- kcs_bmc->io_outputb = aspeed_kcs_outb; >- kcs_bmc->io_updateb = aspeed_kcs_updateb; >+ kcs_bmc->ops = &aspeed_kcs_ops; > > priv->map = syscon_node_to_regmap(pdev->dev.parent->of_node); > if (IS_ERR(priv->map)) { >diff --git a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c >index 0ca71c135a1a..fd852d8abe48 100644 >--- a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c >+++ b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c >@@ -22,7 +22,6 @@ > > #define KCS_ZERO_DATA 0 > >- > /* IPMI 2.0 - Table 9-1, KCS Interface Status Register Bits */ > #define KCS_STATUS_STATE(state) (state << 6) > #define KCS_STATUS_STATE_MASK GENMASK(7, 6) >@@ -179,12 +178,19 @@ static void kcs_bmc_ipmi_handle_cmd(struct kcs_bmc *kcs_bmc) > } > } > >-int kcs_bmc_ipmi_event(struct kcs_bmc *kcs_bmc); >-int kcs_bmc_ipmi_event(struct kcs_bmc *kcs_bmc) >+static inline struct kcs_bmc *client_to_kcs_bmc(struct kcs_bmc_client *client) > { >+ return container_of(client, struct kcs_bmc, client); >+} >+ >+static int kcs_bmc_ipmi_event(struct kcs_bmc_client *client) >+{ >+ struct kcs_bmc *kcs_bmc; > unsigned long flags; >- int ret = -ENODATA; > u8 status; >+ int ret; >+ >+ kcs_bmc = client_to_kcs_bmc(client); > > spin_lock_irqsave(&kcs_bmc->lock, flags); > >@@ -197,23 +203,28 @@ int kcs_bmc_ipmi_event(struct kcs_bmc *kcs_bmc) > else > kcs_bmc_ipmi_handle_data(kcs_bmc); > >- ret = 0; >+ ret = KCS_BMC_EVENT_HANDLED; >+ } else { >+ ret = KCS_BMC_EVENT_NONE; > } > > spin_unlock_irqrestore(&kcs_bmc->lock, flags); > > return ret; > } >-EXPORT_SYMBOL(kcs_bmc_ipmi_event); > >-static inline struct kcs_bmc *to_kcs_bmc(struct file *filp) >+static const struct kcs_bmc_client_ops kcs_bmc_ipmi_client_ops = { >+ .event = kcs_bmc_ipmi_event, >+}; >+ >+static inline struct kcs_bmc *file_to_kcs_bmc(struct file *filp) > { > return container_of(filp->private_data, struct kcs_bmc, miscdev); > } > > static int kcs_bmc_ipmi_open(struct inode *inode, struct file *filp) > { >- struct kcs_bmc *kcs_bmc = to_kcs_bmc(filp); >+ struct kcs_bmc *kcs_bmc = file_to_kcs_bmc(filp); > int ret = 0; > > spin_lock_irq(&kcs_bmc->lock); >@@ -228,7 +239,7 @@ static int kcs_bmc_ipmi_open(struct inode *inode, struct file *filp) > > static __poll_t kcs_bmc_ipmi_poll(struct file *filp, poll_table *wait) > { >- struct kcs_bmc *kcs_bmc = to_kcs_bmc(filp); >+ struct kcs_bmc *kcs_bmc = file_to_kcs_bmc(filp); > __poll_t mask = 0; > > poll_wait(filp, &kcs_bmc->queue, wait); >@@ -244,7 +255,7 @@ static __poll_t kcs_bmc_ipmi_poll(struct file *filp, poll_table *wait) > static ssize_t kcs_bmc_ipmi_read(struct file *filp, char __user *buf, > size_t count, loff_t *ppos) > { >- struct kcs_bmc *kcs_bmc = to_kcs_bmc(filp); >+ struct kcs_bmc *kcs_bmc = file_to_kcs_bmc(filp); > bool data_avail; > size_t data_len; > ssize_t ret; >@@ -306,7 +317,7 @@ static ssize_t kcs_bmc_ipmi_read(struct file *filp, char __user *buf, > static ssize_t kcs_bmc_ipmi_write(struct file *filp, const char __user *buf, > size_t count, loff_t *ppos) > { >- struct kcs_bmc *kcs_bmc = to_kcs_bmc(filp); >+ struct kcs_bmc *kcs_bmc = file_to_kcs_bmc(filp); > ssize_t ret; > > /* a minimum response size '3' : netfn + cmd + ccode */ >@@ -342,7 +353,7 @@ static ssize_t kcs_bmc_ipmi_write(struct file *filp, const char __user *buf, > static long kcs_bmc_ipmi_ioctl(struct file *filp, unsigned int cmd, > unsigned long arg) > { >- struct kcs_bmc *kcs_bmc = to_kcs_bmc(filp); >+ struct kcs_bmc *kcs_bmc = file_to_kcs_bmc(filp); > long ret = 0; > > spin_lock_irq(&kcs_bmc->lock); >@@ -372,7 +383,7 @@ static long kcs_bmc_ipmi_ioctl(struct file *filp, unsigned int cmd, > > static int kcs_bmc_ipmi_release(struct inode *inode, struct file *filp) > { >- struct kcs_bmc *kcs_bmc = to_kcs_bmc(filp); >+ struct kcs_bmc *kcs_bmc = file_to_kcs_bmc(filp); > > spin_lock_irq(&kcs_bmc->lock); > kcs_bmc->running = 0; >@@ -401,6 +412,8 @@ int kcs_bmc_ipmi_attach_cdev(struct kcs_bmc *kcs_bmc) > mutex_init(&kcs_bmc->mutex); > init_waitqueue_head(&kcs_bmc->queue); > >+ kcs_bmc->client.dev = kcs_bmc; >+ kcs_bmc->client.ops = &kcs_bmc_ipmi_client_ops; > kcs_bmc->data_in = devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ, GFP_KERNEL); > kcs_bmc->data_out = devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ, GFP_KERNEL); > kcs_bmc->kbuffer = devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ, GFP_KERNEL); >diff --git a/drivers/char/ipmi/kcs_bmc_client.h b/drivers/char/ipmi/kcs_bmc_client.h >new file mode 100644 >index 000000000000..140631d157d8 >--- /dev/null >+++ b/drivers/char/ipmi/kcs_bmc_client.h >@@ -0,0 +1,29 @@ >+/* SPDX-License-Identifier: GPL-2.0 */ >+/* Copyright (c) 2021, IBM Corp. */ >+ >+#ifndef __KCS_BMC_CONSUMER_H__ >+#define __KCS_BMC_CONSUMER_H__ >+ >+#include <linux/list.h> >+#include <linux/notifier.h> >+#include <stdbool.h> >+ >+struct kcs_bmc; >+struct kcs_bmc_client_ops; >+ >+struct kcs_bmc_client { >+ const struct kcs_bmc_client_ops *ops; >+ >+ struct kcs_bmc *dev; >+}; >+ >+struct kcs_bmc_client_ops { >+ int (*event)(struct kcs_bmc_client *client); >+}; >+ >+u8 kcs_bmc_read_data(struct kcs_bmc *kcs_bmc); >+void kcs_bmc_write_data(struct kcs_bmc *kcs_bmc, u8 data); >+u8 kcs_bmc_read_status(struct kcs_bmc *kcs_bmc); >+void kcs_bmc_write_status(struct kcs_bmc *kcs_bmc, u8 data); >+void kcs_bmc_update_status(struct kcs_bmc *kcs_bmc, u8 mask, u8 val); >+#endif >diff --git a/drivers/char/ipmi/kcs_bmc_device.h b/drivers/char/ipmi/kcs_bmc_device.h >new file mode 100644 >index 000000000000..33462174516d >--- /dev/null >+++ b/drivers/char/ipmi/kcs_bmc_device.h >@@ -0,0 +1,19 @@ >+/* SPDX-License-Identifier: GPL-2.0 */ >+/* Copyright (c) 2021, IBM Corp. */ >+ >+#ifndef __KCS_BMC_DEVICE_H__ >+#define __KCS_BMC_DEVICE_H__ >+ >+#include "kcs_bmc.h" >+ >+struct kcs_bmc_device_ops { >+ u8 (*io_inputb)(struct kcs_bmc *kcs_bmc, u32 reg); >+ void (*io_outputb)(struct kcs_bmc *kcs_bmc, u32 reg, u8 b); >+ void (*io_updateb)(struct kcs_bmc *kcs_bmc, u32 reg, u8 mask, u8 b); >+}; >+ >+int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc); >+int kcs_bmc_add_device(struct kcs_bmc *kcs_bmc); >+int kcs_bmc_remove_device(struct kcs_bmc *kcs_bmc); >+ >+#endif >diff --git a/drivers/char/ipmi/kcs_bmc_npcm7xx.c b/drivers/char/ipmi/kcs_bmc_npcm7xx.c >index 5d017498dc69..1d21697fc585 100644 >--- a/drivers/char/ipmi/kcs_bmc_npcm7xx.c >+++ b/drivers/char/ipmi/kcs_bmc_npcm7xx.c >@@ -17,7 +17,7 @@ > #include <linux/regmap.h> > #include <linux/slab.h> > >-#include "kcs_bmc.h" >+#include "kcs_bmc_device.h" > > #define DEVICE_NAME "npcm-kcs-bmc" > #define KCS_CHANNEL_MAX 3 >@@ -127,11 +127,13 @@ static void npcm7xx_kcs_enable_channel(struct kcs_bmc *kcs_bmc, bool enable) > static irqreturn_t npcm7xx_kcs_irq(int irq, void *arg) > { > struct kcs_bmc *kcs_bmc = arg; >+ int rc; > >- if (!kcs_bmc_handle_event(kcs_bmc)) >- return IRQ_HANDLED; >+ rc = kcs_bmc_handle_event(kcs_bmc); >+ if (rc < 0) >+ dev_warn(kcs_bmc->dev, "Failed to service irq: %d\n", rc); > >- return IRQ_NONE; >+ return rc == KCS_BMC_EVENT_HANDLED ? IRQ_HANDLED : IRQ_NONE; > } > > static int npcm7xx_kcs_config_irq(struct kcs_bmc *kcs_bmc, >@@ -148,6 +150,12 @@ static int npcm7xx_kcs_config_irq(struct kcs_bmc *kcs_bmc, > dev_name(dev), kcs_bmc); > } > >+static const struct kcs_bmc_device_ops npcm7xx_kcs_ops = { >+ .io_inputb = npcm7xx_kcs_inb, >+ .io_outputb = npcm7xx_kcs_outb, >+ .io_updateb = npcm7xx_kcs_updateb, >+}; >+ > static int npcm7xx_kcs_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; >@@ -179,9 +187,7 @@ static int npcm7xx_kcs_probe(struct platform_device *pdev) > kcs_bmc->ioreg.idr = priv->reg->dib; > kcs_bmc->ioreg.odr = priv->reg->dob; > kcs_bmc->ioreg.str = priv->reg->sts; >- kcs_bmc->io_inputb = npcm7xx_kcs_inb; >- kcs_bmc->io_outputb = npcm7xx_kcs_outb; >- kcs_bmc->io_updateb = npcm7xx_kcs_updateb; >+ kcs_bmc->ops = &npcm7xx_kcs_ops; > > platform_set_drvdata(pdev, priv); > >-- >2.27.0 >
On Fri, Mar 19, 2021 at 01:27:44AM CDT, Andrew Jeffery wrote: >Now that we have untangled the data-structures, split the userspace >interface out into its own module. Userspace interfaces and drivers are >registered to the KCS BMC core to support arbitrary binding of either. > >Signed-off-by: Andrew Jeffery <andrew@aj.id.au> >--- > drivers/char/ipmi/Kconfig | 13 +++++ > drivers/char/ipmi/Makefile | 3 +- > drivers/char/ipmi/kcs_bmc.c | 78 ++++++++++++++++++++++++++- > drivers/char/ipmi/kcs_bmc.h | 4 -- > drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 33 +++++++++--- > drivers/char/ipmi/kcs_bmc_client.h | 14 +++++ > 6 files changed, 132 insertions(+), 13 deletions(-) > >diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig >index 07847d9a459a..bc5f81899b62 100644 >--- a/drivers/char/ipmi/Kconfig >+++ b/drivers/char/ipmi/Kconfig >@@ -124,6 +124,19 @@ config NPCM7XX_KCS_IPMI_BMC > This support is also available as a module. If so, the module > will be called kcs_bmc_npcm7xx. > >+config IPMI_KCS_BMC_CDEV_IPMI >+ depends on IPMI_KCS_BMC >+ tristate "IPMI character device interface for BMC KCS devices" >+ help >+ Provides a BMC-side character device implementing IPMI >+ semantics for KCS IPMI devices. >+ >+ Say YES if you wish to expose KCS devices on the BMC for IPMI >+ purposes. >+ >+ This support is also available as a module. The module will be >+ called kcs_bmc_cdev_ipmi. >+ > config ASPEED_BT_IPMI_BMC > depends on ARCH_ASPEED || COMPILE_TEST > depends on REGMAP && REGMAP_MMIO && MFD_SYSCON >diff --git a/drivers/char/ipmi/Makefile b/drivers/char/ipmi/Makefile >index a302bc865370..fcfa676afddb 100644 >--- a/drivers/char/ipmi/Makefile >+++ b/drivers/char/ipmi/Makefile >@@ -22,7 +22,8 @@ obj-$(CONFIG_IPMI_SSIF) += ipmi_ssif.o > obj-$(CONFIG_IPMI_POWERNV) += ipmi_powernv.o > obj-$(CONFIG_IPMI_WATCHDOG) += ipmi_watchdog.o > obj-$(CONFIG_IPMI_POWEROFF) += ipmi_poweroff.o >-obj-$(CONFIG_IPMI_KCS_BMC) += kcs_bmc.o kcs_bmc_cdev_ipmi.o >+obj-$(CONFIG_IPMI_KCS_BMC) += kcs_bmc.o >+obj-$(CONFIG_IPMI_KCS_BMC_CDEV_IPMI) += kcs_bmc_cdev_ipmi.o > obj-$(CONFIG_ASPEED_BT_IPMI_BMC) += bt-bmc.o > obj-$(CONFIG_ASPEED_KCS_IPMI_BMC) += kcs_bmc_aspeed.o > obj-$(CONFIG_NPCM7XX_KCS_IPMI_BMC) += kcs_bmc_npcm7xx.o >diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c >index 266ebec71d6f..694db6ee2a92 100644 >--- a/drivers/char/ipmi/kcs_bmc.c >+++ b/drivers/char/ipmi/kcs_bmc.c >@@ -5,7 +5,9 @@ > */ > > #include <linux/device.h> >+#include <linux/list.h> > #include <linux/module.h> >+#include <linux/mutex.h> > > #include "kcs_bmc.h" > >@@ -13,6 +15,11 @@ > #include "kcs_bmc_device.h" > #include "kcs_bmc_client.h" > >+/* Record probed devices and cdevs */ >+static DEFINE_MUTEX(kcs_bmc_lock); >+static LIST_HEAD(kcs_bmc_devices); >+static LIST_HEAD(kcs_bmc_cdevs); >+ > /* Consumer data access */ > > u8 kcs_bmc_read_data(struct kcs_bmc_device *kcs_bmc) >@@ -100,16 +107,83 @@ EXPORT_SYMBOL(kcs_bmc_disable_device); > > int kcs_bmc_add_device(struct kcs_bmc_device *kcs_bmc) > { >- return kcs_bmc_ipmi_attach_cdev(kcs_bmc); >+ struct kcs_bmc_cdev *cdev; >+ int rc; >+ >+ spin_lock_init(&kcs_bmc->lock); >+ kcs_bmc->client = NULL; >+ >+ mutex_lock(&kcs_bmc_lock); >+ list_add(&kcs_bmc->entry, &kcs_bmc_devices); >+ list_for_each_entry(cdev, &kcs_bmc_cdevs, entry) { >+ rc = cdev->ops->add_device(kcs_bmc); >+ if (rc) >+ dev_err(kcs_bmc->dev, "Failed to add chardev for KCS channel %d: %d", >+ kcs_bmc->channel, rc); >+ } >+ mutex_unlock(&kcs_bmc_lock); >+ >+ return 0; We're ignoring failed ->add_device() calls here? > } > EXPORT_SYMBOL(kcs_bmc_add_device); > > int kcs_bmc_remove_device(struct kcs_bmc_device *kcs_bmc) > { >- return kcs_bmc_ipmi_detach_cdev(kcs_bmc); >+ struct kcs_bmc_cdev *cdev; >+ int rc; >+ >+ mutex_lock(&kcs_bmc_lock); >+ list_del(&kcs_bmc->entry); >+ list_for_each_entry(cdev, &kcs_bmc_cdevs, entry) { >+ rc = cdev->ops->remove_device(kcs_bmc); >+ if (rc) >+ dev_err(kcs_bmc->dev, "Failed to remove chardev for KCS channel %d: %d", >+ kcs_bmc->channel, rc); >+ } >+ mutex_unlock(&kcs_bmc_lock); >+ >+ return 0; Similarly with the return value here... > } > EXPORT_SYMBOL(kcs_bmc_remove_device); > >+int kcs_bmc_register_cdev(struct kcs_bmc_cdev *cdev) >+{ >+ struct kcs_bmc_device *kcs_bmc; >+ int rc; >+ >+ mutex_lock(&kcs_bmc_lock); >+ list_add(&cdev->entry, &kcs_bmc_cdevs); >+ list_for_each_entry(kcs_bmc, &kcs_bmc_devices, entry) { >+ rc = cdev->ops->add_device(kcs_bmc); >+ if (rc) >+ dev_err(kcs_bmc->dev, "Failed to add chardev for KCS channel %d: %d", >+ kcs_bmc->channel, rc); >+ } >+ mutex_unlock(&kcs_bmc_lock); >+ >+ return 0; ...return value again here... >+} >+EXPORT_SYMBOL(kcs_bmc_register_cdev); >+ >+int kcs_bmc_unregister_cdev(struct kcs_bmc_cdev *cdev) >+{ >+ struct kcs_bmc_device *kcs_bmc; >+ int rc; >+ >+ mutex_lock(&kcs_bmc_lock); >+ list_del(&cdev->entry); >+ list_for_each_entry(kcs_bmc, &kcs_bmc_devices, entry) { >+ rc = cdev->ops->remove_device(kcs_bmc); >+ if (rc) >+ dev_err(kcs_bmc->dev, "Failed to add chardev for KCS channel %d: %d", s/add/remove/ Might also want to differentiate the *_device() error messages from the *_cdev() ones a bit more? >+ kcs_bmc->channel, rc); >+ } >+ mutex_unlock(&kcs_bmc_lock); >+ >+ return rc; ...but this one is a bit incongruous, propagating the return value of only the last ->remove_device() call. (I'd have expected this to trigger a warning about returning a potentially uninitialized 'rc', but in some manual testing it doesn't seem to do so for me...not certain why.) >+} >+EXPORT_SYMBOL(kcs_bmc_unregister_cdev); >+ > MODULE_LICENSE("GPL v2"); > MODULE_AUTHOR("Haiyue Wang <haiyue.wang@linux.intel.com>"); > MODULE_AUTHOR("Andrew Jeffery <andrew@aj.id.au>"); >diff --git a/drivers/char/ipmi/kcs_bmc.h b/drivers/char/ipmi/kcs_bmc.h >index 3f266740c759..5deb9a0b8e60 100644 >--- a/drivers/char/ipmi/kcs_bmc.h >+++ b/drivers/char/ipmi/kcs_bmc.h >@@ -42,8 +42,4 @@ struct kcs_bmc_device { > spinlock_t lock; > struct kcs_bmc_client *client; > }; >- >-/* Temporary exports while refactoring */ >-int kcs_bmc_ipmi_attach_cdev(struct kcs_bmc_device *kcs_bmc); >-int kcs_bmc_ipmi_detach_cdev(struct kcs_bmc_device *kcs_bmc); > #endif /* __KCS_BMC_H__ */ >diff --git a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c >index 58c42e76483d..df83d67851ac 100644 >--- a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c >+++ b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c >@@ -469,8 +469,7 @@ static const struct file_operations kcs_bmc_ipmi_fops = { > static DEFINE_SPINLOCK(kcs_bmc_ipmi_instances_lock); > static LIST_HEAD(kcs_bmc_ipmi_instances); > >-int kcs_bmc_ipmi_attach_cdev(struct kcs_bmc_device *kcs_bmc); >-int kcs_bmc_ipmi_attach_cdev(struct kcs_bmc_device *kcs_bmc) >+static int kcs_bmc_ipmi_attach_cdev(struct kcs_bmc_device *kcs_bmc) > { > struct kcs_bmc_ipmi *priv; > int rc; >@@ -512,10 +511,8 @@ int kcs_bmc_ipmi_attach_cdev(struct kcs_bmc_device *kcs_bmc) > > return 0; > } >-EXPORT_SYMBOL(kcs_bmc_ipmi_attach_cdev); > >-int kcs_bmc_ipmi_detach_cdev(struct kcs_bmc_device *kcs_bmc); >-int kcs_bmc_ipmi_detach_cdev(struct kcs_bmc_device *kcs_bmc) >+static int kcs_bmc_ipmi_detach_cdev(struct kcs_bmc_device *kcs_bmc) > { > struct kcs_bmc_ipmi *priv = NULL, *pos; > >@@ -541,7 +538,31 @@ int kcs_bmc_ipmi_detach_cdev(struct kcs_bmc_device *kcs_bmc) > > return 0; > } >-EXPORT_SYMBOL(kcs_bmc_ipmi_detach_cdev); >+ >+static const struct kcs_bmc_cdev_ops kcs_bmc_ipmi_cdev_ops = { >+ .add_device = kcs_bmc_ipmi_attach_cdev, >+ .remove_device = kcs_bmc_ipmi_detach_cdev, >+}; >+ >+static struct kcs_bmc_cdev kcs_bmc_ipmi_cdev = { >+ .ops = &kcs_bmc_ipmi_cdev_ops, >+}; >+ >+static int kcs_bmc_ipmi_init(void) >+{ >+ return kcs_bmc_register_cdev(&kcs_bmc_ipmi_cdev); >+} >+module_init(kcs_bmc_ipmi_init); >+ >+static void kcs_bmc_ipmi_exit(void) >+{ >+ int rc; >+ >+ rc = kcs_bmc_unregister_cdev(&kcs_bmc_ipmi_cdev); >+ if (rc) >+ pr_warn("Failed to remove KCS BMC client: %d", rc); >+} >+module_exit(kcs_bmc_ipmi_exit); > > MODULE_LICENSE("GPL v2"); > MODULE_AUTHOR("Haiyue Wang <haiyue.wang@linux.intel.com>"); >diff --git a/drivers/char/ipmi/kcs_bmc_client.h b/drivers/char/ipmi/kcs_bmc_client.h >index 2dd710f4b4aa..d0a7404ff584 100644 >--- a/drivers/char/ipmi/kcs_bmc_client.h >+++ b/drivers/char/ipmi/kcs_bmc_client.h >@@ -10,6 +10,17 @@ > > #include "kcs_bmc.h" > >+struct kcs_bmc_cdev_ops { >+ int (*add_device)(struct kcs_bmc_device *kcs_bmc); >+ int (*remove_device)(struct kcs_bmc_device *kcs_bmc); >+}; >+ >+struct kcs_bmc_cdev { >+ struct list_head entry; >+ >+ const struct kcs_bmc_cdev_ops *ops; >+}; >+ > struct kcs_bmc_client_ops { > int (*event)(struct kcs_bmc_client *client); > }; >@@ -20,6 +31,9 @@ struct kcs_bmc_client { > struct kcs_bmc_device *dev; > }; > >+int kcs_bmc_register_cdev(struct kcs_bmc_cdev *cdev); >+int kcs_bmc_unregister_cdev(struct kcs_bmc_cdev *cdev); >+ > int kcs_bmc_enable_device(struct kcs_bmc_device *kcs_bmc, struct kcs_bmc_client *client); > void kcs_bmc_disable_device(struct kcs_bmc_device *kcs_bmc, struct kcs_bmc_client *client); > >-- >2.27.0 >
On Fri, Mar 19, 2021 at 01:27:46AM CDT, Andrew Jeffery wrote: >Soon it will be possible for one KCS device to have multiple associated >chardevs exposed to userspace (for IPMI and raw-style access). However, >don't prevent userspace from: > >1. Opening more than one chardev at a time, or >2. Opening the same chardev more than once. > >System behaviour is undefined for both classes of multiple access, so >userspace must manage itself accordingly. > >The implementation delivers IBF and OBF events to the first chardev >client to associate with the KCS device. An open on a related chardev >cannot associate its client with the KCS device and so will not >receive notification of events. However, any fd on any chardev may race >their accesses to the data and status registers. > >Signed-off-by: Andrew Jeffery <andrew@aj.id.au> >--- > drivers/char/ipmi/kcs_bmc.c | 34 ++++++++++------------------- > drivers/char/ipmi/kcs_bmc_aspeed.c | 3 +-- > drivers/char/ipmi/kcs_bmc_npcm7xx.c | 3 +-- > 3 files changed, 14 insertions(+), 26 deletions(-) > >diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c >index 05bbb72418b2..2fafa9541934 100644 >--- a/drivers/char/ipmi/kcs_bmc.c >+++ b/drivers/char/ipmi/kcs_bmc.c >@@ -55,24 +55,12 @@ EXPORT_SYMBOL(kcs_bmc_update_status); > int kcs_bmc_handle_event(struct kcs_bmc_device *kcs_bmc) > { > struct kcs_bmc_client *client; >- int rc; >+ int rc = KCS_BMC_EVENT_NONE; > > spin_lock(&kcs_bmc->lock); > client = kcs_bmc->client; >- if (client) { >+ if (!WARN_ON_ONCE(!client)) > rc = client->ops->event(client); The double-negation split by a macro seems a bit confusing to me readability-wise; could we simplify to something like if (client) rc = client->ops->event(client); else WARN_ONCE(); ? >- } else { >- u8 status; >- >- status = kcs_bmc_read_status(kcs_bmc); >- if (status & KCS_BMC_STR_IBF) { >- /* Ack the event by reading the data */ >- kcs_bmc_read_data(kcs_bmc); >- rc = KCS_BMC_EVENT_HANDLED; >- } else { >- rc = KCS_BMC_EVENT_NONE; >- } >- } > spin_unlock(&kcs_bmc->lock); > > return rc; >@@ -81,26 +69,28 @@ EXPORT_SYMBOL(kcs_bmc_handle_event); > > int kcs_bmc_enable_device(struct kcs_bmc_device *kcs_bmc, struct kcs_bmc_client *client) > { >- int rc; >- > spin_lock_irq(&kcs_bmc->lock); >- if (kcs_bmc->client) { >- rc = -EBUSY; >- } else { >+ if (!kcs_bmc->client) { >+ u8 mask = KCS_BMC_EVENT_TYPE_IBF; >+ > kcs_bmc->client = client; >- rc = 0; >+ kcs_bmc_update_event_mask(kcs_bmc, mask, mask); > } > spin_unlock_irq(&kcs_bmc->lock); > >- return rc; >+ return 0; Since this function appears to be infallible now, should it just return void? (Might be more churn than it's worth...shrug.) > } > EXPORT_SYMBOL(kcs_bmc_enable_device); > > void kcs_bmc_disable_device(struct kcs_bmc_device *kcs_bmc, struct kcs_bmc_client *client) > { > spin_lock_irq(&kcs_bmc->lock); >- if (client == kcs_bmc->client) >+ if (client == kcs_bmc->client) { >+ u8 mask = KCS_BMC_EVENT_TYPE_IBF | KCS_BMC_EVENT_TYPE_OBE; >+ >+ kcs_bmc_update_event_mask(kcs_bmc, mask, 0); > kcs_bmc->client = NULL; >+ } > spin_unlock_irq(&kcs_bmc->lock); > } > EXPORT_SYMBOL(kcs_bmc_disable_device); >diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c b/drivers/char/ipmi/kcs_bmc_aspeed.c >index 5f26471c038c..271845eb2e26 100644 >--- a/drivers/char/ipmi/kcs_bmc_aspeed.c >+++ b/drivers/char/ipmi/kcs_bmc_aspeed.c >@@ -419,8 +419,7 @@ static int aspeed_kcs_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, priv); > >- aspeed_kcs_irq_mask_update(kcs_bmc, (KCS_BMC_EVENT_TYPE_IBF | KCS_BMC_EVENT_TYPE_OBE), >- KCS_BMC_EVENT_TYPE_IBF); >+ aspeed_kcs_irq_mask_update(kcs_bmc, (KCS_BMC_EVENT_TYPE_IBF | KCS_BMC_EVENT_TYPE_OBE), 0); > aspeed_kcs_enable_channel(kcs_bmc, true); > > rc = kcs_bmc_add_device(&priv->kcs_bmc); >diff --git a/drivers/char/ipmi/kcs_bmc_npcm7xx.c b/drivers/char/ipmi/kcs_bmc_npcm7xx.c >index c2032728a03d..fdf35cad2eba 100644 >--- a/drivers/char/ipmi/kcs_bmc_npcm7xx.c >+++ b/drivers/char/ipmi/kcs_bmc_npcm7xx.c >@@ -207,8 +207,7 @@ static int npcm7xx_kcs_probe(struct platform_device *pdev) > if (rc) > return rc; > >- npcm7xx_kcs_irq_mask_update(kcs_bmc, (KCS_BMC_EVENT_TYPE_IBF | KCS_BMC_EVENT_TYPE_OBE), >- KCS_BMC_EVENT_TYPE_IBF); >+ npcm7xx_kcs_irq_mask_update(kcs_bmc, (KCS_BMC_EVENT_TYPE_IBF | KCS_BMC_EVENT_TYPE_OBE), 0); > npcm7xx_kcs_enable_channel(kcs_bmc, true); > > pr_info("channel=%u idr=0x%x odr=0x%x str=0x%x\n", >-- >2.27.0 >
On Fri, Mar 19, 2021 at 01:27:48AM CDT, Andrew Jeffery wrote: >Given the deprecated binding, improve the ability to detect issues in >the platform devicetrees. Further, a subsequent patch will introduce a >new interrupts property for specifying SerIRQ behaviour, so convert >before we do any further additions. > >Signed-off-by: Andrew Jeffery <andrew@aj.id.au> >--- > .../bindings/ipmi/aspeed,ast2400-kcs-bmc.yaml | 92 +++++++++++++++++++ > .../bindings/ipmi/aspeed-kcs-bmc.txt | 33 ------- > 2 files changed, 92 insertions(+), 33 deletions(-) > create mode 100644 Documentation/devicetree/bindings/ipmi/aspeed,ast2400-kcs-bmc.yaml > delete mode 100644 Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt > >diff --git a/Documentation/devicetree/bindings/ipmi/aspeed,ast2400-kcs-bmc.yaml b/Documentation/devicetree/bindings/ipmi/aspeed,ast2400-kcs-bmc.yaml >new file mode 100644 >index 000000000000..697ca575454f >--- /dev/null >+++ b/Documentation/devicetree/bindings/ipmi/aspeed,ast2400-kcs-bmc.yaml >@@ -0,0 +1,92 @@ >+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >+%YAML 1.2 >+--- >+$id: http://devicetree.org/schemas/ipmi/aspeed,ast2400-kcs-bmc.yaml >+$schema: http://devicetree.org/meta-schemas/core.yaml >+ >+title: ASPEED BMC KCS Devices >+ >+maintainers: >+ - Andrew Jeffery <andrew@aj.id.au> >+ >+description: | >+ The Aspeed BMC SoCs typically use the Keyboard-Controller-Style (KCS) >+ interfaces on the LPC bus for in-band IPMI communication with their host. >+ >+properties: >+ compatible: >+ oneOf: >+ - description: Channel ID derived from reg >+ items: >+ enum: >+ - aspeed,ast2400-kcs-bmc-v2 >+ - aspeed,ast2500-kcs-bmc-v2 >+ - aspeed,ast2600-kcs-bmc Should this have a "-v2" suffix? >+ >+ - description: Old-style with explicit channel ID, no reg >+ deprecated: true >+ items: >+ enum: >+ - aspeed,ast2400-kcs-bmc >+ - aspeed,ast2500-kcs-bmc >+ >+ interrupts: >+ maxItems: 1 >+ >+ reg: >+ # maxItems: 3 >+ items: >+ - description: IDR register >+ - description: ODR register >+ - description: STR register >+ >+ aspeed,lpc-io-reg: >+ $ref: '/schemas/types.yaml#/definitions/uint32-array' >+ minItems: 1 >+ maxItems: 2 >+ description: | >+ The host CPU LPC IO data and status addresses for the device. For most >+ channels the status address is derived from the data address, but the >+ status address may be optionally provided. >+ >+ kcs_chan: >+ deprecated: true >+ $ref: '/schemas/types.yaml#/definitions/uint32' >+ description: The LPC channel number in the controller >+ >+ kcs_addr: >+ deprecated: true >+ $ref: '/schemas/types.yaml#/definitions/uint32' >+ description: The host CPU IO map address >+ >+required: >+ - compatible >+ - interrupts >+ >+additionalProperties: false >+ >+allOf: >+ - if: >+ properties: >+ compatible: >+ contains: >+ enum: >+ - aspeed,ast2400-kcs-bmc >+ - aspeed,ast2500-kcs-bmc >+ then: >+ required: >+ - kcs_chan >+ - kcs_addr >+ else: >+ required: >+ - reg >+ - aspeed,lpc-io-reg >+ >+examples: >+ - | >+ kcs3: kcs@24 { >+ compatible = "aspeed,ast2600-kcs-bmc"; And likewise here. >+ reg = <0x24 0x1>, <0x30 0x1>, <0x3c 0x1>; >+ aspeed,lpc-io-reg = <0xca2>; >+ interrupts = <8>; >+ }; >diff --git a/Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt b/Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt >deleted file mode 100644 >index 193e71ca96b0..000000000000 >--- a/Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt >+++ /dev/null >@@ -1,33 +0,0 @@ >-# Aspeed KCS (Keyboard Controller Style) IPMI interface >- >-The Aspeed SOCs (AST2400 and AST2500) are commonly used as BMCs >-(Baseboard Management Controllers) and the KCS interface can be >-used to perform in-band IPMI communication with their host. >- >-## v1 >-Required properties: >-- compatible : should be one of >- "aspeed,ast2400-kcs-bmc" >- "aspeed,ast2500-kcs-bmc" >-- interrupts : interrupt generated by the controller >-- kcs_chan : The LPC channel number in the controller >-- kcs_addr : The host CPU IO map address >- >-## v2 >-Required properties: >-- compatible : should be one of >- "aspeed,ast2400-kcs-bmc-v2" >- "aspeed,ast2500-kcs-bmc-v2" >-- reg : The address and size of the IDR, ODR and STR registers >-- interrupts : interrupt generated by the controller >-- aspeed,lpc-io-reg : The host CPU LPC IO address for the device >- >-Example: >- >- kcs3: kcs@24 { >- compatible = "aspeed,ast2500-kcs-bmc-v2"; >- reg = <0x24 0x1>, <0x30 0x1>, <0x3c 0x1>; >- aspeed,lpc-reg = <0xca2>; >- interrupts = <8>; >- status = "okay"; >- }; >-- >2.27.0 >
On Fri, 9 Apr 2021, at 12:48, Joel Stanley wrote: > On Fri, 19 Mar 2021 at 06:28, Andrew Jeffery <andrew@aj.id.au> wrote: > > > > From: "Chia-Wei, Wang" <chiawei_wang@aspeedtech.com> > > > > The LPC controller has no concept of the BMC and the Host partitions. > > This patch fixes the documentation by removing the description on LPC > > partitions. The register offsets illustrated in the DTS node examples > > are also fixed to adapt to the LPC DTS change. > > Is this accurate: > > The node examples change their reg address to be an offset from the > LPC HC to be an offset from the base of the LPC region. Everything becomes based from the start of the LPC region, yes. Andrew
On Fri, Mar 19, 2021 at 01:27:38AM CDT, Andrew Jeffery wrote: >Enable more efficient implementation of read-modify-write sequences. >Both device drivers for the KCS BMC stack use regmaps. The new callback >allows us to exploit regmap_update_bits(). > >Signed-off-by: Andrew Jeffery <andrew@aj.id.au> Reviewed-by: Zev Weiss <zweiss@equinix.com>
On Fri, 9 Apr 2021, at 14:45, Zev Weiss wrote: > On Fri, Mar 19, 2021 at 01:27:48AM CDT, Andrew Jeffery wrote: > >Given the deprecated binding, improve the ability to detect issues in > >the platform devicetrees. Further, a subsequent patch will introduce a > >new interrupts property for specifying SerIRQ behaviour, so convert > >before we do any further additions. > > > >Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > >--- > > .../bindings/ipmi/aspeed,ast2400-kcs-bmc.yaml | 92 +++++++++++++++++++ > > .../bindings/ipmi/aspeed-kcs-bmc.txt | 33 ------- > > 2 files changed, 92 insertions(+), 33 deletions(-) > > create mode 100644 Documentation/devicetree/bindings/ipmi/aspeed,ast2400-kcs-bmc.yaml > > delete mode 100644 Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt > > > >diff --git a/Documentation/devicetree/bindings/ipmi/aspeed,ast2400-kcs-bmc.yaml b/Documentation/devicetree/bindings/ipmi/aspeed,ast2400-kcs-bmc.yaml > >new file mode 100644 > >index 000000000000..697ca575454f > >--- /dev/null > >+++ b/Documentation/devicetree/bindings/ipmi/aspeed,ast2400-kcs-bmc.yaml > >@@ -0,0 +1,92 @@ > >+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > >+%YAML 1.2 > >+--- > >+$id: http://devicetree.org/schemas/ipmi/aspeed,ast2400-kcs-bmc.yaml > >+$schema: http://devicetree.org/meta-schemas/core.yaml > >+ > >+title: ASPEED BMC KCS Devices > >+ > >+maintainers: > >+ - Andrew Jeffery <andrew@aj.id.au> > >+ > >+description: | > >+ The Aspeed BMC SoCs typically use the Keyboard-Controller-Style (KCS) > >+ interfaces on the LPC bus for in-band IPMI communication with their host. > >+ > >+properties: > >+ compatible: > >+ oneOf: > >+ - description: Channel ID derived from reg > >+ items: > >+ enum: > >+ - aspeed,ast2400-kcs-bmc-v2 > >+ - aspeed,ast2500-kcs-bmc-v2 > >+ - aspeed,ast2600-kcs-bmc > > Should this have a "-v2" suffix? Well, that was kind of a matter of perspective. The 2600 compatible was added after we'd done the v2 of the binding for the 2400 and 2500 so it never needed correcting. But it is a case of "don't use the deprecated properties with the 2600 compatible". I don't think a change is necessary? Cheers, Andrew
On Fri, Mar 19, 2021 at 01:27:51AM CDT, Andrew Jeffery wrote: >Input Buffer Full Interrupt Enable (IBFIE) is typoed as IBFIF for some >registers in the datasheet. Fix the driver to use the sensible acronym. > >Signed-off-by: Andrew Jeffery <andrew@aj.id.au> Reviewed-by: Zev Weiss <zweiss@equinix.com>
On Fri, Apr 09, 2021 at 12:33:10AM CDT, Andrew Jeffery wrote: > > >On Fri, 9 Apr 2021, at 14:45, Zev Weiss wrote: >> On Fri, Mar 19, 2021 at 01:27:48AM CDT, Andrew Jeffery wrote: >> >Given the deprecated binding, improve the ability to detect issues in >> >the platform devicetrees. Further, a subsequent patch will introduce a >> >new interrupts property for specifying SerIRQ behaviour, so convert >> >before we do any further additions. >> > >> >Signed-off-by: Andrew Jeffery <andrew@aj.id.au> >> >--- >> > .../bindings/ipmi/aspeed,ast2400-kcs-bmc.yaml | 92 +++++++++++++++++++ >> > .../bindings/ipmi/aspeed-kcs-bmc.txt | 33 ------- >> > 2 files changed, 92 insertions(+), 33 deletions(-) >> > create mode 100644 Documentation/devicetree/bindings/ipmi/aspeed,ast2400-kcs-bmc.yaml >> > delete mode 100644 Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt >> > >> >diff --git a/Documentation/devicetree/bindings/ipmi/aspeed,ast2400-kcs-bmc.yaml b/Documentation/devicetree/bindings/ipmi/aspeed,ast2400-kcs-bmc.yaml >> >new file mode 100644 >> >index 000000000000..697ca575454f >> >--- /dev/null >> >+++ b/Documentation/devicetree/bindings/ipmi/aspeed,ast2400-kcs-bmc.yaml >> >@@ -0,0 +1,92 @@ >> >+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> >+%YAML 1.2 >> >+--- >> >+$id: http://devicetree.org/schemas/ipmi/aspeed,ast2400-kcs-bmc.yaml >> >+$schema: http://devicetree.org/meta-schemas/core.yaml >> >+ >> >+title: ASPEED BMC KCS Devices >> >+ >> >+maintainers: >> >+ - Andrew Jeffery <andrew@aj.id.au> >> >+ >> >+description: | >> >+ The Aspeed BMC SoCs typically use the Keyboard-Controller-Style (KCS) >> >+ interfaces on the LPC bus for in-band IPMI communication with their host. >> >+ >> >+properties: >> >+ compatible: >> >+ oneOf: >> >+ - description: Channel ID derived from reg >> >+ items: >> >+ enum: >> >+ - aspeed,ast2400-kcs-bmc-v2 >> >+ - aspeed,ast2500-kcs-bmc-v2 >> >+ - aspeed,ast2600-kcs-bmc >> >> Should this have a "-v2" suffix? > >Well, that was kind of a matter of perspective. The 2600 compatible was >added after we'd done the v2 of the binding for the 2400 and 2500 so it >never needed correcting. But it is a case of "don't use the deprecated >properties with the 2600 compatible". > >I don't think a change is necessary? > It just looked inconsistent with the corresponding string in the ast_kcs_bmc_match[] table; perhaps that should be changed instead then? Zev
On Fri, 9 Apr 2021, at 13:31, Zev Weiss wrote: > On Fri, Mar 19, 2021 at 01:27:42AM CDT, Andrew Jeffery wrote: > >Strengthen the distinction between code that abstracts the > >implementation of the KCS behaviours (device drivers) and code that > >exploits KCS behaviours (clients). Neither needs to know about the APIs > >required by the other, so provide separate headers. > > > >Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > >--- > > drivers/char/ipmi/kcs_bmc.c | 21 ++++++++++----- > > drivers/char/ipmi/kcs_bmc.h | 30 ++++++++++----------- > > drivers/char/ipmi/kcs_bmc_aspeed.c | 20 +++++++++----- > > drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 39 ++++++++++++++++++--------- > > drivers/char/ipmi/kcs_bmc_client.h | 29 ++++++++++++++++++++ > > drivers/char/ipmi/kcs_bmc_device.h | 19 +++++++++++++ > > drivers/char/ipmi/kcs_bmc_npcm7xx.c | 20 +++++++++----- > > 7 files changed, 129 insertions(+), 49 deletions(-) > > create mode 100644 drivers/char/ipmi/kcs_bmc_client.h > > create mode 100644 drivers/char/ipmi/kcs_bmc_device.h > > > >diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c > >index 709b6bdec165..1046ce2bbefc 100644 > >--- a/drivers/char/ipmi/kcs_bmc.c > >+++ b/drivers/char/ipmi/kcs_bmc.c > >@@ -1,46 +1,52 @@ > > // SPDX-License-Identifier: GPL-2.0 > > /* > > * Copyright (c) 2015-2018, Intel Corporation. > >+ * Copyright (c) 2021, IBM Corp. > > */ > > > > #include <linux/module.h> > > > > #include "kcs_bmc.h" > > > >+/* Implement both the device and client interfaces here */ > >+#include "kcs_bmc_device.h" > >+#include "kcs_bmc_client.h" > >+ > >+/* Consumer data access */ > >+ > > u8 kcs_bmc_read_data(struct kcs_bmc *kcs_bmc) > > { > >- return kcs_bmc->io_inputb(kcs_bmc, kcs_bmc->ioreg.idr); > >+ return kcs_bmc->ops->io_inputb(kcs_bmc, kcs_bmc->ioreg.idr); > > } > > EXPORT_SYMBOL(kcs_bmc_read_data); > > > > void kcs_bmc_write_data(struct kcs_bmc *kcs_bmc, u8 data) > > { > >- kcs_bmc->io_outputb(kcs_bmc, kcs_bmc->ioreg.odr, data); > >+ kcs_bmc->ops->io_outputb(kcs_bmc, kcs_bmc->ioreg.odr, data); > > } > > EXPORT_SYMBOL(kcs_bmc_write_data); > > > > u8 kcs_bmc_read_status(struct kcs_bmc *kcs_bmc) > > { > >- return kcs_bmc->io_inputb(kcs_bmc, kcs_bmc->ioreg.str); > >+ return kcs_bmc->ops->io_inputb(kcs_bmc, kcs_bmc->ioreg.str); > > } > > EXPORT_SYMBOL(kcs_bmc_read_status); > > > > void kcs_bmc_write_status(struct kcs_bmc *kcs_bmc, u8 data) > > { > >- kcs_bmc->io_outputb(kcs_bmc, kcs_bmc->ioreg.str, data); > >+ kcs_bmc->ops->io_outputb(kcs_bmc, kcs_bmc->ioreg.str, data); > > } > > EXPORT_SYMBOL(kcs_bmc_write_status); > > > > void kcs_bmc_update_status(struct kcs_bmc *kcs_bmc, u8 mask, u8 val) > > { > >- kcs_bmc->io_updateb(kcs_bmc, kcs_bmc->ioreg.str, mask, val); > >+ kcs_bmc->ops->io_updateb(kcs_bmc, kcs_bmc->ioreg.str, mask, val); > > } > > EXPORT_SYMBOL(kcs_bmc_update_status); > > > >-int kcs_bmc_ipmi_event(struct kcs_bmc *kcs_bmc); > > int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc) > > { > >- return kcs_bmc_ipmi_event(kcs_bmc); > >+ return kcs_bmc->client.ops->event(&kcs_bmc->client); > > } > > EXPORT_SYMBOL(kcs_bmc_handle_event); > > > >@@ -60,4 +66,5 @@ EXPORT_SYMBOL(kcs_bmc_remove_device); > > > > MODULE_LICENSE("GPL v2"); > > MODULE_AUTHOR("Haiyue Wang <haiyue.wang@linux.intel.com>"); > >+MODULE_AUTHOR("Andrew Jeffery <andrew@aj.id.au>"); > > MODULE_DESCRIPTION("KCS BMC to handle the IPMI request from system software"); > >diff --git a/drivers/char/ipmi/kcs_bmc.h b/drivers/char/ipmi/kcs_bmc.h > >index bf0ae327997f..a1350e567723 100644 > >--- a/drivers/char/ipmi/kcs_bmc.h > >+++ b/drivers/char/ipmi/kcs_bmc.h > >@@ -8,6 +8,15 @@ > > > > #include <linux/miscdevice.h> > > > >+#include "kcs_bmc_client.h" > >+ > >+#define KCS_BMC_EVENT_NONE 0 > >+#define KCS_BMC_EVENT_HANDLED 1 > > Is there a particular reason we're introducing these macros and using an > int return type for kcs_bmc_client_ops.event instead of just having it > be irqreturn_t? Other event types or outcomes we're anticipating needing > to handle maybe? In earlier iterations of the patches I was doing some extra work in the event handling path and felt it was useful at the time. However I've refactored things a little and this may have outlived its usefulness. I'll reasses! > > >+ > >+#define KCS_BMC_STR_OBF BIT(0) > >+#define KCS_BMC_STR_IBF BIT(1) > >+#define KCS_BMC_STR_CMD_DAT BIT(3) > > The first two of these macros are used later in the series, but the third > doesn't end up used at all I think? I think I just added it as documentation as the hardware-defined bits aren't contiguous. Andrew
On Fri, 9 Apr 2021, at 14:05, Zev Weiss wrote: > On Fri, Mar 19, 2021 at 01:27:44AM CDT, Andrew Jeffery wrote: > >Now that we have untangled the data-structures, split the userspace > >interface out into its own module. Userspace interfaces and drivers are > >registered to the KCS BMC core to support arbitrary binding of either. > > > >Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > >--- > > drivers/char/ipmi/Kconfig | 13 +++++ > > drivers/char/ipmi/Makefile | 3 +- > > drivers/char/ipmi/kcs_bmc.c | 78 ++++++++++++++++++++++++++- > > drivers/char/ipmi/kcs_bmc.h | 4 -- > > drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 33 +++++++++--- > > drivers/char/ipmi/kcs_bmc_client.h | 14 +++++ > > 6 files changed, 132 insertions(+), 13 deletions(-) > > > >diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig > >index 07847d9a459a..bc5f81899b62 100644 > >--- a/drivers/char/ipmi/Kconfig > >+++ b/drivers/char/ipmi/Kconfig > >@@ -124,6 +124,19 @@ config NPCM7XX_KCS_IPMI_BMC > > This support is also available as a module. If so, the module > > will be called kcs_bmc_npcm7xx. > > > >+config IPMI_KCS_BMC_CDEV_IPMI > >+ depends on IPMI_KCS_BMC > >+ tristate "IPMI character device interface for BMC KCS devices" > >+ help > >+ Provides a BMC-side character device implementing IPMI > >+ semantics for KCS IPMI devices. > >+ > >+ Say YES if you wish to expose KCS devices on the BMC for IPMI > >+ purposes. > >+ > >+ This support is also available as a module. The module will be > >+ called kcs_bmc_cdev_ipmi. > >+ > > config ASPEED_BT_IPMI_BMC > > depends on ARCH_ASPEED || COMPILE_TEST > > depends on REGMAP && REGMAP_MMIO && MFD_SYSCON > >diff --git a/drivers/char/ipmi/Makefile b/drivers/char/ipmi/Makefile > >index a302bc865370..fcfa676afddb 100644 > >--- a/drivers/char/ipmi/Makefile > >+++ b/drivers/char/ipmi/Makefile > >@@ -22,7 +22,8 @@ obj-$(CONFIG_IPMI_SSIF) += ipmi_ssif.o > > obj-$(CONFIG_IPMI_POWERNV) += ipmi_powernv.o > > obj-$(CONFIG_IPMI_WATCHDOG) += ipmi_watchdog.o > > obj-$(CONFIG_IPMI_POWEROFF) += ipmi_poweroff.o > >-obj-$(CONFIG_IPMI_KCS_BMC) += kcs_bmc.o kcs_bmc_cdev_ipmi.o > >+obj-$(CONFIG_IPMI_KCS_BMC) += kcs_bmc.o > >+obj-$(CONFIG_IPMI_KCS_BMC_CDEV_IPMI) += kcs_bmc_cdev_ipmi.o > > obj-$(CONFIG_ASPEED_BT_IPMI_BMC) += bt-bmc.o > > obj-$(CONFIG_ASPEED_KCS_IPMI_BMC) += kcs_bmc_aspeed.o > > obj-$(CONFIG_NPCM7XX_KCS_IPMI_BMC) += kcs_bmc_npcm7xx.o > >diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c > >index 266ebec71d6f..694db6ee2a92 100644 > >--- a/drivers/char/ipmi/kcs_bmc.c > >+++ b/drivers/char/ipmi/kcs_bmc.c > >@@ -5,7 +5,9 @@ > > */ > > > > #include <linux/device.h> > >+#include <linux/list.h> > > #include <linux/module.h> > >+#include <linux/mutex.h> > > > > #include "kcs_bmc.h" > > > >@@ -13,6 +15,11 @@ > > #include "kcs_bmc_device.h" > > #include "kcs_bmc_client.h" > > > >+/* Record probed devices and cdevs */ > >+static DEFINE_MUTEX(kcs_bmc_lock); > >+static LIST_HEAD(kcs_bmc_devices); > >+static LIST_HEAD(kcs_bmc_cdevs); > >+ > > /* Consumer data access */ > > > > u8 kcs_bmc_read_data(struct kcs_bmc_device *kcs_bmc) > >@@ -100,16 +107,83 @@ EXPORT_SYMBOL(kcs_bmc_disable_device); > > > > int kcs_bmc_add_device(struct kcs_bmc_device *kcs_bmc) > > { > >- return kcs_bmc_ipmi_attach_cdev(kcs_bmc); > >+ struct kcs_bmc_cdev *cdev; > >+ int rc; > >+ > >+ spin_lock_init(&kcs_bmc->lock); > >+ kcs_bmc->client = NULL; > >+ > >+ mutex_lock(&kcs_bmc_lock); > >+ list_add(&kcs_bmc->entry, &kcs_bmc_devices); > >+ list_for_each_entry(cdev, &kcs_bmc_cdevs, entry) { > >+ rc = cdev->ops->add_device(kcs_bmc); > >+ if (rc) > >+ dev_err(kcs_bmc->dev, "Failed to add chardev for KCS channel %d: %d", > >+ kcs_bmc->channel, rc); > >+ } > >+ mutex_unlock(&kcs_bmc_lock); > >+ > >+ return 0; > > We're ignoring failed ->add_device() calls here? Yep. If one chardev module is failing to accept new devices we don't want to not add them to the remaining chardev modules. What would the caller do with a error return value? Maybe it should just be void. > > > } > > EXPORT_SYMBOL(kcs_bmc_add_device); > > > > int kcs_bmc_remove_device(struct kcs_bmc_device *kcs_bmc) > > { > >- return kcs_bmc_ipmi_detach_cdev(kcs_bmc); > >+ struct kcs_bmc_cdev *cdev; > >+ int rc; > >+ > >+ mutex_lock(&kcs_bmc_lock); > >+ list_del(&kcs_bmc->entry); > >+ list_for_each_entry(cdev, &kcs_bmc_cdevs, entry) { > >+ rc = cdev->ops->remove_device(kcs_bmc); > >+ if (rc) > >+ dev_err(kcs_bmc->dev, "Failed to remove chardev for KCS channel %d: %d", > >+ kcs_bmc->channel, rc); > >+ } > >+ mutex_unlock(&kcs_bmc_lock); > >+ > >+ return 0; > > Similarly with the return value here... As above. > > > } > > EXPORT_SYMBOL(kcs_bmc_remove_device); > > > >+int kcs_bmc_register_cdev(struct kcs_bmc_cdev *cdev) > >+{ > >+ struct kcs_bmc_device *kcs_bmc; > >+ int rc; > >+ > >+ mutex_lock(&kcs_bmc_lock); > >+ list_add(&cdev->entry, &kcs_bmc_cdevs); > >+ list_for_each_entry(kcs_bmc, &kcs_bmc_devices, entry) { > >+ rc = cdev->ops->add_device(kcs_bmc); > >+ if (rc) > >+ dev_err(kcs_bmc->dev, "Failed to add chardev for KCS channel %d: %d", > >+ kcs_bmc->channel, rc); > >+ } > >+ mutex_unlock(&kcs_bmc_lock); > >+ > >+ return 0; > > ...return value again here... As above. > > >+} > >+EXPORT_SYMBOL(kcs_bmc_register_cdev); > >+ > >+int kcs_bmc_unregister_cdev(struct kcs_bmc_cdev *cdev) > >+{ > >+ struct kcs_bmc_device *kcs_bmc; > >+ int rc; > >+ > >+ mutex_lock(&kcs_bmc_lock); > >+ list_del(&cdev->entry); > >+ list_for_each_entry(kcs_bmc, &kcs_bmc_devices, entry) { > >+ rc = cdev->ops->remove_device(kcs_bmc); > >+ if (rc) > >+ dev_err(kcs_bmc->dev, "Failed to add chardev for KCS channel %d: %d", > > s/add/remove/ Thanks. > > Might also want to differentiate the *_device() error messages from the > *_cdev() ones a bit more? I'll look into it. > > >+ kcs_bmc->channel, rc); > >+ } > >+ mutex_unlock(&kcs_bmc_lock); > >+ > >+ return rc; > > ...but this one is a bit incongruous, propagating the return value of > only the last ->remove_device() call. Hah. good catch. Andrew
On Fri, Apr 09, 2021 at 12:44:04AM CDT, Zev Weiss wrote: >On Fri, Apr 09, 2021 at 12:33:10AM CDT, Andrew Jeffery wrote: >> >> >>On Fri, 9 Apr 2021, at 14:45, Zev Weiss wrote: >>>On Fri, Mar 19, 2021 at 01:27:48AM CDT, Andrew Jeffery wrote: >>>>Given the deprecated binding, improve the ability to detect issues in >>>>the platform devicetrees. Further, a subsequent patch will introduce a >>>>new interrupts property for specifying SerIRQ behaviour, so convert >>>>before we do any further additions. >>>> >>>>Signed-off-by: Andrew Jeffery <andrew@aj.id.au> >>>>--- >>>> .../bindings/ipmi/aspeed,ast2400-kcs-bmc.yaml | 92 +++++++++++++++++++ >>>> .../bindings/ipmi/aspeed-kcs-bmc.txt | 33 ------- >>>> 2 files changed, 92 insertions(+), 33 deletions(-) >>>> create mode 100644 Documentation/devicetree/bindings/ipmi/aspeed,ast2400-kcs-bmc.yaml >>>> delete mode 100644 Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt >>>> >>>>diff --git a/Documentation/devicetree/bindings/ipmi/aspeed,ast2400-kcs-bmc.yaml b/Documentation/devicetree/bindings/ipmi/aspeed,ast2400-kcs-bmc.yaml >>>>new file mode 100644 >>>>index 000000000000..697ca575454f >>>>--- /dev/null >>>>+++ b/Documentation/devicetree/bindings/ipmi/aspeed,ast2400-kcs-bmc.yaml >>>>@@ -0,0 +1,92 @@ >>>>+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>>>+%YAML 1.2 >>>>+--- >>>>+$id: http://devicetree.org/schemas/ipmi/aspeed,ast2400-kcs-bmc.yaml >>>>+$schema: http://devicetree.org/meta-schemas/core.yaml >>>>+ >>>>+title: ASPEED BMC KCS Devices >>>>+ >>>>+maintainers: >>>>+ - Andrew Jeffery <andrew@aj.id.au> >>>>+ >>>>+description: | >>>>+ The Aspeed BMC SoCs typically use the Keyboard-Controller-Style (KCS) >>>>+ interfaces on the LPC bus for in-band IPMI communication with their host. >>>>+ >>>>+properties: >>>>+ compatible: >>>>+ oneOf: >>>>+ - description: Channel ID derived from reg >>>>+ items: >>>>+ enum: >>>>+ - aspeed,ast2400-kcs-bmc-v2 >>>>+ - aspeed,ast2500-kcs-bmc-v2 >>>>+ - aspeed,ast2600-kcs-bmc >>> >>>Should this have a "-v2" suffix? >> >>Well, that was kind of a matter of perspective. The 2600 compatible was >>added after we'd done the v2 of the binding for the 2400 and 2500 so it >>never needed correcting. But it is a case of "don't use the deprecated >>properties with the 2600 compatible". >> >>I don't think a change is necessary? >> > >It just looked inconsistent with the corresponding string in the >ast_kcs_bmc_match[] table; perhaps that should be changed instead then? > ...except I realize now I only saw the 2600 v2 string in the match table because I put it there myself in the process of resolving a conflict when applying your series to the openbmc dev-5.10 branch for testing purposes. So nevermind on this. Reviewed-by: Zev Weiss <zweiss@equinix.com>
diff --git a/Documentation/devicetree/bindings/mfd/aspeed-lpc.txt b/Documentation/devicetree/bindings/mfd/aspeed-lpc.txt index d0a38ba8b9ce..936aa108eab4 100644 --- a/Documentation/devicetree/bindings/mfd/aspeed-lpc.txt +++ b/Documentation/devicetree/bindings/mfd/aspeed-lpc.txt @@ -9,13 +9,7 @@ primary use case of the Aspeed LPC controller is as a slave on the bus conditions it can also take the role of bus master. The LPC controller is represented as a multi-function device to account for the -mix of functionality it provides. The principle split is between the register -layout at the start of the I/O space which is, to quote the Aspeed datasheet, -"basically compatible with the [LPC registers from the] popular BMC controller -H8S/2168[1]", and everything else, where everything else is an eclectic -collection of functions with a esoteric register layout. "Everything else", -here labeled the "host" portion of the controller, includes, but is not limited -to: +mix of functionality, which includes, but is not limited to: * An IPMI Block Transfer[2] Controller @@ -44,80 +38,36 @@ Required properties =================== - compatible: One of: - "aspeed,ast2400-lpc", "simple-mfd" - "aspeed,ast2500-lpc", "simple-mfd" - "aspeed,ast2600-lpc", "simple-mfd" + "aspeed,ast2400-lpc-v2", "simple-mfd", "syscon" + "aspeed,ast2500-lpc-v2", "simple-mfd", "syscon" + "aspeed,ast2600-lpc-v2", "simple-mfd", "syscon" - reg: contains the physical address and length values of the Aspeed LPC memory region. - #address-cells: <1> - #size-cells: <1> -- ranges: Maps 0 to the physical address and length of the LPC memory - region - -Required LPC Child nodes -======================== - -BMC Node --------- - -- compatible: One of: - "aspeed,ast2400-lpc-bmc" - "aspeed,ast2500-lpc-bmc" - "aspeed,ast2600-lpc-bmc" - -- reg: contains the physical address and length values of the - H8S/2168-compatible LPC controller memory region - -Host Node ---------- - -- compatible: One of: - "aspeed,ast2400-lpc-host", "simple-mfd", "syscon" - "aspeed,ast2500-lpc-host", "simple-mfd", "syscon" - "aspeed,ast2600-lpc-host", "simple-mfd", "syscon" - -- reg: contains the address and length values of the host-related - register space for the Aspeed LPC controller - -- #address-cells: <1> -- #size-cells: <1> -- ranges: Maps 0 to the address and length of the host-related LPC memory +- ranges: Maps 0 to the physical address and length of the LPC memory region Example: lpc: lpc@1e789000 { - compatible = "aspeed,ast2500-lpc", "simple-mfd"; + compatible = "aspeed,ast2500-lpc-v2", "simple-mfd", "syscon"; reg = <0x1e789000 0x1000>; #address-cells = <1>; #size-cells = <1>; ranges = <0x0 0x1e789000 0x1000>; - lpc_bmc: lpc-bmc@0 { - compatible = "aspeed,ast2500-lpc-bmc"; + lpc_snoop: lpc-snoop@0 { + compatible = "aspeed,ast2600-lpc-snoop"; reg = <0x0 0x80>; - }; - - lpc_host: lpc-host@80 { - compatible = "aspeed,ast2500-lpc-host", "simple-mfd", "syscon"; - reg = <0x80 0x1e0>; - reg-io-width = <4>; - - #address-cells = <1>; - #size-cells = <1>; - ranges = <0x0 0x80 0x1e0>; + interrupts = <GIC_SPI 144 IRQ_TYPE_LEVEL_HIGH>; + snoop-ports = <0x80>; }; }; -BMC Node Children -================== - - -Host Node Children -================== LPC Host Interface Controller ------------------- @@ -149,14 +99,12 @@ Optional properties: Example: -lpc-host@80 { - lpc_ctrl: lpc-ctrl@0 { - compatible = "aspeed,ast2500-lpc-ctrl"; - reg = <0x0 0x80>; - clocks = <&syscon ASPEED_CLK_GATE_LCLK>; - memory-region = <&flash_memory>; - flash = <&spi>; - }; +lpc_ctrl: lpc-ctrl@80 { + compatible = "aspeed,ast2500-lpc-ctrl"; + reg = <0x80 0x80>; + clocks = <&syscon ASPEED_CLK_GATE_LCLK>; + memory-region = <&flash_memory>; + flash = <&spi>; }; LPC Host Controller @@ -179,9 +127,9 @@ Required properties: Example: -lhc: lhc@20 { +lhc: lhc@a0 { compatible = "aspeed,ast2500-lhc"; - reg = <0x20 0x24 0x48 0x8>; + reg = <0xa0 0x24 0xc8 0x8>; }; LPC reset control @@ -192,16 +140,18 @@ state of the LPC bus. Some systems may chose to modify this configuration. Required properties: - - compatible: "aspeed,ast2600-lpc-reset" or - "aspeed,ast2500-lpc-reset" - "aspeed,ast2400-lpc-reset" + - compatible: One of: + "aspeed,ast2600-lpc-reset"; + "aspeed,ast2500-lpc-reset"; + "aspeed,ast2400-lpc-reset"; + - reg: offset and length of the IP in the LHC memory region - #reset-controller indicates the number of reset cells expected Example: -lpc_reset: reset-controller@18 { +lpc_reset: reset-controller@98 { compatible = "aspeed,ast2500-lpc-reset"; - reg = <0x18 0x4>; + reg = <0x98 0x4>; #reset-cells = <1>; };