[PATCH RFC] Specifying default-disabled devices

[PATCH RFC] Specifying default-disabled devices

[Zev Weiss]

Hi all,

I'd like to hear people's thoughts on how to approach specifying that a device
is present, but should be left alone by default.  The specific example I have
at hand is that of the host firmware (BIOS) SPI flash -- it's attached to a SPI
controller on my BMC, but actually accessing it via that interface requires a
bit of a dance involving three GPIOs and exchanging some IPMI commands with the
ME, so if I have it defined in my dts, the mtd driver's probe routine during
the BMC's boot sequence just spews errors into dmesg and fails (I can then
manually bind it later via sysfs after fiddling with GPIOs and IPMI from
userspace).  This "works" in its clumsy way in this specific case, but I'd like
to try to upstream a cleaner (and hopefully reasonably general) solution.

What I've had patched into my local fork for a while is a 'noautobind' kernel
command-line argument that lists devices to skip automatic probing on, and then
just a simple check in driver_probe_device() against that list (bypassed if
called from a sysfs 'bind' write).  (Patch for dev-5.10 inline below, though it
doesn't currently apply to mainline.)

The other alternative I've considered (though not actually implemented thus
far) is to start using the "reserved" status value defined in the device-tree
spec (section 2.3.4, [0]) to mean this -- from the wording in the spec it seems
like a not-terribly-unreasonable interpretation, and the existing kernel &
u-boot fdt code don't seem to make any use of it as far as I can see (though I
don't know what might be out there in device-tree implementations floating
around in other projects).

Anyone have any particular thoughts?  The 'noautobind' approach is a pretty
small, simple patch, and I'd guess a 'status = "reserved"' patch probably
wouldn't be much bigger, but I figured it might be worth it to solicit opinions
from OpenBMC people before potentially ruffling driver-core/device-tree
feathers on upstream lists.


Thanks,
Zev

[0] https://github.com/devicetree-org/devicetree-specification/releases/download/v0.3/devicetree-specification-v0.3.pdf

---
 drivers/base/base.h |  2 +-
 drivers/base/bus.c  |  2 +-
 drivers/base/dd.c   | 33 +++++++++++++++++++++++++++------
 3 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index 52b3d7b75c27..98bd131dacfc 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -152,7 +152,7 @@ extern int driver_add_groups(struct device_driver *drv,
 			     const struct attribute_group **groups);
 extern void driver_remove_groups(struct device_driver *drv,
 				 const struct attribute_group **groups);
-int device_driver_attach(struct device_driver *drv, struct device *dev);
+int device_driver_attach(struct device_driver *drv, struct device *dev, bool explicit);
 void device_driver_detach(struct device *dev);
 
 extern char *make_class_name(const char *name, struct kobject *kobj);
diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 36d0c654ea61..4a672a7051e5 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -211,7 +211,7 @@ static ssize_t bind_store(struct device_driver *drv, const char *buf,
 
 	dev = bus_find_device_by_name(bus, NULL, buf);
 	if (dev && dev->driver == NULL && driver_match_device(drv, dev)) {
-		err = device_driver_attach(drv, dev);
+		err = device_driver_attach(drv, dev, true);
 
 		if (err > 0) {
 			/* success */
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index e2cf3b29123e..bf723a30bbaf 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -62,6 +62,10 @@ static bool initcalls_done;
 #define ASYNC_DRV_NAMES_MAX_LEN	256
 static char async_probe_drv_names[ASYNC_DRV_NAMES_MAX_LEN];
 
+/* Saved copy of noautobind kernel cmdline arg */
+#define NOAUTOBIND_DEVS_MAX_LEN 256
+static char noautobind_dev_names[NOAUTOBIND_DEVS_MAX_LEN];
+
 /*
  * In some cases, like suspend to RAM or hibernation, It might be reasonable
  * to prohibit probing of devices as it could be unsafe.
@@ -713,6 +717,7 @@ EXPORT_SYMBOL_GPL(wait_for_device_probe);
  * driver_probe_device - attempt to bind device & driver together
  * @drv: driver to bind a device to
  * @dev: device to try to bind to the driver
+ * @explicit: whether or not this is an explicit bind request from userspace
  *
  * This function returns -ENODEV if the device is not registered,
  * 1 if the device is bound successfully and 0 otherwise.
@@ -722,13 +727,18 @@ EXPORT_SYMBOL_GPL(wait_for_device_probe);
  *
  * If the device has a parent, runtime-resume the parent before driver probing.
  */
-static int driver_probe_device(struct device_driver *drv, struct device *dev)
+static int driver_probe_device(struct device_driver *drv, struct device *dev, bool explicit)
 {
 	int ret = 0;
 
 	if (!device_is_registered(dev))
 		return -ENODEV;
 
+	if (!explicit && parse_option_str(noautobind_dev_names, dev_name(dev))) {
+		dev_info(dev, "skipping %s probe of noautobind device\n", drv->name);
+		return -EPROBE_DEFER;
+	}
+
 	pr_debug("bus: '%s': %s: matched device %s with driver %s\n",
 		 drv->bus->name, __func__, dev_name(dev), drv->name);
 
@@ -766,6 +776,16 @@ static int __init save_async_options(char *buf)
 }
 __setup("driver_async_probe=", save_async_options);
 
+static int __init save_noautobind_options(char *buf)
+{
+	if (strlen(buf) >= NOAUTOBIND_DEVS_MAX_LEN)
+		pr_warn("'noautobind' device name list too long!\n");
+
+	strlcpy(noautobind_dev_names, buf, NOAUTOBIND_DEVS_MAX_LEN);
+	return 0;
+}
+__setup("noautobind=", save_noautobind_options);
+
 bool driver_allows_async_probing(struct device_driver *drv)
 {
 	switch (drv->probe_type) {
@@ -846,7 +866,7 @@ static int __device_attach_driver(struct device_driver *drv, void *_data)
 	if (data->check_async && async_allowed != data->want_async)
 		return 0;
 
-	return driver_probe_device(drv, dev);
+	return driver_probe_device(drv, dev, false);
 }
 
 static void __device_attach_async_helper(void *_dev, async_cookie_t cookie)
@@ -1000,11 +1020,12 @@ static void __device_driver_unlock(struct device *dev, struct device *parent)
  * device_driver_attach - attach a specific driver to a specific device
  * @drv: Driver to attach
  * @dev: Device to attach it to
+ * @explicit: whether or not this is an explicit bind request from userspace
  *
  * Manually attach driver to a device. Will acquire both @dev lock and
  * @dev->parent lock if needed.
  */
-int device_driver_attach(struct device_driver *drv, struct device *dev)
+int device_driver_attach(struct device_driver *drv, struct device *dev, bool explicit)
 {
 	int ret = 0;
 
@@ -1015,7 +1036,7 @@ int device_driver_attach(struct device_driver *drv, struct device *dev)
 	 * bound a driver before us just skip the driver probe call.
 	 */
 	if (!dev->p->dead && !dev->driver)
-		ret = driver_probe_device(drv, dev);
+		ret = driver_probe_device(drv, dev, explicit);
 
 	__device_driver_unlock(dev, dev->parent);
 
@@ -1037,7 +1058,7 @@ static void __driver_attach_async_helper(void *_dev, async_cookie_t cookie)
 	 * bound a driver before us just skip the driver probe call.
 	 */
 	if (!dev->p->dead && !dev->driver)
-		ret = driver_probe_device(drv, dev);
+		ret = driver_probe_device(drv, dev, false);
 
 	__device_driver_unlock(dev, dev->parent);
 
@@ -1092,7 +1113,7 @@ static int __driver_attach(struct device *dev, void *data)
 		return 0;
 	}
 
-	device_driver_attach(drv, dev);
+	device_driver_attach(drv, dev, false);
 
 	return 0;
 }
-- 
2.33.0

Re: [PATCH RFC] Specifying default-disabled devices

[Jeremy Kerr]

Hi Zev,

> I'd like to hear people's thoughts on how to approach specifying that
> a device is present, but should be left alone by default.

Sounds good!

> The other alternative I've considered (though not actually implemented
> thus far) is to start using the "reserved" status value defined in the
> device-tree spec (section 2.3.4, [0]) to mean this

I'd prefer this approach - it seems quite neat, and means we can keep
the hardware definitions together.

Do you have thoughts about how you'd then un- and re-reserve the device?

Cheers,


Jeremy

Re: [PATCH RFC] Specifying default-disabled devices

[Zev Weiss]

On Thu, Sep 09, 2021 at 07:33:42PM PDT, Jeremy Kerr wrote:
>Hi Zev,
>
>> I'd like to hear people's thoughts on how to approach specifying that
>> a device is present, but should be left alone by default.
>
>Sounds good!
>
>> The other alternative I've considered (though not actually implemented
>> thus far) is to start using the "reserved" status value defined in the
>> device-tree spec (section 2.3.4, [0]) to mean this
>
>I'd prefer this approach - it seems quite neat, and means we can keep
>the hardware definitions together.
>

Yeah -- I'm still on the command-line approach at the moment basically
just because it was the first thing that occurred to me and has worked
well enough so far for my purposes, but I think I'd agree that the
"reserved" option seems more desirable overall.

However, I think I may have spoken too soon regarding the relative
simplicity of implementing it -- from a quick glance at it, I think I'd
want to take of_device_is_available() and split it into some variants
for strictly-okay and okay-or-reserved (and similarly for
of_get_next_available_child()).  The problem there is that there are
currently circa 200 callers of those functions scattered thoughout the
tree, and auditing each of them individually to determine which of those
semantics is actually appropriate in each case seems...a bit daunting.

>Do you have thoughts about how you'd then un- and re-reserve the device?
>

I hadn't been thinking of being quite that ambitious with it, I figured
it'd basically just get used as a boot-time-only flag like my noautobind
argument, and beyond that it would just remain reserved, leaving it up
to userspace to take it in and out of use via bind/unbind operations.
AFAIK the fdt is (outside of things like DT overlays and such that I
don't think have ever hit mainline) currently an entirely read-only data
structure at runtime, and I'm not sure what ramifications there might be
to introducing runtime mutations...possibly not a big deal though?


Zev

Re: [PATCH RFC] Specifying default-disabled devices

[Jeremy Kerr]

Hi Zev,

> However, I think I may have spoken too soon regarding the relative
> simplicity of implementing it -- from a quick glance at it, I think
> I'd want to take of_device_is_available() and split it into some
> variants for strictly-okay and okay-or-reserved (and similarly for
> of_get_next_available_child()).  The problem there is that there are
> currently circa 200 callers of those functions scattered thoughout the
> tree, and auditing each of them individually to determine which of
> those semantics is actually appropriate in each case seems...a bit
> daunting.

I think you should be OK, if you stage it this way:

 - add status = "reserved" to your device tree; this will supress the
   automatic binding right away. With the current code, all it cares
   about is status = "okay" (or "ok"), so you'll at least keep the
   device quiesced.

   with that, there won't be an easy way to initiate the driver probe,
   but maybe that's OK for your use-case if you're doing the bind
   manually.

   There might be some bits doing their own DT parsing of the status
   property, but it sounds like you don't need to worry about those for
   now; they'd mainly be about devices' sub-nodes, and not related to
   driver binding.

 - propose a mechanism to trigger an un-reserve (and possibly
   re-reserve?), if you need.

The core code handles this for you, just setting status = "reserved"
will get you 90% there (if I've understood your use-case correctly!).

Cheers,


Jeremy

Re: [PATCH RFC] Specifying default-disabled devices

[Oliver O'Halloran]

On Fri, Sep 10, 2021 at 1:30 PM Zev Weiss <zweiss@equinix.com> wrote:
>
> The other alternative I've considered (though not actually implemented thus
> far) is to start using the "reserved" status value defined in the device-tree
> spec (section 2.3.4, [0]) to mean this -- from the wording in the spec it seems
> like a not-terribly-unreasonable interpretation, and the existing kernel &
> u-boot fdt code don't seem to make any use of it as far as I can see (though I
> don't know what might be out there in device-tree implementations floating
> around in other projects).

Nothing explicitly looks for status = "reserved" because Linux (and
u-boot apparently) ignore the device unless it's marked as okay. I
added the reserved state to the DTS spec since OpenPower platform
firmware was using it to mark devices that are owned by firmware
rather than the OS. I'd say the idea of having a bus device
instantiated for "reserved" devices is largely compatible with that
usage.

The one thing I'd be concerned about is what happens when the bus
itself can modify the device. For example, during PCI device probing
Linux will do some basic device configuration (enabling / disabling
various features in the PCIe caps) so you could have the OS modifying
a "reserved" device even if a driver is never attached to it. That's a
slightly academic problem for PCI since it doesn't even check the
status in the DT node, but it might be an issue for other busses.

Oliver

Re: [PATCH RFC] Specifying default-disabled devices

[Zev Weiss]

On Thu, Sep 09, 2021 at 09:04:05PM PDT, Jeremy Kerr wrote:
>Hi Zev,
>
>> However, I think I may have spoken too soon regarding the relative
>> simplicity of implementing it -- from a quick glance at it, I think
>> I'd want to take of_device_is_available() and split it into some
>> variants for strictly-okay and okay-or-reserved (and similarly for
>> of_get_next_available_child()).  The problem there is that there are
>> currently circa 200 callers of those functions scattered thoughout the
>> tree, and auditing each of them individually to determine which of
>> those semantics is actually appropriate in each case seems...a bit
>> daunting.
>
>I think you should be OK, if you stage it this way:
>
> - add status = "reserved" to your device tree; this will supress the
>   automatic binding right away. With the current code, all it cares
>   about is status = "okay" (or "ok"), so you'll at least keep the
>   device quiesced.
>
>   with that, there won't be an easy way to initiate the driver probe,
>   but maybe that's OK for your use-case if you're doing the bind
>   manually.
>

From some grepping around it looks like the only check is for
"okay"/"ok", and nothing actually checks for "disabled", so I'd think
any non-OK string (including "reserved") would end up being equivalent
to "disabled", and hence result in the device node not being
instantiated at all.  (A quick test appears to confirm; with status =
"reserved", an attempt to bind via sysfs fails with ENODEV.)


Zev

Re: [PATCH RFC] Specifying default-disabled devices

[Jeremy Kerr]

Hi Zev,

> From some grepping around it looks like the only check is for
> "okay"/"ok", and nothing actually checks for "disabled", so I'd think
> any non-OK string (including "reserved") would end up being
> equivalent
> to "disabled", and hence result in the device node not being
> instantiated at all.  (A quick test appears to confirm; with status =
> "reserved", an attempt to bind via sysfs fails with ENODEV.)

Ah, so you still want the device created, but not bound?

That might not work for status = "reserved" then, and I'm not sure we
want to change the semantics for that.

Just so I'm following along correctly: you still need this described in
the DT (rather than instantiating entirely from userspace), because you
need additional platform data for the new device, is that correct?

Cheers,


Jeremy

Re: [PATCH RFC] Specifying default-disabled devices

[Zev Weiss]

On Fri, Sep 10, 2021 at 12:59:10AM PDT, Jeremy Kerr wrote:
>Hi Zev,
>
>> From some grepping around it looks like the only check is for
>> "okay"/"ok", and nothing actually checks for "disabled", so I'd think
>> any non-OK string (including "reserved") would end up being
>> equivalent
>> to "disabled", and hence result in the device node not being
>> instantiated at all.  (A quick test appears to confirm; with status =
>> "reserved", an attempt to bind via sysfs fails with ENODEV.)
>
>Ah, so you still want the device created, but not bound?
>

That's what I'm hoping to achieve, yeah -- a device that'll be left
detached on boot, and only have a driver bound to it when userspace
explicitly requests it via a sysfs 'bind' write (e.g. in my particular
case, only when performing a BIOS update).

>That might not work for status = "reserved" then, and I'm not sure we
>want to change the semantics for that.
>

Sorry, which semantics exactly do you mean we might not want to change?
It sounded like Oliver thought that interpretation of "reserved" should
be viable, modulo some possible bus-specific caveats...

>Just so I'm following along correctly: you still need this described in
>the DT (rather than instantiating entirely from userspace), because you
>need additional platform data for the new device, is that correct?
>

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;
  	};
  };

