Hi,
Am 13.08.2020 um 02:51 schrieb Nick Elsmore nicholaselsmore@gmail.com:
Hi all,
I have identified a few bugs and features I would like to fix and add to the existing bq2429x driver. They are listed below. To begin this effort, I am first submitting a number of patches to clean the driver up and prepare it for eventual submission to mainline.
Identified bugs:
- At system startup, device information is not populated by the power_supply subsystem and is not present in sysfs. It takes 30s to multiple minutes before this information is available.
Looks good to have this.
- Interrupts/poll events are often missed, possibly due to hangups in the work queue.
I haven't observed this in my tests. Do you have a specific example? Or a theory when and why it can happen?
- Plug/unplug events are not properly detected, due to either work queue hangups as described above or faulty logic determining plug status
Yes, plug/unplug detection is quite some guesswork. A better strategy would be to connect to the USB-PHY...
- Watchdog is left on (if not disabled by bootloader) and not serviced.
It should be disabled by bootloader and kept in disabled state by the driver.
Contrary to what we all would suspect it is not a safety watchdog which would e.g. cut processor power or charging after a kernel panic or stall. It just a mechanism to auto-reset the bq2429x registers to default values which could also be done through writing all registers. So I do not see how to use it for practical purposes.
If IRQ or poll events in the work queue are not serviced due to the hangups described above, the watchdog will trigger.
This is something we must better understand: why it can happen at all that events are not serviced. Maybe we are missing a lock?
This is not properly handled.
- The following properties are missing:
- POWER_SUPPLY_PROP_HEALTH
I am not sure if the charger can report a reasonable value for health.
- POWER_SUPPLY_PROP_PRECHARGE_CURRENT
- POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT
ok!
Generally I find the patches good from a cursory glance, but I have to do a life check since it is not easy to see from the git diffs, what is changed (any why) and if all functions are transformed correctly. This takes at least until next week.
BR and thanks, Nikolaus
Hi Nikolaus,
- Interrupts/poll events are often missed, possibly due to hangups in the
work queue.
I haven't observed this in my tests. Do you have a specific example? Or a theory when and why it can happen?
I instrumented the driver to print before var_lock was acquired (I left loglevel below debug, because currently there are lots of debug prints which flood the output and make it difficult to filter out whats important). I noticed that I would often not see bq2429x_usb_detect executing until a number of seconds or minutes after I plugged or unplugged the USB OTG. If I plugged or unplugged multiple times, I would eventually get all of the calls occurring at once after the delay. This leads me to believe that the workqueue is hanging somewhere.
Interestingly, I would sometimes see no new interrupts for this device triggered in /proc/interrupts. This contradicts the theory that the issue is a hangup in the workqueue, but it could be a combination of both issues in the workqueue and IRQ handler.
- Plug/unplug events are not properly detected, due to either work queue
hangups as described above or faulty logic determining plug status
Yes, plug/unplug detection is quite some guesswork. A better strategy would be to connect to the USB-PHY...
I do not see why we would need to connect to the USB-PHY. Interrupts from the bq2429x should be sufficient to determine whether or not the bq2429x is connected to VBUS and whether or not the bq2429x has determined this to be a good power source and is using it. I have patches which implement the correct logic for handling this in my queue.
- Watchdog is left on (if not disabled by bootloader) and not serviced.
It should be disabled by bootloader and kept in disabled state by the driver.
Contrary to what we all would suspect it is not a safety watchdog which would e.g. cut processor power or charging after a kernel panic or stall. It just a mechanism to auto-reset the bq2429x registers to default values which could also be done through writing all registers. So I do not see how to use it for practical purposes.
Yes. We should not assume, however, that the bootloader has disabled the watchdog. We should make sure its disabled.
This is not properly handled.
- The following properties are missing:
- POWER_SUPPLY_PROP_HEALTH
I am not sure if the charger can report a reasonable value for health.
We'll see. I have patches in my queue that determine reasonable values for health, but we will see how they fare in review.
Generally I find the patches good from a cursory glance, but I have to do a life check since it is not easy to see from the git diffs, what is changed (any why) and if all functions are transformed correctly. This takes at least until next week.
Yep, while these patches don't (shouldn't) modify any of the logic, they are rather large and introduce a good amount of restructuring. I have submitted these patches first before integrating any logic modifications as its possible they have introduced regression. From my tests, the behavior seems the same with and without the patches.
Thanks, Nick Elsmore
Hi,
Am 13.08.2020 um 17:51 schrieb Nick Elsmore nicholaselsmore@gmail.com:
Hi Nikolaus,
- Interrupts/poll events are often missed, possibly due to hangups in the work queue.
I haven't observed this in my tests. Do you have a specific example? Or a theory when and why it can happen?
I instrumented the driver to print before var_lock was acquired (I left loglevel below debug, because currently there are lots of debug prints which flood the output and make it difficult to filter out whats important). I noticed that I would often not see bq2429x_usb_detect executing until a number of seconds or minutes after I plugged or unplugged the USB OTG. If I plugged or unplugged multiple times, I would eventually get all of the calls occurring at once after the delay. This leads me to believe that the workqueue is hanging somewhere.
Interestingly, I would sometimes see no new interrupts for this device triggered in /proc/interrupts. This contradicts the theory that the issue is a hangup in the workqueue, but it could be a combination of both issues in the workqueue and IRQ handler.
Well, the interrupt is going through the pcal6524 and I am not convinced if it handles all IRQs properly. Last time I did look there was no level interrrupt and just edge triggered. This imposes a possibility to loose interrupts if they come too quickly or multiple interrupts are not processed in one batch. So the trouble may come from there.
We should have connected it directly to some omap5 gpio but the idea with replaceable processor board made us think it is better to go through the pcal6524 to be more universal.
- Plug/unplug events are not properly detected, due to either work queue hangups as described above or faulty logic determining plug status
Yes, plug/unplug detection is quite some guesswork. A better strategy would be to connect to the USB-PHY...
I do not see why we would need to connect to the USB-PHY.
The idea is that the USB-PHY should already reliably detect VBUS changes.
Interrupts from the bq2429x should be sufficient to determine whether or not the bq2429x is connected to VBUS and whether or not the bq2429x has determined this to be a good power source and is using it. I have patches which implement the correct logic for handling this in my queue.
Yes, if we can make it reliable without PHY interfacing, it would be much better of course.
- Watchdog is left on (if not disabled by bootloader) and not serviced.
It should be disabled by bootloader and kept in disabled state by the driver.
Contrary to what we all would suspect it is not a safety watchdog which would e.g. cut processor power or charging after a kernel panic or stall. It just a mechanism to auto-reset the bq2429x registers to default values which could also be done through writing all registers. So I do not see how to use it for practical purposes.
Yes. We should not assume, however, that the bootloader has disabled the watchdog. We should make sure its disabled.
Indeed, we should explicitly disable it.
Maybe I had code to disable it and then did some experiments with enabled watchdog to make use of it. And after finding that the watchdog has no good use-case I may have removed the disabling code :)
This is not properly handled.
- The following properties are missing:
- POWER_SUPPLY_PROP_HEALTH
I am not sure if the charger can report a reasonable value for health.
We'll see. I have patches in my queue that determine reasonable values for health, but we will see how they fare in review.
Generally I find the patches good from a cursory glance, but I have to do a life check since it is not easy to see from the git diffs, what is changed (any why) and if all functions are transformed correctly. This takes at least until next week.
Yep, while these patches don't (shouldn't) modify any of the logic, they are rather large and introduce a good amount of restructuring. I have submitted these patches first before integrating any logic modifications as its possible they have introduced regression.
That is fine and I want to take a look at it before we continue. This should hopefully avoid regressions before we submit it...
What we certainly have to fix/adjust is the bq2429x.yaml bindings document. I had started to write one
https://git.goldelico.com/?p=letux-kernel.git;a=commit;h=1c094ca615be1a5ff7e...
but I am not sure if we can enforce some values in the monitored battery node or formally define the defaults for it... This must also pass make dt_binding_check dtbs_check
BR and thanks, Nikolaus
From my tests, the behavior seems the same with and without the patches.
Thanks, Nick Elsmore _______________________________________________ https://projects.goldelico.com/p/gta04-kernel/ Letux-kernel mailing list Letux-kernel@openphoenux.org http://lists.goldelico.com/mailman/listinfo.cgi/letux-kernel
Hi Nick, I have started a little to test your patches on top of v5.8-rc7 (including also David's patches so that the Panel is working again). There was a missing comma - easy to fix for compilation.
Basically the driver seems to work, but OTG doesn't. It does not even try to switch power on, although DWC3 detects it.
Unplugging the OTG cable once did lead to:
root@letux:~# [ 397.149733] ------------[ cut here ]------------ [ 397.154707] WARNING: CPU: 0 PID: 37 at drivers/regulator/core.c:2603 _regulator_disable+0x3c/0x168 [ 397.170413] unbalanced disables for otg_path
And once the temperature report was 60°C.
So I think there is something wrong with interrupts / status change detection. Just a note: I have tested on an older Pyra prototype with tca6424 and there, IRQs are definitively broken. So we must poll the status bits.
Really nice is reading the bit fields from
cat /sys/class/power_supply/bq24297/device/registers
Much better information than the pure register values.
Still, using regmap still allows to read them the "old" way for comparison (except the volatile ones):
cat /sys/kernel/debug/regmap/1-006b/registers
There are differences in registers 0 and 1 compared to the old driver. The other registers are the same (except 8 and 9).
But this is just a first and quick "hello" test, nothing fundamental. I'll try to find out more in the next days as I find time. And after upgrading to v5.9-rc1.
BR and thanks, Nikolaus
Am 13.08.2020 um 17:51 schrieb Nick Elsmore nicholaselsmore@gmail.com:
Hi Nikolaus,
- Interrupts/poll events are often missed, possibly due to hangups in the work queue.
I haven't observed this in my tests. Do you have a specific example? Or a theory when and why it can happen?
I instrumented the driver to print before var_lock was acquired (I left loglevel below debug, because currently there are lots of debug prints which flood the output and make it difficult to filter out whats important). I noticed that I would often not see bq2429x_usb_detect executing until a number of seconds or minutes after I plugged or unplugged the USB OTG. If I plugged or unplugged multiple times, I would eventually get all of the calls occurring at once after the delay. This leads me to believe that the workqueue is hanging somewhere.
Interestingly, I would sometimes see no new interrupts for this device triggered in /proc/interrupts. This contradicts the theory that the issue is a hangup in the workqueue, but it could be a combination of both issues in the workqueue and IRQ handler.
- Plug/unplug events are not properly detected, due to either work queue hangups as described above or faulty logic determining plug status
Yes, plug/unplug detection is quite some guesswork. A better strategy would be to connect to the USB-PHY...
I do not see why we would need to connect to the USB-PHY. Interrupts from the bq2429x should be sufficient to determine whether or not the bq2429x is connected to VBUS and whether or not the bq2429x has determined this to be a good power source and is using it. I have patches which implement the correct logic for handling this in my queue.
- Watchdog is left on (if not disabled by bootloader) and not serviced.
It should be disabled by bootloader and kept in disabled state by the driver.
Contrary to what we all would suspect it is not a safety watchdog which would e.g. cut processor power or charging after a kernel panic or stall. It just a mechanism to auto-reset the bq2429x registers to default values which could also be done through writing all registers. So I do not see how to use it for practical purposes.
Yes. We should not assume, however, that the bootloader has disabled the watchdog. We should make sure its disabled.
This is not properly handled.
- The following properties are missing:
- POWER_SUPPLY_PROP_HEALTH
I am not sure if the charger can report a reasonable value for health.
We'll see. I have patches in my queue that determine reasonable values for health, but we will see how they fare in review.
Generally I find the patches good from a cursory glance, but I have to do a life check since it is not easy to see from the git diffs, what is changed (any why) and if all functions are transformed correctly. This takes at least until next week.
Yep, while these patches don't (shouldn't) modify any of the logic, they are rather large and introduce a good amount of restructuring. I have submitted these patches first before integrating any logic modifications as its possible they have introduced regression. From my tests, the behavior seems the same with and without the patches.
Thanks, Nick Elsmore _______________________________________________ Kernel mailing list Kernel@pyra-handheld.com http://pyra-handheld.com/cgi-bin/mailman/listinfo/kernel
Am 16.08.2020 um 22:09 schrieb H. Nikolaus Schaller hns@goldelico.com:
And after upgrading to v5.9-rc1.
has been pushed 20 minutes ago...
Hi Nikolaus,
I also saw the high temperature issue on 5.6 when charging sometimes - I guess you know already from your previous bq work but "60°" just means too hot to charge as there is no exact readout. This also resulted in a "HOT" print from the driver.
Maybe we should be lowering the charge current a bit for safer and more reliable charging, I don't know how much control over this the bq gives. The battery didn't feel that hot externally, but the thermistor will be sensing the temperature inside the pack which will be higher.
Best
David
On Sun, 2020-08-16 at 22:09 +0200, H. Nikolaus Schaller wrote:
Hi Nick, I have started a little to test your patches on top of v5.8-rc7 (including also David's patches so that the Panel is working again). There was a missing comma - easy to fix for compilation.
Basically the driver seems to work, but OTG doesn't. It does not even try to switch power on, although DWC3 detects it.
Unplugging the OTG cable once did lead to:
root@letux:~# [ 397.149733] ------------[ cut here ]------------ [ 397.154707] WARNING: CPU: 0 PID: 37 at drivers/regulator/core.c:2603 _regulator_disable+0x3c/0x168 [ 397.170413] unbalanced disables for otg_path
And once the temperature report was 60°C.
So I think there is something wrong with interrupts / status change detection. Just a note: I have tested on an older Pyra prototype with tca6424 and there, IRQs are definitively broken. So we must poll the status bits.
Really nice is reading the bit fields from
cat /sys/class/power_supply/bq24297/device/registers
Much better information than the pure register values.
Still, using regmap still allows to read them the "old" way for comparison (except the volatile ones):
cat /sys/kernel/debug/regmap/1-006b/registers
There are differences in registers 0 and 1 compared to the old driver. The other registers are the same (except 8 and 9).
But this is just a first and quick "hello" test, nothing fundamental. I'll try to find out more in the next days as I find time. And after upgrading to v5.9-rc1.
BR and thanks, Nikolaus
Am 13.08.2020 um 17:51 schrieb Nick Elsmore nicholaselsmore@gmail.com:
Hi Nikolaus,
- Interrupts/poll events are often missed, possibly due to hangups in the
work queue.
I haven't observed this in my tests. Do you have a specific example? Or a theory when and why it can happen?
I instrumented the driver to print before var_lock was acquired (I left loglevel below debug, because currently there are lots of debug prints which flood the output and make it difficult to filter out whats important). I noticed that I would often not see bq2429x_usb_detect executing until a number of seconds or minutes after I plugged or unplugged the USB OTG. If I plugged or unplugged multiple times, I would eventually get all of the calls occurring at once after the delay. This leads me to believe that the workqueue is hanging somewhere.
Interestingly, I would sometimes see no new interrupts for this device triggered in /proc/interrupts. This contradicts the theory that the issue is a hangup in the workqueue, but it could be a combination of both issues in the workqueue and IRQ handler.
- Plug/unplug events are not properly detected, due to either work queue
hangups as described above or faulty logic determining plug status
Yes, plug/unplug detection is quite some guesswork. A better strategy would be to connect to the USB-PHY...
I do not see why we would need to connect to the USB-PHY. Interrupts from the bq2429x should be sufficient to determine whether or not the bq2429x is connected to VBUS and whether or not the bq2429x has determined this to be a good power source and is using it. I have patches which implement the correct logic for handling this in my queue.
- Watchdog is left on (if not disabled by bootloader) and not serviced.
It should be disabled by bootloader and kept in disabled state by the driver.
Contrary to what we all would suspect it is not a safety watchdog which would e.g. cut processor power or charging after a kernel panic or stall. It just a mechanism to auto-reset the bq2429x registers to default values which could also be done through writing all registers. So I do not see how to use it for practical purposes.
Yes. We should not assume, however, that the bootloader has disabled the watchdog. We should make sure its disabled.
This is not properly handled.
- The following properties are missing:
- POWER_SUPPLY_PROP_HEALTH
I am not sure if the charger can report a reasonable value for health.
We'll see. I have patches in my queue that determine reasonable values for health, but we will see how they fare in review.
Generally I find the patches good from a cursory glance, but I have to do a life check since it is not easy to see from the git diffs, what is changed (any why) and if all functions are transformed correctly. This takes at least until next week.
Yep, while these patches don't (shouldn't) modify any of the logic, they are rather large and introduce a good amount of restructuring. I have submitted these patches first before integrating any logic modifications as its possible they have introduced regression. From my tests, the behavior seems the same with and without the patches.
Thanks, Nick Elsmore _______________________________________________ Kernel mailing list Kernel@pyra-handheld.com http://pyra-handheld.com/cgi-bin/mailman/listinfo/kernel
Kernel mailing list Kernel@pyra-handheld.com http://pyra-handheld.com/cgi-bin/mailman/listinfo/kernel
Am 16.08.2020 um 22:33 schrieb David Shah dave@ds0.me:
Hi Nikolaus,
I also saw the high temperature issue on 5.6 when charging sometimes - I guess you know already from your previous bq work but "60°" just means too hot to charge as there is no exact readout. This also resulted in a "HOT" print from the driver.
Yes, there are only three levels: too cold, of, too hot. And these are mapped to -10°C, 22.5°C and 60°C.
But the battery wasn't charging (battery is full) and after a reboot it did read out correctly. So it was a driver glitch. And I never had that with the old driver.
I remember that these bits reporting NTC temperature are not easy to be handled correctly by the driver so the rework may have changed something subtle.
Maybe we should be lowering the charge current a bit for safer and more reliable charging, I don't know how much control over this the bq gives. The battery didn't feel that hot externally, but the thermistor will be sensing the temperature inside the pack which will be higher.
If we would (could) do a redesign, there are successor chips of the bq2429x which have a real ADC that can be read out through i2c so that we would know the temperature much better...
BR, Nikolaus
Best
David
On Sun, 2020-08-16 at 22:09 +0200, H. Nikolaus Schaller wrote:
Hi Nick, I have started a little to test your patches on top of v5.8-rc7 (including also David's patches so that the Panel is working again). There was a missing comma - easy to fix for compilation.
Basically the driver seems to work, but OTG doesn't. It does not even try to switch power on, although DWC3 detects it.
Unplugging the OTG cable once did lead to:
root@letux:~# [ 397.149733] ------------[ cut here ]------------ [ 397.154707] WARNING: CPU: 0 PID: 37 at drivers/regulator/core.c:2603 _regulator_disable+0x3c/0x168 [ 397.170413] unbalanced disables for otg_path
And once the temperature report was 60°C.
So I think there is something wrong with interrupts / status change detection. Just a note: I have tested on an older Pyra prototype with tca6424 and there, IRQs are definitively broken. So we must poll the status bits.
Really nice is reading the bit fields from
cat /sys/class/power_supply/bq24297/device/registers
Much better information than the pure register values.
Still, using regmap still allows to read them the "old" way for comparison (except the volatile ones):
cat /sys/kernel/debug/regmap/1-006b/registers
There are differences in registers 0 and 1 compared to the old driver. The other registers are the same (except 8 and 9).
But this is just a first and quick "hello" test, nothing fundamental. I'll try to find out more in the next days as I find time. And after upgrading to v5.9-rc1.
BR and thanks, Nikolaus
Am 13.08.2020 um 17:51 schrieb Nick Elsmore nicholaselsmore@gmail.com:
Hi Nikolaus,
- Interrupts/poll events are often missed, possibly due to hangups in the
work queue.
I haven't observed this in my tests. Do you have a specific example? Or a theory when and why it can happen?
I instrumented the driver to print before var_lock was acquired (I left loglevel below debug, because currently there are lots of debug prints which flood the output and make it difficult to filter out whats important). I noticed that I would often not see bq2429x_usb_detect executing until a number of seconds or minutes after I plugged or unplugged the USB OTG. If I plugged or unplugged multiple times, I would eventually get all of the calls occurring at once after the delay. This leads me to believe that the workqueue is hanging somewhere.
Interestingly, I would sometimes see no new interrupts for this device triggered in /proc/interrupts. This contradicts the theory that the issue is a hangup in the workqueue, but it could be a combination of both issues in the workqueue and IRQ handler.
- Plug/unplug events are not properly detected, due to either work queue
hangups as described above or faulty logic determining plug status
Yes, plug/unplug detection is quite some guesswork. A better strategy would be to connect to the USB-PHY...
I do not see why we would need to connect to the USB-PHY. Interrupts from the bq2429x should be sufficient to determine whether or not the bq2429x is connected to VBUS and whether or not the bq2429x has determined this to be a good power source and is using it. I have patches which implement the correct logic for handling this in my queue.
- Watchdog is left on (if not disabled by bootloader) and not serviced.
It should be disabled by bootloader and kept in disabled state by the driver.
Contrary to what we all would suspect it is not a safety watchdog which would e.g. cut processor power or charging after a kernel panic or stall. It just a mechanism to auto-reset the bq2429x registers to default values which could also be done through writing all registers. So I do not see how to use it for practical purposes.
Yes. We should not assume, however, that the bootloader has disabled the watchdog. We should make sure its disabled.
This is not properly handled.
- The following properties are missing:
- POWER_SUPPLY_PROP_HEALTH
I am not sure if the charger can report a reasonable value for health.
We'll see. I have patches in my queue that determine reasonable values for health, but we will see how they fare in review.
Generally I find the patches good from a cursory glance, but I have to do a life check since it is not easy to see from the git diffs, what is changed (any why) and if all functions are transformed correctly. This takes at least until next week.
Yep, while these patches don't (shouldn't) modify any of the logic, they are rather large and introduce a good amount of restructuring. I have submitted these patches first before integrating any logic modifications as its possible they have introduced regression. From my tests, the behavior seems the same with and without the patches.
Thanks, Nick Elsmore _______________________________________________ Kernel mailing list Kernel@pyra-handheld.com http://pyra-handheld.com/cgi-bin/mailman/listinfo/kernel
Kernel mailing list Kernel@pyra-handheld.com http://pyra-handheld.com/cgi-bin/mailman/listinfo/kernel
I also saw the high temperature issue on 5.6 when charging sometimes - I
guess
you know already from your previous bq work but "60°" just means too hot
to
charge as there is no exact readout. This also resulted in a "HOT" print
from
the driver.
Yes, there are only three levels: too cold, of, too hot. And these are mapped to -10°C, 22.5°C and 60°C.
But the battery wasn't charging (battery is full) and after a reboot it did read out correctly. So it was a driver glitch. And I never had that with the old driver.
I remember that these bits reporting NTC temperature are not easy to be handled correctly by the driver so the rework may have changed something subtle.
I have seen this on the old driver on 5.6.y. I will see if I can reproduce again on the old driver and get a register dump and some other information. Re-examining the logic in the restructure patches, the reporting in the NTC fields looks ok. So there may be something in the configuration of the chip causing these NTC bits to be set. Have you dumped the registers when you reproduce this issue to see if the actual values are as expected?
I have started a little to test your patches on top of v5.8-rc7
(including
also David's patches so that the Panel is working again). There was a missing comma - easy to fix for compilation.
Yes I noticed that, sorry. Something broke in my submission.
Basically the driver seems to work, but OTG doesn't. It does not even
try to
switch power on, although DWC3 detects it.
Unplugging the OTG cable once did lead to:
root@letux:~# [ 397.149733] ------------[ cut here ]------------ [ 397.154707] WARNING: CPU: 0 PID: 37 at drivers/regulator/core.c:2603 _regulator_disable+0x3c/0x168 [ 397.170413] unbalanced disables for otg_path
This is interesting. I don't have a device handy to test OTG, so this is probably why I didn't catch this. I'll try and get a device to test OTG mode with.
And once the temperature report was 60°C.
So I think there is something wrong with interrupts / status change
detection.
Just a note: I have tested on an older Pyra prototype with tca6424 and
there,
IRQs are definitively broken. So we must poll the status bits.
Also interesting, I'll need to investigate what broke with the interrupts. My original driver rewrite behaved quite well on 4.19.
Hi Nick,
Am 22.08.2020 um 00:55 schrieb Nick Elsmore nicholaselsmore@gmail.com:
I also saw the high temperature issue on 5.6 when charging sometimes - I guess you know already from your previous bq work but "60°" just means too hot to charge as there is no exact readout. This also resulted in a "HOT" print from the driver.
Yes, there are only three levels: too cold, of, too hot. And these are mapped to -10°C, 22.5°C and 60°C.
But the battery wasn't charging (battery is full) and after a reboot it did read out correctly. So it was a driver glitch. And I never had that with the old driver.
I remember that these bits reporting NTC temperature are not easy to be handled correctly by the driver so the rework may have changed something subtle.
I have seen this on the old driver on 5.6.y. I will see if I can reproduce again on the old driver and get a register dump and some other information. Re-examining the logic in the restructure patches, the reporting in the NTC fields looks ok. So there may be something in the configuration of the chip causing these NTC bits to be set. Have you dumped the registers when you reproduce this issue to see if the actual values are as expected?
No. It was just a very quick (ca. 5 minutes) look at it: apply patches, compile, compile fix, boot and look what happens...
I have started a little to test your patches on top of v5.8-rc7 (including also David's patches so that the Panel is working again). There was a missing comma - easy to fix for compilation.
Yes I noticed that, sorry. Something broke in my submission.
No problem. Such things happen...
Basically the driver seems to work, but OTG doesn't. It does not even try to switch power on, although DWC3 detects it.
Unplugging the OTG cable once did lead to:
root@letux:~# [ 397.149733] ------------[ cut here ]------------ [ 397.154707] WARNING: CPU: 0 PID: 37 at drivers/regulator/core.c:2603 _regulator_disable+0x3c/0x168 [ 397.170413] unbalanced disables for otg_path
This is interesting. I don't have a device handy to test OTG, so this is probably why I didn't catch this. I'll try and get a device to test OTG mode with.
And once the temperature report was 60°C.
So I think there is something wrong with interrupts / status change detection. Just a note: I have tested on an older Pyra prototype with tca6424 and there, IRQs are definitively broken. So we must poll the status bits.
Also interesting, I'll need to investigate what broke with the interrupts. My original driver rewrite behaved quite well on 4.19.
I just did two initial tests. Maybe I find more time after rebasing everything to v5.9-rc2 this week. And add some printk to understand what is happening around OTG...
BR and thanks, Nikolaus
Hi Nick, I am currently testing the (new driver) with 5.9-rc2. The spurious problems have disappeared, maybe some -rc1 issues.
I already have some smaller fixes, e.g. for polled mode (which stops if there is no state change, i.e. almost immediately). There are also small bugs in usb detection and setting the input current limit.
What I am currently hunting for is that vbus becoming available now properly sets the registers to charging, but charging does not start before I read back the status. Strange. Looks like the chip needs some additional trigger.
And OTG is being programmed, the registers look fine but does not start according to REG08 vbus_stat. Even if there is no HOT report. So I have to look into the old driver what it is doing differently.
Regarding "HOT" I indeed sometimes see the hot indication. The bq27421 reports ca. 38°C which is the chip temperature (not the battery).
Therefore the battery may be hotter. And I found a notice in the data sheet that our choice of R709/R710 will reach the HOT point at 45°C.
In addition we have R2001 and R2002 as a voltage divider so that the GPADC can also read the temperature. What came to my mind is that we have an additional resistor in parallel to the NTC and R710 which likely reduces the HOT point further. A 10k NTC and the 133k Voltage divider makes ca. 7% deviation. So to optimize that for a 45°C point we should modify R709/710.
It is quite likely that the charger stops charging (and OTG) at >40°C. Which is good for battery life and safety but not so good for charging the Pyra while it is running... Especially in hot environment (outdoor, car, bike) or while using WWAN in weak radio situation (where RF power and heat dissipation of the module goes up). I think there is a new "energy management" framework coming to Linux which should make it easier to manage heat sources (processor OPPs etc.) depending on temperature.
So far another intermediate result. Let's continue testing...
BR, Nikolaus
Am 24.08.2020 um 18:18 schrieb H. Nikolaus Schaller hns@goldelico.com:
Hi Nick,
Am 22.08.2020 um 00:55 schrieb Nick Elsmore nicholaselsmore@gmail.com:
I also saw the high temperature issue on 5.6 when charging sometimes - I guess you know already from your previous bq work but "60°" just means too hot to charge as there is no exact readout. This also resulted in a "HOT" print from the driver.
Yes, there are only three levels: too cold, of, too hot. And these are mapped to -10°C, 22.5°C and 60°C.
But the battery wasn't charging (battery is full) and after a reboot it did read out correctly. So it was a driver glitch. And I never had that with the old driver.
I remember that these bits reporting NTC temperature are not easy to be handled correctly by the driver so the rework may have changed something subtle.
I have seen this on the old driver on 5.6.y. I will see if I can reproduce again on the old driver and get a register dump and some other information. Re-examining the logic in the restructure patches, the reporting in the NTC fields looks ok. So there may be something in the configuration of the chip causing these NTC bits to be set. Have you dumped the registers when you reproduce this issue to see if the actual values are as expected?
No. It was just a very quick (ca. 5 minutes) look at it: apply patches, compile, compile fix, boot and look what happens...
I have started a little to test your patches on top of v5.8-rc7 (including also David's patches so that the Panel is working again). There was a missing comma - easy to fix for compilation.
Yes I noticed that, sorry. Something broke in my submission.
No problem. Such things happen...
Basically the driver seems to work, but OTG doesn't. It does not even try to switch power on, although DWC3 detects it.
Unplugging the OTG cable once did lead to:
root@letux:~# [ 397.149733] ------------[ cut here ]------------ [ 397.154707] WARNING: CPU: 0 PID: 37 at drivers/regulator/core.c:2603 _regulator_disable+0x3c/0x168 [ 397.170413] unbalanced disables for otg_path
This is interesting. I don't have a device handy to test OTG, so this is probably why I didn't catch this. I'll try and get a device to test OTG mode with.
And once the temperature report was 60°C.
So I think there is something wrong with interrupts / status change detection. Just a note: I have tested on an older Pyra prototype with tca6424 and there, IRQs are definitively broken. So we must poll the status bits.
Also interesting, I'll need to investigate what broke with the interrupts. My original driver rewrite behaved quite well on 4.19.
I just did two initial tests. Maybe I find more time after rebasing everything to v5.9-rc2 this week. And add some printk to understand what is happening around OTG...
BR and thanks, Nikolaus
Hi, I finally have fixed the OTG mode. What is still a little unstable is turning on charging.
And what I am not sure about is handling the F_EN_HIZ bit when switching between OTG and Client mode. It may not be properly reset to the state before OTG mode (i.e. input_current_limit = 0mA if that was defined through writing sysfs).
Code is now rebased on letux-5.9-rc3 which still includes the older code.
Your patch set sits on top of that and I have appended my fixes and tried to write good commit messages so that it should be easy to follow and discuss. The patches including yours are
letux-letux-5.9-rc3..work-bq2429x
in this branch:
https://git.goldelico.com/?p=letux-kernel.git;a=shortlog;h=refs/heads/work-b...
If this works well enough (or has got additional fixes) I think we can squash everything into 3 or 4 patches that can replace letux/bq2429x-v5 i.e. become letux/bq2429x-v6.
Then we can do fine-tuning to make it ready for upstream submission.
I think that this can and should be done as a first step to submit Pyra specific stuff. For upstream support of the Pyra device tree the main blocking point is the MIPI DSI driver which does not yet support DRM panels (and I do not want to submit it without any display support). But we won't get our panel driver upstream before it has been converted.
So we have to wait until omapdrm supports generic MIPI DRM panels and we have converted our panel driver. Then my plan is to submit the panel driver and finally an 80% complete Pyra device tree. The remaining 20% are for further study like nub driver, sensors, mmc switch, tiler rotation, etc. how long do we have to wait for that? Maybe v5.11 or v5.12 i.e. 6-9 months. Depends on how quickly Sebastian Reichel is doing the Droid 4 work...
Thanks for converting the driver to regmap, which makes it much easier and safer to compare code to the data sheet description.
BR, Nikolaus
Am 22.08.2020 um 00:55 schrieb Nick Elsmore nicholaselsmore@gmail.com:
I also saw the high temperature issue on 5.6 when charging sometimes - I guess you know already from your previous bq work but "60°" just means too hot to charge as there is no exact readout. This also resulted in a "HOT" print from the driver.
Yes, there are only three levels: too cold, of, too hot. And these are mapped to -10°C, 22.5°C and 60°C.
But the battery wasn't charging (battery is full) and after a reboot it did read out correctly. So it was a driver glitch. And I never had that with the old driver.
I remember that these bits reporting NTC temperature are not easy to be handled correctly by the driver so the rework may have changed something subtle.
I have seen this on the old driver on 5.6.y. I will see if I can reproduce again on the old driver and get a register dump and some other information. Re-examining the logic in the restructure patches, the reporting in the NTC fields looks ok. So there may be something in the configuration of the chip causing these NTC bits to be set. Have you dumped the registers when you reproduce this issue to see if the actual values are as expected?
I have started a little to test your patches on top of v5.8-rc7 (including also David's patches so that the Panel is working again). There was a missing comma - easy to fix for compilation.
Yes I noticed that, sorry. Something broke in my submission.
Basically the driver seems to work, but OTG doesn't. It does not even try to switch power on, although DWC3 detects it.
Unplugging the OTG cable once did lead to:
root@letux:~# [ 397.149733] ------------[ cut here ]------------ [ 397.154707] WARNING: CPU: 0 PID: 37 at drivers/regulator/core.c:2603 _regulator_disable+0x3c/0x168 [ 397.170413] unbalanced disables for otg_path
This is interesting. I don't have a device handy to test OTG, so this is probably why I didn't catch this. I'll try and get a device to test OTG mode with.
And once the temperature report was 60°C.
So I think there is something wrong with interrupts / status change detection. Just a note: I have tested on an older Pyra prototype with tca6424 and there, IRQs are definitively broken. So we must poll the status bits.
Also interesting, I'll need to investigate what broke with the interrupts. My original driver rewrite behaved quite well on 4.19.