| Message ID | E1oTkeW-003t9Y-Ey@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Apple Mac System Management Controller GPIOs | expand |
Thanks for trying to upstream this! I just have three minor comments: On Thu, Sep 1, 2022, at 15:54, Russell King wrote: > From: Hector Martin <marcan@marcan.st> > > This driver implements support for the SMC (System Management > Controller) in Apple Macs. In contrast to the existing applesmc driver, > it uses pluggable backends that allow it to support different SMC > implementations, and uses the MFD subsystem to expose the core SMC > functionality so that specific features (gpio, hwmon, battery, etc.) can > be implemented by separate drivers in their respective downstream > subsystems. > > The initial RTKit backend adds support for Apple Silicon Macs (M1 et > al). We hope a backend for T2 Macs will be written in the future > (since those are not supported by applesmc), and eventually an x86 > backend would allow us to fully deprecate applesmc in favor of this > driver. > > Signed-off-by: Hector Martin <marcan@marcan.st> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > --- > drivers/platform/Kconfig | 2 + > drivers/platform/Makefile | 1 + > drivers/platform/apple/Kconfig | 49 ++++ > drivers/platform/apple/Makefile | 11 + > drivers/platform/apple/smc.h | 28 ++ > drivers/platform/apple/smc_core.c | 249 ++++++++++++++++ > drivers/platform/apple/smc_rtkit.c | 451 +++++++++++++++++++++++++++++ > include/linux/mfd/macsmc.h | 86 ++++++ > 8 files changed, 877 insertions(+) > create mode 100644 drivers/platform/apple/Kconfig > create mode 100644 drivers/platform/apple/Makefile > create mode 100644 drivers/platform/apple/smc.h > create mode 100644 drivers/platform/apple/smc_core.c > create mode 100644 drivers/platform/apple/smc_rtkit.c > create mode 100644 include/linux/mfd/macsmc.h > > diff --git a/drivers/platform/Kconfig b/drivers/platform/Kconfig > index b437847b6237..5f8b9bcdb830 100644 > --- a/drivers/platform/Kconfig > +++ b/drivers/platform/Kconfig > @@ -13,4 +13,6 @@ source "drivers/platform/olpc/Kconfig" > > source "drivers/platform/surface/Kconfig" > > +source "drivers/platform/apple/Kconfig" > + > source "drivers/platform/x86/Kconfig" > diff --git a/drivers/platform/Makefile b/drivers/platform/Makefile > index 4de08ef4ec9d..3e5d5039a28c 100644 > --- a/drivers/platform/Makefile > +++ b/drivers/platform/Makefile > @@ -10,3 +10,4 @@ obj-$(CONFIG_OLPC_EC) += olpc/ > obj-$(CONFIG_GOLDFISH) += goldfish/ > obj-$(CONFIG_CHROME_PLATFORMS) += chrome/ > obj-$(CONFIG_SURFACE_PLATFORMS) += surface/ > +obj-$(CONFIG_APPLE_PLATFORMS) += apple/ > diff --git a/drivers/platform/apple/Kconfig > b/drivers/platform/apple/Kconfig > new file mode 100644 > index 000000000000..42525aa9fbbe > --- /dev/null > +++ b/drivers/platform/apple/Kconfig > @@ -0,0 +1,49 @@ > +# SPDX-License-Identifier: GPL-2.0 > +# > +# Apple Platform-Specific Drivers > +# > + > +menuconfig APPLE_PLATFORMS > + bool "Apple Mac Platform-Specific Device Drivers" > + default y > + help > + Say Y here to get to see options for platform-specific device > drivers > + for Apple devices. This option alone does not add any kernel code. > + > + If you say N, all options in this submenu will be skipped and > disabled. > + > +if APPLE_PLATFORMS > + > +config APPLE_SMC > + tristate "Apple SMC Driver" > + depends on ARCH_APPLE || COMPILE_TEST > + default ARCH_APPLE > + select MFD_CORE > + help > + Build support for the Apple System Management Controller present in > + Apple Macs. This driver currently supports the SMC in Apple Silicon > + Macs. For x86 Macs, see the applesmc driver (SENSORS_APPLESMC). > + > + Say Y here if you have an Apple Silicon Mac. > + > + To compile this driver as a module, choose M here: the module will > + be called macsmc. > + > +if APPLE_SMC > + > +config APPLE_SMC_RTKIT > + tristate "RTKit (Apple Silicon) backend" > + depends on ARCH_APPLE || COMPILE_TEST > + depends on APPLE_RTKIT > + default ARCH_APPLE > + help > + Build support for SMC communications via the RTKit backend. This is > + required for Apple Silicon Macs. > + > + Say Y here if you have an Apple Silicon Mac. > + > + To compile this driver as a module, choose M here: the module will > + be called macsmc-rtkit. > + > +endif > +endif > diff --git a/drivers/platform/apple/Makefile > b/drivers/platform/apple/Makefile > new file mode 100644 > index 000000000000..79fac195398b > --- /dev/null > +++ b/drivers/platform/apple/Makefile > @@ -0,0 +1,11 @@ > +# SPDX-License-Identifier: GPL-2.0 > +# > +# Makefile for linux/drivers/platform/apple > +# Apple Platform-Specific Drivers > +# > + > +macsmc-y += smc_core.o > +macsmc-rtkit-y += smc_rtkit.o > + > +obj-$(CONFIG_APPLE_SMC) += macsmc.o > +obj-$(CONFIG_APPLE_SMC_RTKIT) += macsmc-rtkit.o > diff --git a/drivers/platform/apple/smc.h b/drivers/platform/apple/smc.h > new file mode 100644 > index 000000000000..8ae51887b2c5 > --- /dev/null > +++ b/drivers/platform/apple/smc.h > @@ -0,0 +1,28 @@ > +// SPDX-License-Identifier: GPL-2.0-only OR MIT > +/* > + * Apple SMC internal core definitions > + * Copyright (C) The Asahi Linux Contributors > + */ > + > +#ifndef _SMC_H > +#define _SMC_H > + > +#include <linux/mfd/macsmc.h> > + > +struct apple_smc_backend_ops { > + int (*read_key)(void *cookie, smc_key key, void *buf, size_t size); > + int (*write_key)(void *cookie, smc_key key, void *buf, size_t size); > + int (*write_key_atomic)(void *cookie, smc_key key, void *buf, size_t > size); > + int (*rw_key)(void *cookie, smc_key key, void *wbuf, size_t wsize, > + void *rbuf, size_t rsize); > + int (*get_key_by_index)(void *cookie, int index, smc_key *key); > + int (*get_key_info)(void *cookie, smc_key key, struct > apple_smc_key_info *info); > +}; > + > +struct apple_smc *apple_smc_probe(struct device *dev, const struct > apple_smc_backend_ops *ops, > + void *cookie); > +void *apple_smc_get_cookie(struct apple_smc *smc); > +int apple_smc_remove(struct apple_smc *smc); > +void apple_smc_event_received(struct apple_smc *smc, uint32_t event); > + > +#endif > diff --git a/drivers/platform/apple/smc_core.c > b/drivers/platform/apple/smc_core.c > new file mode 100644 > index 000000000000..daf029cd072f > --- /dev/null > +++ b/drivers/platform/apple/smc_core.c > @@ -0,0 +1,249 @@ > +// SPDX-License-Identifier: GPL-2.0-only OR MIT > +/* > + * Apple SMC core framework > + * Copyright The Asahi Linux Contributors > + */ > + > +#include <linux/device.h> > +#include <linux/mfd/core.h> > +#include <linux/mutex.h> > +#include <linux/notifier.h> > +#include "smc.h" > + > +struct apple_smc { > + struct device *dev; > + > + void *be_cookie; > + const struct apple_smc_backend_ops *be; > + > + struct mutex mutex; > + > + u32 key_count; > + smc_key first_key; > + smc_key last_key; > + > + struct blocking_notifier_head event_handlers; > +}; > + > +static const struct mfd_cell apple_smc_devs[] = { > + { > + .name = "macsmc-gpio", > + }, > + { > + .name = "macsmc-hid", > + }, > + { > + .name = "macsmc-power", > + }, > + { > + .name = "macsmc-reboot", > + }, > + { > + .name = "macsmc-rtc", > + }, > +}; > + > +int apple_smc_read(struct apple_smc *smc, smc_key key, void *buf, > size_t size) > +{ > + int ret; > + > + mutex_lock(&smc->mutex); > + ret = smc->be->read_key(smc->be_cookie, key, buf, size); > + mutex_unlock(&smc->mutex); > + > + return ret; > +} > +EXPORT_SYMBOL(apple_smc_read); > + > +int apple_smc_write(struct apple_smc *smc, smc_key key, void *buf, > size_t size) > +{ > + int ret; > + > + mutex_lock(&smc->mutex); > + ret = smc->be->write_key(smc->be_cookie, key, buf, size); > + mutex_unlock(&smc->mutex); > + > + return ret; > +} > +EXPORT_SYMBOL(apple_smc_write); > + > +int apple_smc_write_atomic(struct apple_smc *smc, smc_key key, void > *buf, size_t size) > +{ > + int ret; > + > + /* > + * Will fail if SMC is busy. This is only used by SMC reboot/poweroff > + * final calls, so it doesn't really matter at that point. > + */ > + if (!mutex_trylock(&smc->mutex)) > + return -EBUSY; > + > + ret = smc->be->write_key_atomic(smc->be_cookie, key, buf, size); > + mutex_unlock(&smc->mutex); > + > + return ret; > +} > +EXPORT_SYMBOL(apple_smc_write_atomic); > + > +int apple_smc_rw(struct apple_smc *smc, smc_key key, void *wbuf, > size_t wsize, > + void *rbuf, size_t rsize) > +{ > + int ret; > + > + mutex_lock(&smc->mutex); > + ret = smc->be->rw_key(smc->be_cookie, key, wbuf, wsize, rbuf, rsize); > + mutex_unlock(&smc->mutex); > + > + return ret; > +} > +EXPORT_SYMBOL(apple_smc_rw); > + > +int apple_smc_get_key_by_index(struct apple_smc *smc, int index, > smc_key *key) > +{ > + int ret; > + > + mutex_lock(&smc->mutex); > + ret = smc->be->get_key_by_index(smc->be_cookie, index, key); > + mutex_unlock(&smc->mutex); > + > + return ret; > +} > +EXPORT_SYMBOL(apple_smc_get_key_by_index); > + > +int apple_smc_get_key_info(struct apple_smc *smc, smc_key key, struct > apple_smc_key_info *info) > +{ > + int ret; > + > + mutex_lock(&smc->mutex); > + ret = smc->be->get_key_info(smc->be_cookie, key, info); > + mutex_unlock(&smc->mutex); > + > + return ret; > +} > +EXPORT_SYMBOL(apple_smc_get_key_info); > + > +int apple_smc_find_first_key_index(struct apple_smc *smc, smc_key key) > +{ > + int start = 0, count = smc->key_count; > + int ret; > + > + if (key <= smc->first_key) > + return 0; > + if (key > smc->last_key) > + return smc->key_count; > + > + while (count > 1) { > + int pivot = start + ((count - 1) >> 1); > + smc_key pkey; > + > + ret = apple_smc_get_key_by_index(smc, pivot, &pkey); > + if (ret < 0) > + return ret; > + > + if (pkey == key) > + return pivot; > + > + pivot++; > + > + if (pkey < key) { > + count -= pivot - start; > + start = pivot; > + } else { > + count = pivot - start; > + } > + } > + > + return start; > +} > +EXPORT_SYMBOL(apple_smc_find_first_key_index); > + > +int apple_smc_get_key_count(struct apple_smc *smc) > +{ > + return smc->key_count; > +} > +EXPORT_SYMBOL(apple_smc_get_key_count); > + > +void apple_smc_event_received(struct apple_smc *smc, uint32_t event) > +{ > + dev_dbg(smc->dev, "Event: 0x%08x\n", event); > + blocking_notifier_call_chain(&smc->event_handlers, event, NULL); > +} > +EXPORT_SYMBOL(apple_smc_event_received); > + > +int apple_smc_register_notifier(struct apple_smc *smc, struct > notifier_block *n) > +{ > + return blocking_notifier_chain_register(&smc->event_handlers, n); > +} > +EXPORT_SYMBOL(apple_smc_register_notifier); > + > +int apple_smc_unregister_notifier(struct apple_smc *smc, struct > notifier_block *n) > +{ > + return blocking_notifier_chain_unregister(&smc->event_handlers, n); > +} > +EXPORT_SYMBOL(apple_smc_unregister_notifier); > + > +void *apple_smc_get_cookie(struct apple_smc *smc) > +{ > + return smc->be_cookie; > +} > +EXPORT_SYMBOL(apple_smc_get_cookie); > + > +struct apple_smc *apple_smc_probe(struct device *dev, const struct > apple_smc_backend_ops *ops, void *cookie) > +{ > + struct apple_smc *smc; > + u32 count; > + int ret; > + > + smc = devm_kzalloc(dev, sizeof(*smc), GFP_KERNEL); > + if (!smc) > + return ERR_PTR(-ENOMEM); > + > + smc->dev = dev; > + smc->be_cookie = cookie; > + smc->be = ops; > + mutex_init(&smc->mutex); > + BLOCKING_INIT_NOTIFIER_HEAD(&smc->event_handlers); > + > + ret = apple_smc_read_u32(smc, SMC_KEY(#KEY), &count); > + if (ret) > + return ERR_PTR(dev_err_probe(dev, ret, "Failed to get key count")); > + smc->key_count = be32_to_cpu(count); > + > + ret = apple_smc_get_key_by_index(smc, 0, &smc->first_key); > + if (ret) > + return ERR_PTR(dev_err_probe(dev, ret, "Failed to get first key")); > + > + ret = apple_smc_get_key_by_index(smc, smc->key_count - 1, > &smc->last_key); > + if (ret) > + return ERR_PTR(dev_err_probe(dev, ret, "Failed to get last key")); > + > + /* Enable notifications */ > + apple_smc_write_flag(smc, SMC_KEY(NTAP), 1); > + > + dev_info(dev, "Initialized (%d keys %p4ch..%p4ch)\n", > + smc->key_count, &smc->first_key, &smc->last_key); I don't think upstream supports %p4ch. marcan added that format in our downstream tree iirc. > + > + dev_set_drvdata(dev, smc); > + > + ret = mfd_add_devices(dev, -1, apple_smc_devs, > ARRAY_SIZE(apple_smc_devs), NULL, 0, NULL); > + if (ret) > + return ERR_PTR(dev_err_probe(dev, ret, "Subdevice initialization > failed")); > + > + return smc; > +} > +EXPORT_SYMBOL(apple_smc_probe); > + > +int apple_smc_remove(struct apple_smc *smc) > +{ > + mfd_remove_devices(smc->dev); > + > + /* Disable notifications */ > + apple_smc_write_flag(smc, SMC_KEY(NTAP), 1); > + > + return 0; > +} > +EXPORT_SYMBOL(apple_smc_remove); > + > +MODULE_AUTHOR("Hector Martin <marcan@marcan.st>"); > +MODULE_LICENSE("Dual MIT/GPL"); > +MODULE_DESCRIPTION("Apple SMC core"); > diff --git a/drivers/platform/apple/smc_rtkit.c > b/drivers/platform/apple/smc_rtkit.c > new file mode 100644 > index 000000000000..5b7c4c475bbb > --- /dev/null > +++ b/drivers/platform/apple/smc_rtkit.c > @@ -0,0 +1,451 @@ > +// SPDX-License-Identifier: GPL-2.0-only OR MIT > +/* > + * Apple SMC RTKit backend > + * Copyright The Asahi Linux Contributors > + */ > + > +#include <asm/unaligned.h> > +#include <linux/delay.h> > +#include <linux/device.h> > +#include <linux/io.h> > +#include <linux/ioport.h> > +#include <linux/of.h> > +#include <linux/of_platform.h> > +#include <linux/soc/apple/rtkit.h> > +#include "smc.h" > + > +#define SMC_ENDPOINT 0x20 > + > +/* Guess */ > +#define SMC_SHMEM_SIZE 0x1000 > + > +#define SMC_MSG_READ_KEY 0x10 > +#define SMC_MSG_WRITE_KEY 0x11 > +#define SMC_MSG_GET_KEY_BY_INDEX 0x12 > +#define SMC_MSG_GET_KEY_INFO 0x13 > +#define SMC_MSG_INITIALIZE 0x17 > +#define SMC_MSG_NOTIFICATION 0x18 > +#define SMC_MSG_RW_KEY 0x20 > + > +#define SMC_DATA GENMASK(63, 32) > +#define SMC_WSIZE GENMASK(31, 24) > +#define SMC_SIZE GENMASK(23, 16) > +#define SMC_ID GENMASK(15, 12) > +#define SMC_MSG GENMASK(7, 0) > +#define SMC_RESULT SMC_MSG > + > +#define SMC_RECV_TIMEOUT 100 > + > +struct apple_smc_rtkit { > + struct device *dev; > + struct apple_smc *core; > + struct apple_rtkit *rtk; > + > + struct completion init_done; > + bool initialized; > + bool alive; > + > + struct resource *sram; > + void __iomem *sram_base; > + struct apple_rtkit_shmem shmem; > + > + unsigned int msg_id; > + > + bool atomic_pending; > + struct completion cmd_done; > + u64 cmd_ret; > +}; > + > +static int apple_smc_rtkit_write_key_atomic(void *cookie, smc_key key, > void *buf, size_t size) > +{ > + struct apple_smc_rtkit *smc = cookie; > + int ret; > + u64 msg; > + u8 result; > + > + if (size > SMC_SHMEM_SIZE || size == 0) > + return -EINVAL; > + > + if (!smc->alive) > + return -EIO; > + > + memcpy_toio(smc->shmem.iomem, buf, size); > + smc->msg_id = (smc->msg_id + 1) & 0xf; > + msg = (FIELD_PREP(SMC_MSG, SMC_MSG_WRITE_KEY) | > + FIELD_PREP(SMC_SIZE, size) | > + FIELD_PREP(SMC_ID, smc->msg_id) | > + FIELD_PREP(SMC_DATA, key)); > + smc->atomic_pending = true; > + > + ret = apple_rtkit_send_message(smc->rtk, SMC_ENDPOINT, msg, NULL, > true); > + if (ret < 0) { > + dev_err(smc->dev, "Failed to send command (%d)\n", ret); > + return ret; > + } > + > + while (smc->atomic_pending) { > + ret = apple_rtkit_poll(smc->rtk); > + if (ret < 0) { > + dev_err(smc->dev, "RTKit poll failed (%llx)", msg); > + return ret; > + } > + udelay(100); > + } I guess we could use try_wait_for_completion here and get rid of the special smc->atomic_pending path. Not sure if it makes the code much simpler though. > + > + if (FIELD_GET(SMC_ID, smc->cmd_ret) != smc->msg_id) { > + dev_err(smc->dev, "Command sequence mismatch (expected %d, got > %d)\n", > + smc->msg_id, (unsigned int)FIELD_GET(SMC_ID, smc->cmd_ret)); > + return -EIO; > + } > + > + result = FIELD_GET(SMC_RESULT, smc->cmd_ret); > + if (result != 0) > + return -result; > + > + return FIELD_GET(SMC_SIZE, smc->cmd_ret); > +} > + > +static int apple_smc_cmd(struct apple_smc_rtkit *smc, u64 cmd, u64 arg, > + u64 size, u64 wsize, u32 *ret_data) > +{ > + int ret; > + u64 msg; > + u8 result; > + > + if (!smc->alive) > + return -EIO; > + > + reinit_completion(&smc->cmd_done); > + > + smc->msg_id = (smc->msg_id + 1) & 0xf; > + msg = (FIELD_PREP(SMC_MSG, cmd) | > + FIELD_PREP(SMC_SIZE, size) | > + FIELD_PREP(SMC_WSIZE, wsize) | > + FIELD_PREP(SMC_ID, smc->msg_id) | > + FIELD_PREP(SMC_DATA, arg)); > + > + ret = apple_rtkit_send_message(smc->rtk, SMC_ENDPOINT, msg, NULL, > false); > + if (ret < 0) { > + dev_err(smc->dev, "Failed to send command\n"); > + return ret; > + } > + > + do { > + if (wait_for_completion_timeout(&smc->cmd_done, > + msecs_to_jiffies(SMC_RECV_TIMEOUT)) == 0) { > + dev_err(smc->dev, "Command timed out (%llx)", msg); > + return -ETIMEDOUT; > + } > + if (FIELD_GET(SMC_ID, smc->cmd_ret) == smc->msg_id) > + break; > + dev_err(smc->dev, "Command sequence mismatch (expected %d, got > %d)\n", > + smc->msg_id, (unsigned int)FIELD_GET(SMC_ID, smc->cmd_ret)); > + } while(1); > + > + result = FIELD_GET(SMC_RESULT, smc->cmd_ret); > + if (result != 0) > + return -result; > + > + if (ret_data) > + *ret_data = FIELD_GET(SMC_DATA, smc->cmd_ret); > + > + return FIELD_GET(SMC_SIZE, smc->cmd_ret); > +} > + > +static int _apple_smc_rtkit_read_key(struct apple_smc_rtkit *smc, > smc_key key, > + void *buf, size_t size, size_t wsize) > +{ > + int ret; > + u32 rdata; > + u64 cmd; > + > + if (size > SMC_SHMEM_SIZE || size == 0) > + return -EINVAL; > + > + cmd = wsize ? SMC_MSG_RW_KEY : SMC_MSG_READ_KEY; > + > + ret = apple_smc_cmd(smc, cmd, key, size, wsize, &rdata); > + if (ret < 0) > + return ret; > + > + if (size <= 4) > + memcpy(buf, &rdata, size); > + else > + memcpy_fromio(buf, smc->shmem.iomem, size); > + > + return ret; > +} > + > +static int apple_smc_rtkit_read_key(void *cookie, smc_key key, void > *buf, size_t size) > +{ > + return _apple_smc_rtkit_read_key(cookie, key, buf, size, 0); > +} > + > +static int apple_smc_rtkit_write_key(void *cookie, smc_key key, void > *buf, size_t size) > +{ > + struct apple_smc_rtkit *smc = cookie; > + > + if (size > SMC_SHMEM_SIZE || size == 0) > + return -EINVAL; > + > + memcpy_toio(smc->shmem.iomem, buf, size); > + return apple_smc_cmd(smc, SMC_MSG_WRITE_KEY, key, size, 0, NULL); > +} > + > +static int apple_smc_rtkit_rw_key(void *cookie, smc_key key, > + void *wbuf, size_t wsize, void *rbuf, size_t rsize) > +{ > + struct apple_smc_rtkit *smc = cookie; > + > + if (wsize > SMC_SHMEM_SIZE || wsize == 0) > + return -EINVAL; > + > + memcpy_toio(smc->shmem.iomem, wbuf, wsize); > + return _apple_smc_rtkit_read_key(smc, key, rbuf, rsize, wsize); > +} > + > +static int apple_smc_rtkit_get_key_by_index(void *cookie, int index, > smc_key *key) > +{ > + struct apple_smc_rtkit *smc = cookie; > + int ret; > + > + ret = apple_smc_cmd(smc, SMC_MSG_GET_KEY_BY_INDEX, index, 0, 0, key); > + > + *key = swab32(*key); > + return ret; > +} > + > +static int apple_smc_rtkit_get_key_info(void *cookie, smc_key key, > struct apple_smc_key_info *info) > +{ > + struct apple_smc_rtkit *smc = cookie; > + u8 key_info[6]; > + int ret; > + > + ret = apple_smc_cmd(smc, SMC_MSG_GET_KEY_INFO, key, 0, 0, NULL); > + if (ret >= 0 && info) { > + info->size = key_info[0]; > + info->type_code = get_unaligned_be32(&key_info[1]); > + info->flags = key_info[5]; > + } > + return ret; > +} > + > +static const struct apple_smc_backend_ops apple_smc_rtkit_be_ops = { > + .read_key = apple_smc_rtkit_read_key, > + .write_key = apple_smc_rtkit_write_key, > + .write_key_atomic = apple_smc_rtkit_write_key_atomic, > + .rw_key = apple_smc_rtkit_rw_key, > + .get_key_by_index = apple_smc_rtkit_get_key_by_index, > + .get_key_info = apple_smc_rtkit_get_key_info, > +}; > + > +static void apple_smc_rtkit_crashed(void *cookie) > +{ > + struct apple_smc_rtkit *smc = cookie; > + > + dev_err(smc->dev, "SMC crashed! Your system will reboot in a few > seconds...\n"); > + smc->alive = false; > +} > + > +static int apple_smc_rtkit_shmem_setup(void *cookie, struct > apple_rtkit_shmem *bfr) > +{ > + struct apple_smc_rtkit *smc = cookie; > + struct resource res = { > + .start = bfr->iova, > + .end = bfr->iova + bfr->size - 1, > + .name = "rtkit_map", > + .flags = smc->sram->flags, > + }; > + > + if (!bfr->iova) { > + dev_err(smc->dev, "RTKit wants a RAM buffer\n"); > + return -EIO; > + } > + > + if (res.end < res.start || !resource_contains(smc->sram, &res)) { > + dev_err(smc->dev, > + "RTKit buffer request outside SRAM region: %pR", &res); > + return -EFAULT; > + } > + > + bfr->iomem = smc->sram_base + (res.start - smc->sram->start); > + bfr->is_mapped = true; > + > + return 0; > +} > + > +static void apple_smc_rtkit_shmem_destroy(void *cookie, struct > apple_rtkit_shmem *bfr) > +{ > + // no-op > +} > + > +static bool apple_smc_rtkit_recv_early(void *cookie, u8 endpoint, u64 > message) > +{ > + struct apple_smc_rtkit *smc = cookie; > + > + if (endpoint != SMC_ENDPOINT) { > + dev_err(smc->dev, "Received message for unknown endpoint 0x%x\n", > endpoint); > + return false; > + } > + > + if (!smc->initialized) { > + int ret; > + > + smc->shmem.iova = message; > + smc->shmem.size = SMC_SHMEM_SIZE; > + ret = apple_smc_rtkit_shmem_setup(smc, &smc->shmem); > + if (ret < 0) > + dev_err(smc->dev, "Failed to initialize shared memory\n"); > + else > + smc->alive = true; > + smc->initialized = true; > + complete(&smc->init_done); > + } else if (FIELD_GET(SMC_MSG, message) == SMC_MSG_NOTIFICATION) { > + /* Handle these in the RTKit worker thread */ > + return false; > + } else { > + smc->cmd_ret = message; > + if (smc->atomic_pending) { > + smc->atomic_pending = false; > + } else { > + complete(&smc->cmd_done); > + } > + } > + > + return true; > +} > + > +static void apple_smc_rtkit_recv(void *cookie, u8 endpoint, u64 > message) > +{ > + struct apple_smc_rtkit *smc = cookie; > + > + if (endpoint != SMC_ENDPOINT) { > + dev_err(smc->dev, "Received message for unknown endpoint 0x%x\n", > endpoint); > + return; > + } > + > + if (FIELD_GET(SMC_MSG, message) != SMC_MSG_NOTIFICATION) { > + dev_err(smc->dev, "Received unknown message from worker: 0x%llx\n", > message); > + return; > + } > + > + apple_smc_event_received(smc->core, FIELD_GET(SMC_DATA, message)); > +} > + > +static const struct apple_rtkit_ops apple_smc_rtkit_ops = { > + .crashed = apple_smc_rtkit_crashed, > + .recv_message = apple_smc_rtkit_recv, > + .recv_message_early = apple_smc_rtkit_recv_early, > + .shmem_setup = apple_smc_rtkit_shmem_setup, > + .shmem_destroy = apple_smc_rtkit_shmem_destroy, > +}; > + > +static int apple_smc_rtkit_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct apple_smc_rtkit *smc; > + int ret; > + > + smc = devm_kzalloc(dev, sizeof(*smc), GFP_KERNEL); > + if (!smc) > + return -ENOMEM; > + > + smc->dev = dev; > + > + smc->sram = platform_get_resource_byname(pdev, IORESOURCE_MEM, > "sram"); > + if (!smc->sram) > + return dev_err_probe(dev, EIO, > + "No SRAM region"); > + > + smc->sram_base = devm_ioremap_resource(dev, smc->sram); > + if (IS_ERR(smc->sram_base)) > + return dev_err_probe(dev, PTR_ERR(smc->sram_base), > + "Failed to map SRAM region"); > + > + smc->rtk = > + devm_apple_rtkit_init(dev, smc, NULL, 0, &apple_smc_rtkit_ops); > + if (IS_ERR(smc->rtk)) > + return dev_err_probe(dev, PTR_ERR(smc->rtk), > + "Failed to intialize RTKit"); > + > + ret = apple_rtkit_wake(smc->rtk); > + if (ret != 0) > + return dev_err_probe(dev, ret, > + "Failed to wake up SMC"); > + > + ret = apple_rtkit_start_ep(smc->rtk, SMC_ENDPOINT); > + if (ret != 0) { > + dev_err(dev, "Failed to start endpoint"); > + goto cleanup; > + } > + > + init_completion(&smc->init_done); > + init_completion(&smc->cmd_done); > + > + ret = apple_rtkit_send_message(smc->rtk, SMC_ENDPOINT, > + FIELD_PREP(SMC_MSG, SMC_MSG_INITIALIZE), NULL, false); > + if (ret < 0) > + return dev_err_probe(dev, ret, > + "Failed to send init message"); This should probably also "goto cleanup" here just in case we somehow manage to send the shutdown message after this one failed. > + > + if (wait_for_completion_timeout(&smc->init_done, > + msecs_to_jiffies(SMC_RECV_TIMEOUT)) == 0) { > + ret = -ETIMEDOUT; > + dev_err(dev, "Timed out initializing SMC"); > + goto cleanup; > + } > + > + if (!smc->alive) { > + ret = -EIO; > + goto cleanup; > + } > + > + smc->core = apple_smc_probe(dev, &apple_smc_rtkit_be_ops, smc); > + if (IS_ERR(smc->core)) { > + ret = PTR_ERR(smc->core); > + goto cleanup; > + } > + > + return 0; > + > +cleanup: > + /* Try to shut down RTKit, if it's not completely wedged */ > + if (apple_rtkit_is_running(smc->rtk)) > + apple_rtkit_quiesce(smc->rtk); > + > + return ret; > +} > + > +static int apple_smc_rtkit_remove(struct platform_device *pdev) > +{ > + struct apple_smc *core = platform_get_drvdata(pdev); > + struct apple_smc_rtkit *smc = apple_smc_get_cookie(core); > + > + apple_smc_remove(core); > + > + if (apple_rtkit_is_running(smc->rtk)) > + apple_rtkit_quiesce(smc->rtk); > + > + return 0; > +} > + > +static const struct of_device_id apple_smc_rtkit_of_match[] = { > + { .compatible = "apple,smc" }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, apple_smc_rtkit_of_match); > + > +static struct platform_driver apple_smc_rtkit_driver = { > + .driver = { > + .name = "macsmc-rtkit", > + .owner = THIS_MODULE, > + .of_match_table = apple_smc_rtkit_of_match, > + }, > + .probe = apple_smc_rtkit_probe, > + .remove = apple_smc_rtkit_remove, > +}; > +module_platform_driver(apple_smc_rtkit_driver); > + > +MODULE_AUTHOR("Hector Martin <marcan@marcan.st>"); > +MODULE_LICENSE("Dual MIT/GPL"); > +MODULE_DESCRIPTION("Apple SMC RTKit backend driver"); > diff --git a/include/linux/mfd/macsmc.h b/include/linux/mfd/macsmc.h > new file mode 100644 > index 000000000000..39b4dc4ca881 > --- /dev/null > +++ b/include/linux/mfd/macsmc.h > @@ -0,0 +1,86 @@ > +// SPDX-License-Identifier: GPL-2.0-only OR MIT > +/* > + * Apple SMC core definitions > + * Copyright (C) The Asahi Linux Contributors > + */ > + > +#ifndef _LINUX_MFD_MACSMC_H > +#define _LINUX_MFD_MACSMC_H > + > +struct apple_smc; > + > +typedef u32 smc_key; > + > +#define SMC_KEY(s) (smc_key)(_SMC_KEY(#s)) > +#define _SMC_KEY(s) (((s)[0] << 24) | ((s)[1] << 16) | ((s)[2] << 8) | > (s)[3]) > + > +#define APPLE_SMC_READABLE BIT(7) > +#define APPLE_SMC_WRITABLE BIT(6) > +#define APPLE_SMC_FUNCTION BIT(4) > + > +struct apple_smc_key_info { > + u8 size; > + u32 type_code; > + u8 flags; > +}; > + > +int apple_smc_read(struct apple_smc *smc, smc_key key, void *buf, > size_t size); > +int apple_smc_write(struct apple_smc *smc, smc_key key, void *buf, > size_t size); > +int apple_smc_write_atomic(struct apple_smc *smc, smc_key key, void > *buf, size_t size); > +int apple_smc_rw(struct apple_smc *smc, smc_key key, void *wbuf, > size_t wsize, > + void *rbuf, size_t rsize); > + > +int apple_smc_get_key_count(struct apple_smc *smc); > +int apple_smc_find_first_key_index(struct apple_smc *smc, smc_key key); > +int apple_smc_get_key_by_index(struct apple_smc *smc, int index, > smc_key *key); > +int apple_smc_get_key_info(struct apple_smc *smc, smc_key key, struct > apple_smc_key_info *info); > + > +static inline bool apple_smc_key_exists(struct apple_smc *smc, smc_key > key) > +{ > + return apple_smc_get_key_info(smc, key, NULL) >= 0; > +} > + > +#define APPLE_SMC_TYPE_OPS(type) \ > + static inline int apple_smc_read_##type(struct apple_smc *smc, > smc_key key, type *p) \ > + { \ > + int ret = apple_smc_read(smc, key, p, sizeof(*p)); \ > + return (ret < 0) ? ret : ((ret != sizeof(*p)) ? -EINVAL : 0); \ > + } \ > + static inline int apple_smc_write_##type(struct apple_smc *smc, > smc_key key, type p) \ > + { \ > + return apple_smc_write(smc, key, &p, sizeof(p)); \ > + } \ > + static inline int apple_smc_write_##type##_atomic(struct apple_smc > *smc, smc_key key, type p) \ > + { \ > + return apple_smc_write_atomic(smc, key, &p, sizeof(p)); \ > + } \ > + static inline int apple_smc_rw_##type(struct apple_smc *smc, smc_key > key, \ > + type w, type *r) \ > + { \ > + int ret = apple_smc_rw(smc, key, &w, sizeof(w), r, sizeof(*r)); \ > + return (ret < 0) ? ret : ((ret != sizeof(*r)) ? -EINVAL : 0); \ > + } > + > +APPLE_SMC_TYPE_OPS(u64) > +APPLE_SMC_TYPE_OPS(u32) > +APPLE_SMC_TYPE_OPS(u16) > +APPLE_SMC_TYPE_OPS(u8) > +APPLE_SMC_TYPE_OPS(s64) > +APPLE_SMC_TYPE_OPS(s32) > +APPLE_SMC_TYPE_OPS(s16) > +APPLE_SMC_TYPE_OPS(s8) > + > +static inline int apple_smc_read_flag(struct apple_smc *smc, smc_key > key) > +{ > + u8 val; > + int ret = apple_smc_read_u8(smc, key, &val); > + if (ret < 0) > + return ret; > + return val ? 1 : 0; > +} > +#define apple_smc_write_flag apple_smc_write_u8 > + > +int apple_smc_register_notifier(struct apple_smc *smc, struct > notifier_block *n); > +int apple_smc_unregister_notifier(struct apple_smc *smc, struct > notifier_block *n); > + > +#endif > -- > 2.30.2 Sven
On Thu, Sep 1, 2022 at 5:18 PM Russell King <rmk+kernel@armlinux.org.uk> wrote: > > From: Hector Martin <marcan@marcan.st> > > This driver implements support for the SMC (System Management > Controller) in Apple Macs. In contrast to the existing applesmc driver, > it uses pluggable backends that allow it to support different SMC > implementations, and uses the MFD subsystem to expose the core SMC > functionality so that specific features (gpio, hwmon, battery, etc.) can > be implemented by separate drivers in their respective downstream > subsystems. > > The initial RTKit backend adds support for Apple Silicon Macs (M1 et > al). We hope a backend for T2 Macs will be written in the future > (since those are not supported by applesmc), and eventually an x86 > backend would allow us to fully deprecate applesmc in favor of this > driver. ... > drivers/platform/Kconfig | 2 + > drivers/platform/Makefile | 1 + > drivers/platform/apple/Kconfig | 49 ++++ > drivers/platform/apple/Makefile | 11 + Are you going to collect the code from, e.g., PDx86 which supports some apple devices here? ... > +EXPORT_SYMBOL(apple_smc_read); Can you from day 1 make it a namespaced variant? Ditto for the rest of the exported stuff. ... > +#include <asm/unaligned.h> Usually we put asm/* after linux/*. Missed bits.h. > +#include <linux/delay.h> > +#include <linux/device.h> > +#include <linux/io.h> > +#include <linux/ioport.h> > +#include <linux/of.h> > +#include <linux/of_platform.h> > +#include <linux/soc/apple/rtkit.h> ... > + smc->msg_id = (smc->msg_id + 1) & 0xf; % 16 will tell much cleaner of the purpose, no? ... > + while (smc->atomic_pending) { > + ret = apple_rtkit_poll(smc->rtk); > + if (ret < 0) { > + dev_err(smc->dev, "RTKit poll failed (%llx)", msg); > + return ret; > + } > + udelay(100); > + } Something from iopoll.h to be utilised? ... > + if (FIELD_GET(SMC_ID, smc->cmd_ret) != smc->msg_id) { > + dev_err(smc->dev, "Command sequence mismatch (expected %d, got %d)\n", > + smc->msg_id, (unsigned int)FIELD_GET(SMC_ID, smc->cmd_ret)); Why casting? > + return -EIO; > + } ... > + result = FIELD_GET(SMC_RESULT, smc->cmd_ret); > + if (result != 0) > + return -result; And this is in Linux error numbering space?! ... > + smc->msg_id = (smc->msg_id + 1) & 0xf; See above. Perhaps you need a macro / inline helper for this to avoid dups. ... > + do { > + if (wait_for_completion_timeout(&smc->cmd_done, > + msecs_to_jiffies(SMC_RECV_TIMEOUT)) == 0) { > + dev_err(smc->dev, "Command timed out (%llx)", msg); > + return -ETIMEDOUT; > + } > + if (FIELD_GET(SMC_ID, smc->cmd_ret) == smc->msg_id) > + break; > + dev_err(smc->dev, "Command sequence mismatch (expected %d, got %d)\n", > + smc->msg_id, (unsigned int)FIELD_GET(SMC_ID, smc->cmd_ret)); Guaranteed to flood the logs... > + } while(1); ...with such a conditional. ... > + result = FIELD_GET(SMC_RESULT, smc->cmd_ret); > + if (result != 0) > + return -result; Linux error numbering space? ... > + if (size <= 4) > + memcpy(buf, &rdata, size); > + else > + memcpy_fromio(buf, smc->shmem.iomem, size); This is unclear why plain memcpy() for the small size and what are the side effects of the memory. Maybe you wanted memremap() instead of ioremap() to begin with? ... > + *key = swab32(*key); swab32s() ... > + if (res.end < res.start || !resource_contains(smc->sram, &res)) { Is it a reimplementation of something like resource_intersect() and Co? > + dev_err(smc->dev, > + "RTKit buffer request outside SRAM region: %pR", &res); > + return -EFAULT; > + } ... > + bfr->iomem = smc->sram_base + (res.start - smc->sram->start); Isn't it better to write as res.start + (base - start) ? ... > + if (smc->atomic_pending) { > + smc->atomic_pending = false; > + } else { > + complete(&smc->cmd_done); > + } Redundant {} in both cases. ... > + smc->sram = platform_get_resource_byname(pdev, IORESOURCE_MEM, "sram"); > + if (!smc->sram) > + return dev_err_probe(dev, EIO, > + "No SRAM region"); Dup, the below does this message for you. > + smc->sram_base = devm_ioremap_resource(dev, smc->sram); > + if (IS_ERR(smc->sram_base)) > + return dev_err_probe(dev, PTR_ERR(smc->sram_base), > + "Failed to map SRAM region"); Don't we have devm_platform_ioremap_resource_byname() ? ... > + ret = apple_rtkit_wake(smc->rtk); > + if (ret != 0) Drop all these ' != 0' > + return dev_err_probe(dev, ret, > + "Failed to wake up SMC"); Why not on one line? ... > +static const struct of_device_id apple_smc_rtkit_of_match[] = { > + { .compatible = "apple,smc" }, > + {}, No comma for the terminator entry. > +}; ... > +static struct platform_driver apple_smc_rtkit_driver = { > + .driver = { > + .name = "macsmc-rtkit", > + .owner = THIS_MODULE, Unneeded dup. > + .of_match_table = apple_smc_rtkit_of_match, > + }, > + .probe = apple_smc_rtkit_probe, > + .remove = apple_smc_rtkit_remove, > +}; ... > +typedef u32 smc_key; Why?! ... > +#define _SMC_KEY(s) (((s)[0] << 24) | ((s)[1] << 16) | ((s)[2] << 8) | (s)[3]) If s is a byte buffer, the above is NIH get_unaligned_be32(). Or in case of alignment be32_to_cpu() with respective type (__be32) to be used. ... > +static inline int apple_smc_read_flag(struct apple_smc *smc, smc_key key) > +{ > + u8 val; > + int ret = apple_smc_read_u8(smc, key, &val); Split assignment and definition. > + if (ret < 0) > + return ret; > + return val ? 1 : 0; > +} ... > +#define apple_smc_write_flag apple_smc_write_u8 Why is it needed?
On Thu, Sep 1, 2022, at 21:26, Andy Shevchenko wrote: > On Thu, Sep 1, 2022 at 5:18 PM Russell King <rmk+kernel@armlinux.org.uk> wrote: [...] > > ... > >> + if (size <= 4) >> + memcpy(buf, &rdata, size); >> + else >> + memcpy_fromio(buf, smc->shmem.iomem, size); > > This is unclear why plain memcpy() for the small size and what are the > side effects of the memory. Maybe you wanted memremap() instead of > ioremap() to begin with? rdata is used for small buffers, comes directly as part of a message sent on the underlying hardware FIFO and is on the stack (and thus mapped as as Normal memory) while smc->shmem.iomem has to be Device-nGnRnE. Mapping it with any other attributes will generate SErrors for each access. Sven
On Thu, Sep 01, 2022 at 07:50:31PM +0200, Sven Peter wrote: > Thanks for trying to upstream this! > I just have three minor comments: > > On Thu, Sep 1, 2022, at 15:54, Russell King wrote: > > + dev_info(dev, "Initialized (%d keys %p4ch..%p4ch)\n", > > + smc->key_count, &smc->first_key, &smc->last_key); > > I don't think upstream supports %p4ch. marcan added that format in our downstream > tree iirc. Before that can be resolved, we need the discussion about how to deal with SMC keys to be resolved. Andy was suggesting using the endian conversion macros in the GPIO driver to convert the hex XX value in the gPXX keys which doesn't sound right. If we changed the smc_key typedef to something like: typedef struct { char s[4]; } smc_key; Then we can safely print them using %.4s. We can also pass: key.s + 2 to hex2bin() since it will be in the correct order. We may need to do some fixups when placing the key in the SMC messages. If I'm reading the smc_rtkit code correctly, then apple_smc_rtkit_get_key_by_index() swabs the returned key from the SMC, but when we pass keys to the SMC, we don't swab them? So keys to the SMC are in reverse byte order compared to the natural string, but keys returned from the SMC are in conventional byte order for a string? Is that right, or am I getting confused? What I mean is that for a key ABCD, it's passed to the SMC as A in bits 63..56, B in 55..48, C in 47..40, D in 39..32. When we receive a key, A is in bits 7..0, B in bits 15..8, C in bits 23..16 and D in 31..24? > > +static int apple_smc_rtkit_write_key_atomic(void *cookie, smc_key key, > > void *buf, size_t size) > > +{ > > + struct apple_smc_rtkit *smc = cookie; > > + int ret; > > + u64 msg; > > + u8 result; > > + > > + if (size > SMC_SHMEM_SIZE || size == 0) > > + return -EINVAL; > > + > > + if (!smc->alive) > > + return -EIO; > > + > > + memcpy_toio(smc->shmem.iomem, buf, size); > > + smc->msg_id = (smc->msg_id + 1) & 0xf; > > + msg = (FIELD_PREP(SMC_MSG, SMC_MSG_WRITE_KEY) | > > + FIELD_PREP(SMC_SIZE, size) | > > + FIELD_PREP(SMC_ID, smc->msg_id) | > > + FIELD_PREP(SMC_DATA, key)); > > + smc->atomic_pending = true; > > + > > + ret = apple_rtkit_send_message(smc->rtk, SMC_ENDPOINT, msg, NULL, > > true); > > + if (ret < 0) { > > + dev_err(smc->dev, "Failed to send command (%d)\n", ret); > > + return ret; > > + } > > + > > + while (smc->atomic_pending) { > > + ret = apple_rtkit_poll(smc->rtk); > > + if (ret < 0) { > > + dev_err(smc->dev, "RTKit poll failed (%llx)", msg); > > + return ret; > > + } > > + udelay(100); > > + } > > I guess we could use try_wait_for_completion here and get rid of the special > smc->atomic_pending path. Not sure if it makes the code much simpler though. Delving into this, it seems this code is buggy. If apple_smc_rtkit_write_key_atomic() is used from atomic contexts, what prevents apple_smc_rtkit_write_key_atomic() being called while apple_smc_rtkit_write_key() is executing? Both these functions copy data to smc->shmem.iomem, so a call to the atomic function will overwrite the non-atomic function's data. Given that there doesn't seem to be a way to specify the offset of the data in shmem to the SMC, are we sure that we can sensibly support an atomic write operation? Using try_wait_for_completion() just ends up destroying more context in the non-atomic path. In any case, how can we be sure that the call to apple_smc_rtkit_recv_early() was really meant to complete the atomic write as opposed to something else? I guess that would trigger a "Command sequence mismatch" and an EIO error if it happens. Have these occurred? Lastly: #define SMC_SHMEM_SIZE 0x1000 #define SMC_WSIZE GENMASK(31, 24) #define SMC_SIZE GENMASK(23, 16) The size fields are one byte, but we error out if the size is larger than the shmem size: if (size > SMC_SHMEM_SIZE || size == 0) return -EINVAL; but we still try to squeeze the size into this byte-wide field: FIELD_PREP(SMC_SIZE, size) | which isn't goint to work. If the size is limited by the protocol to 255 bytes (or is it 256, where 0 means 256?) then surely we should be erroring out if size is above that maximum rather than the shmem size. > > +static int apple_smc_rtkit_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct apple_smc_rtkit *smc; > > + int ret; > > + > > + smc = devm_kzalloc(dev, sizeof(*smc), GFP_KERNEL); > > + if (!smc) > > + return -ENOMEM; > > + > > + smc->dev = dev; > > + > > + smc->sram = platform_get_resource_byname(pdev, IORESOURCE_MEM, > > "sram"); > > + if (!smc->sram) > > + return dev_err_probe(dev, EIO, > > + "No SRAM region"); > > + > > + smc->sram_base = devm_ioremap_resource(dev, smc->sram); > > + if (IS_ERR(smc->sram_base)) > > + return dev_err_probe(dev, PTR_ERR(smc->sram_base), > > + "Failed to map SRAM region"); > > + > > + smc->rtk = > > + devm_apple_rtkit_init(dev, smc, NULL, 0, &apple_smc_rtkit_ops); > > + if (IS_ERR(smc->rtk)) > > + return dev_err_probe(dev, PTR_ERR(smc->rtk), > > + "Failed to intialize RTKit"); > > + > > + ret = apple_rtkit_wake(smc->rtk); > > + if (ret != 0) > > + return dev_err_probe(dev, ret, > > + "Failed to wake up SMC"); > > + > > + ret = apple_rtkit_start_ep(smc->rtk, SMC_ENDPOINT); > > + if (ret != 0) { > > + dev_err(dev, "Failed to start endpoint"); > > + goto cleanup; > > + } > > + > > + init_completion(&smc->init_done); > > + init_completion(&smc->cmd_done); > > + > > + ret = apple_rtkit_send_message(smc->rtk, SMC_ENDPOINT, > > + FIELD_PREP(SMC_MSG, SMC_MSG_INITIALIZE), NULL, false); > > + if (ret < 0) > > + return dev_err_probe(dev, ret, > > + "Failed to send init message"); > > This should probably also "goto cleanup" here just in case we somehow manage to > send the shutdown message after this one failed. Good point, thanks.
On 02/09/2022 04.26, Andy Shevchenko wrote: > On Thu, Sep 1, 2022 at 5:18 PM Russell King <rmk+kernel@armlinux.org.uk> wrote: >> >> From: Hector Martin <marcan@marcan.st> >> >> This driver implements support for the SMC (System Management >> Controller) in Apple Macs. In contrast to the existing applesmc driver, >> it uses pluggable backends that allow it to support different SMC >> implementations, and uses the MFD subsystem to expose the core SMC >> functionality so that specific features (gpio, hwmon, battery, etc.) can >> be implemented by separate drivers in their respective downstream >> subsystems. >> >> The initial RTKit backend adds support for Apple Silicon Macs (M1 et >> al). We hope a backend for T2 Macs will be written in the future >> (since those are not supported by applesmc), and eventually an x86 >> backend would allow us to fully deprecate applesmc in favor of this >> driver. > > ... > >> drivers/platform/Kconfig | 2 + >> drivers/platform/Makefile | 1 + >> drivers/platform/apple/Kconfig | 49 ++++ >> drivers/platform/apple/Makefile | 11 + > > Are you going to collect the code from, e.g., PDx86 which supports > some apple devices here? This driver is intended to eventually supersede hwmon/applesmc.c, once it gets the missing features (hwmon in particular) and someone writes a legacy x86 backend. In the meantime, it is likely to first gain support for T2 machines, which applesmc.c does not have. >> + smc->msg_id = (smc->msg_id + 1) & 0xf; > > % 16 will tell much cleaner of the purpose, no? I disagree. msg_id goes in a bit field in SMC messages, and & 0xf perfectly conveys the idea that it is limited to 4 bits. > > ... > >> + while (smc->atomic_pending) { >> + ret = apple_rtkit_poll(smc->rtk); >> + if (ret < 0) { >> + dev_err(smc->dev, "RTKit poll failed (%llx)", msg); >> + return ret; >> + } >> + udelay(100); >> + } > > Something from iopoll.h to be utilised? No? Andy, I know you like to rapid-fire code reviews, but please read and understand the code you're reviewing with context, don't just fire off random comments based on gut feeling. This is calling through a framework that winds up in the mailbox code and then through a callback processes incoming messages. It's not an iopoll. > >> + return -EIO; >> + } > > ... > >> + result = FIELD_GET(SMC_RESULT, smc->cmd_ret); >> + if (result != 0) >> + return -result; > > And this is in Linux error numbering space?! It's in some random error numbering space, and who knows how we should map it. Maybe we should just return -EIO unconditionally, but that loses all available information... >> + smc->msg_id = (smc->msg_id + 1) & 0xf; > > See above. Perhaps you need a macro / inline helper for this to avoid dups. I really don't think incrementing a 4-bit counter is an idiom that kernel driver programmers are going to find confusing, and gratuituous macro use just makes the code harder to read. >> + do { >> + if (wait_for_completion_timeout(&smc->cmd_done, >> + msecs_to_jiffies(SMC_RECV_TIMEOUT)) == 0) { >> + dev_err(smc->dev, "Command timed out (%llx)", msg); >> + return -ETIMEDOUT; >> + } >> + if (FIELD_GET(SMC_ID, smc->cmd_ret) == smc->msg_id) >> + break; > >> + dev_err(smc->dev, "Command sequence mismatch (expected %d, got %d)\n", >> + smc->msg_id, (unsigned int)FIELD_GET(SMC_ID, smc->cmd_ret)); > > Guaranteed to flood the logs... No. > >> + } while(1); > > ...with such a conditional. It's waiting on a completion every loop. And the message ID is 4 bits, so the most you will ever get is 15 messages (but in practice what happens is that if this goes wrong, it just times out on the next loop, and generally everything breaks anyway, and all of it is indicative of a major bug. Again, please read the code, don't just go "dev_err() in a while(1), bad!". >> + if (size <= 4) >> + memcpy(buf, &rdata, size); >> + else >> + memcpy_fromio(buf, smc->shmem.iomem, size); > > This is unclear why plain memcpy() for the small size and what are the > side effects of the memory. It's on the stack. Can you *please* read more than two lines of context while reviewing? This is getting tiring. You do this *all* the time. It's even taking the address of the variable right there, you didn't even need to read past that one line to understand it. > Maybe you wanted memremap() instead of > ioremap() to begin with? ioremap() uses a completely different mapping mode to memremap(). They are not interchangeable and memremap will not work here. >> + if (res.end < res.start || !resource_contains(smc->sram, &res)) { > > Is it a reimplementation of something like resource_intersect() and Co? I don't know, is it? Can you please make specific suggestions instead of randomly throwing around ideas without checking whether they actually make sense? >> + bfr->iomem = smc->sram_base + (res.start - smc->sram->start); > > Isn't it better to write as > > res.start + (base - start) > > ? smc->sram_base is the iomem base of the sram, smc->sram->start is the physical address of the sram, and res.start is the physical address of the area the SMC is referencing. __iomem + (physaddr - physaddr) == __iomem + offset == __iomem You are proposing: physaddr + (__iomem - physaddr) which not only does not make sense, it's undefined behavior in C, since you are doing pointer arithmetic exceeding the object bounds of the iomem map. >> + smc->sram = platform_get_resource_byname(pdev, IORESOURCE_MEM, "sram"); > >> + if (!smc->sram) >> + return dev_err_probe(dev, EIO, >> + "No SRAM region"); > > Dup, the below does this message for you. There is a difference between "the DT is missing the SRAM resource" and "couldn't map the SRAM resource for some reason". Why merge them into one error? > >> + smc->sram_base = devm_ioremap_resource(dev, smc->sram); >> + if (IS_ERR(smc->sram_base)) >> + return dev_err_probe(dev, PTR_ERR(smc->sram_base), >> + "Failed to map SRAM region"); > > Don't we have devm_platform_ioremap_resource_byname() ? Except I use smc->sram elsewhere. You cannot review patches by ignoring everything but the two lines of context you are staring at right now. Before complaining about something, please *look* to see if there is a good reason for the code to be the way it is. > ... > >> +typedef u32 smc_key; > > Why?! Because SMC keys are 32 bits and giving them their own type makes it explicit what is supposed to be an SMC key. > >> +#define _SMC_KEY(s) (((s)[0] << 24) | ((s)[1] << 16) | ((s)[2] << 8) | (s)[3]) > > If s is a byte buffer, the above is NIH get_unaligned_be32(). Or in > case of alignment be32_to_cpu() with respective type (__be32) to be > used. Except this is a constexpr, and get_unaligned_be32 is not. s is a 32-bit identifier that happens to consist of 4 8-bit character ASCII fields. This is a macro to generate them from ASCII strings, as compile-time constants. You'd have figured that out if you read ahead to where it is used, instead of continuing your two-line-context review. >> +#define apple_smc_write_flag apple_smc_write_u8 > > Why is it needed? Because some SMC keys are boolean flags, and the ABI is identical to u8 but the logical semantics are not, and the code is clearer when it makes it explicit that the value is supposed to be a boolean, not just an arbitrary 8-bit integer. Andy, no offense, but you drive-by everything I try to upstream (or author in this case) and half of your suggestions are wrong and I have to waste my time explaining why, and most of the rest are negligible style nitpicks. Every now and then you point out some useful kernel function that I didn't know about, but your signal to noise rate is terrible. Please put some effort into your reviews. It feels like you're on some kind of quest to review as much code as possible, without the slightest care for quality. - Hector
On Mon, Sep 5, 2022 at 5:45 PM Hector Martin <marcan@marcan.st> wrote: > On 02/09/2022 04.26, Andy Shevchenko wrote: ... > Andy, no offense, but you drive-by everything I try to upstream (or > author in this case) and half of your suggestions are wrong and I have > to waste my time explaining why, and most of the rest are negligible > style nitpicks. Every now and then you point out some useful kernel > function that I didn't know about, but your signal to noise rate is > terrible. Please put some effort into your reviews. It feels like you're > on some kind of quest to review as much code as possible, without the > slightest care for quality. Okay, if I ever review your code in the future, I'll try my best.
On 05/09/2022 19.55, Russell King (Oracle) wrote: > Delving into this, it seems this code is buggy. > > If apple_smc_rtkit_write_key_atomic() is used from atomic contexts, > what prevents apple_smc_rtkit_write_key_atomic() being called while > apple_smc_rtkit_write_key() is executing? This does: > +int apple_smc_write_atomic(struct apple_smc *smc, smc_key key, void *buf, size_t size) > +{ > + int ret; > + > + /* > + * Will fail if SMC is busy. This is only used by SMC reboot/poweroff > + * final calls, so it doesn't really matter at that point. > + */ > + if (!mutex_trylock(&smc->mutex)) > + return -EBUSY; > + > + ret = smc->be->write_key_atomic(smc->be_cookie, key, buf, size); > + mutex_unlock(&smc->mutex); > + > + return ret; > +} > +EXPORT_SYMBOL(apple_smc_write_atomic); I tried making this actually work properly while I was considering using the atomic call in non-shutdown contexts, but gave up - it gets very hairy, and is probably impossible under the premise that the atomic side can always block on the non-atomic side and end up deadlocking due to the nature of things. But right now, since this is only ever used for final shutdown, there is no contention possible. The mutex_trylock there is a safety, in case someone tries using this in some other context, that will just bail if there is contention with non-atomic acesses. Maybe there should be a warning there, something like "SMC: contention between atomic accesses and non-atomic accesses is not supported". > Lastly: > > #define SMC_SHMEM_SIZE 0x1000 > > #define SMC_WSIZE GENMASK(31, 24) > #define SMC_SIZE GENMASK(23, 16) > > The size fields are one byte, but we error out if the size is larger > than the shmem size: > > if (size > SMC_SHMEM_SIZE || size == 0) > return -EINVAL; > > but we still try to squeeze the size into this byte-wide field: > > FIELD_PREP(SMC_SIZE, size) | > > which isn't goint to work. If the size is limited by the protocol to > 255 bytes (or is it 256, where 0 means 256?) then surely we should be > erroring out if size is above that maximum rather than the shmem size. Good point, this should be 255 bytes max. *checks* Yeah, the longest SMC key we have in practice is 120 bytes, so let's just #define SMC_MAX_SIZE 255 or something like that. - Hector
On Thu, 01 Sep 2022, Russell King wrote: > From: Hector Martin <marcan@marcan.st> > > This driver implements support for the SMC (System Management > Controller) in Apple Macs. In contrast to the existing applesmc driver, > it uses pluggable backends that allow it to support different SMC > implementations, and uses the MFD subsystem to expose the core SMC > functionality so that specific features (gpio, hwmon, battery, etc.) can > be implemented by separate drivers in their respective downstream > subsystems. > > The initial RTKit backend adds support for Apple Silicon Macs (M1 et > al). We hope a backend for T2 Macs will be written in the future > (since those are not supported by applesmc), and eventually an x86 > backend would allow us to fully deprecate applesmc in favor of this > driver. > > Signed-off-by: Hector Martin <marcan@marcan.st> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > --- > drivers/platform/Kconfig | 2 + > drivers/platform/Makefile | 1 + > drivers/platform/apple/Kconfig | 49 ++++ > drivers/platform/apple/Makefile | 11 + > drivers/platform/apple/smc.h | 28 ++ > drivers/platform/apple/smc_core.c | 249 ++++++++++++++++ > drivers/platform/apple/smc_rtkit.c | 451 +++++++++++++++++++++++++++++ > include/linux/mfd/macsmc.h | 86 ++++++ > 8 files changed, 877 insertions(+) > create mode 100644 drivers/platform/apple/Kconfig > create mode 100644 drivers/platform/apple/Makefile > create mode 100644 drivers/platform/apple/smc.h > create mode 100644 drivers/platform/apple/smc_core.c > create mode 100644 drivers/platform/apple/smc_rtkit.c > create mode 100644 include/linux/mfd/macsmc.h > > diff --git a/drivers/platform/Kconfig b/drivers/platform/Kconfig > index b437847b6237..5f8b9bcdb830 100644 > --- a/drivers/platform/Kconfig > +++ b/drivers/platform/Kconfig > @@ -13,4 +13,6 @@ source "drivers/platform/olpc/Kconfig" > > source "drivers/platform/surface/Kconfig" > > +source "drivers/platform/apple/Kconfig" > + > source "drivers/platform/x86/Kconfig" > diff --git a/drivers/platform/Makefile b/drivers/platform/Makefile > index 4de08ef4ec9d..3e5d5039a28c 100644 > --- a/drivers/platform/Makefile > +++ b/drivers/platform/Makefile > @@ -10,3 +10,4 @@ obj-$(CONFIG_OLPC_EC) += olpc/ > obj-$(CONFIG_GOLDFISH) += goldfish/ > obj-$(CONFIG_CHROME_PLATFORMS) += chrome/ > obj-$(CONFIG_SURFACE_PLATFORMS) += surface/ > +obj-$(CONFIG_APPLE_PLATFORMS) += apple/ > diff --git a/drivers/platform/apple/Kconfig b/drivers/platform/apple/Kconfig > new file mode 100644 > index 000000000000..42525aa9fbbe > --- /dev/null > +++ b/drivers/platform/apple/Kconfig > @@ -0,0 +1,49 @@ > +# SPDX-License-Identifier: GPL-2.0 > +# > +# Apple Platform-Specific Drivers > +# > + > +menuconfig APPLE_PLATFORMS > + bool "Apple Mac Platform-Specific Device Drivers" > + default y > + help > + Say Y here to get to see options for platform-specific device drivers > + for Apple devices. This option alone does not add any kernel code. > + > + If you say N, all options in this submenu will be skipped and disabled. > + > +if APPLE_PLATFORMS > + > +config APPLE_SMC > + tristate "Apple SMC Driver" > + depends on ARCH_APPLE || COMPILE_TEST > + default ARCH_APPLE > + select MFD_CORE > + help > + Build support for the Apple System Management Controller present in > + Apple Macs. This driver currently supports the SMC in Apple Silicon > + Macs. For x86 Macs, see the applesmc driver (SENSORS_APPLESMC). > + > + Say Y here if you have an Apple Silicon Mac. > + > + To compile this driver as a module, choose M here: the module will > + be called macsmc. > + > +if APPLE_SMC > + > +config APPLE_SMC_RTKIT > + tristate "RTKit (Apple Silicon) backend" > + depends on ARCH_APPLE || COMPILE_TEST > + depends on APPLE_RTKIT > + default ARCH_APPLE > + help > + Build support for SMC communications via the RTKit backend. This is > + required for Apple Silicon Macs. > + > + Say Y here if you have an Apple Silicon Mac. > + > + To compile this driver as a module, choose M here: the module will > + be called macsmc-rtkit. > + > +endif > +endif > diff --git a/drivers/platform/apple/Makefile b/drivers/platform/apple/Makefile > new file mode 100644 > index 000000000000..79fac195398b > --- /dev/null > +++ b/drivers/platform/apple/Makefile > @@ -0,0 +1,11 @@ > +# SPDX-License-Identifier: GPL-2.0 > +# > +# Makefile for linux/drivers/platform/apple > +# Apple Platform-Specific Drivers > +# > + > +macsmc-y += smc_core.o > +macsmc-rtkit-y += smc_rtkit.o > + > +obj-$(CONFIG_APPLE_SMC) += macsmc.o > +obj-$(CONFIG_APPLE_SMC_RTKIT) += macsmc-rtkit.o > diff --git a/drivers/platform/apple/smc.h b/drivers/platform/apple/smc.h > new file mode 100644 > index 000000000000..8ae51887b2c5 > --- /dev/null > +++ b/drivers/platform/apple/smc.h > @@ -0,0 +1,28 @@ > +// SPDX-License-Identifier: GPL-2.0-only OR MIT > +/* > + * Apple SMC internal core definitions > + * Copyright (C) The Asahi Linux Contributors > + */ > + > +#ifndef _SMC_H > +#define _SMC_H > + > +#include <linux/mfd/macsmc.h> > + > +struct apple_smc_backend_ops { > + int (*read_key)(void *cookie, smc_key key, void *buf, size_t size); > + int (*write_key)(void *cookie, smc_key key, void *buf, size_t size); > + int (*write_key_atomic)(void *cookie, smc_key key, void *buf, size_t size); > + int (*rw_key)(void *cookie, smc_key key, void *wbuf, size_t wsize, > + void *rbuf, size_t rsize); > + int (*get_key_by_index)(void *cookie, int index, smc_key *key); > + int (*get_key_info)(void *cookie, smc_key key, struct apple_smc_key_info *info); > +}; > + > +struct apple_smc *apple_smc_probe(struct device *dev, const struct apple_smc_backend_ops *ops, > + void *cookie); > +void *apple_smc_get_cookie(struct apple_smc *smc); > +int apple_smc_remove(struct apple_smc *smc); > +void apple_smc_event_received(struct apple_smc *smc, uint32_t event); > + > +#endif > diff --git a/drivers/platform/apple/smc_core.c b/drivers/platform/apple/smc_core.c > new file mode 100644 > index 000000000000..daf029cd072f > --- /dev/null > +++ b/drivers/platform/apple/smc_core.c > @@ -0,0 +1,249 @@ > +// SPDX-License-Identifier: GPL-2.0-only OR MIT > +/* > + * Apple SMC core framework > + * Copyright The Asahi Linux Contributors > + */ > + > +#include <linux/device.h> > +#include <linux/mfd/core.h> Please refrain from using the MFD API outside of drivers/mfd. If you need an MFD driver, please separate it out. If not, please replace it with the platform_*() API instead. Thanks.
On 08/09/2022 19.58, Lee Jones wrote: > On Thu, 01 Sep 2022, Russell King wrote: > >> From: Hector Martin <marcan@marcan.st> >> >> This driver implements support for the SMC (System Management >> Controller) in Apple Macs. In contrast to the existing applesmc driver, >> it uses pluggable backends that allow it to support different SMC >> implementations, and uses the MFD subsystem to expose the core SMC >> functionality so that specific features (gpio, hwmon, battery, etc.) can >> be implemented by separate drivers in their respective downstream >> subsystems. >> >> The initial RTKit backend adds support for Apple Silicon Macs (M1 et >> al). We hope a backend for T2 Macs will be written in the future >> (since those are not supported by applesmc), and eventually an x86 >> backend would allow us to fully deprecate applesmc in favor of this >> driver. >> >> Signed-off-by: Hector Martin <marcan@marcan.st> >> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> >> --- >> drivers/platform/Kconfig | 2 + >> drivers/platform/Makefile | 1 + >> drivers/platform/apple/Kconfig | 49 ++++ >> drivers/platform/apple/Makefile | 11 + >> drivers/platform/apple/smc.h | 28 ++ >> drivers/platform/apple/smc_core.c | 249 ++++++++++++++++ >> drivers/platform/apple/smc_rtkit.c | 451 +++++++++++++++++++++++++++++ >> include/linux/mfd/macsmc.h | 86 ++++++ >> 8 files changed, 877 insertions(+) >> create mode 100644 drivers/platform/apple/Kconfig >> create mode 100644 drivers/platform/apple/Makefile >> create mode 100644 drivers/platform/apple/smc.h >> create mode 100644 drivers/platform/apple/smc_core.c >> create mode 100644 drivers/platform/apple/smc_rtkit.c >> create mode 100644 include/linux/mfd/macsmc.h >> >> diff --git a/drivers/platform/Kconfig b/drivers/platform/Kconfig >> index b437847b6237..5f8b9bcdb830 100644 >> --- a/drivers/platform/Kconfig >> +++ b/drivers/platform/Kconfig >> @@ -13,4 +13,6 @@ source "drivers/platform/olpc/Kconfig" >> >> source "drivers/platform/surface/Kconfig" >> >> +source "drivers/platform/apple/Kconfig" >> + >> source "drivers/platform/x86/Kconfig" >> diff --git a/drivers/platform/Makefile b/drivers/platform/Makefile >> index 4de08ef4ec9d..3e5d5039a28c 100644 >> --- a/drivers/platform/Makefile >> +++ b/drivers/platform/Makefile >> @@ -10,3 +10,4 @@ obj-$(CONFIG_OLPC_EC) += olpc/ >> obj-$(CONFIG_GOLDFISH) += goldfish/ >> obj-$(CONFIG_CHROME_PLATFORMS) += chrome/ >> obj-$(CONFIG_SURFACE_PLATFORMS) += surface/ >> +obj-$(CONFIG_APPLE_PLATFORMS) += apple/ >> diff --git a/drivers/platform/apple/Kconfig b/drivers/platform/apple/Kconfig >> new file mode 100644 >> index 000000000000..42525aa9fbbe >> --- /dev/null >> +++ b/drivers/platform/apple/Kconfig >> @@ -0,0 +1,49 @@ >> +# SPDX-License-Identifier: GPL-2.0 >> +# >> +# Apple Platform-Specific Drivers >> +# >> + >> +menuconfig APPLE_PLATFORMS >> + bool "Apple Mac Platform-Specific Device Drivers" >> + default y >> + help >> + Say Y here to get to see options for platform-specific device drivers >> + for Apple devices. This option alone does not add any kernel code. >> + >> + If you say N, all options in this submenu will be skipped and disabled. >> + >> +if APPLE_PLATFORMS >> + >> +config APPLE_SMC >> + tristate "Apple SMC Driver" >> + depends on ARCH_APPLE || COMPILE_TEST >> + default ARCH_APPLE >> + select MFD_CORE >> + help >> + Build support for the Apple System Management Controller present in >> + Apple Macs. This driver currently supports the SMC in Apple Silicon >> + Macs. For x86 Macs, see the applesmc driver (SENSORS_APPLESMC). >> + >> + Say Y here if you have an Apple Silicon Mac. >> + >> + To compile this driver as a module, choose M here: the module will >> + be called macsmc. >> + >> +if APPLE_SMC >> + >> +config APPLE_SMC_RTKIT >> + tristate "RTKit (Apple Silicon) backend" >> + depends on ARCH_APPLE || COMPILE_TEST >> + depends on APPLE_RTKIT >> + default ARCH_APPLE >> + help >> + Build support for SMC communications via the RTKit backend. This is >> + required for Apple Silicon Macs. >> + >> + Say Y here if you have an Apple Silicon Mac. >> + >> + To compile this driver as a module, choose M here: the module will >> + be called macsmc-rtkit. >> + >> +endif >> +endif >> diff --git a/drivers/platform/apple/Makefile b/drivers/platform/apple/Makefile >> new file mode 100644 >> index 000000000000..79fac195398b >> --- /dev/null >> +++ b/drivers/platform/apple/Makefile >> @@ -0,0 +1,11 @@ >> +# SPDX-License-Identifier: GPL-2.0 >> +# >> +# Makefile for linux/drivers/platform/apple >> +# Apple Platform-Specific Drivers >> +# >> + >> +macsmc-y += smc_core.o >> +macsmc-rtkit-y += smc_rtkit.o >> + >> +obj-$(CONFIG_APPLE_SMC) += macsmc.o >> +obj-$(CONFIG_APPLE_SMC_RTKIT) += macsmc-rtkit.o >> diff --git a/drivers/platform/apple/smc.h b/drivers/platform/apple/smc.h >> new file mode 100644 >> index 000000000000..8ae51887b2c5 >> --- /dev/null >> +++ b/drivers/platform/apple/smc.h >> @@ -0,0 +1,28 @@ >> +// SPDX-License-Identifier: GPL-2.0-only OR MIT >> +/* >> + * Apple SMC internal core definitions >> + * Copyright (C) The Asahi Linux Contributors >> + */ >> + >> +#ifndef _SMC_H >> +#define _SMC_H >> + >> +#include <linux/mfd/macsmc.h> >> + >> +struct apple_smc_backend_ops { >> + int (*read_key)(void *cookie, smc_key key, void *buf, size_t size); >> + int (*write_key)(void *cookie, smc_key key, void *buf, size_t size); >> + int (*write_key_atomic)(void *cookie, smc_key key, void *buf, size_t size); >> + int (*rw_key)(void *cookie, smc_key key, void *wbuf, size_t wsize, >> + void *rbuf, size_t rsize); >> + int (*get_key_by_index)(void *cookie, int index, smc_key *key); >> + int (*get_key_info)(void *cookie, smc_key key, struct apple_smc_key_info *info); >> +}; >> + >> +struct apple_smc *apple_smc_probe(struct device *dev, const struct apple_smc_backend_ops *ops, >> + void *cookie); >> +void *apple_smc_get_cookie(struct apple_smc *smc); >> +int apple_smc_remove(struct apple_smc *smc); >> +void apple_smc_event_received(struct apple_smc *smc, uint32_t event); >> + >> +#endif >> diff --git a/drivers/platform/apple/smc_core.c b/drivers/platform/apple/smc_core.c >> new file mode 100644 >> index 000000000000..daf029cd072f >> --- /dev/null >> +++ b/drivers/platform/apple/smc_core.c >> @@ -0,0 +1,249 @@ >> +// SPDX-License-Identifier: GPL-2.0-only OR MIT >> +/* >> + * Apple SMC core framework >> + * Copyright The Asahi Linux Contributors >> + */ >> + >> +#include <linux/device.h> >> +#include <linux/mfd/core.h> > > Please refrain from using the MFD API outside of drivers/mfd. > > If you need an MFD driver, please separate it out. > > If not, please replace it with the platform_*() API instead. There is precedent for MFD devices under platform/: drivers/platform/x86/intel/int3472/tps68470.c As well as other examples in the tree: drivers/firmware/xilinx/zynqmp.c drivers/iio/common/ssp_sensors/ssp_dev.c drivers/misc/cardreader/alcor_pci.c drivers/misc/cardreader/rtsx_pcr.c drivers/misc/cardreader/rtsx_usb.c drivers/soc/samsung/exynos-pmu.c drivers/staging/nvec/nvec.c Since it's a driver for a platform-specific firmware service, I thought it made more sense in platform/ than mfd/. It's using the MFD API because the firmware exposes multiple subsystems, and this maps very nicely to the MFD model - NIHing that scaffolding would require a whole bunch of custom matching/device creation code. But it's not exactly your typical MFD device (it's not even a separate chip, it's part of the main SoC), so I'm not sure if it really belongs in mfd/ from an organizational standpoint? - Hector
On Thu, 08 Sep 2022, Hector Martin wrote: > On 08/09/2022 19.58, Lee Jones wrote: > > On Thu, 01 Sep 2022, Russell King wrote: > > > >> From: Hector Martin <marcan@marcan.st> > >> > >> This driver implements support for the SMC (System Management > >> Controller) in Apple Macs. In contrast to the existing applesmc driver, > >> it uses pluggable backends that allow it to support different SMC > >> implementations, and uses the MFD subsystem to expose the core SMC > >> functionality so that specific features (gpio, hwmon, battery, etc.) can > >> be implemented by separate drivers in their respective downstream > >> subsystems. > >> > >> The initial RTKit backend adds support for Apple Silicon Macs (M1 et > >> al). We hope a backend for T2 Macs will be written in the future > >> (since those are not supported by applesmc), and eventually an x86 > >> backend would allow us to fully deprecate applesmc in favor of this > >> driver. > >> > >> Signed-off-by: Hector Martin <marcan@marcan.st> > >> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > >> --- > >> drivers/platform/Kconfig | 2 + > >> drivers/platform/Makefile | 1 + > >> drivers/platform/apple/Kconfig | 49 ++++ > >> drivers/platform/apple/Makefile | 11 + > >> drivers/platform/apple/smc.h | 28 ++ > >> drivers/platform/apple/smc_core.c | 249 ++++++++++++++++ > >> drivers/platform/apple/smc_rtkit.c | 451 +++++++++++++++++++++++++++++ > >> include/linux/mfd/macsmc.h | 86 ++++++ > >> 8 files changed, 877 insertions(+) > >> create mode 100644 drivers/platform/apple/Kconfig > >> create mode 100644 drivers/platform/apple/Makefile > >> create mode 100644 drivers/platform/apple/smc.h > >> create mode 100644 drivers/platform/apple/smc_core.c > >> create mode 100644 drivers/platform/apple/smc_rtkit.c > >> create mode 100644 include/linux/mfd/macsmc.h > >> > >> diff --git a/drivers/platform/Kconfig b/drivers/platform/Kconfig > >> index b437847b6237..5f8b9bcdb830 100644 > >> --- a/drivers/platform/Kconfig > >> +++ b/drivers/platform/Kconfig > >> @@ -13,4 +13,6 @@ source "drivers/platform/olpc/Kconfig" > >> > >> source "drivers/platform/surface/Kconfig" > >> > >> +source "drivers/platform/apple/Kconfig" > >> + > >> source "drivers/platform/x86/Kconfig" > >> diff --git a/drivers/platform/Makefile b/drivers/platform/Makefile > >> index 4de08ef4ec9d..3e5d5039a28c 100644 > >> --- a/drivers/platform/Makefile > >> +++ b/drivers/platform/Makefile > >> @@ -10,3 +10,4 @@ obj-$(CONFIG_OLPC_EC) += olpc/ > >> obj-$(CONFIG_GOLDFISH) += goldfish/ > >> obj-$(CONFIG_CHROME_PLATFORMS) += chrome/ > >> obj-$(CONFIG_SURFACE_PLATFORMS) += surface/ > >> +obj-$(CONFIG_APPLE_PLATFORMS) += apple/ > >> diff --git a/drivers/platform/apple/Kconfig b/drivers/platform/apple/Kconfig > >> new file mode 100644 > >> index 000000000000..42525aa9fbbe > >> --- /dev/null > >> +++ b/drivers/platform/apple/Kconfig > >> @@ -0,0 +1,49 @@ > >> +# SPDX-License-Identifier: GPL-2.0 > >> +# > >> +# Apple Platform-Specific Drivers > >> +# > >> + > >> +menuconfig APPLE_PLATFORMS > >> + bool "Apple Mac Platform-Specific Device Drivers" > >> + default y > >> + help > >> + Say Y here to get to see options for platform-specific device drivers > >> + for Apple devices. This option alone does not add any kernel code. > >> + > >> + If you say N, all options in this submenu will be skipped and disabled. > >> + > >> +if APPLE_PLATFORMS > >> + > >> +config APPLE_SMC > >> + tristate "Apple SMC Driver" > >> + depends on ARCH_APPLE || COMPILE_TEST > >> + default ARCH_APPLE > >> + select MFD_CORE > >> + help > >> + Build support for the Apple System Management Controller present in > >> + Apple Macs. This driver currently supports the SMC in Apple Silicon > >> + Macs. For x86 Macs, see the applesmc driver (SENSORS_APPLESMC). > >> + > >> + Say Y here if you have an Apple Silicon Mac. > >> + > >> + To compile this driver as a module, choose M here: the module will > >> + be called macsmc. > >> + > >> +if APPLE_SMC > >> + > >> +config APPLE_SMC_RTKIT > >> + tristate "RTKit (Apple Silicon) backend" > >> + depends on ARCH_APPLE || COMPILE_TEST > >> + depends on APPLE_RTKIT > >> + default ARCH_APPLE > >> + help > >> + Build support for SMC communications via the RTKit backend. This is > >> + required for Apple Silicon Macs. > >> + > >> + Say Y here if you have an Apple Silicon Mac. > >> + > >> + To compile this driver as a module, choose M here: the module will > >> + be called macsmc-rtkit. > >> + > >> +endif > >> +endif > >> diff --git a/drivers/platform/apple/Makefile b/drivers/platform/apple/Makefile > >> new file mode 100644 > >> index 000000000000..79fac195398b > >> --- /dev/null > >> +++ b/drivers/platform/apple/Makefile > >> @@ -0,0 +1,11 @@ > >> +# SPDX-License-Identifier: GPL-2.0 > >> +# > >> +# Makefile for linux/drivers/platform/apple > >> +# Apple Platform-Specific Drivers > >> +# > >> + > >> +macsmc-y += smc_core.o > >> +macsmc-rtkit-y += smc_rtkit.o > >> + > >> +obj-$(CONFIG_APPLE_SMC) += macsmc.o > >> +obj-$(CONFIG_APPLE_SMC_RTKIT) += macsmc-rtkit.o > >> diff --git a/drivers/platform/apple/smc.h b/drivers/platform/apple/smc.h > >> new file mode 100644 > >> index 000000000000..8ae51887b2c5 > >> --- /dev/null > >> +++ b/drivers/platform/apple/smc.h > >> @@ -0,0 +1,28 @@ > >> +// SPDX-License-Identifier: GPL-2.0-only OR MIT > >> +/* > >> + * Apple SMC internal core definitions > >> + * Copyright (C) The Asahi Linux Contributors > >> + */ > >> + > >> +#ifndef _SMC_H > >> +#define _SMC_H > >> + > >> +#include <linux/mfd/macsmc.h> > >> + > >> +struct apple_smc_backend_ops { > >> + int (*read_key)(void *cookie, smc_key key, void *buf, size_t size); > >> + int (*write_key)(void *cookie, smc_key key, void *buf, size_t size); > >> + int (*write_key_atomic)(void *cookie, smc_key key, void *buf, size_t size); > >> + int (*rw_key)(void *cookie, smc_key key, void *wbuf, size_t wsize, > >> + void *rbuf, size_t rsize); > >> + int (*get_key_by_index)(void *cookie, int index, smc_key *key); > >> + int (*get_key_info)(void *cookie, smc_key key, struct apple_smc_key_info *info); > >> +}; > >> + > >> +struct apple_smc *apple_smc_probe(struct device *dev, const struct apple_smc_backend_ops *ops, > >> + void *cookie); > >> +void *apple_smc_get_cookie(struct apple_smc *smc); > >> +int apple_smc_remove(struct apple_smc *smc); > >> +void apple_smc_event_received(struct apple_smc *smc, uint32_t event); > >> + > >> +#endif > >> diff --git a/drivers/platform/apple/smc_core.c b/drivers/platform/apple/smc_core.c > >> new file mode 100644 > >> index 000000000000..daf029cd072f > >> --- /dev/null > >> +++ b/drivers/platform/apple/smc_core.c > >> @@ -0,0 +1,249 @@ > >> +// SPDX-License-Identifier: GPL-2.0-only OR MIT > >> +/* > >> + * Apple SMC core framework > >> + * Copyright The Asahi Linux Contributors > >> + */ > >> + > >> +#include <linux/device.h> > >> +#include <linux/mfd/core.h> > > > > Please refrain from using the MFD API outside of drivers/mfd. > > > > If you need an MFD driver, please separate it out. > > > > If not, please replace it with the platform_*() API instead. > > There is precedent for MFD devices under platform/: > > drivers/platform/x86/intel/int3472/tps68470.c > > As well as other examples in the tree: > > drivers/firmware/xilinx/zynqmp.c > drivers/iio/common/ssp_sensors/ssp_dev.c > drivers/misc/cardreader/alcor_pci.c > drivers/misc/cardreader/rtsx_pcr.c > drivers/misc/cardreader/rtsx_usb.c > drivers/soc/samsung/exynos-pmu.c > drivers/staging/nvec/nvec.c I'm aware of the previous offences. They each slipped-in before I could catch them. Ideally I'd like to reverse the act. However, finding time for such low-priority activities has proved challenging. I only noticed *this* occurrence due to the MFD header file. > Since it's a driver for a platform-specific firmware service, I thought > it made more sense in platform/ than mfd/. It's using the MFD API > because the firmware exposes multiple subsystems, and this maps very > nicely to the MFD model - NIHing that scaffolding would require a whole > bunch of custom matching/device creation code. I'm also aware of the convenience value of {ab}using the MFD API. > But it's not exactly your > typical MFD device (it's not even a separate chip, it's part of the main > SoC), so I'm not sure if it really belongs in mfd/ from an > organizational standpoint? There is a strong argument for all SoCs to be classed as (massive) MFDs. However seeing as they represent more of a whole platform, rather than an add-on chip, we have had the sense to represent them differently. Some such submissions I have made explicit requests to be moved form drivers/mfd *into* drivers/platform in the past. Most SoCs are solely represented in DT, omitting the requirement for subsequent device registration. Have you considered this? If so, why does this not suit your use-case? The long and the short of it is; if you wish to treat this device, or at least a section of it, as a type of MFD, then please draft that part of it as an MFD driver, residing in drivers/mfd. If it's "not really an MFD", then find another way to represent the conglomeration please. If the MFD route is the best, then you can register each of the devices, including the *-core from drivers/mfd. Grep for "cross-ec" as a relatively recently good example.
On 08/09/2022 21.31, Lee Jones wrote: > On Thu, 08 Sep 2022, Hector Martin wrote: >> But it's not exactly your >> typical MFD device (it's not even a separate chip, it's part of the main >> SoC), so I'm not sure if it really belongs in mfd/ from an >> organizational standpoint? > > There is a strong argument for all SoCs to be classed as (massive) > MFDs. However seeing as they represent more of a whole platform, > rather than an add-on chip, we have had the sense to represent them > differently. Some such submissions I have made explicit requests to be > moved form drivers/mfd *into* drivers/platform in the past. > > Most SoCs are solely represented in DT, omitting the requirement for > subsequent device registration. Have you considered this? If so, why > does this not suit your use-case? This driver and a subset of its sub-drivers are intended to generalize (via different backends, but no changes to sub-drivers) to legacy and T2 Mac platforms, eventually superseding applesmc (which is in hwmon/ because it started out as that, but has now grown random features and is quite a mess). Those are are Intel/UEFI machines and not DT platforms, and on those the SMC is actually a separate chip in some form (much like an EC). > The long and the short of it is; if you wish to treat this device, or > at least a section of it, as a type of MFD, then please draft that > part of it as an MFD driver, residing in drivers/mfd. If it's "not > really an MFD", then find another way to represent the conglomeration > please. > > If the MFD route is the best, then you can register each of the > devices, including the *-core from drivers/mfd. Grep for "cross-ec" > as a relatively recently good example. I think cros-ec is similar enough, yeah. As long as you don't mind having the core codebase in mfd/ (4 files: core, rtkit backend, and future T2 and legacy backends) we can do that. - Hector
On Thu, Sep 8, 2022 at 2:58 PM Hector Martin <marcan@marcan.st> wrote: > > If the MFD route is the best, then you can register each of the > > devices, including the *-core from drivers/mfd. Grep for "cross-ec" > > as a relatively recently good example. > > I think cros-ec is similar enough, yeah. As long as you don't mind > having the core codebase in mfd/ (4 files: core, rtkit backend, and > future T2 and legacy backends) we can do that. I'd even suggest its own subfolder under MFD, if you already know that there will likely be more than these 4 files. Though nothing else has a subdirectory in MFD right now so maybe Lee should approve of that first... The obvious upside to using MFD is that it has an active maintainer and patches get reviewed and merged swiftly, whereas drivers/platform is usually managed by SoC and x86 tip maintainers and what not. Yours, Linus Walleij
On Thu, 08 Sep 2022, Hector Martin wrote: > On 08/09/2022 21.31, Lee Jones wrote: > > On Thu, 08 Sep 2022, Hector Martin wrote: > >> But it's not exactly your > >> typical MFD device (it's not even a separate chip, it's part of the main > >> SoC), so I'm not sure if it really belongs in mfd/ from an > >> organizational standpoint? > > > > There is a strong argument for all SoCs to be classed as (massive) > > MFDs. However seeing as they represent more of a whole platform, > > rather than an add-on chip, we have had the sense to represent them > > differently. Some such submissions I have made explicit requests to be > > moved form drivers/mfd *into* drivers/platform in the past. > > > > Most SoCs are solely represented in DT, omitting the requirement for > > subsequent device registration. Have you considered this? If so, why > > does this not suit your use-case? > > This driver and a subset of its sub-drivers are intended to generalize > (via different backends, but no changes to sub-drivers) to legacy and T2 > Mac platforms, eventually superseding applesmc (which is in hwmon/ > because it started out as that, but has now grown random features and is > quite a mess). Those are are Intel/UEFI machines and not DT platforms, > and on those the SMC is actually a separate chip in some form (much like > an EC). Understood. Fair point. > > The long and the short of it is; if you wish to treat this device, or > > at least a section of it, as a type of MFD, then please draft that > > part of it as an MFD driver, residing in drivers/mfd. If it's "not > > really an MFD", then find another way to represent the conglomeration > > please. > > > > If the MFD route is the best, then you can register each of the > > devices, including the *-core from drivers/mfd. Grep for "cross-ec" > > as a relatively recently good example. > > I think cros-ec is similar enough, yeah. As long as you don't mind > having the core codebase in mfd/ (4 files: core, rtkit backend, and > future T2 and legacy backends) we can do that. That's actually not what I'm suggesting. You *only* need to move the subsequent-device-registration handling into drivers/mfd. The remainder really should be treated as Platform (not to be confused with Arch Platform) code and should reside in drivers/platform. Just as we do with cros-ec.
On 08/09/2022 22.36, Lee Jones wrote: > On Thu, 08 Sep 2022, Hector Martin wrote: > >> On 08/09/2022 21.31, Lee Jones wrote: >>> The long and the short of it is; if you wish to treat this device, or >>> at least a section of it, as a type of MFD, then please draft that >>> part of it as an MFD driver, residing in drivers/mfd. If it's "not >>> really an MFD", then find another way to represent the conglomeration >>> please. >>> >>> If the MFD route is the best, then you can register each of the >>> devices, including the *-core from drivers/mfd. Grep for "cross-ec" >>> as a relatively recently good example. >> >> I think cros-ec is similar enough, yeah. As long as you don't mind >> having the core codebase in mfd/ (4 files: core, rtkit backend, and >> future T2 and legacy backends) we can do that. > > That's actually not what I'm suggesting. > > You *only* need to move the subsequent-device-registration handling > into drivers/mfd. The remainder really should be treated as Platform > (not to be confused with Arch Platform) code and should reside in > drivers/platform. Just as we do with cros-ec. That's... an interesting approach. Is the code in drivers/mfd supposed to be a subdevice itself? That seems to be what's going on with cros_ec_dev.c, but do we really need that layer of indirection? What's the point of just having effectively an array of mfd_cell and wrappers to call into the mfd core in the drivers/mfd/ tree and the rest of the driver elsewhere? - Hector
On Thu, 08 Sep 2022, Hector Martin wrote: > On 08/09/2022 22.36, Lee Jones wrote: > > On Thu, 08 Sep 2022, Hector Martin wrote: > > > >> On 08/09/2022 21.31, Lee Jones wrote: > >>> The long and the short of it is; if you wish to treat this device, or > >>> at least a section of it, as a type of MFD, then please draft that > >>> part of it as an MFD driver, residing in drivers/mfd. If it's "not > >>> really an MFD", then find another way to represent the conglomeration > >>> please. > >>> > >>> If the MFD route is the best, then you can register each of the > >>> devices, including the *-core from drivers/mfd. Grep for "cross-ec" > >>> as a relatively recently good example. > >> > >> I think cros-ec is similar enough, yeah. As long as you don't mind > >> having the core codebase in mfd/ (4 files: core, rtkit backend, and > >> future T2 and legacy backends) we can do that. > > > > That's actually not what I'm suggesting. > > > > You *only* need to move the subsequent-device-registration handling > > into drivers/mfd. The remainder really should be treated as Platform > > (not to be confused with Arch Platform) code and should reside in > > drivers/platform. Just as we do with cros-ec. > > That's... an interesting approach. How you decide to initially architect it would be your choice. We can then discuss any potential improvements / suggestions. > Is the code in drivers/mfd supposed > to be a subdevice itself? That seems to be what's going on with > cros_ec_dev.c, but do we really need that layer of indirection? Ideally not. The evolution of cros-ec happened over many iterations and much time. Initially it was almost entirely implemented in drivers/mfd until I requested for a lot of the truly platform code to be moved out, as it grew beyond the bounds of, and was therefore no longer relevant to MFD. If we were to design and build it up again from scratch, I'd suggest that the MFD part would be the core-driver / entry-point. That driver should request and initialise shared resources and register the other devices, which is essentially the MFD's mantra. > What's the point of just having effectively an array of mfd_cell and > wrappers to call into the mfd core in the drivers/mfd/ tree and the > rest of the driver elsewhere? They should be separate drivers, with MFD registering the Platform.
On Fri, Sep 09, 2022 at 08:50:07AM +0100, Lee Jones wrote: > On Thu, 08 Sep 2022, Hector Martin wrote: > > > On 08/09/2022 22.36, Lee Jones wrote: > > > On Thu, 08 Sep 2022, Hector Martin wrote: > > > > > >> On 08/09/2022 21.31, Lee Jones wrote: > > >>> The long and the short of it is; if you wish to treat this device, or > > >>> at least a section of it, as a type of MFD, then please draft that > > >>> part of it as an MFD driver, residing in drivers/mfd. If it's "not > > >>> really an MFD", then find another way to represent the conglomeration > > >>> please. > > >>> > > >>> If the MFD route is the best, then you can register each of the > > >>> devices, including the *-core from drivers/mfd. Grep for "cross-ec" > > >>> as a relatively recently good example. > > >> > > >> I think cros-ec is similar enough, yeah. As long as you don't mind > > >> having the core codebase in mfd/ (4 files: core, rtkit backend, and > > >> future T2 and legacy backends) we can do that. > > > > > > That's actually not what I'm suggesting. > > > > > > You *only* need to move the subsequent-device-registration handling > > > into drivers/mfd. The remainder really should be treated as Platform > > > (not to be confused with Arch Platform) code and should reside in > > > drivers/platform. Just as we do with cros-ec. > > > > That's... an interesting approach. > > How you decide to initially architect it would be your choice. > > We can then discuss any potential improvements / suggestions. > > > Is the code in drivers/mfd supposed > > to be a subdevice itself? That seems to be what's going on with > > cros_ec_dev.c, but do we really need that layer of indirection? > > Ideally not. The evolution of cros-ec happened over many iterations > and much time. Initially it was almost entirely implemented in > drivers/mfd until I requested for a lot of the truly platform code to > be moved out, as it grew beyond the bounds of, and was therefore no > longer relevant to MFD. > > If we were to design and build it up again from scratch, I'd suggest > that the MFD part would be the core-driver / entry-point. That driver > should request and initialise shared resources and register the other > devices, which is essentially the MFD's mantra. > > > What's the point of just having effectively an array of mfd_cell and > > wrappers to call into the mfd core in the drivers/mfd/ tree and the > > rest of the driver elsewhere? > > They should be separate drivers, with MFD registering the Platform. I'm guessing this series is now dead, and Hector needs to re-spin the patch set according to your views. I'm guessing this is going to take a major re-work of the patch series. I suspect my attempt and trying to get this upstream has made things more complicated, because I doubt Hector has updated his patch set with the review comments that have been made so far... so this is now quite a mess. I think, once this is sorted, the entire series will need to be re-reviewed entirely afresh. I've also completely lost where I was in updating the patches with all the discussion on this posting of the patch set (which is why I posted v2, because I couldn't keep track of all the emails on this version.) When I posted v2, I had already lost track, which is why it got posted. Sorry.
On Mon, 12 Sep 2022, Russell King (Oracle) wrote: > On Fri, Sep 09, 2022 at 08:50:07AM +0100, Lee Jones wrote: > > On Thu, 08 Sep 2022, Hector Martin wrote: > > > > > On 08/09/2022 22.36, Lee Jones wrote: > > > > On Thu, 08 Sep 2022, Hector Martin wrote: > > > > > > > >> On 08/09/2022 21.31, Lee Jones wrote: > > > >>> The long and the short of it is; if you wish to treat this device, or > > > >>> at least a section of it, as a type of MFD, then please draft that > > > >>> part of it as an MFD driver, residing in drivers/mfd. If it's "not > > > >>> really an MFD", then find another way to represent the conglomeration > > > >>> please. > > > >>> > > > >>> If the MFD route is the best, then you can register each of the > > > >>> devices, including the *-core from drivers/mfd. Grep for "cross-ec" > > > >>> as a relatively recently good example. > > > >> > > > >> I think cros-ec is similar enough, yeah. As long as you don't mind > > > >> having the core codebase in mfd/ (4 files: core, rtkit backend, and > > > >> future T2 and legacy backends) we can do that. > > > > > > > > That's actually not what I'm suggesting. > > > > > > > > You *only* need to move the subsequent-device-registration handling > > > > into drivers/mfd. The remainder really should be treated as Platform > > > > (not to be confused with Arch Platform) code and should reside in > > > > drivers/platform. Just as we do with cros-ec. > > > > > > That's... an interesting approach. > > > > How you decide to initially architect it would be your choice. > > > > We can then discuss any potential improvements / suggestions. > > > > > Is the code in drivers/mfd supposed > > > to be a subdevice itself? That seems to be what's going on with > > > cros_ec_dev.c, but do we really need that layer of indirection? > > > > Ideally not. The evolution of cros-ec happened over many iterations > > and much time. Initially it was almost entirely implemented in > > drivers/mfd until I requested for a lot of the truly platform code to > > be moved out, as it grew beyond the bounds of, and was therefore no > > longer relevant to MFD. > > > > If we were to design and build it up again from scratch, I'd suggest > > that the MFD part would be the core-driver / entry-point. That driver > > should request and initialise shared resources and register the other > > devices, which is essentially the MFD's mantra. > > > > > What's the point of just having effectively an array of mfd_cell and > > > wrappers to call into the mfd core in the drivers/mfd/ tree and the > > > rest of the driver elsewhere? > > > > They should be separate drivers, with MFD registering the Platform. > > I'm guessing this series is now dead, and Hector needs to re-spin the > patch set according to your views. I'm guessing this is going to take > a major re-work of the patch series. > > I suspect my attempt and trying to get this upstream has made things > more complicated, because I doubt Hector has updated his patch set > with the review comments that have been made so far... so this is > now quite a mess. I think, once this is sorted, the entire series > will need to be re-reviewed entirely afresh. I have no insight into what Hector is doing, or plans to do. > I've also completely lost where I was in updating the patches with > all the discussion on this posting of the patch set (which is why I > posted v2, because I couldn't keep track of all the emails on this > version.) When I posted v2, I had already lost track, which is why > it got posted. Apologies if this has hindered your good work.
On Mon, Sep 12, 2022 at 11:55:14AM +0100, Lee Jones wrote: > > I'm guessing this series is now dead, and Hector needs to re-spin the > > patch set according to your views. I'm guessing this is going to take > > a major re-work of the patch series. > > > > I suspect my attempt and trying to get this upstream has made things > > more complicated, because I doubt Hector has updated his patch set > > with the review comments that have been made so far... so this is > > now quite a mess. I think, once this is sorted, the entire series > > will need to be re-reviewed entirely afresh. > > I have no insight into what Hector is doing, or plans to do. It seems there's no plans by Hector to address this, so it comes down to me. So, guessing what you're after, would something like the following work for you? I don't see *any* point in creating more yet more platform devices unless we're on a mission to maximise wasted memory resources (which this split will already be doing by creating two small modules instead of one.) Obviously, this is not an official patch yet, it's just to find out what code structure you are looking for. diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index 78c6d9d99c3f..8d4c0508a2c8 100644 --- a/drivers/mfd/Makefile +++ b/drivers/mfd/Makefile @@ -18,6 +18,8 @@ obj-$(CONFIG_MFD_ENE_KB3930) += ene-kb3930.o obj-$(CONFIG_MFD_EXYNOS_LPASS) += exynos-lpass.o obj-$(CONFIG_MFD_GATEWORKS_GSC) += gateworks-gsc.o +obj-$(CONFIG_APPLE_SMC) += apple-smc.o + obj-$(CONFIG_HTC_PASIC3) += htc-pasic3.o obj-$(CONFIG_HTC_I2CPLD) += htc-i2cpld.o diff --git a/drivers/mfd/apple-smc.c b/drivers/mfd/apple-smc.c new file mode 100644 index 000000000000..bc59d1c5e13d --- /dev/null +++ b/drivers/mfd/apple-smc.c @@ -0,0 +1,38 @@ +#include <linux/mfd/core.h> +#include <linux/mfd/apple-smc.h> + +static const struct mfd_cell apple_smc_devs[] = { + { + .name = "macsmc-gpio", + .of_compatible = "apple,smc-gpio", + }, + { + .name = "macsmc-hid", + }, + { + .name = "macsmc-power", + }, + { + .name = "macsmc-reboot", + }, + { + .name = "macsmc-rtc", + }, +}; + +int apple_smc_mfd_probe(struct device *dev) +{ + return mfd_add_devices(dev, -1, apple_smc_devs, + ARRAY_SIZE(apple_smc_devs), NULL, 0, NULL); +} +EXPORT_SYMBOL(apple_smc_mfd_probe); + +void apple_smc_mfd_remove(struct device *dev) +{ + mfd_remove_devices(dev); +} +EXPORT_SYMBOL(apple_smc_mfd_remove); + +MODULE_AUTHOR("Hector Martin <marcan@marcan.st>"); +MODULE_LICENSE("Dual MIT/GPL"); +MODULE_DESCRIPTION("Apple SMC MFD core"); diff --git a/drivers/platform/apple/smc_core.c b/drivers/platform/apple/smc_core.c index 148a3f8173d3..d4a502835b27 100644 --- a/drivers/platform/apple/smc_core.c +++ b/drivers/platform/apple/smc_core.c @@ -5,7 +5,7 @@ */ #include <linux/device.h> -#include <linux/mfd/core.h> +#include <linux/mfd/apple-smc.h> #include <linux/mutex.h> #include <linux/notifier.h> #include "smc.h" @@ -25,25 +25,6 @@ struct apple_smc { struct blocking_notifier_head event_handlers; }; -static const struct mfd_cell apple_smc_devs[] = { - { - .name = "macsmc-gpio", - .of_compatible = "apple,smc-gpio", - }, - { - .name = "macsmc-hid", - }, - { - .name = "macsmc-power", - }, - { - .name = "macsmc-reboot", - }, - { - .name = "macsmc-rtc", - }, -}; - int apple_smc_read(struct apple_smc *smc, smc_key key, void *buf, size_t size) { int ret; @@ -226,7 +207,7 @@ struct apple_smc *apple_smc_probe(struct device *dev, const struct apple_smc_bac dev_set_drvdata(dev, smc); - ret = mfd_add_devices(dev, -1, apple_smc_devs, ARRAY_SIZE(apple_smc_devs), NULL, 0, NULL); + ret = apple_smc_mfd_probe(dev); if (ret) return ERR_PTR(dev_err_probe(dev, ret, "Subdevice initialization failed")); @@ -236,7 +217,7 @@ EXPORT_SYMBOL(apple_smc_probe); int apple_smc_remove(struct apple_smc *smc) { - mfd_remove_devices(smc->dev); + apple_smc_mfd_remove(smc->dev); /* Disable notifications */ apple_smc_write_flag(smc, SMC_KEY(NTAP), 1); diff --git a/include/linux/mfd/apple-smc.h b/include/linux/mfd/apple-smc.h new file mode 100644 index 000000000000..2f34ca0f07f3 --- /dev/null +++ b/include/linux/mfd/apple-smc.h @@ -0,0 +1,9 @@ +#ifndef LINUX_MFD_APPLE_SMC_H +#define LINUX_MFD_APPLE_SMC_H + +struct device; + +int apple_smc_mfd_probe(struct device *dev); +void apple_smc_mfd_remove(struct device *dev); + +#endif
On 09/09/2022 16.50, Lee Jones wrote: >> What's the point of just having effectively an array of mfd_cell and >> wrappers to call into the mfd core in the drivers/mfd/ tree and the >> rest of the driver elsewhere? > > They should be separate drivers, with MFD registering the Platform. Why? What purpose does this serve? I'm still confused. There's one parent device, which provides services to the child devices. There isn't one parent device which wraps a platform service which is used by children. This makes no sense. The platform device is the root, if it exposes MFD services, then it has to be in that direction, not the other way around. Look at how this patch series is architected. There is smc_core.c, which implements SMC helpers and wrappers on top of a generic backend, and registers with the MFD subsystem. And then there is smc_rtkit.c which is the actual platform implementation on top of the RTKit framework, and is the actual platform device entry point. A priori, the only thing that makes sense to me right now would be to move smc_core.c into drivers/mfd, and leave smc_rtkit.c in platform. That way the mfd registration would be in drivers/mfd (as would be the services offered to sub-drivers), but the actual backend implementation would be in platform/ (and there would eventually be others, e.g. at least two more for x86 systems). That does mean that the driver entry point will be in platform/, with mfd/smc_core.c serving as effectively library code to plumb in the mfd stuff into one of several possible platform devices. Would that work for you? - Hector
On Fri, 28 Oct 2022, Russell King (Oracle) wrote: > On Mon, Sep 12, 2022 at 11:55:14AM +0100, Lee Jones wrote: > > > I'm guessing this series is now dead, and Hector needs to re-spin the > > > patch set according to your views. I'm guessing this is going to take > > > a major re-work of the patch series. > > > > > > I suspect my attempt and trying to get this upstream has made things > > > more complicated, because I doubt Hector has updated his patch set > > > with the review comments that have been made so far... so this is > > > now quite a mess. I think, once this is sorted, the entire series > > > will need to be re-reviewed entirely afresh. > > > > I have no insight into what Hector is doing, or plans to do. > > It seems there's no plans by Hector to address this, so it comes down > to me. > > So, guessing what you're after, would something like the following > work for you? I don't see *any* point in creating more yet more > platform devices unless we're on a mission to maximise wasted memory > resources (which this split will already be doing by creating two > small modules instead of one.) > > Obviously, this is not an official patch yet, it's just to find out > what code structure you are looking for. > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > index 78c6d9d99c3f..8d4c0508a2c8 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -18,6 +18,8 @@ obj-$(CONFIG_MFD_ENE_KB3930) += ene-kb3930.o > obj-$(CONFIG_MFD_EXYNOS_LPASS) += exynos-lpass.o > obj-$(CONFIG_MFD_GATEWORKS_GSC) += gateworks-gsc.o > > +obj-$(CONFIG_APPLE_SMC) += apple-smc.o > + > obj-$(CONFIG_HTC_PASIC3) += htc-pasic3.o > obj-$(CONFIG_HTC_I2CPLD) += htc-i2cpld.o > > diff --git a/drivers/mfd/apple-smc.c b/drivers/mfd/apple-smc.c > new file mode 100644 > index 000000000000..bc59d1c5e13d > --- /dev/null > +++ b/drivers/mfd/apple-smc.c > @@ -0,0 +1,38 @@ > +#include <linux/mfd/core.h> > +#include <linux/mfd/apple-smc.h> > + > +static const struct mfd_cell apple_smc_devs[] = { > + { > + .name = "macsmc-gpio", > + .of_compatible = "apple,smc-gpio", > + }, > + { > + .name = "macsmc-hid", > + }, > + { > + .name = "macsmc-power", > + }, > + { > + .name = "macsmc-reboot", > + }, > + { > + .name = "macsmc-rtc", > + }, > +}; > + > +int apple_smc_mfd_probe(struct device *dev) > +{ > + return mfd_add_devices(dev, -1, apple_smc_devs, > + ARRAY_SIZE(apple_smc_devs), NULL, 0, NULL); > +} > +EXPORT_SYMBOL(apple_smc_mfd_probe); > + > +void apple_smc_mfd_remove(struct device *dev) > +{ > + mfd_remove_devices(dev); > +} > +EXPORT_SYMBOL(apple_smc_mfd_remove); > + > +MODULE_AUTHOR("Hector Martin <marcan@marcan.st>"); > +MODULE_LICENSE("Dual MIT/GPL"); > +MODULE_DESCRIPTION("Apple SMC MFD core"); Conceptually interesting, not seen this one before, but clearly a hack, no? Pretty sure all of the other cores in MFD are represented by a Platform Device. Why not implement the inverse? The Apple SMC is clearly an MFD, in Linux terms, so why not move the Platform Device into here, fetch all of the global resources, register the sub-devices, then call into the rtkit implementation in drivers/platform?
On Sat, 29 Oct 2022, Hector Martin wrote: > On 09/09/2022 16.50, Lee Jones wrote: > >> What's the point of just having effectively an array of mfd_cell and > >> wrappers to call into the mfd core in the drivers/mfd/ tree and the > >> rest of the driver elsewhere? > > > > They should be separate drivers, with MFD registering the Platform. > > Why? What purpose does this serve? I'm still confused. There's one > parent device, which provides services to the child devices. There isn't > one parent device which wraps a platform service which is used by > children. This makes no sense. The platform device is the root, if it > exposes MFD services, then it has to be in that direction, not the other > way around. > > Look at how this patch series is architected. There is smc_core.c, which > implements SMC helpers and wrappers on top of a generic backend, and > registers with the MFD subsystem. And then there is smc_rtkit.c which is > the actual platform implementation on top of the RTKit framework, and is > the actual platform device entry point. > > A priori, the only thing that makes sense to me right now would be to > move smc_core.c into drivers/mfd, and leave smc_rtkit.c in platform. > That way the mfd registration would be in drivers/mfd (as would be the > services offered to sub-drivers), but the actual backend implementation > would be in platform/ (and there would eventually be others, e.g. at > least two more for x86 systems). That does mean that the driver entry > point will be in platform/, with mfd/smc_core.c serving as effectively > library code to plumb in the mfd stuff into one of several possible > platform devices. Would that work for you? Yes, sounds sensible. However, keep all of the abstraction craziness somewhere else and fetch and share all of your shared resources from the MFD (SMC) driver.
On 31/10/2022 17.48, Lee Jones wrote: > On Sat, 29 Oct 2022, Hector Martin wrote: > >> On 09/09/2022 16.50, Lee Jones wrote: >>>> What's the point of just having effectively an array of mfd_cell and >>>> wrappers to call into the mfd core in the drivers/mfd/ tree and the >>>> rest of the driver elsewhere? >>> >>> They should be separate drivers, with MFD registering the Platform. >> >> Why? What purpose does this serve? I'm still confused. There's one >> parent device, which provides services to the child devices. There isn't >> one parent device which wraps a platform service which is used by >> children. This makes no sense. The platform device is the root, if it >> exposes MFD services, then it has to be in that direction, not the other >> way around. >> >> Look at how this patch series is architected. There is smc_core.c, which >> implements SMC helpers and wrappers on top of a generic backend, and >> registers with the MFD subsystem. And then there is smc_rtkit.c which is >> the actual platform implementation on top of the RTKit framework, and is >> the actual platform device entry point. >> >> A priori, the only thing that makes sense to me right now would be to >> move smc_core.c into drivers/mfd, and leave smc_rtkit.c in platform. >> That way the mfd registration would be in drivers/mfd (as would be the >> services offered to sub-drivers), but the actual backend implementation >> would be in platform/ (and there would eventually be others, e.g. at >> least two more for x86 systems). That does mean that the driver entry >> point will be in platform/, with mfd/smc_core.c serving as effectively >> library code to plumb in the mfd stuff into one of several possible >> platform devices. Would that work for you? > > Yes, sounds sensible. However, keep all of the abstraction craziness > somewhere else and fetch and share all of your shared resources from > the MFD (SMC) driver. I'm not sure what you mean by that. The abstraction (smc_core.c) *is* the shared resource. All it does is wrap ops callbacks with a mutex and add a couple helpers for finding keys. Do you literally want us to just have this in drivers/mfd? // SPDX-License-Identifier: GPL-2.0-only OR MIT /* * Apple SMC MFD wrapper * Copyright The Asahi Linux Contributors */ #include <linux/device.h> #include "smc.h" static const struct mfd_cell apple_smc_devs[] = { { .name = "macsmc-gpio", }, { .name = "macsmc-hid", }, { .name = "macsmc-power", }, { .name = "macsmc-reboot", }, { .name = "macsmc-rtc", }, }; int apple_smc_add_mfd_devices(struct device *dev) { ret = mfd_add_devices(dev, -1, apple_smc_devs, ARRAY_SIZE(apple_smc_devs), NULL, 0, NULL); if (ret) return dev_err_probe(dev, ret, "Subdevice initialization failed"); return 0; } EXPORT_SYMBOL(apple_smc_add_mfd_devices); int apple_smc_remove_mfd_devices(struct device *dev) { mfd_remove_devices(smc->dev); return 0; } EXPORT_SYMBOL(apple_smc_add_mfd_devices); MODULE_AUTHOR("Hector Martin <marcan@marcan.st>"); MODULE_LICENSE("Dual MIT/GPL"); MODULE_DESCRIPTION("Apple SMC MFD wrapper"); Because this feels *immensely* silly and pointless. - Hector
On 31/10/2022 17.46, Lee Jones wrote: > On Fri, 28 Oct 2022, Russell King (Oracle) wrote: > >> On Mon, Sep 12, 2022 at 11:55:14AM +0100, Lee Jones wrote: >>>> I'm guessing this series is now dead, and Hector needs to re-spin the >>>> patch set according to your views. I'm guessing this is going to take >>>> a major re-work of the patch series. >>>> >>>> I suspect my attempt and trying to get this upstream has made things >>>> more complicated, because I doubt Hector has updated his patch set >>>> with the review comments that have been made so far... so this is >>>> now quite a mess. I think, once this is sorted, the entire series >>>> will need to be re-reviewed entirely afresh. >>> >>> I have no insight into what Hector is doing, or plans to do. >> >> It seems there's no plans by Hector to address this, so it comes down >> to me. >> >> So, guessing what you're after, would something like the following >> work for you? I don't see *any* point in creating more yet more >> platform devices unless we're on a mission to maximise wasted memory >> resources (which this split will already be doing by creating two >> small modules instead of one.) >> >> Obviously, this is not an official patch yet, it's just to find out >> what code structure you are looking for. >> >> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile >> index 78c6d9d99c3f..8d4c0508a2c8 100644 >> --- a/drivers/mfd/Makefile >> +++ b/drivers/mfd/Makefile >> @@ -18,6 +18,8 @@ obj-$(CONFIG_MFD_ENE_KB3930) += ene-kb3930.o >> obj-$(CONFIG_MFD_EXYNOS_LPASS) += exynos-lpass.o >> obj-$(CONFIG_MFD_GATEWORKS_GSC) += gateworks-gsc.o >> >> +obj-$(CONFIG_APPLE_SMC) += apple-smc.o >> + >> obj-$(CONFIG_HTC_PASIC3) += htc-pasic3.o >> obj-$(CONFIG_HTC_I2CPLD) += htc-i2cpld.o >> >> diff --git a/drivers/mfd/apple-smc.c b/drivers/mfd/apple-smc.c >> new file mode 100644 >> index 000000000000..bc59d1c5e13d >> --- /dev/null >> +++ b/drivers/mfd/apple-smc.c >> @@ -0,0 +1,38 @@ >> +#include <linux/mfd/core.h> >> +#include <linux/mfd/apple-smc.h> >> + >> +static const struct mfd_cell apple_smc_devs[] = { >> + { >> + .name = "macsmc-gpio", >> + .of_compatible = "apple,smc-gpio", >> + }, >> + { >> + .name = "macsmc-hid", >> + }, >> + { >> + .name = "macsmc-power", >> + }, >> + { >> + .name = "macsmc-reboot", >> + }, >> + { >> + .name = "macsmc-rtc", >> + }, >> +}; >> + >> +int apple_smc_mfd_probe(struct device *dev) >> +{ >> + return mfd_add_devices(dev, -1, apple_smc_devs, >> + ARRAY_SIZE(apple_smc_devs), NULL, 0, NULL); >> +} >> +EXPORT_SYMBOL(apple_smc_mfd_probe); >> + >> +void apple_smc_mfd_remove(struct device *dev) >> +{ >> + mfd_remove_devices(dev); >> +} >> +EXPORT_SYMBOL(apple_smc_mfd_remove); >> + >> +MODULE_AUTHOR("Hector Martin <marcan@marcan.st>"); >> +MODULE_LICENSE("Dual MIT/GPL"); >> +MODULE_DESCRIPTION("Apple SMC MFD core"); > > Conceptually interesting, not seen this one before, but clearly a > hack, no? Pretty sure all of the other cores in MFD are represented > by a Platform Device. > > Why not implement the inverse? The Apple SMC is clearly an MFD, in > Linux terms, so why not move the Platform Device into here, fetch all > of the global resources, register the sub-devices, then call into the > rtkit implementation in drivers/platform? Because the RTKit implementation is *one* of several possible backends, and the others aren't even necessarily platform devices, and may have their own registration requirements (e.g. probing for ACPI stuff on x86). The entry points are completely different depending on the flavor. They will have to be different modules that compile on different architectures and load based on completely different device IDs. This is common in Linux. There's the core xHCI driver, then an xhci-plat wrapper for platform devices, and an xhci-pci wrapper for PCI devices. It makes no sense to have the driver entry point be the core and then somehow call back out to xhci-pci and xhci-plat when those are different drivers with different match lists and different registration requirements. It sounds like you have a mental model of what you want for MFD that doesn't actually fit how hardware works, and you're trying to shoehorn this into it without thinking. Linux is perfectly capable of representing things in a way that works with this hardware, but you need to let go of this idea that "the mfd driver lives in drivers/mfd and is the entry point but also I don't want any platform/abstraction/silliness in there" because that just doesn't work. - Hector
On Mon, 31 Oct 2022, Hector Martin wrote: > On 31/10/2022 17.48, Lee Jones wrote: > > On Sat, 29 Oct 2022, Hector Martin wrote: > > > >> On 09/09/2022 16.50, Lee Jones wrote: > >>>> What's the point of just having effectively an array of mfd_cell and > >>>> wrappers to call into the mfd core in the drivers/mfd/ tree and the > >>>> rest of the driver elsewhere? > >>> > >>> They should be separate drivers, with MFD registering the Platform. > >> > >> Why? What purpose does this serve? I'm still confused. There's one > >> parent device, which provides services to the child devices. There isn't > >> one parent device which wraps a platform service which is used by > >> children. This makes no sense. The platform device is the root, if it > >> exposes MFD services, then it has to be in that direction, not the other > >> way around. > >> > >> Look at how this patch series is architected. There is smc_core.c, which > >> implements SMC helpers and wrappers on top of a generic backend, and > >> registers with the MFD subsystem. And then there is smc_rtkit.c which is > >> the actual platform implementation on top of the RTKit framework, and is > >> the actual platform device entry point. > >> > >> A priori, the only thing that makes sense to me right now would be to > >> move smc_core.c into drivers/mfd, and leave smc_rtkit.c in platform. > >> That way the mfd registration would be in drivers/mfd (as would be the > >> services offered to sub-drivers), but the actual backend implementation > >> would be in platform/ (and there would eventually be others, e.g. at > >> least two more for x86 systems). That does mean that the driver entry > >> point will be in platform/, with mfd/smc_core.c serving as effectively > >> library code to plumb in the mfd stuff into one of several possible > >> platform devices. Would that work for you? > > > > Yes, sounds sensible. However, keep all of the abstraction craziness > > somewhere else and fetch and share all of your shared resources from > > the MFD (SMC) driver. > > I'm not sure what you mean by that. The abstraction (smc_core.c) *is* > the shared resource. All it does is wrap ops callbacks with a mutex and > add a couple helpers for finding keys. Do you literally want us to just > have this in drivers/mfd? > > // SPDX-License-Identifier: GPL-2.0-only OR MIT > /* > * Apple SMC MFD wrapper > * Copyright The Asahi Linux Contributors > */ > > #include <linux/device.h> > #include "smc.h" > > static const struct mfd_cell apple_smc_devs[] = { > { > .name = "macsmc-gpio", > }, > { > .name = "macsmc-hid", > }, > { > .name = "macsmc-power", > }, > { > .name = "macsmc-reboot", > }, > { > .name = "macsmc-rtc", > }, > }; > > int apple_smc_add_mfd_devices(struct device *dev) > { > ret = mfd_add_devices(dev, -1, apple_smc_devs, > ARRAY_SIZE(apple_smc_devs), NULL, 0, NULL); > if (ret) > return dev_err_probe(dev, ret, "Subdevice initialization failed"); > > return 0; > } > EXPORT_SYMBOL(apple_smc_add_mfd_devices); > > int apple_smc_remove_mfd_devices(struct device *dev) > { > mfd_remove_devices(smc->dev); > > return 0; > } > EXPORT_SYMBOL(apple_smc_add_mfd_devices); > > MODULE_AUTHOR("Hector Martin <marcan@marcan.st>"); > MODULE_LICENSE("Dual MIT/GPL"); > MODULE_DESCRIPTION("Apple SMC MFD wrapper"); > > Because this feels *immensely* silly and pointless. ... and hacky. I agree. [BTW: if this is all you want to do, have you considered simple-mfd?] No, I want you to author a proper MFD device. The hardware you're describing in this submission *is* an MFD. So use the subsystem properly, instead of abusing it as a shim API to simply register platform devices. Request the device-wide memory (and other shared resources) here. Conduct core operations and initialisation here, then call into your Platform and other child devices to initiate the real work.
On Mon, Oct 31, 2022 at 08:46:25AM +0000, Lee Jones wrote: > On Fri, 28 Oct 2022, Russell King (Oracle) wrote: > > > On Mon, Sep 12, 2022 at 11:55:14AM +0100, Lee Jones wrote: > > > > I'm guessing this series is now dead, and Hector needs to re-spin the > > > > patch set according to your views. I'm guessing this is going to take > > > > a major re-work of the patch series. > > > > > > > > I suspect my attempt and trying to get this upstream has made things > > > > more complicated, because I doubt Hector has updated his patch set > > > > with the review comments that have been made so far... so this is > > > > now quite a mess. I think, once this is sorted, the entire series > > > > will need to be re-reviewed entirely afresh. > > > > > > I have no insight into what Hector is doing, or plans to do. > > > > It seems there's no plans by Hector to address this, so it comes down > > to me. > > > > So, guessing what you're after, would something like the following > > work for you? I don't see *any* point in creating more yet more > > platform devices unless we're on a mission to maximise wasted memory > > resources (which this split will already be doing by creating two > > small modules instead of one.) > > > > Obviously, this is not an official patch yet, it's just to find out > > what code structure you are looking for. > > > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > > index 78c6d9d99c3f..8d4c0508a2c8 100644 > > --- a/drivers/mfd/Makefile > > +++ b/drivers/mfd/Makefile > > @@ -18,6 +18,8 @@ obj-$(CONFIG_MFD_ENE_KB3930) += ene-kb3930.o > > obj-$(CONFIG_MFD_EXYNOS_LPASS) += exynos-lpass.o > > obj-$(CONFIG_MFD_GATEWORKS_GSC) += gateworks-gsc.o > > > > +obj-$(CONFIG_APPLE_SMC) += apple-smc.o > > + > > obj-$(CONFIG_HTC_PASIC3) += htc-pasic3.o > > obj-$(CONFIG_HTC_I2CPLD) += htc-i2cpld.o > > > > diff --git a/drivers/mfd/apple-smc.c b/drivers/mfd/apple-smc.c > > new file mode 100644 > > index 000000000000..bc59d1c5e13d > > --- /dev/null > > +++ b/drivers/mfd/apple-smc.c > > @@ -0,0 +1,38 @@ > > +#include <linux/mfd/core.h> > > +#include <linux/mfd/apple-smc.h> > > + > > +static const struct mfd_cell apple_smc_devs[] = { > > + { > > + .name = "macsmc-gpio", > > + .of_compatible = "apple,smc-gpio", > > + }, > > + { > > + .name = "macsmc-hid", > > + }, > > + { > > + .name = "macsmc-power", > > + }, > > + { > > + .name = "macsmc-reboot", > > + }, > > + { > > + .name = "macsmc-rtc", > > + }, > > +}; > > + > > +int apple_smc_mfd_probe(struct device *dev) > > +{ > > + return mfd_add_devices(dev, -1, apple_smc_devs, > > + ARRAY_SIZE(apple_smc_devs), NULL, 0, NULL); > > +} > > +EXPORT_SYMBOL(apple_smc_mfd_probe); > > + > > +void apple_smc_mfd_remove(struct device *dev) > > +{ > > + mfd_remove_devices(dev); > > +} > > +EXPORT_SYMBOL(apple_smc_mfd_remove); > > + > > +MODULE_AUTHOR("Hector Martin <marcan@marcan.st>"); > > +MODULE_LICENSE("Dual MIT/GPL"); > > +MODULE_DESCRIPTION("Apple SMC MFD core"); > > Conceptually interesting, not seen this one before, but clearly a > hack, no? Pretty sure all of the other cores in MFD are represented > by a Platform Device. No one seems to understand what you actually want to see with the smc-core.c part, so I'm trying to find out what code structure would suit you. It seemed from the thread that moving smc-core.c to drivers/mfd wasn't desirable, but there was the desire to move the mfd bits into there - so that's what I've done with this patch. It doesn't make any sense what so ever to add yet another platform device into this structure with all of the complication around what happens if the user forces it to unbind, so I didn't. > Why not implement the inverse? What do you mean "the inverse" ? The inverse of this patch is moving everything of smc-core.c except the MFD bits into drivers/mfd leaving the MFD bits in drivers/platform/apple, which makes no sense. > The Apple SMC is clearly an MFD, in > Linux terms, so why not move the Platform Device into here, fetch all > of the global resources, register the sub-devices, then call into the > rtkit implementation in drivers/platform? I thought you had previously ruled out the idea of moving the contents of drivers/platform/apple into drivers/mfd, but maybe your position on that had changed through the course of the discussion. It's really not obvious to me what you want from what's been said in this thread. So, I ask the direct question - would moving the code that is in this patch set from drivers/platform/apple to drivers/mfd then make it acceptable to you? In other words: drivers/platform/apple/smc_core.c drivers/platform/apple/smc.h drivers/platform/apple/smc_rtkit.c If not, then please clearly and fully state what you want to see. Thanks.
On 31/10/2022 18.29, Lee Jones wrote: > On Mon, 31 Oct 2022, Hector Martin wrote: > >> On 31/10/2022 17.48, Lee Jones wrote: >>> On Sat, 29 Oct 2022, Hector Martin wrote: >>> >>>> On 09/09/2022 16.50, Lee Jones wrote: >>>>>> What's the point of just having effectively an array of mfd_cell and >>>>>> wrappers to call into the mfd core in the drivers/mfd/ tree and the >>>>>> rest of the driver elsewhere? >>>>> >>>>> They should be separate drivers, with MFD registering the Platform. >>>> >>>> Why? What purpose does this serve? I'm still confused. There's one >>>> parent device, which provides services to the child devices. There isn't >>>> one parent device which wraps a platform service which is used by >>>> children. This makes no sense. The platform device is the root, if it >>>> exposes MFD services, then it has to be in that direction, not the other >>>> way around. >>>> >>>> Look at how this patch series is architected. There is smc_core.c, which >>>> implements SMC helpers and wrappers on top of a generic backend, and >>>> registers with the MFD subsystem. And then there is smc_rtkit.c which is >>>> the actual platform implementation on top of the RTKit framework, and is >>>> the actual platform device entry point. >>>> >>>> A priori, the only thing that makes sense to me right now would be to >>>> move smc_core.c into drivers/mfd, and leave smc_rtkit.c in platform. >>>> That way the mfd registration would be in drivers/mfd (as would be the >>>> services offered to sub-drivers), but the actual backend implementation >>>> would be in platform/ (and there would eventually be others, e.g. at >>>> least two more for x86 systems). That does mean that the driver entry >>>> point will be in platform/, with mfd/smc_core.c serving as effectively >>>> library code to plumb in the mfd stuff into one of several possible >>>> platform devices. Would that work for you? >>> >>> Yes, sounds sensible. However, keep all of the abstraction craziness >>> somewhere else and fetch and share all of your shared resources from >>> the MFD (SMC) driver. >> >> I'm not sure what you mean by that. The abstraction (smc_core.c) *is* >> the shared resource. All it does is wrap ops callbacks with a mutex and >> add a couple helpers for finding keys. Do you literally want us to just >> have this in drivers/mfd? >> >> // SPDX-License-Identifier: GPL-2.0-only OR MIT >> /* >> * Apple SMC MFD wrapper >> * Copyright The Asahi Linux Contributors >> */ >> >> #include <linux/device.h> >> #include "smc.h" >> >> static const struct mfd_cell apple_smc_devs[] = { >> { >> .name = "macsmc-gpio", >> }, >> { >> .name = "macsmc-hid", >> }, >> { >> .name = "macsmc-power", >> }, >> { >> .name = "macsmc-reboot", >> }, >> { >> .name = "macsmc-rtc", >> }, >> }; >> >> int apple_smc_add_mfd_devices(struct device *dev) >> { >> ret = mfd_add_devices(dev, -1, apple_smc_devs, >> ARRAY_SIZE(apple_smc_devs), NULL, 0, NULL); >> if (ret) >> return dev_err_probe(dev, ret, "Subdevice initialization failed"); >> >> return 0; >> } >> EXPORT_SYMBOL(apple_smc_add_mfd_devices); >> >> int apple_smc_remove_mfd_devices(struct device *dev) >> { >> mfd_remove_devices(smc->dev); >> >> return 0; >> } >> EXPORT_SYMBOL(apple_smc_add_mfd_devices); >> >> MODULE_AUTHOR("Hector Martin <marcan@marcan.st>"); >> MODULE_LICENSE("Dual MIT/GPL"); >> MODULE_DESCRIPTION("Apple SMC MFD wrapper"); >> >> Because this feels *immensely* silly and pointless. > > ... and hacky. I agree. > > [BTW: if this is all you want to do, have you considered simple-mfd?] > > No, I want you to author a proper MFD device. You don't seem to understand how MFD devices actually map to the hardware we're working with here. > The hardware you're describing in this submission *is* an MFD. So use > the subsystem properly, instead of abusing it as a shim API to simply > register platform devices. *sigh* the hardware I'm describing is a *class* of MFD devices. They all work the same way as far as the sub-devices see, but operate on completely different hardware backends. If you do not want "gooey platform stuff" in drivers/mfd, then it *has* to be a dumb shim. You have 3 options: - We move everything into drivers/mfd, which means there will (eventually) be 3 backend modules binding to real hardware devices and one shared core which actually does the MFD registration and provides common services. - We move just the core into drivers/mfd, which means the device binding will happen elsewhere, and the only code in the MFD subsystem will be common code and will be called as a library (via module exports, not via device binding). - We give up and just have a dumb shim in drivers/mfd as above, because you don't want to work with us. Either you work with how our hardware works or we go with this dumb shim workaround. You seem to want us to simultaneously "author a proper MFD device" and "not put platform stuff in MFD". We can't do both at the same time. Either the code is here or it is elsewhere. > Request the device-wide memory (and other shared resources) here. That's what smc_rtkit.c does, but you seem not to want that code in mfd. > Conduct core operations and initialisation here The RTKit library is in charge of core RTKit initialization, smc_rtkit is in charge of SMC-specific initialization, and smc_core.c is in charge of core SMC operations and initialization. What, exactly, do you want to move into drivers/mfd? (hint: not the RTKit library, that is shared by many other drivers). > then call into your Platform and other child devices to initiate the real work. There is nothing to call into, the child devices will bind and call *back* into the SMC core to do their job. - Hector
On Mon, 31 Oct 2022, Hector Martin wrote: > On 31/10/2022 18.29, Lee Jones wrote: > > On Mon, 31 Oct 2022, Hector Martin wrote: > > > >> On 31/10/2022 17.48, Lee Jones wrote: > >>> On Sat, 29 Oct 2022, Hector Martin wrote: > >>> > >>>> On 09/09/2022 16.50, Lee Jones wrote: > >>>>>> What's the point of just having effectively an array of mfd_cell and > >>>>>> wrappers to call into the mfd core in the drivers/mfd/ tree and the > >>>>>> rest of the driver elsewhere? > >>>>> > >>>>> They should be separate drivers, with MFD registering the Platform. > >>>> > >>>> Why? What purpose does this serve? I'm still confused. There's one > >>>> parent device, which provides services to the child devices. There isn't > >>>> one parent device which wraps a platform service which is used by > >>>> children. This makes no sense. The platform device is the root, if it > >>>> exposes MFD services, then it has to be in that direction, not the other > >>>> way around. > >>>> > >>>> Look at how this patch series is architected. There is smc_core.c, which > >>>> implements SMC helpers and wrappers on top of a generic backend, and > >>>> registers with the MFD subsystem. And then there is smc_rtkit.c which is > >>>> the actual platform implementation on top of the RTKit framework, and is > >>>> the actual platform device entry point. > >>>> > >>>> A priori, the only thing that makes sense to me right now would be to > >>>> move smc_core.c into drivers/mfd, and leave smc_rtkit.c in platform. > >>>> That way the mfd registration would be in drivers/mfd (as would be the > >>>> services offered to sub-drivers), but the actual backend implementation > >>>> would be in platform/ (and there would eventually be others, e.g. at > >>>> least two more for x86 systems). That does mean that the driver entry > >>>> point will be in platform/, with mfd/smc_core.c serving as effectively > >>>> library code to plumb in the mfd stuff into one of several possible > >>>> platform devices. Would that work for you? > >>> > >>> Yes, sounds sensible. However, keep all of the abstraction craziness > >>> somewhere else and fetch and share all of your shared resources from > >>> the MFD (SMC) driver. > >> > >> I'm not sure what you mean by that. The abstraction (smc_core.c) *is* > >> the shared resource. All it does is wrap ops callbacks with a mutex and > >> add a couple helpers for finding keys. Do you literally want us to just > >> have this in drivers/mfd? > >> > >> // SPDX-License-Identifier: GPL-2.0-only OR MIT > >> /* > >> * Apple SMC MFD wrapper > >> * Copyright The Asahi Linux Contributors > >> */ > >> > >> #include <linux/device.h> > >> #include "smc.h" > >> > >> static const struct mfd_cell apple_smc_devs[] = { > >> { > >> .name = "macsmc-gpio", > >> }, > >> { > >> .name = "macsmc-hid", > >> }, > >> { > >> .name = "macsmc-power", > >> }, > >> { > >> .name = "macsmc-reboot", > >> }, > >> { > >> .name = "macsmc-rtc", > >> }, > >> }; > >> > >> int apple_smc_add_mfd_devices(struct device *dev) > >> { > >> ret = mfd_add_devices(dev, -1, apple_smc_devs, > >> ARRAY_SIZE(apple_smc_devs), NULL, 0, NULL); > >> if (ret) > >> return dev_err_probe(dev, ret, "Subdevice initialization failed"); > >> > >> return 0; > >> } > >> EXPORT_SYMBOL(apple_smc_add_mfd_devices); > >> > >> int apple_smc_remove_mfd_devices(struct device *dev) > >> { > >> mfd_remove_devices(smc->dev); > >> > >> return 0; > >> } > >> EXPORT_SYMBOL(apple_smc_add_mfd_devices); > >> > >> MODULE_AUTHOR("Hector Martin <marcan@marcan.st>"); > >> MODULE_LICENSE("Dual MIT/GPL"); > >> MODULE_DESCRIPTION("Apple SMC MFD wrapper"); > >> > >> Because this feels *immensely* silly and pointless. > > > > ... and hacky. I agree. > > > > [BTW: if this is all you want to do, have you considered simple-mfd?] > > > > No, I want you to author a proper MFD device. > > You don't seem to understand how MFD devices actually map to the > hardware we're working with here. Via an abstracted message passing API and a little shared memory? The former eventually ending up in the MBOX API? > > The hardware you're describing in this submission *is* an MFD. So use > > the subsystem properly, instead of abusing it as a shim API to simply > > register platform devices. > > *sigh* the hardware I'm describing is a *class* of MFD devices. They all > work the same way as far as the sub-devices see, but operate on > completely different hardware backends. That's common. Usually we're using talking; SPI, I2C, USB and MMIO, but a bespoke message passing API is not out of the realms of normal. > If you do not want "gooey > platform stuff" in drivers/mfd, then it *has* to be a dumb shim. > > You have 3 options: > > - We move everything into drivers/mfd, which means there will > (eventually) be 3 backend modules binding to real hardware devices and > one shared core which actually does the MFD registration and provides > common services. Moving everything isn't an option. A split would be the most reasonable approach. We just need to decide on which parts should reside on which side of the MFD / Platform boundary. > - We move just the core into drivers/mfd, which means the device binding > will happen elsewhere, and the only code in the MFD subsystem will be > common code and will be called as a library (via module exports, not via > device binding). What are you describing as 'the core' here? And which device binding are you alluding to? Sub-device registration or something else? > - We give up and just have a dumb shim in drivers/mfd as above, because > you don't want to work with us. If I wasn't willing to work with you, who are you conversing with? Side-note: Tweeting derogatory comments about the people who have volunteered to take valuable time out of their day to help you, is no good way to encourage people to "work with you"! Most, if not all of us do this willingly and with no additional benefit, purely to help others. Seeing such horrid things spread widely, is a guaranteed way to spoil someone's day. > Either you work with how our hardware works or we go with this dumb shim > workaround. It sounds like you have this the wrong way around. Linux and its associated subsystems aren't going to bend and flex around your design decisions. You need to apply a little flexibility and work with us to find something that'll work for everyone. > You seem to want us to simultaneously "author a proper MFD > device" and "not put platform stuff in MFD". We can't do both at the > same time. Either the code is here or it is elsewhere. Of course you can. Just like everyone else does. After taking quite a bit of time out of my day to look at this today, I can see that your RTKit initialisation implementation is actually much more suited to MFD than the SMC wrappers. One thing that I'm still a little unsure about is future the hierarchy and layering of all these drivers. Which is it you say can / will be swapped out for different methods of initialisation? I see that you pass a bunch of function pointers from the RTKit implementation into the SMC. Which in turn offers an exported (apple_smc_*) API. In most of the frameworks I have knowledge of, the core provides the Ops structure and it's populated by the client device. I'm sure having that clear in my head will go some ways to put me in a position to advise you further. > > Request the device-wide memory (and other shared resources) here. > > That's what smc_rtkit.c does, but you seem not to want that code in mfd. I'm not sure I explicitly said that. > > Conduct core operations and initialisation here > > The RTKit library is in charge of core RTKit initialization, smc_rtkit > is in charge of SMC-specific initialization, and smc_core.c is in charge > of core SMC operations and initialization. What, exactly, do you want to > move into drivers/mfd? (hint: not the RTKit library, that is shared by > many other drivers). > > > then call into your Platform and other child devices to initiate the real work. > > There is nothing to call into, the child devices will bind and call > *back* into the SMC core to do their job. "call into" was not a good choice of words here. Simply, let the child devices go about their business and do whatever they were designed to do.
On Mon, 31 Oct 2022, Russell King (Oracle) wrote: > On Mon, Oct 31, 2022 at 08:46:25AM +0000, Lee Jones wrote: > > On Fri, 28 Oct 2022, Russell King (Oracle) wrote: > > > > > On Mon, Sep 12, 2022 at 11:55:14AM +0100, Lee Jones wrote: > > > > > I'm guessing this series is now dead, and Hector needs to re-spin the > > > > > patch set according to your views. I'm guessing this is going to take > > > > > a major re-work of the patch series. > > > > > > > > > > I suspect my attempt and trying to get this upstream has made things > > > > > more complicated, because I doubt Hector has updated his patch set > > > > > with the review comments that have been made so far... so this is > > > > > now quite a mess. I think, once this is sorted, the entire series > > > > > will need to be re-reviewed entirely afresh. > > > > > > > > I have no insight into what Hector is doing, or plans to do. > > > > > > It seems there's no plans by Hector to address this, so it comes down > > > to me. > > > > > > So, guessing what you're after, would something like the following > > > work for you? I don't see *any* point in creating more yet more > > > platform devices unless we're on a mission to maximise wasted memory > > > resources (which this split will already be doing by creating two > > > small modules instead of one.) > > > > > > Obviously, this is not an official patch yet, it's just to find out > > > what code structure you are looking for. > > > > > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > > > index 78c6d9d99c3f..8d4c0508a2c8 100644 > > > --- a/drivers/mfd/Makefile > > > +++ b/drivers/mfd/Makefile > > > @@ -18,6 +18,8 @@ obj-$(CONFIG_MFD_ENE_KB3930) += ene-kb3930.o > > > obj-$(CONFIG_MFD_EXYNOS_LPASS) += exynos-lpass.o > > > obj-$(CONFIG_MFD_GATEWORKS_GSC) += gateworks-gsc.o > > > > > > +obj-$(CONFIG_APPLE_SMC) += apple-smc.o > > > + > > > obj-$(CONFIG_HTC_PASIC3) += htc-pasic3.o > > > obj-$(CONFIG_HTC_I2CPLD) += htc-i2cpld.o > > > > > > diff --git a/drivers/mfd/apple-smc.c b/drivers/mfd/apple-smc.c > > > new file mode 100644 > > > index 000000000000..bc59d1c5e13d > > > --- /dev/null > > > +++ b/drivers/mfd/apple-smc.c > > > @@ -0,0 +1,38 @@ > > > +#include <linux/mfd/core.h> > > > +#include <linux/mfd/apple-smc.h> > > > + > > > +static const struct mfd_cell apple_smc_devs[] = { > > > + { > > > + .name = "macsmc-gpio", > > > + .of_compatible = "apple,smc-gpio", > > > + }, > > > + { > > > + .name = "macsmc-hid", > > > + }, > > > + { > > > + .name = "macsmc-power", > > > + }, > > > + { > > > + .name = "macsmc-reboot", > > > + }, > > > + { > > > + .name = "macsmc-rtc", > > > + }, > > > +}; > > > + > > > +int apple_smc_mfd_probe(struct device *dev) > > > +{ > > > + return mfd_add_devices(dev, -1, apple_smc_devs, > > > + ARRAY_SIZE(apple_smc_devs), NULL, 0, NULL); > > > +} > > > +EXPORT_SYMBOL(apple_smc_mfd_probe); > > > + > > > +void apple_smc_mfd_remove(struct device *dev) > > > +{ > > > + mfd_remove_devices(dev); > > > +} > > > +EXPORT_SYMBOL(apple_smc_mfd_remove); > > > + > > > +MODULE_AUTHOR("Hector Martin <marcan@marcan.st>"); > > > +MODULE_LICENSE("Dual MIT/GPL"); > > > +MODULE_DESCRIPTION("Apple SMC MFD core"); > > > > Conceptually interesting, not seen this one before, but clearly a > > hack, no? Pretty sure all of the other cores in MFD are represented > > by a Platform Device. > > No one seems to understand what you actually want to see with the > smc-core.c part, so I'm trying to find out what code structure > would suit you. > > It seemed from the thread that moving smc-core.c to drivers/mfd > wasn't desirable, but there was the desire to move the mfd bits > into there - so that's what I've done with this patch. It doesn't > make any sense what so ever to add yet another platform device > into this structure with all of the complication around what happens > if the user forces it to unbind, so I didn't. > > > Why not implement the inverse? > > What do you mean "the inverse" ? The inverse of this patch is moving > everything of smc-core.c except the MFD bits into drivers/mfd leaving > the MFD bits in drivers/platform/apple, which makes no sense. > > > The Apple SMC is clearly an MFD, in > > Linux terms, so why not move the Platform Device into here, fetch all > > of the global resources, register the sub-devices, then call into the > > rtkit implementation in drivers/platform? > > I thought you had previously ruled out the idea of moving the contents > of drivers/platform/apple into drivers/mfd, but maybe your position on > that had changed through the course of the discussion. It's really not > obvious to me what you want from what's been said in this thread. > > So, I ask the direct question - would moving the code that is in this > patch set from drivers/platform/apple to drivers/mfd then make it > acceptable to you? In other words: > > drivers/platform/apple/smc_core.c > drivers/platform/apple/smc.h > drivers/platform/apple/smc_rtkit.c > > If not, then please clearly and fully state what you want to see. Sorry Russell, I'm out of time today. Please see my recent reply to Hector for now and I'll get back to you first thing.
On Mon, Oct 31, 2022 at 05:23:12PM +0000, Lee Jones wrote: > I see that you pass a bunch of function pointers from the RTKit > implementation into the SMC. Which in turn offers an exported > (apple_smc_*) API. In most of the frameworks I have knowledge of, the > core provides the Ops structure and it's populated by the client > device. Sorry Lee, I don't get this point. From what I can see, the apple_smc_backend_ops struct is owned by the core System Management Controller code, and RTKit backend fills in an instance of these ops and provides that to the core SMC code. The RTKit backend is just how we walk to the System Management Controller. It is not a client. I don't see this being any different to struct file_operations, seq_operations, vm_operations_struct, block_Device_operations, and so on and so forth. Having read your response, I wonder if you're confused about what the smc_core and smc_rtkit code actually are - because you seem to think that smc_rtkit is a _client_ of the smc_core code. It isn't, as I explain above, it's how we talk to the System Management Controller, and smc_core provides a uniform interface to the client drivers such as GPIO, RTC etc. Essentially, we have: Hardware Backend Core Clients .---- RTC / .--- GPIO Mailbox -- RTKit -- SMC Core -- MFD -- HID \ `--- Power `---- Reboot RTKit is just _one_ possible backend, there are other backends that can be used to interface to the underlying platform implementation to talk to the SMC. > I'm sure having that clear in my head will go some ways to put me in a > position to advise you further. > > > > Request the device-wide memory (and other shared resources) here. > > > > That's what smc_rtkit.c does, but you seem not to want that code in mfd. > > I'm not sure I explicitly said that. On Fri, Sep 09, 2022 at 08:50:07AM +0100, Lee Jones wrote: | If we were to design and build it up again from scratch, I'd suggest | that the MFD part would be the core-driver / entry-point. That driver | should request and initialise shared resources and register the other | devices, which is essentially the MFD's mantra. This is exactly what smc_rtkit is doing, which as I've mentioned above is the backend provider of access to the System Management Controller. Backend-independent access to the System Management Controller is done via smc_core which - at least to me - seems to be entirely correct, and it seems entirely appropriate that this should be responsible for creating the individual clients that make use of the System Management Controller's facilities such as GPIO, RTC etc. > "call into" was not a good choice of words here. Simply, let the > child devices go about their business and do whatever they were > designed to do. ... by calling into the code which provides them with access to the System Management Controller - that being through smc_core and ultimately which ever backend is used to finally communicate with the System Management Controller. At this point, I'm wondering whether you're somehow expecting client devices to map memory and read/write some registers. This is not that kind of setup. The address space is entirely virtual, through a set of four byte keys that indicate to the System Management Controller which fine-grained resource one wants to access. That being an individual GPIO line or some other parameter of the system. The memory that you see smc_rtkit claim is for passing messages, none of the clients have a right to directly access that memory - indeed, doing so would be a total layering violation and really bad bit of design. So, I hope my response helps fill in some of the detail about what this code is doing, how it works and how it's been designed.
On Mon, Oct 31, 2022 at 05:24:53PM +0000, Lee Jones wrote: > On Mon, 31 Oct 2022, Russell King (Oracle) wrote: > > > On Mon, Oct 31, 2022 at 08:46:25AM +0000, Lee Jones wrote: > > > On Fri, 28 Oct 2022, Russell King (Oracle) wrote: > > > > > > > On Mon, Sep 12, 2022 at 11:55:14AM +0100, Lee Jones wrote: > > > > > > I'm guessing this series is now dead, and Hector needs to re-spin the > > > > > > patch set according to your views. I'm guessing this is going to take > > > > > > a major re-work of the patch series. > > > > > > > > > > > > I suspect my attempt and trying to get this upstream has made things > > > > > > more complicated, because I doubt Hector has updated his patch set > > > > > > with the review comments that have been made so far... so this is > > > > > > now quite a mess. I think, once this is sorted, the entire series > > > > > > will need to be re-reviewed entirely afresh. > > > > > > > > > > I have no insight into what Hector is doing, or plans to do. > > > > > > > > It seems there's no plans by Hector to address this, so it comes down > > > > to me. > > > > > > > > So, guessing what you're after, would something like the following > > > > work for you? I don't see *any* point in creating more yet more > > > > platform devices unless we're on a mission to maximise wasted memory > > > > resources (which this split will already be doing by creating two > > > > small modules instead of one.) > > > > > > > > Obviously, this is not an official patch yet, it's just to find out > > > > what code structure you are looking for. > > > > > > > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > > > > index 78c6d9d99c3f..8d4c0508a2c8 100644 > > > > --- a/drivers/mfd/Makefile > > > > +++ b/drivers/mfd/Makefile > > > > @@ -18,6 +18,8 @@ obj-$(CONFIG_MFD_ENE_KB3930) += ene-kb3930.o > > > > obj-$(CONFIG_MFD_EXYNOS_LPASS) += exynos-lpass.o > > > > obj-$(CONFIG_MFD_GATEWORKS_GSC) += gateworks-gsc.o > > > > > > > > +obj-$(CONFIG_APPLE_SMC) += apple-smc.o > > > > + > > > > obj-$(CONFIG_HTC_PASIC3) += htc-pasic3.o > > > > obj-$(CONFIG_HTC_I2CPLD) += htc-i2cpld.o > > > > > > > > diff --git a/drivers/mfd/apple-smc.c b/drivers/mfd/apple-smc.c > > > > new file mode 100644 > > > > index 000000000000..bc59d1c5e13d > > > > --- /dev/null > > > > +++ b/drivers/mfd/apple-smc.c > > > > @@ -0,0 +1,38 @@ > > > > +#include <linux/mfd/core.h> > > > > +#include <linux/mfd/apple-smc.h> > > > > + > > > > +static const struct mfd_cell apple_smc_devs[] = { > > > > + { > > > > + .name = "macsmc-gpio", > > > > + .of_compatible = "apple,smc-gpio", > > > > + }, > > > > + { > > > > + .name = "macsmc-hid", > > > > + }, > > > > + { > > > > + .name = "macsmc-power", > > > > + }, > > > > + { > > > > + .name = "macsmc-reboot", > > > > + }, > > > > + { > > > > + .name = "macsmc-rtc", > > > > + }, > > > > +}; > > > > + > > > > +int apple_smc_mfd_probe(struct device *dev) > > > > +{ > > > > + return mfd_add_devices(dev, -1, apple_smc_devs, > > > > + ARRAY_SIZE(apple_smc_devs), NULL, 0, NULL); > > > > +} > > > > +EXPORT_SYMBOL(apple_smc_mfd_probe); > > > > + > > > > +void apple_smc_mfd_remove(struct device *dev) > > > > +{ > > > > + mfd_remove_devices(dev); > > > > +} > > > > +EXPORT_SYMBOL(apple_smc_mfd_remove); > > > > + > > > > +MODULE_AUTHOR("Hector Martin <marcan@marcan.st>"); > > > > +MODULE_LICENSE("Dual MIT/GPL"); > > > > +MODULE_DESCRIPTION("Apple SMC MFD core"); > > > > > > Conceptually interesting, not seen this one before, but clearly a > > > hack, no? Pretty sure all of the other cores in MFD are represented > > > by a Platform Device. > > > > No one seems to understand what you actually want to see with the > > smc-core.c part, so I'm trying to find out what code structure > > would suit you. > > > > It seemed from the thread that moving smc-core.c to drivers/mfd > > wasn't desirable, but there was the desire to move the mfd bits > > into there - so that's what I've done with this patch. It doesn't > > make any sense what so ever to add yet another platform device > > into this structure with all of the complication around what happens > > if the user forces it to unbind, so I didn't. > > > > > Why not implement the inverse? > > > > What do you mean "the inverse" ? The inverse of this patch is moving > > everything of smc-core.c except the MFD bits into drivers/mfd leaving > > the MFD bits in drivers/platform/apple, which makes no sense. > > > > > The Apple SMC is clearly an MFD, in > > > Linux terms, so why not move the Platform Device into here, fetch all > > > of the global resources, register the sub-devices, then call into the > > > rtkit implementation in drivers/platform? > > > > I thought you had previously ruled out the idea of moving the contents > > of drivers/platform/apple into drivers/mfd, but maybe your position on > > that had changed through the course of the discussion. It's really not > > obvious to me what you want from what's been said in this thread. > > > > So, I ask the direct question - would moving the code that is in this > > patch set from drivers/platform/apple to drivers/mfd then make it > > acceptable to you? In other words: > > > > drivers/platform/apple/smc_core.c > > drivers/platform/apple/smc.h > > drivers/platform/apple/smc_rtkit.c > > > > If not, then please clearly and fully state what you want to see. > > Sorry Russell, I'm out of time today. Please see my recent reply to > Hector for now and I'll get back to you first thing. Hi Lee, Thanks - I look forward to it. Having read your response to Hector, I am wondering whether there's a misunderstanding of the code, so I'm hoping that my attempt in my reply helps to clear up any code misunderstandings. If you want to ask questions about the code, you know where to find me on irc, and I'll more than happily answer anything you want to know about the code structure. Russell.
On Mon, 31 Oct 2022, Russell King (Oracle) wrote: > On Mon, Oct 31, 2022 at 05:24:53PM +0000, Lee Jones wrote: > > On Mon, 31 Oct 2022, Russell King (Oracle) wrote: > > > > > On Mon, Oct 31, 2022 at 08:46:25AM +0000, Lee Jones wrote: > > > > On Fri, 28 Oct 2022, Russell King (Oracle) wrote: > > > > > > > > > On Mon, Sep 12, 2022 at 11:55:14AM +0100, Lee Jones wrote: > > > > > > > I'm guessing this series is now dead, and Hector needs to re-spin the > > > > > > > patch set according to your views. I'm guessing this is going to take > > > > > > > a major re-work of the patch series. > > > > > > > > > > > > > > I suspect my attempt and trying to get this upstream has made things > > > > > > > more complicated, because I doubt Hector has updated his patch set > > > > > > > with the review comments that have been made so far... so this is > > > > > > > now quite a mess. I think, once this is sorted, the entire series > > > > > > > will need to be re-reviewed entirely afresh. > > > > > > > > > > > > I have no insight into what Hector is doing, or plans to do. > > > > > > > > > > It seems there's no plans by Hector to address this, so it comes down > > > > > to me. > > > > > > > > > > So, guessing what you're after, would something like the following > > > > > work for you? I don't see *any* point in creating more yet more > > > > > platform devices unless we're on a mission to maximise wasted memory > > > > > resources (which this split will already be doing by creating two > > > > > small modules instead of one.) > > > > > > > > > > Obviously, this is not an official patch yet, it's just to find out > > > > > what code structure you are looking for. > > > > > > > > > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > > > > > index 78c6d9d99c3f..8d4c0508a2c8 100644 > > > > > --- a/drivers/mfd/Makefile > > > > > +++ b/drivers/mfd/Makefile > > > > > @@ -18,6 +18,8 @@ obj-$(CONFIG_MFD_ENE_KB3930) += ene-kb3930.o > > > > > obj-$(CONFIG_MFD_EXYNOS_LPASS) += exynos-lpass.o > > > > > obj-$(CONFIG_MFD_GATEWORKS_GSC) += gateworks-gsc.o > > > > > > > > > > +obj-$(CONFIG_APPLE_SMC) += apple-smc.o > > > > > + > > > > > obj-$(CONFIG_HTC_PASIC3) += htc-pasic3.o > > > > > obj-$(CONFIG_HTC_I2CPLD) += htc-i2cpld.o > > > > > > > > > > diff --git a/drivers/mfd/apple-smc.c b/drivers/mfd/apple-smc.c > > > > > new file mode 100644 > > > > > index 000000000000..bc59d1c5e13d > > > > > --- /dev/null > > > > > +++ b/drivers/mfd/apple-smc.c > > > > > @@ -0,0 +1,38 @@ > > > > > +#include <linux/mfd/core.h> > > > > > +#include <linux/mfd/apple-smc.h> > > > > > + > > > > > +static const struct mfd_cell apple_smc_devs[] = { > > > > > + { > > > > > + .name = "macsmc-gpio", > > > > > + .of_compatible = "apple,smc-gpio", > > > > > + }, > > > > > + { > > > > > + .name = "macsmc-hid", > > > > > + }, > > > > > + { > > > > > + .name = "macsmc-power", > > > > > + }, > > > > > + { > > > > > + .name = "macsmc-reboot", > > > > > + }, > > > > > + { > > > > > + .name = "macsmc-rtc", > > > > > + }, > > > > > +}; > > > > > + > > > > > +int apple_smc_mfd_probe(struct device *dev) > > > > > +{ > > > > > + return mfd_add_devices(dev, -1, apple_smc_devs, > > > > > + ARRAY_SIZE(apple_smc_devs), NULL, 0, NULL); > > > > > +} > > > > > +EXPORT_SYMBOL(apple_smc_mfd_probe); > > > > > + > > > > > +void apple_smc_mfd_remove(struct device *dev) > > > > > +{ > > > > > + mfd_remove_devices(dev); > > > > > +} > > > > > +EXPORT_SYMBOL(apple_smc_mfd_remove); > > > > > + > > > > > +MODULE_AUTHOR("Hector Martin <marcan@marcan.st>"); > > > > > +MODULE_LICENSE("Dual MIT/GPL"); > > > > > +MODULE_DESCRIPTION("Apple SMC MFD core"); > > > > > > > > Conceptually interesting, not seen this one before, but clearly a > > > > hack, no? Pretty sure all of the other cores in MFD are represented > > > > by a Platform Device. > > > > > > No one seems to understand what you actually want to see with the > > > smc-core.c part, so I'm trying to find out what code structure > > > would suit you. > > > > > > It seemed from the thread that moving smc-core.c to drivers/mfd > > > wasn't desirable, but there was the desire to move the mfd bits > > > into there - so that's what I've done with this patch. It doesn't > > > make any sense what so ever to add yet another platform device > > > into this structure with all of the complication around what happens > > > if the user forces it to unbind, so I didn't. > > > > > > > Why not implement the inverse? > > > > > > What do you mean "the inverse" ? The inverse of this patch is moving > > > everything of smc-core.c except the MFD bits into drivers/mfd leaving > > > the MFD bits in drivers/platform/apple, which makes no sense. > > > > > > > The Apple SMC is clearly an MFD, in > > > > Linux terms, so why not move the Platform Device into here, fetch all > > > > of the global resources, register the sub-devices, then call into the > > > > rtkit implementation in drivers/platform? > > > > > > I thought you had previously ruled out the idea of moving the contents > > > of drivers/platform/apple into drivers/mfd, but maybe your position on > > > that had changed through the course of the discussion. It's really not > > > obvious to me what you want from what's been said in this thread. > > > > > > So, I ask the direct question - would moving the code that is in this > > > patch set from drivers/platform/apple to drivers/mfd then make it > > > acceptable to you? In other words: > > > > > > drivers/platform/apple/smc_core.c > > > drivers/platform/apple/smc.h > > > drivers/platform/apple/smc_rtkit.c > > > > > > If not, then please clearly and fully state what you want to see. > > > > Sorry Russell, I'm out of time today. Please see my recent reply to > > Hector for now and I'll get back to you first thing. > > Hi Lee, > > Thanks - I look forward to it. Having read your response to Hector, I > am wondering whether there's a misunderstanding of the code, so I'm > hoping that my attempt in my reply helps to clear up any code > misunderstandings. > > If you want to ask questions about the code, you know where to find > me on irc, and I'll more than happily answer anything you want to > know about the code structure. That might be helpful, thanks. Let's keep in on-list for now, in case others are following along. For now, I'll go take a look at your other response.
On Mon, 31 Oct 2022, Russell King (Oracle) wrote: > On Mon, Oct 31, 2022 at 05:23:12PM +0000, Lee Jones wrote: > > I see that you pass a bunch of function pointers from the RTKit > > implementation into the SMC. Which in turn offers an exported > > (apple_smc_*) API. In most of the frameworks I have knowledge of, the > > core provides the Ops structure and it's populated by the client > > device. > > Sorry Lee, I don't get this point. From what I can see, the > apple_smc_backend_ops struct is owned by the core System Management > Controller code, and RTKit backend fills in an instance of these ops > and provides that to the core SMC code. The RTKit backend is just > how we walk to the System Management Controller. It is not a client. > > I don't see this being any different to struct file_operations, > seq_operations, vm_operations_struct, block_Device_operations, > and so on and so forth. > > Having read your response, I wonder if you're confused about what the > smc_core and smc_rtkit code actually are - because you seem to think > that smc_rtkit is a _client_ of the smc_core code. It isn't, as I > explain above, it's how we talk to the System Management Controller, > and smc_core provides a uniform interface to the client drivers such > as GPIO, RTC etc. > > Essentially, we have: > > Hardware Backend Core Clients > .---- RTC > / .--- GPIO > Mailbox -- RTKit -- SMC Core -- MFD -- HID > \ `--- Power > `---- Reboot The issue I see with the current implementation is that, what you are calling the SMC Core here, is being viewed as the parent to all of these client (child / sub) devices. However, in reality, the SMC Core is little more than a function pointer shim / pass-through without it's own Device. In order for it to register the child-devices, it's effectively borrowing and branching off of the RTKit's Device, which from a Device Driver hierarchy stand-point feels odd. > RTKit is just _one_ possible backend, there are other backends that > can be used to interface to the underlying platform implementation to > talk to the SMC. Right, this is the tricky part. The way I see it, the best route would be to make the RTKit, which is the real Linux Device (with real resources), the direct parent (no sharing) and move it to MFD. However, the looming question then becomes, "what happens when the RTKit is removed and swapped out for something else?", does that mean anything you swap it out with will need to become an MFD also? How would the children get registered otherwise? The TL;DR is, I can see why it's been architected this way, but it doesn't really fit in with the Device Driver norms, which makes it quite difficult to shoehorn in without the issues (using MFD API outside of MFD and borrowing Devices) we're encountering here. > > I'm sure having that clear in my head will go some ways to put me in a > > position to advise you further. > > > > > > Request the device-wide memory (and other shared resources) here. > > > > > > That's what smc_rtkit.c does, but you seem not to want that code in mfd. > > > > I'm not sure I explicitly said that. > > On Fri, Sep 09, 2022 at 08:50:07AM +0100, Lee Jones wrote: > | If we were to design and build it up again from scratch, I'd suggest > | that the MFD part would be the core-driver / entry-point. That driver > | should request and initialise shared resources and register the other > | devices, which is essentially the MFD's mantra. > > This is exactly what smc_rtkit is doing, which as I've mentioned above > is the backend provider of access to the System Management Controller. > Backend-independent access to the System Management Controller is done > via smc_core which - at least to me - seems to be entirely correct, > and it seems entirely appropriate that this should be responsible for > creating the individual clients that make use of the System Management > Controller's facilities such as GPIO, RTC etc. Making a shim-layer without a real Device become a parent to sub-devices is the part I'm finding most difficult to swallow. > > "call into" was not a good choice of words here. Simply, let the > > child devices go about their business and do whatever they were > > designed to do. > > ... by calling into the code which provides them with access to the > System Management Controller - that being through smc_core and > ultimately which ever backend is used to finally communicate with the > System Management Controller. Right. No problem with that part. Mailbox, I2C, SPI, MMIO, USB, makes no difference to me. > At this point, I'm wondering whether you're somehow expecting client > devices to map memory and read/write some registers. This is not that > kind of setup. The address space is entirely virtual, through a set > of four byte keys that indicate to the System Management Controller > which fine-grained resource one wants to access. That being an > individual GPIO line or some other parameter of the system. No, not at all. A public API mapping to whatever the H/W communication strategy (I2C, SPI, ...) is a perfectly viable implementation. > The memory that you see smc_rtkit claim is for passing messages, none > of the clients have a right to directly access that memory - indeed, > doing so would be a total layering violation and really bad bit of > design. This is not a concept I have an issue understanding. > So, I hope my response helps fill in some of the detail about what > this code is doing, how it works and how it's been designed. I have a strong grasp of the concept presented. :) If we can find an acceptable way to create a proper and correct device hierarchy, I think we're in a good place. My present suggestion is to make the RTKit the MFD and spawn the child devices directly from it.
Hi Lee,
One of the things we discussed on IRC was moving the shim to a header
file. Moving code into functions in a header file requires the
functions to be marked "static inline", and when I mentioned that
you seemed to be rather surprised.
They need to be static to give them compilation-unit scope so two
different users of them don't expose the functions to each other,
creating a linker error. They also need to be "inline" so the
compiler knows that they're supposed to be shim-style functions and
avoids the compiler warning that static functions aren't being used.
Most can be moved out, but we end up with one rather large function
which feels way too bulky to move into a header file, that being
apple_smc_find_first_key_index().
So, this is what it looks like moving everything except that function,
and it means we need somewhere for that function to live outside of a
header file. This raises the question whether there really is much
point to moving the shim out.
If we do manage to move the shim out, we're left with the shim
initialisation and the MFD bits left in smc_core.c, and we've already
established that oving the MFD bits isn't what you're after. So, we
could move that initialisation and MFD bits to smc_rtkit.c... but then
what happens when the next backend comes along (which will be for Intel
based MACs), we would need to duplicate that code in another backend.
To me, making this change just feels wrong - not just mildly wrong -
because it exposes things that consumers of the SMC API offered by the
core shouldn't really know about, such as the contents of struct
apple_smc and struct apple_smc_backend_ops. I'm really not sure doing
this results in an improvement. Here's the changes on top of this
patch set anyway.
From what I understand that you want for MFD, you want a single .c file
that is a device driver itself, which then talks directly to MFD with
nothing else inbetween.
The only way I can think of achieving that is to essentially move the
contents of smc_rtkit.c into smc_core.c, and when it comes to merging
the Intel SMC support, that also has to go into smc_core.c as well -
which means we end up with a lot of dead code and #ifdef's in that .c
file - which is not nice.
drivers/platform/apple/smc.h | 24 +++---
drivers/platform/apple/smc_core.c | 127 ----------------------------
include/linux/mfd/macsmc.h | 136 +++++++++++++++++++++++++++---
3 files changed, 138 insertions(+), 149 deletions(-)
diff --git a/drivers/platform/apple/smc.h b/drivers/platform/apple/smc.h
index 8ae51887b2c5..fa86411d5044 100644
--- a/drivers/platform/apple/smc.h
+++ b/drivers/platform/apple/smc.h
@@ -7,22 +7,24 @@
#ifndef _SMC_H
#define _SMC_H
+#include <linux/device.h>
#include <linux/mfd/macsmc.h>
-struct apple_smc_backend_ops {
- int (*read_key)(void *cookie, smc_key key, void *buf, size_t size);
- int (*write_key)(void *cookie, smc_key key, void *buf, size_t size);
- int (*write_key_atomic)(void *cookie, smc_key key, void *buf, size_t size);
- int (*rw_key)(void *cookie, smc_key key, void *wbuf, size_t wsize,
- void *rbuf, size_t rsize);
- int (*get_key_by_index)(void *cookie, int index, smc_key *key);
- int (*get_key_info)(void *cookie, smc_key key, struct apple_smc_key_info *info);
-};
+static inline void apple_smc_event_received(struct apple_smc *smc,
+ uint32_t event)
+{
+ dev_dbg(smc->dev, "Event: 0x%08x\n", event);
+
+ blocking_notifier_call_chain(&smc->event_handlers, event, NULL);
+}
+
+static inline void *apple_smc_get_cookie(struct apple_smc *smc)
+{
+ return smc->be_cookie;
+}
struct apple_smc *apple_smc_probe(struct device *dev, const struct apple_smc_backend_ops *ops,
void *cookie);
-void *apple_smc_get_cookie(struct apple_smc *smc);
int apple_smc_remove(struct apple_smc *smc);
-void apple_smc_event_received(struct apple_smc *smc, uint32_t event);
#endif
diff --git a/drivers/platform/apple/smc_core.c b/drivers/platform/apple/smc_core.c
index 148a3f8173d3..0902d2f811f4 100644
--- a/drivers/platform/apple/smc_core.c
+++ b/drivers/platform/apple/smc_core.c
@@ -6,25 +6,8 @@
#include <linux/device.h>
#include <linux/mfd/core.h>
-#include <linux/mutex.h>
-#include <linux/notifier.h>
#include "smc.h"
-struct apple_smc {
- struct device *dev;
-
- void *be_cookie;
- const struct apple_smc_backend_ops *be;
-
- struct mutex mutex;
-
- u32 key_count;
- smc_key first_key;
- smc_key last_key;
-
- struct blocking_notifier_head event_handlers;
-};
-
static const struct mfd_cell apple_smc_devs[] = {
{
.name = "macsmc-gpio",
@@ -44,85 +27,6 @@ static const struct mfd_cell apple_smc_devs[] = {
},
};
-int apple_smc_read(struct apple_smc *smc, smc_key key, void *buf, size_t size)
-{
- int ret;
-
- mutex_lock(&smc->mutex);
- ret = smc->be->read_key(smc->be_cookie, key, buf, size);
- mutex_unlock(&smc->mutex);
-
- return ret;
-}
-EXPORT_SYMBOL(apple_smc_read);
-
-int apple_smc_write(struct apple_smc *smc, smc_key key, void *buf, size_t size)
-{
- int ret;
-
- mutex_lock(&smc->mutex);
- ret = smc->be->write_key(smc->be_cookie, key, buf, size);
- mutex_unlock(&smc->mutex);
-
- return ret;
-}
-EXPORT_SYMBOL(apple_smc_write);
-
-int apple_smc_write_atomic(struct apple_smc *smc, smc_key key, void *buf, size_t size)
-{
- int ret;
-
- /*
- * Will fail if SMC is busy. This is only used by SMC reboot/poweroff
- * final calls, so it doesn't really matter at that point.
- */
- if (!mutex_trylock(&smc->mutex))
- return -EBUSY;
-
- ret = smc->be->write_key_atomic(smc->be_cookie, key, buf, size);
- mutex_unlock(&smc->mutex);
-
- return ret;
-}
-EXPORT_SYMBOL(apple_smc_write_atomic);
-
-int apple_smc_rw(struct apple_smc *smc, smc_key key, void *wbuf, size_t wsize,
- void *rbuf, size_t rsize)
-{
- int ret;
-
- mutex_lock(&smc->mutex);
- ret = smc->be->rw_key(smc->be_cookie, key, wbuf, wsize, rbuf, rsize);
- mutex_unlock(&smc->mutex);
-
- return ret;
-}
-EXPORT_SYMBOL(apple_smc_rw);
-
-int apple_smc_get_key_by_index(struct apple_smc *smc, int index, smc_key *key)
-{
- int ret;
-
- mutex_lock(&smc->mutex);
- ret = smc->be->get_key_by_index(smc->be_cookie, index, key);
- mutex_unlock(&smc->mutex);
-
- return ret;
-}
-EXPORT_SYMBOL(apple_smc_get_key_by_index);
-
-int apple_smc_get_key_info(struct apple_smc *smc, smc_key key, struct apple_smc_key_info *info)
-{
- int ret;
-
- mutex_lock(&smc->mutex);
- ret = smc->be->get_key_info(smc->be_cookie, key, info);
- mutex_unlock(&smc->mutex);
-
- return ret;
-}
-EXPORT_SYMBOL(apple_smc_get_key_info);
-
int apple_smc_find_first_key_index(struct apple_smc *smc, smc_key key)
{
int start = 0, count = smc->key_count;
@@ -158,37 +62,6 @@ int apple_smc_find_first_key_index(struct apple_smc *smc, smc_key key)
}
EXPORT_SYMBOL(apple_smc_find_first_key_index);
-int apple_smc_get_key_count(struct apple_smc *smc)
-{
- return smc->key_count;
-}
-EXPORT_SYMBOL(apple_smc_get_key_count);
-
-void apple_smc_event_received(struct apple_smc *smc, uint32_t event)
-{
- dev_dbg(smc->dev, "Event: 0x%08x\n", event);
- blocking_notifier_call_chain(&smc->event_handlers, event, NULL);
-}
-EXPORT_SYMBOL(apple_smc_event_received);
-
-int apple_smc_register_notifier(struct apple_smc *smc, struct notifier_block *n)
-{
- return blocking_notifier_chain_register(&smc->event_handlers, n);
-}
-EXPORT_SYMBOL(apple_smc_register_notifier);
-
-int apple_smc_unregister_notifier(struct apple_smc *smc, struct notifier_block *n)
-{
- return blocking_notifier_chain_unregister(&smc->event_handlers, n);
-}
-EXPORT_SYMBOL(apple_smc_unregister_notifier);
-
-void *apple_smc_get_cookie(struct apple_smc *smc)
-{
- return smc->be_cookie;
-}
-EXPORT_SYMBOL(apple_smc_get_cookie);
-
struct apple_smc *apple_smc_probe(struct device *dev, const struct apple_smc_backend_ops *ops, void *cookie)
{
struct apple_smc *smc;
diff --git a/include/linux/mfd/macsmc.h b/include/linux/mfd/macsmc.h
index 39b4dc4ca881..147d6f957cb1 100644
--- a/include/linux/mfd/macsmc.h
+++ b/include/linux/mfd/macsmc.h
@@ -7,7 +7,10 @@
#ifndef _LINUX_MFD_MACSMC_H
#define _LINUX_MFD_MACSMC_H
-struct apple_smc;
+#include <linux/errno.h>
+#include <linux/mutex.h>
+#include <linux/notifier.h>
+#include <linux/types.h>
typedef u32 smc_key;
@@ -24,16 +27,118 @@ struct apple_smc_key_info {
u8 flags;
};
-int apple_smc_read(struct apple_smc *smc, smc_key key, void *buf, size_t size);
-int apple_smc_write(struct apple_smc *smc, smc_key key, void *buf, size_t size);
-int apple_smc_write_atomic(struct apple_smc *smc, smc_key key, void *buf, size_t size);
-int apple_smc_rw(struct apple_smc *smc, smc_key key, void *wbuf, size_t wsize,
- void *rbuf, size_t rsize);
+struct device;
+
+struct apple_smc_backend_ops {
+ int (*read_key)(void *cookie, smc_key key, void *buf, size_t size);
+ int (*write_key)(void *cookie, smc_key key, void *buf, size_t size);
+ int (*write_key_atomic)(void *cookie, smc_key key, void *buf, size_t size);
+ int (*rw_key)(void *cookie, smc_key key, void *wbuf, size_t wsize,
+ void *rbuf, size_t rsize);
+ int (*get_key_by_index)(void *cookie, int index, smc_key *key);
+ int (*get_key_info)(void *cookie, smc_key key, struct apple_smc_key_info *info);
+};
+
+struct apple_smc {
+ struct device *dev;
+
+ void *be_cookie;
+ const struct apple_smc_backend_ops *be;
+
+ struct mutex mutex;
+
+ u32 key_count;
+ smc_key first_key;
+ smc_key last_key;
+
+ struct blocking_notifier_head event_handlers;
+};
+
+static inline int apple_smc_read(struct apple_smc *smc, smc_key key,
+ void *buf, size_t size)
+{
+ int ret;
+
+ mutex_lock(&smc->mutex);
+ ret = smc->be->read_key(smc->be_cookie, key, buf, size);
+ mutex_unlock(&smc->mutex);
+
+ return ret;
+}
+
+static inline int apple_smc_write(struct apple_smc *smc, smc_key key,
+ void *buf, size_t size)
+{
+ int ret;
+
+ mutex_lock(&smc->mutex);
+ ret = smc->be->write_key(smc->be_cookie, key, buf, size);
+ mutex_unlock(&smc->mutex);
+
+ return ret;
+}
+
+static inline int apple_smc_write_atomic(struct apple_smc *smc, smc_key key,
+ void *buf, size_t size)
+{
+ int ret;
+
+ /*
+ * Will fail if SMC is busy. This is only used by SMC reboot/poweroff
+ * final calls, so it shouldn't really matter at that point.
+ */
+ if (!mutex_trylock(&smc->mutex))
+ return -EBUSY;
+
+ ret = smc->be->write_key_atomic(smc->be_cookie, key, buf, size);
+ mutex_unlock(&smc->mutex);
+
+ return ret;
+}
+
+static inline int apple_smc_rw(struct apple_smc *smc, smc_key key,
+ void *wbuf, size_t wsize,
+ void *rbuf, size_t rsize)
+{
+ int ret;
+
+ mutex_lock(&smc->mutex);
+ ret = smc->be->rw_key(smc->be_cookie, key, wbuf, wsize, rbuf, rsize);
+ mutex_unlock(&smc->mutex);
+
+ return ret;
+}
+
+static inline int apple_smc_get_key_count(struct apple_smc *smc)
+{
+ return smc->key_count;
+}
+
+static inline int apple_smc_get_key_by_index(struct apple_smc *smc, int index,
+ smc_key *key)
+{
+ int ret;
+
+ mutex_lock(&smc->mutex);
+ ret = smc->be->get_key_by_index(smc->be_cookie, index, key);
+ mutex_unlock(&smc->mutex);
+
+ return ret;
+}
+
+static inline int apple_smc_get_key_info(struct apple_smc *smc, smc_key key,
+ struct apple_smc_key_info *info)
+{
+ int ret;
+
+ mutex_lock(&smc->mutex);
+ ret = smc->be->get_key_info(smc->be_cookie, key, info);
+ mutex_unlock(&smc->mutex);
+
+ return ret;
+}
-int apple_smc_get_key_count(struct apple_smc *smc);
int apple_smc_find_first_key_index(struct apple_smc *smc, smc_key key);
-int apple_smc_get_key_by_index(struct apple_smc *smc, int index, smc_key *key);
-int apple_smc_get_key_info(struct apple_smc *smc, smc_key key, struct apple_smc_key_info *info);
static inline bool apple_smc_key_exists(struct apple_smc *smc, smc_key key)
{
@@ -80,7 +185,16 @@ static inline int apple_smc_read_flag(struct apple_smc *smc, smc_key key)
}
#define apple_smc_write_flag apple_smc_write_u8
-int apple_smc_register_notifier(struct apple_smc *smc, struct notifier_block *n);
-int apple_smc_unregister_notifier(struct apple_smc *smc, struct notifier_block *n);
+static inline int apple_smc_register_notifier(struct apple_smc *smc,
+ struct notifier_block *n)
+{
+ return blocking_notifier_chain_register(&smc->event_handlers, n);
+}
+
+static inline int apple_smc_unregister_notifier(struct apple_smc *smc,
+ struct notifier_block *n)
+{
+ return blocking_notifier_chain_unregister(&smc->event_handlers, n);
+}
#endif
diff --git a/drivers/platform/Kconfig b/drivers/platform/Kconfig index b437847b6237..5f8b9bcdb830 100644 --- a/drivers/platform/Kconfig +++ b/drivers/platform/Kconfig @@ -13,4 +13,6 @@ source "drivers/platform/olpc/Kconfig" source "drivers/platform/surface/Kconfig" +source "drivers/platform/apple/Kconfig" + source "drivers/platform/x86/Kconfig" diff --git a/drivers/platform/Makefile b/drivers/platform/Makefile index 4de08ef4ec9d..3e5d5039a28c 100644 --- a/drivers/platform/Makefile +++ b/drivers/platform/Makefile @@ -10,3 +10,4 @@ obj-$(CONFIG_OLPC_EC) += olpc/ obj-$(CONFIG_GOLDFISH) += goldfish/ obj-$(CONFIG_CHROME_PLATFORMS) += chrome/ obj-$(CONFIG_SURFACE_PLATFORMS) += surface/ +obj-$(CONFIG_APPLE_PLATFORMS) += apple/ diff --git a/drivers/platform/apple/Kconfig b/drivers/platform/apple/Kconfig new file mode 100644 index 000000000000..42525aa9fbbe --- /dev/null +++ b/drivers/platform/apple/Kconfig @@ -0,0 +1,49 @@ +# SPDX-License-Identifier: GPL-2.0 +# +# Apple Platform-Specific Drivers +# + +menuconfig APPLE_PLATFORMS + bool "Apple Mac Platform-Specific Device Drivers" + default y + help + Say Y here to get to see options for platform-specific device drivers + for Apple devices. This option alone does not add any kernel code. + + If you say N, all options in this submenu will be skipped and disabled. + +if APPLE_PLATFORMS + +config APPLE_SMC + tristate "Apple SMC Driver" + depends on ARCH_APPLE || COMPILE_TEST + default ARCH_APPLE + select MFD_CORE + help + Build support for the Apple System Management Controller present in + Apple Macs. This driver currently supports the SMC in Apple Silicon + Macs. For x86 Macs, see the applesmc driver (SENSORS_APPLESMC). + + Say Y here if you have an Apple Silicon Mac. + + To compile this driver as a module, choose M here: the module will + be called macsmc. + +if APPLE_SMC + +config APPLE_SMC_RTKIT + tristate "RTKit (Apple Silicon) backend" + depends on ARCH_APPLE || COMPILE_TEST + depends on APPLE_RTKIT + default ARCH_APPLE + help + Build support for SMC communications via the RTKit backend. This is + required for Apple Silicon Macs. + + Say Y here if you have an Apple Silicon Mac. + + To compile this driver as a module, choose M here: the module will + be called macsmc-rtkit. + +endif +endif diff --git a/drivers/platform/apple/Makefile b/drivers/platform/apple/Makefile new file mode 100644 index 000000000000..79fac195398b --- /dev/null +++ b/drivers/platform/apple/Makefile @@ -0,0 +1,11 @@ +# SPDX-License-Identifier: GPL-2.0 +# +# Makefile for linux/drivers/platform/apple +# Apple Platform-Specific Drivers +# + +macsmc-y += smc_core.o +macsmc-rtkit-y += smc_rtkit.o + +obj-$(CONFIG_APPLE_SMC) += macsmc.o +obj-$(CONFIG_APPLE_SMC_RTKIT) += macsmc-rtkit.o diff --git a/drivers/platform/apple/smc.h b/drivers/platform/apple/smc.h new file mode 100644 index 000000000000..8ae51887b2c5 --- /dev/null +++ b/drivers/platform/apple/smc.h @@ -0,0 +1,28 @@ +// SPDX-License-Identifier: GPL-2.0-only OR MIT +/* + * Apple SMC internal core definitions + * Copyright (C) The Asahi Linux Contributors + */ + +#ifndef _SMC_H +#define _SMC_H + +#include <linux/mfd/macsmc.h> + +struct apple_smc_backend_ops { + int (*read_key)(void *cookie, smc_key key, void *buf, size_t size); + int (*write_key)(void *cookie, smc_key key, void *buf, size_t size); + int (*write_key_atomic)(void *cookie, smc_key key, void *buf, size_t size); + int (*rw_key)(void *cookie, smc_key key, void *wbuf, size_t wsize, + void *rbuf, size_t rsize); + int (*get_key_by_index)(void *cookie, int index, smc_key *key); + int (*get_key_info)(void *cookie, smc_key key, struct apple_smc_key_info *info); +}; + +struct apple_smc *apple_smc_probe(struct device *dev, const struct apple_smc_backend_ops *ops, + void *cookie); +void *apple_smc_get_cookie(struct apple_smc *smc); +int apple_smc_remove(struct apple_smc *smc); +void apple_smc_event_received(struct apple_smc *smc, uint32_t event); + +#endif diff --git a/drivers/platform/apple/smc_core.c b/drivers/platform/apple/smc_core.c new file mode 100644 index 000000000000..daf029cd072f --- /dev/null +++ b/drivers/platform/apple/smc_core.c @@ -0,0 +1,249 @@ +// SPDX-License-Identifier: GPL-2.0-only OR MIT +/* + * Apple SMC core framework + * Copyright The Asahi Linux Contributors + */ + +#include <linux/device.h> +#include <linux/mfd/core.h> +#include <linux/mutex.h> +#include <linux/notifier.h> +#include "smc.h" + +struct apple_smc { + struct device *dev; + + void *be_cookie; + const struct apple_smc_backend_ops *be; + + struct mutex mutex; + + u32 key_count; + smc_key first_key; + smc_key last_key; + + struct blocking_notifier_head event_handlers; +}; + +static const struct mfd_cell apple_smc_devs[] = { + { + .name = "macsmc-gpio", + }, + { + .name = "macsmc-hid", + }, + { + .name = "macsmc-power", + }, + { + .name = "macsmc-reboot", + }, + { + .name = "macsmc-rtc", + }, +}; + +int apple_smc_read(struct apple_smc *smc, smc_key key, void *buf, size_t size) +{ + int ret; + + mutex_lock(&smc->mutex); + ret = smc->be->read_key(smc->be_cookie, key, buf, size); + mutex_unlock(&smc->mutex); + + return ret; +} +EXPORT_SYMBOL(apple_smc_read); + +int apple_smc_write(struct apple_smc *smc, smc_key key, void *buf, size_t size) +{ + int ret; + + mutex_lock(&smc->mutex); + ret = smc->be->write_key(smc->be_cookie, key, buf, size); + mutex_unlock(&smc->mutex); + + return ret; +} +EXPORT_SYMBOL(apple_smc_write); + +int apple_smc_write_atomic(struct apple_smc *smc, smc_key key, void *buf, size_t size) +{ + int ret; + + /* + * Will fail if SMC is busy. This is only used by SMC reboot/poweroff + * final calls, so it doesn't really matter at that point. + */ + if (!mutex_trylock(&smc->mutex)) + return -EBUSY; + + ret = smc->be->write_key_atomic(smc->be_cookie, key, buf, size); + mutex_unlock(&smc->mutex); + + return ret; +} +EXPORT_SYMBOL(apple_smc_write_atomic); + +int apple_smc_rw(struct apple_smc *smc, smc_key key, void *wbuf, size_t wsize, + void *rbuf, size_t rsize) +{ + int ret; + + mutex_lock(&smc->mutex); + ret = smc->be->rw_key(smc->be_cookie, key, wbuf, wsize, rbuf, rsize); + mutex_unlock(&smc->mutex); + + return ret; +} +EXPORT_SYMBOL(apple_smc_rw); + +int apple_smc_get_key_by_index(struct apple_smc *smc, int index, smc_key *key) +{ + int ret; + + mutex_lock(&smc->mutex); + ret = smc->be->get_key_by_index(smc->be_cookie, index, key); + mutex_unlock(&smc->mutex); + + return ret; +} +EXPORT_SYMBOL(apple_smc_get_key_by_index); + +int apple_smc_get_key_info(struct apple_smc *smc, smc_key key, struct apple_smc_key_info *info) +{ + int ret; + + mutex_lock(&smc->mutex); + ret = smc->be->get_key_info(smc->be_cookie, key, info); + mutex_unlock(&smc->mutex); + + return ret; +} +EXPORT_SYMBOL(apple_smc_get_key_info); + +int apple_smc_find_first_key_index(struct apple_smc *smc, smc_key key) +{ + int start = 0, count = smc->key_count; + int ret; + + if (key <= smc->first_key) + return 0; + if (key > smc->last_key) + return smc->key_count; + + while (count > 1) { + int pivot = start + ((count - 1) >> 1); + smc_key pkey; + + ret = apple_smc_get_key_by_index(smc, pivot, &pkey); + if (ret < 0) + return ret; + + if (pkey == key) + return pivot; + + pivot++; + + if (pkey < key) { + count -= pivot - start; + start = pivot; + } else { + count = pivot - start; + } + } + + return start; +} +EXPORT_SYMBOL(apple_smc_find_first_key_index); + +int apple_smc_get_key_count(struct apple_smc *smc) +{ + return smc->key_count; +} +EXPORT_SYMBOL(apple_smc_get_key_count); + +void apple_smc_event_received(struct apple_smc *smc, uint32_t event) +{ + dev_dbg(smc->dev, "Event: 0x%08x\n", event); + blocking_notifier_call_chain(&smc->event_handlers, event, NULL); +} +EXPORT_SYMBOL(apple_smc_event_received); + +int apple_smc_register_notifier(struct apple_smc *smc, struct notifier_block *n) +{ + return blocking_notifier_chain_register(&smc->event_handlers, n); +} +EXPORT_SYMBOL(apple_smc_register_notifier); + +int apple_smc_unregister_notifier(struct apple_smc *smc, struct notifier_block *n) +{ + return blocking_notifier_chain_unregister(&smc->event_handlers, n); +} +EXPORT_SYMBOL(apple_smc_unregister_notifier); + +void *apple_smc_get_cookie(struct apple_smc *smc) +{ + return smc->be_cookie; +} +EXPORT_SYMBOL(apple_smc_get_cookie); + +struct apple_smc *apple_smc_probe(struct device *dev, const struct apple_smc_backend_ops *ops, void *cookie) +{ + struct apple_smc *smc; + u32 count; + int ret; + + smc = devm_kzalloc(dev, sizeof(*smc), GFP_KERNEL); + if (!smc) + return ERR_PTR(-ENOMEM); + + smc->dev = dev; + smc->be_cookie = cookie; + smc->be = ops; + mutex_init(&smc->mutex); + BLOCKING_INIT_NOTIFIER_HEAD(&smc->event_handlers); + + ret = apple_smc_read_u32(smc, SMC_KEY(#KEY), &count); + if (ret) + return ERR_PTR(dev_err_probe(dev, ret, "Failed to get key count")); + smc->key_count = be32_to_cpu(count); + + ret = apple_smc_get_key_by_index(smc, 0, &smc->first_key); + if (ret) + return ERR_PTR(dev_err_probe(dev, ret, "Failed to get first key")); + + ret = apple_smc_get_key_by_index(smc, smc->key_count - 1, &smc->last_key); + if (ret) + return ERR_PTR(dev_err_probe(dev, ret, "Failed to get last key")); + + /* Enable notifications */ + apple_smc_write_flag(smc, SMC_KEY(NTAP), 1); + + dev_info(dev, "Initialized (%d keys %p4ch..%p4ch)\n", + smc->key_count, &smc->first_key, &smc->last_key); + + dev_set_drvdata(dev, smc); + + ret = mfd_add_devices(dev, -1, apple_smc_devs, ARRAY_SIZE(apple_smc_devs), NULL, 0, NULL); + if (ret) + return ERR_PTR(dev_err_probe(dev, ret, "Subdevice initialization failed")); + + return smc; +} +EXPORT_SYMBOL(apple_smc_probe); + +int apple_smc_remove(struct apple_smc *smc) +{ + mfd_remove_devices(smc->dev); + + /* Disable notifications */ + apple_smc_write_flag(smc, SMC_KEY(NTAP), 1); + + return 0; +} +EXPORT_SYMBOL(apple_smc_remove); + +MODULE_AUTHOR("Hector Martin <marcan@marcan.st>"); +MODULE_LICENSE("Dual MIT/GPL"); +MODULE_DESCRIPTION("Apple SMC core"); diff --git a/drivers/platform/apple/smc_rtkit.c b/drivers/platform/apple/smc_rtkit.c new file mode 100644 index 000000000000..5b7c4c475bbb --- /dev/null +++ b/drivers/platform/apple/smc_rtkit.c @@ -0,0 +1,451 @@ +// SPDX-License-Identifier: GPL-2.0-only OR MIT +/* + * Apple SMC RTKit backend + * Copyright The Asahi Linux Contributors + */ + +#include <asm/unaligned.h> +#include <linux/delay.h> +#include <linux/device.h> +#include <linux/io.h> +#include <linux/ioport.h> +#include <linux/of.h> +#include <linux/of_platform.h> +#include <linux/soc/apple/rtkit.h> +#include "smc.h" + +#define SMC_ENDPOINT 0x20 + +/* Guess */ +#define SMC_SHMEM_SIZE 0x1000 + +#define SMC_MSG_READ_KEY 0x10 +#define SMC_MSG_WRITE_KEY 0x11 +#define SMC_MSG_GET_KEY_BY_INDEX 0x12 +#define SMC_MSG_GET_KEY_INFO 0x13 +#define SMC_MSG_INITIALIZE 0x17 +#define SMC_MSG_NOTIFICATION 0x18 +#define SMC_MSG_RW_KEY 0x20 + +#define SMC_DATA GENMASK(63, 32) +#define SMC_WSIZE GENMASK(31, 24) +#define SMC_SIZE GENMASK(23, 16) +#define SMC_ID GENMASK(15, 12) +#define SMC_MSG GENMASK(7, 0) +#define SMC_RESULT SMC_MSG + +#define SMC_RECV_TIMEOUT 100 + +struct apple_smc_rtkit { + struct device *dev; + struct apple_smc *core; + struct apple_rtkit *rtk; + + struct completion init_done; + bool initialized; + bool alive; + + struct resource *sram; + void __iomem *sram_base; + struct apple_rtkit_shmem shmem; + + unsigned int msg_id; + + bool atomic_pending; + struct completion cmd_done; + u64 cmd_ret; +}; + +static int apple_smc_rtkit_write_key_atomic(void *cookie, smc_key key, void *buf, size_t size) +{ + struct apple_smc_rtkit *smc = cookie; + int ret; + u64 msg; + u8 result; + + if (size > SMC_SHMEM_SIZE || size == 0) + return -EINVAL; + + if (!smc->alive) + return -EIO; + + memcpy_toio(smc->shmem.iomem, buf, size); + smc->msg_id = (smc->msg_id + 1) & 0xf; + msg = (FIELD_PREP(SMC_MSG, SMC_MSG_WRITE_KEY) | + FIELD_PREP(SMC_SIZE, size) | + FIELD_PREP(SMC_ID, smc->msg_id) | + FIELD_PREP(SMC_DATA, key)); + smc->atomic_pending = true; + + ret = apple_rtkit_send_message(smc->rtk, SMC_ENDPOINT, msg, NULL, true); + if (ret < 0) { + dev_err(smc->dev, "Failed to send command (%d)\n", ret); + return ret; + } + + while (smc->atomic_pending) { + ret = apple_rtkit_poll(smc->rtk); + if (ret < 0) { + dev_err(smc->dev, "RTKit poll failed (%llx)", msg); + return ret; + } + udelay(100); + } + + if (FIELD_GET(SMC_ID, smc->cmd_ret) != smc->msg_id) { + dev_err(smc->dev, "Command sequence mismatch (expected %d, got %d)\n", + smc->msg_id, (unsigned int)FIELD_GET(SMC_ID, smc->cmd_ret)); + return -EIO; + } + + result = FIELD_GET(SMC_RESULT, smc->cmd_ret); + if (result != 0) + return -result; + + return FIELD_GET(SMC_SIZE, smc->cmd_ret); +} + +static int apple_smc_cmd(struct apple_smc_rtkit *smc, u64 cmd, u64 arg, + u64 size, u64 wsize, u32 *ret_data) +{ + int ret; + u64 msg; + u8 result; + + if (!smc->alive) + return -EIO; + + reinit_completion(&smc->cmd_done); + + smc->msg_id = (smc->msg_id + 1) & 0xf; + msg = (FIELD_PREP(SMC_MSG, cmd) | + FIELD_PREP(SMC_SIZE, size) | + FIELD_PREP(SMC_WSIZE, wsize) | + FIELD_PREP(SMC_ID, smc->msg_id) | + FIELD_PREP(SMC_DATA, arg)); + + ret = apple_rtkit_send_message(smc->rtk, SMC_ENDPOINT, msg, NULL, false); + if (ret < 0) { + dev_err(smc->dev, "Failed to send command\n"); + return ret; + } + + do { + if (wait_for_completion_timeout(&smc->cmd_done, + msecs_to_jiffies(SMC_RECV_TIMEOUT)) == 0) { + dev_err(smc->dev, "Command timed out (%llx)", msg); + return -ETIMEDOUT; + } + if (FIELD_GET(SMC_ID, smc->cmd_ret) == smc->msg_id) + break; + dev_err(smc->dev, "Command sequence mismatch (expected %d, got %d)\n", + smc->msg_id, (unsigned int)FIELD_GET(SMC_ID, smc->cmd_ret)); + } while(1); + + result = FIELD_GET(SMC_RESULT, smc->cmd_ret); + if (result != 0) + return -result; + + if (ret_data) + *ret_data = FIELD_GET(SMC_DATA, smc->cmd_ret); + + return FIELD_GET(SMC_SIZE, smc->cmd_ret); +} + +static int _apple_smc_rtkit_read_key(struct apple_smc_rtkit *smc, smc_key key, + void *buf, size_t size, size_t wsize) +{ + int ret; + u32 rdata; + u64 cmd; + + if (size > SMC_SHMEM_SIZE || size == 0) + return -EINVAL; + + cmd = wsize ? SMC_MSG_RW_KEY : SMC_MSG_READ_KEY; + + ret = apple_smc_cmd(smc, cmd, key, size, wsize, &rdata); + if (ret < 0) + return ret; + + if (size <= 4) + memcpy(buf, &rdata, size); + else + memcpy_fromio(buf, smc->shmem.iomem, size); + + return ret; +} + +static int apple_smc_rtkit_read_key(void *cookie, smc_key key, void *buf, size_t size) +{ + return _apple_smc_rtkit_read_key(cookie, key, buf, size, 0); +} + +static int apple_smc_rtkit_write_key(void *cookie, smc_key key, void *buf, size_t size) +{ + struct apple_smc_rtkit *smc = cookie; + + if (size > SMC_SHMEM_SIZE || size == 0) + return -EINVAL; + + memcpy_toio(smc->shmem.iomem, buf, size); + return apple_smc_cmd(smc, SMC_MSG_WRITE_KEY, key, size, 0, NULL); +} + +static int apple_smc_rtkit_rw_key(void *cookie, smc_key key, + void *wbuf, size_t wsize, void *rbuf, size_t rsize) +{ + struct apple_smc_rtkit *smc = cookie; + + if (wsize > SMC_SHMEM_SIZE || wsize == 0) + return -EINVAL; + + memcpy_toio(smc->shmem.iomem, wbuf, wsize); + return _apple_smc_rtkit_read_key(smc, key, rbuf, rsize, wsize); +} + +static int apple_smc_rtkit_get_key_by_index(void *cookie, int index, smc_key *key) +{ + struct apple_smc_rtkit *smc = cookie; + int ret; + + ret = apple_smc_cmd(smc, SMC_MSG_GET_KEY_BY_INDEX, index, 0, 0, key); + + *key = swab32(*key); + return ret; +} + +static int apple_smc_rtkit_get_key_info(void *cookie, smc_key key, struct apple_smc_key_info *info) +{ + struct apple_smc_rtkit *smc = cookie; + u8 key_info[6]; + int ret; + + ret = apple_smc_cmd(smc, SMC_MSG_GET_KEY_INFO, key, 0, 0, NULL); + if (ret >= 0 && info) { + info->size = key_info[0]; + info->type_code = get_unaligned_be32(&key_info[1]); + info->flags = key_info[5]; + } + return ret; +} + +static const struct apple_smc_backend_ops apple_smc_rtkit_be_ops = { + .read_key = apple_smc_rtkit_read_key, + .write_key = apple_smc_rtkit_write_key, + .write_key_atomic = apple_smc_rtkit_write_key_atomic, + .rw_key = apple_smc_rtkit_rw_key, + .get_key_by_index = apple_smc_rtkit_get_key_by_index, + .get_key_info = apple_smc_rtkit_get_key_info, +}; + +static void apple_smc_rtkit_crashed(void *cookie) +{ + struct apple_smc_rtkit *smc = cookie; + + dev_err(smc->dev, "SMC crashed! Your system will reboot in a few seconds...\n"); + smc->alive = false; +} + +static int apple_smc_rtkit_shmem_setup(void *cookie, struct apple_rtkit_shmem *bfr) +{ + struct apple_smc_rtkit *smc = cookie; + struct resource res = { + .start = bfr->iova, + .end = bfr->iova + bfr->size - 1, + .name = "rtkit_map", + .flags = smc->sram->flags, + }; + + if (!bfr->iova) { + dev_err(smc->dev, "RTKit wants a RAM buffer\n"); + return -EIO; + } + + if (res.end < res.start || !resource_contains(smc->sram, &res)) { + dev_err(smc->dev, + "RTKit buffer request outside SRAM region: %pR", &res); + return -EFAULT; + } + + bfr->iomem = smc->sram_base + (res.start - smc->sram->start); + bfr->is_mapped = true; + + return 0; +} + +static void apple_smc_rtkit_shmem_destroy(void *cookie, struct apple_rtkit_shmem *bfr) +{ + // no-op +} + +static bool apple_smc_rtkit_recv_early(void *cookie, u8 endpoint, u64 message) +{ + struct apple_smc_rtkit *smc = cookie; + + if (endpoint != SMC_ENDPOINT) { + dev_err(smc->dev, "Received message for unknown endpoint 0x%x\n", endpoint); + return false; + } + + if (!smc->initialized) { + int ret; + + smc->shmem.iova = message; + smc->shmem.size = SMC_SHMEM_SIZE; + ret = apple_smc_rtkit_shmem_setup(smc, &smc->shmem); + if (ret < 0) + dev_err(smc->dev, "Failed to initialize shared memory\n"); + else + smc->alive = true; + smc->initialized = true; + complete(&smc->init_done); + } else if (FIELD_GET(SMC_MSG, message) == SMC_MSG_NOTIFICATION) { + /* Handle these in the RTKit worker thread */ + return false; + } else { + smc->cmd_ret = message; + if (smc->atomic_pending) { + smc->atomic_pending = false; + } else { + complete(&smc->cmd_done); + } + } + + return true; +} + +static void apple_smc_rtkit_recv(void *cookie, u8 endpoint, u64 message) +{ + struct apple_smc_rtkit *smc = cookie; + + if (endpoint != SMC_ENDPOINT) { + dev_err(smc->dev, "Received message for unknown endpoint 0x%x\n", endpoint); + return; + } + + if (FIELD_GET(SMC_MSG, message) != SMC_MSG_NOTIFICATION) { + dev_err(smc->dev, "Received unknown message from worker: 0x%llx\n", message); + return; + } + + apple_smc_event_received(smc->core, FIELD_GET(SMC_DATA, message)); +} + +static const struct apple_rtkit_ops apple_smc_rtkit_ops = { + .crashed = apple_smc_rtkit_crashed, + .recv_message = apple_smc_rtkit_recv, + .recv_message_early = apple_smc_rtkit_recv_early, + .shmem_setup = apple_smc_rtkit_shmem_setup, + .shmem_destroy = apple_smc_rtkit_shmem_destroy, +}; + +static int apple_smc_rtkit_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct apple_smc_rtkit *smc; + int ret; + + smc = devm_kzalloc(dev, sizeof(*smc), GFP_KERNEL); + if (!smc) + return -ENOMEM; + + smc->dev = dev; + + smc->sram = platform_get_resource_byname(pdev, IORESOURCE_MEM, "sram"); + if (!smc->sram) + return dev_err_probe(dev, EIO, + "No SRAM region"); + + smc->sram_base = devm_ioremap_resource(dev, smc->sram); + if (IS_ERR(smc->sram_base)) + return dev_err_probe(dev, PTR_ERR(smc->sram_base), + "Failed to map SRAM region"); + + smc->rtk = + devm_apple_rtkit_init(dev, smc, NULL, 0, &apple_smc_rtkit_ops); + if (IS_ERR(smc->rtk)) + return dev_err_probe(dev, PTR_ERR(smc->rtk), + "Failed to intialize RTKit"); + + ret = apple_rtkit_wake(smc->rtk); + if (ret != 0) + return dev_err_probe(dev, ret, + "Failed to wake up SMC"); + + ret = apple_rtkit_start_ep(smc->rtk, SMC_ENDPOINT); + if (ret != 0) { + dev_err(dev, "Failed to start endpoint"); + goto cleanup; + } + + init_completion(&smc->init_done); + init_completion(&smc->cmd_done); + + ret = apple_rtkit_send_message(smc->rtk, SMC_ENDPOINT, + FIELD_PREP(SMC_MSG, SMC_MSG_INITIALIZE), NULL, false); + if (ret < 0) + return dev_err_probe(dev, ret, + "Failed to send init message"); + + if (wait_for_completion_timeout(&smc->init_done, + msecs_to_jiffies(SMC_RECV_TIMEOUT)) == 0) { + ret = -ETIMEDOUT; + dev_err(dev, "Timed out initializing SMC"); + goto cleanup; + } + + if (!smc->alive) { + ret = -EIO; + goto cleanup; + } + + smc->core = apple_smc_probe(dev, &apple_smc_rtkit_be_ops, smc); + if (IS_ERR(smc->core)) { + ret = PTR_ERR(smc->core); + goto cleanup; + } + + return 0; + +cleanup: + /* Try to shut down RTKit, if it's not completely wedged */ + if (apple_rtkit_is_running(smc->rtk)) + apple_rtkit_quiesce(smc->rtk); + + return ret; +} + +static int apple_smc_rtkit_remove(struct platform_device *pdev) +{ + struct apple_smc *core = platform_get_drvdata(pdev); + struct apple_smc_rtkit *smc = apple_smc_get_cookie(core); + + apple_smc_remove(core); + + if (apple_rtkit_is_running(smc->rtk)) + apple_rtkit_quiesce(smc->rtk); + + return 0; +} + +static const struct of_device_id apple_smc_rtkit_of_match[] = { + { .compatible = "apple,smc" }, + {}, +}; +MODULE_DEVICE_TABLE(of, apple_smc_rtkit_of_match); + +static struct platform_driver apple_smc_rtkit_driver = { + .driver = { + .name = "macsmc-rtkit", + .owner = THIS_MODULE, + .of_match_table = apple_smc_rtkit_of_match, + }, + .probe = apple_smc_rtkit_probe, + .remove = apple_smc_rtkit_remove, +}; +module_platform_driver(apple_smc_rtkit_driver); + +MODULE_AUTHOR("Hector Martin <marcan@marcan.st>"); +MODULE_LICENSE("Dual MIT/GPL"); +MODULE_DESCRIPTION("Apple SMC RTKit backend driver"); diff --git a/include/linux/mfd/macsmc.h b/include/linux/mfd/macsmc.h new file mode 100644 index 000000000000..39b4dc4ca881 --- /dev/null +++ b/include/linux/mfd/macsmc.h @@ -0,0 +1,86 @@ +// SPDX-License-Identifier: GPL-2.0-only OR MIT +/* + * Apple SMC core definitions + * Copyright (C) The Asahi Linux Contributors + */ + +#ifndef _LINUX_MFD_MACSMC_H +#define _LINUX_MFD_MACSMC_H + +struct apple_smc; + +typedef u32 smc_key; + +#define SMC_KEY(s) (smc_key)(_SMC_KEY(#s)) +#define _SMC_KEY(s) (((s)[0] << 24) | ((s)[1] << 16) | ((s)[2] << 8) | (s)[3]) + +#define APPLE_SMC_READABLE BIT(7) +#define APPLE_SMC_WRITABLE BIT(6) +#define APPLE_SMC_FUNCTION BIT(4) + +struct apple_smc_key_info { + u8 size; + u32 type_code; + u8 flags; +}; + +int apple_smc_read(struct apple_smc *smc, smc_key key, void *buf, size_t size); +int apple_smc_write(struct apple_smc *smc, smc_key key, void *buf, size_t size); +int apple_smc_write_atomic(struct apple_smc *smc, smc_key key, void *buf, size_t size); +int apple_smc_rw(struct apple_smc *smc, smc_key key, void *wbuf, size_t wsize, + void *rbuf, size_t rsize); + +int apple_smc_get_key_count(struct apple_smc *smc); +int apple_smc_find_first_key_index(struct apple_smc *smc, smc_key key); +int apple_smc_get_key_by_index(struct apple_smc *smc, int index, smc_key *key); +int apple_smc_get_key_info(struct apple_smc *smc, smc_key key, struct apple_smc_key_info *info); + +static inline bool apple_smc_key_exists(struct apple_smc *smc, smc_key key) +{ + return apple_smc_get_key_info(smc, key, NULL) >= 0; +} + +#define APPLE_SMC_TYPE_OPS(type) \ + static inline int apple_smc_read_##type(struct apple_smc *smc, smc_key key, type *p) \ + { \ + int ret = apple_smc_read(smc, key, p, sizeof(*p)); \ + return (ret < 0) ? ret : ((ret != sizeof(*p)) ? -EINVAL : 0); \ + } \ + static inline int apple_smc_write_##type(struct apple_smc *smc, smc_key key, type p) \ + { \ + return apple_smc_write(smc, key, &p, sizeof(p)); \ + } \ + static inline int apple_smc_write_##type##_atomic(struct apple_smc *smc, smc_key key, type p) \ + { \ + return apple_smc_write_atomic(smc, key, &p, sizeof(p)); \ + } \ + static inline int apple_smc_rw_##type(struct apple_smc *smc, smc_key key, \ + type w, type *r) \ + { \ + int ret = apple_smc_rw(smc, key, &w, sizeof(w), r, sizeof(*r)); \ + return (ret < 0) ? ret : ((ret != sizeof(*r)) ? -EINVAL : 0); \ + } + +APPLE_SMC_TYPE_OPS(u64) +APPLE_SMC_TYPE_OPS(u32) +APPLE_SMC_TYPE_OPS(u16) +APPLE_SMC_TYPE_OPS(u8) +APPLE_SMC_TYPE_OPS(s64) +APPLE_SMC_TYPE_OPS(s32) +APPLE_SMC_TYPE_OPS(s16) +APPLE_SMC_TYPE_OPS(s8) + +static inline int apple_smc_read_flag(struct apple_smc *smc, smc_key key) +{ + u8 val; + int ret = apple_smc_read_u8(smc, key, &val); + if (ret < 0) + return ret; + return val ? 1 : 0; +} +#define apple_smc_write_flag apple_smc_write_u8 + +int apple_smc_register_notifier(struct apple_smc *smc, struct notifier_block *n); +int apple_smc_unregister_notifier(struct apple_smc *smc, struct notifier_block *n); + +#endif