* [PATCH] drivers: firmware: xilinx: Add ZynqMP firmware driver
From: Jolly Shah @ 2018-01-11 0:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180109121911.GA18770@mordor.localdomain>
Thanks Aishwarya for the review,
> -----Original Message-----
> From: Aishwarya Pant [mailto:aishpant at gmail.com]
> Sent: Tuesday, January 09, 2018 4:19 AM
> To: Jolly Shah <JOLLYS@xilinx.com>
> Cc: ard.biesheuvel at linaro.org; mingo at kernel.org;
> gregkh at linuxfoundation.org; matt at codeblueprint.co.uk;
> sudeep.holla at arm.com; hkallweit1 at gmail.com; keescook at chromium.org;
> dmitry.torokhov at gmail.com; michal.simek at xilinx.com; linux-arm-
> kernel at lists.infradead.org; linux-kernel at vger.kernel.org; Jolly Shah
> <JOLLYS@xilinx.com>; Rajan Vaja <RAJANV@xilinx.com>
> Subject: Re: [PATCH] drivers: firmware: xilinx: Add ZynqMP firmware driver
>
> On Mon, Jan 08, 2018 at 02:07:07PM -0800, Jolly Shah wrote:
> > This patch is adding communication layer with firmware.
> > Firmware driver provides an interface to firmware APIs.
> > Interface APIs can be used by any driver to communicate to
> > PMUFW(Platform Management Unit). All requests go through ATF.
> > Firmware-debug provides debugfs interface to all APIs.
> > Firmware-ggs provides read/write interface to
> > global storage registers.
> >
> > Signed-off-by: Jolly Shah <jollys@xilinx.com>
> > Signed-off-by: Rajan Vaja <rajanv@xilinx.com>
> > ---
> > .../firmware/xilinx/xlnx,zynqmp-firmware.txt | 16 +
> > arch/arm64/Kconfig.platforms | 1 +
> > drivers/firmware/Kconfig | 1 +
> > drivers/firmware/Makefile | 1 +
> > drivers/firmware/xilinx/Kconfig | 4 +
> > drivers/firmware/xilinx/Makefile | 4 +
> > drivers/firmware/xilinx/zynqmp/Kconfig | 23 +
> > drivers/firmware/xilinx/zynqmp/Makefile | 5 +
> > drivers/firmware/xilinx/zynqmp/firmware-debug.c | 540 +++++++++++
> > drivers/firmware/xilinx/zynqmp/firmware-ggs.c | 298 ++++++
> > drivers/firmware/xilinx/zynqmp/firmware.c | 1024
> ++++++++++++++++++++
> > .../linux/firmware/xilinx/zynqmp/firmware-debug.h | 32 +
> > include/linux/firmware/xilinx/zynqmp/firmware.h | 573 +++++++++++
> > 13 files changed, 2522 insertions(+)
> > create mode 100644
> Documentation/devicetree/bindings/firmware/xilinx/xlnx,zynqmp-firmware.txt
> > create mode 100644 drivers/firmware/xilinx/Kconfig
> > create mode 100644 drivers/firmware/xilinx/Makefile
> > create mode 100644 drivers/firmware/xilinx/zynqmp/Kconfig
> > create mode 100644 drivers/firmware/xilinx/zynqmp/Makefile
> > create mode 100644 drivers/firmware/xilinx/zynqmp/firmware-debug.c
> > create mode 100644 drivers/firmware/xilinx/zynqmp/firmware-ggs.c
> > create mode 100644 drivers/firmware/xilinx/zynqmp/firmware.c
> > create mode 100644 include/linux/firmware/xilinx/zynqmp/firmware-debug.h
> > create mode 100644 include/linux/firmware/xilinx/zynqmp/firmware.h
> >
> > +static ssize_t ggs_store(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf,
> > + size_t count,
> > + u32 reg)
> > +{
> > + if (!dev || !attr || !buf || !count || reg >= GSS_NUM_REGS)
> > + return -EINVAL;
> > +
> > + return write_register(buf, count, IOCTL_WRITE_GGS, reg);
> > +}
> > +
> > +/* GGS register show functions */
> > +#define GGS0_SHOW(N) \
> > + ssize_t ggs##N##_show(struct device *dev, \
> > + struct device_attribute *attr, \
> > + char *buf) \
> > + { \
> > + return ggs_show(dev, attr, buf, N); \
> > + }
> > +
> > +static GGS0_SHOW(0);
> > +static GGS0_SHOW(1);
> > +static GGS0_SHOW(2);
> > +static GGS0_SHOW(3);
> > +
> > +/* GGS register store function */
> > +#define GGS0_STORE(N) \
> > + ssize_t ggs##N##_store(struct device *dev, \
> > + struct device_attribute *attr, \
> > + const char *buf, \
> > + size_t count) \
> > + { \
> > + return ggs_store(dev, attr, buf, count, N); \
> > + }
> > +
> > +static GGS0_STORE(0);
> > +static GGS0_STORE(1);
> > +static GGS0_STORE(2);
> > +static GGS0_STORE(3);
> > +
> > +/* GGS register device attributes */
> > +static DEVICE_ATTR_RW(ggs0);
> > +static DEVICE_ATTR_RW(ggs1);
> > +static DEVICE_ATTR_RW(ggs2);
> > +static DEVICE_ATTR_RW(ggs3);
>
> Hi
>
> You added some files to the sysfs ABI. These interfaces should be documented in
> Documentation/ABI.
>
Sure. Will add it in next version.
Thanks,
Jolly Shah
^ permalink raw reply
* [PATCH] arm64: dts: add #cooling-cells to CPU nodes
From: Chunfeng Yun @ 2018-01-11 0:38 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180110210706.2331225-1-arnd@arndb.de>
On Wed, 2018-01-10 at 22:06 +0100, Arnd Bergmann wrote:
> dtc complains about the lack of #coolin-cells properties for the
> CPU nodes that are referred to as "cooling-device":
>
> arch/arm64/boot/dts/mediatek/mt8173-evb.dtb: Warning (cooling_device_property): Missing property '#cooling-cells' in node /cpus/cpu at 0 or bad phandle (referred from /thermal-zones/cpu_thermal/cooling-maps/map at 0:cooling-device[0])
> arch/arm64/boot/dts/mediatek/mt8173-evb.dtb: Warning (cooling_device_property): Missing property '#cooling-cells' in node /cpus/cpu at 100 or bad phandle (referred from /thermal-zones/cpu_thermal/cooling-maps/map at 1:cooling-device[0])
>
> Apparently this property must be '<2>' to match the binding.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> If this looks ok, I'd apply it directly to the fixes branch
> for 4.15, as the warning is one that was introduced in this
> release.
> ---
> arch/arm64/boot/dts/mediatek/mt8173.dtsi | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> index b477ad790071..9fbe4705ee88 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> @@ -155,6 +155,7 @@
> reg = <0x000>;
> enable-method = "psci";
> cpu-idle-states = <&CPU_SLEEP_0>;
> + #cooling-cells = <2>;
> clocks = <&infracfg CLK_INFRA_CA53SEL>,
> <&apmixedsys CLK_APMIXED_MAINPLL>;
> clock-names = "cpu", "intermediate";
> @@ -179,6 +180,7 @@
> reg = <0x100>;
> enable-method = "psci";
> cpu-idle-states = <&CPU_SLEEP_0>;
> + #cooling-cells = <2>;
> clocks = <&infracfg CLK_INFRA_CA57SEL>,
> <&apmixedsys CLK_APMIXED_MAINPLL>;
> clock-names = "cpu", "intermediate";
Tested-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
^ permalink raw reply
* [PATCH] usb: mtu3: fix ssusb_wakeup_set dummy
From: Chunfeng Yun @ 2018-01-11 0:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180110164530.1399750-1-arnd@arndb.de>
On Wed, 2018-01-10 at 17:45 +0100, Arnd Bergmann wrote:
> Changing from ssusb_wakeup_enable/disable to ssusb_wakeup_set was done
> in only one of two places in the kernel, the other one now causes a
> build failure:
>
> drivers/usb/mtu3/mtu3_plat.c: In function 'mtu3_suspend':
> drivers/usb/mtu3/mtu3_plat.c:462:2: error: implicit declaration of function 'ssusb_wakeup_set'; did you mean 'ssusb_wakeup_disable'? [-Werror=implicit-function-declaration]
>
> This adapts the dummy helpers the same way that the extern declarations
> were.
>
> Fixes: f0ede2c6282b ("usb: mtu3: supports remote wakeup for mt2712 with two SSUSB IPs")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> drivers/usb/mtu3/mtu3_dr.h | 7 +------
> 1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/usb/mtu3/mtu3_dr.h b/drivers/usb/mtu3/mtu3_dr.h
> index ae1598d76e02..50702fdcde28 100644
> --- a/drivers/usb/mtu3/mtu3_dr.h
> +++ b/drivers/usb/mtu3/mtu3_dr.h
> @@ -48,12 +48,7 @@ static inline int ssusb_host_disable(struct ssusb_mtk *ssusb, bool suspend)
> return 0;
> }
>
> -static inline int ssusb_wakeup_enable(struct ssusb_mtk *ssusb)
> -{
> - return 0;
> -}
> -
> -static inline void ssusb_wakeup_disable(struct ssusb_mtk *ssusb)
> +static inline void ssusb_wakeup_set(struct ssusb_mtk *ssusb, bool enable)
> {}
>
> #endif
Forgot to make it change,
Acked-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
Thanks a lot
^ permalink raw reply
* [PATCH] drivers: firmware: xilinx: Add ZynqMP firmware driver
From: Jolly Shah @ 2018-01-11 0:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <cbe4ccad-993b-76d7-bf7a-9ef0eb284459@arm.com>
Hi Sudeep,
Thanks for the review,
> -----Original Message-----
> From: Sudeep Holla [mailto:sudeep.holla at arm.com]
> Sent: Tuesday, January 09, 2018 6:40 AM
> To: Jolly Shah <JOLLYS@xilinx.com>; ard.biesheuvel at linaro.org;
> mingo at kernel.org; gregkh at linuxfoundation.org; matt at codeblueprint.co.uk;
> hkallweit1 at gmail.com; keescook at chromium.org;
> dmitry.torokhov at gmail.com; michal.simek at xilinx.com
> Cc: Sudeep Holla <sudeep.holla@arm.com>; linux-arm-
> kernel at lists.infradead.org; linux-kernel at vger.kernel.org; Jolly Shah
> <JOLLYS@xilinx.com>; Rajan Vaja <RAJANV@xilinx.com>
> Subject: Re: [PATCH] drivers: firmware: xilinx: Add ZynqMP firmware driver
>
>
>
> On 08/01/18 22:07, Jolly Shah wrote:
> > This patch is adding communication layer with firmware.
> > Firmware driver provides an interface to firmware APIs.
> > Interface APIs can be used by any driver to communicate to
> > PMUFW(Platform Management Unit). All requests go through ATF.
> > Firmware-debug provides debugfs interface to all APIs.
> > Firmware-ggs provides read/write interface to global storage
> > registers.
> >
> > Signed-off-by: Jolly Shah <jollys@xilinx.com>
> > Signed-off-by: Rajan Vaja <rajanv@xilinx.com>
> > ---
> > .../firmware/xilinx/xlnx,zynqmp-firmware.txt | 16 +
> > arch/arm64/Kconfig.platforms | 1 +
> > drivers/firmware/Kconfig | 1 +
> > drivers/firmware/Makefile | 1 +
> > drivers/firmware/xilinx/Kconfig | 4 +
> > drivers/firmware/xilinx/Makefile | 4 +
> > drivers/firmware/xilinx/zynqmp/Kconfig | 23 +
> > drivers/firmware/xilinx/zynqmp/Makefile | 5 +
> > drivers/firmware/xilinx/zynqmp/firmware-debug.c | 540 +++++++++++
> > drivers/firmware/xilinx/zynqmp/firmware-ggs.c | 298 ++++++
> > drivers/firmware/xilinx/zynqmp/firmware.c | 1024
> ++++++++++++++++++++
> > .../linux/firmware/xilinx/zynqmp/firmware-debug.h | 32 +
> > include/linux/firmware/xilinx/zynqmp/firmware.h | 573 +++++++++++
> > 13 files changed, 2522 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/firmware/xilinx/xlnx,zynqmp-firmware
> > .txt create mode 100644 drivers/firmware/xilinx/Kconfig create mode
> > 100644 drivers/firmware/xilinx/Makefile create mode 100644
> > drivers/firmware/xilinx/zynqmp/Kconfig
> > create mode 100644 drivers/firmware/xilinx/zynqmp/Makefile
> > create mode 100644 drivers/firmware/xilinx/zynqmp/firmware-debug.c
> > create mode 100644 drivers/firmware/xilinx/zynqmp/firmware-ggs.c
> > create mode 100644 drivers/firmware/xilinx/zynqmp/firmware.c
> > create mode 100644
> > include/linux/firmware/xilinx/zynqmp/firmware-debug.h
> > create mode 100644 include/linux/firmware/xilinx/zynqmp/firmware.h
> >
> > diff --git
> > a/Documentation/devicetree/bindings/firmware/xilinx/xlnx,zynqmp-firmwa
> > re.txt
> > b/Documentation/devicetree/bindings/firmware/xilinx/xlnx,zynqmp-firmwa
> > re.txt
> > new file mode 100644
> > index 0000000..ace111c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/firmware/xilinx/xlnx,zynqmp-fi
> > +++ rmware.txt
> > @@ -0,0 +1,16 @@
> > +Xilinx Zynq MPSoC Firmware Device Tree Bindings
> > +
> > +The zynqmp-firmware node describes the interface to platform firmware.
> > +
> > +Required properties:
> > + - compatible: Must contain: "xlnx,zynqmp-firmware"
> > + - method: The method of calling the PM-API firmware layer.
> > + Permitted values are:
> > + - "smc" : To be used in configurations without a hypervisor
> > + - "hvc" : To be used when hypervisor is present
> > +
>
> If we are having a mailbox using smc/hvc, then it can be made generic rather
> than xilinx specific. I can see other user of the same. As Jassi pointed out in
> some other thread, Andre has some generic implementation.
> Please see how it can be reused.
>
Andre's mailbox implementation in not in kernel yet. We can change later if it
satisfies our use case.
> Also please keep any bindings separate from the driver changes so that it can be
> reviewed separately.
Sure. Will do it in next version.
>
> > +Examples:
> > + firmware: firmware {
> > + compatible = "xlnx,zynqmp-firmware";
> > + method = "smc";
>
> Ideally this should point to mailbox if we move to using smc/hvc based mailbox.
>
> [...]
>
> > +
> > +config ZYNQMP_FIRMWARE_DEBUG
> > + bool "Enable Xilinx Zynq MPSoC firmware debug APIs"
> > + depends on ARCH_ZYNQMP && DEBUG_FS
>
> Do you need a separate Kconfig option, can't you just use DEBUG_FS ?
>
> [...]
>
As its zynqmp specific, we wanted to keep an option to enable/disable it separately.
It can be combined as ZYNQMP FIRMWARE+DEBUG_FS instead of a separate Kconfig.
> > +
> > + if (strncasecmp(pm_api_req, "REQUEST_SUSPEND", 15) == 0)
> > + pm_id = REQUEST_SUSPEND;
> > + else if (strncasecmp(pm_api_req, "SELF_SUSPEND", 12) == 0)
> > + pm_id = SELF_SUSPEND;
> > + else if (strncasecmp(pm_api_req, "FORCE_POWERDOWN", 15) == 0)
> > + pm_id = FORCE_POWERDOWN;
> > + else if (strncasecmp(pm_api_req, "ABORT_SUSPEND", 13) == 0)
> > + pm_id = ABORT_SUSPEND;
>
>
> Can this be changed to a loop with a static structure array containing {pm_id,
> pm_string, strlen(pm_string)} ?
Will fix it in next version
>
> Also I see hard-coded string length is wrong in some cases like IOCTL.
> Isn't it better to just use strlen("..") instead ?
>
Will fix it in next version
> I will stop here as the patch can be easily split and several features can be added
> incrementally making the base patch simpler and shorter.
Sure. Will break it into a few patches in next version
Thanks,
Jolly Shah
>
> --
> Regards,
> Sudeep
^ permalink raw reply
* [PATCH 00/10] perf tools: Add support for CoreSight trace decoding
From: Kim Phillips @ 2018-01-11 0:08 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1513356299-26274-1-git-send-email-mathieu.poirier@linaro.org>
On Fri, 15 Dec 2017 09:44:49 -0700
Mathieu Poirier <mathieu.poirier@linaro.org> wrote:
> This patchset adds support for per-thread CoreSight trace decoding from the
> "perf report" interface. It is largely modelled on what has been done for
> intelPT traces and currently targets the ETMv4 architecture. Support for
> cpu-wide scenarios and ETMv3/PTMv1.1 will follow shortly.
>
> The trace decoding support is done using the Open CoreSight Decoding
> Library (openCSD), a stand alone open source project available here [1].
> Integration of the openCSD library with the perf tools follow what has
> been done for other support libraries. If the library has been installed
> on a system the build scripts will include support for CoreSight trace
> decoding:
>
> ... zlib: [ on ]
> ... lzma: [ OFF ]
> ... get_cpuid: [ on ]
> ... bpf: [ on ]
> ... libopencsd: [ on ] <------
>
> Instructions on how to build and install the openCSD library are provided
> in the HOWTO.md of the project repository.
Usually when a perf builder sees something they need "on," they - or,
at least I - start querying the host's package manager for something
that provides it (e.g., apt search/install libopencsd), but since no
distro provides libopencsd, this is bad because it misleads the user.
> We elected to keep the decoder
> library independent of the kernel tree as it is also used outside of the
> perf toolset and various non-linux projects.
Where? Not that that won't mean it can't be included in the kernel
source tree anyway: Doing so would enable support without burdening
the linux perf user with adding any external custom library
dependencies.
Keeping the library external will also inevitably introduce more
source level synchronization problems because the perf sources being
built may not be compatible with their version of the library, whether
due to new features like new trace hardware support, or API changes.
As Mark Brown (cc'd) mentioned on the Coresight mailing list, this may
be able to be done the same way the dtc is incorporated into the
kernel, where only its relevant sources are included and updated as
needed: see linux/scripts/dtc/update-dtc-source.sh.
Hopefully the upstream maintainers (acme, etc.) can chime in with
their opinion on how to include the library, if at all? It's
essentially the same type of code as the Intel-PT decoder code,
currently found under tools/perf/util/intel-pt-decoder, except we're
trying to keep some sense of connection to the parent project ("Intel
(R) Processor Trace Decoder Library" in Intel's case [1]) alive.
Thanks,
Kim
[1] https://github.com/01org/processor-trace
^ permalink raw reply
* [nomadik:gemini-ethernet 18/18] drivers/net/ethernet/cortina/gemini.c:1410:63: warning: format '%x' expects argument of type 'unsigned int', but argument 4 has type 'long unsigned int'
From: kbuild test robot @ 2018-01-11 0:03 UTC (permalink / raw)
To: linux-arm-kernel
tree: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-nomadik.git gemini-ethernet
head: de0cb586c9e534c926cd7c74c92dc3d4104ea569
commit: de0cb586c9e534c926cd7c74c92dc3d4104ea569 [18/18] net: ethernet: Add a driver for Gemini gigabit ethernet
config: i386-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.2.0-12) 7.2.1 20171025
reproduce:
git checkout de0cb586c9e534c926cd7c74c92dc3d4104ea569
# save the attached .config to linux build tree
make ARCH=i386
All warnings (new ones prefixed by >>):
drivers/net/ethernet/cortina/gemini.c: In function 'gmac_rx':
>> drivers/net/ethernet/cortina/gemini.c:1410:63: warning: format '%x' expects argument of type 'unsigned int', but argument 4 has type 'long unsigned int' [-Wformat=]
dev_err(geth->dev, "could not find mapping %08x (page = %08x, page off = %08x)\n",
~~~^
%08lx
In file included from drivers/net/ethernet/cortina/gemini.c:16:0:
drivers/net/ethernet/cortina/gemini.c: At top level:
include/linux/module.h:129:42: error: redefinition of '__inittest'
static inline initcall_t __maybe_unused __inittest(void) \
^
include/linux/device.h:1513:1: note: in expansion of macro 'module_init'
module_init(__driver##_init); \
^~~~~~~~~~~
include/linux/platform_device.h:228:2: note: in expansion of macro 'module_driver'
module_driver(__platform_driver, platform_driver_register, \
^~~~~~~~~~~~~
drivers/net/ethernet/cortina/gemini.c:2567:1: note: in expansion of macro 'module_platform_driver'
module_platform_driver(gemini_ethernet_driver);
^~~~~~~~~~~~~~~~~~~~~~
include/linux/module.h:129:42: note: previous definition of '__inittest' was here
static inline initcall_t __maybe_unused __inittest(void) \
^
include/linux/device.h:1513:1: note: in expansion of macro 'module_init'
module_init(__driver##_init); \
^~~~~~~~~~~
include/linux/platform_device.h:228:2: note: in expansion of macro 'module_driver'
module_driver(__platform_driver, platform_driver_register, \
^~~~~~~~~~~~~
drivers/net/ethernet/cortina/gemini.c:2495:1: note: in expansion of macro 'module_platform_driver'
module_platform_driver(gemini_ethernet_port_driver);
^~~~~~~~~~~~~~~~~~~~~~
include/linux/module.h:131:6: error: redefinition of 'init_module'
int init_module(void) __attribute__((alias(#initfn)));
^
include/linux/device.h:1513:1: note: in expansion of macro 'module_init'
module_init(__driver##_init); \
^~~~~~~~~~~
include/linux/platform_device.h:228:2: note: in expansion of macro 'module_driver'
module_driver(__platform_driver, platform_driver_register, \
^~~~~~~~~~~~~
drivers/net/ethernet/cortina/gemini.c:2567:1: note: in expansion of macro 'module_platform_driver'
module_platform_driver(gemini_ethernet_driver);
^~~~~~~~~~~~~~~~~~~~~~
include/linux/module.h:131:6: note: previous definition of 'init_module' was here
int init_module(void) __attribute__((alias(#initfn)));
^
include/linux/device.h:1513:1: note: in expansion of macro 'module_init'
module_init(__driver##_init); \
^~~~~~~~~~~
include/linux/platform_device.h:228:2: note: in expansion of macro 'module_driver'
module_driver(__platform_driver, platform_driver_register, \
^~~~~~~~~~~~~
drivers/net/ethernet/cortina/gemini.c:2495:1: note: in expansion of macro 'module_platform_driver'
module_platform_driver(gemini_ethernet_port_driver);
^~~~~~~~~~~~~~~~~~~~~~
include/linux/module.h:135:42: error: redefinition of '__exittest'
static inline exitcall_t __maybe_unused __exittest(void) \
^
include/linux/device.h:1518:1: note: in expansion of macro 'module_exit'
module_exit(__driver##_exit);
^~~~~~~~~~~
include/linux/platform_device.h:228:2: note: in expansion of macro 'module_driver'
module_driver(__platform_driver, platform_driver_register, \
^~~~~~~~~~~~~
drivers/net/ethernet/cortina/gemini.c:2567:1: note: in expansion of macro 'module_platform_driver'
module_platform_driver(gemini_ethernet_driver);
^~~~~~~~~~~~~~~~~~~~~~
include/linux/module.h:135:42: note: previous definition of '__exittest' was here
static inline exitcall_t __maybe_unused __exittest(void) \
^
include/linux/device.h:1518:1: note: in expansion of macro 'module_exit'
module_exit(__driver##_exit);
^~~~~~~~~~~
include/linux/platform_device.h:228:2: note: in expansion of macro 'module_driver'
module_driver(__platform_driver, platform_driver_register, \
^~~~~~~~~~~~~
drivers/net/ethernet/cortina/gemini.c:2495:1: note: in expansion of macro 'module_platform_driver'
module_platform_driver(gemini_ethernet_port_driver);
^~~~~~~~~~~~~~~~~~~~~~
include/linux/module.h:137:7: error: redefinition of 'cleanup_module'
void cleanup_module(void) __attribute__((alias(#exitfn)));
^
include/linux/device.h:1518:1: note: in expansion of macro 'module_exit'
module_exit(__driver##_exit);
^~~~~~~~~~~
include/linux/platform_device.h:228:2: note: in expansion of macro 'module_driver'
module_driver(__platform_driver, platform_driver_register, \
^~~~~~~~~~~~~
drivers/net/ethernet/cortina/gemini.c:2567:1: note: in expansion of macro 'module_platform_driver'
module_platform_driver(gemini_ethernet_driver);
^~~~~~~~~~~~~~~~~~~~~~
include/linux/module.h:137:7: note: previous definition of 'cleanup_module' was here
void cleanup_module(void) __attribute__((alias(#exitfn)));
^
include/linux/device.h:1518:1: note: in expansion of macro 'module_exit'
module_exit(__driver##_exit);
^~~~~~~~~~~
include/linux/platform_device.h:228:2: note: in expansion of macro 'module_driver'
module_driver(__platform_driver, platform_driver_register, \
^~~~~~~~~~~~~
drivers/net/ethernet/cortina/gemini.c:2495:1: note: in expansion of macro 'module_platform_driver'
module_platform_driver(gemini_ethernet_port_driver);
vim +1410 drivers/net/ethernet/cortina/gemini.c
1359
1360 static unsigned int gmac_rx(struct net_device *netdev, unsigned int budget)
1361 {
1362 struct gemini_ethernet_port *port = netdev_priv(netdev);
1363 unsigned short m = (1 << port->rxq_order) - 1;
1364 struct gemini_ethernet *geth = port->geth;
1365 void __iomem *ptr_reg = port->rxq_rwptr;
1366 unsigned int frame_len, frag_len;
1367 struct gmac_rxdesc *rx = NULL;
1368 struct gmac_queue_page *gpage;
1369 static struct sk_buff *skb;
1370 union gmac_rxdesc_0 word0;
1371 union gmac_rxdesc_1 word1;
1372 union gmac_rxdesc_3 word3;
1373 struct page *page = NULL;
1374 unsigned int page_offs;
1375 unsigned short r, w;
1376 union dma_rwptr rw;
1377 dma_addr_t mapping;
1378 int frag_nr = 0;
1379
1380 rw.bits32 = readl(ptr_reg);
1381 /* Reset interrupt as all packages until here are taken into account */
1382 writel(DEFAULT_Q0_INT_BIT << netdev->dev_id,
1383 geth->base + GLOBAL_INTERRUPT_STATUS_1_REG);
1384 r = rw.bits.rptr;
1385 w = rw.bits.wptr;
1386
1387 while (budget && w != r) {
1388 rx = port->rxq_ring + r;
1389 word0 = rx->word0;
1390 word1 = rx->word1;
1391 mapping = rx->word2.buf_adr;
1392 word3 = rx->word3;
1393
1394 r++;
1395 r &= m;
1396
1397 frag_len = word0.bits.buffer_size;
1398 frame_len = word1.bits.byte_count;
1399 page_offs = mapping & ~PAGE_MASK;
1400
1401 if (!mapping) {
1402 netdev_err(netdev,
1403 "rxq[%u]: HW BUG: zero DMA desc\n", r);
1404 goto err_drop;
1405 }
1406
1407 /* Freeq pointers are one page off */
1408 gpage = gmac_get_queue_page(geth, port, mapping + PAGE_SIZE);
1409 if (!gpage) {
> 1410 dev_err(geth->dev, "could not find mapping %08x (page = %08x, page off = %08x)\n",
1411 mapping, mapping & PAGE_MASK, page_offs);
1412 continue;
1413 }
1414 page = gpage->page;
1415
1416 if (word3.bits32 & SOF_BIT) {
1417 if (skb) {
1418 napi_free_frags(&port->napi);
1419 port->stats.rx_dropped++;
1420 }
1421
1422 skb = gmac_skb_if_good_frame(port, word0, frame_len);
1423 if (!skb)
1424 goto err_drop;
1425
1426 page_offs += NET_IP_ALIGN;
1427 frag_len -= NET_IP_ALIGN;
1428 frag_nr = 0;
1429
1430 } else if (!skb) {
1431 put_page(page);
1432 continue;
1433 }
1434
1435 if (word3.bits32 & EOF_BIT)
1436 frag_len = frame_len - skb->len;
1437
1438 /* append page frag to skb */
1439 if (frag_nr == MAX_SKB_FRAGS)
1440 goto err_drop;
1441
1442 if (frag_len == 0)
1443 netdev_err(netdev, "Received fragment with len = 0\n");
1444
1445 skb_fill_page_desc(skb, frag_nr, page, page_offs, frag_len);
1446 skb->len += frag_len;
1447 skb->data_len += frag_len;
1448 skb->truesize += frag_len;
1449 frag_nr++;
1450
1451 if (word3.bits32 & EOF_BIT) {
1452 napi_gro_frags(&port->napi);
1453 skb = NULL;
1454 --budget;
1455 }
1456 continue;
1457
1458 err_drop:
1459 if (skb) {
1460 napi_free_frags(&port->napi);
1461 skb = NULL;
1462 }
1463
1464 if (mapping)
1465 put_page(page);
1466
1467 port->stats.rx_dropped++;
1468 }
1469
1470 writew(r, ptr_reg);
1471 return budget;
1472 }
1473
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 62432 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180111/cd79a02e/attachment-0001.gz>
^ permalink raw reply
* [GIT PULL] updates to soc/fsl drivers for v4.16
From: Li Yang @ 2018-01-10 23:56 UTC (permalink / raw)
To: linux-arm-kernel
Hi arm-soc maintainer,
Please merge the following tag to get updates to the soc/fsl/guts driver
for support of additional SoCs and more error path handling.
Thanks,
Leo
The following changes since commit b2cd1df66037e7c4697c7e40496bf7e4a5e16a2d:
Linux 4.15-rc7 (2018-01-07 14:22:41 -0800)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/leo/linux.git tags/soc-fsl-for-4.16
for you to fetch changes up to 00ce0a2304014c73c2b7915215c7b3c73e2a25aa:
soc: fsl: guts: Add a NULL check for devm_kasprintf() (2018-01-10 16:54:26 -0600)
----------------------------------------------------------------
FSL/NXP SoC drivers updates for 4.16
This adds new SoC support and more error path handling to the guts
driver.
----------------------------------------------------------------
Fabio Estevam (1):
soc: fsl: guts: Add a NULL check for devm_kasprintf()
yangbo lu (1):
soc: fsl: guts: add support for ls1012a and ls1046a
drivers/soc/fsl/guts.c | 8 ++++++++
1 file changed, 8 insertions(+)
^ permalink raw reply
* [PATCH 3/9] soc: samsung: pmu: Add the PMU data of exynos5433 to support low-power state
From: Chanwoo Choi @ 2018-01-10 23:51 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <0adffb4c-019f-a637-881c-d919c67cdbcf@arm.com>
On 2018? 01? 10? 19:53, Sudeep Holla wrote:
>
>
> On 10/01/18 01:46, Chanwoo Choi wrote:
>> On 2018? 01? 09? 23:11, Sudeep Holla wrote:
>>>
>>>
>>> On 09/01/18 07:59, Chanwoo Choi wrote:
>>>> This patch adds the PMU (Power Management Unit) data of exynos5433 SoC
>>>> in order to support the various power modes. Each power mode has
>>>> the different value for reducing the power-consumption.
>>>>
>>>> Signed-off-by: Jonghwa Lee <jonghwa3.lee@samsung.com>
>>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>> ---
>>>> arch/arm/mach-exynos/common.h | 2 -
>>>> drivers/soc/samsung/Makefile | 3 +-
>>>> drivers/soc/samsung/exynos-pmu.c | 1 +
>>>> drivers/soc/samsung/exynos-pmu.h | 2 +
>>>> drivers/soc/samsung/exynos5433-pmu.c | 286 ++++++++++++++++++++++++++++
>>>> include/linux/soc/samsung/exynos-regs-pmu.h | 148 ++++++++++++++
>>>> 6 files changed, 439 insertions(+), 3 deletions(-)
>>>> create mode 100644 drivers/soc/samsung/exynos5433-pmu.c
>>>>
>>>
>>>> diff --git a/drivers/soc/samsung/exynos5433-pmu.c b/drivers/soc/samsung/exynos5433-pmu.c
>>>> new file mode 100644
>>>> index 000000000000..2571e61522f0
>>>> --- /dev/null
>>>> +++ b/drivers/soc/samsung/exynos5433-pmu.c
>>>> @@ -0,0 +1,286 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +//
>>>> +// Copyright (c) 2018 Samsung Electronics Co., Ltd.
>>>> +// Copyright (c) Jonghwa Lee <jonghwa3.lee@samsung.com>
>>>> +// Copyright (c) Chanwoo Choi <cw00.choi@samsung.com>
>>>> +//
>>>> +// EXYNOS5433 - CPU PMU (Power Management Unit) support
>>>> +
>>>> +#include <linux/soc/samsung/exynos-regs-pmu.h>
>>>> +#include <linux/soc/samsung/exynos-pmu.h>
>>>> +
>>>> +#include "exynos-pmu.h"
>>>> +
>>>> +static struct exynos_pmu_conf exynos5433_pmu_config[] = {
>>>> + /* { .offset = address, .val = { AFTR, LPA, SLEEP } } */
>>>> + { EXYNOS5433_ATLAS_CPU0_SYS_PWR_REG, { 0x0, 0x0, 0x8 } },
>>>> + { EXYNOS5433_DIS_IRQ_ATLAS_CPU0_CENTRAL_SYS_PWR_REG, { 0x0, 0x0, 0x0 } },
>>>> + { EXYNOS5433_ATLAS_CPU1_SYS_PWR_REG, { 0x0, 0x0, 0x8 } },
>>>> + { EXYNOS5433_DIS_IRQ_ATLAS_CPU1_CENTRAL_SYS_PWR_REG, { 0x0, 0x0, 0x0 } },
>>>> + { EXYNOS5433_ATLAS_CPU2_SYS_PWR_REG, { 0x0, 0x0, 0x8 } },
>>>> + { EXYNOS5433_DIS_IRQ_ATLAS_CPU2_CENTRAL_SYS_PWR_REG, { 0x0, 0x0, 0x0 } },
>>>> + { EXYNOS5433_ATLAS_CPU3_SYS_PWR_REG, { 0x0, 0x0, 0x8 } },
>>>> + { EXYNOS5433_DIS_IRQ_ATLAS_CPU3_CENTRAL_SYS_PWR_REG, { 0x0, 0x0, 0x0 } },
>>>> + { EXYNOS5433_APOLLO_CPU0_SYS_PWR_REG, { 0x0, 0x0, 0x8 } },
>>>> + { EXYNOS5433_DIS_IRQ_APOLLO_CPU0_CENTRAL_SYS_PWR_REG, { 0x0, 0x0, 0x0 } },
>>>> + { EXYNOS5433_APOLLO_CPU1_SYS_PWR_REG, { 0x0, 0x0, 0x8 } },
>>>> + { EXYNOS5433_DIS_IRQ_APOLLO_CPU1_CENTRAL_SYS_PWR_REG, { 0x0, 0x0, 0x0 } },
>>>> + { EXYNOS5433_APOLLO_CPU2_SYS_PWR_REG, { 0x0, 0x0, 0x8 } },
>>>> + { EXYNOS5433_DIS_IRQ_APOLLO_CPU2_CENTRAL_SYS_PWR_REG, { 0x0, 0x0, 0x0 } },
>>>> + { EXYNOS5433_APOLLO_CPU3_SYS_PWR_REG, { 0x0, 0x0, 0x8 } },
>>>> + { EXYNOS5433_DIS_IRQ_APOLLO_CPU3_CENTRAL_SYS_PWR_REG, { 0x0, 0x0, 0x0 } },
>>>> + { EXYNOS5433_ATLAS_NONCPU_SYS_PWR_REG, { 0x0, 0x0, 0x8 } },
>>>> + { EXYNOS5433_APOLLO_NONCPU_SYS_PWR_REG, { 0x0, 0x0, 0x8 } },
>>>
>>>
>>> 1. First of all why do you need any of these CPU related PMU config
>>> registers in kernel ? From the information I gathered this is ARM64
>>> SoC using PSCI. These are needed just in PSCI implementation and not
>>> in kernel. So can you elaborate on why there are present here ?
>>
>> The 32bit Exynos used the 'smc' call to enter the suspend mode
>> and need to handle the PMU registers.
>>
>
> OK
>
>> Even if PSCI replaces the 'smc' call on the Exynos5433,
>> the Exynos5433's document requires the handling of PMU config
>> related to CPU for the suspend mode.
>>
>
> OK but does it state it needs to be in kernel ?(i.e. in EL1 NS)
>
>> IMHO, If the secure OS implemented the all something related to CPU,
>> it might be unnecessary to handle the PMU registers. I think that
>> it depend on how to design the SoC by H/W Architect. This is just my opinion.
>>
>
> Even if it can be accessed in kernel, better to keep all these in PSCI
> implementation that does the actual power state management. Mixing them
> in both kernel and PSCI implementation is just a mess TBH and not easy
> to maintain the platform if some issues pop up.
>
>>>
>>> 2. Are there any public documents that these names map to ?
>>
>> There is no public document. It is confidential.
>>
>
> I guessed so and hence raised to remove it.
>
>>> If there is none, please replace these codenames(ATLAS, APOLLO) with
>>> appropriately.
>>
>> In the Exynos5433, 'apollo' indicates the LITTLE cores (cpu0-3, cortex-a53)
>> and 'atlas' indicates the big cores (cpu4-7, cortex-a57)
>>
>> Exynos5433 already used the 'apollo' and 'atlas' on clk-exynos5433.c driver
>> and thermal device-tree node. It is better to use the original register name
>> in the document in order to reduce the confusion of the change of register name
>> even if document is not public.
>>
>> Also, exynos7 used the 'atlas' word for big cores.
>>
>
> I disagree. If you don't have any public documents, then better use
> names other developers are used to and not some code names which
> otherwise is not seen in any other part of the code base. Grepping
> just shows Exynos using these code names which are generally restricted
I mentioned, these words were already used on other device drivers.
Exynos5433 used them to indicate the big.LITTLE cores consistently.
They are an alias for big.LITTLE. Latest Exynos SoC must use these words continuously.
I think that each vendor SoC could define the alias or expression for their SoC.
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* [PATCH linux dev-4.10 6/6] drivers/hwmon: Add a driver for a generic PECI hwmon
From: Jae Hyun Yoo @ 2018-01-10 23:45 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAK8P3a31+9HGQ=fQby5Zi1RZ671hKraZP_2ggLTMX3qvOiTx0w@mail.gmail.com>
On 1/10/2018 4:29 AM, Arnd Bergmann wrote:
> On Tue, Jan 9, 2018 at 11:31 PM, Jae Hyun Yoo
> <jae.hyun.yoo@linux.intel.com> wrote:
>> This commit adds driver implementation for a generic PECI hwmon.
>>
>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>
>> +static int xfer_peci_msg(int cmd, void *pmsg)
>> +{
>> + int rc;
>> +
>> + mutex_lock(&peci_hwmon_lock);
>> + rc = peci_ioctl(NULL, cmd, (unsigned long)pmsg);
>> + mutex_unlock(&peci_hwmon_lock);
>> +
>> + return rc;
>> +}
>
> I said earlier that peci_ioctl() looked unused, that was obviously
> wrong, but what you have here
> is not a proper way to abstract a bus.
>
> Maybe this can be done more like an i2c bus: make the peci controller
> a bus device
> and register all known target/index pairs as devices with the peci bus
> type, and have
> them probed from DT. The driver can then bind to each of those individually.
> Not sure if that is getting to granular at that point, I'd have to
> understand better
> how it is expected to get used, and what the variances are between
> implementations.
>
> Arnd
>
Thanks for sharing your opinion. In fact, this was also suggested by
openbmc community so I should consider of redesigning it. I'm currently
thinking about adding a new PECI device class as an abstract layer and
any BMC chipset specific driver could be attached to the PECI class
driver. Then, each CPU client could be registered as an individual
device as you suggested. Will consider your suggestion.
Thanks a lot!
Jae
^ permalink raw reply
* [PATCH linux dev-4.10 4/6] Documentation: dt-bindings: Add a generic PECI hwmon
From: Jae Hyun Yoo @ 2018-01-10 23:20 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAK8P3a1qiD1sWj0oQ=WbvpEDhtcYbzCxjg7t1iPs45WamfUVgw@mail.gmail.com>
On 1/10/2018 4:20 AM, Arnd Bergmann wrote:
> On Tue, Jan 9, 2018 at 11:31 PM, Jae Hyun Yoo
> <jae.hyun.yoo@linux.intel.com> wrote:
>> This commit add a dt-bindings document for a generic PECI hwmon
>> driver.
>>
>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>> ---
>> .../devicetree/bindings/hwmon/peci-hwmon.txt | 33 ++++++++++++++++++++++
>> 1 file changed, 33 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/hwmon/peci-hwmon.txt
>>
>> diff --git a/Documentation/devicetree/bindings/hwmon/peci-hwmon.txt b/Documentation/devicetree/bindings/hwmon/peci-hwmon.txt
>> new file mode 100644
>> index 0000000..20b86f5
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/hwmon/peci-hwmon.txt
>> @@ -0,0 +1,33 @@
>> +* Generic PECI (Platform Environment Control Interface) hwmon driver.
>> +
>> +Dependency:
>> +- This driver uses a PECI misc driver as a controller interface so one of PECI
>> + misc drivers which provides compatible ioctls has to be enabled.
>
> The binding should only explain how we describe the hardware in an
> operating-system
> independent way, but not talk about how an OS is supposed to implement those
> drivers.
>
> Having multiple drivers each provide an exported function is not possible
> in Linux: it immediately breaks building an 'allyesconfig' kernel, and prevents
> you from running the same kernel across multiple implementations, so that
> has to be redesigned anyway.
>
Agreed, I'll consider redesigning of it.
>> +Required properties:
>> +- compatible: "peci-hwmon"
>> +- cpu-id: Should contain CPU socket ID
>> + - 0 ~ 7
>> +
>> +Optional properties:
>> +- show-core: If this protperty is defined, core tmeperature attrubites will be
>
> s/protperty/property/
> s/tmeperature/temperature/
> s/attrubites/attributes/
>
Oops! I made this many typos in this single line. Thanks for your
pointing it out. Will fix these.
>> + enumerated.
>> +- dimm-nums: Should contain the number of DIMM slots that attached to each CPU
>> + which is indicated by cpu-id.
>> + 0 ~ 16 (default: 16)
>> + In case of 0, DIMM temperature attrubites will not be enumerated.
>
> Is this only an initial list that you expect to be extended in the
> future, or is this
> a complete list of sensors that can ever be connected to PECI?
>
> Should this be PECI version specific?
>
> Arnd
>
The maximum supportable number of dimm slots is not PECI version
specific but depends on CPU architecture. Currently IA supports up to 16
slot as the maximum but it could vary in the future architecture.
Thanks,
Jae
^ permalink raw reply
* [nomadik:gemini-ethernet 18/18] include/linux/module.h:129:42: error: redefinition of '__inittest'
From: kbuild test robot @ 2018-01-10 23:18 UTC (permalink / raw)
To: linux-arm-kernel
tree: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-nomadik.git gemini-ethernet
head: de0cb586c9e534c926cd7c74c92dc3d4104ea569
commit: de0cb586c9e534c926cd7c74c92dc3d4104ea569 [18/18] net: ethernet: Add a driver for Gemini gigabit ethernet
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout de0cb586c9e534c926cd7c74c92dc3d4104ea569
# save the attached .config to linux build tree
make.cross ARCH=ia64
All error/warnings (new ones prefixed by >>):
In file included from include/linux/printk.h:329:0,
from include/linux/kernel.h:14,
from include/linux/list.h:9,
from include/linux/module.h:9,
from drivers/net//ethernet/cortina/gemini.c:16:
drivers/net//ethernet/cortina/gemini.c: In function 'geth_freeq_alloc_map_page':
>> drivers/net//ethernet/cortina/gemini.c:821:21: warning: format '%x' expects argument of type 'unsigned int', but argument 5 has type 'dma_addr_t {aka long long unsigned int}' [-Wformat=]
dev_dbg(geth->dev, "page %d, DMA addr: %08x, page %p\n",
^
include/linux/dynamic_debug.h:135:39: note: in definition of macro 'dynamic_dev_dbg'
__dynamic_dev_dbg(&descriptor, dev, fmt, \
^~~
>> drivers/net//ethernet/cortina/gemini.c:821:2: note: in expansion of macro 'dev_dbg'
dev_dbg(geth->dev, "page %d, DMA addr: %08x, page %p\n",
^~~~~~~
drivers/net//ethernet/cortina/gemini.c: In function 'gmac_rx':
drivers/net//ethernet/cortina/gemini.c:1410:50: warning: format '%x' expects argument of type 'unsigned int', but argument 3 has type 'dma_addr_t {aka long long unsigned int}' [-Wformat=]
dev_err(geth->dev, "could not find mapping %08x (page = %08x, page off = %08x)\n",
~~~^
%08llx
>> drivers/net//ethernet/cortina/gemini.c:1410:63: warning: format '%x' expects argument of type 'unsigned int', but argument 4 has type 'long long unsigned int' [-Wformat=]
dev_err(geth->dev, "could not find mapping %08x (page = %08x, page off = %08x)\n",
~~~^
%08llx
drivers/net//ethernet/cortina/gemini.c: In function 'gemini_ethernet_port_probe':
>> drivers/net//ethernet/cortina/gemini.c:2456:28: warning: format '%x' expects argument of type 'unsigned int', but argument 4 has type 'resource_size_t {aka long long unsigned int}' [-Wformat=]
"irq %d, DMA @ 0x%08x, GMAC @ 0x%08x\n",
~~~^
%08llx
port->irq, dmares->start,
~~~~~~~~~~~~~
drivers/net//ethernet/cortina/gemini.c:2456:43: warning: format '%x' expects argument of type 'unsigned int', but argument 5 has type 'resource_size_t {aka long long unsigned int}' [-Wformat=]
"irq %d, DMA @ 0x%08x, GMAC @ 0x%08x\n",
~~~^
%08llx
drivers/net//ethernet/cortina/gemini.c:2458:8:
gmacres->start);
~~~~~~~~~~~~~~
In file included from drivers/net//ethernet/cortina/gemini.c:16:0:
drivers/net//ethernet/cortina/gemini.c: At top level:
>> include/linux/module.h:129:42: error: redefinition of '__inittest'
static inline initcall_t __maybe_unused __inittest(void) \
^
include/linux/device.h:1513:1: note: in expansion of macro 'module_init'
module_init(__driver##_init); \
^~~~~~~~~~~
>> include/linux/platform_device.h:228:2: note: in expansion of macro 'module_driver'
module_driver(__platform_driver, platform_driver_register, \
^~~~~~~~~~~~~
>> drivers/net//ethernet/cortina/gemini.c:2567:1: note: in expansion of macro 'module_platform_driver'
module_platform_driver(gemini_ethernet_driver);
^~~~~~~~~~~~~~~~~~~~~~
include/linux/module.h:129:42: note: previous definition of '__inittest' was here
static inline initcall_t __maybe_unused __inittest(void) \
^
include/linux/device.h:1513:1: note: in expansion of macro 'module_init'
module_init(__driver##_init); \
^~~~~~~~~~~
>> include/linux/platform_device.h:228:2: note: in expansion of macro 'module_driver'
module_driver(__platform_driver, platform_driver_register, \
^~~~~~~~~~~~~
drivers/net//ethernet/cortina/gemini.c:2495:1: note: in expansion of macro 'module_platform_driver'
module_platform_driver(gemini_ethernet_port_driver);
^~~~~~~~~~~~~~~~~~~~~~
>> include/linux/module.h:131:6: error: redefinition of 'init_module'
int init_module(void) __attribute__((alias(#initfn)));
^
include/linux/device.h:1513:1: note: in expansion of macro 'module_init'
module_init(__driver##_init); \
^~~~~~~~~~~
>> include/linux/platform_device.h:228:2: note: in expansion of macro 'module_driver'
module_driver(__platform_driver, platform_driver_register, \
^~~~~~~~~~~~~
>> drivers/net//ethernet/cortina/gemini.c:2567:1: note: in expansion of macro 'module_platform_driver'
module_platform_driver(gemini_ethernet_driver);
^~~~~~~~~~~~~~~~~~~~~~
include/linux/module.h:131:6: note: previous definition of 'init_module' was here
int init_module(void) __attribute__((alias(#initfn)));
^
include/linux/device.h:1513:1: note: in expansion of macro 'module_init'
module_init(__driver##_init); \
^~~~~~~~~~~
>> include/linux/platform_device.h:228:2: note: in expansion of macro 'module_driver'
module_driver(__platform_driver, platform_driver_register, \
^~~~~~~~~~~~~
drivers/net//ethernet/cortina/gemini.c:2495:1: note: in expansion of macro 'module_platform_driver'
module_platform_driver(gemini_ethernet_port_driver);
^~~~~~~~~~~~~~~~~~~~~~
>> include/linux/module.h:135:42: error: redefinition of '__exittest'
static inline exitcall_t __maybe_unused __exittest(void) \
^
include/linux/device.h:1518:1: note: in expansion of macro 'module_exit'
module_exit(__driver##_exit);
^~~~~~~~~~~
>> include/linux/platform_device.h:228:2: note: in expansion of macro 'module_driver'
module_driver(__platform_driver, platform_driver_register, \
^~~~~~~~~~~~~
>> drivers/net//ethernet/cortina/gemini.c:2567:1: note: in expansion of macro 'module_platform_driver'
module_platform_driver(gemini_ethernet_driver);
^~~~~~~~~~~~~~~~~~~~~~
include/linux/module.h:135:42: note: previous definition of '__exittest' was here
static inline exitcall_t __maybe_unused __exittest(void) \
^
include/linux/device.h:1518:1: note: in expansion of macro 'module_exit'
module_exit(__driver##_exit);
^~~~~~~~~~~
>> include/linux/platform_device.h:228:2: note: in expansion of macro 'module_driver'
module_driver(__platform_driver, platform_driver_register, \
^~~~~~~~~~~~~
drivers/net//ethernet/cortina/gemini.c:2495:1: note: in expansion of macro 'module_platform_driver'
module_platform_driver(gemini_ethernet_port_driver);
^~~~~~~~~~~~~~~~~~~~~~
>> include/linux/module.h:137:7: error: redefinition of 'cleanup_module'
void cleanup_module(void) __attribute__((alias(#exitfn)));
^
include/linux/device.h:1518:1: note: in expansion of macro 'module_exit'
module_exit(__driver##_exit);
^~~~~~~~~~~
>> include/linux/platform_device.h:228:2: note: in expansion of macro 'module_driver'
module_driver(__platform_driver, platform_driver_register, \
^~~~~~~~~~~~~
>> drivers/net//ethernet/cortina/gemini.c:2567:1: note: in expansion of macro 'module_platform_driver'
module_platform_driver(gemini_ethernet_driver);
^~~~~~~~~~~~~~~~~~~~~~
include/linux/module.h:137:7: note: previous definition of 'cleanup_module' was here
void cleanup_module(void) __attribute__((alias(#exitfn)));
^
include/linux/device.h:1518:1: note: in expansion of macro 'module_exit'
module_exit(__driver##_exit);
^~~~~~~~~~~
>> include/linux/platform_device.h:228:2: note: in expansion of macro 'module_driver'
module_driver(__platform_driver, platform_driver_register, \
^~~~~~~~~~~~~
drivers/net//ethernet/cortina/gemini.c:2495:1: note: in expansion of macro 'module_platform_driver'
module_platform_driver(gemini_ethernet_port_driver);
^~~~~~~~~~~~~~~~~~~~~~
--
In file included from include/linux/printk.h:329:0,
from include/linux/kernel.h:14,
from include/linux/list.h:9,
from include/linux/module.h:9,
from drivers/net/ethernet/cortina/gemini.c:16:
drivers/net/ethernet/cortina/gemini.c: In function 'geth_freeq_alloc_map_page':
drivers/net/ethernet/cortina/gemini.c:821:21: warning: format '%x' expects argument of type 'unsigned int', but argument 5 has type 'dma_addr_t {aka long long unsigned int}' [-Wformat=]
dev_dbg(geth->dev, "page %d, DMA addr: %08x, page %p\n",
^
include/linux/dynamic_debug.h:135:39: note: in definition of macro 'dynamic_dev_dbg'
__dynamic_dev_dbg(&descriptor, dev, fmt, \
^~~
drivers/net/ethernet/cortina/gemini.c:821:2: note: in expansion of macro 'dev_dbg'
dev_dbg(geth->dev, "page %d, DMA addr: %08x, page %p\n",
^~~~~~~
drivers/net/ethernet/cortina/gemini.c: In function 'gmac_rx':
drivers/net/ethernet/cortina/gemini.c:1410:50: warning: format '%x' expects argument of type 'unsigned int', but argument 3 has type 'dma_addr_t {aka long long unsigned int}' [-Wformat=]
dev_err(geth->dev, "could not find mapping %08x (page = %08x, page off = %08x)\n",
~~~^
%08llx
drivers/net/ethernet/cortina/gemini.c:1410:63: warning: format '%x' expects argument of type 'unsigned int', but argument 4 has type 'long long unsigned int' [-Wformat=]
dev_err(geth->dev, "could not find mapping %08x (page = %08x, page off = %08x)\n",
~~~^
%08llx
drivers/net/ethernet/cortina/gemini.c: In function 'gemini_ethernet_port_probe':
drivers/net/ethernet/cortina/gemini.c:2456:28: warning: format '%x' expects argument of type 'unsigned int', but argument 4 has type 'resource_size_t {aka long long unsigned int}' [-Wformat=]
"irq %d, DMA @ 0x%08x, GMAC @ 0x%08x\n",
~~~^
%08llx
port->irq, dmares->start,
~~~~~~~~~~~~~
drivers/net/ethernet/cortina/gemini.c:2456:43: warning: format '%x' expects argument of type 'unsigned int', but argument 5 has type 'resource_size_t {aka long long unsigned int}' [-Wformat=]
"irq %d, DMA @ 0x%08x, GMAC @ 0x%08x\n",
~~~^
%08llx
drivers/net/ethernet/cortina/gemini.c:2458:8:
gmacres->start);
~~~~~~~~~~~~~~
In file included from drivers/net/ethernet/cortina/gemini.c:16:0:
drivers/net/ethernet/cortina/gemini.c: At top level:
>> include/linux/module.h:129:42: error: redefinition of '__inittest'
static inline initcall_t __maybe_unused __inittest(void) \
^
include/linux/device.h:1513:1: note: in expansion of macro 'module_init'
module_init(__driver##_init); \
^~~~~~~~~~~
>> include/linux/platform_device.h:228:2: note: in expansion of macro 'module_driver'
module_driver(__platform_driver, platform_driver_register, \
^~~~~~~~~~~~~
drivers/net/ethernet/cortina/gemini.c:2567:1: note: in expansion of macro 'module_platform_driver'
module_platform_driver(gemini_ethernet_driver);
^~~~~~~~~~~~~~~~~~~~~~
include/linux/module.h:129:42: note: previous definition of '__inittest' was here
static inline initcall_t __maybe_unused __inittest(void) \
^
include/linux/device.h:1513:1: note: in expansion of macro 'module_init'
module_init(__driver##_init); \
^~~~~~~~~~~
>> include/linux/platform_device.h:228:2: note: in expansion of macro 'module_driver'
module_driver(__platform_driver, platform_driver_register, \
^~~~~~~~~~~~~
drivers/net/ethernet/cortina/gemini.c:2495:1: note: in expansion of macro 'module_platform_driver'
module_platform_driver(gemini_ethernet_port_driver);
^~~~~~~~~~~~~~~~~~~~~~
>> include/linux/module.h:131:6: error: redefinition of 'init_module'
int init_module(void) __attribute__((alias(#initfn)));
^
include/linux/device.h:1513:1: note: in expansion of macro 'module_init'
module_init(__driver##_init); \
^~~~~~~~~~~
>> include/linux/platform_device.h:228:2: note: in expansion of macro 'module_driver'
module_driver(__platform_driver, platform_driver_register, \
^~~~~~~~~~~~~
drivers/net/ethernet/cortina/gemini.c:2567:1: note: in expansion of macro 'module_platform_driver'
module_platform_driver(gemini_ethernet_driver);
^~~~~~~~~~~~~~~~~~~~~~
include/linux/module.h:131:6: note: previous definition of 'init_module' was here
int init_module(void) __attribute__((alias(#initfn)));
^
include/linux/device.h:1513:1: note: in expansion of macro 'module_init'
module_init(__driver##_init); \
^~~~~~~~~~~
>> include/linux/platform_device.h:228:2: note: in expansion of macro 'module_driver'
module_driver(__platform_driver, platform_driver_register, \
^~~~~~~~~~~~~
drivers/net/ethernet/cortina/gemini.c:2495:1: note: in expansion of macro 'module_platform_driver'
module_platform_driver(gemini_ethernet_port_driver);
^~~~~~~~~~~~~~~~~~~~~~
>> include/linux/module.h:135:42: error: redefinition of '__exittest'
static inline exitcall_t __maybe_unused __exittest(void) \
^
include/linux/device.h:1518:1: note: in expansion of macro 'module_exit'
module_exit(__driver##_exit);
^~~~~~~~~~~
>> include/linux/platform_device.h:228:2: note: in expansion of macro 'module_driver'
module_driver(__platform_driver, platform_driver_register, \
^~~~~~~~~~~~~
drivers/net/ethernet/cortina/gemini.c:2567:1: note: in expansion of macro 'module_platform_driver'
module_platform_driver(gemini_ethernet_driver);
^~~~~~~~~~~~~~~~~~~~~~
include/linux/module.h:135:42: note: previous definition of '__exittest' was here
static inline exitcall_t __maybe_unused __exittest(void) \
^
include/linux/device.h:1518:1: note: in expansion of macro 'module_exit'
module_exit(__driver##_exit);
^~~~~~~~~~~
>> include/linux/platform_device.h:228:2: note: in expansion of macro 'module_driver'
module_driver(__platform_driver, platform_driver_register, \
^~~~~~~~~~~~~
drivers/net/ethernet/cortina/gemini.c:2495:1: note: in expansion of macro 'module_platform_driver'
module_platform_driver(gemini_ethernet_port_driver);
^~~~~~~~~~~~~~~~~~~~~~
>> include/linux/module.h:137:7: error: redefinition of 'cleanup_module'
void cleanup_module(void) __attribute__((alias(#exitfn)));
^
include/linux/device.h:1518:1: note: in expansion of macro 'module_exit'
module_exit(__driver##_exit);
^~~~~~~~~~~
>> include/linux/platform_device.h:228:2: note: in expansion of macro 'module_driver'
module_driver(__platform_driver, platform_driver_register, \
^~~~~~~~~~~~~
drivers/net/ethernet/cortina/gemini.c:2567:1: note: in expansion of macro 'module_platform_driver'
module_platform_driver(gemini_ethernet_driver);
^~~~~~~~~~~~~~~~~~~~~~
include/linux/module.h:137:7: note: previous definition of 'cleanup_module' was here
void cleanup_module(void) __attribute__((alias(#exitfn)));
^
include/linux/device.h:1518:1: note: in expansion of macro 'module_exit'
module_exit(__driver##_exit);
^~~~~~~~~~~
>> include/linux/platform_device.h:228:2: note: in expansion of macro 'module_driver'
module_driver(__platform_driver, platform_driver_register, \
^~~~~~~~~~~~~
drivers/net/ethernet/cortina/gemini.c:2495:1: note: in expansion of macro 'module_platform_driver'
module_platform_driver(gemini_ethernet_port_driver);
^~~~~~~~~~~~~~~~~~~~~~
vim +/__inittest +129 include/linux/module.h
0fd972a7 Paul Gortmaker 2015-05-01 126
0fd972a7 Paul Gortmaker 2015-05-01 127 /* Each module must use one module_init(). */
0fd972a7 Paul Gortmaker 2015-05-01 128 #define module_init(initfn) \
1f318a8b Arnd Bergmann 2017-02-01 @129 static inline initcall_t __maybe_unused __inittest(void) \
0fd972a7 Paul Gortmaker 2015-05-01 130 { return initfn; } \
0fd972a7 Paul Gortmaker 2015-05-01 @131 int init_module(void) __attribute__((alias(#initfn)));
0fd972a7 Paul Gortmaker 2015-05-01 132
0fd972a7 Paul Gortmaker 2015-05-01 133 /* This is only required if you want to be unloadable. */
0fd972a7 Paul Gortmaker 2015-05-01 134 #define module_exit(exitfn) \
1f318a8b Arnd Bergmann 2017-02-01 @135 static inline exitcall_t __maybe_unused __exittest(void) \
0fd972a7 Paul Gortmaker 2015-05-01 136 { return exitfn; } \
0fd972a7 Paul Gortmaker 2015-05-01 @137 void cleanup_module(void) __attribute__((alias(#exitfn)));
0fd972a7 Paul Gortmaker 2015-05-01 138
:::::: The code at line 129 was first introduced by commit
:::::: 1f318a8bafcfba9f0d623f4870c4e890fd22e659 modules: mark __inittest/__exittest as __maybe_unused
:::::: TO: Arnd Bergmann <arnd@arndb.de>
:::::: CC: Jessica Yu <jeyu@redhat.com>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 49802 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180111/ec884718/attachment-0001.gz>
^ permalink raw reply
* [PATCH V6 Resend 00/13] drivers: Boot Constraint core
From: Rob Herring @ 2018-01-10 23:13 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <cover.1515554879.git.viresh.kumar@linaro.org>
On Tue, Jan 9, 2018 at 9:47 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> Hi Greg,
>
> I am re-sending V6 as you suggested. There is no change from the patches
> sent on 14/15th of December, apart from rebasing on driver-core-next.
>
> I have tested the Hisilicon patches (again) on hikey 9660 board, IMX
> stuff was earlier tested by Sascha (Pengutronix) on i.MX6 and Qualcomm
> stuff was earlier tested by Rajendra (Qualcomm) on Dragonboard 410C
> (This required some more patches related to display driver which
> Rajendra should be sending separately later on).
>
>
> Problem statement:
>
> Some devices are powered ON by the bootloader before the bootloader
> handovers control to Linux. It maybe important for those devices to keep
> working until the time a Linux device driver probes the device and
> reconfigure its resources.
Some devices are powered on by a bootloader, but only a small few have
to be maintained thru booting. Most you can just re-initialized.
> A typical example of that can be the LCD controller, which is used by
> the bootloaders to show image(s) while the platform is booting into
> Linux. The LCD controller can be using some resources, like clk,
> regulators, etc, that are shared between several devices. These shared
> resources should be configured to satisfy need of all the users. If
> another device's (X) driver gets probed before the LCD controller driver
> in this case, then it may end up disabling or reconfiguring these
> resources to ranges satisfying the current users (only device X) and
> that can make the LCD screen unstable.
We already have simple fb and a binding for it. It only handles clocks
I think, but could be extended to other things. I rather not extend
it, but it is there already and we don't need different solutions for
this.
> Another case can be a debug serial port enabled from the bootloader.
I looked at your case with HiKey some. As far as the PL011
driver/console is concerned, it should work as the clock is never
enabled/disabled and then probe deferred (IMO, doing any h/w init
before all resources are acquired is a driver error). The problem is
the AMBA bus enabling apb_pclk (which has a dedicated clk gate) and
then disabling it on deferred probe. The AMBA bus is fairly odd in
this regard. We could solve this just with an initcall to find
stdout-path node and enable all the clocks in the node and then a late
initcall to disable those clocks. Kind of hacky, but so is this
series.
Really, I think the clock framework is broken in that we leave clocks
in a mismatched state (reset state or whatever the bootloader decided)
until the end of booting. Then we are left with dealing with these
various platform specific issues. We should either not actually
disable clocks until the end of boot (just defer until we turn off all
unused clocks) or start requiring the clock drivers to turn off all
the clocks except the ones needed to continue booting (or otherwise
known to be constraints). The former should be easy to implement
because the code to turn off clocks is already there. We just need a
boot done flag and check that flag in disable function.
Rob
^ permalink raw reply
* [PATCH linux dev-4.10 3/6] drivers/misc: Add driver for Aspeed PECI and generic PECI headers
From: Jae Hyun Yoo @ 2018-01-10 23:11 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAK8P3a3MaSEYnwk2_OadBSyKnBiwXsHw_RocvPDqL6oea9MuJA@mail.gmail.com>
On 1/10/2018 3:55 AM, Arnd Bergmann wrote:
> On Tue, Jan 9, 2018 at 11:31 PM, Jae Hyun Yoo
> <jae.hyun.yoo@linux.intel.com> wrote:
>> This commit adds driver implementation for Aspeed PECI. Also adds
>> generic peci.h and peci_ioctl.h files to provide compatibility
>> to peci drivers that can be implemented later e.g. Nuvoton's BMC
>> SoC family.
>>
>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>
>> +#include <linux/clk.h>
>> +#include <linux/crc8.h>
>> +#include <linux/delay.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/jiffies.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/miscdevice.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/peci_ioctl.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/semaphore.h>
>> +#include <linux/types.h>
>> +#include <linux/uaccess.h>
>
> semaphore.h is not used here and can be dropped.
>
You are right. Will drop it.
>> +static struct aspeed_peci *aspeed_peci_priv;
>
> Try to avoid instance variables like this one. You should always be able to find
> that pointer from whatever structure you were called with.
>
>
Okay. I will use driver_data instead.
>> + timeout = wait_for_completion_interruptible_timeout(
>> + &priv->xfer_complete,
>> + msecs_to_jiffies(priv->cmd_timeout_ms));
>> +
>> + dev_dbg(priv->dev, "INT_STS : 0x%08x\n", priv->sts);
>> + if (!regmap_read(priv->regmap, AST_PECI_CMD, &peci_state))
>> + dev_dbg(priv->dev, "PECI_STATE : 0x%lx\n",
>> + PECI_CMD_STS_GET(peci_state));
>> + else
>> + dev_dbg(priv->dev, "PECI_STATE : read error\n");
>> +
>> + if (timeout <= 0 || !(priv->sts & PECI_INT_CMD_DONE)) {
>> + if (timeout <= 0) {
>> + dev_dbg(priv->dev, "Timeout waiting for a response!\n");
>> + rc = -ETIME;
>> + } else {
>> + dev_dbg(priv->dev, "No valid response!\n");
>> + rc = -EFAULT;
>> + }
>> + return rc;
>> + }
>
> You don't seem to handle -ERESTARTSYS correct here. Either do it
> right, or drop the _interruptible part above.
>
Will add a handling logic for the -ERESTARTSYS.
>> +typedef int (*ioctl_fn)(struct aspeed_peci *, void *);
>> +
>> +static ioctl_fn peci_ioctl_fn[PECI_CMD_MAX] = {
>> + ioctl_xfer_msg,
>> + ioctl_ping,
>> + ioctl_get_dib,
>> + ioctl_get_temp,
>> + ioctl_rd_pkg_cfg,
>> + ioctl_wr_pkg_cfg,
>> + ioctl_rd_ia_msr,
>> + NULL, /* Reserved */
>> + ioctl_rd_pci_cfg,
>> + NULL, /* Reserved */
>> + ioctl_rd_pci_cfg_local,
>> + ioctl_wr_pci_cfg_local,
>> +};
>> +
>> +
>> +long peci_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>> +{
>> + struct aspeed_peci *priv;
>> + long ret = 0;
>> + void __user *argp = (void __user *)arg;
>> + int timeout = PECI_IDLE_CHECK_TIMEOUT;
>> + u8 msg[sizeof(struct peci_xfer_msg)];
>> + unsigned int peci_cmd, msg_size;
>> + u32 cmd_sts;
>> +
>> + /*
>> + * Treat it as an inter module call when filp is null but only in case
>> + * the private data is initialized.
>> + */
>> + if (filp)
>> + priv = container_of(filp->private_data,
>> + struct aspeed_peci, miscdev);
>> + else
>> + priv = aspeed_peci_priv;
>
> Drop this.
>
peci_ioctl is being called from peci_hwmon as an inter-module call so it
is needed, but as you suggested in the other patch, I'll consider
redesign it with adding a peci device class.
>> + if (!priv)
>> + return -ENXIO;
>> +
>> + switch (cmd) {
>> + case PECI_IOC_XFER:
>> + case PECI_IOC_PING:
>> + case PECI_IOC_GET_DIB:
>> + case PECI_IOC_GET_TEMP:
>> + case PECI_IOC_RD_PKG_CFG:
>> + case PECI_IOC_WR_PKG_CFG:
>> + case PECI_IOC_RD_IA_MSR:
>> + case PECI_IOC_RD_PCI_CFG:
>> + case PECI_IOC_RD_PCI_CFG_LOCAL:
>> + case PECI_IOC_WR_PCI_CFG_LOCAL:
>> + peci_cmd = _IOC_TYPE(cmd) - PECI_IOC_BASE;
>> + msg_size = _IOC_SIZE(cmd);
>> + break;
>
> Having to keep the switch() statement and the array above seems a
> little fragile. Can you just do one or the other?
>
> Regarding the command set, you have both a low-level PECI_IOC_XFER
> interface and a high-level interface. Can you explain why? I'd think that
> generally speaking it's better to have only one of the two.
>
I was intended to provide generic peci command set, also the low level
PECI_IOC_XFER to provide flexibility for a case when compose a custom
peci command which cannot be covered by the high-level command set. As
you said, all other commands can be implemented in the upper layer but
the benefit of when this driver has the implementation is, it's easy to
manage retry logic since peci is retrial based protocol intends to do
not disturb a CPU if the CPU is doing more important task.
However, your thought also makes sense. I'll check the spec again
whether the high-level command set can cover all cases. If so, I'll
remove the low-level command.
>> + /* Check command sts and bus idle state */
>> + while (!regmap_read(priv->regmap, AST_PECI_CMD, &cmd_sts)
>> + && (cmd_sts & (PECI_CMD_STS_MASK | PECI_CMD_PIN_MON))) {
>> + if (timeout-- < 0) {
>> + dev_dbg(priv->dev, "Timeout waiting for idle state!\n");
>> + ret = -ETIME;
>> + goto out;
>> + }
>> + usleep_range(10000, 11000);
>> + };
>
> To implement timeout, it's better to replace the counter with a
> jiffies/time_before or ktime_get()/ktime_before() check, since usleep_range()
> is might sleep considerably longer than expected.
>
Thanks for the suggestion. Will rewrite it using ktime_get()/ktime_before().
>> +EXPORT_SYMBOL_GPL(peci_ioctl);
>
> No user of this, so drop it.
>
peci_hwmon is using it.
>> +static int aspeed_peci_open(struct inode *inode, struct file *filp)
>> +{
>> + struct aspeed_peci *priv =
>> + container_of(filp->private_data, struct aspeed_peci, miscdev);
>> +
>> + atomic_inc(&priv->ref_count);
>> +
>> + dev_dbg(priv->dev, "ref_count : %d\n", atomic_read(&priv->ref_count));
>> +
>> + return 0;
>> +}
>> +
>> +static int aspeed_peci_release(struct inode *inode, struct file *filp)
>> +{
>> + struct aspeed_peci *priv =
>> + container_of(filp->private_data, struct aspeed_peci, miscdev);
>> +
>> + atomic_dec(&priv->ref_count);
>> +
>> + dev_dbg(priv->dev, "ref_count : %d\n", atomic_read(&priv->ref_count));
>> +
>> + return 0;
>> +}
>
> Nothing uses that reference count, drop it.
>
You are right. Will drop it.
>> new file mode 100644
>> index 0000000..66322c6
>> --- /dev/null
>> +++ b/include/misc/peci.h
>> @@ -0,0 +1,11 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (c) 2017 Intel Corporation
>> +
>> +#ifndef __PECI_H
>> +#define __PECI_H
>> +
>> +#include <linux/peci_ioctl.h>
>> +
>> +long peci_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
>> +
>> +#endif /* __PECI_H */
>
> Not used anywhere.
>
peci_hwmon is using it.
>> diff --git a/include/uapi/linux/peci_ioctl.h b/include/uapi/linux/peci_ioctl.h
>> new file mode 100644
>> index 0000000..8386848
>> --- /dev/null
>> +++ b/include/uapi/linux/peci_ioctl.h
>> @@ -0,0 +1,270 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (c) 2017 Intel Corporation
>> +
>> +#ifndef __PECI_IOCTL_H
>> +#define __PECI_IOCTL_H
>> +
>> +#include <linux/ioctl.h>
>> +
>> +/* Base Address of 48d */
>> +#define PECI_BASE_ADDR 0x30 /* The PECI client's default address of 0x30 */
>> +#define PECI_OFFSET_MAX 8 /* Max numver of CPU clients */
>> +
>> +/* PCI Access */
>> +#define MAX_PCI_READ_LEN 24 /* Number of bytes of the PCI Space read */
>> +
>> +#define PCI_BUS0_CPU0 0x00
>> +#define PCI_BUS0_CPU1 0x80
>> +#define PCI_CPUBUSNO_BUS 0x00
>> +#define PCI_CPUBUSNO_DEV 0x08
>> +#define PCI_CPUBUSNO_FUNC 0x02
>> +#define PCI_CPUBUSNO 0xcc
>> +#define PCI_CPUBUSNO_1 0xd0
>> +#define PCI_CPUBUSNO_VALID 0xd4
>
> I can't tell for sure, but this file seems to be mixing the kernel API with
> hardware specific macros that are not needed in user space. Can you move
> some of this file into the driver itself?
>
> This might go back to the previous question about the high-level and
> low-level interfaces: if you can drop the low-level ioctl interface, more
> of this header can become private to the driver.
>
As I answered above, I'll check the spec again and remove the low-level
command if the high-level command set covers all cases.
>> +/* Package Identifier Read Parameter Value */
>> +#define PKG_ID_CPU_ID 0x0000 /* 0 - CPUID Info */
>> +#define PKG_ID_PLATFORM_ID 0x0001 /* 1 - Platform ID */
>> +#define PKG_ID_UNCORE_ID 0x0002 /* 2 - Uncore Device ID */
>> +#define PKG_ID_MAX_THREAD_ID 0x0003 /* 3 - Max Thread ID */
>> +#define PKG_ID_MICROCODE_REV 0x0004 /* 4 - CPU Microcode Update Revision */
>> +#define PKG_ID_MACHINE_CHECK_STATUS 0x0005 /* 5 - Machine Check Status */
>> +
>> +/* RdPkgConfig Index */
>> +#define MBX_INDEX_CPU_ID 0 /* Package Identifier Read */
>> +#define MBX_INDEX_VR_DEBUG 1 /* VR Debug */
>> +#define MBX_INDEX_PKG_TEMP_READ 2 /* Package Temperature Read */
>> +#define MBX_INDEX_ENERGY_COUNTER 3 /* Energy counter */
>> +#define MBX_INDEX_ENERGY_STATUS 4 /* DDR Energy Status */
>> +#define MBX_INDEX_WAKE_MODE_BIT 5 /* "Wake on PECI" Mode bit */
>> +#define MBX_INDEX_EPI 6 /* Efficient Performance Indication */
>
> Who defines these constants? Are they specific to the aspeed BMC, to the HECI
> protocol, or to a particular version of the remote endpoint?
>
These are common peci definitions, not the aspeed BMC specific.
>> +#pragma pack(push, 1)
>> +struct peci_xfer_msg {
>> + unsigned char client_addr;
>> + unsigned char tx_len;
>> + unsigned char rx_len;
>> + unsigned char tx_buf[MAX_BUFFER_SIZE];
>> + unsigned char rx_buf[MAX_BUFFER_SIZE];
>> +};
>> +#pragma pack(pop)
>> +
>> +struct peci_ping_msg {
>> + unsigned char target;
>> +};
>> +
>> +struct peci_get_dib_msg {
>> + unsigned char target;
>> + unsigned int dib;
>> +};
>> +
>> +struct peci_get_temp_msg {
>> + unsigned char target;
>> + signed short temp_raw;
>> +};
>
> Aside from what Greg already said about the types, please be careful to
> also avoid implicit padding in the API data structures, including the end of the
> structure.
>
Okay, I'll expand the pack() scope for all these definition.
>> +#define PECI_IOC_RD_PCI_CFG \
>> + _IOWR(PECI_IOC_BASE + PECI_CMD_RD_PCI_CFG, 0, \
>> + struct peci_rd_pci_cfg_msg)
>> +
>> +#define PECI_IOC_RD_PCI_CFG_LOCAL \
>> + _IOWR(PECI_IOC_BASE + PECI_CMD_RD_PCI_CFG_LOCAL, 0, \
>> + struct peci_rd_pci_cfg_local_msg)
>> +
>> +#define PECI_IOC_WR_PCI_CFG_LOCAL \
>> + _IOWR(PECI_IOC_BASE + PECI_CMD_WR_PCI_CFG_LOCAL, 0, \
>> + struct peci_wr_pci_cfg_local_msg)
>
> Can you give some background on what these do? In particular, who
> is configuring whose PCI devices?
>
> Arnd
>
These are commands to read/write a client CPU's PCI configuration which
could be an end-point of the physical PECI interface connection. BMC
controller will be a host and a CPU will be a client.
Thanks,
Jae
^ permalink raw reply
* [PATCH v3 7/7] EDAC: Add driver for the Marvell Armada XP SDRAM and L2 cache ECC
From: Chris Packham @ 2018-01-10 23:05 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171116193145.s2nja2kfevjxlnt5@pd.tnic>
Hi Borislav,
On 17/11/17 08:32, Borislav Petkov wrote:
> That struct name is too long. So long that it makes you break parameter
> names below.
>
> For example, you don't need that lenghty prefix "armada_xp_mc_edac"
> prepended to static symbols. Same for the function names below.
Would you suggest dropping "armada_xp" altogether or just shortening to
"axp"? What about dropping "edac" as well?
^ permalink raw reply
* [PATCH v2 0/6] ARM branch predictor hardening
From: Nishanth Menon @ 2018-01-10 22:59 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180108185533.9698-1-marc.zyngier@arm.com>
On 01/08/2018 12:55 PM, Marc Zyngier wrote:
[...]
> These patches are closely modelled against what we do on arm64,
> although simpler as we can rely on an architected instruction to
> perform the invalidation. The notable exception is Cortex-A15, where
> BTB invalidation behaves like a NOP, and the only way to shoot the
> predictor down is to invalidate the icache *and* to have ACTLR[0] set
> to 1 (which is a secure-only operation).
On u-boot side, we'd probably have to work off ->
http://git.denx.de/?p=u-boot.git;a=commitdiff;h=a615d0be6a73fc48a22e5662608260fe9b9149ff
[...]
--
Regards,
Nishanth Menon
^ permalink raw reply
* [GIT PULL] Allwinner clock changes for 4.16, round 2
From: Stephen Boyd @ 2018-01-10 22:47 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180108034901.GA5577@wens.csie.org>
On 01/08, Chen-Yu Tsai wrote:
> Hi Mike, Stephen,
>
> Here are a few more clock changes for 4.16. The DE2 clock changes have
> been around for awhile. The A83T clock fix was found by Jernej while
> working on HDMI output for the SoC. We'd like to get these in so patches
> don't pile up on the mailing lists.
>
> ChenYu
>
> The following changes since commit e952ca3c6b2ffdfbf9618e4bd3e9aad1ff3f5eb4:
>
> clk: sunxi-ng: sun8i: a83t: Use sigma-delta modulation for audio PLL (2017-12-08 10:08:32 +0100)
>
> are available in the Git repository at:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/sunxi/linux.git tags/sunxi-clk-for-4.16-2
>
> for you to fetch changes up to 7dbc7f5f4904cfddc199af171ea095490a434f15:
>
> clk: sunxi-ng: a83t: Add M divider to TCON1 clock (2018-01-03 13:45:04 +0800)
>
> ----------------------------------------------------------------
> Allwinner clock changes for 4.16, part 2
>
> The hardware description for the DE2 clocks on the A64 and H5 SoCs is
> fixed, and support for these clocks on the H3 is added. Also, the
> description for the TCON1 clock on the A83T is fixed. All these are
> non-critical, as they have no users.
Thanks. Pulled into clk-next.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* [PATCH v5 01/44] dt-bindings: clock: Add new bindings for TI Davinci PLL clocks
From: Adam Ford @ 2018-01-10 22:24 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5aacc350-6236-2e4f-35bb-a681fc9d47e7@ti.com>
On Wed, Jan 10, 2018 at 12:52 PM, Sekhar Nori <nsekhar@ti.com> wrote:
> On Wednesday 10 January 2018 08:31 AM, David Lechner wrote:
>> On 01/09/2018 06:35 AM, Sekhar Nori wrote:
>>> On Monday 08 January 2018 09:59 PM, David Lechner wrote:
>>>> On 01/08/2018 08:00 AM, Sekhar Nori wrote:
>>>>> On Monday 08 January 2018 07:47 AM, David Lechner wrote:
>
>>>>>> diff --git
>>>>>> a/Documentation/devicetree/bindings/clock/ti/davinci/pll.txt
>>>>>> b/Documentation/devicetree/bindings/clock/ti/davinci/pll.txt
>>>>>> new file mode 100644
>>>>>> index 0000000..99bf5da
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/clock/ti/davinci/pll.txt
>>>>>> @@ -0,0 +1,47 @@
>>>>>> +Binding for TI DaVinci PLL Controllers
>>>>>> +
>>>>>> +The PLL provides clocks to most of the components on the SoC. In
>>>>>> addition
>>>>>> +to the PLL itself, this controller also contains bypasses, gates,
>>>>>> dividers,
>>>>>> +an multiplexers for various clock signals.
>>>>>> +
>>>>>> +Required properties:
>>>>>> +- compatible: shall be one of:
>>>>>> + - "ti,da850-pll0" for PLL0 on DA850/OMAP-L138/AM18XX
>>>>>> + - "ti,da850-pll1" for PLL1 on DA850/OMAP-L138/AM18XX
>>>>>
>>>>> These PLLs are same IP so they should use the same compatible. You can
>>>>> initialize both PLLs for DA850 based on the same compatible.
>>>>>
>>>>
>>>> But they are not exactly the same. For example, PLL0 has 7 PLLDIV clocks
>>>> while
>>>> PLL1 only has 3. PLL0 has PREDIV while PLL1 does not. PLL0 has certain
>>>> SYSCLKs
>>>> that are fixed-ratio but PLL1 does not have any of these. There are even
>>>> more
>>>> differences, but these are the ones we are actually using.
>>>
>>> We need each element of the PLLC to be modeled individually as a clock
>>> node.
>>
>> I gave this a good think while I have been working on this series
>> and I came to the conclusion that we really don't need to do this.
>> These components are all internal to the PLL IP block, so the
>> compatible string is enough to tell us what we have. They only
>> thing we need really in the device tree bindings are the connections
>> that are external to the IP block.
>>
>>
>>> That is, PLL should only model the multiplier, the dividers
>>> including post and prediv should be modeled as divider clocks (hopefully
>>> being able to use the clk-divider.c library). The sysclks can be
>>> fixed-factor-clock type clocks.
>>>
>>> Without this flexible mechanism, we cannot (at least later) model things
>>> like DIV4.5 clock which is the only clock which derives from the output
>>> of PLL multiplier before the post divider is applied.
>>>
>>> Since with DT there are are no retakes, we need to get this right the
>>> first time and modifying later will not be an option.
>>>
>>
>> So, the full device tree binding would look something like this:
>>
>> +
>> + pll0: clock-controller at 11000 {
>> + compatible = "ti,da850-pll0";
>> + reg = <0x11000 0x1000>;
>> + clocks = <&ref_clk>, <&pll1_sysclk 3>, <&pll1_obsclk>;
>> + clock-names = "oscin", pll1_sysclk3", "pll1_osbclk";
>> + oscin-square-wave;
>> +
>> + pll0_sysclk: sysclk {
>> + #clock-cells = <1>;
>> + };
>> +
>> + pll0_auxclk: auxclk {
>> + #clock-cells = <0>;
>> + };
>> +
>> + pll0_div45: div4.5 {
>> + #clock-cells = <0>;
>> + };
>> +
>> + pll0_obsclk: obsclk {
>> + #clock-cells = <0>;
>> + assigned-clocks = <&pll0_sysclk 1>;
>> + assigned-clock-names = "ocsrc";
>> + };
>> + };
>
> Well, I guess this will work as well. And I am probably biased towards
> the style I mentioned because AM335x and other TI OMAP processors
> follow that.
>
> To make it easy to review that we have all bases covered, can you model
> the all PLLC0 and PLLC1 (input and output) clocks for the next version?
>
>>
>> There are three clocks coming into the IP block and there are 11 clocks
>> going out (sysclk is 7 clocks). And you can specify the board-specific
>> configuration, like having the "oscin-square-wave" flag when a square wave
>> is used instead of a crystal oscillator and you can assign the multiplexer
>
> Ideally the OSCIN vs CLKIN selection should be another clock mux whose
> output is one of the input clocks to PLL controller. But I can see the
> difficulty in handling that as the mux itself is controlled by the PLL
> controller.
>
>> input that will be used by obsclk. (And, this binding is totally compatible
>> with the binding I have already proposed - although, I see now it would
>> be better to go ahead and add the clocks-names property.)
>
> Also, please add the oscin-square-wave to the binding definition too.
>
> For the benefit of others reviewing and not familiar with the hardware,
> the users guide for DA850 is here:
> http://www.ti.com/lit/ug/spruh77c/spruh77c.pdf
>
I am available tomorrow to build and test patches against the
da850-evm. I just need to know which version(s) to test.
adam
> and the PLL block diagram is on page 143 (Figure 8-1).
>
> Thanks,
> Sekhar
^ permalink raw reply
* [PATCH net-next 2/2 v10] net: ethernet: Add a driver for Gemini gigabit ethernet
From: Linus Walleij @ 2018-01-10 22:02 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180110.154322.2235369645563224203.davem@davemloft.net>
On Wed, Jan 10, 2018 at 9:43 PM, David Miller <davem@davemloft.net> wrote:
> Actually I had to revert these two patches, this stuff doesn't even
> build.
Sorry about that, I did build it before sending out but I guess not on enough
architectures :(
I will iterate with the help of the zeroday build servers, my own machines
just don't suffice.
Yours,
Linus Walleij
^ permalink raw reply
* [PATCH v3 7/7] EDAC: Add driver for the Marvell Armada XP SDRAM and L2 cache ECC
From: Chris Packham @ 2018-01-10 21:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171116193145.s2nja2kfevjxlnt5@pd.tnic>
Hi Borislav,
On 17/11/17 08:32, Borislav Petkov wrote:
> On Fri, Nov 10, 2017 at 10:03:08AM +0100, Jan Luebbe wrote:
>> Add support for the ECC functionality as found in the DDR RAM and L2
>> cache controllers on the MV78230/MV78x60 SoCs. This driver has been
>> tested on the MV78460 (on a custom board with a DDR3 ECC DIMM).
>>
>> Signed-off-by: Jan Luebbe <jlu@pengutronix.de>
>
> ...
>
>> diff --git a/drivers/edac/armada_xp_edac.c b/drivers/edac/armada_xp_edac.c
>> new file mode 100644
>> index 000000000000..cb9173b30aa9
>> --- /dev/null
>> +++ b/drivers/edac/armada_xp_edac.c
>> @@ -0,0 +1,658 @@
>> +/*
>> + * Copyright (C) 2017 Pengutronix, Jan Luebbe <kernel@pengutronix.de>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/edac.h>
>> +#include <linux/of_platform.h>
>> +
>> +#include <asm/hardware/cache-l2x0.h>
>
> I get this when I cross-build it:
>
> drivers/edac/armada_xp_edac.c:19:37: fatal error: asm/hardware/cache-l2x0.h: No such file or directory
> #include <asm/hardware/cache-l2x0.h>
> ^
> compilation terminated.
> make[2]: *** [drivers/edac/armada_xp_edac.o] Error 1
> make[1]: *** [drivers/edac] Error 2
> make[1]: *** Waiting for unfinished jobs....
> make: *** [drivers] Error 2
>
>
> and that driver depends on ARCH_MVEBU which is arm64, AFAICT.
>
> That header is
>
> arch/arm/include/asm/hardware/cache-l2x0.h
>
> however and not in arch/arm64/.
>
> I could very well be missing something though...
>
As Andrew said ARCH_MVEBU includes both 32-bit and 64-bit processors.
In terms of fixing this issue there are a few options. My first blunt
tool approach is
- depends on ARCH_MVEBU
+ depends on ARCH_MVEBU && !ARM64
But that's not a particularly common pattern outside of arch/arm
Another option would be
- depends on ARCH_MVEBU
+ depends on MACH_MVEBU_V7
There seems to be plenty of precedent for depending on a MACH family in
other drivers.
We could even go as far as wrapping header include and L2 cache parts
with #ifdef CONFIG_CACHE_L2X0.
Do you have any preference for which option to take? I'm leaning towards
MACH_MVEBU_V7.
^ permalink raw reply
* [PATCH v2 1/6] arm: Add BTB invalidation on switch_mm for Cortex-A9, A12 and A17
From: Nishanth Menon @ 2018-01-10 21:52 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <6662dd72-121c-0cfb-ea31-aeecad4c7d03@arm.com>
On 01/10/2018 11:57 AM, Marc Zyngier wrote:
> On 10/01/18 17:53, Tony Lindgren wrote:
>> * Marc Zyngier <marc.zyngier@arm.com> [180108 19:00]:
>>> In order to avoid aliasing attacks against the branch predictor,
>>> some implementations require to invalidate the BTB when switching
>>> from one user context to another.
>>>
>>> For this, we reuse the existing implementation for Cortex-A8, and
>>> apply it to A9, A12 and A17.
>>
>> I suspect we now must also make sure Cortex-A8 has the IBE bit
>> set unconditionally for this to work. Currently the assumption is
>> that IBE bit needs to be set only on the earlier CPU revisions
>> that suffer from ARM_ERRATA_430973.
>>
>>> --- a/arch/arm/mm/proc-v7-2level.S
>>> +++ b/arch/arm/mm/proc-v7-2level.S
>>> @@ -41,7 +41,7 @@
>>> * even on Cortex-A8 revisions not affected by 430973.
>>> * If IBE is not set, the flush BTAC/BTB won't do anything.
>>> */
>>> -ENTRY(cpu_ca8_switch_mm)
>>> +ENTRY(cpu_v7_btbinv_switch_mm)
>>> #ifdef CONFIG_MMU
>>> mov r2, #0
>>> mcr p15, 0, r2, c7, c5, 6 @ flush BTAC/BTB
>>
>> So without IBE set, as the comments above say, the flush won't
>> do anything.
>
> Indeed. Firmware/bootloaders must be updated to set IBE, just like on
> Cortex-A15. I'll add a note to that effect.
OK. in u-boot, I had helped on the following:
http://git.denx.de/?p=u-boot.git;a=commitdiff;h=5902f4ce0f2bd1411e40dc0ece3598a0fc19b2ae
maybe be build off that?
--
Regards,
Nishanth Menon
^ permalink raw reply
* [PATCH 2/2] soc: imx: gpc: Do not pass static memory as platform data
From: Andrey Smirnov @ 2018-01-10 21:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1f914ea33fef8ede0c81085bedf46ed5@agner.ch>
On Wed, Jan 10, 2018 at 12:49 PM, Stefan Agner <stefan@agner.ch> wrote:
> On 2018-01-10 17:16, Andrey Smirnov wrote:
>> Platform device core assumes the ownership of dev.platform_data as
>> well as that it is dynamically allocated and it will try to kfree it
>> as a part of platform_device_release(). Change the code to pass
>> kzalloc'ed chunk of memory instead of a pointer to a static memory to
>> avoid causing a BUG() when calling platform_device_put().
>
> I tried to get around that by setting platform_data to null before
> unregistring the device, see:
> https://marc.info/?l=linux-arm-kernel&m=151553216030129&w=2
>
Sorry should've commented in that thread: I saw that in your code, but
it felt to me like playing with fire a bit. IMHO calling
platform_device_put() should just work and not depend on certain field
being set to NULL prior.
>
> This solutions still seems to miss unregistering the platform devices,
> which shows when binding the driver again:
>
Absolutely, this patch solves a problem, not the problem :-) I think
solving the problem is orthogonal to this and warrants a separate
patch.
> root at colibri-imx6:~# echo 20dc000.gpc >
> /sys/bus/platform/drivers/imx-gpc/unbind
> [ 80.702627] imx-pgc-pd imx-pgc-power-domain.0: Dropping the link to
> 20dc000.gpc
> [ 80.710808] genpd_remove: unable to remove PU
> [ 80.716408] imx-pgc-pd imx-pgc-power-domain.1: Dropping the link to
> 20dc000.gpc
> root at colibri-imx6:~# find /sys -name *pgc-power*
> /sys/devices/soc0/soc/2000000.aips-bus/20dc000.gpc/imx-pgc-power-domain.0
> /sys/devices/soc0/soc/2000000.aips-bus/20dc000.gpc/imx-pgc-power-domain.1
> /sys/bus/platform/devices/imx-pgc-power-domain.0
> /sys/bus/platform/devices/imx-pgc-power-domain.1
> root at colibri-imx6:~# echo 20dc000.gpc >
> /sys/bus/platform/drivers/imx-gpc/bind
> [ 89.002754] ------------[ cut here ]------------
> [ 89.007411] WARNING: CPU: 0 PID: 516 at fs/sysfs/dir.c:31
> sysfs_warn_dup+0x64/0x74
> [ 89.015057] sysfs: cannot create duplicate filename
> '/devices/soc0/soc/2000000.aips-bus/20dc000.gpc/imx-pgc-power-domain.0'
>
>
>>
>> The problem can be reproduced by artificially enabling the error path
>> of platform_device_add() call (around line 452).
>>
>> Cc: Shawn Guo <shawnguo@kernel.org>
>> Cc: Stefan Agner <stefan@agner.ch>
>> Cc: Lucas Stach <l.stach@pengutronix.de>
>> Cc: linux-arm-kernel at lists.infradead.org
>> Cc: linux-kernel at vger.kernel.org
>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>> ---
>>
>> This patch is a follow up to fix one of the bugs discussed in
>> lkml.kernel.org/r/3f836677c6e98aaf01bc1ac8c3410083 at agner.ch
>>
>> drivers/soc/imx/gpc.c | 9 ++++++++-
>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c
>> index 47e7aa963dbb..ec8b79abebac 100644
>> --- a/drivers/soc/imx/gpc.c
>> +++ b/drivers/soc/imx/gpc.c
>> @@ -18,6 +18,7 @@
>> #include <linux/pm_domain.h>
>> #include <linux/regmap.h>
>> #include <linux/regulator/consumer.h>
>> +#include <linux/slab.h>
>>
>> #define GPC_CNTR 0x000
>>
>> @@ -428,13 +429,19 @@ static int imx_gpc_probe(struct platform_device *pdev)
>> if (domain_index >= of_id_data->num_domains)
>> continue;
>>
>> - domain = &imx_gpc_domains[domain_index];
>> + domain = kzalloc(sizeof(*domain), GFP_KERNEL);
>
> I guess you could use just kalloc here since you memcpy below.
>
Good point. Will change in v2.
Thanks,
Andrey Smirnov
^ permalink raw reply
* [linux, dev-4.10, 6/6] drivers/hwmon: Add a driver for a generic PECI hwmon
From: Guenter Roeck @ 2018-01-10 21:47 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180109223126.13093-7-jae.hyun.yoo@linux.intel.com>
On Tue, Jan 09, 2018 at 02:31:26PM -0800, Jae Hyun Yoo wrote:
> This commit adds driver implementation for a generic PECI hwmon.
>
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> ---
> drivers/hwmon/Kconfig | 6 +
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/peci-hwmon.c | 953 +++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 960 insertions(+)
> create mode 100644 drivers/hwmon/peci-hwmon.c
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 9256dd0..3a62c60 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1234,6 +1234,12 @@ config SENSORS_NCT7904
> This driver can also be built as a module. If so, the module
> will be called nct7904.
>
> +config SENSORS_PECI_HWMON
> + tristate "PECI hwmon support"
> + depends on ASPEED_PECI
> + help
> + If you say yes here you get support for the generic PECI hwmon driver.
> +
> config SENSORS_NSA320
> tristate "ZyXEL NSA320 and compatible fan speed and temperature sensors"
> depends on GPIOLIB && OF
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 98000fc..41d43a5 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -131,6 +131,7 @@ obj-$(CONFIG_SENSORS_NCT7802) += nct7802.o
> obj-$(CONFIG_SENSORS_NCT7904) += nct7904.o
> obj-$(CONFIG_SENSORS_NSA320) += nsa320-hwmon.o
> obj-$(CONFIG_SENSORS_NTC_THERMISTOR) += ntc_thermistor.o
> +obj-$(CONFIG_SENSORS_PECI_HWMON) += peci-hwmon.o
> obj-$(CONFIG_SENSORS_PC87360) += pc87360.o
> obj-$(CONFIG_SENSORS_PC87427) += pc87427.o
> obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
> diff --git a/drivers/hwmon/peci-hwmon.c b/drivers/hwmon/peci-hwmon.c
> new file mode 100644
> index 0000000..2d2a288
> --- /dev/null
> +++ b/drivers/hwmon/peci-hwmon.c
> @@ -0,0 +1,953 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2017 Intel Corporation
> +
> +#include <linux/delay.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/jiffies.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/syscalls.h>
> +#include <misc/peci.h>
misc, not linux ? That seems wrong.
> +
> +#define DEVICE_NAME "peci-hwmon"
> +#define HWMON_NAME "peci_hwmon"
> +
> +#define CPU_ID_MAX 8 /* Max CPU number configured by socket ID */
> +#define DIMM_NUMS_MAX 16 /* Max DIMM numbers (channel ranks x 2) */
> +#define CORE_NUMS_MAX 28 /* Max core numbers (max on SKX Platinum) */
I won't insist, but it would be better if this were dynamic,
otherwise we'll end up having to increase the defines in the future.
> +#define TEMP_TYPE_PECI 6 /* Sensor type 6: Intel PECI */
> +#define CORE_INDEX_OFFSET 100 /* sysfs filename start offset for core temp */
> +#define DIMM_INDEX_OFFSET 200 /* sysfs filename start offset for DIMM temp */
Did you test with the "sensors" command to ensure that this works,
with the large gaps in index values ?
Overall, I am not very happy with the indexing. Since each sensor as
a label, it might be better to just make it dynamic.
> +#define TEMP_NAME_HEADER_LEN 4 /* sysfs temp type header length */
> +#define OF_DIMM_NUMS_DEFAULT 16 /* default dimm-nums setting */
> +
> +#define CORE_TEMP_ATTRS 5
> +#define DIMM_TEMP_ATTRS 2
> +#define ATTR_NAME_LEN 24
> +
> +#define UPDATE_INTERVAL_MIN HZ
> +
> +enum sign_t {
> + POS,
> + NEG
> +};
> +
> +struct cpuinfo_t {
> + bool valid;
> + u32 dib;
> + u8 cpuid;
> + u8 platform_id;
> + u32 microcode;
> + u8 logical_thread_nums;
> +};
> +
> +struct temp_data_t {
> + bool valid;
> + s32 value;
> + unsigned long last_updated;
> +};
> +
> +struct temp_group_t {
> + struct temp_data_t tjmax;
> + struct temp_data_t tcontrol;
> + struct temp_data_t tthrottle;
> + struct temp_data_t dts_margin;
> + struct temp_data_t die;
> + struct temp_data_t core[CORE_NUMS_MAX];
> + struct temp_data_t dimm[DIMM_NUMS_MAX];
> +};
> +
> +struct core_temp_attr_group_t {
> + struct sensor_device_attribute sd_attrs[CORE_NUMS_MAX][CORE_TEMP_ATTRS];
> + char attr_name[CORE_NUMS_MAX][CORE_TEMP_ATTRS][ATTR_NAME_LEN];
> + struct attribute *attrs[CORE_NUMS_MAX][CORE_TEMP_ATTRS + 1];
> + struct attribute_group attr_group[CORE_NUMS_MAX];
> +};
> +
> +struct dimm_temp_attr_group_t {
> + struct sensor_device_attribute sd_attrs[DIMM_NUMS_MAX][DIMM_TEMP_ATTRS];
> + char attr_name[DIMM_NUMS_MAX][DIMM_TEMP_ATTRS][ATTR_NAME_LEN];
> + struct attribute *attrs[DIMM_NUMS_MAX][DIMM_TEMP_ATTRS + 1];
> + struct attribute_group attr_group[DIMM_NUMS_MAX];
> +};
> +
> +struct peci_hwmon {
> + struct device *dev;
> + struct device *hwmon_dev;
> + char name[NAME_MAX];
> + const struct attribute_group **groups;
> + struct cpuinfo_t cpuinfo;
> + struct temp_group_t temp;
> + u32 cpu_id;
> + bool show_core;
> + u32 core_nums;
> + u32 dimm_nums;
> + atomic_t core_group_created;
> + struct core_temp_attr_group_t core;
> + struct dimm_temp_attr_group_t dimm;
> +};
> +
> +enum label_t {
> + L_DIE,
> + L_DTS,
> + L_TCONTROL,
> + L_TTHROTTLE,
> + L_MAX
> +};
> +
> +static const char *peci_label[L_MAX] = {
> + "Die temperature\n",
> + "DTS thermal margin to Tcontrol\n",
> + "Tcontrol temperature\n",
> + "Tthrottle temperature\n",
"temperature" is redundant for a temperature label.
> +};
> +
> +static DEFINE_MUTEX(peci_hwmon_lock);
> +
> +static int create_core_temp_group(struct peci_hwmon *priv, int core_no);
Please avoid forward declarations.
> +
> +
Please run your patches throuch checkpatch --strict and fix what it reports,
or provide a reason why you don't.
> +static int xfer_peci_msg(int cmd, void *pmsg)
> +{
> + int rc;
> +
> + mutex_lock(&peci_hwmon_lock);
> + rc = peci_ioctl(NULL, cmd, (unsigned long)pmsg);
> + mutex_unlock(&peci_hwmon_lock);
> +
> + return rc;
> +}
> +
> +static int get_cpuinfo(struct peci_hwmon *priv)
> +{
> + struct peci_get_dib_msg dib_msg;
> + struct peci_rd_pkg_cfg_msg cfg_msg;
> + int rc, i;
> +
> + if (!priv->cpuinfo.valid) {
> + dib_msg.target = PECI_BASE_ADDR + priv->cpu_id;
> +
> + rc = xfer_peci_msg(PECI_IOC_GET_DIB, (void *)&dib_msg);
> + if (rc < 0)
> + return rc;
> +
> + priv->cpuinfo.dib = dib_msg.dib;
> +
> + cfg_msg.target = PECI_BASE_ADDR + priv->cpu_id;
> + cfg_msg.index = MBX_INDEX_CPU_ID;
> + cfg_msg.param = 0;
> + cfg_msg.rx_len = 4;
> +
> + rc = xfer_peci_msg(PECI_IOC_RD_PKG_CFG, (void *)&cfg_msg);
> + if (rc < 0)
> + return rc;
> +
> + priv->cpuinfo.cpuid = cfg_msg.pkg_config[0];
> +
> + cfg_msg.target = PECI_BASE_ADDR + priv->cpu_id;
> + cfg_msg.index = MBX_INDEX_CPU_ID;
> + cfg_msg.param = 1;
> + cfg_msg.rx_len = 4;
> +
> + rc = xfer_peci_msg(PECI_IOC_RD_PKG_CFG, (void *)&cfg_msg);
> + if (rc < 0)
> + return rc;
> +
> + priv->cpuinfo.platform_id = cfg_msg.pkg_config[0];
> +
> + cfg_msg.target = PECI_BASE_ADDR + priv->cpu_id;
> + cfg_msg.index = MBX_INDEX_CPU_ID;
> + cfg_msg.param = 3;
> + cfg_msg.rx_len = 4;
> +
> + rc = xfer_peci_msg(PECI_IOC_RD_PKG_CFG, (void *)&cfg_msg);
> + if (rc < 0)
> + return rc;
> +
> + priv->cpuinfo.logical_thread_nums = cfg_msg.pkg_config[0] + 1;
> +
> + cfg_msg.target = PECI_BASE_ADDR + priv->cpu_id;
> + cfg_msg.index = MBX_INDEX_CPU_ID;
> + cfg_msg.param = 4;
> + cfg_msg.rx_len = 4;
> +
> + rc = xfer_peci_msg(PECI_IOC_RD_PKG_CFG, (void *)&cfg_msg);
> + if (rc < 0)
> + return rc;
> +
> + priv->cpuinfo.microcode = (cfg_msg.pkg_config[3] << 24) |
> + (cfg_msg.pkg_config[2] << 16) |
> + (cfg_msg.pkg_config[1] << 8) |
> + cfg_msg.pkg_config[0];
> +
> + priv->core_nums = priv->cpuinfo.logical_thread_nums / 2;
This seems to assume a 1:2 relationship between number of threads and
number of CPUs, which is incorrect.
> +
> + if (priv->show_core &&
> + atomic_inc_return(&priv->core_group_created) == 1) {
> + for (i = 0; i < priv->core_nums; i++) {
> + rc = create_core_temp_group(priv, i);
This is messy. Sensor groups should be created before or during
hwmon registration, not at some arbitrary later time.
I don't know the logic behind this, but if it is supposed to track CPUs
coming online and going offline it is the wrong approach.
> + if (rc != 0) {
> + dev_err(priv->dev,
> + "Failed to create core temp group\n");
> + for (--i; i >= 0; i--) {
> + sysfs_remove_group(
> + &priv->hwmon_dev->kobj,
> + &priv->core.attr_group[i]);
> + }
> + atomic_set(&priv->core_group_created,
> + 0);
> + return rc;
> + }
> + }
> + }
> +
> + priv->cpuinfo.valid = true;
> + }
> +
> + return 0;
> +}
> +
> +static int get_tjmax(struct peci_hwmon *priv)
> +{
> + struct peci_rd_pkg_cfg_msg msg;
> + int rc;
> +
> + rc = get_cpuinfo(priv);
> + if (rc < 0)
> + return rc;
> +
> + if (!priv->temp.tjmax.valid) {
> + msg.target = PECI_BASE_ADDR + priv->cpu_id;
> + msg.index = MBX_INDEX_TEMP_TARGET;
> + msg.param = 0;
> + msg.rx_len = 4;
> +
> + rc = xfer_peci_msg(PECI_IOC_RD_PKG_CFG, (void *)&msg);
> + if (rc < 0)
> + return rc;
> +
> + priv->temp.tjmax.value = (s32)msg.pkg_config[2] * 1000;
> + priv->temp.tjmax.valid = true;
> + }
> +
> + return 0;
> +}
> +
> +static int get_tcontrol(struct peci_hwmon *priv)
> +{
> + struct peci_rd_pkg_cfg_msg msg;
> + s32 tcontrol_margin;
> + int rc;
> +
> + if (priv->temp.tcontrol.valid &&
> + time_before(jiffies, priv->temp.tcontrol.last_updated +
> + UPDATE_INTERVAL_MIN))
> + return 0;
> +
Is the delay necessary ? Otherwise I would suggest to drop it.
It adds a lot of complexity to the driver. Also, if the user polls
values more often, that is presumably on purpose.
> + rc = get_tjmax(priv);
> + if (rc < 0)
> + return rc;
> +
> + msg.target = PECI_BASE_ADDR + priv->cpu_id;
> + msg.index = MBX_INDEX_TEMP_TARGET;
> + msg.param = 0;
> + msg.rx_len = 4;
> +
> + rc = xfer_peci_msg(PECI_IOC_RD_PKG_CFG, (void *)&msg);
> + if (rc < 0)
> + return rc;
> +
> + tcontrol_margin = msg.pkg_config[1];
> + tcontrol_margin = ((tcontrol_margin ^ 0x80) - 0x80) * 1000;
> +
> + priv->temp.tcontrol.value = priv->temp.tjmax.value - tcontrol_margin;
> +
> + if (!priv->temp.tcontrol.valid) {
> + priv->temp.tcontrol.last_updated = INITIAL_JIFFIES;
> + priv->temp.tcontrol.valid = true;
> + } else {
> + priv->temp.tcontrol.last_updated = jiffies;
> + }
> +
> + return 0;
> +}
> +
> +static int get_tthrottle(struct peci_hwmon *priv)
> +{
> + struct peci_rd_pkg_cfg_msg msg;
> + s32 tthrottle_offset;
> + int rc;
> +
> + if (priv->temp.tthrottle.valid &&
> + time_before(jiffies, priv->temp.tthrottle.last_updated +
> + UPDATE_INTERVAL_MIN))
> + return 0;
> +
> + rc = get_tjmax(priv);
> + if (rc < 0)
> + return rc;
> +
> + msg.target = PECI_BASE_ADDR + priv->cpu_id;
> + msg.index = MBX_INDEX_TEMP_TARGET;
> + msg.param = 0;
> + msg.rx_len = 4;
> +
> + rc = xfer_peci_msg(PECI_IOC_RD_PKG_CFG, (void *)&msg);
> + if (rc < 0)
> + return rc;
> +
> + tthrottle_offset = (msg.pkg_config[3] & 0x2f) * 1000;
> + priv->temp.tthrottle.value = priv->temp.tjmax.value - tthrottle_offset;
> +
> + if (!priv->temp.tthrottle.valid) {
> + priv->temp.tthrottle.last_updated = INITIAL_JIFFIES;
> + priv->temp.tthrottle.valid = true;
> + } else {
> + priv->temp.tthrottle.last_updated = jiffies;
> + }
> +
> + return 0;
> +}
> +
> +static int get_die_temp(struct peci_hwmon *priv)
> +{
> + struct peci_get_temp_msg msg;
> + int rc;
> +
> + if (priv->temp.die.valid &&
> + time_before(jiffies, priv->temp.die.last_updated +
> + UPDATE_INTERVAL_MIN))
> + return 0;
> +
> + rc = get_tjmax(priv);
> + if (rc < 0)
> + return rc;
> +
> + msg.target = PECI_BASE_ADDR + priv->cpu_id;
> +
> + rc = xfer_peci_msg(PECI_IOC_GET_TEMP, (void *)&msg);
> + if (rc < 0)
> + return rc;
> +
> + priv->temp.die.value = priv->temp.tjmax.value +
> + ((s32)msg.temp_raw * 1000 / 64);
> +
> + if (!priv->temp.die.valid) {
> + priv->temp.die.last_updated = INITIAL_JIFFIES;
> + priv->temp.die.valid = true;
> + } else {
> + priv->temp.die.last_updated = jiffies;
> + }
> +
> + return 0;
> +}
> +
> +static int get_dts_margin(struct peci_hwmon *priv)
> +{
> + struct peci_rd_pkg_cfg_msg msg;
> + s32 dts_margin;
> + int rc;
> +
> + if (priv->temp.dts_margin.valid &&
> + time_before(jiffies, priv->temp.dts_margin.last_updated +
> + UPDATE_INTERVAL_MIN))
> + return 0;
> +
Are all those values expected to change dynamically, or are some static ?
Static values do not have to be re-read repeatedly but can be cached
permanently.
> + rc = get_cpuinfo(priv);
> + if (rc < 0)
> + return rc;
> +
> + msg.target = PECI_BASE_ADDR + priv->cpu_id;
> + msg.index = MBX_INDEX_DTS_MARGIN;
> + msg.param = 0;
> + msg.rx_len = 4;
> +
> + rc = xfer_peci_msg(PECI_IOC_RD_PKG_CFG, (void *)&msg);
> + if (rc < 0)
> + return rc;
> +
> + dts_margin = (msg.pkg_config[1] << 8) | msg.pkg_config[0];
> +
> + /*
> + * Processors return a value of DTS reading in 10.6 format
> + * (10 bits signed decimal, 6 bits fractional).
> + * Error codes:
> + * 0x8000: General sensor error
> + * 0x8001: Reserved
> + * 0x8002: Underflow on reading value
> + * 0x8003-0x81ff: Reserved
> + */
> + if (dts_margin >= 0x8000 && dts_margin <= 0x81ff)
> + return -1;
> +
> + dts_margin = ((dts_margin ^ 0x8000) - 0x8000) * 1000 / 64;
> +
The above code is repeated several times. Please consider moving it
into a function to reduce duplication.
> + priv->temp.dts_margin.value = dts_margin;
> +
> + if (!priv->temp.dts_margin.valid) {
> + priv->temp.dts_margin.last_updated = INITIAL_JIFFIES;
> + priv->temp.dts_margin.valid = true;
> + } else {
> + priv->temp.dts_margin.last_updated = jiffies;
> + }
> +
> + return 0;
> +}
> +
> +static int get_core_temp(struct peci_hwmon *priv, int core_index)
> +{
> + struct peci_rd_pkg_cfg_msg msg;
> + s32 core_dts_margin;
> + int rc;
> +
> + if (priv->temp.core[core_index].valid &&
> + time_before(jiffies, priv->temp.core[core_index].last_updated +
> + UPDATE_INTERVAL_MIN))
> + return 0;
> +
> + rc = get_tjmax(priv);
> + if (rc < 0)
> + return rc;
> +
> + msg.target = PECI_BASE_ADDR + priv->cpu_id;
> + msg.index = MBX_INDEX_PER_CORE_DTS_TEMP;
> + msg.param = core_index;
> + msg.rx_len = 4;
> +
> + rc = xfer_peci_msg(PECI_IOC_RD_PKG_CFG, (void *)&msg);
> + if (rc < 0)
> + return rc;
> +
> + core_dts_margin = (msg.pkg_config[1] << 8) | msg.pkg_config[0];
> +
> + /*
> + * Processors return a value of the core DTS reading in 10.6 format
> + * (10 bits signed decimal, 6 bits fractional).
> + * Error codes:
> + * 0x8000: General sensor error
> + * 0x8001: Reserved
> + * 0x8002: Underflow on reading value
> + * 0x8003-0x81ff: Reserved
> + */
> + if (core_dts_margin >= 0x8000 && core_dts_margin <= 0x81ff)
> + return -1;
> +
> + core_dts_margin = ((core_dts_margin ^ 0x8000) - 0x8000) * 1000 / 64;
> +
> + priv->temp.core[core_index].value = priv->temp.tjmax.value +
> + core_dts_margin;
> +
> + if (!priv->temp.core[core_index].valid) {
> + priv->temp.core[core_index].last_updated = INITIAL_JIFFIES;
> + priv->temp.core[core_index].valid = true;
> + } else {
> + priv->temp.core[core_index].last_updated = jiffies;
> + }
> +
> + return 0;
> +}
> +
> +static int get_dimm_temp(struct peci_hwmon *priv, int dimm_index)
> +{
> + struct peci_rd_pkg_cfg_msg msg;
> + int channel_rank = dimm_index / 2;
> + int dimm_order = dimm_index % 2;
> + int rc;
> +
> + if (priv->temp.core[dimm_index].valid &&
> + time_before(jiffies, priv->temp.core[dimm_index].last_updated +
> + UPDATE_INTERVAL_MIN))
> + return 0;
> +
> + rc = get_cpuinfo(priv);
> + if (rc < 0)
> + return rc;
> +
> + msg.target = PECI_BASE_ADDR + priv->cpu_id;
> + msg.index = MBX_INDEX_DDR_DIMM_TEMP;
> + msg.param = channel_rank;
> + msg.rx_len = 4;
> +
> + rc = xfer_peci_msg(PECI_IOC_RD_PKG_CFG, (void *)&msg);
> + if (rc < 0)
> + return rc;
> +
> + priv->temp.dimm[dimm_index].value = msg.pkg_config[dimm_order] * 1000;
> +
> + if (!priv->temp.dimm[dimm_index].valid) {
> + priv->temp.dimm[dimm_index].last_updated = INITIAL_JIFFIES;
> + priv->temp.dimm[dimm_index].valid = true;
> + } else {
> + priv->temp.dimm[dimm_index].last_updated = jiffies;
> + }
> +
> + return 0;
> +}
> +
> +static ssize_t show_info(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct peci_hwmon *priv = dev_get_drvdata(dev);
> + int rc;
> +
> + rc = get_cpuinfo(priv);
> + if (rc < 0)
> + return rc;
> +
> + return sprintf(buf, "dib : 0x%08x\n"
> + "cpuid : 0x%x\n"
> + "platform id : %d\n"
> + "stepping : %d\n"
> + "microcode : 0x%08x\n"
> + "logical thread nums : %d\n",
> + priv->cpuinfo.dib,
> + priv->cpuinfo.cpuid,
> + priv->cpuinfo.platform_id,
> + priv->cpuinfo.cpuid & 0xf,
> + priv->cpuinfo.microcode,
> + priv->cpuinfo.logical_thread_nums);
> +}
Please no non-standard attributes, much less attributes not following sysfs
attribute rules. If you want to display such information, consider using
debugfs. Besides, this information specifically appears to duplicate
the content of /proc/cpuid, which doesn't really add any value at all.
> +
> +static ssize_t show_tcontrol(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct peci_hwmon *priv = dev_get_drvdata(dev);
> + int rc;
> +
> + rc = get_tcontrol(priv);
> + if (rc < 0)
> + return rc;
> +
> + return sprintf(buf, "%d\n", priv->temp.tcontrol.value);
> +}
> +
> +static ssize_t show_tcontrol_margin(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct peci_hwmon *priv = dev_get_drvdata(dev);
> + struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> + int rc;
> +
> + rc = get_tcontrol(priv);
> + if (rc < 0)
> + return rc;
> +
> + return sprintf(buf, "%d\n", sensor_attr->index == POS ?
> + priv->temp.tjmax.value -
> + priv->temp.tcontrol.value :
> + priv->temp.tcontrol.value -
> + priv->temp.tjmax.value);
> +}
> +
> +static ssize_t show_tthrottle(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct peci_hwmon *priv = dev_get_drvdata(dev);
> + int rc;
> +
> + rc = get_tthrottle(priv);
> + if (rc < 0)
> + return rc;
> +
> + return sprintf(buf, "%d\n", priv->temp.tthrottle.value);
> +}
> +
> +static ssize_t show_tjmax(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct peci_hwmon *priv = dev_get_drvdata(dev);
> + int rc;
> +
> + rc = get_tjmax(priv);
> + if (rc < 0)
> + return rc;
> +
> + return sprintf(buf, "%d\n", priv->temp.tjmax.value);
> +}
> +
> +static ssize_t show_die_temp(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct peci_hwmon *priv = dev_get_drvdata(dev);
> + int rc;
> +
> + rc = get_die_temp(priv);
> + if (rc < 0)
> + return rc;
> +
> + return sprintf(buf, "%d\n", priv->temp.die.value);
> +}
> +
> +static ssize_t show_dts_therm_margin(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct peci_hwmon *priv = dev_get_drvdata(dev);
> + int rc;
> +
> + rc = get_dts_margin(priv);
> + if (rc < 0)
> + return rc;
> +
> + return sprintf(buf, "%d\n", priv->temp.dts_margin.value);
> +}
> +
> +static ssize_t show_core_temp(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct peci_hwmon *priv = dev_get_drvdata(dev);
> + struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> + int core_index = sensor_attr->index;
> + int rc;
> +
> + rc = get_core_temp(priv, core_index);
> + if (rc < 0)
> + return rc;
> +
> + return sprintf(buf, "%d\n", priv->temp.core[core_index].value);
> +}
> +
> +static ssize_t show_dimm_temp(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct peci_hwmon *priv = dev_get_drvdata(dev);
> + struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> + int dimm_index = sensor_attr->index;
> + int rc;
> +
> + rc = get_dimm_temp(priv, dimm_index);
> + if (rc < 0)
> + return rc;
> +
> + return sprintf(buf, "%d\n", priv->temp.dimm[dimm_index].value);
> +}
> +
> +static ssize_t show_value(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> +
> + return sprintf(buf, "%d\n", sensor_attr->index);
> +}
> +
> +static ssize_t show_label(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> +
> + return sprintf(buf, peci_label[sensor_attr->index]);
> +}
> +
> +static ssize_t show_core_label(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> +
> + return sprintf(buf, "Core #%d temperature\n", sensor_attr->index);
> +}
Your label strings are quite long. How does that look like with the
sensors command ?
Plus, again, "temperature" in a temperature label is redundant.
> +
> +static ssize_t show_dimm_label(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> +
> + char channel = 'A' + (sensor_attr->index / 2);
> + int index = sensor_attr->index % 2;
> +
> + return sprintf(buf, "Channel Rank %c DDR DIMM #%d temperature\n",
> + channel, index);
> +}
> +
> +/* Die temperature */
> +static SENSOR_DEVICE_ATTR(temp1_label, 0444, show_label, NULL, L_DIE);
> +static SENSOR_DEVICE_ATTR(temp1_input, 0444, show_die_temp, NULL, 0);
> +static SENSOR_DEVICE_ATTR(temp1_max, 0444, show_tcontrol, NULL, 0);
> +static SENSOR_DEVICE_ATTR(temp1_crit, 0444, show_tjmax, NULL, 0);
> +static SENSOR_DEVICE_ATTR(temp1_crit_hyst, 0444, show_tcontrol_margin, NULL,
> + POS);
> +
> +static struct attribute *die_temp_attrs[] = {
> + &sensor_dev_attr_temp1_label.dev_attr.attr,
> + &sensor_dev_attr_temp1_input.dev_attr.attr,
> + &sensor_dev_attr_temp1_max.dev_attr.attr,
> + &sensor_dev_attr_temp1_crit.dev_attr.attr,
> + &sensor_dev_attr_temp1_crit_hyst.dev_attr.attr,
> + NULL
> +};
> +
> +static const struct attribute_group die_temp_attr_group = {
> + .attrs = die_temp_attrs,
> +};
> +
> +/* DTS thermal margin temperature */
> +static SENSOR_DEVICE_ATTR(temp2_label, 0444, show_label, NULL, L_DTS);
> +static SENSOR_DEVICE_ATTR(temp2_input, 0444, show_dts_therm_margin, NULL, 0);
> +static SENSOR_DEVICE_ATTR(temp2_min, 0444, show_value, NULL, 0);
> +static SENSOR_DEVICE_ATTR(temp2_lcrit, 0444, show_tcontrol_margin, NULL, NEG);
> +
> +static struct attribute *dts_margin_temp_attrs[] = {
> + &sensor_dev_attr_temp2_label.dev_attr.attr,
> + &sensor_dev_attr_temp2_input.dev_attr.attr,
> + &sensor_dev_attr_temp2_min.dev_attr.attr,
> + &sensor_dev_attr_temp2_lcrit.dev_attr.attr,
> + NULL
> +};
> +
> +static const struct attribute_group dts_margin_temp_attr_group = {
> + .attrs = dts_margin_temp_attrs,
> +};
> +
> +/* Tcontrol temperature */
> +static SENSOR_DEVICE_ATTR(temp3_label, 0444, show_label, NULL, L_TCONTROL);
> +static SENSOR_DEVICE_ATTR(temp3_input, 0444, show_tcontrol, NULL, 0);
> +static SENSOR_DEVICE_ATTR(temp3_crit, 0444, show_tjmax, NULL, 0);
> +
> +static struct attribute *tcontrol_temp_attrs[] = {
> + &sensor_dev_attr_temp3_label.dev_attr.attr,
> + &sensor_dev_attr_temp3_input.dev_attr.attr,
> + &sensor_dev_attr_temp3_crit.dev_attr.attr,
> + NULL
> +};
> +
> +static const struct attribute_group tcontrol_temp_attr_group = {
> + .attrs = tcontrol_temp_attrs,
> +};
> +
> +/* Tthrottle temperature */
> +static SENSOR_DEVICE_ATTR(temp4_label, 0444, show_label, NULL, L_TTHROTTLE);
> +static SENSOR_DEVICE_ATTR(temp4_input, 0444, show_tthrottle, NULL, 0);
> +
> +static struct attribute *tthrottle_temp_attrs[] = {
> + &sensor_dev_attr_temp4_label.dev_attr.attr,
> + &sensor_dev_attr_temp4_input.dev_attr.attr,
> + NULL
> +};
> +
> +static const struct attribute_group tthrottle_temp_attr_group = {
> + .attrs = tthrottle_temp_attrs,
> +};
> +
> +/* CPU info */
> +static SENSOR_DEVICE_ATTR(info, 0444, show_info, NULL, 0);
> +
> +static struct attribute *info_attrs[] = {
> + &sensor_dev_attr_info.dev_attr.attr,
> + NULL
> +};
> +
> +static const struct attribute_group info_attr_group = {
> + .attrs = info_attrs,
> +};
> +
> +const struct attribute_group *peci_hwmon_attr_groups[] = {
> + &info_attr_group,
> + &die_temp_attr_group,
> + &dts_margin_temp_attr_group,
> + &tcontrol_temp_attr_group,
> + &tthrottle_temp_attr_group,
> + NULL
> +};
> +
> +static ssize_t (*const core_show_fn[CORE_TEMP_ATTRS]) (struct device *dev,
> + struct device_attribute *devattr, char *buf) = {
> + show_core_label,
> + show_core_temp,
> + show_tcontrol,
> + show_tjmax,
> + show_tcontrol_margin,
> +};
> +
> +static const char *const core_suffix[CORE_TEMP_ATTRS] = {
> + "label",
> + "input",
> + "max",
> + "crit",
> + "crit_hyst",
> +};
> +
> +static int create_core_temp_group(struct peci_hwmon *priv, int core_no)
> +{
> + int i;
> +
> + for (i = 0; i < CORE_TEMP_ATTRS; i++) {
> + snprintf(priv->core.attr_name[core_no][i],
> + ATTR_NAME_LEN, "temp%d_%s",
> + CORE_INDEX_OFFSET + core_no, core_suffix[i]);
> + sysfs_attr_init(
> + &priv->core.sd_attrs[core_no][i].dev_attr.attr);
> + priv->core.sd_attrs[core_no][i].dev_attr.attr.name =
> + priv->core.attr_name[core_no][i];
> + priv->core.sd_attrs[core_no][i].dev_attr.attr.mode = 0444;
> + priv->core.sd_attrs[core_no][i].dev_attr.show = core_show_fn[i];
> + if (i == 0 || i == 1) /* label or temp */
> + priv->core.sd_attrs[core_no][i].index = core_no;
> + priv->core.attrs[core_no][i] =
> + &priv->core.sd_attrs[core_no][i].dev_attr.attr;
> + }
> +
> + priv->core.attr_group[core_no].attrs = priv->core.attrs[core_no];
> +
> + return sysfs_create_group(&priv->hwmon_dev->kobj,
> + &priv->core.attr_group[core_no]);
> +}
> +
> +static ssize_t (*const dimm_show_fn[DIMM_TEMP_ATTRS]) (struct device *dev,
> + struct device_attribute *devattr, char *buf) = {
> + show_dimm_label,
> + show_dimm_temp,
> +};
> +
> +static const char *const dimm_suffix[DIMM_TEMP_ATTRS] = {
> + "label",
> + "input",
> +};
> +
> +static int create_dimm_temp_group(struct peci_hwmon *priv, int dimm_no)
> +{
> + int i;
> +
> + for (i = 0; i < DIMM_TEMP_ATTRS; i++) {
> + snprintf(priv->dimm.attr_name[dimm_no][i],
> + ATTR_NAME_LEN, "temp%d_%s",
> + DIMM_INDEX_OFFSET + dimm_no, dimm_suffix[i]);
> + sysfs_attr_init(&priv->dimm.sd_attrs[dimm_no][i].dev_attr.attr);
> + priv->dimm.sd_attrs[dimm_no][i].dev_attr.attr.name =
> + priv->dimm.attr_name[dimm_no][i];
> + priv->dimm.sd_attrs[dimm_no][i].dev_attr.attr.mode = 0444;
> + priv->dimm.sd_attrs[dimm_no][i].dev_attr.show = dimm_show_fn[i];
> + priv->dimm.sd_attrs[dimm_no][i].index = dimm_no;
> + priv->dimm.attrs[dimm_no][i] =
> + &priv->dimm.sd_attrs[dimm_no][i].dev_attr.attr;
> + }
> +
> + priv->dimm.attr_group[dimm_no].attrs = priv->dimm.attrs[dimm_no];
> +
> + return sysfs_create_group(&priv->hwmon_dev->kobj,
> + &priv->dimm.attr_group[dimm_no]);
> +}
> +
> +static int peci_hwmon_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *np = dev->of_node;
> + struct peci_hwmon *priv;
> + struct device *hwmon;
> + int rc, i;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + dev_set_drvdata(dev, priv);
> + priv->dev = dev;
> +
> + rc = of_property_read_u32(np, "cpu-id", &priv->cpu_id);
What entity determines cpu-id ?
> + if (rc || priv->cpu_id >= CPU_ID_MAX) {
> + dev_err(dev, "Invalid cpu-id configuration\n");
> + return rc;
> + }
> +
> + rc = of_property_read_u32(np, "dimm-nums", &priv->dimm_nums);
This is an odd devicetree attribute. Normally the number of DIMMs
is dynamic. Isn't there a means to get all that information dynamically
instead of having to set it through devicetree ? What if someone adds
or removes a DIMM ? Who updates the devicetree ?
> + if (rc || priv->dimm_nums > DIMM_NUMS_MAX) {
> + dev_warn(dev, "Invalid dimm-nums : %u. Use default : %u\n",
> + priv->dimm_nums, OF_DIMM_NUMS_DEFAULT);
> + priv->dimm_nums = OF_DIMM_NUMS_DEFAULT;
> + }
> +
> + priv->show_core = of_property_read_bool(np, "show-core");
This does not look like an appropriate devicetree attribute.
> +
> + priv->groups = peci_hwmon_attr_groups;
> +
This assignment (and the ->groups variable) is quite pointless.
> + snprintf(priv->name, NAME_MAX, HWMON_NAME ".cpu%d", priv->cpu_id);
> +
> + hwmon = devm_hwmon_device_register_with_groups(dev,
> + priv->name,
> + priv, priv->groups);
Please rewrite the driver to use devm_hwmon_device_register_with_info(),
and avoid dynamic attributes.
> +
> + rc = PTR_ERR_OR_ZERO(hwmon);
> + if (rc != 0) {
> + dev_err(dev, "Failed to register peci hwmon\n");
> + return rc;
> + }
> +
> + priv->hwmon_dev = hwmon;
Something is logically wrong if you need to store hwmon_dev in the
private data structure. Specifically, creating attributes dynamically
after hwmon registration is wrong.
> +
> + for (i = 0; i < priv->dimm_nums; i++) {
> + rc = create_dimm_temp_group(priv, i);
No. See earlier comments. All attribute groups must be created during
registration (or before, but I am not inclined to accept a new driver
doing that).
> + if (rc != 0) {
> + dev_err(dev, "Failed to create dimm temp group\n");
> + for (--i; i >= 0; i--) {
> + sysfs_remove_group(&priv->hwmon_dev->kobj,
> + &priv->dimm.attr_group[i]);
> + }
> + return rc;
> + }
> + }
> +
> + /*
> + * Try to create core temp group now. It will be created if CPU is
> + * curretnly online or it will be created after the first reading of
> + * cpuinfo from the online CPU otherwise.
This is not how CPUs are supposed to be detected, and it does not handle CPUs
taken offline. If the driver is instantiated as a CPU comes online, or as it
goes offline, the driver should use the appropriate kernel interfaces to
trigger that instantiation or removal. However, if so, it may be inappropriate
to associate CPU temperatures with other system temperatures in the same
instance of the driver; after all, those are all independent of each other.
Overall, I suspect that there should be a callback or some other mechanism
in the peci core to trigger instantiation and removal of this driver, and
I am not sure if any of the devicetree properties makes sense at all.
For example, if an instance of this driver is associated with a PECI
agent (with assorted CPU/DIMM temperature reporting), the instantiation
could be triggered as soon as the PECI core detects that the agent is
available, and the PECI core could report what exactly that instance
supports.
> + */
> + if (priv->show_core)
> + (void) get_cpuinfo(priv);
> +
> + dev_info(dev, "peci hwmon for CPU#%d registered\n", priv->cpu_id);
Is this logging noise necessary ? Besides, some of it is redundant.
> +
> + return rc;
> +}
> +
> +static int peci_hwmon_remove(struct platform_device *pdev)
> +{
> + struct peci_hwmon *priv = dev_get_drvdata(&pdev->dev);
> + int i;
> +
> + if (atomic_read(&priv->core_group_created))
> + for (i = 0; i < priv->core_nums; i++) {
> + sysfs_remove_group(&priv->hwmon_dev->kobj,
> + &priv->core.attr_group[i]);
> + }
> +
> + for (i = 0; i < priv->dimm_nums; i++) {
> + sysfs_remove_group(&priv->hwmon_dev->kobj,
> + &priv->dimm.attr_group[i]);
> + }
If you need to call sysfs_remove_group from here,
something is conceptually wrong in your driver.
> +
> + return 0;
> +}
> +
> +static const struct of_device_id peci_of_table[] = {
> + { .compatible = "peci-hwmon", },
This does not look like a reference to some piece of hardware.
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, peci_of_table);
> +
> +static struct platform_driver peci_hwmon_driver = {
> + .probe = peci_hwmon_probe,
> + .remove = peci_hwmon_remove,
> + .driver = {
> + .name = DEVICE_NAME,
> + .of_match_table = peci_of_table,
> + },
> +};
> +
> +module_platform_driver(peci_hwmon_driver);
> +MODULE_AUTHOR("Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>");
> +MODULE_DESCRIPTION("PECI hwmon driver");
> +MODULE_LICENSE("GPL v2");
^ permalink raw reply
* [PATCH linux dev-4.10 0/6] Add support PECI and PECI hwmon drivers
From: Jae Hyun Yoo @ 2018-01-10 21:46 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180110202740.GA27703@kroah.com>
On 1/10/2018 12:27 PM, Greg KH wrote:
> On Wed, Jan 10, 2018 at 11:30:05AM -0800, Jae Hyun Yoo wrote:
>> On 1/10/2018 11:17 AM, Greg KH wrote:
>>> On Wed, Jan 10, 2018 at 11:14:34AM -0800, Jae Hyun Yoo wrote:
>>>> On 1/10/2018 2:17 AM, Greg KH wrote:
>>>>> On Tue, Jan 09, 2018 at 02:31:20PM -0800, Jae Hyun Yoo wrote:
>>>>>> From: Jae Hyun Yoo <jae.hyun.yoo@intel.com>
>>>>>>
>>>>>> Hello,
>>>>>>
>>>>>> This patch set provides support for PECI of AST2400/2500 which can give us PECI
>>>>>> functionalities such as temperature monitoring, platform manageability,
>>>>>> processor diagnostics and failure analysis. Also provides generic peci.h and
>>>>>> peci_ioctl.h headers to provide compatibility to peci drivers that can be
>>>>>> implemented later e.g. Nuvoton's BMC SoC family.
>>>>>
>>>>> What is the "dev-4.10" in the subject for? 4.10 is really old and
>>>>> obsolete :(
>>>>>
>>>>> thanks,
>>>>>
>>>>> greg k-h
>>>>>
>>>>
>>>> I made this patch set on top of the v4.10 which OpenBmc project is currently
>>>> using. I'll rebase this patch set onto the current kernel.org mainline.
>>>
>>> What is "OpenBmc", and why are they using an obsolete and insecure
>>> kernel for their project? That seems like a very foolish thing to do...
>>>
>>> thanks,
>>>
>>> greg k-h
>>>
>>
>> OpenBmc is an open source project to create a highly extensible framework
>> for BMC (Board Management Controller) software for data-center computer
>> systems:
>> https://github.com/openbmc
>>
>> Its current mainline is v4.10 but it is being kept upgrading so it will be
>> upgraded to the latest stable or long-term version soon.
>
> Why hasn't it been updated in the year since 4.10 was released? That's
> a _very_ long time to be running on a totally insecure kernel, and no
> new development should ever be done on old kernels, that's even crazier
> (as we can't go back in time and accept patches for new features to old
> releases...)
>
Thanks for your pointing it out and I totally agree with you. Actually,
we are preparing 4.13 update for now and an another update will be
followed up. As I answered above, I'll rebase this patch set onto the
latest kernel.org mainline. Sorry for my misunderstanding of upstream
process.
> It sounds like the openbmc project needs to learn how to manage their
> kernels a whole lot better, who do I need to go poke about this?
> > thanks,
>
> greg k-h
>
I've already cc'ed openbmc developers so they are also seeing this
thread. Joel Stanley <joel@jms.id.au> is the openbmc kernel maintainer.
Thanks,
Jae
^ permalink raw reply
* [PATCH -next] clk: meson-axg: fix potential NULL dereference in axg_clkc_probe()
From: Stephen Boyd @ 2018-01-10 21:24 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1515117059-176004-1-git-send-email-weiyongjun1@huawei.com>
On 01/05, Wei Yongjun wrote:
> platform_get_resource() may return NULL, add proper
> check to avoid potential NULL dereferencing.
>
> This is detected by Coccinelle semantic patch.
>
> @@
> expression pdev, res, n, t, e, e1, e2;
> @@
>
> res = platform_get_resource(pdev, t, n);
> + if (!res)
> + return -EINVAL;
> ... when != res == NULL
> e = devm_ioremap(e1, res->start, e2);
>
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
> ---
Applied to clk-next
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* [PATCH -next] clk: meson-axg: fix potential NULL dereference in axg_clkc_probe()
From: Stephen Boyd @ 2018-01-10 21:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1515117059-176004-1-git-send-email-weiyongjun1@huawei.com>
On 01/05, Wei Yongjun wrote:
> platform_get_resource() may return NULL, add proper
> check to avoid potential NULL dereferencing.
>
> This is detected by Coccinelle semantic patch.
>
> @@
> expression pdev, res, n, t, e, e1, e2;
> @@
>
> res = platform_get_resource(pdev, t, n);
> + if (!res)
> + return -EINVAL;
> ... when != res == NULL
> e = devm_ioremap(e1, res->start, e2);
Can this script be put into scripts/coccinelle/? I'd like to be
able to run it instead of getting emails from you after patches
merge.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox