Re: [PATCH RFC] Specifying default-disabled devices

[Zev Weiss <zweiss@equinix.com>]

On Fri, Sep 10, 2021 at 02:59:45PM PDT, Zev Weiss wrote:
>>>Well, I'm aiming to be able to use a dts fragment looking something
>>>like (on an ast2500):
>>>
>>>  &spi1 {
>>>        status = "reserved";
>>>        pinctrl-names = "default";
>>>        pinctrl-0 = <&pinctrl_spi1_default>;
>>>        flash@0 {
>>>                status = "okay";
>>>                label = "bios";
>>>                m25p,fast-read;
>>>        };
>>>  };
>>
>>[do you want just the flash node to be reserved, or the entire
>>controller? I assume the controller is always available...]
>>
>
>The flash node would make more sense, but thus far with my noautobind
>argument I've been doing it at the spi1 controller level because I don't
>know of a way to do the runtime attach/detach of individual flash
>devices behind the controller (the analog of doing the driver bind via
>sysfs), and from a glance at the aspeed-smc driver it doesn't look like
>there is one (the aspeed_smc_setup_flash() call in the controller's
>probe path seems to be the only path to instantiating any child
>devices).
>

Pursuing this angle a bit, however, I now have the below patch which
enables attaching & detaching individual flash chips behind an
aspeed-smc controller at runtime (via sysfs).

This has the downside of burying 'status = "reserved"' support in a
piece of somewhat niche functionality in one specific driver, rather
than for DT devices in general (though after investigating a bit, it
looks like even adding driver-core support would only cover devices that
get drivers bound, e.g. the aspeed-smc controller itself, and not any
child devices further down the tree, like individual flash chips).

That aside though, this seems to solve my problem in a fairly clean,
non-invasive manner that shouldn't cause any compatibility problems
elsewhere, which is appealing...a viable approach perhaps?


Zev

From b17ad478b9d2e8357461412f9d5d734a8ad8df0b Mon Sep 17 00:00:00 2001
From: Zev Weiss <zev@bewilderbeest.net>
Date: Fri, 17 Sep 2021 17:03:18 -0500
Subject: [PATCH] mtd: spi-nor: aspeed: make flash chips runtime
 attachable/detachable

There are now two new sysfs attributes, attach_chip and detach_chip,
into which a chip select number can be written to attach or detach the
corresponding chip.  Chips marked with a DT status of "reserved" are
left detached initially and can be attached on demand by userspace via
attach_chip.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 drivers/mtd/spi-nor/controllers/aspeed-smc.c | 166 +++++++++++++++----
 1 file changed, 138 insertions(+), 28 deletions(-)