...but I'm wondering about your mention of "rather than instantiating
entirely from userspace" -- is there some mechanism for
runtime-materializing a device ex nihilo that I've remained
(embarrassingly) unaware of?


Zev

Re: [PATCH RFC] Specifying default-disabled devices

[Jeremy Kerr]

Hi Zev,

> Sorry, which semantics exactly do you mean we might not want to
> change? It sounded like Oliver thought that interpretation of
> "reserved" should be viable, modulo some possible bus-specific
> caveats...

At the moment (as you've noticed), status = "reserved" does not
instantiate the device. For what you're proposing here, we'd need to
change that: "reserved" would instantiate the device, but suppress the
probe. I'm not sure what might break if were were to make that change.

> 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...]

> ...but I'm wondering about your mention of "rather than instantiating
> entirely from userspace" -- is there some mechanism for
> runtime-materializing a device ex nihilo that I've remained
> (embarrassingly) unaware of?

It depends on the bus; we can instantiate (and bind) i2c devices with
something like:

 # echo tmp75 0x50 > /sys/bus/i2c/devices/i2c-7/new_device
         ^    ^
	 |    i2c addr
	 |
	 i2c device id

- which requires no DT node at all.

But on a quick check, it looks like there's no equivalent facility for
SPI (which makes sense, as there's likely to be additional platform data
required for most devices..)

