Not a real review, but your patch series seems to be repeating a lot from drivers/i2c/busses/i2c-amd-mp2*. Is there any chance we could re-use the code? E.g. the AMD_C2P_* definitions from drivers/i2c/busses/i2c-amd-mp2.h? Am Donnerstag, den 27.02.2020, 10:28 +0530 schrieb Sandeep Singh: > From: Sandeep Singh > > 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: Sandeep Singh > Signed-off-by: Nehal Shah > Reported-by: kbuild test robot > --- > drivers/hid/Kconfig | 2 + > drivers/hid/Makefile | 1 + > drivers/hid/amd-sfh-hid/Kconfig | 20 +++ > drivers/hid/amd-sfh-hid/Makefile | 16 +++ > drivers/hid/amd-sfh-hid/amd_mp2_pcie.c | 243 > +++++++++++++++++++++++++++++++++ > drivers/hid/amd-sfh-hid/amd_mp2_pcie.h | 176 > ++++++++++++++++++++++++ > 6 files changed, 458 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 > > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig > index 494a39e..b253ad1 100644 > --- a/drivers/hid/Kconfig > +++ b/drivers/hid/Kconfig > @@ -1155,4 +1155,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 bfefa36..15a08e8 100644 > --- a/drivers/hid/Makefile > +++ b/drivers/hid/Makefile > @@ -139,3 +139,4 @@ 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 0000000..7a224a1 > --- /dev/null > +++ b/drivers/hid/amd-sfh-hid/Kconfig > @@ -0,0 +1,20 @@ > +# SPDX-License-Identifier: GPL-2.0-only > +menu "AMD SFH HID support" > + depends on (X86_64 || COMPILE_TEST) && 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-mp2-pcie and 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 0000000..fa38d84 > --- /dev/null > +++ b/drivers/hid/amd-sfh-hid/Makefile > @@ -0,0 +1,16 @@ > +# SPDX-License-Identifier: GPL-2.0 > +# > +# Makefile - AMD SFH HID drivers > +# Copyright (c) 2020-2021, Advanced Micro Devices, Inc. > +# > +# > +ccflags-m := -Werror > +obj-$(CONFIG_AMD_SFH_HID) += amd-mp2-pcie.o > +amd-mp2-pcie-objs := amd_mp2_pcie.o > + > +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+= 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 0000000..c67f389 > --- /dev/null > +++ b/drivers/hid/amd-sfh-hid/amd_mp2_pcie.c > @@ -0,0 +1,243 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * AMD MP2 PCIe communication driver > + * > + * Authors: Shyam Sundar S K > + * Nehal Bakulchandra Shah > > + */ > + > +#include > +#include > +#include > +#include > +#include > +#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(AMBIENT_LIGHT_IDX) > + > +void amd_start_sensor(struct pci_dev *pdev, struct > amd_mp2_sensor_info info) > +{ > + struct amd_mp2_dev *privdata = pci_get_drvdata(pdev); > + union sfh_command_parameter 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.buffer_layout = 1; > + cmd_param.s.buffer_length = 16; > + > + write64((u64)info.phy_address, privdata->mmio + AMD_C2P_MSG2); > + writel(cmd_param.ul, privdata->mmio + AMD_C2P_MSG1); > + writel(cmd_base.ul, privdata->mmio + AMD_C2P_MSG0); > +} > +EXPORT_SYMBOL_GPL(amd_start_sensor); > + > +void amd_stop_sensor(struct pci_dev *pdev, u16 sensor_idx) > +{ > + struct amd_mp2_dev *privdata = pci_get_drvdata(pdev); > + 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; > + > + write64(0x0, privdata->mmio + AMD_C2P_MSG2); > + writel(cmd_base.ul, privdata->mmio + AMD_C2P_MSG0); > +} > +EXPORT_SYMBOL_GPL(amd_stop_sensor); > + > +void amd_stop_all_sensors(struct pci_dev *pdev) > +{ > + struct amd_mp2_dev *privdata = pci_get_drvdata(pdev); > + 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); > +} > +EXPORT_SYMBOL_GPL(amd_stop_all_sensors); > + > +int amd_mp2_get_sensor_num(struct pci_dev *dev, u8 *sensor_id) > +{ > + struct amd_mp2_dev *privdata = pci_get_drvdata(dev); > + int activestatus; > + int num_of_sensors = 0; > + > + if (!sensor_id) > + return -ENOMEM; > + > + privdata->eventreg.activecontrolstatus = > + readl(privdata->mmio + AMD_P2C_MSG3); > + activestatus = privdata->eventreg.activecontrolstatus >> 4; > + > + if (ACEL_EN & activestatus) { > + sensor_id[num_of_sensors] = ACCEL_IDX; > + num_of_sensors++; > + } > + if (GYRO_EN & activestatus) { > + sensor_id[num_of_sensors] = GYRO_IDX; > + num_of_sensors++; > + } > + if (MAGNO_EN & activestatus) { > + sensor_id[num_of_sensors] = MAG_IDX; > + num_of_sensors++; > + } > + > + if (ALS_EN & activestatus) { > + sensor_id[num_of_sensors] = AMBIENT_LIGHT_IDX; > + num_of_sensors++; > + } > + > + return num_of_sensors; > +} > +EXPORT_SYMBOL_GPL(amd_mp2_get_sensor_num); > + > +static int amd_mp2_pci_init(struct amd_mp2_dev *privdata, struct > pci_dev *pdev) > +{ > + int rc; > + int bar_index = 2; > + resource_size_t size, base; > + > + pci_set_drvdata(pdev, privdata); > + > + rc = pcim_enable_device(pdev); > + if (rc) > + goto err_pci_enable; > + > + rc = pcim_iomap_regions(pdev, 1 >> 2, DRIVER_NAME); > + if (rc) > + goto err_pci_regions; > + > + 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)); > + if (rc) > + goto err_dma_mask; > + } > + > + rc = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(64)); > + if (rc) { > + rc = pci_set_consistent_dma_mask(pdev, > DMA_BIT_MASK(32)); > + if (rc) > + goto err_dma_mask; > + } > + > + base = pci_resource_start(pdev, bar_index); > + size = pci_resource_len(pdev, bar_index); > + dev_dbg(ndev_dev(privdata), "Base addr:%llx size:%llx\n", > + (unsigned long long)base, (unsigned long long)size); > + > + privdata->mmio = ioremap(base, size); > + if (!privdata->mmio) { > + rc = -EIO; > + goto err_dma_mask; > + } > + > + return 0; > + > +err_dma_mask: > + pci_clear_master(pdev); > +err_pci_regions: > + pci_disable_device(pdev); > +err_pci_enable: > + pci_set_drvdata(pdev, NULL); > + return rc; > +} > + > +static void amd_mp2_pci_deinit(struct amd_mp2_dev *privdata) > +{ > + struct pci_dev *pdev = ndev_pdev(privdata); > + > + amd_stop_all_sensors(pdev); > + pci_iounmap(pdev, privdata->mmio); > + > + pci_clear_master(pdev); > + pci_disable_device(pdev); > + pci_set_drvdata(pdev, NULL); > +} > + > +static int amd_mp2_pci_probe(struct pci_dev *pdev, > + const struct pci_device_id *id) > +{ > + struct amd_mp2_dev *privdata; > + int rc; > + > + dev_info(&pdev->dev, "MP2 device found [%04x:%04x] (rev %x)\n", > + (int)pdev->vendor, (int)pdev->device, (int)pdev- > >revision); > + > + privdata = devm_kzalloc(&pdev->dev, sizeof(*privdata), > GFP_KERNEL); > + > + if (!privdata) { > + rc = -ENOMEM; > + goto err_dev; > + } > + > + privdata->pdev = pdev; > + > + rc = amd_mp2_pci_init(privdata, pdev); > + if (rc) > + goto err_pci_init; > + return 0; > + > +err_pci_init: > + return rc; > +err_dev: > + return rc; > +} > + > +static void amd_mp2_pci_remove(struct pci_dev *pdev) > +{ > + struct amd_mp2_dev *privdata = pci_get_drvdata(pdev); > + > + amd_mp2_pci_deinit(privdata); > +} > + > +static const struct pci_device_id amd_mp2_pci_tbl[] = { > + {PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_MP2)}, > + {0} > +}; > +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, > +}; > + > +static int __init amd_mp2_pci_driver_init(void) > +{ > + return pci_register_driver(&amd_mp2_pci_driver); > +} > +module_init(amd_mp2_pci_driver_init); > + > +static void __exit amd_mp2_pci_driver_exit(void) > +{ > + pci_unregister_driver(&amd_mp2_pci_driver); > +} > +module_exit(amd_mp2_pci_driver_exit); > +MODULE_DESCRIPTION(DRIVER_DESC); > +MODULE_LICENSE("Dual BSD/GPL"); > +MODULE_AUTHOR("Shyam Sundar S K "); > +MODULE_AUTHOR("Nehal Bakulchandra Shah < > Nehal-bakulchandra.Shah@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 0000000..3ba69ac > --- /dev/null > +++ b/drivers/hid/amd-sfh-hid/amd_mp2_pcie.h > @@ -0,0 +1,176 @@ > +/* SPDX-License-Identifier: GPL-2.0 > + * > + * AMD MP2 PCIe communication driver > + * > + * Authors: Shyam Sundar S K > + * Nehal Bakulchandra Shah > > + */ > + > +#ifndef PCIE_MP2_AMD_H > +#define PCIE_MP2_AMD_H > + > +#include > +#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 > +#define AMD_C2P_MSG3 0x1050c > +#define AMD_C2P_MSG4 0x10510 > +#define AMD_C2P_MSG5 0x10514 > +#define AMD_C2P_MSG6 0x10518 > +#define AMD_C2P_MSG7 0x1051c > +#define AMD_C2P_MSG8 0x10520 > +#define AMD_C2P_MSG9 0x10524 > + > +/* MP2 P2C Message Registers */ > +#define AMD_P2C_MSG0 0x10680 /*Do not use*/ > +#define AMD_P2C_MSG1 0x10684 > +#define AMD_P2C_MSG2 0x10688 > +#define AMD_P2C_MSG3 0x1068C /*MP2 debug info*/ > +#define AMD_P2C_MSG_INTEN 0x10690 /*MP2 int gen register*/ > +#define AMD_P2C_MSG_INTSTS 0x10694 /*Interrupt sts*/ > + > +#define write64 amdsfh_write64 > +static inline void amdsfh_write64(u64 val, void __iomem *mmio) > +{ > + writel(val, mmio); > + writel(val >> 32, mmio + sizeof(u32)); > +} > + > +#define read64 amdsfh_read64 > +static inline u64 amdsfh_read64(void __iomem *mmio) > +{ > + u64 low, high; > + > + low = readl(mmio); > + high = readl(mmio + sizeof(u32)); > + return low | (high << 32); > +} > + > +/* > + * SFH Command registers > + */ > +union sfh_cmd_base { > + u32 ul; > + struct { > + u32 cmd_id : 8; > + u32 sensor_id : 8; > + u32 period : 16; > + } s; > +}; > + > +union sfh_command_parameter { > + u32 ul; > + struct { > + u32 buffer_layout : 2; > + u32 buffer_length : 6; > + u32 rsvd : 24; > + } s; > +}; > + > +struct sfh_command_register { > + union sfh_cmd_base cmd_base; > + union sfh_command_parameter cmd_param; > + phys_addr_t phy_addr; > +}; > + > +/* > + * SFH Response registers > + */ > +enum response_type { > + non_operationevent, > + command_success, > + command_failed, > + sfi_dataready_event, > + invalid_response = 0xff, > +}; > + > +enum status_type { > + cmd_success, > + invalid_data_payload, > + invalid_data_length, > + invalid_sensor_id, > + invalid_dram_addr, > + invalid_command, > + sensor_enabled, > + sensor_disabled, > + status_end, > +}; > + > +enum command_id { > + non_operation = 0, > + enable_sensor = 1, > + disable_sensor = 2, > + dump_sensorinfo = 3, > + numberof_sensordiscovered = 4, > + who_am_i_regchipid = 5, > + set_dcd_data = 6, > + get_dcd_data = 7, > + stop_all_sensors = 8, > + invalid_cmd = 0xf, > +}; > + > +/** > + * union sfh_event_base : bit access of C2P commands > + * @response: bit: 0..3 SFI response_type > + * @status: bit: 6..5 status_type > + * @out_in_c2p: bit: 5 0- output in DRAM,1-in C2PMsg > + * @length: bit: 8..13 length > + * @dbg:bit: 14.15 dbg msg include in p2c msg 1-2 > + * @sensor_id:bit: 16..23 Sensor ID > + * @rsvd:bit: 24..31 Reservered for future use > + */ > +union sfh_event_base { > + u32 ul; > + struct { > + u32 response : 4; > + u32 status : 3; > + u32 out_in_c2p : 1; > + u32 length : 6; > + u32 dbg : 2; > + u32 sensor_id : 8; > + u32 rsvd : 8; > + } s; > +}; > + > +struct sfi_event_register { > + union sfh_event_base evtbase; > + u32 debuginfo1; > + u32 debuginfo2; > + u32 activecontrolstatus; > +}; > + > +enum sensor_idx { > + ACCEL_IDX = 0, > + GYRO_IDX = 1, > + MAG_IDX = 2, > + AMBIENT_LIGHT_IDX = 19, > + NUM_ALL_SENSOR_CONSUMERS > +}; > + > +struct amd_mp2_dev { > + struct pci_dev *pdev; > + void __iomem *mmio; > + union sfh_event_base eventval; > + struct sfi_event_register eventreg; > + struct delayed_work work; > + void *ctx; > + void *cl_data; > +}; > + > +struct amd_mp2_sensor_info { > + u8 sensor_idx; > + u32 period; > + phys_addr_t phy_address; > +}; > + > +void amd_start_sensor(struct pci_dev *pdev, struct > amd_mp2_sensor_info info); > +void amd_stop_sensor(struct pci_dev *pdev, u16 sensor_idx); > +void amd_stop_all_sensors(struct pci_dev *pdev); > +int amd_mp2_get_sensor_num(struct pci_dev *dev, u8 *sensor_id); > +#define ndev_pdev(ndev) ((ndev)->pdev) > +#define ndev_name(ndev) pci_name(ndev_pdev(ndev)) > +#define ndev_dev(ndev) (&ndev_pdev(ndev)->dev) > +#endif