| Message ID | CABatt_wQpbtktD=bXwJCzdm_5aLeHcQrSW2pNMRergC9jZ0sMw@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SPI driver probe problem during boot | expand |
On Tue, May 5, 2020 at 11:04 AM Martin Townsend <mtownsend1973@gmail.com> wrote: > I've just finished debugging a SPI comms problem for a TPM device on a > TI Sitara AM4372 SoC. The SPI bus has 3 devices > > 1) CAN FD Controller 0 (Using native CS) > 2) CAN FD Controller 1 (Using GPIO for CS) > 3) TPM Device (Using GPIO for CS) > > All CS are active low. > > During boot the TPM would fail it's probe but if I unbind and then > rebind the driver after boot the TPM would work fine so I got the > logic analyser out to probe the SPI bus and could see that the voltage > on the MISO line was half the expected voltage whilst the TPM was > being probed and this was due to CS0 being driven low for this period > of time. After the TPM was probed the CS0 went high. After debugging > this I found that the OMAP SPI controller defaults internal CS lines > to active high so when the SPI master controller is initialised this > is the state of CS0 so it's driven low for inactive. During the probe > of the SPI master controller it calls devm_spi_register_master -> > spi_register_master which calls of_register_spi_devices which will add > the devices to the SPI master controller via spi_add_device. This > function would call device_add. Due to the way the device tree is > parsed the SPI devices above are enumerated from the highest CS to the > lowest so the TPM device is setup first. When device_add is called it > triggers it's probe function but the SPI bus hasn't been setup yet and > hence the TPM driver tries to communicate with the TPM device whilst > CS0 is being driven low causing the CAN FD controller to also respond > reducing the voltage on the MISO line. In spi_device_add it calls > > status = spi_setup(spi); > > which would setup the CS lines so I modified the > of_register_spi_devices function to make it a 2 phase operation so the > CS lines are all setup and then it iterates of the SPI devices a > second time to add them to the driver model via device_add and the TPM > now probes fine. ... > Now this is on a oldish kernel (4.12) and moving the kernel forward > isn't trivial. I was just wondering if this has been fixed already so > I could backport it, I couldn't see anything in the latest kernel but > maybe it has been solved a different way. If not is there a better > way of fixing this? Or is the OMAP SPI controller driver the problem, > should it parse the child nodes first and set itself up accordingly? Can you confirm the issue on v5.7-rc4?
On Tue, May 5, 2020 at 9:43 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Tue, May 5, 2020 at 11:04 AM Martin Townsend <mtownsend1973@gmail.com> wrote: > > > I've just finished debugging a SPI comms problem for a TPM device on a > > TI Sitara AM4372 SoC. The SPI bus has 3 devices > > > > 1) CAN FD Controller 0 (Using native CS) > > 2) CAN FD Controller 1 (Using GPIO for CS) > > 3) TPM Device (Using GPIO for CS) > > > > All CS are active low. > > > > During boot the TPM would fail it's probe but if I unbind and then > > rebind the driver after boot the TPM would work fine so I got the > > logic analyser out to probe the SPI bus and could see that the voltage > > on the MISO line was half the expected voltage whilst the TPM was > > being probed and this was due to CS0 being driven low for this period > > of time. After the TPM was probed the CS0 went high. After debugging > > this I found that the OMAP SPI controller defaults internal CS lines > > to active high so when the SPI master controller is initialised this > > is the state of CS0 so it's driven low for inactive. During the probe > > of the SPI master controller it calls devm_spi_register_master -> > > spi_register_master which calls of_register_spi_devices which will add > > the devices to the SPI master controller via spi_add_device. This > > function would call device_add. Due to the way the device tree is > > parsed the SPI devices above are enumerated from the highest CS to the > > lowest so the TPM device is setup first. When device_add is called it > > triggers it's probe function but the SPI bus hasn't been setup yet and > > hence the TPM driver tries to communicate with the TPM device whilst > > CS0 is being driven low causing the CAN FD controller to also respond > > reducing the voltage on the MISO line. In spi_device_add it calls > > > > status = spi_setup(spi); > > > > which would setup the CS lines so I modified the > > of_register_spi_devices function to make it a 2 phase operation so the > > CS lines are all setup and then it iterates of the SPI devices a > > second time to add them to the driver model via device_add and the TPM > > now probes fine. > > ... > > > Now this is on a oldish kernel (4.12) and moving the kernel forward > > isn't trivial. I was just wondering if this has been fixed already so > > I could backport it, I couldn't see anything in the latest kernel but > > maybe it has been solved a different way. If not is there a better > > way of fixing this? Or is the OMAP SPI controller driver the problem, > > should it parse the child nodes first and set itself up accordingly? > > Can you confirm the issue on v5.7-rc4? > I will try but it's not always possible with these embedded SoC's without a lot of work. > -- > With Best Regards, > Andy Shevchenko
On Tue, May 5, 2020 at 12:47 PM Martin Townsend <mtownsend1973@gmail.com> wrote: > On Tue, May 5, 2020 at 9:43 AM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Tue, May 5, 2020 at 11:04 AM Martin Townsend <mtownsend1973@gmail.com> wrote: ... > > > Now this is on a oldish kernel (4.12) and moving the kernel forward > > > isn't trivial. I was just wondering if this has been fixed already so > > > I could backport it, I couldn't see anything in the latest kernel but > > > maybe it has been solved a different way. If not is there a better > > > way of fixing this? Or is the OMAP SPI controller driver the problem, > > > should it parse the child nodes first and set itself up accordingly? > > > > Can you confirm the issue on v5.7-rc4? > > > > I will try but it's not always possible with these embedded SoC's > without a lot of work. I understand that, but if issue is fixed the proper (but might be painful and long) way is to bisect to the fix and try to backport. Otherwise it will need to be fixed in newest available kernel first. So, in either case you should test on latest available.
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 89254a5..1012933 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -545,14 +545,6 @@ int spi_add_device(struct spi_device *spi) goto done; } - /* Device may be bound to an active driver when this returns */ - status = device_add(&spi->dev); - if (status < 0) - dev_err(dev, "can't add %s, status %d\n", - dev_name(&spi->dev), status); - else - dev_dbg(dev, "registered child %s\n", dev_name(&spi->dev)); - done: mutex_unlock(&spi_add_lock); return status; @@ -614,6 +606,9 @@ struct spi_device *spi_new_device(struct spi_master *master, status = spi_add_device(proxy); if (status < 0) goto err_remove_props; + status = device_add(&proxy->dev); + if (status < 0) + goto err_remove_props; return proxy; @@ -1663,12 +1658,19 @@ of_register_spi_device(struct spi_master *master, struct device_node *nc) */ static void of_register_spi_devices(struct spi_master *master) { + struct spi_device_list_elem { + struct spi_device *spi_dev; + struct list_head list; + }; + struct spi_device_list_elem *spi_dev_elem; struct spi_device *spi; struct device_node *nc; + struct list_head spi_devices, *elem, *tmp; if (!master->dev.of_node) return; + INIT_LIST_HEAD(&spi_devices); for_each_available_child_of_node(master->dev.of_node, nc) { if (of_node_test_and_set_flag(nc, OF_POPULATED))
Hi, I've just finished debugging a SPI comms problem for a TPM device on a TI Sitara AM4372 SoC. The SPI bus has 3 devices 1) CAN FD Controller 0 (Using native CS) 2) CAN FD Controller 1 (Using GPIO for CS) 3) TPM Device (Using GPIO for CS) All CS are active low. During boot the TPM would fail it's probe but if I unbind and then rebind the driver after boot the TPM would work fine so I got the logic analyser out to probe the SPI bus and could see that the voltage on the MISO line was half the expected voltage whilst the TPM was being probed and this was due to CS0 being driven low for this period of time. After the TPM was probed the CS0 went high. After debugging this I found that the OMAP SPI controller defaults internal CS lines to active high so when the SPI master controller is initialised this is the state of CS0 so it's driven low for inactive. During the probe of the SPI master controller it calls devm_spi_register_master -> spi_register_master which calls of_register_spi_devices which will add the devices to the SPI master controller via spi_add_device. This function would call device_add. Due to the way the device tree is parsed the SPI devices above are enumerated from the highest CS to the lowest so the TPM device is setup first. When device_add is called it triggers it's probe function but the SPI bus hasn't been setup yet and hence the TPM driver tries to communicate with the TPM device whilst CS0 is being driven low causing the CAN FD controller to also respond reducing the voltage on the MISO line. In spi_device_add it calls status = spi_setup(spi); which would setup the CS lines so I modified the of_register_spi_devices function to make it a 2 phase operation so the CS lines are all setup and then it iterates of the SPI devices a second time to add them to the driver model via device_add and the TPM now probes fine. Here's the change I made (it won't apply I'm afraid as gmail doesn't like pasting tabs) continue; @@ -1678,6 +1680,30 @@ static void of_register_spi_devices(struct spi_master *master) nc->full_name); of_node_clear_flag(nc, OF_POPULATED); } + spi_dev_elem = kmalloc(sizeof(*spi_dev_elem), GFP_KERNEL); + if (spi_dev_elem == NULL) { + printk(KERN_ERR "of_register_spi_devices : failed to allocate spi dev elem\n"); + continue; + } + spi_dev_elem->spi_dev = spi; + list_add_tail(&spi_dev_elem->list, &spi_devices); + } + + list_for_each_safe(elem, tmp, &spi_devices) { + int status; + struct spi_device *spi_dev; + + spi_dev_elem = list_entry(elem, struct spi_device_list_elem, list); + spi_dev = spi_dev_elem->spi_dev; + status = device_add(&spi_dev->dev); + if (status < 0) + dev_err(&master->dev, "can't add %s, status %d\n", + dev_name(&spi_dev->dev), status); + else + dev_dbg(&master->dev, "registered child %s\n", dev_name(&spi_dev->dev)); + + list_del(elem); + kfree(elem); } } #else @@ -1772,12 +1798,18 @@ static acpi_status acpi_register_spi_device(struct spi_master *master, acpi_device_set_enumerated(adev); adev->power.flags.ignore_parent = true; - if (spi_add_device(spi)) { - adev->power.flags.ignore_parent = false; - dev_err(&master->dev, "failed to add SPI device %s from ACPI\n", - dev_name(&adev->dev)); - spi_dev_put(spi); - } + if (spi_add_device(spi)) + goto err_put_dev; + if (device_add(&proxy->dev)) + goto err_put_dev; + + return AE_OK; + +err_put_dev: + adev->power.flags.ignore_parent = false; + dev_err(&master->dev, "failed to add SPI device %s from ACPI\n", + dev_name(&adev->dev)); + spi_dev_put(spi); return AE_OK; } Now this is on a oldish kernel (4.12) and moving the kernel forward isn't trivial. I was just wondering if this has been fixed already so I could backport it, I couldn't see anything in the latest kernel but maybe it has been solved a different way. If not is there a better way of fixing this? Or is the OMAP SPI controller driver the problem, should it parse the child nodes first and set itself up accordingly? I'm more than happy to cleanup the hack I have put in place and submit it if you think it's an acceptable solution. Regards, Martin.