| Message ID | 90fb762e5840f9d5a6ae46f81692fb947a7796a4.camel@egauge.net (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | RFQ: wifi: wilc1000: make wilc1000-spi bus-probe useful | expand |
Hello, On 1/19/24 22:51, David Mosberger-Tang wrote: > The current version of the wilc1000 driver has a probe function that simply > assumes the chip is present. It is only later, in wilc_spi_init(), that the > driver verifies that it can actually communicate with the chip. The result of > this is that the net device (typically wlan0) is created and remains in place as > long as the wilc1000-spi driver is loaded, even if the WILC1000 chip is not > present or not working. > > Is there any reason not to detect the chip's present in wilc_bus_probe()? The > patch below (relative to 5.15.147) works for me, but perhaps I'm missing > something? Would it make sense to merge something along these lines into > mainline? The general statement sounds relevant to me, it looks not so useful to register the corresponding netdevice if we can not even detect the chip at probe time. I have a series under work which, by "side effect", accomplishes the same kind of detection: it aims to fix faulty mac address (00:00:00:00:00:00) which is set correctly only after interface has been brought up. The series tries to read the mac address from NV memory right at probe time. If it fails, it can make the probe procedure fail and not register the wireless device. Nonetheless, validating chip presence with chip id sounds better than with mac address from NV memory. Aside from that, I have a few more specific comments below > > --david > > diff --git a/drivers/net/wireless/microchip/wilc1000/spi.c [...] > + /* we need power to configure the bus protocol and to read the chip id: */ > + > + wilc_wlan_power(wilc, true); > + > + ret = wilc_spi_configure_bus_protocol(wilc); > + > + wilc_wlan_power(wilc, false); > + > + if (ret) { > + ret = -ENODEV; I would keep wilc_spi_configure_bus_protocol original error instead of rewriting/forcing it to -ENODEV here. I mean, if something fails in wilc_spi_configure_bus_protocol but not right at the first attempt to communicate with the chip, it does not translate automatically to an absence of chip, right ? > + goto netdev_cleanup; > + } > + > return 0; > netdev_cleanup: [...] > @@ -1187,16 +1189,38 @@ static int wilc_spi_init(struct wilc *wilc, bool resume) > ret = wilc_spi_read_reg(wilc, WILC_CHIPID, &chipid); > if (ret) { > dev_err(&spi->dev, "Fail cmd read chip id...\n"); > - goto fail; > + return ret; > + } > + return 0; > +} > + > +static int wilc_spi_init(struct wilc *wilc, bool resume) > +{ > + struct spi_device *spi = to_spi_device(wilc->dev); > + struct wilc_spi *spi_priv = wilc->bus_data; > + u32 chipid; > + int ret; > + > + if (spi_priv->isinit) { > + /* Confirm we can read chipid register without error: */ > + ret = wilc_spi_read_reg(wilc, WILC_CHIPID, &chipid); > + if (ret == 0) > + return 0; > + > + dev_err(&spi->dev, "Fail cmd read chip id...\n"); > + } > + > + wilc_wlan_power(wilc, true); I guess something will break here. This updates now mark the chip as initialized (sp_priv->isinit) at probe time, but unpower the chip before finishing probe, so this wilc_wlan_power(wilc, true) needs more likely to be called earlier in wilc_spi_init (ie: before trying to read again the chip id). More generally, there is an important change about the meaning of this flag (meant: chip is on and configured, now means: I know chip is here but it may be unpowered), and since wlan.c can check for this flag to know if it can communicate with the chip, there will be an issue here. Thanks, Alexis
On 1/19/24 14:51, David Mosberger-Tang wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > The current version of the wilc1000 driver has a probe function that simply > assumes the chip is present. It is only later, in wilc_spi_init(), that the > driver verifies that it can actually communicate with the chip. The result of > this is that the net device (typically wlan0) is created and remains in place as > long as the wilc1000-spi driver is loaded, even if the WILC1000 chip is not > present or not working. > > Is there any reason not to detect the chip's present in wilc_bus_probe()? The > patch below (relative to 5.15.147) works for me, but perhaps I'm missing > something? Would it make sense to merge something along these lines into > mainline? > I think it is the WILC driver design that the firmware/chip operations are executed only when the netdev interface(wlan0) is up. The firmware is started only after the interface is up. However, it should be okay to read the register values since the bus interface is up. As I understand, this condition is raised since the auto-load started to work after the patch[1], now the driver is getting loaded at the boot-up time. Actually, the auto-detect(hot-plug) for SPI bus can't be supported like the SDIO bus where the driver gets loaded/unloaded when the device is connected/removed. In case of SPI devices, the driver probe will be called at the boot-up time based on the Device-tree(DT) entry. If the SPI device is connected after board boot-up, the board reboot is required for probe function to get called i.e. even wilc_bus_probe() returns failure for first time then the probe will not get called again. One way to handle this is by modifying the DT entry of the system to define whether the SPI device is connected or not. 1. https://github.com/torvalds/linux/commit/f2f16ae9cc9cba4e3c70941cf6a6443c9ea920f4 Regards, Ajay
Alexis, Thanks for your feedback! On Mon, 2024-01-22 at 15:19 +0100, Alexis Lothoré wrote: > Hello, > > On 1/19/24 22:51, David Mosberger-Tang wrote: > > The current version of the wilc1000 driver has a probe function that simply > > assumes the chip is present. It is only later, in wilc_spi_init(), that the > > driver verifies that it can actually communicate with the chip. The result of > > this is that the net device (typically wlan0) is created and remains in place as > > long as the wilc1000-spi driver is loaded, even if the WILC1000 chip is not > > present or not working. > > > > Is there any reason not to detect the chip's present in wilc_bus_probe()? The > > patch below (relative to 5.15.147) works for me, but perhaps I'm missing > > something? Would it make sense to merge something along these lines into > > mainline? > > The general statement sounds relevant to me, it looks not so useful to register > the corresponding netdevice if we can not even detect the chip at probe time. > I have a series under work which, by "side effect", accomplishes the same kind > of detection: it aims to fix faulty mac address (00:00:00:00:00:00) which is set > correctly only after interface has been brought up. Ahh, that sounds like another useful improvement! > The series tries to read the > mac address from NV memory right at probe time. If it fails, it can make the > probe procedure fail and not register the wireless device. Nonetheless, > validating chip presence with chip id sounds better than with mac address from > NV memory. Great! I'll send an update patch soon (properly formatted this time, I hope...). > Aside from that, I have a few more specific comments below > > > > > --david > > > > diff --git a/drivers/net/wireless/microchip/wilc1000/spi.c > > [...] > > > + /* we need power to configure the bus protocol and to read the chip id: */ > > + > > + wilc_wlan_power(wilc, true); > > + > > + ret = wilc_spi_configure_bus_protocol(wilc); > > + > > + wilc_wlan_power(wilc, false); > > + > > + if (ret) { > > + ret = -ENODEV; > > I would keep wilc_spi_configure_bus_protocol original error instead of > rewriting/forcing it to -ENODEV here. I mean, if something fails in > wilc_spi_configure_bus_protocol but not right at the first attempt to > communicate with the chip, it does not translate automatically to an absence of > chip, right ? Hmmh, I'm happy to change it, but, as it happens, all failure returns in wilc_spi_configure_bus_protocol() basically mean that the device isn't present or a device is present which the driver doesn't support, so I think -ENODEV is more useful than returning -EINVAL (as would be the case). Let me know if you still think I should change it. In the new patch, I broke out the chipid validation code into its own function since it felt wrong to do that in a function called "configure bus protocol". > > + goto netdev_cleanup; > > + } > > + > > return 0; > > netdev_cleanup: > > [...] > > > @@ -1187,16 +1189,38 @@ static int wilc_spi_init(struct wilc *wilc, bool resume) > > ret = wilc_spi_read_reg(wilc, WILC_CHIPID, &chipid); > > if (ret) { > > dev_err(&spi->dev, "Fail cmd read chip id...\n"); > > - goto fail; > > + return ret; > > + } > > + return 0; > > +} > > + > > +static int wilc_spi_init(struct wilc *wilc, bool resume) > > +{ > > + struct spi_device *spi = to_spi_device(wilc->dev); > > + struct wilc_spi *spi_priv = wilc->bus_data; > > + u32 chipid; > > + int ret; > > + > > + if (spi_priv->isinit) { > > + /* Confirm we can read chipid register without error: */ > > + ret = wilc_spi_read_reg(wilc, WILC_CHIPID, &chipid); > > + if (ret == 0) > > + return 0; > > + > > + dev_err(&spi->dev, "Fail cmd read chip id...\n"); > > + } > > + > > + wilc_wlan_power(wilc, true); > > I guess something will break here. This updates now mark the chip as initialized > (sp_priv->isinit) at probe time, but unpower the chip before finishing probe, so > this wilc_wlan_power(wilc, true) needs more likely to be called earlier in > wilc_spi_init (ie: before trying to read again the chip id). Oh, no that's definitely not the intention. The garbled formatting makes reading the patch more confusing than it should be, but isinit still gets set in wilc_spi_init(), not in wilc_bus_probe(). That is the reason I had to update the comment for the "isinit" member in wilc_spi. Let me know if I'm missing something, though. --david
On Mon, 2024-01-22 at 16:57 +0000, Ajay.Kathat@microchip.com wrote: > On 1/19/24 14:51, David Mosberger-Tang wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > The current version of the wilc1000 driver has a probe function that simply > > assumes the chip is present. It is only later, in wilc_spi_init(), that the > > driver verifies that it can actually communicate with the chip. The result of > > this is that the net device (typically wlan0) is created and remains in place as > > long as the wilc1000-spi driver is loaded, even if the WILC1000 chip is not > > present or not working. > > > > Is there any reason not to detect the chip's present in wilc_bus_probe()? The > > patch below (relative to 5.15.147) works for me, but perhaps I'm missing > > something? Would it make sense to merge something along these lines into > > mainline? > > > > I think it is the WILC driver design that the firmware/chip operations > are executed only when the netdev interface(wlan0) is up. The firmware > is started only after the interface is up. However, it should be okay to > read the register values since the bus interface is up. Yep, I didn't see any issues in my testing. > As I understand, this condition is raised since the auto-load started to > work after the patch[1], now the driver is getting loaded at the boot-up > time. > Actually, the auto-detect(hot-plug) for SPI bus can't be supported like > the SDIO bus where the driver gets loaded/unloaded when the device is > connected/removed. In case of SPI devices, the driver probe will be > called at the boot-up time based on the Device-tree(DT) entry. If the > SPI device is connected after board boot-up, the board reboot is > required for probe function to get called i.e. even wilc_bus_probe() > returns failure for first time then the probe will not get called again. > One way to handle this is by modifying the DT entry of the system to > define whether the SPI device is connected or not. Makes sense. In our system, we don't have the ability to dynamically patch the device tree so we rely on driver probing to confirm that a device actually exists. --david
This is an updated version of the patch to check for WILCxxxx presence during bus probe time. Compared to the earlier proposed patch, this one: - Is relative to linux-next. - Breaks out chip id validation into its own function. The function also checks to ensure that an expected id is detected (WILC1000 or WILC3000). One caveat: we're still at v5.15 of the kernel, so I wasn't actually able to test the patch with the linux-next kernel but I reviewed the differences between the 5.15.147 version of spi.c against the linux-next version and didn't see anything that looked to me like it would cause trouble. --david
<Ajay.Kathat@microchip.com> writes: > On 1/19/24 14:51, David Mosberger-Tang wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >> >> The current version of the wilc1000 driver has a probe function that simply >> assumes the chip is present. It is only later, in wilc_spi_init(), that the >> driver verifies that it can actually communicate with the chip. The result of >> this is that the net device (typically wlan0) is created and remains in place as >> long as the wilc1000-spi driver is loaded, even if the WILC1000 chip is not >> present or not working. >> >> Is there any reason not to detect the chip's present in wilc_bus_probe()? The >> patch below (relative to 5.15.147) works for me, but perhaps I'm missing >> something? Would it make sense to merge something along these lines into >> mainline? >> > > I think it is the WILC driver design that the firmware/chip operations > are executed only when the netdev interface(wlan0) is up. Yeah, that's good design. I think that wlan0 is down wireless drivers should turn off the hardware to reduce power consumption as much as possible. Many drivers do start the firmware during probe() to query the capabilities and then turn it off immediately.
Hello David, just reacting to your answers, but I will take a look at your updated patch On 1/22/24 21:41, David Mosberger-Tang wrote: > Alexis, > > Thanks for your feedback! > > On Mon, 2024-01-22 at 15:19 +0100, Alexis Lothoré wrote: >> Hello, >> >> On 1/19/24 22:51, David Mosberger-Tang wrote: [...] >>> + if (ret) { >>> + ret = -ENODEV; >> >> I would keep wilc_spi_configure_bus_protocol original error instead of >> rewriting/forcing it to -ENODEV here. I mean, if something fails in >> wilc_spi_configure_bus_protocol but not right at the first attempt to >> communicate with the chip, it does not translate automatically to an absence of >> chip, right ? > > Hmmh, I'm happy to change it, but, as it happens, all failure returns in > wilc_spi_configure_bus_protocol() basically mean that the device isn't present > or a device is present which the driver doesn't support, so I think -ENODEV is > more useful than returning -EINVAL (as would be the case). Let me know if you > still think I should change it. Yeah, but then I would change wilc_spi_configure_bus_protocol() to return -ENODEV instead of -EINVAL, if that's really the cause, and just let calling functions propagate it. That may just be a personal taste, but I find it pretty tedious to debug some error code and eventually realize that some intermediate function just rewrote the real error to another one (and sometime, loosing some info while doing so).
Alexis Lothoré <alexis.lothore@bootlin.com> writes: > Hello David, > just reacting to your answers, but I will take a look at your updated patch > > On 1/22/24 21:41, David Mosberger-Tang wrote: >> Alexis, >> >> Thanks for your feedback! >> >> On Mon, 2024-01-22 at 15:19 +0100, Alexis Lothoré wrote: >>> Hello, >>> >>> On 1/19/24 22:51, David Mosberger-Tang wrote: > > [...] > >>>> + if (ret) { >>>> + ret = -ENODEV; >>> >>> I would keep wilc_spi_configure_bus_protocol original error instead of >>> rewriting/forcing it to -ENODEV here. I mean, if something fails in >>> wilc_spi_configure_bus_protocol but not right at the first attempt to >>> communicate with the chip, it does not translate automatically to an absence of >>> chip, right ? >> >> Hmmh, I'm happy to change it, but, as it happens, all failure returns in >> wilc_spi_configure_bus_protocol() basically mean that the device isn't present >> or a device is present which the driver doesn't support, so I think -ENODEV is >> more useful than returning -EINVAL (as would be the case). Let me know if you >> still think I should change it. > > Yeah, but then I would change wilc_spi_configure_bus_protocol() to return > -ENODEV instead of -EINVAL, if that's really the cause, and just let calling > functions propagate it. That may just be a personal taste, but I find it pretty > tedious to debug some error code and eventually realize that some intermediate > function just rewrote the real error to another one (and sometime, loosing some > info while doing so). Yeah, changing error values is very much discouraged.
diff --git a/drivers/net/wireless/microchip/wilc1000/spi.c b/drivers/net/wireless/microchip/wilc1000/spi.c index 6bac52527e38..c7ab816d65bc 100644 --- a/drivers/net/wireless/microchip/wilc1000/spi.c +++ b/drivers/net/wireless/microchip/wilc1000/spi.c @@ -42,7 +42,7 @@ MODULE_PARM_DESC(enable_crc16, #define WILC_SPI_RSP_HDR_EXTRA_DATA 8 struct wilc_spi { - bool isinit; /* true if SPI protocol has been configured */ + bool isinit; /* true if wilc_spi_init was successful */ bool probing_crc; /* true if we're probing chip's CRC config */ bool crc7_enabled; /* true if crc7 is currently enabled */ bool crc16_enabled; /* true if crc16 is currently enabled */ @@ -55,6 +55,7 @@ struct wilc_spi {
The current version of the wilc1000 driver has a probe function that simply assumes the chip is present. It is only later, in wilc_spi_init(), that the driver verifies that it can actually communicate with the chip. The result of this is that the net device (typically wlan0) is created and remains in place as long as the wilc1000-spi driver is loaded, even if the WILC1000 chip is not present or not working. Is there any reason not to detect the chip's present in wilc_bus_probe()? The patch below (relative to 5.15.147) works for me, but perhaps I'm missing something? Would it make sense to merge something along these lines into mainline? --david static const struct wilc_hif_func wilc_hif_spi; static int wilc_spi_reset(struct wilc *wilc); +static int wilc_spi_configure_bus_protocol(struct wilc *); /******************************************** * @@ -232,6 +233,22 @@ static int wilc_bus_probe(struct spi_device *spi) } clk_prepare_enable(wilc->rtc_clk); + dev_info(&spi->dev, "crc7=%sabled crc16=%sabled", + enable_crc7 ? "en" : "dis", enable_crc16 ? "en" : "dis"); + + /* we need power to configure the bus protocol and to read the chip id: */ + + wilc_wlan_power(wilc, true); + + ret = wilc_spi_configure_bus_protocol(wilc); + + wilc_wlan_power(wilc, false); + + if (ret) { + ret = -ENODEV; + goto netdev_cleanup; + } + return 0; netdev_cleanup: @@ -1108,7 +1125,7 @@ static int wilc_spi_deinit(struct wilc *wilc) return 0; } -static int wilc_spi_init(struct wilc *wilc, bool resume) +static int wilc_spi_configure_bus_protocol (struct wilc *wilc) { struct spi_device *spi = to_spi_device(wilc->dev); struct wilc_spi *spi_priv = wilc->bus_data; @@ -1116,21 +1133,6 @@ static int wilc_spi_init(struct wilc *wilc, bool resume) u32 chipid; int ret, i; - if (spi_priv->isinit) { - /* Confirm we can read chipid register without error: */ - ret = wilc_spi_read_reg(wilc, WILC_CHIPID, &chipid); - if (ret == 0) - return 0; - - dev_err(&spi->dev, "Fail cmd read chip id...\n"); - } - - wilc_wlan_power(wilc, true); - - /* - * configure protocol - */ - /* * Infer the CRC settings that are currently in effect. This * is necessary because we can't be sure that the chip has @@ -1147,7 +1149,7 @@ static int wilc_spi_init(struct wilc *wilc, bool resume) } if (ret) { dev_err(&spi->dev, "Failed with CRC7 on and off.\n"); - goto fail; + return ret; } /* set up the desired CRC configuration: */ @@ -1170,7 +1172,7 @@ static int wilc_spi_init(struct wilc *wilc, bool resume) dev_err(&spi->dev, "[wilc spi %d]: Failed internal write reg\n", __LINE__); - goto fail; + return ret; } /* update our state to match new protocol settings: */ spi_priv->crc7_enabled = enable_crc7; @@ -1187,16 +1189,38 @@ static int wilc_spi_init(struct wilc *wilc, bool resume) ret = wilc_spi_read_reg(wilc, WILC_CHIPID, &chipid); if (ret) { dev_err(&spi->dev, "Fail cmd read chip id...\n"); - goto fail; + return ret; + } + return 0; +} + +static int wilc_spi_init(struct wilc *wilc, bool resume) +{ + struct spi_device *spi = to_spi_device(wilc->dev); + struct wilc_spi *spi_priv = wilc->bus_data; + u32 chipid; + int ret; + + if (spi_priv->isinit) { + /* Confirm we can read chipid register without error: */ + ret = wilc_spi_read_reg(wilc, WILC_CHIPID, &chipid); + if (ret == 0) + return 0; + + dev_err(&spi->dev, "Fail cmd read chip id...\n"); + } + + wilc_wlan_power(wilc, true); + + ret = wilc_spi_configure_bus_protocol(wilc); + if (ret) { + wilc_wlan_power(wilc, false); + return ret; } spi_priv->isinit = true; return 0; - - fail: - wilc_wlan_power(wilc, false); - return ret; } static int wilc_spi_read_size(struct wilc *wilc, u32 *size)