| Message ID | 20200809102511.2657644-3-Sandeep.Singh@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SFH: Add Support for AMD Sensor Fusion Hub | expand |
Hi Sandeep,
I love your patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v5.8 next-20200807]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Sandeep-Singh/SFH-Add-Support-for-AMD-Sensor-Fusion-Hub/20200809-182729
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 06a81c1c7db9bd5de0bd38cd5acc44bb22b99150
config: riscv-allyesconfig (attached as .config)
compiler: riscv64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=riscv
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/hid/amd-sfh-hid/amd_mp2_pcie.c: In function 'amd_mp2_pci_init':
>> drivers/hid/amd-sfh-hid/amd_mp2_pcie.c:107:2: warning: ignoring return value of 'pcim_enable_device', declared with attribute warn_unused_result [-Wunused-result]
107 | pcim_enable_device(pdev);
| ^~~~~~~~~~~~~~~~~~~~~~~~
vim +/pcim_enable_device +107 drivers/hid/amd-sfh-hid/amd_mp2_pcie.c
101
102 static int amd_mp2_pci_init(struct amd_mp2_dev *privdata, struct pci_dev *pdev)
103 {
104 int rc;
105
106 pci_set_drvdata(pdev, privdata);
> 107 pcim_enable_device(pdev);
108 pcim_iomap_regions(pdev, BIT(2), DRIVER_NAME);
109
110 privdata->mmio = pcim_iomap_table(pdev)[2];
111 pci_set_master(pdev);
112
113 rc = pci_set_dma_mask(pdev, DMA_BIT_MASK(64));
114 if (rc)
115 rc = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
116 return rc;
117 }
118
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On 8/9/20 3:25 AM, Sandeep Singh wrote: > From: Sandeep Singh <sandeep.singh@amd.com> > > AMD SFH uses HID over PCIe bus.SFH fw is part of MP2 processor > (MP2 which is an ARM® Cortex-M4 core based co-processor to x86) and > it runs on MP2 where in driver resides on X86. This part of module > will communicate with MP2 FW and provide that data into DRAM > > Signed-off-by: Nehal Shah <Nehal-bakulchandra.Shah@amd.com> > Signed-off-by: Sandeep Singh <sandeep.singh@amd.com> > --- > drivers/hid/Kconfig | 2 + > drivers/hid/Makefile | 2 + > drivers/hid/amd-sfh-hid/Kconfig | 21 ++++ > drivers/hid/amd-sfh-hid/Makefile | 15 +++ > drivers/hid/amd-sfh-hid/amd_mp2_pcie.c | 162 +++++++++++++++++++++++++ > drivers/hid/amd-sfh-hid/amd_mp2_pcie.h | 83 +++++++++++++ > 6 files changed, 285 insertions(+) > create mode 100644 drivers/hid/amd-sfh-hid/Kconfig > create mode 100644 drivers/hid/amd-sfh-hid/Makefile > create mode 100644 drivers/hid/amd-sfh-hid/amd_mp2_pcie.c > create mode 100644 drivers/hid/amd-sfh-hid/amd_mp2_pcie.h Hi, > diff --git a/drivers/hid/amd-sfh-hid/Kconfig b/drivers/hid/amd-sfh-hid/Kconfig > new file mode 100644 > index 000000000000..e73cf9fe1324 > --- /dev/null > +++ b/drivers/hid/amd-sfh-hid/Kconfig > @@ -0,0 +1,21 @@ > +# SPDX-License-Identifier: GPL-2.0-or-later > +menu "AMD SFH HID support" > + depends on X86_64 || COMPILE_TEST > + depends on PCI > + > +config AMD_SFH_HID > + tristate "AMD Sensor Fusion Hub" > + select HID How about depends on HID We try hard not to select/enable entire subsystems just because one driver wants it. Also, HID depends on INPUT, so it's not safe to select HID unless INPUT is already enabled. > + help > + If you say yes to this option, support will be included for the AMD > + Sensor Fusion Hub. > + This driver will enable sensors functionality to user through HID > + framework. Basically this driver will get data from MP2 FW s/FW/firmware/ or is it "framework" ? > + and provide that data to HID framework. > + MP2 which is an ARM® Cortex-M4 core based co-processor to x86. > + > + This driver can also be built as modules. If so, the modules will built as a module. If so, the module will > + be called amd-sfhtp-hid. > + Say Y or M here if you want to support AMD SFH. If unsure, say N. > + > +endmenu thanks.
On Sun, Aug 9, 2020 at 1:25 PM Sandeep Singh <Sandeep.Singh@amd.com> wrote: > AMD SFH uses HID over PCIe bus.SFH fw is part of MP2 processor > (MP2 which is an ARM® Cortex-M4 core based co-processor to x86) and > it runs on MP2 where in driver resides on X86. This part of module where the driver > will communicate with MP2 FW and provide that data into DRAM ... > +# > +# One is enough. ... > +#define ACEL_EN BIT(accel_idx) > +#define GYRO_EN BIT(gyro_idx) > +#define MAGNO_EN BIT(mag_idx) > +#define ALS_EN BIT(als_idx) What is this? ... > +int amd_mp2_get_sensor_num(struct amd_mp2_dev *privdata, u8 *sensor_id) > +{ > + int activestatus, num_of_sensors = 0; > + > + if (!sensor_id) > + return -EINVAL; Is it possible? > + privdata->activecontrolstatus = readl(privdata->mmio + AMD_P2C_MSG3); > + activestatus = privdata->activecontrolstatus >> 4; > + if (ACEL_EN & activestatus) > + sensor_id[num_of_sensors++] = accel_idx; > + > + if (GYRO_EN & activestatus) > + sensor_id[num_of_sensors++] = gyro_idx; > + > + if (MAGNO_EN & activestatus) > + sensor_id[num_of_sensors++] = mag_idx; > + > + if (ALS_EN & activestatus) > + sensor_id[num_of_sensors++] = als_idx; > + > + return num_of_sensors; > +} ... > +static int amd_mp2_pci_init(struct amd_mp2_dev *privdata, struct pci_dev *pdev) > +{ > + int rc; > + > + pci_set_drvdata(pdev, privdata); This is better to have after initial resources were retrieved. > + pcim_enable_device(pdev); > + pcim_iomap_regions(pdev, BIT(2), DRIVER_NAME); Where is the error check? > + privdata->mmio = pcim_iomap_table(pdev)[2]; > + pci_set_master(pdev); > + > + rc = pci_set_dma_mask(pdev, DMA_BIT_MASK(64)); > + if (rc) > + rc = pci_set_dma_mask(pdev, DMA_BIT_MASK(32)); > + return rc; > +} What is the point to have this function separated from ->probe()? ... > + rc = amd_sfh_hid_client_init(privdata); > + if (rc) > + return rc; > + return 0; return amd_...(...); ... > +static const struct pci_device_id amd_mp2_pci_tbl[] = { > + { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_MP2) }, > + {}, No comma. > +}; ... > +#include <linux/pci.h> I don't see any users of it in the file. Use forward declaration instead. > +#include <linux/types.h> ... > +enum command_id { > + enable_sensor = 1, > + disable_sensor = 2, > + stop_all_sensors = 8, > + invalid_cmd = 0xf GENMASK()? (Will require bits.h) > +}; > + > +enum sensor_idx { > + accel_idx = 0, > + gyro_idx = 1, > + mag_idx = 2, > + als_idx = 19 + comma. > +}; > + > +struct amd_mp2_dev { > + struct pci_dev *pdev; > + struct amdtp_cl_data *cl_data; > + void __iomem *mmio; Is __iomem provided by linux/types.h? Otherwise include corresponding header. > + u32 activecontrolstatus; > +};
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig index 45e87dc59d4e..c0a1c07ed6b1 100644 --- a/drivers/hid/Kconfig +++ b/drivers/hid/Kconfig @@ -1174,4 +1174,6 @@ source "drivers/hid/i2c-hid/Kconfig" source "drivers/hid/intel-ish-hid/Kconfig" +source "drivers/hid/amd-sfh-hid/Kconfig" + endmenu diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile index d8ea4b8c95af..7d8ca4e34572 100644 --- a/drivers/hid/Makefile +++ b/drivers/hid/Makefile @@ -141,3 +141,5 @@ obj-$(CONFIG_I2C_HID) += i2c-hid/ obj-$(CONFIG_INTEL_ISH_HID) += intel-ish-hid/ obj-$(INTEL_ISH_FIRMWARE_DOWNLOADER) += intel-ish-hid/ + +obj-$(CONFIG_AMD_SFH_HID) += amd-sfh-hid/ diff --git a/drivers/hid/amd-sfh-hid/Kconfig b/drivers/hid/amd-sfh-hid/Kconfig new file mode 100644 index 000000000000..e73cf9fe1324 --- /dev/null +++ b/drivers/hid/amd-sfh-hid/Kconfig @@ -0,0 +1,21 @@ +# SPDX-License-Identifier: GPL-2.0-or-later +menu "AMD SFH HID support" + depends on X86_64 || COMPILE_TEST + depends on PCI + +config AMD_SFH_HID + tristate "AMD Sensor Fusion Hub" + select HID + help + If you say yes to this option, support will be included for the AMD + Sensor Fusion Hub. + This driver will enable sensors functionality to user through HID + framework. Basically this driver will get data from MP2 FW + and provide that data to HID framework. + MP2 which is an ARM® Cortex-M4 core based co-processor to x86. + + This driver can also be built as modules. If so, the modules will + be called amd-sfhtp-hid. + Say Y or M here if you want to support AMD SFH. If unsure, say N. + +endmenu diff --git a/drivers/hid/amd-sfh-hid/Makefile b/drivers/hid/amd-sfh-hid/Makefile new file mode 100644 index 000000000000..a163c7f62b32 --- /dev/null +++ b/drivers/hid/amd-sfh-hid/Makefile @@ -0,0 +1,15 @@ +# SPDX-License-Identifier: GPL-2.0-or-later +# +# Makefile - AMD SFH HID drivers +# Copyright (c) 2019-2020, Advanced Micro Devices, Inc. +# +# + +ccflags-m := -Werror + obj-$(CONFIG_AMD_SFH_HID) +=amd-sfhtp-hid.o + amd-sfhtp-hid-objs := amdsfh_hid.o + amd-sfhtp-hid-objs+= amdsfh_hid_client.o + amd-sfhtp-hid-objs+= amd_mp2_pcie.o + amd-sfhtp-hid-objs+= hid_descriptor/amd_sfh_hid_descriptor.o + +ccflags-y += -I$(srctree)/$(src)/ diff --git a/drivers/hid/amd-sfh-hid/amd_mp2_pcie.c b/drivers/hid/amd-sfh-hid/amd_mp2_pcie.c new file mode 100644 index 000000000000..cdd480c287db --- /dev/null +++ b/drivers/hid/amd-sfh-hid/amd_mp2_pcie.c @@ -0,0 +1,162 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * AMD MP2 PCIe communication driver + * Copyright 2020 Advanced Micro Devices, Inc. + * + * Authors: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> + * Sandeep Singh <Sandeep.singh@amd.com> + */ + +#include <linux/bitops.h> +#include <linux/delay.h> +#include <linux/dma-mapping.h> +#include <linux/interrupt.h> +#include <linux/io-64-nonatomic-lo-hi.h> +#include <linux/module.h> +#include <linux/pci.h> +#include <linux/slab.h> +#include <linux/delay.h> +#include <linux/types.h> +#include "amd_mp2_pcie.h" + +#define DRIVER_NAME "pcie_mp2_amd" +#define DRIVER_DESC "AMD(R) PCIe MP2 Communication Driver" + +#define ACEL_EN BIT(accel_idx) +#define GYRO_EN BIT(gyro_idx) +#define MAGNO_EN BIT(mag_idx) +#define ALS_EN BIT(als_idx) + +void amd_start_sensor(struct amd_mp2_dev *privdata, struct amd_mp2_sensor_info info) +{ + union sfh_cmd_param cmd_param; + union sfh_cmd_base cmd_base; + + /* fill up command register */ + cmd_base.ul = 0; + cmd_base.s.cmd_id = enable_sensor; + cmd_base.s.period = info.period; + cmd_base.s.sensor_id = info.sensor_idx; + + /* fill up command param register */ + cmd_param.ul = 0; + cmd_param.s.buf_layout = 1; + cmd_param.s.buf_length = 16; + + writeq(info.phys_address, privdata->mmio + AMD_C2P_MSG2); + writel(cmd_param.ul, privdata->mmio + AMD_C2P_MSG1); + writel(cmd_base.ul, privdata->mmio + AMD_C2P_MSG0); +} + +void amd_stop_sensor(struct amd_mp2_dev *privdata, u16 sensor_idx) +{ + union sfh_cmd_base cmd_base; + + /* fill up command register */ + cmd_base.ul = 0; + cmd_base.s.cmd_id = disable_sensor; + cmd_base.s.period = 0; + cmd_base.s.sensor_id = sensor_idx; + + writeq(0x0, privdata->mmio + AMD_C2P_MSG2); + writel(cmd_base.ul, privdata->mmio + AMD_C2P_MSG0); +} + +void amd_stop_all_sensors(struct amd_mp2_dev *privdata) +{ + union sfh_cmd_base cmd_base; + + /* fill up command register */ + cmd_base.ul = 0; + cmd_base.s.cmd_id = stop_all_sensors; + cmd_base.s.period = 0; + cmd_base.s.sensor_id = 0; + + writel(cmd_base.ul, privdata->mmio + AMD_C2P_MSG0); +} + +int amd_mp2_get_sensor_num(struct amd_mp2_dev *privdata, u8 *sensor_id) +{ + int activestatus, num_of_sensors = 0; + + if (!sensor_id) + return -EINVAL; + + privdata->activecontrolstatus = readl(privdata->mmio + AMD_P2C_MSG3); + activestatus = privdata->activecontrolstatus >> 4; + if (ACEL_EN & activestatus) + sensor_id[num_of_sensors++] = accel_idx; + + if (GYRO_EN & activestatus) + sensor_id[num_of_sensors++] = gyro_idx; + + if (MAGNO_EN & activestatus) + sensor_id[num_of_sensors++] = mag_idx; + + if (ALS_EN & activestatus) + sensor_id[num_of_sensors++] = als_idx; + + return num_of_sensors; +} + +static int amd_mp2_pci_init(struct amd_mp2_dev *privdata, struct pci_dev *pdev) +{ + int rc; + + pci_set_drvdata(pdev, privdata); + pcim_enable_device(pdev); + pcim_iomap_regions(pdev, BIT(2), DRIVER_NAME); + + privdata->mmio = pcim_iomap_table(pdev)[2]; + pci_set_master(pdev); + + rc = pci_set_dma_mask(pdev, DMA_BIT_MASK(64)); + if (rc) + rc = pci_set_dma_mask(pdev, DMA_BIT_MASK(32)); + return rc; +} + +static int amd_mp2_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) +{ + struct amd_mp2_dev *privdata; + int rc; + + privdata = devm_kzalloc(&pdev->dev, sizeof(*privdata), GFP_KERNEL); + if (!privdata) + return -ENOMEM; + privdata->pdev = pdev; + rc = amd_mp2_pci_init(privdata, pdev); + if (rc) + return rc; + rc = amd_sfh_hid_client_init(privdata); + if (rc) + return rc; + return 0; +} + +static void amd_mp2_pci_remove(struct pci_dev *pdev) +{ + struct amd_mp2_dev *privdata = pci_get_drvdata(pdev); + + amd_sfh_hid_client_deinit(privdata); + amd_stop_all_sensors(privdata); +} + +static const struct pci_device_id amd_mp2_pci_tbl[] = { + { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_MP2) }, + {}, +}; +MODULE_DEVICE_TABLE(pci, amd_mp2_pci_tbl); + +static struct pci_driver amd_mp2_pci_driver = { + .name = DRIVER_NAME, + .id_table = amd_mp2_pci_tbl, + .probe = amd_mp2_pci_probe, + .remove = amd_mp2_pci_remove, +}; +module_pci_driver(amd_mp2_pci_driver); + +MODULE_DESCRIPTION(DRIVER_DESC); +MODULE_LICENSE("Dual BSD/GPL"); +MODULE_AUTHOR("Shyam Sundar S K <Shyam-sundar.S-k@amd.com>"); +MODULE_AUTHOR("Sandeep Singh <Sandeep.singh@amd.com>"); diff --git a/drivers/hid/amd-sfh-hid/amd_mp2_pcie.h b/drivers/hid/amd-sfh-hid/amd_mp2_pcie.h new file mode 100644 index 000000000000..a4ef604c4fe8 --- /dev/null +++ b/drivers/hid/amd-sfh-hid/amd_mp2_pcie.h @@ -0,0 +1,83 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * AMD MP2 PCIe communication driver + * Copyright 2020 Advanced Micro Devices, Inc. + * Authors: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> + * Sandeep Singh <Sandeep.singh@amd.com> + */ + +#ifndef PCIE_MP2_AMD_H +#define PCIE_MP2_AMD_H + +#include <linux/pci.h> +#include <linux/types.h> + +#define PCI_DEVICE_ID_AMD_MP2 0x15E4 + +/* MP2 C2P Message Registers */ +#define AMD_C2P_MSG0 0x10500 +#define AMD_C2P_MSG1 0x10504 +#define AMD_C2P_MSG2 0x10508 + +/* MP2 P2C Message Registers */ +#define AMD_P2C_MSG3 0x1068C /* Supported Sensors info */ + +/* SFH Command register */ +union sfh_cmd_base { + u32 ul; + struct { + u32 cmd_id : 8; + u32 sensor_id : 8; + u32 period : 16; + } s; +}; + +union sfh_cmd_param { + u32 ul; + struct { + u32 buf_layout : 2; + u32 buf_length : 6; + u32 rsvd : 24; + } s; +}; + +struct sfh_cmd_reg { + union sfh_cmd_base cmd_base; + union sfh_cmd_param cmd_param; + phys_addr_t phys_addr; +}; + +enum command_id { + enable_sensor = 1, + disable_sensor = 2, + stop_all_sensors = 8, + invalid_cmd = 0xf +}; + +enum sensor_idx { + accel_idx = 0, + gyro_idx = 1, + mag_idx = 2, + als_idx = 19 +}; + +struct amd_mp2_dev { + struct pci_dev *pdev; + struct amdtp_cl_data *cl_data; + void __iomem *mmio; + u32 activecontrolstatus; +}; + +struct amd_mp2_sensor_info { + u8 sensor_idx; + u32 period; + phys_addr_t phys_address; +}; + +void amd_start_sensor(struct amd_mp2_dev *privdata, struct amd_mp2_sensor_info info); +void amd_stop_sensor(struct amd_mp2_dev *privdata, u16 sensor_idx); +void amd_stop_all_sensors(struct amd_mp2_dev *privdata); +int amd_mp2_get_sensor_num(struct amd_mp2_dev *privdata, u8 *sensor_id); +int amd_sfh_hid_client_init(struct amd_mp2_dev *privdata); +int amd_sfh_hid_client_deinit(struct amd_mp2_dev *privdata); +#endif