Re: [PATCH linux-next v5 08/13] drivers/mfd: Add PECI client mfd driver

[Lee Jones <lee.jones@linaro.org>]

On Thu, 14 Jun 2018, Jae Hyun Yoo wrote:

> Thanks for the review. Please see my inline answers.
> 
> On 6/12/2018 11:30 PM, Lee Jones wrote:
> > On Fri, 01 Jun 2018, Jae Hyun Yoo wrote:
> > 
> > > This commit adds PECI client mfd driver.
> > > 
> > > Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> > > Cc: Andrew Jeffery <andrew@aj.id.au>
> > > Cc: James Feist <james.feist@linux.intel.com>
> > > Cc: Jason M Biils <jason.m.bills@linux.intel.com>
> > > Cc: Joel Stanley <joel@jms.id.au>
> > > Cc: Vernon Mauery <vernon.mauery@linux.intel.com>
> > > ---
> > >   drivers/mfd/Kconfig             |  11 ++
> > >   drivers/mfd/Makefile            |   1 +
> > >   drivers/mfd/peci-client.c       | 205 ++++++++++++++++++++++++++++++++
> > >   include/linux/mfd/peci-client.h |  60 ++++++++++
> > >   4 files changed, 277 insertions(+)
> > >   create mode 100644 drivers/mfd/peci-client.c
> > >   create mode 100644 include/linux/mfd/peci-client.h

[...]

> > I thought you defined this device as a simple-mfd?
> > 
> > This leaves me wondering why you have a driver as well?
> 
> Then what is proper setting for it instead of a simple-mfd?

Drivers described as "simple-mfd" have a special function.  If you
don't know what that function is, I suggest that you do not need
it. :)

"simple-mfd" devices do not usually have drivers.

[...]

> > > +/**
> > > + * 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)
> > 
> > In the case of X86 based devices, are these being redefined?
> > 
> 
> No define even in x86 arch build. These masks just for PECI use cases.
> Also, the CPUs in this implementation is not for local CPUs, but for
> remote CPUs which are connected through PECI interface. It also means
> that this code can be running on non-x86 kernel too.

This is starting to sound like a 'remoteproc' driver, no?

> > > +enum cpu_gens {
> > > +	CPU_GEN_HSX = 0, /* Haswell Xeon */
> > > +	CPU_GEN_BRX,     /* Broadwell Xeon */
> > > +	CPU_GEN_SKX,     /* Skylake Xeon */
> > > +};
> > > +
> > > +static struct mfd_cell peci_functions[] = {
> > > +	{
> > > +		.name = "peci-cputemp",
> > > +		.of_compatible = "intel,peci-cputemp",
> > > +	},
> > > +	{
> > > +		.name = "peci-dimmtemp",
> > > +		.of_compatible = "intel,peci-dimmtemp",
> > > +	},
> > > +};
> > 
> > A couple of temp sensors?  This isn't looking very MFD-like.
> > 
> > What makes this an MFD?
> > 
> 
> Currently it has only a couple of temp sensors but it's just one of
> PECI function. Other PECI functions will be added later so it's the
> reason why it should be an MFD. Please see the following PECI sideband
> functions that I introduced in the cover letter of this patch set.
> 
> * Processor and DRAM thermal management
> * Platform Manageability
> * Processor Interface Tuning and Diagnostics
> * Failure Analysis

Are these all devices in their own right?

Will they each have drivers associated with them?

Is there a datasheet for this PECI device?

[...]

> > > +static struct peci_driver peci_client_driver = {
> > 
> > I'm pretty sure this will be NAKED by the platform Maintainer.
> 
> Does it have problems? Can you please give me a hint?

New bus types are usually only added for well defined, heavily used
buses which AFAIK all have their own subsystems.  Why can't you use
one of the existing bus types?  Platform is the most frequently used.

> > > +	.probe    = peci_client_probe,
> > > +	.id_table = peci_client_ids,
> > > +	.driver   = {
> > > +			.name           = "peci-client",
> > > +			.of_match_table =
> > > +				of_match_ptr(peci_client_of_table),
> > > +	},
> > > +};
> > > +module_peci_driver(peci_client_driver);

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog