Re: [PATCH linux-next v6 03/13] peci: Add support for PECI bus driver core

[Jae Hyun Yoo]

On 7/6/2018 12:03 AM, Lee Jones wrote:
> On Thu, 21 Jun 2018, Jae Hyun Yoo wrote:
>
> > This commit adds driver implementation for PECI bus core into linux
> > driver framework.
> >
> > Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@xxxxxxxxxxxxxxx>
> > Signed-off-by: Fengguang Wu <fengguang.wu@xxxxxxxxx>
> > Reviewed-by: Haiyue Wang <haiyue.wang@xxxxxxxxxxxxxxx>
> > Reviewed-by: James Feist <james.feist@xxxxxxxxxxxxxxx>
> > Reviewed-by: Vernon Mauery <vernon.mauery@xxxxxxxxxxxxxxx>
> > Cc: Alan Cox <alan@xxxxxxxxxxxxxxx>
> > Cc: Andrew Lunn <andrew@xxxxxxx>
> > Cc: Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>
> > Cc: Arnd Bergmann <arnd@xxxxxxxx>
> > Cc: Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx>
> > Cc: Fengguang Wu <fengguang.wu@xxxxxxxxx>
> > Cc: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
> > Cc: Jason M Biils <jason.m.bills@xxxxxxxxxxxxxxx>
> > Cc: Julia Cartwright <juliac@xxxxxxxxxxxx>
> > ---
> >   drivers/Kconfig                 |    2 +
> >   drivers/Makefile                |    1 +
> >   drivers/peci/Kconfig            |   12 +
> >   drivers/peci/Makefile           |    6 +
> >   drivers/peci/peci-core.c        | 1438 +++++++++++++++++++++++++++++++
> >   include/linux/peci.h            |  104 +++
> >   include/uapi/linux/peci-ioctl.h |  265 ++++++
> >   7 files changed, 1828 insertions(+)
> >   create mode 100644 drivers/peci/Kconfig
> >   create mode 100644 drivers/peci/Makefile
> >   create mode 100644 drivers/peci/peci-core.c
> >   create mode 100644 include/linux/peci.h
> >   create mode 100644 include/uapi/linux/peci-ioctl.h
>
> I'm struggling to see the justification for adding an entirely new
> subsystem for what looks like a bespoke, and perhaps more damning,
> *proprietary* 1-wire interface.  Especially one which has such
> limited use.  Between yourself and the other silicon chip vendors
> there must be 100s of these knocking about.  What makes this one
> special?  Or even useful?  Will there ever be more than a single
> source file in this directory?
>
> Don't get me wrong, I'm all for upstreaming code, but to create a new
> subsystem and bus for this kind of device seems very over the top.
>
> Since PECI's main purpose in life is Thermal Management, perhaps the
> whole thing should live in drivers/thermal or drivers/hwmon.  I've
> also seen you reference this as a kind of BMC too, so maybe
> drivers/platform/x86 would also be a nice place for it to reside.

I think, I already discussed about it with other reviewers but I have to
explain it again. Yes, PECI is yet not a popular interface but plays a
vital role in BMC system, so most of BMC chip vendors provide PECI
interface in their BMC chipsets, and it is the reason why the PECI
subsystem is added by this implementation because we should provide a
generic bus system for those BMC chipsets from multiple vendors so
that they can add their chip set specific driver to support that through
the generic interface. At this moment, the folder has only PECI core and
ASPEED PECI adapter drivers but other vendors' (i.e. Nuvoton) driver
could be added into the folder.

As you said, PECI's main purpose is Thermal Management but it's not
limited on that. As I already said in another thread and the cover
letter, PECI also can provide other sideband functions such as Platform
Manageability, Processor Tuning and Diagnostics and Failure Analysis.

AFAIK, only ARM core based BMC chipsets are rolled out so this
PECI implementation will be running on an ARM kernel at this moment.
It's not an x86 limited implementation.

Thanks,

Jae