Re: [PATCH v3 06/10] drivers/peci: Add a PECI adapter driver for Aspeed AST24xx/AST25xx
Hi Robin, On 4/17/2018 6:37 AM, Robin Murphy wrote: Just a drive-by nit: On 10/04/18 19:32, Jae Hyun Yoo wrote: [...] +#define PECI_CTRL_SAMPLING_MASK GENMASK(19, 16) +#define PECI_CTRL_SAMPLING(x) (((x) << 16) & PECI_CTRL_SAMPLING_MASK) +#define PECI_CTRL_SAMPLING_GET(x) (((x) & PECI_CTRL_SAMPLING_MASK) >> 16) FWIW, already provides functionality like this, so it might be worth taking a look at FIELD_{GET,PREP}() to save all these local definitions. Robin. Yes, that looks better. Thanks a lot for your pointing it out. Jae -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 06/10] drivers/peci: Add a PECI adapter driver for Aspeed AST24xx/AST25xx
Just a drive-by nit: On 10/04/18 19:32, Jae Hyun Yoo wrote: [...] +#define PECI_CTRL_SAMPLING_MASK GENMASK(19, 16) +#define PECI_CTRL_SAMPLING(x) (((x) << 16) & PECI_CTRL_SAMPLING_MASK) +#define PECI_CTRL_SAMPLING_GET(x) (((x) & PECI_CTRL_SAMPLING_MASK) >> 16) FWIW, already provides functionality like this, so it might be worth taking a look at FIELD_{GET,PREP}() to save all these local definitions. Robin. -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 06/10] drivers/peci: Add a PECI adapter driver for Aspeed AST24xx/AST25xx
Hello Joel, Thanks for sharing your time. Please see my answers inline. On 4/11/2018 4:51 AM, Joel Stanley wrote: Hello Jae, On 11 April 2018 at 04:02, Jae Hyun Yoo wrote: This commit adds PECI adapter driver implementation for Aspeed AST24xx/AST25xx. The driver is looking good! It looks like you've done some kind of review that we weren't allowed to see, which is a double edged sword - I might be asking about things that you've already spoken about with someone else. I'm only just learning about PECI, but I do have some general comments below. Yes, it took a hidden review process between v2 and v3. I know it's an unusual process but it was requested. Hopefully, change logs in cover letter could roughly provide the details. Thanks for your comments. --- drivers/peci/Kconfig | 28 +++ drivers/peci/Makefile | 3 + drivers/peci/peci-aspeed.c | 504 + 3 files changed, 535 insertions(+) create mode 100644 drivers/peci/peci-aspeed.c diff --git a/drivers/peci/Kconfig b/drivers/peci/Kconfig index 1fbc13f9e6c2..0e33420365de 100644 --- a/drivers/peci/Kconfig +++ b/drivers/peci/Kconfig @@ -14,4 +14,32 @@ config PECI processors and chipset components to external monitoring or control devices. + If you want PECI support, you should say Y here and also to the + specific driver for your bus adapter(s) below. + +if PECI + +# +# PECI hardware bus configuration +# + +menu "PECI Hardware Bus support" + +config PECI_ASPEED + tristate "Aspeed AST24xx/AST25xx PECI support" I think just saying ASPEED PECI support is enough. That way if the next ASPEED SoC happens to have PECI we don't need to update all of the help text :) Agreed. I'll change the description. + select REGMAP_MMIO + depends on OF + depends on ARCH_ASPEED || COMPILE_TEST + help + Say Y here if you want support for the Platform Environment Control + Interface (PECI) bus adapter driver on the Aspeed AST24XX and AST25XX + SoCs. + + This support is also available as a module. If so, the module + will be called peci-aspeed. + +endmenu + +endif # PECI + endmenu diff --git a/drivers/peci/Makefile b/drivers/peci/Makefile index 9e8615e0d3ff..886285e69765 100644 --- a/drivers/peci/Makefile +++ b/drivers/peci/Makefile @@ -4,3 +4,6 @@ # Core functionality obj-$(CONFIG_PECI) += peci-core.o + +# Hardware specific bus drivers +obj-$(CONFIG_PECI_ASPEED) += peci-aspeed.o diff --git a/drivers/peci/peci-aspeed.c b/drivers/peci/peci-aspeed.c new file mode 100644 index ..be2a1f327eb1 --- /dev/null +++ b/drivers/peci/peci-aspeed.c @@ -0,0 +1,504 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright (C) 2012-2017 ASPEED Technology Inc. +// Copyright (c) 2018 Intel Corporation + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#define DUMP_DEBUG 0 + +/* Aspeed PECI Registers */ +#define AST_PECI_CTRL 0x00 Nit: we use ASPEED instead of AST in the upstream kernel to distingush from the aspeed sdk drivers. If you feel strongly about this then I won't insist you change. Okay then, better change it now than later. Will change all defines. +#define AST_PECI_TIMING 0x04 +#define AST_PECI_CMD 0x08 +#define AST_PECI_CMD_CTRL 0x0c +#define AST_PECI_EXP_FCS 0x10 +#define AST_PECI_CAP_FCS 0x14 +#define AST_PECI_INT_CTRL 0x18 +#define AST_PECI_INT_STS 0x1c +#define AST_PECI_W_DATA0 0x20 +#define AST_PECI_W_DATA1 0x24 +#define AST_PECI_W_DATA2 0x28 +#define AST_PECI_W_DATA3 0x2c +#define AST_PECI_R_DATA0 0x30 +#define AST_PECI_R_DATA1 0x34 +#define AST_PECI_R_DATA2 0x38 +#define AST_PECI_R_DATA3 0x3c +#define AST_PECI_W_DATA4 0x40 +#define AST_PECI_W_DATA5 0x44 +#define AST_PECI_W_DATA6 0x48 +#define AST_PECI_W_DATA7 0x4c +#define AST_PECI_R_DATA4 0x50 +#define AST_PECI_R_DATA5 0x54 +#define AST_PECI_R_DATA6 0x58 +#define AST_PECI_R_DATA7 0x5c + +/* AST_PECI_CTRL - 0x00 : Control Register */ +#define PECI_CTRL_SAMPLING_MASK GENMASK(19, 16) +#define PECI_CTRL_SAMPLING(x) (((x) << 16) & PECI_CTRL_SAMPLING_MASK) +#define PECI_CTRL_SAMPLING_GET(x) (((x) & PECI_CTRL_SAMPLING_MASK) >> 16) +#define PECI_CTRL_READ_MODE_MASKGENMASK(13, 12) +#define PECI_CTRL_READ_MODE(x) (((x) << 12) & PECI_CTRL_READ_MODE_MASK) +#define PECI_CTRL_READ_MODE_GET(x) (((x) & PECI_CTRL_READ_MODE_MASK) >> 12) +#define PECI_CTRL_READ_MODE_COUNT BIT(12) +#define PECI_CTRL_READ_MODE_DBG BIT(13) +#define PECI_CTRL_CLK_SOURCE_MASK BIT(11) +#define PECI_CTRL_CLK_SOURCE(x) (((x) << 11) & PECI_CTRL_CLK_SOURCE_MASK) +#define PECI_CTRL_CLK_SOURCE_GET(x) (((x) & PECI_CTRL_CLK_SOURCE_MASK) >> 11) +#define PECI_CTRL_CLK_DIV_MASK GENMASK(10, 8) +#define PECI_CTRL_CLK_DIV(x)(((x) << 8) & PECI_CTRL_CLK_DIV_MASK) +#define PECI_CTRL_CLK_DIV_GET(x)(((x) & PECI_CTRL_CLK_DIV_MASK)
Re: [PATCH v3 06/10] drivers/peci: Add a PECI adapter driver for Aspeed AST24xx/AST25xx
Hello Jae, On 11 April 2018 at 04:02, Jae Hyun Yoo wrote: > This commit adds PECI adapter driver implementation for Aspeed > AST24xx/AST25xx. The driver is looking good! It looks like you've done some kind of review that we weren't allowed to see, which is a double edged sword - I might be asking about things that you've already spoken about with someone else. I'm only just learning about PECI, but I do have some general comments below. > --- > drivers/peci/Kconfig | 28 +++ > drivers/peci/Makefile | 3 + > drivers/peci/peci-aspeed.c | 504 > + > 3 files changed, 535 insertions(+) > create mode 100644 drivers/peci/peci-aspeed.c > > diff --git a/drivers/peci/Kconfig b/drivers/peci/Kconfig > index 1fbc13f9e6c2..0e33420365de 100644 > --- a/drivers/peci/Kconfig > +++ b/drivers/peci/Kconfig > @@ -14,4 +14,32 @@ config PECI > processors and chipset components to external monitoring or control > devices. > > + If you want PECI support, you should say Y here and also to the > + specific driver for your bus adapter(s) below. > + > +if PECI > + > +# > +# PECI hardware bus configuration > +# > + > +menu "PECI Hardware Bus support" > + > +config PECI_ASPEED > + tristate "Aspeed AST24xx/AST25xx PECI support" I think just saying ASPEED PECI support is enough. That way if the next ASPEED SoC happens to have PECI we don't need to update all of the help text :) > + select REGMAP_MMIO > + depends on OF > + depends on ARCH_ASPEED || COMPILE_TEST > + help > + Say Y here if you want support for the Platform Environment Control > + Interface (PECI) bus adapter driver on the Aspeed AST24XX and > AST25XX > + SoCs. > + > + This support is also available as a module. If so, the module > + will be called peci-aspeed. > + > +endmenu > + > +endif # PECI > + > endmenu > diff --git a/drivers/peci/Makefile b/drivers/peci/Makefile > index 9e8615e0d3ff..886285e69765 100644 > --- a/drivers/peci/Makefile > +++ b/drivers/peci/Makefile > @@ -4,3 +4,6 @@ > > # Core functionality > obj-$(CONFIG_PECI) += peci-core.o > + > +# Hardware specific bus drivers > +obj-$(CONFIG_PECI_ASPEED) += peci-aspeed.o > diff --git a/drivers/peci/peci-aspeed.c b/drivers/peci/peci-aspeed.c > new file mode 100644 > index ..be2a1f327eb1 > --- /dev/null > +++ b/drivers/peci/peci-aspeed.c > @@ -0,0 +1,504 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// Copyright (C) 2012-2017 ASPEED Technology Inc. > +// Copyright (c) 2018 Intel Corporation > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define DUMP_DEBUG 0 > + > +/* Aspeed PECI Registers */ > +#define AST_PECI_CTRL 0x00 Nit: we use ASPEED instead of AST in the upstream kernel to distingush from the aspeed sdk drivers. If you feel strongly about this then I won't insist you change. > +#define AST_PECI_TIMING 0x04 > +#define AST_PECI_CMD 0x08 > +#define AST_PECI_CMD_CTRL 0x0c > +#define AST_PECI_EXP_FCS 0x10 > +#define AST_PECI_CAP_FCS 0x14 > +#define AST_PECI_INT_CTRL 0x18 > +#define AST_PECI_INT_STS 0x1c > +#define AST_PECI_W_DATA0 0x20 > +#define AST_PECI_W_DATA1 0x24 > +#define AST_PECI_W_DATA2 0x28 > +#define AST_PECI_W_DATA3 0x2c > +#define AST_PECI_R_DATA0 0x30 > +#define AST_PECI_R_DATA1 0x34 > +#define AST_PECI_R_DATA2 0x38 > +#define AST_PECI_R_DATA3 0x3c > +#define AST_PECI_W_DATA4 0x40 > +#define AST_PECI_W_DATA5 0x44 > +#define AST_PECI_W_DATA6 0x48 > +#define AST_PECI_W_DATA7 0x4c > +#define AST_PECI_R_DATA4 0x50 > +#define AST_PECI_R_DATA5 0x54 > +#define AST_PECI_R_DATA6 0x58 > +#define AST_PECI_R_DATA7 0x5c > + > +/* AST_PECI_CTRL - 0x00 : Control Register */ > +#define PECI_CTRL_SAMPLING_MASK GENMASK(19, 16) > +#define PECI_CTRL_SAMPLING(x) (((x) << 16) & PECI_CTRL_SAMPLING_MASK) > +#define PECI_CTRL_SAMPLING_GET(x) (((x) & PECI_CTRL_SAMPLING_MASK) >> 16) > +#define PECI_CTRL_READ_MODE_MASKGENMASK(13, 12) > +#define PECI_CTRL_READ_MODE(x) (((x) << 12) & PECI_CTRL_READ_MODE_MASK) > +#define PECI_CTRL_READ_MODE_GET(x) (((x) & PECI_CTRL_READ_MODE_MASK) >> 12) > +#define PECI_CTRL_READ_MODE_COUNT BIT(12) > +#define PECI_CTRL_READ_MODE_DBG BIT(13) > +#define PECI_CTRL_CLK_SOURCE_MASK BIT(11) > +#define PECI_CTRL_CLK_SOURCE(x) (((x) << 11) & PECI_CTRL_CLK_SOURCE_MASK) > +#define PECI_CTRL_CLK_SOURCE_GET(x) (((x) & PECI_CTRL_CLK_SOURCE_MASK) >> 11) > +#define PECI_CTRL_CLK_DIV_MASK GENMASK(10, 8) > +#define PECI_CTRL_CLK_DIV(x)(((x) << 8) & PECI_CTRL_CLK_DIV_MASK) > +#define PECI_CTRL_CLK_DIV_GET(x)(((x) & PECI_CTRL_CLK_DIV_MASK) >> 8) > +#define PECI_CTRL_INVERT_OUTBIT(7) > +#define PECI_CTRL_INVERT_IN BIT(6) > +#define PECI_CTRL_BUS_CONTENT_ENBIT(5) > +#define PECI_CTRL_PECI_EN BIT(4) > +#define PECI_CTRL_PECI_CLK