| Message ID | 1582779537-25662-3-git-send-email-Sandeep.Singh@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | SFH: Add Support for AMD Sensor Fusion Hub | expand |
[snipped] On 2/27/2020 10:28 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: Sandeep Singh <sandeep.singh@amd.com> > Signed-off-by: Nehal Shah <Nehal-bakulchandra.Shah@amd.com> > Reported-by: kbuild test robot <lkp@intel.com> Guys , Do you have any updates on this patch series? Can you please make some time to review ?
On Thu, Feb 27, 2020 at 7:01 AM Sandeep Singh <Sandeep.Singh@amd.com> 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 You asked for review, here you are. TL,DR; it requires a bit of work. ... > + depends on (X86_64 || COMPILE_TEST) && PCI For better maintenance depends on X86_64 || COMPILE_TEST depends on PCI ... > +# SPDX-License-Identifier: GPL-2.0 > +# > +# Makefile - AMD SFH HID drivers > +# Copyright (c) 2020-2021, Advanced Micro Devices, Inc. > +# > +# Perhaps simple blank line instead? > +ccflags-y += -I$(srctree)/$(src)/ Why? ... > +#include <linux/interrupt.h> > +#include <linux/module.h> > +#include <linux/pci.h> > +#include <linux/slab.h> > +#include <linux/delay.h> Keep in order? + blank line? Missed bits.h, types.h. > +#include "amd_mp2_pcie.h" ... > + write64((u64)info.phy_address, privdata->mmio + AMD_C2P_MSG2); Why explicit cast? ... > + /*fill up command register */ Space is missed. ... > + if (!sensor_id) > + return -ENOMEM; I can say -EINVAL as per its definition, but why do you need this at all? ... > +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); Better to assign when you are sure (to some extend in both of them): a) it's needed b) driver is going to be probed correctly ... > + rc = pcim_iomap_regions(pdev, 1 >> 2, DRIVER_NAME); What 1 >> 2 means? Shouldn't be simple BIT(2)? How was this been tested? > + if (rc) > + goto err_pci_regions; ... > + 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); Read printk-formats.rst. Now, when you get familiar with it, find proper specifier and drop these ugly castings. But wait, why do you need this? `dmesg` will show it anyway during boot / hotplug event time. ... > + privdata->mmio = ioremap(base, size); > + if (!privdata->mmio) { > + rc = -EIO; > + goto err_dma_mask; > + } Why?! ... > +err_dma_mask: > + pci_clear_master(pdev); > +err_pci_regions: > + pci_disable_device(pdev); Are you using devres or not? Please, be consistent. > +err_pci_enable: > + pci_set_drvdata(pdev, NULL); I think it's some like 5 to 10 years that we don't need this. > + return rc; > +} ... > + pci_iounmap(pdev, privdata->mmio); > + > + pci_clear_master(pdev); > + pci_disable_device(pdev); > + pci_set_drvdata(pdev, NULL); Ditto as above two comments. ... > + dev_info(&pdev->dev, "MP2 device found [%04x:%04x] (rev %x)\n", > + (int)pdev->vendor, (int)pdev->device, (int)pdev->revision); Oh, if you use explicit casting for printf(), 99.9% you are doing something wrong (in kernel space). On top of that, why this noise is here? ... > + privdata = devm_kzalloc(&pdev->dev, sizeof(*privdata), GFP_KERNEL); > + No need for this blank line. > + if (!privdata) { > + } ... > + rc = amd_mp2_pci_init(privdata, pdev); > + if (rc) > + goto err_pci_init; > + return 0; Why its content can't be simple here? I.o.w. why this function is needed? ... > +err_pci_init: > + return rc; > +err_dev: > + return rc; Completely useless code. > +} ... > +static const struct pci_device_id amd_mp2_pci_tbl[] = { > + {PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_MP2)}, > + {0} 0 is not needed, but it's up to you. > +}; ... > +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_pci_driver() We have it for years. ... > +#include <linux/pci.h> I don't see users of it, but missed headers types.h ... > +#define PCI_DEVICE_ID_AMD_MP2 0x15E4 Why it's not in C file? ... > +#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*/ Missed spaces. ... > +#define write64 amdsfh_write64 > +static inline void amdsfh_write64(u64 val, void __iomem *mmio) > +{ > + writel(val, mmio); > + writel(val >> 32, mmio + sizeof(u32)); > +} NIH of lo_hi_writeq(). > +#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); > +} NIH of lo_hi_readq(). ... > +struct sfh_command_register { > + union sfh_cmd_base cmd_base; > + union sfh_command_parameter cmd_param; > + phys_addr_t phy_addr; Are you sure? This type is flexible. And by name of the struct I think it operates with hardware, so, fix it accordingly. > +}; ... > +enum response_type { > + non_operationevent, > + command_success, > + command_failed, > + sfi_dataready_event, > + invalid_response = 0xff, GENMASK() > +}; UPPER CASE? > +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, > +}; Ditto. ... > +enum sensor_idx { Do you need names for enums like this? > + 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; Header for __iomem? > + struct delayed_work work; Header for this? > + void *ctx; > + void *cl_data; > +}; ... > +struct amd_mp2_sensor_info { > + u8 sensor_idx; > + u32 period; > + phys_addr_t phy_address; Same comment as per above use of phys_addr_t type. > +}; ... > +#define ndev_pdev(ndev) ((ndev)->pdev) > +#define ndev_name(ndev) pci_name(ndev_pdev(ndev)) > +#define ndev_dev(ndev) (&ndev_pdev(ndev)->dev) Why? What's the benefit?
Thanks Andy for review comments i will look into it. On 3/27/2020 8:25 PM, Andy Shevchenko wrote: > [CAUTION: External Email] > > On Thu, Feb 27, 2020 at 7:01 AM Sandeep Singh <Sandeep.Singh@amd.com> 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 > You asked for review, here you are. > TL,DR; it requires a bit of work. > > ... > >> + depends on (X86_64 || COMPILE_TEST) && PCI > For better maintenance > depends on X86_64 || COMPILE_TEST > depends on PCI > > ... > >> +# SPDX-License-Identifier: GPL-2.0 >> +# >> +# Makefile - AMD SFH HID drivers >> +# Copyright (c) 2020-2021, Advanced Micro Devices, Inc. >> +# >> +# > Perhaps simple blank line instead? > >> +ccflags-y += -I$(srctree)/$(src)/ > Why? > > ... > >> +#include <linux/interrupt.h> >> +#include <linux/module.h> >> +#include <linux/pci.h> >> +#include <linux/slab.h> >> +#include <linux/delay.h> > Keep in order? > > + blank line? > > Missed bits.h, types.h. > >> +#include "amd_mp2_pcie.h" > ... > >> + write64((u64)info.phy_address, privdata->mmio + AMD_C2P_MSG2); > Why explicit cast? > > ... > >> + /*fill up command register */ > Space is missed. > > ... > >> + if (!sensor_id) >> + return -ENOMEM; > I can say -EINVAL as per its definition, but why do you need this at all? > > ... > >> +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); > Better to assign when you are sure (to some extend in both of them): > a) it's needed > b) driver is going to be probed correctly > > ... > >> + rc = pcim_iomap_regions(pdev, 1 >> 2, DRIVER_NAME); > What 1 >> 2 means? Shouldn't be simple BIT(2)? > How was this been tested? > >> + if (rc) >> + goto err_pci_regions; > ... > >> + 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); > Read printk-formats.rst. > Now, when you get familiar with it, find proper specifier and drop > these ugly castings. > But wait, why do you need this? `dmesg` will show it anyway during > boot / hotplug event time. > > ... > >> + privdata->mmio = ioremap(base, size); >> + if (!privdata->mmio) { >> + rc = -EIO; >> + goto err_dma_mask; >> + } > Why?! > > ... > >> +err_dma_mask: >> + pci_clear_master(pdev); >> +err_pci_regions: >> + pci_disable_device(pdev); > Are you using devres or not? Please, be consistent. > >> +err_pci_enable: >> + pci_set_drvdata(pdev, NULL); > I think it's some like 5 to 10 years that we don't need this. > >> + return rc; >> +} > ... > >> + pci_iounmap(pdev, privdata->mmio); >> + >> + pci_clear_master(pdev); >> + pci_disable_device(pdev); >> + pci_set_drvdata(pdev, NULL); > Ditto as above two comments. > > ... > >> + dev_info(&pdev->dev, "MP2 device found [%04x:%04x] (rev %x)\n", >> + (int)pdev->vendor, (int)pdev->device, (int)pdev->revision); > Oh, if you use explicit casting for printf(), 99.9% you are doing > something wrong (in kernel space). > On top of that, why this noise is here? > > ... > >> + privdata = devm_kzalloc(&pdev->dev, sizeof(*privdata), GFP_KERNEL); >> + > No need for this blank line. > >> + if (!privdata) { >> + } > ... > > >> + rc = amd_mp2_pci_init(privdata, pdev); >> + if (rc) >> + goto err_pci_init; >> + return 0; > Why its content can't be simple here? I.o.w. why this function is needed? > > ... > >> +err_pci_init: >> + return rc; >> +err_dev: >> + return rc; > Completely useless code. > >> +} > ... > >> +static const struct pci_device_id amd_mp2_pci_tbl[] = { >> + {PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_MP2)}, >> + {0} > 0 is not needed, but it's up to you. > >> +}; > ... > >> +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_pci_driver() > We have it for years. > > ... > >> +#include <linux/pci.h> > I don't see users of it, but missed headers > types.h > > ... > >> +#define PCI_DEVICE_ID_AMD_MP2 0x15E4 > Why it's not in C file? > > ... > >> +#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*/ > Missed spaces. > > ... > >> +#define write64 amdsfh_write64 >> +static inline void amdsfh_write64(u64 val, void __iomem *mmio) >> +{ >> + writel(val, mmio); >> + writel(val >> 32, mmio + sizeof(u32)); >> +} > NIH of lo_hi_writeq(). > >> +#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); >> +} > NIH of lo_hi_readq(). > > ... > >> +struct sfh_command_register { >> + union sfh_cmd_base cmd_base; >> + union sfh_command_parameter cmd_param; >> + phys_addr_t phy_addr; > Are you sure? This type is flexible. And by name of the struct I think > it operates with hardware, so, fix it accordingly. > >> +}; > ... > >> +enum response_type { >> + non_operationevent, >> + command_success, >> + command_failed, >> + sfi_dataready_event, >> + invalid_response = 0xff, > GENMASK() > >> +}; > UPPER CASE? > >> +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, >> +}; > Ditto. > > ... > >> +enum sensor_idx { > Do you need names for enums like this? > >> + 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; > Header for __iomem? > >> + struct delayed_work work; > Header for this? > >> + void *ctx; >> + void *cl_data; >> +}; > ... > >> +struct amd_mp2_sensor_info { >> + u8 sensor_idx; >> + u32 period; >> + phys_addr_t phy_address; > Same comment as per above use of phys_addr_t type. > >> +}; > ... > >> +#define ndev_pdev(ndev) ((ndev)->pdev) >> +#define ndev_name(ndev) pci_name(ndev_pdev(ndev)) >> +#define ndev_dev(ndev) (&ndev_pdev(ndev)->dev) > Why? What's the benefit? > > -- > With Best Regards, > Andy Shevchenko Regards Sandeep
I took some time testing the patch series on my HP ENVY x360 13- ag0005ng with a Ryzen 5 2500U. On my machine the readout of register AMD_P2C_MSG3 (0x1068C) always comes back as zero (0x0) and thusly activestatus will be zero as well. Consequently the bistmask (0x0) will not match any sensor types. I came up with a patch [1] on top of this patch series to add module parameters to be able to statically enable the different sensors. If I load the module with "force_accel=1" I get a working accelerometer HID device and working screen rotation through iio-sensor-proxy. This only works so far, if I boot the kernel with "amd_iommu=off", which seems to be a general issue with this driver [2],[3]. [1] https://gist.githubusercontent.com/conqp/e8a0793406fbe7c9714f01f3078ea33a/raw/ac2de743e68d1dd90430e57cb28df8c1dc5d1098/amd-sfh.patch [2] https://bbs.archlinux.org/viewtopic.php?id=253058 [3] https://bbs.archlinux.org/viewtopic.php?id=252815
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 <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: Sandeep Singh <sandeep.singh@amd.com> > Signed-off-by: Nehal Shah <Nehal-bakulchandra.Shah@amd.com> > Reported-by: kbuild test robot <lkp@intel.com> > --- > 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 <Shyam-sundar.S-k@amd.com> > + * Nehal Bakulchandra Shah <Nehal-bakulchandra.Shah@amd.com > > > + */ > + > +#include <linux/interrupt.h> > +#include <linux/module.h> > +#include <linux/pci.h> > +#include <linux/slab.h> > +#include <linux/delay.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(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 <Shyam-sundar.S-k@amd.com>"); > +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 <Shyam-sundar.S-k@amd.com> > + * Nehal Bakulchandra Shah <Nehal-bakulchandra.Shah@amd.com > > > + */ > + > +#ifndef PCIE_MP2_AMD_H > +#define PCIE_MP2_AMD_H > + > +#include <linux/pci.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 > +#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
Hi Richard, On 3/31/2020 6:01 PM, Richard Neumann wrote: > 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? Thanks for the mail. Yes there are some common structures, however as of now we have kept separately considering both are part of different sub systems. But may be will consider this input for future enhancement. Thanks Nehal Shah > > Am Donnerstag, den 27.02.2020, 10:28 +0530 schrieb Sandeep Singh: >> 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: Sandeep Singh <sandeep.singh@amd.com> >> Signed-off-by: Nehal Shah <Nehal-bakulchandra.Shah@amd.com> >> Reported-by: kbuild test robot <lkp@intel.com> >> --- >> 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 <Shyam-sundar.S-k@amd.com> >> + * Nehal Bakulchandra Shah <Nehal-bakulchandra.Shah@amd.com >> + */ >> + >> +#include <linux/interrupt.h> >> +#include <linux/module.h> >> +#include <linux/pci.h> >> +#include <linux/slab.h> >> +#include <linux/delay.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(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 <Shyam-sundar.S-k@amd.com>"); >> +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 <Shyam-sundar.S-k@amd.com> >> + * Nehal Bakulchandra Shah <Nehal-bakulchandra.Shah@amd.com >> + */ >> + >> +#ifndef PCIE_MP2_AMD_H >> +#define PCIE_MP2_AMD_H >> + >> +#include <linux/pci.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 >> +#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
On Tue, Mar 31, 2020 at 4:26 PM Shah, Nehal-bakulchandra <nehal-bakulchandra.shah@amd.com> wrote: > On 3/31/2020 6:01 PM, Richard Neumann wrote: > > 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? > > Thanks for the mail. Yes there are some common structures, however as of now we have kept separately considering both > > are part of different sub systems. But may be will consider this input for future enhancement. It can be done in a form of shared definitions at least in include/linux/platform_data/x86/amd-mp2.h or alike ...
Am Dienstag, den 31.03.2020, 20:24 +0300 schrieb Andy Shevchenko: > On Tue, Mar 31, 2020 at 4:26 PM Shah, Nehal-bakulchandra > <nehal-bakulchandra.shah@amd.com> wrote: > > On 3/31/2020 6:01 PM, Richard Neumann wrote: > > > 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? > > > > Thanks for the mail. Yes there are some common structures, however > > as of now we have kept separately considering both > > > > are part of different sub systems. But may be will consider this > > input for future enhancement. > > It can be done in a form of shared definitions at least in > include/linux/platform_data/x86/amd-mp2.h or alike ... > I managed to add support for the AMD SFH PCI device to i2c-amd-mp2* and outsourced the headers to include/linux/i2c-amd-mp2.h. [1] I also refactored the patch series (excluded the documentation) [2] to use the PCI device now provided by i2c_amd_mp2_pci and removed some duplicate and unncessary code. The driver now consist of just one module (amd_sfhtp_hid). Unfortunately I was not able to solve the problem, that I get AMD-Vi IO_PAGE_FAULT errors when not booted with amd_iommu=off. [1] https://gist.githubusercontent.com/conqp/4d726f86da8a8397d6e70091a124de67/raw/f97e88a0b44d98bfa1258cb73c8afe4dce7afa87/i2c-amd-mp2.patch [2] https://gist.githubusercontent.com/conqp/67036e690aca89d08b958971edac283d/raw/2a1ef122f9c8c8e07164b6d597962ce7bbad6d45/amd-sfhtp.patch
HI On 4/1/2020 9:58 PM, Richard Neumann wrote: > Am Dienstag, den 31.03.2020, 20:24 +0300 schrieb Andy Shevchenko: >> On Tue, Mar 31, 2020 at 4:26 PM Shah, Nehal-bakulchandra >> <nehal-bakulchandra.shah@amd.com> wrote: >>> On 3/31/2020 6:01 PM, Richard Neumann wrote: >>>> 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? >>> Thanks for the mail. Yes there are some common structures, however >>> as of now we have kept separately considering both >>> >>> are part of different sub systems. But may be will consider this >>> input for future enhancement. >> It can be done in a form of shared definitions at least in >> include/linux/platform_data/x86/amd-mp2.h or alike ... >> > I managed to add support for the AMD SFH PCI device to i2c-amd-mp2* and > outsourced the headers to include/linux/i2c-amd-mp2.h. [1] > I also refactored the patch series (excluded the documentation) [2] to > use the PCI device now provided by i2c_amd_mp2_pci and removed some > duplicate and unncessary code. > The driver now consist of just one module (amd_sfhtp_hid). > Unfortunately I was not able to solve the problem, that I get AMD-Vi > IO_PAGE_FAULT errors when not booted with amd_iommu=off. > > [1] https://gist.githubusercontent.com/conqp/4d726f86da8a8397d6e70091a124de67/raw/f97e88a0b44d98bfa1258cb73c8afe4dce7afa87/i2c-amd-mp2.patch > [2] https://gist.githubusercontent.com/conqp/67036e690aca89d08b958971edac283d/raw/2a1ef122f9c8c8e07164b6d597962ce7bbad6d45/amd-sfhtp.patch Thanks for the patch and appreciate your efforts. At this point of time, we would like to have our first patch for SFH to be upstreamed and dont want to complicate the with two sub systems and maintainers. Surely will consider this input for future enhancement. Thanks for your understanding. Thanks Nehal
Am Montag, den 06.04.2020, 10:56 +0530 schrieb Shah, Nehal- bakulchandra: > HI > > On 4/1/2020 9:58 PM, Richard Neumann wrote: > > Am Dienstag, den 31.03.2020, 20:24 +0300 schrieb Andy Shevchenko: > > > On Tue, Mar 31, 2020 at 4:26 PM Shah, Nehal-bakulchandra > > > <nehal-bakulchandra.shah@amd.com> wrote: > > > > On 3/31/2020 6:01 PM, Richard Neumann wrote: > > > > > 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? > > > > Thanks for the mail. Yes there are some common structures, > > > > however > > > > as of now we have kept separately considering both > > > > > > > > are part of different sub systems. But may be will consider > > > > this > > > > input for future enhancement. > > > It can be done in a form of shared definitions at least in > > > include/linux/platform_data/x86/amd-mp2.h or alike ... > > > > > I managed to add support for the AMD SFH PCI device to i2c-amd-mp2* > > and > > outsourced the headers to include/linux/i2c-amd-mp2.h. [1] > > I also refactored the patch series (excluded the documentation) [2] > > to > > use the PCI device now provided by i2c_amd_mp2_pci and removed some > > duplicate and unncessary code. > > The driver now consist of just one module (amd_sfhtp_hid). > > Unfortunately I was not able to solve the problem, that I get AMD- > > Vi > > IO_PAGE_FAULT errors when not booted with amd_iommu=off. > > > > [1] > > https://gist.githubusercontent.com/conqp/4d726f86da8a8397d6e70091a124de67/raw/f97e88a0b44d98bfa1258cb73c8afe4dce7afa87/i2c-amd-mp2.patch > > [2] > > https://gist.githubusercontent.com/conqp/67036e690aca89d08b958971edac283d/raw/2a1ef122f9c8c8e07164b6d597962ce7bbad6d45/amd-sfhtp.patch > > Thanks for the patch and appreciate your efforts. At this point of > time, we would like to have our first patch for SFH to be upstreamed > and dont want to complicate the > > with two sub systems and maintainers. Surely will consider this input > for future enhancement. Thanks for your understanding. > > Thanks > > > Nehal > > Hi all, I spent the last two weeks learning the HID API and refactored [1] the patch series again. I also took into consideration Nehal's input regarding the two subsystems. After tinkering with the integration of the SFH PCI driver into the existing i2c-amd-mp2-pci driver, I realized that this was a futile effort at this point of time, since the SFH protocol, as far as I understand it, does not really implement the I2C protocol. Thusly I turned away from trying to integrate the two drivers. The core changes I did, is to refactor the PCI driver to use a similar initialization as the i2c-amd-mp2-pci driver. The registers are now mapped correctly on initial setup (1 << 2) and not mapped to zero (1 >> 2) and the ioremapped later. With this change, the AMD-Vi page faults when not booting with amd_iommu=off also automagically disappeared. The second change is the HID device handling. I now implemented the hid_ll_driver API with the respective functions to process raw events as well device start / stop and open / close routines. I also refactored the platform driver wich is now solely responsible for spawning and despawning the HID devices. I also reduced the commonly used structures and constats to those that were actually being used by the driver and renamed the PCI registers to express what they are used for in this driver. I also commented most functions and shared structures. The only problem I keep having on my machine is, that the register for the bit-shifted active sensors mask 0x1068C keeps returning zero on readout and thusly I need to override it. I tried to get the sensor mask by sending the "dump_sensorinfo" and "number_of_sensors_discovered" commands to different C2P registers and dumping the corresponding P2C registers. But so far I could not get any information that could be interpreted as the sensor info and mask that I need. Maybe Nehal or Sandeep could provide the information, if and how it ts possible to get information about the amount, position and type of the connected sensors in another way. I also found out, by sweeping the SFH registers (from 0 to 31), that there seem to be other sensors connected on my machine, namely at positions 0, 1, 2, 3, 4, 7, 8 and 9. The ALS at 19 does not seem to be present on my system. I suspect that at least one of those is an additional accelerometer, since the only one working right now ist the accelerometer at position 0, which is located in the sceen / lid. I suspect that the other one is located in the chassis, allowing the use of both to perform tablet mode detection by processing their relative orientation. I'd appreciate a hint on this assumption as well. ;-) Sincere regards, Richard [1] https://gist.github.com/conqp/33baa079d9524914c4c0c196200e4f89
Hi Richard, On 4/13/2020 7:03 PM, Richard Neumann wrote: > Am Montag, den 06.04.2020, 10:56 +0530 schrieb Shah, Nehal- > bakulchandra: >> HI >> >> On 4/1/2020 9:58 PM, Richard Neumann wrote: >>> Am Dienstag, den 31.03.2020, 20:24 +0300 schrieb Andy Shevchenko: >>>> On Tue, Mar 31, 2020 at 4:26 PM Shah, Nehal-bakulchandra >>>> <nehal-bakulchandra.shah@amd.com> wrote: >>>>> On 3/31/2020 6:01 PM, Richard Neumann wrote: >>>>>> 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? >>>>> Thanks for the mail. Yes there are some common structures, >>>>> however >>>>> as of now we have kept separately considering both >>>>> >>>>> are part of different sub systems. But may be will consider >>>>> this >>>>> input for future enhancement. >>>> It can be done in a form of shared definitions at least in >>>> include/linux/platform_data/x86/amd-mp2.h or alike ... >>>> >>> I managed to add support for the AMD SFH PCI device to i2c-amd-mp2* >>> and >>> outsourced the headers to include/linux/i2c-amd-mp2.h. [1] >>> I also refactored the patch series (excluded the documentation) [2] >>> to >>> use the PCI device now provided by i2c_amd_mp2_pci and removed some >>> duplicate and unncessary code. >>> The driver now consist of just one module (amd_sfhtp_hid). >>> Unfortunately I was not able to solve the problem, that I get AMD- >>> Vi >>> IO_PAGE_FAULT errors when not booted with amd_iommu=off. >>> >>> [1] >>> https://gist.githubusercontent.com/conqp/4d726f86da8a8397d6e70091a124de67/raw/f97e88a0b44d98bfa1258cb73c8afe4dce7afa87/i2c-amd-mp2.patch >>> [2] >>> https://gist.githubusercontent.com/conqp/67036e690aca89d08b958971edac283d/raw/2a1ef122f9c8c8e07164b6d597962ce7bbad6d45/amd-sfhtp.patch >> Thanks for the patch and appreciate your efforts. At this point of >> time, we would like to have our first patch for SFH to be upstreamed >> and dont want to complicate the >> >> with two sub systems and maintainers. Surely will consider this input >> for future enhancement. Thanks for your understanding. >> >> Thanks >> >> >> Nehal >> >> > Hi all, > > I spent the last two weeks learning the HID API and refactored [1] the > patch series again. I also took into consideration Nehal's input > regarding the two subsystems. After tinkering with the integration of > the SFH PCI driver into the existing i2c-amd-mp2-pci driver, I realized > that this was a futile effort at this point of time, since the SFH > protocol, as far as I understand it, does not really implement the I2C > protocol. Thusly I turned away from trying to integrate the two > drivers. The core changes I did, is to refactor the PCI driver to use a > similar initialization as the i2c-amd-mp2-pci driver. The registers are > now mapped correctly on initial setup (1 << 2) and not mapped to zero > (1 >> 2) and the ioremapped later. With this change, the AMD-Vi page > faults when not booting with amd_iommu=off also automagically > disappeared. > The second change is the HID device handling. I now implemented the > hid_ll_driver API with the respective functions to process raw events > as well device start / stop and open / close routines. I also > refactored the platform driver wich is now solely responsible for > spawning and despawning the HID devices. I also reduced the commonly > used structures and constats to those that were actually being used by > the driver and renamed the PCI registers to express what they are used > for in this driver. I also commented most functions and shared > structures. The only problem I keep having on my machine is, that the > register for the bit-shifted active sensors mask 0x1068C keeps > returning zero on readout and thusly I need to override it. > I tried to get the sensor mask by sending the "dump_sensorinfo" and > "number_of_sensors_discovered" commands to different C2P registers and > dumping the corresponding P2C registers. But so far I could not get any > information that could be interpreted as the sensor info and mask that > I need. Maybe Nehal or Sandeep could provide the information, if and > how it ts possible to get information about the amount, position and > type of the connected sensors in another way. I also found out, by > sweeping the SFH registers (from 0 to 31), that there seem to be other > sensors connected on my machine, namely at positions 0, 1, 2, 3, 4, 7, > 8 and 9. The ALS at 19 does not seem to be present on my system. I > suspect that at least one of those is an additional accelerometer, > since the only one working right now ist the accelerometer at position > 0, which is located in the sceen / lid. I suspect that the other one is > located in the chassis, allowing the use of both to perform tablet mode > detection by processing their relative orientation. I'd appreciate a > hint on this assumption as well. ;-) > > Sincere regards, > > Richard > > [1] https://gist.github.com/conqp/33baa079d9524914c4c0c196200e4f89 Thanks for the refactoring the patch. The .raw_request in hid_ll_driver is not correct, the output of sensor is not raw but it is processed one, so better to move it to .request function. Regarding your question for sensor position well it comes from firmware. So i am not sure about the firmware for sensor , what you have.Also, regarding the IOMMU, we figured out the issue, earlier patch series has bug that during DMA allocation pdev was passed from platform driver context, where in for IOMMU needs context PCI pdev. So that is taken care in your refactored patch hence issue is disappeared. Now regarding the patch (with your changes) i need to validate at our side. Due to lockdown i dont have access to the system, so may be we can wait on that. Thanks Nehal Shah
Am Mittwoch, den 22.04.2020, 00:01 +0530 schrieb Shah, Nehal- bakulchandra: > Hi Richard, > > Thanks for the refactoring the patch. The .raw_request in > hid_ll_driver is not correct, the output of sensor is not raw but it > is processed one, > > so better to move it to .request function. Regarding your question > for sensor position well it comes from firmware. So i am not sure > about the firmware > > for sensor , what you have.Also, regarding the IOMMU, we figured out > the issue, earlier patch series has bug that during DMA allocation > pdev was passed > > from platform driver context, where in for IOMMU needs context PCI > pdev. So that is taken care in your refactored patch hence issue is > disappeared. > > > Now regarding the patch (with your changes) i need to validate at our > side. Due to lockdown i dont have access to the system, so may be we > can wait on that. > > > Thanks > > Nehal Shah Hi Nehal and thank you for the feedback. Regarding the raw_request / request, I think the "raw" here is meant to indicate, that the implementation is expected to write "raw bytes" [1] into a buffer rather than handling a "struct hid_report", which is exactly what the get_feature_report() and get_input_report() from the original patch seem to be intended to do. However it should be easy to migrate this to ".report", since it'd probably look a lot like __hid_request() [2], which is currently doing the magic automatically for me. Furthermore a ll_driver needs a raw_request implementation anyways as it's being tested for in hid_add_device() [3]. Regarding the validation on your side: Of course! I do in no way want to undermine your work on this in any way. I'm just offering what I've learned so far and what is working for me. If you find parts of it worth taking into a refactored version for upstream, awesome. If you don't agree with other parts, that's fine, too. I'm quite new to this and eager to learn. Also I am in no rush. Good things take time. In case you're missing my patch on gist.github.com, I decided to further develop the driver in a forked linux source tree [4]. It's on the branch amd-sfh. You can just diff to the master branch to get the entire patch. The changed files are still under drivers/hid/amd-sfh-hid and Documentation/hid respectively. You'll find that in the meantime I did some more work / learning and implemented some rudimentary IRQ handling. Kind regards, Richard [1] https://elixir.bootlin.com/linux/latest/source/include/linux/hid.h#L1070 [2] https://elixir.bootlin.com/linux/latest/source/drivers/hid/hid-core.c#L1688 [3] https://elixir.bootlin.com/linux/latest/source/drivers/hid/hid-core.c#L2386 [3] https://github.com/conqp/linux/tree/amd-sfh
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 <Shyam-sundar.S-k@amd.com> + * Nehal Bakulchandra Shah <Nehal-bakulchandra.Shah@amd.com> + */ + +#include <linux/interrupt.h> +#include <linux/module.h> +#include <linux/pci.h> +#include <linux/slab.h> +#include <linux/delay.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(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 <Shyam-sundar.S-k@amd.com>"); +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 <Shyam-sundar.S-k@amd.com> + * Nehal Bakulchandra Shah <Nehal-bakulchandra.Shah@amd.com> + */ + +#ifndef PCIE_MP2_AMD_H +#define PCIE_MP2_AMD_H + +#include <linux/pci.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 +#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