Re: [PATCH v7 5/9] misc: smpro-misc: Add Ampere's Altra SMpro misc driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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@xxxxxxxxxxxxxxxxxxxxxx>

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, &reg);
+	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, &reg_lo);
+	if (!ret)
+		ret = regmap_read(misc->regmap, BOOTSTAGE_STATUS_HI, &reg);
+	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



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux