Re: [PATCH] SUNIX SDC PCIe multi-function card core driver

From: Enrico Weigelt, metux IT consult
Date: Mon Jun 28 2021 - 13:28:47 EST

Next message: Jason Gunthorpe: "Re: [PATCH v2] vfio/pci: Handle concurrent vma faults"
Previous message: Moritz Fischer: "Re: [PATCH v2 5/5] hwmon: intel-m10-bmc-hwmon: add n5010 sensors"
In reply to: Moriis Ku: "[PATCH] SUNIX SDC PCIe multi-function card core driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 25.06.21 10:55, Moriis Ku wrote:

Hi,

first of all: put all patches into one queue, so we know these things
belong together.

From: Morris Ku <saumah@xxxxxxxxx>

Add support for SUNIX SDC PCIe multi-function card

Cc: Jason Lee <jason_lee@xxxxxxxxx>
Cc: Taian Chen <taian.chen@xxxxxxxxx>
Cc: Morris Ku <morris_ku@xxxxxxxxx>
Signed-off-by: Morris Ku <saumah@xxxxxxxxx>
---
sunix-sdc.c | 606 ++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 606 insertions(+)
create mode 100644 sunix-sdc.c

diff --git a/sunix-sdc.c b/sunix-sdc.c
new file mode 100644
index 0000000..5e724a3
--- /dev/null
+++ b/sunix-sdc.c
@@ -0,0 +1,606 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * SUNIX SDC PCIe multi-function card core support.
+ *
+ * Copyright (C) 2021, SUNIX Co., Ltd.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+#include <linux/io.h>
+#include <linux/property.h>
+#include <linux/debugfs.h>
+#include <linux/idr.h>
+#include <linux/mfd/core.h>
+#include "sunix-sdc.h"
+
+#define DRIVER_NAME "sunix-sdc"
+
+enum cib_type {
+ TYPE_CONFIG = 0,
+ TYPE_UART,
+ TYPE_DIO,
+ TYPE_CAN
+};

What is a "CIB" ?

+struct cib_config {
+ u32 mem_offset;
+ u32 mem_size;
+ u8 ic_brand;
+ u8 ic_model;
+};
+
+struct cib_uart {
+ u32 io_offset;
+ u8 io_size;
+ u32 mem_offset;
+ u32 mem_size;
+ u16 tx_fifo_size;
+ u16 rx_fifo_size;
+ u32 significand;
+ u8 exponent;
+ u32 capability;
+};
+
+struct cib_dio_bank {
+ u8 number_of_io;
+ char number_of_io_name[32];
+ u8 capability;
+ char capability_name[32];
+};
+
+struct cib_dio {
+ u32 mem_offset;
+ u32 mem_size;
+ u8 number_of_bank;
+ u8 capability;
+
+ struct cib_dio_bank *banks;
+};
+
+struct cib_can {
+ u32 mem_offset;
+ u32 mem_size;
+ u32 significand;
+ u8 exponent;
+ u8 number_of_device;
+ u8 device_type;
+ u8 gpio_input;
+ u8 gpio_output;
+};
+
+struct cib_info {
+ u8 number;
+ enum cib_type type;
+ u8 version;
+ u8 total_length;
+ u8 resource_cap;
+ u8 event_type;
+
+ struct cib_config *config;
+ struct cib_uart *uart;
+ struct cib_dio *dio;
+ struct cib_can *can;
+};
+
+struct sdc_channel {
+ struct cib_info info;
+
+ struct property_entry *property;
+ struct resource *resource;
+ struct mfd_cell *cell;
+};
+
+struct sdc_board {
+ struct sunix_sdc_platform_info *info;
+
+ u8 major_version;
+ u8 minor_version;
+ u8 available_chls;
+ u8 total_length;
+ char model_name[16];
+
+ struct sdc_channel *channels;
+ struct device *dev;
+ int id;
+
+ struct debugfs_blob_wrapper debugfs_model_name;
+};

I think it might be better splitting this big thing apart into several
drivers for each sub device and have frontend driver(s) that does the
probing / instantiation of the individual components. Note that not
everybody has all the corresponding subsystems enabled.

+static int sdc_board_id = 1;
+static int sdc_uart_id = 1;
+static int sdc_dio_id = 1;
+static int sdc_can_id = 1;

What exactly are these global singletons for ?
Usually device specific data belongs into the device's private data,
except there's a damn good reason.

<snip>

+static void sdc_get_dio_info(struct cib_dio *dio, void __iomem *base,
+ u16 ptr)
+{
+ u32 temp;

what exactly is an "dio" ? do you mean gpio ?

<snip>

+int sunix_sdc_probe(struct device *dev, struct sunix_sdc_platform_info *info)
+{

Doesn't really look like some device probe function. Who calls that and
who fills this struct ?

+ struct sdc_channel *chl;
+ struct sdc_board *sdc;
+ struct cib_dio *dio;
+ struct cib_can *can;
+ void __iomem *mem_base;
+ u16 chl_offset_backup;
+ u16 chl_offset;
+ u32 temp;
+ u8 type;
+ int index;
+ int ret;
+ int i;
+ int j;
+
+ if (!info || !info->b1_io || !info->b2_mem || info->irq <= 0)
+ return -EINVAL;
+
+ sdc = devm_kzalloc(dev, sizeof(*sdc), GFP_KERNEL);
+ if (!sdc)
+ return -ENOMEM;
+
+ mem_base = devm_ioremap(dev, info->b2_mem->start, resource_size(info->b2_mem));
+ if (!mem_base)
+ return -ENOMEM;
+
+ sdc->info = info;
+ sdc->dev = dev;
+ sdc->id = sdc_board_id++;
+
+ temp = sdc_readl(mem_base, 0, 0);
+ sdc->major_version = temp & GENMASK(7, 0);
+ sdc->minor_version = (temp & GENMASK(15, 8)) >> 8;
+ sdc->available_chls = (temp & GENMASK(23, 16)) >> 16;
+ sdc->total_length = (temp & GENMASK(31, 24)) >> 24;
+
+ temp = sdc_readl(mem_base, 0, 1);
+ chl_offset = chl_offset_backup = temp & GENMASK(15, 0);
+
+ j = 0;
+ for (i = 0; i < 4; i++) {
+ temp = sdc_readl(mem_base, 0, 2 + i);
+ sdc->model_name[j++] = temp & GENMASK(7, 0);
+ sdc->model_name[j++] = (temp & GENMASK(15, 8)) >> 8;
+ sdc->model_name[j++] = (temp & GENMASK(23, 16)) >> 16;
+ sdc->model_name[j++] = (temp & GENMASK(31, 24)) >> 24;
+ }
+ sdc->model_name[strlen(sdc->model_name)] = '\n';
+
+ sdc->channels = devm_kcalloc(dev, sdc->available_chls,
+ sizeof(struct sdc_channel), GFP_KERNEL);
+ if (!sdc->channels)
+ return -ENOMEM;
+
+ for (i = 0; i < sdc->available_chls; i++) {
+ chl = &sdc->channels[i];
+ chl_offset_backup = chl_offset;
+
+ temp = sdc_readl(mem_base, chl_offset, 0);
+ chl->info.number = temp & GENMASK(7, 0);
+ type = (temp & GENMASK(15, 8)) >> 8;
+ switch (type) {
+ case 0x00:
+ case 0x01:
+ case 0x02:
+ case 0x03:
+ chl->info.type = (enum cib_type)type;
+ break;
+ default:
+ break;
+ }
+ chl->info.version = (temp & GENMASK(23, 16)) >> 16;
+ chl->info.total_length = (temp & GENMASK(31, 24)) >> 24;
+
+ temp = sdc_readl(mem_base, chl_offset, 1);
+ chl_offset = temp & GENMASK(15, 0);
+ chl->info.resource_cap = (temp & GENMASK(23, 16)) >> 16;
+ chl->info.event_type = (temp & GENMASK(31, 24)) >> 24;
+
+ switch (chl->info.type) {
+ case TYPE_CONFIG:
+ chl->info.config = devm_kzalloc(dev,
+ sizeof(*chl->info.config), GFP_KERNEL);
+ if (!chl->info.config)
+ return -ENOMEM;
+ sdc_get_config_info(chl->info.config, mem_base,
+ chl_offset_backup);
+ break;
+
+ case TYPE_UART:
+ chl->info.uart = devm_kzalloc(dev,
+ sizeof(*chl->info.uart), GFP_KERNEL);
+ if (!chl->info.uart)
+ return -ENOMEM;
+ sdc_get_uart_info(chl->info.uart, mem_base,
+ chl_offset_backup);
+
+ chl->property = devm_kcalloc(dev, 8,
+ sizeof(struct property_entry), GFP_KERNEL);
+ if (!chl->property)
+ return -ENOMEM;
+ index = 0;
+ chl->property[index++] = PROPERTY_ENTRY_U32(
+ "board_id", sdc->id);
+ chl->property[index++] = PROPERTY_ENTRY_U8(
+ "chl_number", chl->info.number);
+ chl->property[index++] = PROPERTY_ENTRY_U8(
+ "version", chl->info.version);
+ chl->property[index++] = PROPERTY_ENTRY_U16(
+ "tx_fifo_size", chl->info.uart->tx_fifo_size);
+ chl->property[index++] = PROPERTY_ENTRY_U16(
+ "rx_fifo_size", chl->info.uart->rx_fifo_size);
+ chl->property[index++] = PROPERTY_ENTRY_U32(
+ "significand", chl->info.uart->significand);
+ chl->property[index++] = PROPERTY_ENTRY_U8(
+ "exponent", chl->info.uart->exponent);
+ chl->property[index++] = PROPERTY_ENTRY_U32(
+ "capability", chl->info.uart->capability);
+
+ chl->resource = devm_kcalloc(dev, 2,
+ sizeof(struct resource), GFP_KERNEL);
+ if (!chl->resource)
+ return -ENOMEM;
+ chl->resource[0].start = info->b1_io->start +
+ chl->info.uart->io_offset;
+ chl->resource[0].end = chl->resource[0].start +
+ chl->info.uart->io_size - 1;
+ chl->resource[0].name = "8250_sdc";
+ chl->resource[0].flags = IORESOURCE_IO;
+ chl->resource[0].desc = IORES_DESC_NONE;
+ chl->resource[1].start = 0;
+ chl->resource[1].end = 0;
+ chl->resource[1].name = "irq";
+ chl->resource[1].flags = IORESOURCE_IRQ;
+ chl->resource[1].desc = IORES_DESC_NONE;

Since this thing is initialized from somewhere else, why not directly passing in struct resource instances ?

+ chl->cell = devm_kzalloc(dev,
+ sizeof(struct mfd_cell), GFP_KERNEL);

MFD ? Seriously ?

+ if (!chl->cell)
+ return -ENOMEM;
+ chl->cell->name = "8250_sdc";
+ chl->cell->id = sdc_uart_id++;
+ chl->cell->properties = chl->property;
+ chl->cell->num_resources = 2;
+ chl->cell->resources = chl->resource;
+ break;
+
+ case TYPE_DIO:
+ chl->info.dio = devm_kzalloc(dev,
+ sizeof(*chl->info.dio), GFP_KERNEL);
+ if (!chl->info.dio)
+ return -ENOMEM;
+ dio = chl->info.dio;
+ sdc_get_dio_info(dio, mem_base, chl_offset_backup);
+ if (dio->number_of_bank) {
+ dio->banks =
+ devm_kcalloc(dev, dio->number_of_bank,
+ sizeof(*dio->banks), GFP_KERNEL);
+ if (!dio->banks)
+ return -ENOMEM;
+
+ sdc_get_dio_banks_info(dio, mem_base,
+ chl_offset_backup);
+ }
+
+ chl->property = devm_kcalloc(dev, 5 + dio->number_of_bank * 2,
+ sizeof(struct property_entry), GFP_KERNEL);
+ if (!chl->property)
+ return -ENOMEM;
+ index = 0;
+ chl->property[index++] = PROPERTY_ENTRY_U32(
+ "board_id", sdc->id);
+ chl->property[index++] = PROPERTY_ENTRY_U8(
+ "chl_number", chl->info.number);
+ chl->property[index++] = PROPERTY_ENTRY_U8(
+ "version", chl->info.version);
+ chl->property[index++] = PROPERTY_ENTRY_U8(
+ "number_of_bank", dio->number_of_bank);
+ chl->property[index++] = PROPERTY_ENTRY_U8(
+ "capability", dio->capability);
+ for (j = 0; j < dio->number_of_bank; j++) {
+ snprintf(dio->banks[j].number_of_io_name,
+ sizeof(dio->banks[j].number_of_io_name),
+ "b%d_number_of_io", j);
+ chl->property[index++] = PROPERTY_ENTRY_U8(
+ dio->banks[j].number_of_io_name, dio->banks[j].number_of_io);
+ snprintf(dio->banks[j].capability_name,
+ sizeof(dio->banks[j].capability_name),
+ "b%d_capability", j);
+ chl->property[index++] = PROPERTY_ENTRY_U8(
+ dio->banks[j].capability_name, dio->banks[j].capability);
+ }
+
+ chl->resource = devm_kcalloc(dev, 4,
+ sizeof(struct resource), GFP_KERNEL);
+ if (!chl->resource)
+ return -ENOMEM;
+ chl->resource[0].start = info->b2_mem->start +
+ chl->info.dio->mem_offset;
+ chl->resource[0].end = chl->resource[0].start +
+ chl->info.dio->mem_size - 1;
+ chl->resource[0].name = "gpio_sdc";
+ chl->resource[0].flags = IORESOURCE_MEM;
+ chl->resource[0].desc = IORES_DESC_NONE;
+ chl->resource[1].start = 0;
+ chl->resource[1].end = 0;
+ chl->resource[1].name = "irq";
+ chl->resource[1].flags = IORESOURCE_IRQ;
+ chl->resource[1].desc = IORES_DESC_NONE;
+ chl->resource[2].start = info->b0_mem->start;
+ chl->resource[2].end = chl->resource[2].start + 32 - 1;
+ chl->resource[2].name = "sdc_irq_vector";
+ chl->resource[2].flags = IORESOURCE_MEM;
+ chl->resource[2].desc = IORES_DESC_NONE;
+ chl->resource[3].start = info->b0_mem->start + 32 +
+ (chl->info.number * 4);
+ chl->resource[3].end = chl->resource[3].start + 4 - 1;
+ chl->resource[3].name = "gpio_sdc_event_header";
+ chl->resource[3].flags = IORESOURCE_MEM;
+ chl->resource[3].desc = IORES_DESC_NONE;
+
+ chl->cell = devm_kzalloc(dev,
+ sizeof(struct mfd_cell), GFP_KERNEL);
+ if (!chl->cell)
+ return -ENOMEM;
+ chl->cell->name = "gpio_sdc";
+ chl->cell->id = sdc_dio_id++;
+ chl->cell->properties = chl->property;
+ chl->cell->num_resources = 4;
+ chl->cell->resources = chl->resource;
+ break;
+
+ case TYPE_CAN:
+ chl->info.can = devm_kzalloc(dev,
+ sizeof(*chl->info.can), GFP_KERNEL);
+ if (!chl->info.can)
+ return -ENOMEM;
+ can = chl->info.can;
+ sdc_get_can_info(can, mem_base, chl_offset_backup);
+
+ if (can->number_of_device != 1 && can->device_type != 0x03)
+ break;
+
+ chl->property = devm_kcalloc(dev, 7,
+ sizeof(struct property_entry), GFP_KERNEL);
+ if (!chl->property)
+ return -ENOMEM;
+ index = 0;
+ chl->property[index++] = PROPERTY_ENTRY_U32(
+ "board_id", sdc->id);
+ chl->property[index++] = PROPERTY_ENTRY_U8(
+ "chl_number", chl->info.number);
+ chl->property[index++] = PROPERTY_ENTRY_U8(
+ "version", chl->info.version);
+ chl->property[index++] = PROPERTY_ENTRY_U32(
+ "significand", can->significand);
+ chl->property[index++] = PROPERTY_ENTRY_U8(
+ "exponent", can->exponent);
+ chl->property[index++] = PROPERTY_ENTRY_U8(
+ "gpio_input", can->gpio_input);
+ chl->property[index++] = PROPERTY_ENTRY_U8(
+ "gpio_output", can->gpio_output);
+
+ chl->resource = devm_kcalloc(dev, 2,
+ sizeof(struct resource), GFP_KERNEL);
+ if (!chl->resource)
+ return -ENOMEM;
+ chl->resource[0].start = info->b2_mem->start +
+ chl->info.can->mem_offset;
+ chl->resource[0].end = chl->resource[0].start +
+ chl->info.can->mem_size - 1;
+ chl->resource[0].name = "sx2010_can";
+ chl->resource[0].flags = IORESOURCE_MEM;
+ chl->resource[0].desc = IORES_DESC_NONE;
+ chl->resource[1].start = 0;
+ chl->resource[1].end = 0;
+ chl->resource[1].name = "irq";
+ chl->resource[1].flags = IORESOURCE_IRQ;
+ chl->resource[1].desc = IORES_DESC_NONE;
+
+ chl->cell = devm_kzalloc(dev,
+ sizeof(struct mfd_cell), GFP_KERNEL);
+ if (!chl->cell)
+ return -ENOMEM;
+ chl->cell->name = "sx2010_can";
+ chl->cell->id = sdc_can_id++;
+ chl->cell->properties = chl->property;
+ chl->cell->num_resources = 2;
+ chl->cell->resources = chl->resource;
+ break;
+
+ default:
+ break;
+ }
+ }
+
+ dev_set_drvdata(dev, sdc);
+ sdc_debugfs_add(sdc);
+
+ for (i = 0; i < sdc->available_chls; i++) {
+ chl = &sdc->channels[i];
+
+ if (chl->cell) {
+ ret = mfd_add_devices(dev, sdc->id, chl->cell, 1,
+ NULL, sdc->info->irq, NULL);
+ if (ret)
+ goto err_remove_sdc;
+ }
+ }
+
+ dev_pm_set_driver_flags(dev, DPM_FLAG_SMART_SUSPEND);
+ return 0;
+
+err_remove_sdc:
+ sdc_debugfs_remove(sdc);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(sunix_sdc_probe);

I really wonder what this functions is actually for. Doesn't seem to be
some driver probe function. Doesn't seem to create any devices.

+void sunix_sdc_remove(struct device *dev)
+{
+ struct sdc_board *sdc = dev_get_drvdata(dev);
+
+ mfd_remove_devices(dev);
+ sdc_debugfs_remove(sdc);
+}
+EXPORT_SYMBOL_GPL(sunix_sdc_remove);
+
+static int __init sunix_sdc_init(void)
+{
+ sdc_debugfs_init();
+ return 0;
+}
+module_init(sunix_sdc_init);
+
+static void __exit sunix_sdc_exit(void)
+{
+ sdc_debugfs_exit();
+}
+module_exit(sunix_sdc_exit);
+
+MODULE_AUTHOR("Jason Lee <jason_lee@xxxxxxxxx>");
+MODULE_DESCRIPTION("SUNIX SDC PCIe multi-function card core driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:" DRIVER_NAME);

Sorry, but it's totally unclear to me what this code is actually
supposed to do.

Please submit a complete queue that does something actually useful and
give some more explainations.

--mtx

--
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@xxxxxxxxx -- +49-151-27565287

Next message: Jason Gunthorpe: "Re: [PATCH v2] vfio/pci: Handle concurrent vma faults"
Previous message: Moritz Fischer: "Re: [PATCH v2 5/5] hwmon: intel-m10-bmc-hwmon: add n5010 sensors"
In reply to: Moriis Ku: "[PATCH] SUNIX SDC PCIe multi-function card core driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]