diff --git a/drivers/mtd/spi-nor/controllers/aspeed-smc.c b/drivers/mtd/spi-nor/controllers/aspeed-smc.c
index c421fad4b3f5..db7cc8d2f4d0 100644
--- a/drivers/mtd/spi-nor/controllers/aspeed-smc.c
+++ b/drivers/mtd/spi-nor/controllers/aspeed-smc.c
@@ -190,6 +190,7 @@ struct aspeed_smc_controller;
 
 struct aspeed_smc_chip {
 	int cs;
+	bool attached;
 	struct aspeed_smc_controller *controller;
 	void __iomem *ctl;			/* control register */
 	void __iomem *ahb_base;			/* base of chip window */
@@ -207,12 +208,13 @@ struct aspeed_smc_controller {
 	const struct aspeed_smc_info *info;	/* type info of controller */
 	void __iomem *regs;			/* controller registers */
 	void __iomem *ahb_base;			/* per-chip window resource */
+	struct resource *ahb_res;		/* resource for AHB address space */
 	u32 ahb_base_phy;			/* phys addr of AHB window  */
 	u32 ahb_window_size;			/* full mapping window size */
 
 	unsigned long	clk_frequency;
 
-	struct aspeed_smc_chip *chips[];	/* pointers to attached chips */
+	struct aspeed_smc_chip *chips[];	/* pointers to connected chips */
 };
 
 #define ASPEED_SPI_DEFAULT_FREQ		50000000
@@ -619,15 +621,27 @@ static ssize_t aspeed_smc_read(struct spi_nor *nor, loff_t from, size_t len,
 	return len;
 }
 
+static int aspeed_smc_unregister_chip(struct aspeed_smc_chip *chip)
+{
+	int ret = mtd_device_unregister(&chip->nor.mtd);
+	if (!ret)
+		chip->attached = false;
+	return ret;
+}
+
 static int aspeed_smc_unregister(struct aspeed_smc_controller *controller)
 {
 	struct aspeed_smc_chip *chip;
-	int n;
+	int n, ret;
 
 	for (n = 0; n < controller->info->nce; n++) {
 		chip = controller->chips[n];
-		if (chip)
-			mtd_device_unregister(&chip->nor.mtd);
+		if (chip && chip->attached) {
+			ret = aspeed_smc_unregister_chip(chip);
+			if (ret)
+				dev_warn(controller->dev, "failed to unregister CS%d: %d\n",
+				         n, ret);
+		}
 	}
 
 	return 0;
@@ -1232,25 +1246,57 @@ static const struct spi_nor_controller_ops aspeed_smc_controller_ops = {
 	.write = aspeed_smc_write_user,
 };
 
-static int aspeed_smc_setup_flash(struct aspeed_smc_controller *controller,
-				  struct device_node *np, struct resource *r)
+static int aspeed_smc_register_chip(struct aspeed_smc_chip *chip)
 {
-	const struct spi_nor_hwcaps hwcaps = {
+	static const struct spi_nor_hwcaps hwcaps = {
 		.mask = SNOR_HWCAPS_READ |
 			SNOR_HWCAPS_READ_FAST |
 			SNOR_HWCAPS_READ_1_1_2 |
 			SNOR_HWCAPS_PP,
 	};
+	int ret;
+
+	ret = aspeed_smc_chip_setup_init(chip, chip->controller->ahb_res);
+	if (ret)
+		return ret;
+
+	/*
+	 * TODO: Add support for Dual and Quad SPI protocols attach when board
+	 * support is present as determined by of property.
+	 */
+	ret = spi_nor_scan(&chip->nor, NULL, &hwcaps);
+	if (ret)
+		return ret;
+
+	ret = aspeed_smc_chip_setup_finish(chip);
+	if (ret)
+		return ret;
+
+	ret = mtd_device_register(&chip->nor.mtd, NULL, 0);
+	if (ret)
+		return ret;
+
+	chip->attached = true;
+
+	return 0;
+}
+
+static int aspeed_smc_setup_flash(struct aspeed_smc_controller *controller,
+				  struct device_node *np, struct resource *r)
+{
 	const struct aspeed_smc_info *info = controller->info;
 	struct device *dev = controller->dev;
 	struct device_node *child;
 	unsigned int cs;
 	int ret = -ENODEV;
 
-	for_each_available_child_of_node(np, child) {
+	for_each_child_of_node(np, child) {
 		struct aspeed_smc_chip *chip;
 		struct spi_nor *nor;
-		struct mtd_info *mtd;
+
+		/* Skip disabled nodes, but include reserved ones for later attachment */
+		if (!of_device_is_available(child) && !of_device_is_reserved(child))
+			continue;
 
 		/* This driver does not support NAND or NOR flash devices. */
 		if (!of_device_is_compatible(child, "jedec,spi-nor"))
@@ -1294,35 +1340,20 @@ static int aspeed_smc_setup_flash(struct aspeed_smc_controller *controller,
 		chip->cs = cs;
 
 		nor = &chip->nor;
-		mtd = &nor->mtd;
 
 		nor->dev = dev;
 		nor->priv = chip;
 		spi_nor_set_flash_node(nor, child);
 		nor->controller_ops = &aspeed_smc_controller_ops;
 
-		ret = aspeed_smc_chip_setup_init(chip, r);
-		if (ret)
-			break;
-
-		/*
-		 * TODO: Add support for Dual and Quad SPI protocols
-		 * attach when board support is present as determined
-		 * by of property.
-		 */
-		ret = spi_nor_scan(nor, NULL, &hwcaps);
-		if (ret)
-			break;
+		controller->chips[cs] = chip;
 
-		ret = aspeed_smc_chip_setup_finish(chip);
-		if (ret)
-			break;
+		if (of_device_is_reserved(child))
+			continue;
 
-		ret = mtd_device_register(mtd, NULL, 0);
+		ret = aspeed_smc_register_chip(chip);
 		if (ret)
 			break;
-
-		controller->chips[cs] = chip;
 	}
 
 	if (ret) {
@@ -1333,6 +1364,78 @@ static int aspeed_smc_setup_flash(struct aspeed_smc_controller *controller,
 	return ret;
 }
 
+static inline struct aspeed_smc_controller *to_aspeed_smc_controller(struct device *dev)
+{
+	struct platform_device *pdev = container_of(dev, struct platform_device, dev);
+	return platform_get_drvdata(pdev);
+}
+
+static ssize_t attach_chip_store(struct device *dev, struct device_attribute *attr,
+                                 const char *buf, size_t count)
+{
+	unsigned long cs;
+	struct aspeed_smc_controller *controller;
+	struct aspeed_smc_chip *chip;
+	ssize_t ret = kstrtoul(buf, 0, &cs);
+	if (ret)
+		return ret;
+
+	controller = to_aspeed_smc_controller(dev);
+	if (cs >= controller->info->nce)
+		return -EINVAL;
+
+	chip = controller->chips[cs];
+
+	if (!chip)
+		return -ENODEV;
+
+	if (chip->attached)
+		return -EEXIST;
+
+	ret = aspeed_smc_register_chip(chip);
+
+	return ret ? ret : count;
+}
+static DEVICE_ATTR_WO(attach_chip);
+
+static ssize_t detach_chip_store(struct device *dev, struct device_attribute *attr,
+                                 const char *buf, size_t count)
+{
+	unsigned long cs;
+	struct aspeed_smc_controller *controller;
+	struct aspeed_smc_chip *chip;
+	ssize_t ret = kstrtoul(buf, 0, &cs);
+	if (ret)
+		return ret;
+
+	controller = to_aspeed_smc_controller(dev);
+	if (cs >= controller->info->nce)
+		return -EINVAL;
+
+	chip = controller->chips[cs];
+
+	if (!chip)
+		return -ENODEV;
+
+	if (!chip->attached)
+		return -ENOENT;
+
+	ret = aspeed_smc_unregister_chip(chip);
+
+	return ret ? ret : count;
+}
+static DEVICE_ATTR_WO(detach_chip);
+
+static struct attribute *aspeed_smc_sysfs_attrs[] = {
+	&dev_attr_attach_chip.attr,
+	&dev_attr_detach_chip.attr,
+	NULL,
+};
+
+static const struct attribute_group aspeed_smc_sysfs_attr_group = {
+	.attrs = aspeed_smc_sysfs_attrs,
+};
+
 static int aspeed_smc_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
@@ -1357,6 +1460,12 @@ static int aspeed_smc_probe(struct platform_device *pdev)
 	controller->info = info;
 	controller->dev = dev;
 
+	ret = devm_device_add_group(dev, &aspeed_smc_sysfs_attr_group);
+	if (ret) {
+		dev_err(dev, "Failed to create sysfs files\n");
+		return ret;
+	}
+
 	mutex_init(&controller->mutex);
 	platform_set_drvdata(pdev, controller);
 
@@ -1366,6 +1475,7 @@ static int aspeed_smc_probe(struct platform_device *pdev)
 		return PTR_ERR(controller->regs);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	controller->ahb_res = res;
 	controller->ahb_base_phy = res->start;
 	controller->ahb_base = devm_ioremap_resource(dev, res);
 	if (IS_ERR(controller->ahb_base))
-- 
2.33.0