diff mbox series

[v6,2/4] SFH: PCIe driver to add support of AMD sensor fusion

Message ID 20200809102511.2657644-3-Sandeep.Singh@amd.com (mailing list archive)
State Superseded
Delegated to: Jiri Kosina
Headers show
Series SFH: Add Support for AMD Sensor Fusion Hub | expand

Commit Message

Sandeep Singh Aug. 9, 2020, 10:25 a.m. UTC
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

Comments

kernel test robot Aug. 9, 2020, 11:54 a.m. UTC | #1
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
Randy Dunlap Aug. 9, 2020, 1:59 p.m. UTC | #2
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.
Andy Shevchenko Aug. 19, 2020, 10:40 a.m. UTC | #3
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 mbox series

Patch

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