| Message ID | 20240213-wilc_1000_reset_line-v1-1-e01da2b23fed@bootlin.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [RFC] wifi: wilc1000: fix reset line assert/deassert polarity | expand |
On Tue, 2024-02-13 at 16:22 +0100, Alexis Lothoré wrote: > When using a wilc1000 chip over a spi bus, users can optionally define a > reset gpio and a chip enable gpio. The reset line of wilc1000 is active > low, so to hold the chip in reset, a low (physical) value must be applied. > > The corresponding device tree binding documentation was introduced by > commit f31ee3c0a555 ("wilc1000: Document enable-gpios and reset-gpios > properties") and correctly indicates that the reset line is an active-low > signal. However, the corresponding driver part, brought by commit > ec031ac4792c ("wilc1000: Add reset/enable GPIO support to SPI driver"), is > misusing the gpiod APIs and apply an inverted logic when powering up/down > the chip (for example, setting the reset line to a logic "1" during power > up, which in fact asserts the reset line when device tree describes the > reset line as GPIO_ACTIVE_LOW). Note that commit ec031ac4792c is doing the right thing in regards to an ACTIVE_LOW RESET pin and the binding documentation is consistent with that code. It was later on that commit fcf690b0 flipped the RESET line polarity to treat it as GPIO_ACTIVE_HIGH. I never understood why that was done and, as you noted, it introduced in inconsistency with the binding documentation. On our platform, we never merged commit fcf690b0 and hence our DTS already defines the RESET pin as GPIO_ACTIVE_LOW. So, I don't have any issues at all with your patch! :-) --david gi
On 2/13/24 17:42, David Mosberger-Tang wrote: > On Tue, 2024-02-13 at 16:22 +0100, Alexis Lothoré wrote: >> When using a wilc1000 chip over a spi bus, users can optionally define a >> reset gpio and a chip enable gpio. The reset line of wilc1000 is active >> low, so to hold the chip in reset, a low (physical) value must be applied. >> >> The corresponding device tree binding documentation was introduced by >> commit f31ee3c0a555 ("wilc1000: Document enable-gpios and reset-gpios >> properties") and correctly indicates that the reset line is an active-low >> signal. However, the corresponding driver part, brought by commit >> ec031ac4792c ("wilc1000: Add reset/enable GPIO support to SPI driver"), is >> misusing the gpiod APIs and apply an inverted logic when powering up/down >> the chip (for example, setting the reset line to a logic "1" during power >> up, which in fact asserts the reset line when device tree describes the >> reset line as GPIO_ACTIVE_LOW). > > Note that commit ec031ac4792c is doing the right thing in regards to an > ACTIVE_LOW RESET pin and the binding documentation is consistent with that code. > > It was later on that commit fcf690b0 flipped the RESET line polarity to treat it > as GPIO_ACTIVE_HIGH. I never understood why that was done and, as you noted, it > introduced in inconsistency with the binding documentation. Ah, you are right, and I was wrong citing your GPIOs patch as faulty (git-blaming too fast !), thanks for the clarification. I missed this patch from Ajay (fcf690b0) flipping the reset logic. Maybe he had issues while missing proper device tree configuration and then submitted this flip ? > On our platform, we never merged commit fcf690b0 and hence our DTS already > defines the RESET pin as GPIO_ACTIVE_LOW. So, I don't have any issues at all > with your patch! :-) So in the end, the patch should be about a mere revert. I will update accordingly when relevant, but before that I'll wait for some feedback about the potential issue of this patch (forcing users to udpate faulty devicetree) Thanks, Alexis
Hi, On 2/13/24 09:58, Alexis Lothoré wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 2/13/24 17:42, David Mosberger-Tang wrote: >> On Tue, 2024-02-13 at 16:22 +0100, Alexis Lothoré wrote: >>> When using a wilc1000 chip over a spi bus, users can optionally define a >>> reset gpio and a chip enable gpio. The reset line of wilc1000 is active >>> low, so to hold the chip in reset, a low (physical) value must be applied. >>> >>> The corresponding device tree binding documentation was introduced by >>> commit f31ee3c0a555 ("wilc1000: Document enable-gpios and reset-gpios >>> properties") and correctly indicates that the reset line is an active-low >>> signal. However, the corresponding driver part, brought by commit >>> ec031ac4792c ("wilc1000: Add reset/enable GPIO support to SPI driver"), is >>> misusing the gpiod APIs and apply an inverted logic when powering up/down >>> the chip (for example, setting the reset line to a logic "1" during power >>> up, which in fact asserts the reset line when device tree describes the >>> reset line as GPIO_ACTIVE_LOW). >> >> Note that commit ec031ac4792c is doing the right thing in regards to an >> ACTIVE_LOW RESET pin and the binding documentation is consistent with that code. >> >> It was later on that commit fcf690b0 flipped the RESET line polarity to treat it >> as GPIO_ACTIVE_HIGH. I never understood why that was done and, as you noted, it >> introduced in inconsistency with the binding documentation. > > Ah, you are right, and I was wrong citing your GPIOs patch as faulty > (git-blaming too fast !), thanks for the clarification. I missed this patch from > Ajay (fcf690b0) flipping the reset logic. Maybe he had issues while missing > proper device tree configuration and then submitted this flip ? Indeed, it was done to align the code as per the DT entry suggested in WILC1000/3000 porting guide[1 -page 18], which is already used by most of the existing users. This change has impact on the users who are using DT entry from porting guide. One approach is to retain the current code and document this if needed. 1. https://ww1.microchip.com/downloads/en/DeviceDoc/ATWILC1000-ATWILC3000-ATWILC-Devices-Linux-Porting-Guide-User-Guide-DS70005329C.pdf Regards, Ajay
(Adding devicetree list for comments) <Ajay.Kathat@microchip.com> writes: > On 2/13/24 09:58, Alexis Lothoré wrote: >> >> On 2/13/24 17:42, David Mosberger-Tang wrote: >>> On Tue, 2024-02-13 at 16:22 +0100, Alexis Lothoré wrote: >>>> When using a wilc1000 chip over a spi bus, users can optionally define a >>>> reset gpio and a chip enable gpio. The reset line of wilc1000 is active >>>> low, so to hold the chip in reset, a low (physical) value must be applied. >>>> >>>> The corresponding device tree binding documentation was introduced by >>>> commit f31ee3c0a555 ("wilc1000: Document enable-gpios and reset-gpios >>>> properties") and correctly indicates that the reset line is an active-low >>>> signal. However, the corresponding driver part, brought by commit >>>> ec031ac4792c ("wilc1000: Add reset/enable GPIO support to SPI driver"), is >>>> misusing the gpiod APIs and apply an inverted logic when powering up/down >>>> the chip (for example, setting the reset line to a logic "1" during power >>>> up, which in fact asserts the reset line when device tree describes the >>>> reset line as GPIO_ACTIVE_LOW). >>> >>> Note that commit ec031ac4792c is doing the right thing in regards to an >>> ACTIVE_LOW RESET pin and the binding documentation is consistent with that code. >>> >>> It was later on that commit fcf690b0 flipped the RESET line polarity to treat it >>> as GPIO_ACTIVE_HIGH. I never understood why that was done and, as you noted, it >>> introduced in inconsistency with the binding documentation. >> >> Ah, you are right, and I was wrong citing your GPIOs patch as faulty >> (git-blaming too fast !), thanks for the clarification. I missed this patch from >> Ajay (fcf690b0) flipping the reset logic. Maybe he had issues while missing >> proper device tree configuration and then submitted this flip ? > > Indeed, it was done to align the code as per the DT entry suggested in > WILC1000/3000 porting guide[1 -page 18], which is already used by most > of the existing users. This change has impact on the users who are using > DT entry from porting guide. One approach is to retain the current code > and document this if needed. So if I'm understanding the situation correctly Microchip's porting guide[1] doesn't match with kernel.org documentation[2]? I'm not the expert here but from my point of view the issue is clear: the code needs to follow kernel.org documentation[2], not external documentation. I'll add devicetree list so hopefully people there can comment also, full patch available in [3]. Alexis, if there are no more comments I'm in favor submitting the revert you mentioned. [1] https://ww1.microchip.com/downloads/en/DeviceDoc/ATWILC1000-ATWILC3000-ATWILC-Devices-Linux-Porting-Guide-User-Guide-DS70005329C.pdf [2] Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml [3] https://patchwork.kernel.org/project/linux-wireless/patch/20240213-wilc_1000_reset_line-v1-1-e01da2b23fed@bootlin.com/
On Fri, Feb 16, 2024 at 06:01:52PM +0200, Kalle Valo wrote: > (Adding devicetree list for comments) > > <Ajay.Kathat@microchip.com> writes: > > > On 2/13/24 09:58, Alexis Lothoré wrote: > >> > >> On 2/13/24 17:42, David Mosberger-Tang wrote: > >>> On Tue, 2024-02-13 at 16:22 +0100, Alexis Lothoré wrote: > >>>> When using a wilc1000 chip over a spi bus, users can optionally define a > >>>> reset gpio and a chip enable gpio. The reset line of wilc1000 is active > >>>> low, so to hold the chip in reset, a low (physical) value must be applied. > >>>> > >>>> The corresponding device tree binding documentation was introduced by > >>>> commit f31ee3c0a555 ("wilc1000: Document enable-gpios and reset-gpios > >>>> properties") and correctly indicates that the reset line is an active-low > >>>> signal. However, the corresponding driver part, brought by commit > >>>> ec031ac4792c ("wilc1000: Add reset/enable GPIO support to SPI driver"), is > >>>> misusing the gpiod APIs and apply an inverted logic when powering up/down > >>>> the chip (for example, setting the reset line to a logic "1" during power > >>>> up, which in fact asserts the reset line when device tree describes the > >>>> reset line as GPIO_ACTIVE_LOW). > >>> > >>> Note that commit ec031ac4792c is doing the right thing in regards to an > >>> ACTIVE_LOW RESET pin and the binding documentation is consistent with that code. > >>> > >>> It was later on that commit fcf690b0 flipped the RESET line polarity to treat it > >>> as GPIO_ACTIVE_HIGH. I never understood why that was done and, as you noted, it > >>> introduced in inconsistency with the binding documentation. > >> > >> Ah, you are right, and I was wrong citing your GPIOs patch as faulty > >> (git-blaming too fast !), thanks for the clarification. I missed this patch from > >> Ajay (fcf690b0) flipping the reset logic. Maybe he had issues while missing > >> proper device tree configuration and then submitted this flip ? > > > > Indeed, it was done to align the code as per the DT entry suggested in > > WILC1000/3000 porting guide[1 -page 18], which is already used by most > > of the existing users. This change has impact on the users who are using > > DT entry from porting guide. One approach is to retain the current code > > and document this if needed. > > So if I'm understanding the situation correctly Microchip's porting > guide[1] doesn't match with kernel.org documentation[2]? I'm not the > expert here but from my point of view the issue is clear: the code needs > to follow kernel.org documentation[2], not external documentation. My point of view would definitely be that drivers in the mainline kernel absolutely should respect the ABI defined in the dt-binding. What a vendor decides to do in their own tree I suppose is their problem, but I would advocate that vendor kernels would also respect the ABI from mainline. Looking a bit more closely at the porting guide, it contains other properties that are not present in the dt-binding - undocumented compatibles and a different enable gpio property for example. I guess it (and the vendor version of the driver) never got updated when wilc1000 supported landed in mainline? > I'll add devicetree list so hopefully people there can comment also, > full patch available in [3]. > > Alexis, if there are no more comments I'm in favor submitting the revert > you mentioned. From a dt-bindings point of view, the aforementioned revert seems correct and would be Acked-by: Conor Dooley <conor.dooley@microchip.com> Getting off my dt-binding maintainer high-horse, linux4microchip is going be updating to a 6.6 based kernel in the coming weeks - maybe that's a good time to update the vendor kernel wilc drivers (and therefore the porting guide?) to match the properties used by mainline Ajay? Cheers, Conor.
On Fri, Feb 16, 2024 at 04:54:29PM +0000, Conor Dooley wrote: > On Fri, Feb 16, 2024 at 06:01:52PM +0200, Kalle Valo wrote: > > (Adding devicetree list for comments) > > > > <Ajay.Kathat@microchip.com> writes: > > > > > On 2/13/24 09:58, Alexis Lothoré wrote: > > >> > > >> On 2/13/24 17:42, David Mosberger-Tang wrote: > > >>> On Tue, 2024-02-13 at 16:22 +0100, Alexis Lothoré wrote: > > >>>> When using a wilc1000 chip over a spi bus, users can optionally define a > > >>>> reset gpio and a chip enable gpio. The reset line of wilc1000 is active > > >>>> low, so to hold the chip in reset, a low (physical) value must be applied. > > >>>> > > >>>> The corresponding device tree binding documentation was introduced by > > >>>> commit f31ee3c0a555 ("wilc1000: Document enable-gpios and reset-gpios > > >>>> properties") and correctly indicates that the reset line is an active-low > > >>>> signal. However, the corresponding driver part, brought by commit > > >>>> ec031ac4792c ("wilc1000: Add reset/enable GPIO support to SPI driver"), is > > >>>> misusing the gpiod APIs and apply an inverted logic when powering up/down > > >>>> the chip (for example, setting the reset line to a logic "1" during power > > >>>> up, which in fact asserts the reset line when device tree describes the > > >>>> reset line as GPIO_ACTIVE_LOW). > > >>> > > >>> Note that commit ec031ac4792c is doing the right thing in regards to an > > >>> ACTIVE_LOW RESET pin and the binding documentation is consistent with that code. > > >>> > > >>> It was later on that commit fcf690b0 flipped the RESET line polarity to treat it > > >>> as GPIO_ACTIVE_HIGH. I never understood why that was done and, as you noted, it > > >>> introduced in inconsistency with the binding documentation. > > >> > > >> Ah, you are right, and I was wrong citing your GPIOs patch as faulty > > >> (git-blaming too fast !), thanks for the clarification. I missed this patch from > > >> Ajay (fcf690b0) flipping the reset logic. Maybe he had issues while missing > > >> proper device tree configuration and then submitted this flip ? > > > > > > Indeed, it was done to align the code as per the DT entry suggested in > > > WILC1000/3000 porting guide[1 -page 18], which is already used by most > > > of the existing users. This change has impact on the users who are using > > > DT entry from porting guide. One approach is to retain the current code > > > and document this if needed. > > > > So if I'm understanding the situation correctly Microchip's porting > > guide[1] doesn't match with kernel.org documentation[2]? I'm not the > > expert here but from my point of view the issue is clear: the code needs > > to follow kernel.org documentation[2], not external documentation. > > My point of view would definitely be that drivers in the mainline kernel > absolutely should respect the ABI defined in the dt-binding. What a vendor > decides to do in their own tree I suppose is their problem, but I would > advocate that vendor kernels would also respect the ABI from mainline. > > Looking a bit more closely at the porting guide, it contains other > properties that are not present in the dt-binding - undocumented > compatibles and a different enable gpio property for example. > I guess it (and the vendor version of the driver) never got updated when > wilc1000 supported landed in mainline? > > > I'll add devicetree list so hopefully people there can comment also, > > full patch available in [3]. > > > > Alexis, if there are no more comments I'm in favor submitting the revert > > you mentioned. > > From a dt-bindings point of view, the aforementioned revert seems > correct and would be > Acked-by: Conor Dooley <conor.dooley@microchip.com> Maybe an R-b is more suitable here, too used to acking trivial patches that are dt related.. > > Getting off my dt-binding maintainer high-horse, linux4microchip is going > be updating to a 6.6 based kernel in the coming weeks - maybe that's a > good time to update the vendor kernel wilc drivers (and therefore the > porting guide?) to match the properties used by mainline Ajay? > > Cheers, > Conor.
Conor Dooley <conor@kernel.org> writes: >> > So if I'm understanding the situation correctly Microchip's porting >> > guide[1] doesn't match with kernel.org documentation[2]? I'm not the >> > expert here but from my point of view the issue is clear: the code needs >> > to follow kernel.org documentation[2], not external documentation. >> >> My point of view would definitely be that drivers in the mainline kernel >> absolutely should respect the ABI defined in the dt-binding. What a vendor >> decides to do in their own tree I suppose is their problem, but I would >> advocate that vendor kernels would also respect the ABI from mainline. >> >> Looking a bit more closely at the porting guide, it contains other >> properties that are not present in the dt-binding - undocumented >> compatibles and a different enable gpio property for example. >> I guess it (and the vendor version of the driver) never got updated when >> wilc1000 supported landed in mainline? >> >> > I'll add devicetree list so hopefully people there can comment also, >> > full patch available in [3]. >> > >> > Alexis, if there are no more comments I'm in favor submitting the revert >> > you mentioned. >> >> From a dt-bindings point of view, the aforementioned revert seems >> correct and would be >> Acked-by: Conor Dooley <conor.dooley@microchip.com> > > Maybe an R-b is more suitable here, too used to acking trivial patches > that are dt related.. On the contrary, I think Acked-by is the right thing here and makes it easier for Alexis and me. Thanks!
On 2/16/24 11:07, Kalle Valo wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Conor Dooley <conor@kernel.org> writes: > >>>> So if I'm understanding the situation correctly Microchip's porting >>>> guide[1] doesn't match with kernel.org documentation[2]? I'm not the >>>> expert here but from my point of view the issue is clear: the code needs >>>> to follow kernel.org documentation[2], not external documentation. >>> >>> My point of view would definitely be that drivers in the mainline kernel >>> absolutely should respect the ABI defined in the dt-binding. What a vendor >>> decides to do in their own tree I suppose is their problem, but I would >>> advocate that vendor kernels would also respect the ABI from mainline. >>> >>> Looking a bit more closely at the porting guide, it contains other >>> properties that are not present in the dt-binding - undocumented >>> compatibles and a different enable gpio property for example. >>> I guess it (and the vendor version of the driver) never got updated when >>> wilc1000 supported landed in mainline? >>> >>>> I'll add devicetree list so hopefully people there can comment also, >>>> full patch available in [3]. >>>> >>>> Alexis, if there are no more comments I'm in favor submitting the revert >>>> you mentioned. >>> >>> From a dt-bindings point of view, the aforementioned revert seems >>> correct and would be >>> Acked-by: Conor Dooley <conor.dooley@microchip.com> >> >> Maybe an R-b is more suitable here, too used to acking trivial patches >> that are dt related.. > > On the contrary, I think Acked-by is the right thing here and makes it > easier for Alexis and me. Thanks! Acked-by: Ajay Singh <ajay.kathat@microchip.com> Agree, we can go ahead with this patch to make the code inline with kernel.org documentation. I don't think any change is required in dt-binding definition after this patch. However external documentation update is needed as Conor has also pointed out, I will be taking care of it. Regards, Ajay
diff --git a/drivers/net/wireless/microchip/wilc1000/spi.c b/drivers/net/wireless/microchip/wilc1000/spi.c index c92ee4b73a74..30eed2ea523d 100644 --- a/drivers/net/wireless/microchip/wilc1000/spi.c +++ b/drivers/net/wireless/microchip/wilc1000/spi.c @@ -192,11 +192,11 @@ static void wilc_wlan_power(struct wilc *wilc, bool on) /* assert ENABLE: */ gpiod_set_value(gpios->enable, 1); mdelay(5); - /* assert RESET: */ - gpiod_set_value(gpios->reset, 1); - } else { /* deassert RESET: */ gpiod_set_value(gpios->reset, 0); + } else { + /* assert RESET: */ + gpiod_set_value(gpios->reset, 1); /* deassert ENABLE: */ gpiod_set_value(gpios->enable, 0); }
When using a wilc1000 chip over a spi bus, users can optionally define a reset gpio and a chip enable gpio. The reset line of wilc1000 is active low, so to hold the chip in reset, a low (physical) value must be applied. The corresponding device tree binding documentation was introduced by commit f31ee3c0a555 ("wilc1000: Document enable-gpios and reset-gpios properties") and correctly indicates that the reset line is an active-low signal. However, the corresponding driver part, brought by commit ec031ac4792c ("wilc1000: Add reset/enable GPIO support to SPI driver"), is misusing the gpiod APIs and apply an inverted logic when powering up/down the chip (for example, setting the reset line to a logic "1" during power up, which in fact asserts the reset line when device tree describes the reset line as GPIO_ACTIVE_LOW). As a consequence, any platform currently using the driver in SPI mode must use a faulty reset line description in device tree, or else chip will be maintained in reset and will not even allow to bring up the chip. Fix reset line usage by inverting back the gpiod APIs usage, setting the reset line to the logic value "0" when powering the chip, and the logic value "1" when powering off the chip. Fixes: ec031ac4792c ("wilc1000: Add reset/enable GPIO support to SPI driver") Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com> --- This issue was detected because I struggled a bit to setup a WILC-over-SPI setup, and eventually realized that it was due to chip being hold in reset. This patch, if accepted, will force any WILC-over-SPI user to update its device tree description: any platform currently working correctly in this setup likely have a wrong GPIO_ACTIVE_HIGH used on the reset line device tree description, contrary to what the documentation says. I am not sure whether this is tolerable ? If not, what would be the proper way to fix this ? Make the driver manually parse this flag and somehow make it able to deal with both versions (so basically: ignoring the setting) ? Just live with it, and possibly document the issue somewhere ? --- drivers/net/wireless/microchip/wilc1000/spi.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) --- base-commit: 246a8c611ace197f43ecc6ea4936c6ca363b8aaa change-id: 20240119-wilc_1000_reset_line-393270fc474e Best regards,