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

[Quan Nguyen]

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 at 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, &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