Re: [PATCH v8 08/12] mfd: intel-peci-client: Add PECI client MFD driver

[Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>]

Hi Lee,

On 10/24/2018 3:59 AM, Lee Jones wrote:
> On Tue, 18 Sep 2018, Jae Hyun Yoo wrote:
> 
>> This commit adds PECI client MFD driver.
>>

<snip>

>> +config MFD_INTEL_PECI_CLIENT
>> +	bool "Intel PECI client"
>> +	depends on (PECI || COMPILE_TEST)
>> +	select MFD_CORE
>> +	help
>> +	  If you say yes to this option, support will be included for the
>> +	  multi-functional Intel PECI (Platform Environment Control Interface)
> 
> Remove 'multi-functional' from this sentence.
> 

Will remove the word.

<snip>

>> +static struct mfd_cell peci_functions[] = {
>> +	{
>> +		.name = "peci-cputemp",
>> +	},
>> +	{
>> +		.name = "peci-dimmtemp",
>> +	},
> 
> 	{ .name = "peci-cputemp", },
> 	{ .name = "peci-dimmtemp", },
> 

Will change it like you suggested.

> Do these have 2 different drivers?  Where are you putting them?
> 

Yes, these have 2 different drivers as hwmon subsystem drivers.
I submitted them into this patch series.
Patch 10/12: https://lkml.org/lkml/2018/9/18/1524
Patch 11/12: https://lkml.org/lkml/2018/9/18/1523

>> +	/* TODO: Add additional PECI sideband functions into here */
> 
> When will this be done?
> 

I'm hoping it will be done by the end of this year.

>> +};
>> +
>> +static const struct cpu_gen_info cpu_gen_info_table[] = {
>> +	[CPU_GEN_HSX] = {
>> +		.family        = 6, /* Family code */
>> +		.model         = INTEL_FAM6_HASWELL_X,
>> +		.core_max      = CORE_MAX_ON_HSX,
>> +		.chan_rank_max = CHAN_RANK_MAX_ON_HSX,
>> +		.dimm_idx_max  = DIMM_IDX_MAX_ON_HSX },
>> +	[CPU_GEN_BRX] = {
>> +		.family        = 6, /* Family code */
>> +		.model         = INTEL_FAM6_BROADWELL_X,
>> +		.core_max      = CORE_MAX_ON_BDX,
>> +		.chan_rank_max = CHAN_RANK_MAX_ON_BDX,
>> +		.dimm_idx_max  = DIMM_IDX_MAX_ON_BDX },
>> +	[CPU_GEN_SKX] = {
>> +		.family        = 6, /* Family code */
>> +		.model         = INTEL_FAM6_SKYLAKE_X,
>> +		.core_max      = CORE_MAX_ON_SKX,
>> +		.chan_rank_max = CHAN_RANK_MAX_ON_SKX,
>> +		.dimm_idx_max  = DIMM_IDX_MAX_ON_SKX },
> 
> The '},'s should go on the line below.
> 

Okay. Will fix it.

>> +};
>> +
>> +static int peci_client_get_cpu_gen_info(struct peci_mfd *priv)
> 
> Remove all mention of 'mfd'.  It's not a thing.
> 

Will remove 'mfd'.

>> +{
>> +	u32 cpu_id;
>> +	int i, rc;
>> +
>> +	rc = peci_get_cpu_id(priv->adapter, priv->addr, &cpu_id);
>> +	if (rc)
>> +		return rc;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(cpu_gen_info_table); i++) {
>> +		if (FIELD_GET(CPU_ID_FAMILY_MASK, cpu_id) +
>> +			FIELD_GET(CPU_ID_EXT_FAMILY_MASK, cpu_id) ==
>> +				cpu_gen_info_table[i].family &&
>> +		    FIELD_GET(CPU_ID_MODEL_MASK, cpu_id) ==
>> +			FIELD_GET(LOWER_NIBBLE_MASK,
>> +				  cpu_gen_info_table[i].model) &&
>> +		    FIELD_GET(CPU_ID_EXT_MODEL_MASK, cpu_id) ==
>> +			FIELD_GET(UPPER_NIBBLE_MASK,
>> +				  cpu_gen_info_table[i].model)) {
>> +			break;
>> +		}
>> +	}
> 
> This is really read.  Please reformat it, even if you have to use
> local variables to make it more legible.
> 