Cheers,


Jeremy

Re: [PATCH RFC] Specifying default-disabled devices

[Zev Weiss]

On Fri, Sep 10, 2021 at 02:08:40AM PDT, Jeremy Kerr wrote:
>Hi Zev,
>
>> Sorry, which semantics exactly do you mean we might not want to
>> change? It sounded like Oliver thought that interpretation of
>> "reserved" should be viable, modulo some possible bus-specific
>> caveats...
>
>At the moment (as you've noticed), status = "reserved" does not
>instantiate the device. For what you're proposing here, we'd need to
>change that: "reserved" would instantiate the device, but suppress the
>probe. I'm not sure what might break if were were to make that change.
>

Any particular possible breakage outside of the possibility of busses
that might poke things independently of driver attachment as Oliver
noted?  That aside, if nothing's actually touching the hardware I'd
think the only real difference would be consuming a small amount of
additional memory.

>> 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).

>> ...but I'm wondering about your mention of "rather than instantiating
>> entirely from userspace" -- is there some mechanism for
>> runtime-materializing a device ex nihilo that I've remained
>> (embarrassingly) unaware of?
>
>It depends on the bus; we can instantiate (and bind) i2c devices with
>something like:
>
> # echo tmp75 0x50 > /sys/bus/i2c/devices/i2c-7/new_device
>         ^    ^
>	 |    i2c addr
>	 |
>	 i2c device id
>
>- which requires no DT node at all.
>
>But on a quick check, it looks like there's no equivalent facility for
>SPI (which makes sense, as there's likely to be additional platform data
>required for most devices..)
>

Ah, right -- I've poked at that particular one for various i2c devices
before, but yeah, it's not quite so easy in the SPI case unfortunately.


Zev

Re: [PATCH RFC] Specifying default-disabled devices

[Zev Weiss]

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