| Message ID | 20220321081355.6802-6-quan@os.amperecomputing.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Add Ampere's Altra SMPro MFD and its child drivers | expand |
On Mon, Mar 21, 2022 at 03:13:51PM +0700, Quan Nguyen wrote: > This commit adds driver support for accessing various information > reported by Ampere's SMpro co-processor such as Boot Progress and > other miscellaneous data. > > Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com> No Documentation/ABI/ entries for your sysfs file? > +static ssize_t boot_progress_show(struct device *dev, struct device_attribute *da, char *buf) > +{ > + struct smpro_misc *misc = dev_get_drvdata(dev); > + u32 boot_progress; > + u8 current_stage; > + u8 boot_status; > + u8 boot_stage; > + u32 select; > + u32 reg_lo; > + u32 reg; > + int ret; > + > + /* Read current boot stage */ > + ret = regmap_read(misc->regmap, BOOTSTAGE_CUR_STAGE, ®); > + if (ret) > + return ret; > + > + current_stage = reg & 0xff; > + > + /* Read the boot progress */ > + ret = regmap_read(misc->regmap, BOOTSTAGE_SELECT, &select); > + if (ret) > + return ret; > + > + boot_stage = (select >> 8) & 0xff; > + boot_status = select & 0xff; > + > + if (boot_stage > current_stage) > + return -EINVAL; > + > + ret = regmap_read(misc->regmap, BOOTSTAGE_STATUS_LO, ®_lo); > + if (!ret) > + ret = regmap_read(misc->regmap, BOOTSTAGE_STATUS_HI, ®); > + if (ret) > + return ret; > + > + boot_progress = swab16(reg) << 16 | swab16(reg_lo); > + > + /* Tell firmware to provide next boot stage next time */ > + if (boot_stage < current_stage) { > + ret = regmap_write(misc->regmap, BOOTSTAGE_SELECT, ((select & 0xff00) | 0x1)); > + if (ret) > + return ret; > + } > + > + return snprintf(buf, PAGE_SIZE, "0x%02x 0x%02x 0x%08x\n", > + boot_stage, boot_status, boot_progress); sysfs_emit() please. Also, this is 3 different things, put all of these in different sysfs files. thanks, greg k-h
On 21/03/2022 15:21, Greg Kroah-Hartman wrote: > On Mon, Mar 21, 2022 at 03:13:51PM +0700, Quan Nguyen wrote: >> This commit adds driver support for accessing various information >> reported by Ampere's SMpro co-processor such as Boot Progress and >> other miscellaneous data. >> >> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com> > > No Documentation/ABI/ entries for your sysfs file? > Thank you, Greg, for a very quick review. I have put this file in other patch. >> +static ssize_t boot_progress_show(struct device *dev, struct device_attribute *da, char *buf) >> +{ >> + struct smpro_misc *misc = dev_get_drvdata(dev); >> + u32 boot_progress; >> + u8 current_stage; >> + u8 boot_status; >> + u8 boot_stage; >> + u32 select; >> + u32 reg_lo; >> + u32 reg; >> + int ret; >> + >> + /* Read current boot stage */ >> + ret = regmap_read(misc->regmap, BOOTSTAGE_CUR_STAGE, ®); >> + if (ret) >> + return ret; >> + >> + current_stage = reg & 0xff; >> + >> + /* Read the boot progress */ >> + ret = regmap_read(misc->regmap, BOOTSTAGE_SELECT, &select); >> + if (ret) >> + return ret; >> + >> + boot_stage = (select >> 8) & 0xff; >> + boot_status = select & 0xff; >> + >> + if (boot_stage > current_stage) >> + return -EINVAL; >> + >> + ret = regmap_read(misc->regmap, BOOTSTAGE_STATUS_LO, ®_lo); >> + if (!ret) >> + ret = regmap_read(misc->regmap, BOOTSTAGE_STATUS_HI, ®); >> + if (ret) >> + return ret; >> + >> + boot_progress = swab16(reg) << 16 | swab16(reg_lo); >> + >> + /* Tell firmware to provide next boot stage next time */ >> + if (boot_stage < current_stage) { >> + ret = regmap_write(misc->regmap, BOOTSTAGE_SELECT, ((select & 0xff00) | 0x1)); >> + if (ret) >> + return ret; >> + } >> + >> + return snprintf(buf, PAGE_SIZE, "0x%02x 0x%02x 0x%08x\n", >> + boot_stage, boot_status, boot_progress); > > sysfs_emit() please. > Thanks, Greg. Will switch to sysfs_emit() in my next version. > Also, this is 3 different things, put all of these in different sysfs > files. > > thanks, > > greg k-h Actually, no. It is single value of boot stage. Let me explain: The boot progress consists of three things together: boot_stage, boot_status and boot_progress and they have no meaning if reported them as three separate values: + boot_stage is to indicate the boot stage + boot_status is to report the result of that boot_stage: started, complete or error. + boot_progress is to report more extra information for the stage other than the boot_stage/boot_status. There is more information in the Documentation/ABI/testing sysfs patch. - Quan
On Mon, Mar 21, 2022 at 04:46:50PM +0700, Quan Nguyen wrote: > > > On 21/03/2022 15:21, Greg Kroah-Hartman wrote: > > On Mon, Mar 21, 2022 at 03:13:51PM +0700, Quan Nguyen wrote: > > > This commit adds driver support for accessing various information > > > reported by Ampere's SMpro co-processor such as Boot Progress and > > > other miscellaneous data. > > > > > > Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com> > > > > No Documentation/ABI/ entries for your sysfs file? > > > > Thank you, Greg, for a very quick review. > I have put this file in other patch. > > > > +static ssize_t boot_progress_show(struct device *dev, struct device_attribute *da, char *buf) > > > +{ > > > + struct smpro_misc *misc = dev_get_drvdata(dev); > > > + u32 boot_progress; > > > + u8 current_stage; > > > + u8 boot_status; > > > + u8 boot_stage; > > > + u32 select; > > > + u32 reg_lo; > > > + u32 reg; > > > + int ret; > > > + > > > + /* Read current boot stage */ > > > + ret = regmap_read(misc->regmap, BOOTSTAGE_CUR_STAGE, ®); > > > + if (ret) > > > + return ret; > > > + > > > + current_stage = reg & 0xff; > > > + > > > + /* Read the boot progress */ > > > + ret = regmap_read(misc->regmap, BOOTSTAGE_SELECT, &select); > > > + if (ret) > > > + return ret; > > > + > > > + boot_stage = (select >> 8) & 0xff; > > > + boot_status = select & 0xff; > > > + > > > + if (boot_stage > current_stage) > > > + return -EINVAL; > > > + > > > + ret = regmap_read(misc->regmap, BOOTSTAGE_STATUS_LO, ®_lo); > > > + if (!ret) > > > + ret = regmap_read(misc->regmap, BOOTSTAGE_STATUS_HI, ®); > > > + if (ret) > > > + return ret; > > > + > > > + boot_progress = swab16(reg) << 16 | swab16(reg_lo); > > > + > > > + /* Tell firmware to provide next boot stage next time */ > > > + if (boot_stage < current_stage) { > > > + ret = regmap_write(misc->regmap, BOOTSTAGE_SELECT, ((select & 0xff00) | 0x1)); > > > + if (ret) > > > + return ret; > > > + } > > > + > > > + return snprintf(buf, PAGE_SIZE, "0x%02x 0x%02x 0x%08x\n", > > > + boot_stage, boot_status, boot_progress); > > > > sysfs_emit() please. > > > Thanks, Greg. > > Will switch to sysfs_emit() in my next version. > > > Also, this is 3 different things, put all of these in different sysfs > > files. > > > > thanks, > > > > greg k-h > > Actually, no. It is single value of boot stage. You are displaying 3 things in a single line, those are 3 different things. > Let me explain: > The boot progress consists of three things together: boot_stage, boot_status > and boot_progress and they have no meaning if reported them as three > separate values: > + boot_stage is to indicate the boot stage > + boot_status is to report the result of that boot_stage: started, complete > or error. > + boot_progress is to report more extra information for the stage other than > the boot_stage/boot_status. Why are these just not 3 different files? They describe three different things, please do not EVER force userspace to parse a sysfs file other than a single value. thanks, greg k-h
On 21/03/2022 17:02, Greg Kroah-Hartman wrote: > On Mon, Mar 21, 2022 at 04:46:50PM +0700, Quan Nguyen wrote: >> >> >> On 21/03/2022 15:21, Greg Kroah-Hartman wrote: >>> On Mon, Mar 21, 2022 at 03:13:51PM +0700, Quan Nguyen wrote: >>>> This commit adds driver support for accessing various information >>>> reported by Ampere's SMpro co-processor such as Boot Progress and >>>> other miscellaneous data. >>>> >>>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com> >>> >>> No Documentation/ABI/ entries for your sysfs file? >>> >> >> Thank you, Greg, for a very quick review. >> I have put this file in other patch. >> >>>> +static ssize_t boot_progress_show(struct device *dev, struct device_attribute *da, char *buf) >>>> +{ >>>> + struct smpro_misc *misc = dev_get_drvdata(dev); >>>> + u32 boot_progress; >>>> + u8 current_stage; >>>> + u8 boot_status; >>>> + u8 boot_stage; >>>> + u32 select; >>>> + u32 reg_lo; >>>> + u32 reg; >>>> + int ret; >>>> + >>>> + /* Read current boot stage */ >>>> + ret = regmap_read(misc->regmap, BOOTSTAGE_CUR_STAGE, ®); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + current_stage = reg & 0xff; >>>> + >>>> + /* Read the boot progress */ >>>> + ret = regmap_read(misc->regmap, BOOTSTAGE_SELECT, &select); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + boot_stage = (select >> 8) & 0xff; >>>> + boot_status = select & 0xff; >>>> + >>>> + if (boot_stage > current_stage) >>>> + return -EINVAL; >>>> + >>>> + ret = regmap_read(misc->regmap, BOOTSTAGE_STATUS_LO, ®_lo); >>>> + if (!ret) >>>> + ret = regmap_read(misc->regmap, BOOTSTAGE_STATUS_HI, ®); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + boot_progress = swab16(reg) << 16 | swab16(reg_lo); >>>> + >>>> + /* Tell firmware to provide next boot stage next time */ >>>> + if (boot_stage < current_stage) { >>>> + ret = regmap_write(misc->regmap, BOOTSTAGE_SELECT, ((select & 0xff00) | 0x1)); >>>> + if (ret) >>>> + return ret; >>>> + } >>>> + >>>> + return snprintf(buf, PAGE_SIZE, "0x%02x 0x%02x 0x%08x\n", >>>> + boot_stage, boot_status, boot_progress); >>> >>> sysfs_emit() please. >>> >> Thanks, Greg. >> >> Will switch to sysfs_emit() in my next version. >> >>> Also, this is 3 different things, put all of these in different sysfs >>> files. >>> >>> thanks, >>> >>> greg k-h >> >> Actually, no. It is single value of boot stage. > > You are displaying 3 things in a single line, those are 3 different > things. > >> Let me explain: >> The boot progress consists of three things together: boot_stage, boot_status >> and boot_progress and they have no meaning if reported them as three >> separate values: >> + boot_stage is to indicate the boot stage >> + boot_status is to report the result of that boot_stage: started, complete >> or error. >> + boot_progress is to report more extra information for the stage other than >> the boot_stage/boot_status. > > Why are these just not 3 different files? They describe three different > things, please do not EVER force userspace to parse a sysfs file other > than a single value. > Thanks Greg for the comment. As there are multiple boot stages that occur even before the firmware is able to report, the firmware will stores all the stages and make them ready for later access. Later, on each access, ie: read to sysfs, the earliest boot stages will be read out and the next stage info is made ready for the next read by clearing the current reported stage. As these three piece of info only make sense when put together, we chose to providing a single file only so that when the sysfs is read, we can provide them as a single value and do the clearing so that the next read will return with the next stage. My intention for next version is to report a single 12-byte hex value for this sysfs. - Quan
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index 4e1a0b451f3d..f1371d6584e8 100644 --- a/drivers/misc/Kconfig +++ b/drivers/misc/Kconfig @@ -183,6 +183,13 @@ config SMPRO_ERRMON If you say yes here you get support for error monitor function provides by Ampere Computing's SoC with SMpro processor. +config SMPRO_MISC + tristate "Ampere Computing SMPro miscellaneous driver" + depends on MFD_SMPRO || COMPILE_TEST + help + If you say yes here you get support for the miscellaleous function + provides by Ampere Computing's SoC with SMpro processor. + config CS5535_MFGPT tristate "CS5535/CS5536 Geode Multi-Function General Purpose Timer (MFGPT) support" depends on MFD_CS5535 diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index 483308a6e113..e61e462924d0 100644 --- a/drivers/misc/Makefile +++ b/drivers/misc/Makefile @@ -24,6 +24,7 @@ obj-$(CONFIG_KGDB_TESTS) += kgdbts.o obj-$(CONFIG_SGI_XP) += sgi-xp/ obj-$(CONFIG_SGI_GRU) += sgi-gru/ obj-$(CONFIG_SMPRO_ERRMON) += smpro-errmon.o +obj-$(CONFIG_SMPRO_MISC) += smpro-misc.o obj-$(CONFIG_CS5535_MFGPT) += cs5535-mfgpt.o obj-$(CONFIG_GEHC_ACHC) += gehc-achc.o obj-$(CONFIG_HP_ILO) += hpilo.o diff --git a/drivers/misc/smpro-misc.c b/drivers/misc/smpro-misc.c new file mode 100644 index 000000000000..fadaa66a699e --- /dev/null +++ b/drivers/misc/smpro-misc.c @@ -0,0 +1,177 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Ampere Computing SoC's SMpro Misc Driver + * + * Copyright (c) 2022, Ampere Computing LLC + */ +#include <linux/mod_devicetable.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> + +/* Boot Stage/Progress Registers */ +#define BOOTSTAGE_SELECT 0xB0 +#define BOOTSTAGE_STATUS_LO 0xB1 +#define BOOTSTAGE_CUR_STAGE 0xB2 +#define BOOTSTAGE_STATUS_HI 0xB3 + +/* SOC State Registers */ +#define SOC_POWER_LIMIT 0xE5 + +/* Boot stages */ +enum { + BOOTSTAGE_SMPRO = 0, + BOOTSTAGE_PMPRO, + BOOTSTAGE_ATF_BL1, + BOOTSTAGE_DDR_INIT, + BOOTSTAGE_DDR_INIT_PROGRESS, + BOOTSTAGE_ATF_BL2, + BOOTSTAGE_ATF_BL31, + BOOTSTAGE_ATF_BL32, + BOOTSTAGE_UEFI, + BOOTSTAGE_OS, + BOOTSTAGE_MAX +}; + +struct smpro_misc { + struct regmap *regmap; +}; + +static ssize_t boot_progress_show(struct device *dev, struct device_attribute *da, char *buf) +{ + struct smpro_misc *misc = dev_get_drvdata(dev); + u32 boot_progress; + u8 current_stage; + u8 boot_status; + u8 boot_stage; + u32 select; + u32 reg_lo; + u32 reg; + int ret; + + /* Read current boot stage */ + ret = regmap_read(misc->regmap, BOOTSTAGE_CUR_STAGE, ®); + if (ret) + return ret; + + current_stage = reg & 0xff; + + /* Read the boot progress */ + ret = regmap_read(misc->regmap, BOOTSTAGE_SELECT, &select); + if (ret) + return ret; + + boot_stage = (select >> 8) & 0xff; + boot_status = select & 0xff; + + if (boot_stage > current_stage) + return -EINVAL; + + ret = regmap_read(misc->regmap, BOOTSTAGE_STATUS_LO, ®_lo); + if (!ret) + ret = regmap_read(misc->regmap, BOOTSTAGE_STATUS_HI, ®); + if (ret) + return ret; + + boot_progress = swab16(reg) << 16 | swab16(reg_lo); + + /* Tell firmware to provide next boot stage next time */ + if (boot_stage < current_stage) { + ret = regmap_write(misc->regmap, BOOTSTAGE_SELECT, ((select & 0xff00) | 0x1)); + if (ret) + return ret; + } + + return snprintf(buf, PAGE_SIZE, "0x%02x 0x%02x 0x%08x\n", + boot_stage, boot_status, boot_progress); +} + +static DEVICE_ATTR_RO(boot_progress); + +static ssize_t soc_power_limit_show(struct device *dev, struct device_attribute *da, char *buf) +{ + struct smpro_misc *misc = dev_get_drvdata(dev); + unsigned int value; + int ret; + + ret = regmap_read(misc->regmap, SOC_POWER_LIMIT, &value); + if (ret) + return ret; + + return snprintf(buf, PAGE_SIZE, "%d\n", value); +} + +static ssize_t soc_power_limit_store(struct device *dev, struct device_attribute *da, + const char *buf, size_t count) +{ + struct smpro_misc *misc = dev_get_drvdata(dev); + unsigned long val; + s32 ret; + + ret = kstrtoul(buf, 0, &val); + if (ret) + return ret; + + ret = regmap_write(misc->regmap, SOC_POWER_LIMIT, (unsigned int)val); + if (ret) + return -EPROTO; + + return count; +} + +static DEVICE_ATTR_RW(soc_power_limit); + +static struct attribute *smpro_misc_attrs[] = { + &dev_attr_boot_progress.attr, + &dev_attr_soc_power_limit.attr, + NULL +}; + +static const struct attribute_group smpro_misc_attr_group = { + .attrs = smpro_misc_attrs +}; + +static int smpro_misc_probe(struct platform_device *pdev) +{ + struct smpro_misc *misc; + int ret; + + misc = devm_kzalloc(&pdev->dev, sizeof(struct smpro_misc), GFP_KERNEL); + if (!misc) + return -ENOMEM; + + platform_set_drvdata(pdev, misc); + + misc->regmap = dev_get_regmap(pdev->dev.parent, NULL); + if (!misc->regmap) + return -ENODEV; + + ret = sysfs_create_group(&pdev->dev.kobj, &smpro_misc_attr_group); + if (ret) + dev_err(&pdev->dev, "SMPro misc sysfs registration failed\n"); + + return 0; +} + +static int smpro_misc_remove(struct platform_device *pdev) +{ + sysfs_remove_group(&pdev->dev.kobj, &smpro_misc_attr_group); + pr_info("SMPro misc sysfs entries removed"); + + return 0; +} + +static struct platform_driver smpro_misc_driver = { + .probe = smpro_misc_probe, + .remove = smpro_misc_remove, + .driver = { + .name = "smpro-misc", + }, +}; + +module_platform_driver(smpro_misc_driver); + +MODULE_AUTHOR("Tung Nguyen <tungnguyen@os.amperecomputing.com>"); +MODULE_AUTHOR("Quan Nguyen <quan@os.amperecomputing.com>"); +MODULE_DESCRIPTION("Ampere Altra SMpro Misc driver"); +MODULE_LICENSE("GPL");
This commit adds driver support for accessing various information reported by Ampere's SMpro co-processor such as Boot Progress and other miscellaneous data. Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com> --- Changes in v7: + Fix wrong return type of *_show/store() functions [kernel robot test] + Adjust patch order to remove dependence with smpro-mfd [Lee Jones] Changes in v6: + First introduced in v6 [Quan] drivers/misc/Kconfig | 7 ++ drivers/misc/Makefile | 1 + drivers/misc/smpro-misc.c | 177 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 185 insertions(+) create mode 100644 drivers/misc/smpro-misc.c