Will reformat it using local variables for better readability as you
suggest.

>> +	if (i >= ARRAY_SIZE(cpu_gen_info_table))
>> +		return -ENODEV;
> 
> Do you really want to fail silently?
> 

No. I'll add a dev_err printing.

>> +	priv->gen_info = &cpu_gen_info_table[i];
> 
> If you do this in the for(), you can then test priv->gen_info instead
> of seeing if the iterator maxed out.  Much nicer I think.
> 

Yes, that would be much nicer. Will fix it.

>> +	return 0;
>> +}
>> +
>> +bool peci_temp_need_update(struct temp_data *temp)
>> +{
>> +	if (temp->valid &&
>> +	    time_before(jiffies, temp->last_updated + UPDATE_INTERVAL))
>> +		return false;
>> +
>> +	return true;
>> +}
>> +EXPORT_SYMBOL_GPL(peci_temp_need_update);
>> +
>> +void peci_temp_mark_updated(struct temp_data *temp)
>> +{
>> +	temp->valid = 1;
>> +	temp->last_updated = jiffies;
>> +}
>> +EXPORT_SYMBOL_GPL(peci_temp_mark_updated);
> 
> These are probably better suited as inline functions to be placed in
> a header file.  No need to export them, since they only use their own
> data.
> 

Yes, that makes sense. I'll change these to inline functions.

>> +int peci_client_command(struct peci_mfd *priv, enum peci_cmd cmd, void *vmsg)
>> +{
>> +	return peci_command(priv->adapter, cmd, vmsg);
>> +}
>> +EXPORT_SYMBOL_GPL(peci_client_command);
> 
> If you share the adaptor with the client, you can call peci_command()
> directly.  There should also be some locking in here somewhere too.
> 

Yes, the client->adapter can be referenced from child drivers. Will make
them call peci_command() directly.

>> +int peci_client_rd_pkg_cfg_cmd(struct peci_mfd *priv, u8 mbx_idx,
> 
> This is gobbledegook.  What's rd?  Read?
> 

Yes, the 'rd' means 'read'. I intended to keep command names as listed
in the PECI specification such as RdPkgConfig, WrPkgConfig and so on.
Should I change it to 'peci_client_read_package_config_command' ?

>> +			       u16 param, u8 *data)
>> +{
>> +	struct peci_rd_pkg_cfg_msg msg;
>> +	int rc;
>> +
>> +	msg.addr = priv->addr;
>> +	msg.index = mbx_idx;
>> +	msg.param = param;
>> +	msg.rx_len = 4;
>> +
>> +	rc = peci_command(priv->adapter, PECI_CMD_RD_PKG_CFG, &msg);
>> +	if (!rc)
>> +		memcpy(data, msg.pkg_config, 4);
>> +
>> +	return rc;
>> +}
>> +EXPORT_SYMBOL_GPL(peci_client_rd_pkg_cfg_cmd);
> 
> This too should be set-up as an inline function, not an exported one.
> 

Will change it to inline function.

