Re: [Openipmi-developer] [PATCH v2 10/21] ipmi: kcs_bmc: Turn the driver data-structures inside-out
On Fri, 9 Apr 2021, at 13:27, Zev Weiss wrote:
> On Fri, Mar 19, 2021 at 01:27:41AM CDT, Andrew Jeffery wrote:
> >Make the KCS device drivers responsible for allocating their own memory.
> >
> >Until now the private data for the device driver was allocated internal
> >to the private data for the chardev interface. This coupling required
> >the slightly awkward API of passing through the struct size for the
> >driver private data to the chardev constructor, and then retrieving a
> >pointer to the driver private data from the allocated chardev memory.
> >
> >In addition to being awkward, the arrangement prevents the
> >implementation of alternative userspace interfaces as the device driver
> >private data is not independent.
> >
> >Peel a layer off the onion and turn the data-structures inside out by
> >exploiting container_of() and embedding `struct kcs_device` in the
> >driver private data.
> >
> >Signed-off-by: Andrew Jeffery
> >---
> > drivers/char/ipmi/kcs_bmc.c | 15 +--
> > drivers/char/ipmi/kcs_bmc.h | 12 ++
> > drivers/char/ipmi/kcs_bmc_aspeed.c| 60 ---
> > drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 60 ++-
> > drivers/char/ipmi/kcs_bmc_npcm7xx.c | 37 ++---
> > 5 files changed, 113 insertions(+), 71 deletions(-)
> >
> >diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
> >index ef5c48ffe74a..709b6bdec165 100644
> >--- a/drivers/char/ipmi/kcs_bmc.c
> >+++ b/drivers/char/ipmi/kcs_bmc.c
> >@@ -44,12 +44,19 @@ int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc)
> > }
> > EXPORT_SYMBOL(kcs_bmc_handle_event);
> >
> >-struct kcs_bmc *kcs_bmc_ipmi_alloc(struct device *dev, int sizeof_priv, u32
> >channel);
> >-struct kcs_bmc *kcs_bmc_alloc(struct device *dev, int sizeof_priv, u32
> >channel)
> >+int kcs_bmc_ipmi_attach_cdev(struct kcs_bmc *kcs_bmc);
>
> Another declaration perhaps intended for kcs_bmc.h?
These are temporary while the code gets shuffled around. The symbol
name is an implementation detail, not a "public" part of the API; after
some further shuffling these are eventually assigned as callbacks in an
ops struct.
>
> >+int kcs_bmc_add_device(struct kcs_bmc *kcs_bmc)
> > {
> >-return kcs_bmc_ipmi_alloc(dev, sizeof_priv, channel);
> >+return kcs_bmc_ipmi_attach_cdev(kcs_bmc);
> > }
> >-EXPORT_SYMBOL(kcs_bmc_alloc);
> >+EXPORT_SYMBOL(kcs_bmc_add_device);
> >+
> >+int kcs_bmc_ipmi_detach_cdev(struct kcs_bmc *kcs_bmc);
>
> Here too.
>
> >+int kcs_bmc_remove_device(struct kcs_bmc *kcs_bmc)
> >+{
> >+return kcs_bmc_ipmi_detach_cdev(kcs_bmc);
> >+}
> >+EXPORT_SYMBOL(kcs_bmc_remove_device);
> >
> > MODULE_LICENSE("GPL v2");
> > MODULE_AUTHOR("Haiyue Wang ");
> >diff --git a/drivers/char/ipmi/kcs_bmc.h b/drivers/char/ipmi/kcs_bmc.h
> >index febea0c8deb4..bf0ae327997f 100644
> >--- a/drivers/char/ipmi/kcs_bmc.h
> >+++ b/drivers/char/ipmi/kcs_bmc.h
> >@@ -67,6 +67,8 @@ struct kcs_ioreg {
> > };
> >
> > struct kcs_bmc {
> >+struct device *dev;
> >+
> > spinlock_t lock;
> >
> > u32 channel;
> >@@ -94,17 +96,11 @@ struct kcs_bmc {
> > u8 *kbuffer;
> >
> > struct miscdevice miscdev;
> >-
> >-unsigned long priv[];
> > };
> >
> >-static inline void *kcs_bmc_priv(struct kcs_bmc *kcs_bmc)
> >-{
> >-return kcs_bmc->priv;
> >-}
> >-
> > int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc);
> >-struct kcs_bmc *kcs_bmc_alloc(struct device *dev, int sizeof_priv, u32
> >channel);
> >+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);
> >diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c
> >b/drivers/char/ipmi/kcs_bmc_aspeed.c
> >index 630cf095560e..0416ac78ce68 100644
> >--- a/drivers/char/ipmi/kcs_bmc_aspeed.c
> >+++ b/drivers/char/ipmi/kcs_bmc_aspeed.c
> >@@ -61,6 +61,8 @@
> > #define LPC_STR4 0x11C
> >
> > struct aspeed_kcs_bmc {
> >+struct kcs_bmc kcs_bmc;
> >+
> > struct regmap *map;
> > };
> >
> >@@ -69,9 +71,14 @@ struct aspeed_kcs_of_ops {
> > int (*get_io_address)(struct platform_device *pdev);
> > };
> >
> >+static inline struct aspeed_kcs_bmc *to_aspeed_kcs_bmc(struct kcs_bmc
> >*kcs_bmc)
> >+{
> >+return container_of(kcs_bmc, struct aspeed_kcs_bmc, kcs_bmc);
> >+}
> >+
> > static u8 aspeed_kcs_inb(struct kcs_bmc *kcs_bmc, u32 reg)
> > {
> >-struct aspeed_kcs_bmc *priv = kcs_bmc_priv(kcs_bmc);
> >+struct aspeed_kcs_bmc *priv = to_aspeed_kcs_bmc(kcs_bmc);
> > u32 val = 0;
> > int rc;
> >
> >@@ -83,7 +90,7 @@ static u8 aspeed_kcs_inb(struct kcs_bmc *kcs_bmc, u32 reg)
> >
> > static void aspeed_kcs_outb(struct kcs_bmc *kcs_bmc, u32 reg, u8 data)
> > {
> >-struct aspeed_kcs_bmc *priv = kcs_bmc_priv(kcs_bmc);
> >+struct aspeed_kcs_bmc *priv = to_aspeed_kcs_bmc(kcs_bmc);
> > int rc;
> >
> > rc = regmap_write(priv->map, reg, data);
>
Re: [Openipmi-developer] [PATCH v2 09/21] ipmi: kcs_bmc: Split out kcs_bmc_cdev_ipmi
On Fri, 9 Apr 2021, at 13:26, Zev Weiss wrote:
> On Fri, Mar 19, 2021 at 01:27:40AM CDT, Andrew Jeffery wrote:
> >Take steps towards defining a coherent API to separate the KCS device
> >drivers from the userspace interface. Decreasing the coupling will
> >improve the separation of concerns and enable the introduction of
> >alternative userspace interfaces.
> >
> >For now, simply split the chardev logic out to a separate file. The code
> >continues to build into the same module.
> >
> >Signed-off-by: Andrew Jeffery
> >---
> > drivers/char/ipmi/Makefile| 2 +-
> > drivers/char/ipmi/kcs_bmc.c | 423 +
> > drivers/char/ipmi/kcs_bmc.h | 10 +-
> > drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 428 ++
> > 4 files changed, 451 insertions(+), 412 deletions(-)
> > create mode 100644 drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
> >
> >diff --git a/drivers/char/ipmi/Makefile b/drivers/char/ipmi/Makefile
> >index 0822adc2ec41..a302bc865370 100644
> >--- a/drivers/char/ipmi/Makefile
> >+++ b/drivers/char/ipmi/Makefile
> >@@ -22,7 +22,7 @@ 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
> >+obj-$(CONFIG_IPMI_KCS_BMC) += kcs_bmc.o 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 c4336c1f2d6d..ef5c48ffe74a 100644
> >--- a/drivers/char/ipmi/kcs_bmc.c
> >+++ b/drivers/char/ipmi/kcs_bmc.c
> >@@ -3,446 +3,51 @@
> > * Copyright (c) 2015-2018, Intel Corporation.
> > */
> >
> >-#define pr_fmt(fmt) "kcs-bmc: " fmt
> >-
> >-#include
> >-#include
> >-#include
> > #include
> >-#include
> >-#include
> >-#include
> >-#include
> >
> > #include "kcs_bmc.h"
> >
> >-#define DEVICE_NAME "ipmi-kcs"
> >-
> >-#define KCS_MSG_BUFSIZ1000
> >-
> >-#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)
> >-#define KCS_STATUS_CMD_DAT BIT(3)
> >-#define KCS_STATUS_SMS_ATN BIT(2)
> >-#define KCS_STATUS_IBF BIT(1)
> >-#define KCS_STATUS_OBF BIT(0)
> >-
> >-/* IPMI 2.0 - Table 9-2, KCS Interface State Bits */
> >-enum kcs_states {
> >-IDLE_STATE = 0,
> >-READ_STATE = 1,
> >-WRITE_STATE = 2,
> >-ERROR_STATE = 3,
> >-};
> >-
> >-/* IPMI 2.0 - Table 9-3, KCS Interface Control Codes */
> >-#define KCS_CMD_GET_STATUS_ABORT 0x60
> >-#define KCS_CMD_WRITE_START 0x61
> >-#define KCS_CMD_WRITE_END 0x62
> >-#define KCS_CMD_READ_BYTE 0x68
> >-
> >-static inline u8 kcs_bmc_read_data(struct kcs_bmc *kcs_bmc)
> >+u8 kcs_bmc_read_data(struct kcs_bmc *kcs_bmc)
> > {
> > return kcs_bmc->io_inputb(kcs_bmc, kcs_bmc->ioreg.idr);
> > }
> >+EXPORT_SYMBOL(kcs_bmc_read_data);
> >
> >-static inline void kcs_bmc_write_data(struct kcs_bmc *kcs_bmc, u8 data)
> >+void kcs_bmc_write_data(struct kcs_bmc *kcs_bmc, u8 data)
> > {
> > kcs_bmc->io_outputb(kcs_bmc, kcs_bmc->ioreg.odr, data);
> > }
> >+EXPORT_SYMBOL(kcs_bmc_write_data);
> >
> >-static inline u8 kcs_bmc_read_status(struct kcs_bmc *kcs_bmc)
> >+u8 kcs_bmc_read_status(struct kcs_bmc *kcs_bmc)
> > {
> > return kcs_bmc->io_inputb(kcs_bmc, kcs_bmc->ioreg.str);
> > }
> >+EXPORT_SYMBOL(kcs_bmc_read_status);
> >
> >-static inline void kcs_bmc_write_status(struct kcs_bmc *kcs_bmc, u8 data)
> >+void kcs_bmc_write_status(struct kcs_bmc *kcs_bmc, u8 data)
> > {
> > kcs_bmc->io_outputb(kcs_bmc, kcs_bmc->ioreg.str, data);
> > }
> >+EXPORT_SYMBOL(kcs_bmc_write_status);
> >
> >-static void kcs_bmc_update_status(struct kcs_bmc *kcs_bmc, u8 mask, u8 val)
> >+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);
> > }
> >+EXPORT_SYMBOL(kcs_bmc_update_status);
> >
> >-static inline void set_state(struct kcs_bmc *kcs_bmc, u8 state)
> >-{
> >-kcs_bmc_update_status(kcs_bmc, KCS_STATUS_STATE_MASK,
> >-KCS_STATUS_STATE(state));
> >-}
> >-
> >-static void kcs_force_abort(struct kcs_bmc *kcs_bmc)
> >-{
> >-set_state(kcs_bmc, ERROR_STATE);
> >-kcs_bmc_read_data(kcs_bmc);
> >-kcs_bmc_write_data(kcs_bmc, KCS_ZERO_DATA);
> >-
> >-kcs_bmc->phase = KCS_PHASE_ERROR;
> >-kcs_bmc->data_in_avail = false;
> >-kcs_bmc->data_in_idx = 0;
> >-}
> >-
> >-static void kcs_bmc_handle_data(struct kcs_bmc *kcs_bmc)
> >-{
> >-u8 data;
> >-
> >-switch (kcs_bmc->phase) {
> >-case KCS_PHASE_WRITE_START:
> >-kcs_bmc->phase = KCS_PHASE_WRITE_DATA;
> >-fallthrough;
> >-
> >-
Re: [Openipmi-developer] [PATCH v2 17/21] dt-bindings: ipmi: Convert ASPEED KCS binding to schema
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 >> >--- >> > .../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 ..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 >> >+ >> >+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 ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH v2 20/21] ipmi: kcs_bmc_aspeed: Fix IBFIE typo from datasheet
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 Reviewed-by: Zev Weiss ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH v2 00/21] ipmi: Allow raw access to KCS devices
On Fri, 9 Apr 2021, at 13:37, Joel Stanley wrote: > On Thu, 8 Apr 2021 at 23:47, Andrew Jeffery wrote: > > On Thu, 8 Apr 2021, at 21:44, Corey Minyard wrote: > > > On Thu, Apr 08, 2021 at 10:27:46AM +0930, Andrew Jeffery wrote: > > > > > > 1. It begins with patches 1-5 put together by Chia-Wei, which I've > > > > > rebased on v5.12-rc2. These fix the ASPEED LPC bindings and other > > > > > non-KCS LPC-related ASPEED device drivers in a way that enables the > > > > > SerIRQ patches at the end of the series. With Joel's review I'm hoping > > > > > these 5 can go through the aspeed tree, and that the rest can go > > > > > through > > > > > the IPMI tree. > > > > > > > > > > > Please review! > > > > > > > > Unfortunately the cover letter got detached from the rest of the series. > > > > > > > > Any chance you can take a look at the patches? > > > > > > There were some minor concerns that were unanswered, and there really > > > was no review by others for many of the patches. > > > > Right; I was planning to clean up the minor concerns once I'd received > > some more feedback. I could have done a better job of communicating > > that :) > > I'll merge the first five through the aspeed tree this coming merge > window. We have acks from the relevant maintainers. > > Arnd: would you prefer that this come as it's own pull request, or as > part of the device tree branch? > > Andrew, Corey: once I've got my pull requests out I'll look at > reviewing the rest of the series. Perhaps it would pay to re-send that > hunk of patches Andrew with the nits fixed? Yep; Zev has done some reviews for me so I'll address those, rebase on your tree once you've sent out the pull-req and send out a v3. Corey: Are you okay with that for now? Cheers, Andrew ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH v2 08/21] ipmi: kcs_bmc: Rename {read, write}_{status, data}() functions
On Fri, Mar 19, 2021 at 01:27:39AM CDT, Andrew Jeffery wrote: >Rename the functions in preparation for separating the IPMI chardev out >from the KCS BMC core. > >Signed-off-by: Andrew Jeffery Reviewed-by: Zev Weiss ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH v2 17/21] dt-bindings: ipmi: Convert ASPEED KCS binding to schema
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 > >--- > > .../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 ..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 > >+ > >+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 ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH v2 07/21] ipmi: kcs_bmc: Make status update atomic
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 Reviewed-by: Zev Weiss ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH v2 01/21] dt-bindings: aspeed-lpc: Remove LPC partitioning
On Fri, 9 Apr 2021, at 12:48, Joel Stanley wrote: > On Fri, 19 Mar 2021 at 06:28, Andrew Jeffery wrote: > > > > From: "Chia-Wei, Wang" > > > > 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 ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH v2 16/21] ipmi: kcs_bmc: Add a "raw" character device interface
On Fri, Mar 19, 2021 at 01:27:47AM CDT, Andrew Jeffery wrote:
>The existing IPMI chardev encodes IPMI behaviours as the name suggests.
>However, KCS devices are useful beyond IPMI (or keyboards), as they
>provide a means to generate IRQs and exchange arbitrary data between a
>BMC and its host system.
>
>Implement a "raw" KCS character device that exposes the IDR, ODR and STR
>registers to userspace via read() and write() implemented on a character
>device:
>
>+++-+
>| Offset | read() | write() |
>+++-+
>| 0| IDR | ODR |
>+++-+
>| 1| STR | STR |
>+++-+
>
>This interface allows userspace to implement arbitrary (though somewhat
>inefficient) protocols for exchanging information between a BMC and host
>firmware. Conceptually the KCS interface can be used as an out-of-band
>machanism for interrupt-signaled control messages while bulk data
Typo ("mechanism")
>transfers occur over more appropriate interfaces between the BMC and the
>host (which may lack their own interrupt mechanism, e.g. LPC FW cycles).
>
>poll() is provided, which will wait for IBF or OBE conditions for data
>reads and writes respectively. Reads of STR on its own never blocks,
>though accessing both offsets in the one system call may block if the
>data registers are not ready.
>
>Signed-off-by: Andrew Jeffery
>---
> Documentation/ABI/testing/dev-raw-kcs | 25 ++
> drivers/char/ipmi/Kconfig | 17 +
> drivers/char/ipmi/Makefile| 1 +
> drivers/char/ipmi/kcs_bmc_cdev_raw.c | 443 ++
> 4 files changed, 486 insertions(+)
> create mode 100644 Documentation/ABI/testing/dev-raw-kcs
> create mode 100644 drivers/char/ipmi/kcs_bmc_cdev_raw.c
>
>diff --git a/Documentation/ABI/testing/dev-raw-kcs
>b/Documentation/ABI/testing/dev-raw-kcs
>new file mode 100644
>index ..06e7e2071562
>--- /dev/null
>+++ b/Documentation/ABI/testing/dev-raw-kcs
>@@ -0,0 +1,25 @@
>+What: /dev/raw-kcs*
>+Date: 2021-02-15
>+KernelVersion:5.13
>+Contact: open...@lists.ozlabs.org
>+Contact: openipmi-developer@lists.sourceforge.net
>+Contact: Andrew Jeffery
>+Description: ``/dev/raw-kcs*`` exposes to userspace the data and
>+ status registers of Keyboard-Controller-Style (KCS) IPMI
>+ interfaces via read() and write() syscalls. Direct
>+ exposure of the data and status registers enables
>+ inefficient but arbitrary protocols to be implemented
>+ over the device. A typical approach is to use KCS
>+ devices for out-of-band signalling for bulk data
>+ transfers over other interfaces between a Baseboard
>+ Management Controller and its host.
>+
>+ +++-+
>+ | Offset | read() | write() |
>+ +++-+
>+ | 0| IDR | ODR |
>+ +++-+
>+ | 1| STR | STR |
>+ +++-+
>+
>+Users:libmctp: https://github.com/openbmc/libmctp
>diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig
>index bc5f81899b62..273ac1a1f870 100644
>--- a/drivers/char/ipmi/Kconfig
>+++ b/drivers/char/ipmi/Kconfig
>@@ -137,6 +137,23 @@ config IPMI_KCS_BMC_CDEV_IPMI
> This support is also available as a module. The module will be
> called kcs_bmc_cdev_ipmi.
>
>+config IPMI_KCS_BMC_CDEV_RAW
>+ depends on IPMI_KCS_BMC
>+ tristate "Raw character device interface for BMC KCS devices"
>+ help
>+Provides a BMC-side character device directly exposing the
>+data and status registers of a KCS device to userspace. While
>+KCS devices are commonly used to implement IPMI message
>+passing, they provide a general interface for exchange of
>+interrupts, data and status information between the BMC and
>+its host.
>+
>+Say YES if you wish to use the KCS devices to implement
>+protocols that are not IPMI.
>+
>+This support is also available as a module. The module will be
>+called kcs_bmc_cdev_raw.
>+
> 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 fcfa676afddb..c8cc248ddd90 100644
>--- a/drivers/char/ipmi/Makefile
>+++ b/drivers/char/ipmi/Makefile
>@@ -24,6 +24,7 @@ obj-$(CONFIG_IPMI_WATCHDOG) += ipmi_watchdog.o
> obj-$(CONFIG_IPMI_POWEROFF) += ipmi_poweroff.o
> obj-$(CONFIG_IPMI_KCS_BMC) += kcs_bmc.o
> obj-$(CONFIG_IPMI_KCS_BMC_CDEV_IPMI) += kcs_bmc_cdev_ipmi.o
>+obj-$(CONFIG_IPMI_KCS_BMC_CDEV_RAW) += kcs_bmc_cdev_raw.o
> obj-$(CONFIG_ASPEED_BT_IPMI_BMC) += bt-bmc.o
> obj-$(CONFIG_ASPEED_KCS_IPMI_BMC) += kcs_bmc_aspeed.o
>
Re: [Openipmi-developer] [PATCH v2 17/21] dt-bindings: ipmi: Convert ASPEED KCS binding to schema
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
>---
> .../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 ..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
>+
>+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..
>--- 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
>
___
Openipmi-developer mailing list
Re: [Openipmi-developer] [PATCH v2 15/21] ipmi: kcs_bmc: Don't enforce single-open policy in the kernel
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
>---
> 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(_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(_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(_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(_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(_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(_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(>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
>
Re: [Openipmi-developer] [PATCH v2 14/21] ipmi: kcs_bmc: Allow clients to control KCS IRQ state
On Fri, Mar 19, 2021 at 01:27:45AM CDT, Andrew Jeffery wrote:
>Add a mechanism for controlling whether the client associated with a
>KCS device will receive Input Buffer Full (IBF) and Output Buffer Empty
>(OBE) events. This enables an abstract implementation of poll() for KCS
>devices.
>
>A wart in the implementation is that the ASPEED KCS devices don't
>support an OBE interrupt for the BMC. Instead we pretend it has one by
>polling the status register waiting for the Output Buffer Full (OBF) bit
>to clear, and generating an event when OBE is observed.
>
>Signed-off-by: Andrew Jeffery
>---
> drivers/char/ipmi/kcs_bmc.c | 6 ++
> drivers/char/ipmi/kcs_bmc.h | 3 +
> drivers/char/ipmi/kcs_bmc_aspeed.c | 150 ++--
> drivers/char/ipmi/kcs_bmc_client.h | 2 +
> drivers/char/ipmi/kcs_bmc_device.h | 1 +
> drivers/char/ipmi/kcs_bmc_npcm7xx.c | 25 -
> 6 files changed, 130 insertions(+), 57 deletions(-)
>
>diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
>index 694db6ee2a92..05bbb72418b2 100644
>--- a/drivers/char/ipmi/kcs_bmc.c
>+++ b/drivers/char/ipmi/kcs_bmc.c
>@@ -184,6 +184,12 @@ int kcs_bmc_unregister_cdev(struct kcs_bmc_cdev *cdev)
> }
> EXPORT_SYMBOL(kcs_bmc_unregister_cdev);
>
>+void kcs_bmc_update_event_mask(struct kcs_bmc_device *kcs_bmc, u8 mask, u8
>events)
>+{
>+ kcs_bmc->ops->irq_mask_update(kcs_bmc, mask, events);
>+}
>+EXPORT_SYMBOL(kcs_bmc_update_event_mask);
>+
> MODULE_LICENSE("GPL v2");
> MODULE_AUTHOR("Haiyue Wang ");
> MODULE_AUTHOR("Andrew Jeffery ");
>diff --git a/drivers/char/ipmi/kcs_bmc.h b/drivers/char/ipmi/kcs_bmc.h
>index 5deb9a0b8e60..11fff935218c 100644
>--- a/drivers/char/ipmi/kcs_bmc.h
>+++ b/drivers/char/ipmi/kcs_bmc.h
>@@ -11,6 +11,9 @@
> #define KCS_BMC_EVENT_NONE0
> #define KCS_BMC_EVENT_HANDLED 1
>
>+#define KCS_BMC_EVENT_TYPE_OBEBIT(0)
>+#define KCS_BMC_EVENT_TYPE_IBFBIT(1)
>+
> #define KCS_BMC_STR_OBF BIT(0)
> #define KCS_BMC_STR_IBF BIT(1)
> #define KCS_BMC_STR_CMD_DAT BIT(3)
>diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c
>b/drivers/char/ipmi/kcs_bmc_aspeed.c
>index 6f26e7366c0b..5f26471c038c 100644
>--- a/drivers/char/ipmi/kcs_bmc_aspeed.c
>+++ b/drivers/char/ipmi/kcs_bmc_aspeed.c
>@@ -60,10 +60,18 @@
> #define LPC_ODR4 0x118
> #define LPC_STR4 0x11C
>
>+#define OBE_POLL_PERIOD(HZ / 2)
>+
> struct aspeed_kcs_bmc {
> struct kcs_bmc_device kcs_bmc;
>
> struct regmap *map;
>+
>+ struct {
>+ spinlock_t lock;
>+ bool remove;
>+ struct timer_list timer;
>+ } obe;
> };
>
> struct aspeed_kcs_of_ops {
>@@ -159,68 +167,89 @@ static void aspeed_kcs_enable_channel(struct
>kcs_bmc_device *kcs_bmc, bool enabl
>
> switch (kcs_bmc->channel) {
> case 1:
>- if (enable) {
>- regmap_update_bits(priv->map, LPC_HICR2,
>- LPC_HICR2_IBFIF1, LPC_HICR2_IBFIF1);
>- regmap_update_bits(priv->map, LPC_HICR0,
>- LPC_HICR0_LPC1E, LPC_HICR0_LPC1E);
>- } else {
>- regmap_update_bits(priv->map, LPC_HICR0,
>- LPC_HICR0_LPC1E, 0);
>- regmap_update_bits(priv->map, LPC_HICR2,
>- LPC_HICR2_IBFIF1, 0);
>- }
>- break;
>-
>+ regmap_update_bits(priv->map, LPC_HICR0, LPC_HICR0_LPC1E,
>enable * LPC_HICR0_LPC1E);
>+ return;
> case 2:
>- if (enable) {
>- regmap_update_bits(priv->map, LPC_HICR2,
>- LPC_HICR2_IBFIF2, LPC_HICR2_IBFIF2);
>- regmap_update_bits(priv->map, LPC_HICR0,
>- LPC_HICR0_LPC2E, LPC_HICR0_LPC2E);
>- } else {
>- regmap_update_bits(priv->map, LPC_HICR0,
>- LPC_HICR0_LPC2E, 0);
>- regmap_update_bits(priv->map, LPC_HICR2,
>- LPC_HICR2_IBFIF2, 0);
>- }
>- break;
>-
>+ regmap_update_bits(priv->map, LPC_HICR0, LPC_HICR0_LPC2E,
>enable * LPC_HICR0_LPC2E);
>+ return;
> case 3:
>- if (enable) {
>- regmap_update_bits(priv->map, LPC_HICR2,
>- LPC_HICR2_IBFIF3, LPC_HICR2_IBFIF3);
>- regmap_update_bits(priv->map, LPC_HICR0,
>- LPC_HICR0_LPC3E, LPC_HICR0_LPC3E);
>- regmap_update_bits(priv->map, LPC_HICR4,
>- LPC_HICR4_KCSENBL, LPC_HICR4_KCSENBL);
>- } else {
>- regmap_update_bits(priv->map, LPC_HICR0,
>-
Re: [Openipmi-developer] [PATCH v2 13/21] ipmi: kcs_bmc: Decouple the IPMI chardev from the core
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
>---
> 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
>+#include
> #include
>+#include
>
> #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(_bmc->lock);
>+ kcs_bmc->client = NULL;
>+
>+ mutex_lock(_bmc_lock);
>+ list_add(_bmc->entry, _bmc_devices);
>+ list_for_each_entry(cdev, _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(_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(_bmc_lock);
>+ list_del(_bmc->entry);
>+ list_for_each_entry(cdev, _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(_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(_bmc_lock);
>+ list_add(>entry, _bmc_cdevs);
>+ list_for_each_entry(kcs_bmc, _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(_bmc_lock);
>+
>+ return 0;
...return value again
Re: [Openipmi-developer] [PATCH v2 12/21] ipmi: kcs_bmc: Strip private client data from struct kcs_bmc
On Fri, Mar 19, 2021 at 01:27:43AM CDT, Andrew Jeffery wrote:
>Move all client-private data out of `struct kcs_bmc` into the KCS client
>implementation.
>
>With this change the KCS BMC core code now only concerns itself with
>abstract `struct kcs_bmc` and `struct kcs_bmc_client` types, achieving
>expected separation of concerns. Further, the change clears the path for
>implementation of alternative userspace interfaces.
>
>The chardev data-structures are rearranged in the same manner applied to
>the KCS device driver data-structures in an earlier patch - `struct
>kcs_bmc_client` is embedded in the client's private data and we exploit
>container_of() to translate as required.
>
>Finally, now that it is free of client data, `struct kcs_bmc` is renamed
>to `struct kcs_bmc_device` to contrast `struct kcs_bmc_client`.
>
>Signed-off-by: Andrew Jeffery
>---
> drivers/char/ipmi/kcs_bmc.c | 68 +++-
> drivers/char/ipmi/kcs_bmc.h | 86 +-
> drivers/char/ipmi/kcs_bmc_aspeed.c| 22 +-
> drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 428 --
> drivers/char/ipmi/kcs_bmc_client.h| 28 +-
> drivers/char/ipmi/kcs_bmc_device.h| 12 +-
> drivers/char/ipmi/kcs_bmc_npcm7xx.c | 20 +-
> 7 files changed, 368 insertions(+), 296 deletions(-)
>
>diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
>index 1046ce2bbefc..266ebec71d6f 100644
>--- a/drivers/char/ipmi/kcs_bmc.c
>+++ b/drivers/char/ipmi/kcs_bmc.c
>@@ -4,6 +4,7 @@
> * Copyright (c) 2021, IBM Corp.
> */
>
>+#include
> #include
>
> #include "kcs_bmc.h"
>@@ -14,51 +15,96 @@
>
> /* Consumer data access */
>
>-u8 kcs_bmc_read_data(struct kcs_bmc *kcs_bmc)
>+u8 kcs_bmc_read_data(struct kcs_bmc_device *kcs_bmc)
> {
> 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)
>+void kcs_bmc_write_data(struct kcs_bmc_device *kcs_bmc, u8 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)
>+u8 kcs_bmc_read_status(struct kcs_bmc_device *kcs_bmc)
> {
> 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)
>+void kcs_bmc_write_status(struct kcs_bmc_device *kcs_bmc, u8 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)
>+void kcs_bmc_update_status(struct kcs_bmc_device *kcs_bmc, u8 mask, u8 val)
> {
> kcs_bmc->ops->io_updateb(kcs_bmc, kcs_bmc->ioreg.str, mask, val);
> }
> EXPORT_SYMBOL(kcs_bmc_update_status);
>
>-int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc)
>+int kcs_bmc_handle_event(struct kcs_bmc_device *kcs_bmc)
> {
>- return kcs_bmc->client.ops->event(_bmc->client);
>+ struct kcs_bmc_client *client;
>+ int rc;
>+
>+ spin_lock(_bmc->lock);
>+ client = kcs_bmc->client;
>+ if (client) {
>+ rc = client->ops->event(client);
>+ } 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(_bmc->lock);
>+
>+ return rc;
> }
> EXPORT_SYMBOL(kcs_bmc_handle_event);
>
>-int kcs_bmc_ipmi_attach_cdev(struct kcs_bmc *kcs_bmc);
>-int kcs_bmc_add_device(struct kcs_bmc *kcs_bmc)
>+int kcs_bmc_enable_device(struct kcs_bmc_device *kcs_bmc, struct
>kcs_bmc_client *client)
>+{
>+ int rc;
>+
>+ spin_lock_irq(_bmc->lock);
>+ if (kcs_bmc->client) {
>+ rc = -EBUSY;
>+ } else {
>+ kcs_bmc->client = client;
>+ rc = 0;
>+ }
>+ spin_unlock_irq(_bmc->lock);
>+
>+ return rc;
>+}
>+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(_bmc->lock);
>+ if (client == kcs_bmc->client)
>+ kcs_bmc->client = NULL;
Is there any situation in which a non-matching client could be passed in
here? Might we consider issuing a warning of some sort or returning an
error to the caller if so?
>+ spin_unlock_irq(_bmc->lock);
>+}
>+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);
> }
> EXPORT_SYMBOL(kcs_bmc_add_device);
>
>-int kcs_bmc_ipmi_detach_cdev(struct kcs_bmc *kcs_bmc);
>-int kcs_bmc_remove_device(struct kcs_bmc *kcs_bmc)
>+int
Re: [Openipmi-developer] [PATCH v2 00/21] ipmi: Allow raw access to KCS devices
On Thu, 8 Apr 2021 at 23:47, Andrew Jeffery wrote: > On Thu, 8 Apr 2021, at 21:44, Corey Minyard wrote: > > On Thu, Apr 08, 2021 at 10:27:46AM +0930, Andrew Jeffery wrote: > > > > 1. It begins with patches 1-5 put together by Chia-Wei, which I've > > > > rebased on v5.12-rc2. These fix the ASPEED LPC bindings and other > > > > non-KCS LPC-related ASPEED device drivers in a way that enables the > > > > SerIRQ patches at the end of the series. With Joel's review I'm hoping > > > > these 5 can go through the aspeed tree, and that the rest can go through > > > > the IPMI tree. > > > > > > > > Please review! > > > > > > Unfortunately the cover letter got detached from the rest of the series. > > > > > > Any chance you can take a look at the patches? > > > > There were some minor concerns that were unanswered, and there really > > was no review by others for many of the patches. > > Right; I was planning to clean up the minor concerns once I'd received > some more feedback. I could have done a better job of communicating > that :) I'll merge the first five through the aspeed tree this coming merge window. We have acks from the relevant maintainers. Arnd: would you prefer that this come as it's own pull request, or as part of the device tree branch? Andrew, Corey: once I've got my pull requests out I'll look at reviewing the rest of the series. Perhaps it would pay to re-send that hunk of patches Andrew with the nits fixed? Cheers, Joel ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH v2 11/21] ipmi: kcs_bmc: Split headers into device and client
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
>---
> 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
>
> #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(_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 ");
>+MODULE_AUTHOR("Andrew Jeffery ");
> 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
>
>+#include "kcs_bmc_client.h"
>+
>+#define KCS_BMC_EVENT_NONE0
>+#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
Re: [Openipmi-developer] [PATCH v2 10/21] ipmi: kcs_bmc: Turn the driver data-structures inside-out
On Fri, Mar 19, 2021 at 01:27:41AM CDT, Andrew Jeffery wrote:
>Make the KCS device drivers responsible for allocating their own memory.
>
>Until now the private data for the device driver was allocated internal
>to the private data for the chardev interface. This coupling required
>the slightly awkward API of passing through the struct size for the
>driver private data to the chardev constructor, and then retrieving a
>pointer to the driver private data from the allocated chardev memory.
>
>In addition to being awkward, the arrangement prevents the
>implementation of alternative userspace interfaces as the device driver
>private data is not independent.
>
>Peel a layer off the onion and turn the data-structures inside out by
>exploiting container_of() and embedding `struct kcs_device` in the
>driver private data.
>
>Signed-off-by: Andrew Jeffery
>---
> drivers/char/ipmi/kcs_bmc.c | 15 +--
> drivers/char/ipmi/kcs_bmc.h | 12 ++
> drivers/char/ipmi/kcs_bmc_aspeed.c| 60 ---
> drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 60 ++-
> drivers/char/ipmi/kcs_bmc_npcm7xx.c | 37 ++---
> 5 files changed, 113 insertions(+), 71 deletions(-)
>
>diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
>index ef5c48ffe74a..709b6bdec165 100644
>--- a/drivers/char/ipmi/kcs_bmc.c
>+++ b/drivers/char/ipmi/kcs_bmc.c
>@@ -44,12 +44,19 @@ int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc)
> }
> EXPORT_SYMBOL(kcs_bmc_handle_event);
>
>-struct kcs_bmc *kcs_bmc_ipmi_alloc(struct device *dev, int sizeof_priv, u32
>channel);
>-struct kcs_bmc *kcs_bmc_alloc(struct device *dev, int sizeof_priv, u32
>channel)
>+int kcs_bmc_ipmi_attach_cdev(struct kcs_bmc *kcs_bmc);
Another declaration perhaps intended for kcs_bmc.h?
>+int kcs_bmc_add_device(struct kcs_bmc *kcs_bmc)
> {
>- return kcs_bmc_ipmi_alloc(dev, sizeof_priv, channel);
>+ return kcs_bmc_ipmi_attach_cdev(kcs_bmc);
> }
>-EXPORT_SYMBOL(kcs_bmc_alloc);
>+EXPORT_SYMBOL(kcs_bmc_add_device);
>+
>+int kcs_bmc_ipmi_detach_cdev(struct kcs_bmc *kcs_bmc);
Here too.
>+int kcs_bmc_remove_device(struct kcs_bmc *kcs_bmc)
>+{
>+ return kcs_bmc_ipmi_detach_cdev(kcs_bmc);
>+}
>+EXPORT_SYMBOL(kcs_bmc_remove_device);
>
> MODULE_LICENSE("GPL v2");
> MODULE_AUTHOR("Haiyue Wang ");
>diff --git a/drivers/char/ipmi/kcs_bmc.h b/drivers/char/ipmi/kcs_bmc.h
>index febea0c8deb4..bf0ae327997f 100644
>--- a/drivers/char/ipmi/kcs_bmc.h
>+++ b/drivers/char/ipmi/kcs_bmc.h
>@@ -67,6 +67,8 @@ struct kcs_ioreg {
> };
>
> struct kcs_bmc {
>+ struct device *dev;
>+
> spinlock_t lock;
>
> u32 channel;
>@@ -94,17 +96,11 @@ struct kcs_bmc {
> u8 *kbuffer;
>
> struct miscdevice miscdev;
>-
>- unsigned long priv[];
> };
>
>-static inline void *kcs_bmc_priv(struct kcs_bmc *kcs_bmc)
>-{
>- return kcs_bmc->priv;
>-}
>-
> int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc);
>-struct kcs_bmc *kcs_bmc_alloc(struct device *dev, int sizeof_priv, u32
>channel);
>+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);
>diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c
>b/drivers/char/ipmi/kcs_bmc_aspeed.c
>index 630cf095560e..0416ac78ce68 100644
>--- a/drivers/char/ipmi/kcs_bmc_aspeed.c
>+++ b/drivers/char/ipmi/kcs_bmc_aspeed.c
>@@ -61,6 +61,8 @@
> #define LPC_STR4 0x11C
>
> struct aspeed_kcs_bmc {
>+ struct kcs_bmc kcs_bmc;
>+
> struct regmap *map;
> };
>
>@@ -69,9 +71,14 @@ struct aspeed_kcs_of_ops {
> int (*get_io_address)(struct platform_device *pdev);
> };
>
>+static inline struct aspeed_kcs_bmc *to_aspeed_kcs_bmc(struct kcs_bmc
>*kcs_bmc)
>+{
>+ return container_of(kcs_bmc, struct aspeed_kcs_bmc, kcs_bmc);
>+}
>+
> static u8 aspeed_kcs_inb(struct kcs_bmc *kcs_bmc, u32 reg)
> {
>- struct aspeed_kcs_bmc *priv = kcs_bmc_priv(kcs_bmc);
>+ struct aspeed_kcs_bmc *priv = to_aspeed_kcs_bmc(kcs_bmc);
> u32 val = 0;
> int rc;
>
>@@ -83,7 +90,7 @@ static u8 aspeed_kcs_inb(struct kcs_bmc *kcs_bmc, u32 reg)
>
> static void aspeed_kcs_outb(struct kcs_bmc *kcs_bmc, u32 reg, u8 data)
> {
>- struct aspeed_kcs_bmc *priv = kcs_bmc_priv(kcs_bmc);
>+ struct aspeed_kcs_bmc *priv = to_aspeed_kcs_bmc(kcs_bmc);
> int rc;
>
> rc = regmap_write(priv->map, reg, data);
>@@ -92,7 +99,7 @@ static void aspeed_kcs_outb(struct kcs_bmc *kcs_bmc, u32
>reg, u8 data)
>
> static void aspeed_kcs_updateb(struct kcs_bmc *kcs_bmc, u32 reg, u8 mask, u8
> val)
> {
>- struct aspeed_kcs_bmc *priv = kcs_bmc_priv(kcs_bmc);
>+ struct aspeed_kcs_bmc *priv = to_aspeed_kcs_bmc(kcs_bmc);
> int rc;
>
> rc = regmap_update_bits(priv->map, reg, mask, val);
>@@ -114,7 +121,7 @@ static void aspeed_kcs_updateb(struct kcs_bmc *kcs_bmc,
>u32 reg, u8 mask, u8 val
> */
> static
Re: [Openipmi-developer] [PATCH v2 09/21] ipmi: kcs_bmc: Split out kcs_bmc_cdev_ipmi
On Fri, Mar 19, 2021 at 01:27:40AM CDT, Andrew Jeffery wrote:
>Take steps towards defining a coherent API to separate the KCS device
>drivers from the userspace interface. Decreasing the coupling will
>improve the separation of concerns and enable the introduction of
>alternative userspace interfaces.
>
>For now, simply split the chardev logic out to a separate file. The code
>continues to build into the same module.
>
>Signed-off-by: Andrew Jeffery
>---
> drivers/char/ipmi/Makefile| 2 +-
> drivers/char/ipmi/kcs_bmc.c | 423 +
> drivers/char/ipmi/kcs_bmc.h | 10 +-
> drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 428 ++
> 4 files changed, 451 insertions(+), 412 deletions(-)
> create mode 100644 drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
>
>diff --git a/drivers/char/ipmi/Makefile b/drivers/char/ipmi/Makefile
>index 0822adc2ec41..a302bc865370 100644
>--- a/drivers/char/ipmi/Makefile
>+++ b/drivers/char/ipmi/Makefile
>@@ -22,7 +22,7 @@ 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
>+obj-$(CONFIG_IPMI_KCS_BMC) += kcs_bmc.o 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 c4336c1f2d6d..ef5c48ffe74a 100644
>--- a/drivers/char/ipmi/kcs_bmc.c
>+++ b/drivers/char/ipmi/kcs_bmc.c
>@@ -3,446 +3,51 @@
> * Copyright (c) 2015-2018, Intel Corporation.
> */
>
>-#define pr_fmt(fmt) "kcs-bmc: " fmt
>-
>-#include
>-#include
>-#include
> #include
>-#include
>-#include
>-#include
>-#include
>
> #include "kcs_bmc.h"
>
>-#define DEVICE_NAME "ipmi-kcs"
>-
>-#define KCS_MSG_BUFSIZ1000
>-
>-#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)
>-#define KCS_STATUS_CMD_DAT BIT(3)
>-#define KCS_STATUS_SMS_ATN BIT(2)
>-#define KCS_STATUS_IBF BIT(1)
>-#define KCS_STATUS_OBF BIT(0)
>-
>-/* IPMI 2.0 - Table 9-2, KCS Interface State Bits */
>-enum kcs_states {
>- IDLE_STATE = 0,
>- READ_STATE = 1,
>- WRITE_STATE = 2,
>- ERROR_STATE = 3,
>-};
>-
>-/* IPMI 2.0 - Table 9-3, KCS Interface Control Codes */
>-#define KCS_CMD_GET_STATUS_ABORT 0x60
>-#define KCS_CMD_WRITE_START 0x61
>-#define KCS_CMD_WRITE_END 0x62
>-#define KCS_CMD_READ_BYTE 0x68
>-
>-static inline u8 kcs_bmc_read_data(struct kcs_bmc *kcs_bmc)
>+u8 kcs_bmc_read_data(struct kcs_bmc *kcs_bmc)
> {
> return kcs_bmc->io_inputb(kcs_bmc, kcs_bmc->ioreg.idr);
> }
>+EXPORT_SYMBOL(kcs_bmc_read_data);
>
>-static inline void kcs_bmc_write_data(struct kcs_bmc *kcs_bmc, u8 data)
>+void kcs_bmc_write_data(struct kcs_bmc *kcs_bmc, u8 data)
> {
> kcs_bmc->io_outputb(kcs_bmc, kcs_bmc->ioreg.odr, data);
> }
>+EXPORT_SYMBOL(kcs_bmc_write_data);
>
>-static inline u8 kcs_bmc_read_status(struct kcs_bmc *kcs_bmc)
>+u8 kcs_bmc_read_status(struct kcs_bmc *kcs_bmc)
> {
> return kcs_bmc->io_inputb(kcs_bmc, kcs_bmc->ioreg.str);
> }
>+EXPORT_SYMBOL(kcs_bmc_read_status);
>
>-static inline void kcs_bmc_write_status(struct kcs_bmc *kcs_bmc, u8 data)
>+void kcs_bmc_write_status(struct kcs_bmc *kcs_bmc, u8 data)
> {
> kcs_bmc->io_outputb(kcs_bmc, kcs_bmc->ioreg.str, data);
> }
>+EXPORT_SYMBOL(kcs_bmc_write_status);
>
>-static void kcs_bmc_update_status(struct kcs_bmc *kcs_bmc, u8 mask, u8 val)
>+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);
> }
>+EXPORT_SYMBOL(kcs_bmc_update_status);
>
>-static inline void set_state(struct kcs_bmc *kcs_bmc, u8 state)
>-{
>- kcs_bmc_update_status(kcs_bmc, KCS_STATUS_STATE_MASK,
>- KCS_STATUS_STATE(state));
>-}
>-
>-static void kcs_force_abort(struct kcs_bmc *kcs_bmc)
>-{
>- set_state(kcs_bmc, ERROR_STATE);
>- kcs_bmc_read_data(kcs_bmc);
>- kcs_bmc_write_data(kcs_bmc, KCS_ZERO_DATA);
>-
>- kcs_bmc->phase = KCS_PHASE_ERROR;
>- kcs_bmc->data_in_avail = false;
>- kcs_bmc->data_in_idx = 0;
>-}
>-
>-static void kcs_bmc_handle_data(struct kcs_bmc *kcs_bmc)
>-{
>- u8 data;
>-
>- switch (kcs_bmc->phase) {
>- case KCS_PHASE_WRITE_START:
>- kcs_bmc->phase = KCS_PHASE_WRITE_DATA;
>- fallthrough;
>-
>- case KCS_PHASE_WRITE_DATA:
>- if (kcs_bmc->data_in_idx < KCS_MSG_BUFSIZ) {
>- set_state(kcs_bmc, WRITE_STATE);
>- kcs_bmc_write_data(kcs_bmc, KCS_ZERO_DATA);
>- kcs_bmc->data_in[kcs_bmc->data_in_idx++] =
>-
Re: [Openipmi-developer] [PATCH v2 05/21] soc: aspeed: Adapt to new LPC device tree layout
On Fri, 19 Mar 2021 at 06:28, Andrew Jeffery wrote: > > From: "Chia-Wei, Wang" > > 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 > Reviewed-by: Andrew Jeffery Reviewed-by: Joel Stanley ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH v2 04/21] pinctrl: aspeed-g5: Adapt to new LPC device tree layout
On Fri, 19 Mar 2021 at 06:28, Andrew Jeffery wrote: > > From: "Chia-Wei, Wang" > > 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 > Reviewed-by: Andrew Jeffery > Acked-by: Linus Walleij Reviewed-by: Joel Stanley ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH v2 03/21] ipmi: kcs: aspeed: Adapt to new LPC DTS layout
On Fri, 19 Mar 2021 at 06:28, Andrew Jeffery wrote: > > From: "Chia-Wei, Wang" > > 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 > Reviewed-by: Andrew Jeffery > Acked-by: Haiyue Wang Reviewed-by: Joel Stanley ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH v2 06/21] ipmi: kcs_bmc_aspeed: Use of match data to extract KCS properties
On Fri, Mar 19, 2021 at 01:27:37AM CDT, Andrew Jeffery wrote:
>Unpack and remove the aspeed_kcs_probe_of_v[12]() functions to aid
>rearranging how the private device-driver memory is allocated.
>
>Signed-off-by: Andrew Jeffery
>---
> drivers/char/ipmi/kcs_bmc_aspeed.c | 146 ++---
> 1 file changed, 68 insertions(+), 78 deletions(-)
>
>diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c
>b/drivers/char/ipmi/kcs_bmc_aspeed.c
>index eefe362f65f0..061f53676206 100644
>--- a/drivers/char/ipmi/kcs_bmc_aspeed.c
>+++ b/drivers/char/ipmi/kcs_bmc_aspeed.c
>@@ -13,6 +13,7 @@
> #include
> #include
> #include
>+#include
> #include
> #include
> #include
>@@ -63,6 +64,10 @@ struct aspeed_kcs_bmc {
> struct regmap *map;
> };
>
>+struct aspeed_kcs_of_ops {
>+ int (*get_channel)(struct platform_device *pdev);
>+ int (*get_io_address)(struct platform_device *pdev);
>+};
>
> static u8 aspeed_kcs_inb(struct kcs_bmc *kcs_bmc, u32 reg)
> {
>@@ -231,13 +236,10 @@ static const struct kcs_ioreg
>ast_kcs_bmc_ioregs[KCS_CHANNEL_MAX] = {
> { .idr = LPC_IDR4, .odr = LPC_ODR4, .str = LPC_STR4 },
> };
>
>-static struct kcs_bmc *aspeed_kcs_probe_of_v1(struct platform_device *pdev)
>+static int aspeed_kcs_of_v1_get_channel(struct platform_device *pdev)
> {
>- struct aspeed_kcs_bmc *priv;
> struct device_node *np;
>- struct kcs_bmc *kcs;
> u32 channel;
>- u32 slave;
> int rc;
>
> np = pdev->dev.of_node;
>@@ -245,105 +247,78 @@ static struct kcs_bmc *aspeed_kcs_probe_of_v1(struct
>platform_device *pdev)
> rc = of_property_read_u32(np, "kcs_chan", );
> if ((rc != 0) || (channel == 0 || channel > KCS_CHANNEL_MAX)) {
> dev_err(>dev, "no valid 'kcs_chan' configured\n");
>- return ERR_PTR(-EINVAL);
>+ return -EINVAL;
> }
>
>- kcs = kcs_bmc_alloc(>dev, sizeof(struct aspeed_kcs_bmc), channel);
>- if (!kcs)
>- return ERR_PTR(-ENOMEM);
>+ return channel;
>+}
>
>- priv = kcs_bmc_priv(kcs);
>- priv->map = syscon_node_to_regmap(pdev->dev.parent->of_node);
>- if (IS_ERR(priv->map)) {
>- dev_err(>dev, "Couldn't get regmap\n");
>- return ERR_PTR(-ENODEV);
>- }
>+static int aspeed_kcs_of_v1_get_io_address(struct platform_device *pdev)
>+{
>+ u32 slave;
>+ int rc;
>
>- rc = of_property_read_u32(np, "kcs_addr", );
>- if (rc) {
>+ rc = of_property_read_u32(pdev->dev.of_node, "kcs_addr", );
>+ if (rc || slave > 0x) {
> dev_err(>dev, "no valid 'kcs_addr' configured\n");
>- return ERR_PTR(-EINVAL);
>+ return -EINVAL;
> }
>
>- kcs->ioreg = ast_kcs_bmc_ioregs[channel - 1];
>- aspeed_kcs_set_address(kcs, slave);
>-
>- return kcs;
>-}
>-
>-static int aspeed_kcs_calculate_channel(const struct kcs_ioreg *regs)
>-{
>- int i;
>-
>- for (i = 0; i < ARRAY_SIZE(ast_kcs_bmc_ioregs); i++) {
>- if (!memcmp(_kcs_bmc_ioregs[i], regs, sizeof(*regs)))
>- return i + 1;
>- }
>-
>- return -EINVAL;
>+ return slave;
> }
>
>-static struct kcs_bmc *aspeed_kcs_probe_of_v2(struct platform_device *pdev)
>+static int aspeed_kcs_of_v2_get_channel(struct platform_device *pdev)
> {
>- struct aspeed_kcs_bmc *priv;
> struct device_node *np;
> struct kcs_ioreg ioreg;
>- struct kcs_bmc *kcs;
> const __be32 *reg;
>- int channel;
>- u32 slave;
>- int rc;
>+ int i;
>
> np = pdev->dev.of_node;
>
> /* Don't translate addresses, we want offsets for the regmaps */
> reg = of_get_address(np, 0, NULL, NULL);
> if (!reg)
>- return ERR_PTR(-EINVAL);
>+ return -EINVAL;
> ioreg.idr = be32_to_cpup(reg);
>
> reg = of_get_address(np, 1, NULL, NULL);
> if (!reg)
>- return ERR_PTR(-EINVAL);
>+ return -EINVAL;
> ioreg.odr = be32_to_cpup(reg);
>
> reg = of_get_address(np, 2, NULL, NULL);
> if (!reg)
>- return ERR_PTR(-EINVAL);
>+ return -EINVAL;
> ioreg.str = be32_to_cpup(reg);
>
>- channel = aspeed_kcs_calculate_channel();
>- if (channel < 0)
>- return ERR_PTR(channel);
>-
>- kcs = kcs_bmc_alloc(>dev, sizeof(struct aspeed_kcs_bmc), channel);
>- if (!kcs)
>- return ERR_PTR(-ENOMEM);
>-
>- kcs->ioreg = ioreg;
>-
>- priv = kcs_bmc_priv(kcs);
>- priv->map = syscon_node_to_regmap(pdev->dev.parent->of_node);
>- if (IS_ERR(priv->map)) {
>- dev_err(>dev, "Couldn't get regmap\n");
>- return ERR_PTR(-ENODEV);
>+ for (i = 0; i < ARRAY_SIZE(ast_kcs_bmc_ioregs); i++) {
>+ if (!memcmp(_kcs_bmc_ioregs[i], , sizeof(ioreg)))
>+ return i + 1;
Did some patches perhaps get a bit jumbled during a rebase here or
something? This patch removes the
Re: [Openipmi-developer] [PATCH v2 01/21] dt-bindings: aspeed-lpc: Remove LPC partitioning
On Fri, 19 Mar 2021 at 06:28, Andrew Jeffery wrote:
>
> From: "Chia-Wei, Wang"
>
> 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
>
> Signed-off-by: Chia-Wei Wang
> Reviewed-by: Andrew Jeffery
> Acked-by: Rob Herring
> Acked-by: Lee Jones
> ---
> .../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 = ;
> + 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>;
> -
Re: [Openipmi-developer] [PATCH v2 00/21] ipmi: Allow raw access to KCS devices
On Thu, 8 Apr 2021, at 21:44, Corey Minyard wrote: > On Thu, Apr 08, 2021 at 10:27:46AM +0930, Andrew Jeffery wrote: > > Hi Corey, > > > > On Fri, 19 Mar 2021, at 16:49, Andrew Jeffery wrote: > > > Hello, > > > > > > This series is a bit of a mix of things, but its primary purpose is to > > > expose BMC KCS IPMI devices to userspace in a way that enables userspace > > > to talk to host firmware using protocols that are not IPMI. > > > > > > v1 can be found here: > > > > > > https://lore.kernel.org/openbmc/20210219142523.3464540-1-and...@aj.id.au/ > > > > > > Changes in v2 include: > > > > > > * A rebase onto v5.12-rc2 > > > * Incorporation of off-list feedback on SerIRQ configuration from > > > Chiawei > > > * Further validation on hardware for ASPEED KCS devices 2, 3 and 4 > > > * Lifting the existing single-open constraint of the IPMI chardev > > > * Fixes addressing Rob's feedback on the conversion of the ASPEED KCS > > > binding to dt-schema > > > * Fixes addressing Rob's feedback on the new aspeed,lpc-interrupts > > > property definition for the ASPEED KCS binding > > > > > > A new chardev device is added whose implementation exposes the Input > > > Data Register (IDR), Output Data Register (ODR) and Status Register > > > (STR) via read() and write(), and implements poll() for event > > > monitoring. > > > > > > The existing /dev/ipmi-kcs* chardev interface exposes the KCS devices in > > > a way which encoded the IPMI protocol in its behaviour. However, as > > > LPC[0] KCS devices give us bi-directional interrupts between the host > > > and a BMC with both a data and status byte, they are useful for purposes > > > beyond IPMI. > > > > > > As a concrete example, libmctp[1] implements a vendor-defined MCTP[2] > > > binding using a combination of LPC Firmware cycles for bulk data > > > transfer and a KCS device via LPC IO cycles for out-of-band protocol > > > control messages[3]. This gives a throughput improvement over the > > > standard KCS binding[4] while continuing to exploit the ease of setup of > > > the LPC bus for early boot firmware on the host processor. > > > > > > The series takes a bit of a winding path to achieve its aim: > > > > > > 1. It begins with patches 1-5 put together by Chia-Wei, which I've > > > rebased on v5.12-rc2. These fix the ASPEED LPC bindings and other > > > non-KCS LPC-related ASPEED device drivers in a way that enables the > > > SerIRQ patches at the end of the series. With Joel's review I'm hoping > > > these 5 can go through the aspeed tree, and that the rest can go through > > > the IPMI tree. > > > > > > 2. Next, patches 6-13 fairly heavily refactor the KCS support in the > > > IPMI part of the tree, re-architecting things such that it's possible to > > > support multiple chardev implementations sitting on top of the ASPEED > > > and Nuvoton device drivers. However, the KCS code didn't really have > > > great separation of concerns as it stood, so even if we disregard the > > > multiple-chardev support I think the cleanups are worthwhile. > > > > > > 3. Patch 14 adds some interrupt management capabilities to the KCS > > > device drivers in preparation for patch 16, which introduces the new > > > "raw" KCS device interface. I'm not stoked about the device name/path, > > > so if people are looking to bikeshed something then feel free to lay > > > into that. > > > > > > 4. The remaining patches switch the ASPEED KCS devicetree binding to > > > dt-schema, add a new interrupt property to describe the SerIRQ behaviour > > > of the device and finally clean up Serial IRQ support in the ASPEED KCS > > > driver. > > > > > > Rob: The dt-binding patches still come before the relevant driver > > > changes, I tried to keep the two close together in the series, hence the > > > bindings changes not being patches 1 and 2. > > > > > > I've exercised the series under qemu with the rainier-bmc machine plus > > > additional patches for KCS support[5]. I've also substituted this series > > > in > > > place of a hacky out-of-tree driver that we've been using for the > > > libmctp stack and successfully booted the host processor under our > > > internal full-platform simulation tools for a Rainier system. > > > > > > Note that this work touches the Nuvoton driver as well as ASPEED's, but > > > I don't have the capability to test those changes or the IPMI chardev > > > path. Tested-by tags would be much appreciated if you can exercise one > > > or both. > > > > > > Please review! > > > > Unfortunately the cover letter got detached from the rest of the series. > > > > Any chance you can take a look at the patches? > > There were some minor concerns that were unanswered, and there really > was no review by others for many of the patches. Right; I was planning to clean up the minor concerns once I'd received some more feedback. I could have done a better job of communicating that :) > > I would like this patch set, it makes some good cleanups. But I would
Re: [Openipmi-developer] [PATCH v1 1/1] kernel.h: Split out panic and oops helpers
On 06/04/2021 15.31, Andy Shevchenko wrote:
> kernel.h is being used as a dump for all kinds of stuff for a long time.
> Here is the attempt to start cleaning it up by splitting out panic and
> oops helpers.
Yay.
Acked-by: Rasmus Villemoes
> At the same time convert users in header and lib folder to use new header.
> Though for time being include new header back to kernel.h to avoid twisted
> indirected includes for existing users.
I think it would be good to have some place to note that "This #include
is just for backwards compatibility, it will go away RealSoonNow, so if
you rely on something from linux/panic.h, include that explicitly
yourself TYVM. And if you're looking for a janitorial task, write a
script to check that every file that uses some identifier defined in
panic.h actually includes that file. When all offenders are found and
dealt with, remove the #include and this note.".
> +
> +struct taint_flag {
> + char c_true;/* character printed when tainted */
> + char c_false; /* character printed when not tainted */
> + bool module;/* also show as a per-module taint flag */
> +};
> +
> +extern const struct taint_flag taint_flags[TAINT_FLAGS_COUNT];
While you're doing this, nothing outside of kernel/panic.c cares about
the definition of struct taint_flag or use the taint_flags array, so
could you make the definition private to that file and make the array
static? (Another patch, of course.)
> +enum lockdep_ok {
> + LOCKDEP_STILL_OK,
> + LOCKDEP_NOW_UNRELIABLE,
> +};
> +
> +extern const char *print_tainted(void);
> +extern void add_taint(unsigned flag, enum lockdep_ok);
> +extern int test_taint(unsigned flag);
> +extern unsigned long get_taint(void);
I know you're just moving code, but it would be a nice opportunity to
drop the redundant externs.
Rasmus
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH v1 1/1] kernel.h: Split out panic and oops helpers
On Thu, Apr 08, 2021 at 02:45:12PM +0200, Rasmus Villemoes wrote:
> On 06/04/2021 15.31, Andy Shevchenko wrote:
> > kernel.h is being used as a dump for all kinds of stuff for a long time.
> > Here is the attempt to start cleaning it up by splitting out panic and
> > oops helpers.
>
> Yay.
>
> Acked-by: Rasmus Villemoes
Thanks!
> > At the same time convert users in header and lib folder to use new header.
> > Though for time being include new header back to kernel.h to avoid twisted
> > indirected includes for existing users.
>
> I think it would be good to have some place to note that "This #include
> is just for backwards compatibility, it will go away RealSoonNow, so if
> you rely on something from linux/panic.h, include that explicitly
> yourself TYVM. And if you're looking for a janitorial task, write a
> script to check that every file that uses some identifier defined in
> panic.h actually includes that file. When all offenders are found and
> dealt with, remove the #include and this note.".
Good and...
> > +struct taint_flag {
> > + char c_true;/* character printed when tainted */
> > + char c_false; /* character printed when not tainted */
> > + bool module;/* also show as a per-module taint flag */
> > +};
> > +
> > +extern const struct taint_flag taint_flags[TAINT_FLAGS_COUNT];
>
> While you're doing this, nothing outside of kernel/panic.c cares about
> the definition of struct taint_flag or use the taint_flags array, so
> could you make the definition private to that file and make the array
> static? (Another patch, of course.)
...according to the above if *you are looking for a janitorial task*... :-))
> > +enum lockdep_ok {
> > + LOCKDEP_STILL_OK,
> > + LOCKDEP_NOW_UNRELIABLE,
> > +};
> > +
> > +extern const char *print_tainted(void);
> > +extern void add_taint(unsigned flag, enum lockdep_ok);
> > +extern int test_taint(unsigned flag);
> > +extern unsigned long get_taint(void);
>
> I know you're just moving code, but it would be a nice opportunity to
> drop the redundant externs.
As above. But for all these I have heard you. So, I'll keep this response
as part of my always only growing TODO list.
--
With Best Regards,
Andy Shevchenko
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH v2 00/21] ipmi: Allow raw access to KCS devices
On Thu, Apr 08, 2021 at 10:27:46AM +0930, Andrew Jeffery wrote: > Hi Corey, > > On Fri, 19 Mar 2021, at 16:49, Andrew Jeffery wrote: > > Hello, > > > > This series is a bit of a mix of things, but its primary purpose is to > > expose BMC KCS IPMI devices to userspace in a way that enables userspace > > to talk to host firmware using protocols that are not IPMI. > > > > v1 can be found here: > > > > https://lore.kernel.org/openbmc/20210219142523.3464540-1-and...@aj.id.au/ > > > > Changes in v2 include: > > > > * A rebase onto v5.12-rc2 > > * Incorporation of off-list feedback on SerIRQ configuration from > > Chiawei > > * Further validation on hardware for ASPEED KCS devices 2, 3 and 4 > > * Lifting the existing single-open constraint of the IPMI chardev > > * Fixes addressing Rob's feedback on the conversion of the ASPEED KCS > > binding to dt-schema > > * Fixes addressing Rob's feedback on the new aspeed,lpc-interrupts > > property definition for the ASPEED KCS binding > > > > A new chardev device is added whose implementation exposes the Input > > Data Register (IDR), Output Data Register (ODR) and Status Register > > (STR) via read() and write(), and implements poll() for event > > monitoring. > > > > The existing /dev/ipmi-kcs* chardev interface exposes the KCS devices in > > a way which encoded the IPMI protocol in its behaviour. However, as > > LPC[0] KCS devices give us bi-directional interrupts between the host > > and a BMC with both a data and status byte, they are useful for purposes > > beyond IPMI. > > > > As a concrete example, libmctp[1] implements a vendor-defined MCTP[2] > > binding using a combination of LPC Firmware cycles for bulk data > > transfer and a KCS device via LPC IO cycles for out-of-band protocol > > control messages[3]. This gives a throughput improvement over the > > standard KCS binding[4] while continuing to exploit the ease of setup of > > the LPC bus for early boot firmware on the host processor. > > > > The series takes a bit of a winding path to achieve its aim: > > > > 1. It begins with patches 1-5 put together by Chia-Wei, which I've > > rebased on v5.12-rc2. These fix the ASPEED LPC bindings and other > > non-KCS LPC-related ASPEED device drivers in a way that enables the > > SerIRQ patches at the end of the series. With Joel's review I'm hoping > > these 5 can go through the aspeed tree, and that the rest can go through > > the IPMI tree. > > > > 2. Next, patches 6-13 fairly heavily refactor the KCS support in the > > IPMI part of the tree, re-architecting things such that it's possible to > > support multiple chardev implementations sitting on top of the ASPEED > > and Nuvoton device drivers. However, the KCS code didn't really have > > great separation of concerns as it stood, so even if we disregard the > > multiple-chardev support I think the cleanups are worthwhile. > > > > 3. Patch 14 adds some interrupt management capabilities to the KCS > > device drivers in preparation for patch 16, which introduces the new > > "raw" KCS device interface. I'm not stoked about the device name/path, > > so if people are looking to bikeshed something then feel free to lay > > into that. > > > > 4. The remaining patches switch the ASPEED KCS devicetree binding to > > dt-schema, add a new interrupt property to describe the SerIRQ behaviour > > of the device and finally clean up Serial IRQ support in the ASPEED KCS > > driver. > > > > Rob: The dt-binding patches still come before the relevant driver > > changes, I tried to keep the two close together in the series, hence the > > bindings changes not being patches 1 and 2. > > > > I've exercised the series under qemu with the rainier-bmc machine plus > > additional patches for KCS support[5]. I've also substituted this series in > > place of a hacky out-of-tree driver that we've been using for the > > libmctp stack and successfully booted the host processor under our > > internal full-platform simulation tools for a Rainier system. > > > > Note that this work touches the Nuvoton driver as well as ASPEED's, but > > I don't have the capability to test those changes or the IPMI chardev > > path. Tested-by tags would be much appreciated if you can exercise one > > or both. > > > > Please review! > > Unfortunately the cover letter got detached from the rest of the series. > > Any chance you can take a look at the patches? There were some minor concerns that were unanswered, and there really was no review by others for many of the patches. I would like this patch set, it makes some good cleanups. But I would like some more review and testing by others, if possible. I'm fairly sure it has already been done, it just needs to be documented. -corey > > https://lore.kernel.org/linux-arm-kernel/20210319062752.145730-1-and...@aj.id.au/ > > Cheers, > > Andrew ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net
