RE: [RFC v1 3/6] platform/x86: intel_pmc_ipc: Use MFD framework to create dependent devices
From: Chakravarty, Souvik K
Date: Wed Aug 02 2017 - 01:14:20 EST
> -----Original Message-----
> From: platform-driver-x86-owner@xxxxxxxxxxxxxxx [mailto:platform-driver-
> x86-owner@xxxxxxxxxxxxxxx] On Behalf Of
> sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx
> Sent: Tuesday, August 1, 2017 11:44 PM
> To: x86@xxxxxxxxxx; mingo@xxxxxxxxxx; Zha, Qipeng
> <qipeng.zha@xxxxxxxxx>; hpa@xxxxxxxxx; dvhart@xxxxxxxxxxxxx;
> tglx@xxxxxxxxxxxxx; andy@xxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx; platform-driver-x86@xxxxxxxxxxxxxxx;
> sathyaosid@xxxxxxxxx; Kuppuswamy Sathyanarayanan
> <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
> Subject: [RFC v1 3/6] platform/x86: intel_pmc_ipc: Use MFD framework to
> create dependent devices
>
> From: Kuppuswamy Sathyanarayanan
> <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
>
> Currently, we have lot of repetitive code in dependent device resource
> allocation and device creation handling code. This logic can be improved if
> we use MFD framework for dependent device creation. This patch adds this
> support.
>
> Signed-off-by: Kuppuswamy Sathyanarayanan
> <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
> ---
> drivers/platform/x86/intel_pmc_ipc.c | 376 +++++++++++++-------------------
> ---
> 1 file changed, 141 insertions(+), 235 deletions(-)
>
> diff --git a/drivers/platform/x86/intel_pmc_ipc.c
> b/drivers/platform/x86/intel_pmc_ipc.c
> index ad0416e..43533ec 100644
> --- a/drivers/platform/x86/intel_pmc_ipc.c
> +++ b/drivers/platform/x86/intel_pmc_ipc.c
> @@ -33,6 +33,7 @@
> #include <linux/suspend.h>
> #include <linux/acpi.h>
> #include <linux/io-64-nonatomic-lo-hi.h>
> +#include <linux/mfd/core.h>
>
> #include <asm/intel_pmc_ipc.h>
>
> @@ -105,8 +106,6 @@
> #define TELEM_SSRAM_SIZE 240
> #define TELEM_PMC_SSRAM_OFFSET 0x1B00
> #define TELEM_PUNIT_SSRAM_OFFSET 0x1A00
> -#define TCO_PMC_OFFSET 0x8
> -#define TCO_PMC_SIZE 0x4
>
> /* PMC register bit definitions */
>
> @@ -115,6 +114,13 @@
> #define PMC_CFG_NO_REBOOT_EN (1 << 4)
> #define PMC_CFG_NO_REBOOT_DIS (0 << 4)
>
> +enum {
> + PMC_IPC_PUNIT_MFD_BLOCK,
> + PMC_IPC_WATCHDOG_MFD_BLOCK,
> + PMC_IPC_TELEMETRY_MFD_BLOCK,
> + PMC_IPC_MAX_MFD_BLOCK
> +};
> +
> static struct intel_pmc_ipc_dev {
> struct device *dev;
> void __iomem *ipc_base;
> @@ -123,25 +129,12 @@ static struct intel_pmc_ipc_dev {
> int cmd;
> struct completion cmd_complete;
>
> - /* The following PMC BARs share the same ACPI device with the IPC
> */
> - resource_size_t acpi_io_base;
> - int acpi_io_size;
> - struct platform_device *tco_dev;
> -
> /* gcr */
> void __iomem *gcr_mem_base;
> bool has_gcr_regs;
>
> - /* punit */
> - struct platform_device *punit_dev;
> -
> /* Telemetry */
> - resource_size_t telem_pmc_ssram_base;
> - resource_size_t telem_punit_ssram_base;
> - int telem_pmc_ssram_size;
> - int telem_punit_ssram_size;
> u8 telem_res_inval;
> - struct platform_device *telemetry_dev;
> } ipcdev;
>
> static char *ipc_err_sources[] = {
> @@ -613,7 +606,7 @@ static const struct attribute_group intel_ipc_group
> = {
> .attrs = intel_ipc_attrs,
> };
>
> -static struct resource punit_res_array[] = {
> +static struct resource punit_ipc_resources[] = {
> /* Punit BIOS */
> {
> .flags = IORESOURCE_MEM,
> @@ -637,10 +630,7 @@ static struct resource punit_res_array[] = {
> },
> };
>
> -#define TCO_RESOURCE_ACPI_IO 0
> -#define TCO_RESOURCE_SMI_EN_IO 1
> -#define TCO_RESOURCE_GCR_MEM 2
> -static struct resource tco_res[] = {
> +static struct resource watchdog_ipc_resources[] = {
> /* ACPI - TCO */
> {
> .flags = IORESOURCE_IO,
> @@ -658,9 +648,7 @@ static struct itco_wdt_platform_data tco_info = {
> .update_no_reboot_bit = update_no_reboot_bit, };
>
> -#define TELEMETRY_RESOURCE_PUNIT_SSRAM 0
> -#define TELEMETRY_RESOURCE_PMC_SSRAM 1
> -static struct resource telemetry_res[] = {
> +static struct resource telemetry_ipc_resources[] = {
> /*Telemetry*/
> {
> .flags = IORESOURCE_MEM,
> @@ -670,224 +658,152 @@ static struct resource telemetry_res[] = {
> },
> };
>
> -static int ipc_create_punit_device(void) -{
> - struct platform_device *pdev;
> - const struct platform_device_info pdevinfo = {
> - .parent = ipcdev.dev,
> - .name = PUNIT_DEVICE_NAME,
> - .id = -1,
> - .res = punit_res_array,
> - .num_res = ARRAY_SIZE(punit_res_array),
> - };
> -
> - pdev = platform_device_register_full(&pdevinfo);
> - if (IS_ERR(pdev))
> - return PTR_ERR(pdev);
> -
> - ipcdev.punit_dev = pdev;
> -
> - return 0;
> -}
> -
> -static int ipc_create_tco_device(void)
> -{
> - struct platform_device *pdev;
> - struct resource *res;
> - const struct platform_device_info pdevinfo = {
> - .parent = ipcdev.dev,
> - .name = TCO_DEVICE_NAME,
> - .id = -1,
> - .res = tco_res,
> - .num_res = ARRAY_SIZE(tco_res),
> - .data = &tco_info,
> - .size_data = sizeof(tco_info),
> - };
> -
> - res = tco_res + TCO_RESOURCE_ACPI_IO;
> - res->start = ipcdev.acpi_io_base + TCO_BASE_OFFSET;
> - res->end = res->start + TCO_REGS_SIZE - 1;
> -
> - res = tco_res + TCO_RESOURCE_SMI_EN_IO;
> - res->start = ipcdev.acpi_io_base + SMI_EN_OFFSET;
> - res->end = res->start + SMI_EN_SIZE - 1;
> -
> - pdev = platform_device_register_full(&pdevinfo);
> - if (IS_ERR(pdev))
> - return PTR_ERR(pdev);
> -
> - ipcdev.tco_dev = pdev;
> -
> - return 0;
> -}
> -
> -static int ipc_create_telemetry_device(void) -{
> - struct platform_device *pdev;
> - struct resource *res;
> - const struct platform_device_info pdevinfo = {
> - .parent = ipcdev.dev,
> - .name = TELEMETRY_DEVICE_NAME,
> - .id = -1,
> - .res = telemetry_res,
> - .num_res = ARRAY_SIZE(telemetry_res),
> - };
> -
> - res = telemetry_res + TELEMETRY_RESOURCE_PUNIT_SSRAM;
> - res->start = ipcdev.telem_punit_ssram_base;
> - res->end = res->start + ipcdev.telem_punit_ssram_size - 1;
> -
> - res = telemetry_res + TELEMETRY_RESOURCE_PMC_SSRAM;
> - res->start = ipcdev.telem_pmc_ssram_base;
> - res->end = res->start + ipcdev.telem_pmc_ssram_size - 1;
> -
> - pdev = platform_device_register_full(&pdevinfo);
> - if (IS_ERR(pdev))
> - return PTR_ERR(pdev);
> -
> - ipcdev.telemetry_dev = pdev;
> -
> - return 0;
> -}
> -
> -static int ipc_create_pmc_devices(void)
> +static int ipc_create_pmc_devices(struct platform_device *pdev)
> {
> - int ret;
> -
> - /* If we have ACPI based watchdog use that instead */
> + u8 n = 0;
> + struct mfd_cell *pmc_mfd_cells;
> +
> + pmc_mfd_cells = devm_kzalloc(&pdev->dev,
> + (sizeof(*pmc_mfd_cells) *
> PMC_IPC_MAX_MFD_BLOCK),
> + GFP_KERNEL);
> + if (!pmc_mfd_cells)
> + return -ENOMEM;
> +
> + /* Create PUNIT IPC MFD cell */
> + pmc_mfd_cells[n].name = PUNIT_DEVICE_NAME;
> + pmc_mfd_cells[n].id = -1;
> + pmc_mfd_cells[n].num_resources =
> ARRAY_SIZE(punit_ipc_resources);
> + pmc_mfd_cells[n].resources = punit_ipc_resources;
> + pmc_mfd_cells[n].ignore_resource_conflicts = 1;
> + n++;
Since it's not really a loop, cant the enum values of MFD block be used instead of 'n'? Will be more readable/intuitive.
> +
> + /* If we have ACPI based watchdog use that instead, othewise
> create
> + * a MFD cell for iTCO watchdog*/
> if (!acpi_has_watchdog()) {
> - ret = ipc_create_tco_device();
> - if (ret) {
> - dev_err(ipcdev.dev, "Failed to add tco platform
> device\n");
> - return ret;
> - }
> - }
> -
> - ret = ipc_create_punit_device();
> - if (ret) {
> - dev_err(ipcdev.dev, "Failed to add punit platform
> device\n");
> - platform_device_unregister(ipcdev.tco_dev);
> + pmc_mfd_cells[n].name = TCO_DEVICE_NAME;
> + pmc_mfd_cells[n].id = -1;
> + pmc_mfd_cells[n].platform_data = &tco_info;
> + pmc_mfd_cells[n].pdata_size = sizeof(tco_info);
> + pmc_mfd_cells[n].num_resources =
> + ARRAY_SIZE(watchdog_ipc_resources);
> + pmc_mfd_cells[n].resources = watchdog_ipc_resources;
> + pmc_mfd_cells[n].ignore_resource_conflicts = 1;
> + n++;
Same as above.
> }
>
> if (!ipcdev.telem_res_inval) {
> - ret = ipc_create_telemetry_device();
> - if (ret)
> - dev_warn(ipcdev.dev,
> - "Failed to add telemetry platform
> device\n");
> + pmc_mfd_cells[n].name = TELEMETRY_DEVICE_NAME;
> + pmc_mfd_cells[n].id = -1;
> + pmc_mfd_cells[n].num_resources =
> + ARRAY_SIZE(telemetry_ipc_resources);
> + pmc_mfd_cells[n].resources = telemetry_ipc_resources;
> + pmc_mfd_cells[n].ignore_resource_conflicts = 1;
> + n++;
Same as above.
> }
>
> - return ret;
> + return devm_mfd_add_devices(&pdev->dev, -1, pmc_mfd_cells, n,
> NULL,
> + 0, NULL);
> }
>
> static int ipc_plat_get_res(struct platform_device *pdev) {
> - struct resource *res, *punit_res;
> + struct resource *res;
> void __iomem *addr;
> - int size;
> + int mindex, pindex = 0;
>
> + /* Get iTCO watchdog resources */
> res = platform_get_resource(pdev, IORESOURCE_IO,
> PLAT_RESOURCE_ACPI_IO_INDEX);
> if (!res) {
> dev_err(&pdev->dev, "Failed to get io resource\n");
> return -ENXIO;
> }
> - size = resource_size(res);
> - ipcdev.acpi_io_base = res->start;
> - ipcdev.acpi_io_size = size;
> - dev_info(&pdev->dev, "io res: %pR\n", res);
> -
> - punit_res = punit_res_array;
> - /* This is index 0 to cover BIOS data register */
> - res = platform_get_resource(pdev, IORESOURCE_MEM,
> - PLAT_RESOURCE_BIOS_DATA_INDEX);
> - if (!res) {
> - dev_err(&pdev->dev, "Failed to get res of punit BIOS
> data\n");
> - return -ENXIO;
> - }
> - *punit_res = *res;
> - dev_info(&pdev->dev, "punit BIOS data res: %pR\n", res);
> -
> - /* This is index 1 to cover BIOS interface register */
> - res = platform_get_resource(pdev, IORESOURCE_MEM,
> - PLAT_RESOURCE_BIOS_IFACE_INDEX);
> - if (!res) {
> - dev_err(&pdev->dev, "Failed to get res of punit BIOS
> iface\n");
> - return -ENXIO;
> - }
> - *++punit_res = *res;
> - dev_info(&pdev->dev, "punit BIOS interface res: %pR\n", res);
> -
> - /* This is index 2 to cover ISP data register, optional */
> - res = platform_get_resource(pdev, IORESOURCE_MEM,
> - PLAT_RESOURCE_ISP_DATA_INDEX);
> - ++punit_res;
> - if (res) {
> - *punit_res = *res;
> - dev_info(&pdev->dev, "punit ISP data res: %pR\n", res);
> - }
>
> - /* This is index 3 to cover ISP interface register, optional */
> - res = platform_get_resource(pdev, IORESOURCE_MEM,
> - PLAT_RESOURCE_ISP_IFACE_INDEX);
> - ++punit_res;
> - if (res) {
> - *punit_res = *res;
> - dev_info(&pdev->dev, "punit ISP interface res: %pR\n", res);
> - }
> -
> - /* This is index 4 to cover GTD data register, optional */
> - res = platform_get_resource(pdev, IORESOURCE_MEM,
> - PLAT_RESOURCE_GTD_DATA_INDEX);
> - ++punit_res;
> - if (res) {
> - *punit_res = *res;
> - dev_info(&pdev->dev, "punit GTD data res: %pR\n", res);
> - }
> -
> - /* This is index 5 to cover GTD interface register, optional */
> - res = platform_get_resource(pdev, IORESOURCE_MEM,
> - PLAT_RESOURCE_GTD_IFACE_INDEX);
> - ++punit_res;
> - if (res) {
> - *punit_res = *res;
> - dev_info(&pdev->dev, "punit GTD interface res: %pR\n",
> res);
> - }
> -
> - res = platform_get_resource(pdev, IORESOURCE_MEM,
> - PLAT_RESOURCE_IPC_INDEX);
> - if (!res) {
> - dev_err(&pdev->dev, "Failed to get ipc resource\n");
> - return -ENXIO;
> - }
> - res->end = (res->start + PLAT_RESOURCE_IPC_SIZE +
> - PLAT_RESOURCE_GCR_SIZE - 1);
> -
> - addr = devm_ioremap_resource(&pdev->dev, res);
> - if (IS_ERR(addr)) {
> - dev_err(&pdev->dev,
> - "PMC I/O memory remapping failed\n");
> - return PTR_ERR(addr);
> - }
> -
> - ipcdev.ipc_base = addr;
> -
> - ipcdev.gcr_mem_base = addr + PLAT_RESOURCE_GCR_OFFSET;
> - dev_info(&pdev->dev, "ipc res: %pR\n", res);
> -
> - ipcdev.telem_res_inval = 0;
> - res = platform_get_resource(pdev, IORESOURCE_MEM,
> - PLAT_RESOURCE_TELEM_SSRAM_INDEX);
> - if (!res) {
> - dev_err(&pdev->dev, "Failed to get telemetry ssram
> resource\n");
> - ipcdev.telem_res_inval = 1;
> - } else {
> - ipcdev.telem_punit_ssram_base = res->start +
> -
> TELEM_PUNIT_SSRAM_OFFSET;
> - ipcdev.telem_punit_ssram_size = TELEM_SSRAM_SIZE;
> - ipcdev.telem_pmc_ssram_base = res->start +
> -
> TELEM_PMC_SSRAM_OFFSET;
> - ipcdev.telem_pmc_ssram_size = TELEM_SSRAM_SIZE;
> - dev_info(&pdev->dev, "telemetry ssram res: %pR\n", res);
> + watchdog_ipc_resources[0].start = res->start + TCO_BASE_OFFSET;
> + watchdog_ipc_resources[0].end = res->start +
> + TCO_BASE_OFFSET + TCO_REGS_SIZE - 1;
> + watchdog_ipc_resources[1].start = res->start + SMI_EN_OFFSET;
> + watchdog_ipc_resources[1].end = res->start +
> + SMI_EN_OFFSET + SMI_EN_SIZE - 1;
The 0/1 magic may be replaced with MACROS here and elsewhere.
> +
> + dev_info(&pdev->dev, "watchdog res 0: %pR\n",
> + &watchdog_ipc_resources[0]);
> + dev_info(&pdev->dev, "watchdog res 1: %pR\n",
> + &watchdog_ipc_resources[1]);
> +
> + for (mindex = 0; mindex <= PLAT_RESOURCE_GTD_IFACE_INDEX;
> mindex++) {
Here we are relying on the fact that the GTD_IFACE_INDEX is at offset 7. It's however not by design but a side-effect of how the BIOS places the resources.
So perhaps not a good idea to use it, since GTD_IFACE is not meant to be a terminator/max delimiter.
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM,
> mindex);
> +
> + switch (mindex) {
> + /* Get PUNIT resources */
> + case PLAT_RESOURCE_BIOS_DATA_INDEX:
> + case PLAT_RESOURCE_BIOS_IFACE_INDEX:
> + /* BIOS resources are required, so return error if not
> + * available */
> + if (!res) {
> + dev_err(&pdev->dev,
> + "Failed to get punit mem resource
> %d\n",
> + pindex);
> + return -ENXIO;
> + }
> + case PLAT_RESOURCE_ISP_DATA_INDEX:
> + case PLAT_RESOURCE_ISP_IFACE_INDEX:
> + case PLAT_RESOURCE_GTD_DATA_INDEX:
> + case PLAT_RESOURCE_GTD_IFACE_INDEX:
> + /* if not valid resource, skip the rest of steps */
> + if (!res) {
> + pindex++;
> + continue;
> + }
> + memcpy(&punit_ipc_resources[pindex], res,
> + sizeof(*res));
> + dev_info(&pdev->dev, "PUNIT memory res: %pR\n",
> + &punit_ipc_resources[pindex]);
> + pindex++;
> + break;
> + /* Get Telemetry resources */
> + case PLAT_RESOURCE_TELEM_SSRAM_INDEX:
> + if (!res) {
> + dev_warn(&pdev->dev,
> + "Failed to get telemtry sram res\n");
> + ipcdev.telem_res_inval = 1;
> + continue;
> + }
> + telemetry_ipc_resources[0].start = res->start +
> + TELEM_PUNIT_SSRAM_OFFSET;
> + telemetry_ipc_resources[0].end = res->start +
> + TELEM_PUNIT_SSRAM_OFFSET +
> TELEM_SSRAM_SIZE - 1;
> + telemetry_ipc_resources[1].start = res->start +
> + TELEM_PMC_SSRAM_OFFSET;
> + telemetry_ipc_resources[1].end = res->start +
> + TELEM_PMC_SSRAM_OFFSET +
> TELEM_SSRAM_SIZE - 1;
> +
> + dev_info(&pdev->dev, "telemetry punit ssram res:
> %pR\n",
> + &telemetry_ipc_resources[0]);
> + dev_info(&pdev->dev, "telemetry pmc ssram res:
> %pR\n",
> + &telemetry_ipc_resources[1]);
> + break;
> + /* Get IPC resources */
> + case PLAT_RESOURCE_IPC_INDEX:
> + if (!res) {
> + dev_err(&pdev->dev,
> + "Failed to get IPC resources\n");
> + return -ENXIO;
> + }
> + res->end = (res->start + PLAT_RESOURCE_IPC_SIZE +
> + PLAT_RESOURCE_GCR_SIZE - 1);
> + addr = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(addr)) {
> + dev_err(&pdev->dev,
> + "PMC I/O memory remapping
> failed\n");
> + return PTR_ERR(addr);
> + }
> + ipcdev.ipc_base = addr;
> + ipcdev.gcr_mem_base = addr +
> PLAT_RESOURCE_GCR_OFFSET;
> + dev_info(&pdev->dev, "PMC IPC resource %pR\n",
> res);
> + break;
> + };
> }
>
> return 0;
> @@ -943,7 +859,7 @@ static int ipc_plat_probe(struct platform_device
> *pdev)
> return ret;
> }
>
> - ret = ipc_create_pmc_devices();
> + ret = ipc_create_pmc_devices(pdev);
> if (ret) {
> dev_err(&pdev->dev, "Failed to create pmc devices\n");
> return ret;
> @@ -952,36 +868,26 @@ static int ipc_plat_probe(struct platform_device
> *pdev)
> if (devm_request_irq(&pdev->dev, ipcdev.irq, ioc,
> IRQF_NO_SUSPEND,
> "intel_pmc_ipc", &ipcdev)) {
> dev_err(&pdev->dev, "Failed to request irq\n");
> - ret = -EBUSY;
> - goto unregister_devices;
> + return -EBUSY;
> }
>
> ret = sysfs_create_group(&pdev->dev.kobj, &intel_ipc_group);
> if (ret) {
> dev_err(&pdev->dev, "Failed to create sysfs group %d\n",
> ret);
> - goto unregister_devices;
> + return ret;
> }
>
> ipcdev.has_gcr_regs = true;
>
> return 0;
> -
> -unregister_devices:
> - platform_device_unregister(ipcdev.tco_dev);
> - platform_device_unregister(ipcdev.punit_dev);
> - platform_device_unregister(ipcdev.telemetry_dev);
> -
> - return ret;
> }
>
> static int ipc_plat_remove(struct platform_device *pdev) {
> sysfs_remove_group(&pdev->dev.kobj, &intel_ipc_group);
> - platform_device_unregister(ipcdev.tco_dev);
> - platform_device_unregister(ipcdev.punit_dev);
> - platform_device_unregister(ipcdev.telemetry_dev);
> ipcdev.dev = NULL;
> +
> return 0;
> }
>
> --
> 2.7.4