>> +static int peci_client_probe(struct peci_client *client)
>> +{
>> +	struct device *dev = &client->dev;
>> +	struct peci_mfd *priv;
>> +	int rc;
> 
> 'ret' is more common.
> 

Will fix it.

>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	dev_set_drvdata(dev, priv);
>> +	priv->client = client;
>> +	priv->dev = dev;
> 
>> +	priv->adapter = client->adapter;
>> +	priv->addr = client->addr;
> 
> You're already saving client.  No need to save its individual
> attributes as well.
> 

I intended to reduce pointer referencing depth because adapter and addr 
are frequently used, but you are right. I'll remove adapter and addr 
from the struct.

>> +	priv->cpu_no = client->addr - PECI_BASE_ADDR;
> 
> This seems clunky.  Does the spec say that the addresses have to be in
> numerical order with no gaps?  What if someone chooses to implement 4
> CPUs at 0x30, 0x35, 0x45 and 0x60?
> 

The CPU address will be assigned by H/W strap settings in order. Also,
there would be a case of using some CPU slots left empty, so gaps could
be happen on the addresses but it's still acceptable.

> What do you then do with cpu_no?
> 

It is being used for child device id parameter when this code calls
devm_mfd_add_devices() below. Also, it is referenced from cputemp and
dimmtemp hwmon drivers but it isn't needed because the hwmon drivers
already know client->addr. I'll change cpu_no as a local variable in 
here for child device id and will change the hwmon drivers.

>> +	snprintf(priv->name, PECI_NAME_SIZE, "peci_client.cpu%d", priv->cpu_no);
>> +
>> +	rc = peci_client_get_cpu_gen_info(priv);
>> +	if (rc)
>> +		return rc;
>> +
>> +	rc = devm_mfd_add_devices(priv->dev, priv->cpu_no, peci_functions,
>> +				  ARRAY_SIZE(peci_functions), NULL, 0, NULL);
>> +	if (rc < 0) {
>> +		dev_err(priv->dev, "devm_mfd_add_devices failed: %d\n", rc);
> 
> This to too specific.
> 
>    "Failed to register child devices"
> 

Will fix it like you suggested.

>> +		return rc;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id peci_client_of_table[] = {
>> +	{ .compatible = "intel,peci-client" },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, peci_client_of_table);
>> +#endif
>> +
>> +static const struct peci_device_id peci_client_ids[] = {
>> +	{ .name = "peci-client", .driver_data = 0 },
> 
> Remove .driver_data if you're not going to use it.
> 

Okay. I'll remove .driver_data.

>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(peci, peci_client_ids);
>> +
>> +static struct peci_driver peci_client_driver = {
>> +	.probe    = peci_client_probe,
>> +	.id_table = peci_client_ids,
>> +	.driver   = {
>> +			.name           = "peci-client",
>> +			.of_match_table =
>> +				of_match_ptr(peci_client_of_table),
>> +	},
>> +};
> 
> Odd tabbing.
> 

Will fix the indentation.

>> +module_peci_driver(peci_client_driver);
>> +
>> +MODULE_AUTHOR("Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>");
>> +MODULE_DESCRIPTION("PECI client MFD driver");
> 
> Remove "MFD".
> 

Will remove it.

>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/mfd/intel-peci-client.h b/include/linux/mfd/intel-peci-client.h
>> new file mode 100644
>> index 000000000000..7ec272cddceb
>> --- /dev/null
>> +++ b/include/linux/mfd/intel-peci-client.h
>> @@ -0,0 +1,81 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/* Copyright (c) 2018 Intel Corporation */
>> +
>> +#ifndef __LINUX_MFD_PECI_CLIENT_H
>> +#define __LINUX_MFD_PECI_CLIENT_H
>> +
>> +#include <linux/peci.h>
>> +
>> +#if IS_ENABLED(CONFIG_X86)
>> +#include <asm/intel-family.h>
>> +#else
>> +/**
>> + * Architectures other than x86 cannot include the header file so define these
>> + * at here. These are needed for detecting type of client x86 CPUs behind a PECI
>> + * connection.
>> + */
>> +#define INTEL_FAM6_HASWELL_X   0x3F
>> +#define INTEL_FAM6_BROADWELL_X 0x4F
>> +#define INTEL_FAM6_SKYLAKE_X   0x55
>> +#endif
>> +
>> +#define LOWER_NIBBLE_MASK      GENMASK(3, 0)
>> +#define UPPER_NIBBLE_MASK      GENMASK(7, 4)
>> +
>> +#define CPU_ID_MODEL_MASK      GENMASK(7, 4)
>> +#define CPU_ID_FAMILY_MASK     GENMASK(11, 8)
>> +#define CPU_ID_EXT_MODEL_MASK  GENMASK(19, 16)
>> +#define CPU_ID_EXT_FAMILY_MASK GENMASK(27, 20)
>> +
>> +#define CORE_MAX_ON_HSX        18 /* Max number of cores on Haswell */
>> +#define CHAN_RANK_MAX_ON_HSX   8  /* Max number of channel ranks on Haswell */
>> +#define DIMM_IDX_MAX_ON_HSX    3  /* Max DIMM index per channel on Haswell */
>> +
>> +#define CORE_MAX_ON_BDX        24 /* Max number of cores on Broadwell */
>> +#define CHAN_RANK_MAX_ON_BDX   4  /* Max number of channel ranks on Broadwell */
>> +#define DIMM_IDX_MAX_ON_BDX    3  /* Max DIMM index per channel on Broadwell */
>> +
>> +#define CORE_MAX_ON_SKX        28 /* Max number of cores on Skylake */
>> +#define CHAN_RANK_MAX_ON_SKX   6  /* Max number of channel ranks on Skylake */
>> +#define DIMM_IDX_MAX_ON_SKX    2  /* Max DIMM index per channel on Skylake */
>> +
>> +#define CORE_NUMS_MAX          CORE_MAX_ON_SKX
>> +#define CHAN_RANK_MAX          CHAN_RANK_MAX_ON_HSX
>> +#define DIMM_IDX_MAX           DIMM_IDX_MAX_ON_HSX
>> +#define DIMM_NUMS_MAX          (CHAN_RANK_MAX * DIMM_IDX_MAX)
>> +
>> +#define TEMP_TYPE_PECI         6 /* Sensor type 6: Intel PECI */
>> +
>> +#define UPDATE_INTERVAL        HZ
>> +
>> +struct temp_data {
>> +	uint  valid;
>> +	s32   value;
>> +	ulong last_updated;
>> +};
> 
> This should not live in here.
> 

Will move it.

>> +struct cpu_gen_info {
>> +	u16  family;
>> +	u8   model;
>> +	uint core_max;
>> +	uint chan_rank_max;
>> +	uint dimm_idx_max;
>> +};
>> +
>> +struct peci_mfd {
> 
> Remove "_mfd".
> 

Will remove 'mfd'.

>> +	struct peci_client *client;
>> +	struct device *dev;
>> +	struct peci_adapter *adapter;
>> +	char name[PECI_NAME_SIZE];
> 
> What is this used for?
> 

This isn't needed. Thanks for your pointing out. I'll remove it.

>> +	u8 addr;
>> +	uint cpu_no;
>> +	const struct cpu_gen_info *gen_info;
> 
> Where is this used?
> 

It is referenced from cputemp and dimmtemp hwmon drivers to specify CPU
generation.

>> +};
> 
> Both of these structs need kerneldoc headers.
> 

Will add kerneldoc headers.

>> +bool peci_temp_need_update(struct temp_data *temp);
>> +void peci_temp_mark_updated(struct temp_data *temp);
> 
> These should live in a temperature driver.
> 

Agreed. I'll move it to temperature driver.

Thanks a lot for your careful review. I'll submit a new patch set soon.

Jae

>> +int  peci_client_command(struct peci_mfd *mfd, enum peci_cmd cmd, void *vmsg);
>> +int  peci_client_rd_pkg_cfg_cmd(struct peci_mfd *mfd, u8 mbx_idx,
>> +				u16 param, u8 *data);
>> +
>> +#endif /* __LINUX_MFD_PECI_CLIENT_H */
>