lkml.org 
[lkml]   [2018]   [Oct]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v8 08/12] mfd: intel-peci-client: Add PECI client MFD driver
From
Date
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 */
>

\
 
 \ /
  Last update: 2018-10-24 23:12    [W:0.108 / U:7.044 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site