Re: [PATCH 0/9] MIPS: CI20: Add WiFi / Bluetooth support

Re: [PATCH 0/9] MIPS: CI20: Add WiFi / Bluetooth support

[Thomas Bogendoerfer]

On Thu, Jun 15, 2023 at 10:39:53AM +0200, Paul Cercueil wrote:
> Thomas: are you able to drop this series from mips-next, or should I/we send fixup patches instead?

as I'm not rebasing mips-next I need fixup patches. This won't solve
bisectability, but not doing rebases is the what Linus prefers.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH 0/9] MIPS: CI20: Add WiFi / Bluetooth support

[H. Nikolaus Schaller]

> Am 15.06.2023 um 11:08 schrieb Thomas Bogendoerfer <tsbogend@alpha.franken.de>:
> 
> On Thu, Jun 15, 2023 at 10:39:53AM +0200, Paul Cercueil wrote:
>> Thomas: are you able to drop this series from mips-next, or should I/we send fixup patches instead?
> 
> as I'm not rebasing mips-next I need fixup patches. This won't solve
> bisectability, but not doing rebases is the what Linus prefers.

Indeed. I have found some very old statements on this topic:
https://yarchive.net/comp/linux/git_rebase.html

> > How do I insert build fixes into existing changesets so that the tree
> > is more bisectable?
> 
> Just delay pushing out. There really is _zero_ downside to this. None at
> all. There are only upsides.

...

> Also, I'd *much* rather have a few problems in the tree than have people
> screw up history in order to hide them. Sure, we want to keep things
> bisectable, but quite frankly, if you do a reasonable job and compile the
> kernel before you push out, it will be "mostly bisectable".
> 
> And yes, mistakes happen. Mistakes will *always* happen. It's ok. Relax.

...

> So in short:
> 
>  - clean trees and bisectability are all wonderful things. No doubt about
>    that at all.
> 
>  - but if getting there means that you lose a lot of _other_ wonderful
>    things (like being able to trust history, and the people being under
>    your watchful eyes having to deal with you re-writing their trees),
>    we'd be much better off taking the occasional commit that fixes things
>    up _after_ the fact rather than before!
> 
>  - and you actually can help fix your issues by doing some simple things
>    *before* pushing out, rather than push out immediately. IOW, do your
>    whitespace sanity fixes, your compile checks etc early, and don't push
>    out until after you've done them.

...

> So:
>  - making things public is *different* from developing them. Don't push
>    out just because you committed something!
> 
>  - you shouldn't publicize a tree until it's in reasonable shape. EVER.
>    Even -mm or -next is *not* better off with a pile of sh*t just because
>    you're working in that area.
> 
>    I cannot stress this enough. I think Andrew has been way too polite to
>    some people.
> 
>  - and once it is, you generally shouldn't mess with old commits even when
>    you fix things. Full cleanliness or always being able to bisect
>    specific configurations is not an excuse for messing up all the other
>    things, and if this problem happens a lot, I would like to point you to
>    the two previous points.
> 

...

> And btw, a *big* part of the above is also:
> 
>  - mistakes happen.
> 
> There will be bugs. There will be cases where things aren't bisectable
> (although they should generally be bisectable for *your* configuration,
> because if they aren't, that shows that you didn't even compile the
> commits you made).
> 
> And there will be kernels that don't boot. Even expecting people to always
> boot-test every single commit would be unrealistic - let's face it, most
> things look really obvious, and the fact that even obvious fixes can have
> bugs doesn't mean that there should be hard rules about "every single
> commit has to be boot-tested on X machines".
> 
> So it's an important part of the process to try to do a good job, and not
> publicizing crap - but it's *equally* important to realize that crap
> happens, and that it's easily *more* distracting to try to clean it up
> after-the-fact than it is to just admit that it happened.

Ok, then we have to keep this series as is and fix it on top (just for
my V2a board since it seems to work for Paul for a different version).

So let's focus on the fixes.

Best regards,
Nikolaus


> 
> Thomas.
> 
> -- 
> Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
> good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH 0/9] MIPS: CI20: Add WiFi / Bluetooth support

[H. Nikolaus Schaller]

Hi Paul,

> Am 15.06.2023 um 10:39 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> Hi Nikolaus,
> 
> Le 15 juin 2023 09:00, "H. Nikolaus Schaller" <hns@goldelico.com> a écrit :
>> 
>> Hi Paul, 
>> I was in holidays and could not review this series earlier. 
>> 
>> 
>>> Am 04.06.2023 um 16:56 schrieb Paul Cercueil <paul@crapouillou.net>: 
>>> 
>>> Hi, 
>>> 
>>> Here's a set of patches to add support for the WiFi / Bluetooth chip on 
>>> the CI20. 
>>> 
>>> WiFi works pretty well, provided it is used with the latest firmware 
>>> provided by linux-firmware. Bluetooth does not work very well here, as 
>>> I cannot get my wireless keyboard to pair; but it does detect it, and it 
>>> does see they key presses when I type the pairing code. 
>>> 
>>> I only tested with a somewhat recent (~2022) Buildroot-based userspace. 
>>> I enabled WEXT compatibility because the CI20 is typically used with a 
>>> very old userspace, but I did not try to use it with old tools like 
>>> ifconfig/iwconfig. 
>> 
>> ^^^ great since not everyone is using memory hungry latest user-space and 
>> ifconfig/iwconfig is also available on some other OS so users like me 
>> can share scripts. 
>> 
>> 
>> But I had quite some issues with this series. 
>> 
>> 1. I could not boot my CI20 V2a board after applying the full series to v6.4-rc6 
>> 2. bisecting failed because vcc_33v is used in a patch preceding its definition 
>>    leading to DTC compile abort 
>> 3. after fixing I could bisect that "MIPS: DTS: CI20: Fix ACT8600 regulator node names" 
>>    is the first bad commit - although I don't see immediately why 
>> 
>> So this series seems to be severely broken and I could not even come to 
>> a test of WiFi and/or Bluetooth which the series claims to support. 
> 
> Well, that's strange. I can assure you it boots and works here.
> 
> Maybe it is a board difference; I have a non-square green board - so the earlier version.

Ok, my V2a is the bordeaux one. That may indeed make a difference.

> 
> Since this patch will actually make the various ACT8600 regulators work at their specified voltage, maybe the voltage on one of the updated regulators is wrong?
> 
> Maybe change the regulators one by one back to their old name, until you find the one that is problematic? That may give us more info.

That is what I also have though about but have not yet done.
Will try as soon as I find a time slot.

> 
>> Comments to some individual patches follow. 
> 
> Sorry about the vcc_33v. Honest mistake.
> 
> Thomas: are you able to drop this series from mips-next, or should I/we send fixup patches instead?

Well, mistakes happen.

Best regards,
Nikolaus

> 
> Cheers,
> -Paul
> 
>> Best regards and looking forward to a v2 for testing, 
>> Nikolaus 
>> 
>> 
>>> 
>>> Cheers, 
>>> -Paul 
>>> 
>>> Paul Cercueil (9): 
>>>   MIPS: DTS: CI20: Fix regulators 
>>>   MIPS: DTS: CI20: Fix ACT8600 regulator node names 
>>>   MIPS: DTS: CI20: Add parent supplies to ACT8600 regulators 
>> 
>> ^^^ should IMHO be a separate series since it is not directly related to WiFi / Bluetooth 
>> 
>>>   MIPS: DTS: CI20: Do not force-enable CIM and WiFi regulators 
>>>   MIPS: DTS: CI20: Misc. cleanups 
>> 
>> ^^^ these two do not compile 
>> The Misc. cleanups do not belong to this topic. 
>> 
>>>   MIPS: DTS: CI20: Parent MSCMUX clock to MPLL 
>> 
>> ^^^ this is only loosely related to Wifi / Bluetooth 
>> 
>>>   MIPS: DTS: CI20: Enable support for WiFi / Bluetooth 
>>>   MIPS: configs: CI20: Regenerate defconfig 
>>>   MIPS: configs: CI20: Enable WiFi / Bluetooth 
>>> 
>>> arch/mips/boot/dts/ingenic/ci20.dts | 148 +++++++++++++++++++--------- 
>>> arch/mips/configs/ci20_defconfig    |  47 ++++++--- 
>>> 2 files changed, 133 insertions(+), 62 deletions(-) 
>>> 
>>> -- 
>>> 2.39.2 
>>> 
>> 

Re: [PATCH 0/9] MIPS: CI20: Add WiFi / Bluetooth support

[H. Nikolaus Schaller]

Hi Pual,


> Am 15.06.2023 um 10:45 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
> 
> Hi Paul,
> 
>> Am 15.06.2023 um 10:39 schrieb Paul Cercueil <paul@crapouillou.net>:
>> 
>> Hi Nikolaus,
>> 
>> Le 15 juin 2023 09:00, "H. Nikolaus Schaller" <hns@goldelico.com> a écrit :
>>> 
>>> Hi Paul, 
>>> I was in holidays and could not review this series earlier. 
>>> 
>>> 
>>>> Am 04.06.2023 um 16:56 schrieb Paul Cercueil <paul@crapouillou.net>: 
>>>> 
>>>> Hi, 
>>>> 
>>>> Here's a set of patches to add support for the WiFi / Bluetooth chip on 
>>>> the CI20. 
>>>> 
>>>> WiFi works pretty well, provided it is used with the latest firmware 
>>>> provided by linux-firmware. Bluetooth does not work very well here, as 
>>>> I cannot get my wireless keyboard to pair; but it does detect it, and it 
>>>> does see they key presses when I type the pairing code. 
>>>> 
>>>> I only tested with a somewhat recent (~2022) Buildroot-based userspace. 
>>>> I enabled WEXT compatibility because the CI20 is typically used with a 
>>>> very old userspace, but I did not try to use it with old tools like 
>>>> ifconfig/iwconfig. 
>>> 
>>> ^^^ great since not everyone is using memory hungry latest user-space and 
>>> ifconfig/iwconfig is also available on some other OS so users like me 
>>> can share scripts. 
>>> 
>>> 
>>> But I had quite some issues with this series. 
>>> 
>>> 1. I could not boot my CI20 V2a board after applying the full series to v6.4-rc6 
>>> 2. bisecting failed because vcc_33v is used in a patch preceding its definition 
>>>   leading to DTC compile abort 
>>> 3. after fixing I could bisect that "MIPS: DTS: CI20: Fix ACT8600 regulator node names" 
>>>   is the first bad commit - although I don't see immediately why 
>>> 
>>> So this series seems to be severely broken and I could not even come to 
>>> a test of WiFi and/or Bluetooth which the series claims to support. 
>> 
>> Well, that's strange. I can assure you it boots and works here.
>> 
>> Maybe it is a board difference; I have a non-square green board - so the earlier version.
> 
> Ok, my V2a is the bordeaux one. That may indeed make a difference.
> 
>> 
>> Since this patch will actually make the various ACT8600 regulators work at their specified voltage, maybe the voltage on one of the updated regulators is wrong?
>> 
>> Maybe change the regulators one by one back to their old name, until you find the one that is problematic? That may give us more info.
> 
> That is what I also have though about but have not yet done.
> Will try as soon as I find a time slot.

I have reverted the whole patch (had only a conflict in wifi_io / LDO6) and now I can boot.

But do not see a WiFi or Bluetooth interface.

So it looks as if the CI20 variants are indeed different. Which would also explain why we
originally came up with two different solutions to add WiFi.

Next I will try to bisect the individual changes...

> 
>> 
>>> Comments to some individual patches follow. 
>> 
>> Sorry about the vcc_33v. Honest mistake.
>> 
>> Thomas: are you able to drop this series from mips-next, or should I/we send fixup patches instead?
> 
> Well, mistakes happen.
> 
> Best regards,
> Nikolaus
> 
>> 
>> Cheers,
>> -Paul
>> 
>>> Best regards and looking forward to a v2 for testing, 
>>> Nikolaus 
>>> 
>>> 
>>>> 
>>>> Cheers, 
>>>> -Paul 
>>>> 
>>>> Paul Cercueil (9): 
>>>>  MIPS: DTS: CI20: Fix regulators 
>>>>  MIPS: DTS: CI20: Fix ACT8600 regulator node names 
>>>>  MIPS: DTS: CI20: Add parent supplies to ACT8600 regulators 
>>> 
>>> ^^^ should IMHO be a separate series since it is not directly related to WiFi / Bluetooth 
>>> 
>>>>  MIPS: DTS: CI20: Do not force-enable CIM and WiFi regulators 
>>>>  MIPS: DTS: CI20: Misc. cleanups 
>>> 
>>> ^^^ these two do not compile 
>>> The Misc. cleanups do not belong to this topic. 
>>> 
>>>>  MIPS: DTS: CI20: Parent MSCMUX clock to MPLL 
>>> 
>>> ^^^ this is only loosely related to Wifi / Bluetooth 
>>> 
>>>>  MIPS: DTS: CI20: Enable support for WiFi / Bluetooth 
>>>>  MIPS: configs: CI20: Regenerate defconfig 
>>>>  MIPS: configs: CI20: Enable WiFi / Bluetooth 
>>>> 
>>>> arch/mips/boot/dts/ingenic/ci20.dts | 148 +++++++++++++++++++--------- 
>>>> arch/mips/configs/ci20_defconfig    |  47 ++++++--- 
>>>> 2 files changed, 133 insertions(+), 62 deletions(-) 
>>>> 
>>>> -- 
>>>> 2.39.2 
>>>> 
>>> 
> 

Re: [PATCH 0/9] MIPS: CI20: Add WiFi / Bluetooth support

[H. Nikolaus Schaller]

Hi Paul,

>>> 
>>> Since this patch will actually make the various ACT8600 regulators work at their specified voltage, maybe the voltage on one of the updated regulators is wrong?
>>> 
>>> Maybe change the regulators one by one back to their old name, until you find the one that is problematic? That may give us more info.
>> 
>> That is what I also have though about but have not yet done.
>> Will try as soon as I find a time slot.
> 
> I have reverted the whole patch (had only a conflict in wifi_io / LDO6) and now I can boot.
> 
> But do not see a WiFi or Bluetooth interface.
> 
> So it looks as if the CI20 variants are indeed different. Which would also explain why we
> originally came up with two different solutions to add WiFi.
> 
> Next I will try to bisect the individual changes...

It is this and not the regulator names:

diff --git a/arch/mips/boot/dts/ingenic/ci20.dts b/arch/mips/boot/dts/ingenic/ci20.dts
index e2221d44e4269..391be48e6427a 100644
--- a/arch/mips/boot/dts/ingenic/ci20.dts
+++ b/arch/mips/boot/dts/ingenic/ci20.dts
@@ -295,7 +295,6 @@ &i2c0 {
        act8600: act8600@5a {
                compatible = "active-semi,act8600";
                reg = <0x5a>;
-               status = "okay";
 
                regulators {
                        vddcore: SUDCDC1 {


Now I wonder how it works without status = "okay" for you but not for me.

Does your test branch have additional patches which add this back?

Or does your board variant have better or different burnt in defaults than
my act8600 so that it runs without any driver?

The chip reads as:

ACTIVE
8601QJ
MD361

BR,
Nikolaus

Re: [PATCH 0/9] MIPS: CI20: Add WiFi / Bluetooth support

[H. Nikolaus Schaller]

Hi Paul,

> 
>>>> Since this patch will actually make the various ACT8600 regulators work at their specified voltage, maybe the voltage on one of the updated regulators is wrong?
>>>> 
>>>> Maybe change the regulators one by one back to their old name, until you find the one that is problematic? That may give us more info.
>>> 
>>> That is what I also have thought about but have not yet done.
>>> Will try as soon as I find a time slot.
>> 
>> I have reverted the whole patch (had only a conflict in wifi_io / LDO6) and now I can boot.
>> 
>> But do not see a WiFi or Bluetooth interface.
>> 
>> So it looks as if the CI20 variants are indeed different. Which would also explain why we
>> originally came up with two different solutions to add WiFi.
>> 
>> Next I will try to bisect the individual changes...
> 
> It is this and not the regulator names:
> 
> diff --git a/arch/mips/boot/dts/ingenic/ci20.dts b/arch/mips/boot/dts/ingenic/ci20.dts
> index e2221d44e4269..391be48e6427a 100644
> --- a/arch/mips/boot/dts/ingenic/ci20.dts
> +++ b/arch/mips/boot/dts/ingenic/ci20.dts
> @@ -295,7 +295,6 @@ &i2c0 {
>        act8600: act8600@5a {
>                compatible = "active-semi,act8600";
>                reg = <0x5a>;
> -               status = "okay";
> 
>                regulators {
>                        vddcore: SUDCDC1 {
> 
> 
> Now I wonder how it works without status = "okay" for you but not for me.

I have not found a reason for this and it was difficult to repeat. Potentially a bisect
with failed boot and wrong setup of some voltages may have damaged the file system on my
SD card (see below). At least I had to fsck -f after running the bisect, but I did not
do it for every bisect step. Sometimes bisecting is difficult if hardware effects are
involved...

So I started with the series + a revert of the offending patch, added some more logging
to the kernel and printk() in the driver.

Results:

- driver is always loaded, so the status = "okay" was spurious and not the problem.

- Adding/Removing the regulator names also does not make a difference.

- But renaming the DT nodes (e.g. SUDCDC1 -> DCDC1) (with or without regulator_name) makes
boot hang with strange errors which indicate that the processor power supply is not stable.
Once a while it did even automatically reboot. In most cases there are some EXT4 errors
afterwards.

Example:

[    3.003096] EXT4-fs error (device mmcblk0p1): ext4_mb_generate_buddy:1100: group 81, block bitmap and bg descriptor inconsistent: 30994 vs 31229 free clusters
/sbin/init: error while loading shared libraries: /lib/mipsel-li
[    3.291901] Kernel panic - not syncing: Attempted to kill init! exitcode=0x00007f00
[    3.305122] Rebooting in 10 seconds..

I have not found a reason but it appears that if the DT nodes do match the
struct regulator_desc list, it is different from having them not match.

At least the result of regulator_of_get_init_data() is NULL if there is no
match and then some other code path is followed in regulator_register().

So at the moment I think that matching DT node names with the act8600_regulators[] list
changes something in the chip initialization which has influence depending on hardware
variation. Maybe your board is simply more robust than mine to that.

Deeper analysis will reveal the issue and indicate a solution...

BR,
Nikolaus

Re: [PATCH 0/9] MIPS: CI20: Add WiFi / Bluetooth support

[H. Nikolaus Schaller]

Hi Paul,

> Am 16.06.2023 um 22:21 schrieb H. Nikolaus Schaller <hns@goldelico.com>:

> - But renaming the DT nodes (e.g. SUDCDC1 -> DCDC1) (with or without regulator_name) makes
> boot hang with strange errors which indicate that the processor power supply is not stable.
> Once a while it did even automatically reboot. In most cases there are some EXT4 errors
> afterwards.

I am coming closer, I think. I have now touched only the DCDC1 node name.

a) with "SUDCDC1" -> "DCDC1" (bad bood):

regulator_of_get_init_node() returns the child node

Then:
[    0.666962] act8865 0-005a: Looking up vp1-supply from device tree
[    0.673191] DCDC1: supplied by vcc_33v
[    0.727070] DCDC1: Bringing 1200000uV into 1100000-1100000uV
[    0.739398] DCDC1: 1100 mV, enabled

b) without patch/series or reverted (good boot):

regulator_of_get_init_node() returns NULL

Then:
[    1.016487] DCDC1: at 1200 mV, enabled
[    1.020578] act8865 0-005a: Looking up vp1-supply from device tree
[    1.026917] DCDC1: supplied by vcc_33v

So at least for my board the patched series seems to reduce DCDC1 voltage
to 1.1V which may trigger the boot and stability problems on my board while
it is fine for yours. This could explain the hardware dependency.

Now I have no data sheets or information which voltages are the right ones
and where the 1200mV come from (most likely some default programmed
into the PMU chip).

And the issue seems to be that without matching the node names the
voltages in the device tree may have been ignored completely all the
time... Now it sets up voltages, which should happen. But different
ones for my board which breaks boot.

Finally I did risk (I have no replacement CI20 board and they are no longer
on sale... RS part# was 125-3305 Mouser 456-VL-62851) to run a test with
rename to "DCDC1" but changing the voltage to 1200mV. And this version boots.

Still without WiFi/Bluetooth but that may be related to missing rename
of the other regulators.

So I tried renaming all regulators as by your [PATCH 2/9], and now I
see something from WiFi (haven't installed firmware yet) and the Bluetooth chip:

[    1.977876] mmc1: new high speed SDIO card at address 0001

[   11.341994] Bluetooth: hci0: BCM: chip id 62
[   11.348811] Bluetooth: hci0: BCM: features 0x0f
[   11.376698] Bluetooth: hci0: BCM4330B1
[   11.380662] Bluetooth: hci0: BCM4330B1 (002.001.003) build 0000
[   11.392053] Bluetooth: hci0: BCM4330B1 'brcm/BCM4330B1.hcd' Patch

[   12.145330] brcmfmac mmc1:0001:1: Direct firmware load for brcm/brcmfmac4330-sdio.img,ci20.bin failed with error -2
[   12.208001] brcmfmac mmc1:0001:1: Direct firmware load for brcm/brcmfmac4330-sdio.clm_blob failed with error -2

Unfortunatley systemd bailed out starting Bluetooth service but
failed to provide a login:

In summary it looks like a potential fix could be to replace the DCDC1
min/max range by 1.0 - 1.2V instead of 1.1 - 1.1V but we need deeper
understanding first. Usually this has something to do with dynamic voltage
scaling depending on processor clock and lower voltages are only allowed
for lower frequencies but max. clock requires the highest possible voltage.
AFAIK we have no cpufreq integrated and therefore always run at max. speed.

BR,
Nikolaus

PS: here is what I read back from the regulator voltages (for DCDC1  min/max = 1.2V):

root@letux:~# cat /sys/kernel/debug/regulator/regulator_summary
 regulator                      use open bypass  opmode voltage current     min     max
---------------------------------------------------------------------------------------
 regulator-dummy                  1    0      0 unknown     0mV     0mA     0mV     0mV 
 eth0_power                       1    1      0 unknown  3300mV     0mA  3300mV  3300mV 
    16000000.dm9000-vcc           1                                 0mA     0mV     0mV
 otg_power                        0    0      0 unknown  5000mV     0mA  5000mV  5000mV 
 vcc_33v                          4    9      0 unknown  3300mV     0mA  3300mV  3300mV 
    13450000.mmc-vqmmc            1                                 0mA     0mV     0mV
    13450000.mmc-vmmc             1                                 0mA  3300mV  3400mV
    DCDC1                         1    0      0 standby  1200mV     0mA  1200mV  1200mV 
    DCDC2                         0    0      0 standby  1500mV     0mA     0mV     0mV 
    DCDC3                         0    0      0 unknown  3300mV     0mA     0mV     0mV 
    LDO5                          0    0      0 unknown  2500mV     0mA     0mV     0mV 
    LDO6                          0    0      0  normal  1800mV     0mA  1800mV  1800mV 
    LDO7                          0    0      0 unknown  3300mV     0mA     0mV     0mV 
    LDO8                          0    0      0 unknown  3300mV     0mA     0mV     0mV 
 SUDCDC_REG4                      0    0      0  normal  5000mV     0mA     0mV     0mV 
 LDO_REG9                         1    0      0 unknown  3300mV     0mA  3300mV  3300mV 
 LDO_REG10                        1    0      0 unknown  1200mV     0mA  1200mV  1200mV 
 bt_power                         0    0      0 unknown  3300mV     0mA  3300mV  3300mV 
root@letux:~# 

This matches device tree except DCDC1, LDO7 and LDO8 (camera).

Re: [PATCH 0/9] MIPS: CI20: Add WiFi / Bluetooth support

[Paul Cercueil]

Hi Nikolaus,

Le samedi 17 juin 2023 à 12:45 +0200, H. Nikolaus Schaller a écrit :
> Hi Paul,
> 
> > Am 16.06.2023 um 22:21 schrieb H. Nikolaus Schaller
> > <hns@goldelico.com>:
> 
> > - But renaming the DT nodes (e.g. SUDCDC1 -> DCDC1) (with or
> > without regulator_name) makes
> > boot hang with strange errors which indicate that the processor
> > power supply is not stable.
> > Once a while it did even automatically reboot. In most cases there
> > are some EXT4 errors
> > afterwards.
> 
> I am coming closer, I think. I have now touched only the DCDC1 node
> name.
> 
> a) with "SUDCDC1" -> "DCDC1" (bad bood):
> 
> regulator_of_get_init_node() returns the child node
> 
> Then:
> [    0.666962] act8865 0-005a: Looking up vp1-supply from device tree
> [    0.673191] DCDC1: supplied by vcc_33v
> [    0.727070] DCDC1: Bringing 1200000uV into 1100000-1100000uV
> [    0.739398] DCDC1: 1100 mV, enabled
> 
> b) without patch/series or reverted (good boot):
> 
> regulator_of_get_init_node() returns NULL
> 
> Then:
> [    1.016487] DCDC1: at 1200 mV, enabled
> [    1.020578] act8865 0-005a: Looking up vp1-supply from device tree
> [    1.026917] DCDC1: supplied by vcc_33v
> 
> So at least for my board the patched series seems to reduce DCDC1
> voltage
> to 1.1V which may trigger the boot and stability problems on my board
> while
> it is fine for yours. This could explain the hardware dependency.
> 
> Now I have no data sheets or information which voltages are the right
> ones
> and where the 1200mV come from (most likely some default programmed
> into the PMU chip).
> 
> And the issue seems to be that without matching the node names the
> voltages in the device tree may have been ignored completely all the
> time... Now it sets up voltages, which should happen. But different
> ones for my board which breaks boot.

So the node names fix caused the driver to actually use the info from
DT, which doesn't allow the board to boot. Nice.

> Finally I did risk (I have no replacement CI20 board and they are no
> longer
> on sale... RS part# was 125-3305 Mouser 456-VL-62851) to run a test
> with
> rename to "DCDC1" but changing the voltage to 1200mV. And this
> version boots.

Looking at the JZ4780_DS.PDF file, the SoC actually wants 1.1V so the
DT is not wrong - in theory. But in practice it does not work, as you
experienced yourself. However, if the ACT8600 defaults to 1.2V, or if
the bootloader configures it to 1.2V, I would think that this is
actually a voltage that the SoC can handle - otherwise the SoC would be
overvolted until the kernel starts, and the board design would be
flawed.

I measured that the old 3.x kernel keeps the SoC voltage at 1.2V, so it
sounds like a better default. Therefore the fix here would be to raise
the DCDC1 regulator to 1.2V.

I'll send a patch later today.

Cheers,
-Paul

> Still without WiFi/Bluetooth but that may be related to missing
> rename
> of the other regulators.
> 
> So I tried renaming all regulators as by your [PATCH 2/9], and now I
> see something from WiFi (haven't installed firmware yet) and the
> Bluetooth chip:
> 
> [    1.977876] mmc1: new high speed SDIO card at address 0001
> 
> [   11.341994] Bluetooth: hci0: BCM: chip id 62
> [   11.348811] Bluetooth: hci0: BCM: features 0x0f
> [   11.376698] Bluetooth: hci0: BCM4330B1
> [   11.380662] Bluetooth: hci0: BCM4330B1 (002.001.003) build 0000
> [   11.392053] Bluetooth: hci0: BCM4330B1 'brcm/BCM4330B1.hcd' Patch
> 
> [   12.145330] brcmfmac mmc1:0001:1: Direct firmware load for
> brcm/brcmfmac4330-sdio.img,ci20.bin failed with error -2
> [   12.208001] brcmfmac mmc1:0001:1: Direct firmware load for
> brcm/brcmfmac4330-sdio.clm_blob failed with error -2
> 
> Unfortunatley systemd bailed out starting Bluetooth service but
> failed to provide a login:
> 
> In summary it looks like a potential fix could be to replace the
> DCDC1
> min/max range by 1.0 - 1.2V instead of 1.1 - 1.1V but we need deeper
> understanding first. Usually this has something to do with dynamic
> voltage
> scaling depending on processor clock and lower voltages are only
> allowed
> for lower frequencies but max. clock requires the highest possible
> voltage.
> AFAIK we have no cpufreq integrated and therefore always run at max.
> speed.
> 
> BR,
> Nikolaus
> 
> PS: here is what I read back from the regulator voltages (for DCDC1 
> min/max = 1.2V):
> 
> root@letux:~# cat /sys/kernel/debug/regulator/regulator_summary
>  regulator                      use open bypass  opmode voltage
> current     min     max
> ---------------------------------------------------------------------
> ------------------
>  regulator-dummy                  1    0      0 unknown     0mV    
> 0mA     0mV     0mV 
>  eth0_power                       1    1      0 unknown  3300mV    
> 0mA  3300mV  3300mV 
>     16000000.dm9000-vcc           1                                
> 0mA     0mV     0mV
>  otg_power                        0    0      0 unknown  5000mV    
> 0mA  5000mV  5000mV 
>  vcc_33v                          4    9      0 unknown  3300mV    
> 0mA  3300mV  3300mV 
>     13450000.mmc-vqmmc            1                                
> 0mA     0mV     0mV
>     13450000.mmc-vmmc             1                                
> 0mA  3300mV  3400mV
>     DCDC1                         1    0      0 standby  1200mV    
> 0mA  1200mV  1200mV 
>     DCDC2                         0    0      0 standby  1500mV    
> 0mA     0mV     0mV 
>     DCDC3                         0    0      0 unknown  3300mV    
> 0mA     0mV     0mV 
>     LDO5                          0    0      0 unknown  2500mV    
> 0mA     0mV     0mV 
>     LDO6                          0    0      0  normal  1800mV    
> 0mA  1800mV  1800mV 
>     LDO7                          0    0      0 unknown  3300mV    
> 0mA     0mV     0mV 
>     LDO8                          0    0      0 unknown  3300mV    
> 0mA     0mV     0mV 
>  SUDCDC_REG4                      0    0      0  normal  5000mV    
> 0mA     0mV     0mV 
>  LDO_REG9                         1    0      0 unknown  3300mV    
> 0mA  3300mV  3300mV 
>  LDO_REG10                        1    0      0 unknown  1200mV    
> 0mA  1200mV  1200mV 
>  bt_power                         0    0      0 unknown  3300mV    
> 0mA  3300mV  3300mV 
> root@letux:~# 
> 
> This matches device tree except DCDC1, LDO7 and LDO8 (camera).
> 

Re: [PATCH 0/9] MIPS: CI20: Add WiFi / Bluetooth support

[Paul Cercueil]

Hi All,

[...]

> Looking at the JZ4780_DS.PDF file, the SoC actually wants 1.1V so the
> DT is not wrong - in theory. But in practice it does not work, as you
> experienced yourself. However, if the ACT8600 defaults to 1.2V, or if
> the bootloader configures it to 1.2V, I would think that this is
> actually a voltage that the SoC can handle - otherwise the SoC would
> be
> overvolted until the kernel starts, and the board design would be
> flawed.
> 
> I measured that the old 3.x kernel keeps the SoC voltage at 1.2V, so
> it
> sounds like a better default. Therefore the fix here would be to
> raise
> the DCDC1 regulator to 1.2V.
> 
> I'll send a patch later today.

After a talk with Christophe (Cc'd), I changed my mind.

A +100 mV overvolt is a *huge* step up, and although the SoC doesn't
burst into flames, it could very well reduce its life span.

I used to have huge stability issues (kernel not booting to userspace
half the times, or just plain reboots after a few minutes of uptime)
and I now realize it's because I was running the core at 1.1V, because
these issues disappeared the moment I switched to 1.2V.

However, I am now running at 1.125 volts, which is just 25mV above the
nominal voltage - and it's been extremely stable so far.

Nikolaus: could you test at 1.125 volts? If it's stable for you as
well, I'd suggest to use this as the new default.

Paul (Burton): As you wrote most of the drivers (and uboot?) for the
board, do you know why VDDCORE was set to 1.2V?

Cheers,
-Paul

Re: [PATCH 0/9] MIPS: CI20: Add WiFi / Bluetooth support

[H. Nikolaus Schaller]

Hi Paul,

> Am 18.06.2023 um 15:58 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> Hi All,
> 
> [...]
> 
>> Looking at the JZ4780_DS.PDF file, the SoC actually wants 1.1V so the
>> DT is not wrong - in theory. But in practice it does not work, as you
>> experienced yourself. However, if the ACT8600 defaults to 1.2V, or if
>> the bootloader configures it to 1.2V, I would think that this is
>> actually a voltage that the SoC can handle - otherwise the SoC would
>> be
>> overvolted until the kernel starts, and the board design would be
>> flawed.
>> 
>> I measured that the old 3.x kernel keeps the SoC voltage at 1.2V, so
>> it
>> sounds like a better default. Therefore the fix here would be to
>> raise
>> the DCDC1 regulator to 1.2V.
>> 
>> I'll send a patch later today.
> 
> After a talk with Christophe (Cc'd), I changed my mind.
> 
> A +100 mV overvolt is a *huge* step up, and although the SoC doesn't
> burst into flames, it could very well reduce its life span.

Well, 1.2V is still within the recommended and absolute limits. See my
previous mail. And it appears that my board simply did run at 1.2V
since I bought it many years ago...

So it should neither burn nor burst into flames since it is no change
at least for my board :)

Anyways running at the lowest possible voltage would be good.
The question is if the driver should enforce this more than e.g. U-Boot.

> 
> I used to have huge stability issues (kernel not booting to userspace
> half the times, or just plain reboots after a few minutes of uptime)

That is exactly what I see with the new 1.10V.

> and I now realize it's because I was running the core at 1.1V, because
> these issues disappeared the moment I switched to 1.2V.

For me as well (and I had 1.2V over the past years).

> 
> However, I am now running at 1.125 volts, which is just 25mV above the
> nominal voltage - and it's been extremely stable so far.

Well, what also could be is that the transient of changing the voltage
from the default 1.20V (it either gets from U-Boot or a preprogrammed
chip setting) to the new 1.1V voltage gives an undershoot.

I remember that I studied the OMAP OPP and dynamic voltage scaling control
some years ago and there it was very critical that voltages and clock
frequencies are changed in a specific sequence and with some delays.

And for the OMAP5 we did find a band within the permitted range where
everything was fine and spurious kernel issues (sudden illegal instructions
or segfaults) outside. The result was that there was a minimum voltage
for low frequencies higher than the maximum voltage for higher frequencies.
So there was not even a single core voltage that could support all
clock frequencies.

> 
> Nikolaus: could you test at 1.125 volts? If it's stable for you as
> well, I'd suggest to use this as the new default.

Yes, this is a good idea!

Especially as there are wires between the regulator output inside
the act chip and the core. There may be a small voltage drop so
that setting 1.10V may be too low and 1.25V may end up in real
1.1V.

> 
> Paul (Burton): As you wrote most of the drivers (and uboot?) for the
> board, do you know why VDDCORE was set to 1.2V?
> 
> Cheers,
> -Paul

Best regards,
Nikolaus

Re: [PATCH 0/9] MIPS: CI20: Add WiFi / Bluetooth support

[H. Nikolaus Schaller]

Hi Paul,

> Am 18.06.2023 um 13:51 schrieb Paul Cercueil <paul@crapouillou.net>:

>> And the issue seems to be that without matching the node names the
>> voltages in the device tree may have been ignored completely all the
>> time... Now it sets up voltages, which should happen. But different
>> ones for my board which breaks boot.
> 
> So the node names fix caused the driver to actually use the info from
> DT, which doesn't allow the board to boot. Nice.

Very good summary :)

As usual the fix is just a one-liner, finding what to do is multi-hours.

> 
>> Finally I did risk (I have no replacement CI20 board and they are no
>> longer
>> on sale... RS part# was 125-3305 Mouser 456-VL-62851) to run a test
>> with
>> rename to "DCDC1" but changing the voltage to 1200mV. And this
>> version boots.
> 
> Looking at the JZ4780_DS.PDF file, the SoC actually wants 1.1V so the
> DT is not wrong - in theory. But in practice it does not work, as you
> experienced yourself. However, if the ACT8600 defaults to 1.2V, or if
> the bootloader configures it to 1.2V, I would think that this is
> actually a voltage that the SoC can handle - otherwise the SoC would be
> overvolted until the kernel starts, and the board design would be
> flawed.
> 
> I measured that the old 3.x kernel keeps the SoC voltage at 1.2V, so it
> sounds like a better default. Therefore the fix here would be to raise
> the DCDC1 regulator to 1.2V.

I finally found my JZ4780_DS.pdf (Release Date: Nov. 20, 2014).

According to Table 3-1 the absolute maximum for VDDcore is 1.21V.
According to Table 3-2 the recommended range is 0.99V to 1.21V. 1.1V is only "typical".
According to 1.3 Characteristic: "the Core should run at 1.1 -0.1/+0.2V"

So 1.1V should be right...

But in practise it may be that the ACT8 seems to come up with 1.2V
as chip-internal default during boot. Or U-Boot is initializing it as well.

Maybe I find some time to measure some test point or capacitor while
breaking into U-Boot.

BR,
Nikolaus

[PATCH 0/9] MIPS: CI20: Add WiFi / Bluetooth support

[Paul Cercueil]

Hi,

Here's a set of patches to add support for the WiFi / Bluetooth chip on
the CI20.

WiFi works pretty well, provided it is used with the latest firmware
provided by linux-firmware. Bluetooth does not work very well here, as
I cannot get my wireless keyboard to pair; but it does detect it, and it
does see they key presses when I type the pairing code.

I only tested with a somewhat recent (~2022) Buildroot-based userspace.
I enabled WEXT compatibility because the CI20 is typically used with a
very old userspace, but I did not try to use it with old tools like
ifconfig/iwconfig.

Cheers,
-Paul

Paul Cercueil (9):
  MIPS: DTS: CI20: Fix regulators
  MIPS: DTS: CI20: Fix ACT8600 regulator node names
  MIPS: DTS: CI20: Add parent supplies to ACT8600 regulators
  MIPS: DTS: CI20: Do not force-enable CIM and WiFi regulators
  MIPS: DTS: CI20: Misc. cleanups
  MIPS: DTS: CI20: Parent MSCMUX clock to MPLL
  MIPS: DTS: CI20: Enable support for WiFi / Bluetooth
  MIPS: configs: CI20: Regenerate defconfig
  MIPS: configs: CI20: Enable WiFi / Bluetooth

 arch/mips/boot/dts/ingenic/ci20.dts | 148 +++++++++++++++++++---------
 arch/mips/configs/ci20_defconfig    |  47 ++++++---
 2 files changed, 133 insertions(+), 62 deletions(-)

-- 
2.39.2

Re: [PATCH 0/9] MIPS: CI20: Add WiFi / Bluetooth support

[Thomas Bogendoerfer]

On Sun, Jun 04, 2023 at 04:56:33PM +0200, Paul Cercueil wrote:
> Hi,
> 
> Here's a set of patches to add support for the WiFi / Bluetooth chip on
> the CI20.
> 
> WiFi works pretty well, provided it is used with the latest firmware
> provided by linux-firmware. Bluetooth does not work very well here, as
> I cannot get my wireless keyboard to pair; but it does detect it, and it
> does see they key presses when I type the pairing code.
> 
> I only tested with a somewhat recent (~2022) Buildroot-based userspace.
> I enabled WEXT compatibility because the CI20 is typically used with a
> very old userspace, but I did not try to use it with old tools like
> ifconfig/iwconfig.
> 
> Cheers,
> -Paul
> 
> Paul Cercueil (9):
>   MIPS: DTS: CI20: Fix regulators
>   MIPS: DTS: CI20: Fix ACT8600 regulator node names
>   MIPS: DTS: CI20: Add parent supplies to ACT8600 regulators
>   MIPS: DTS: CI20: Do not force-enable CIM and WiFi regulators
>   MIPS: DTS: CI20: Misc. cleanups
>   MIPS: DTS: CI20: Parent MSCMUX clock to MPLL
>   MIPS: DTS: CI20: Enable support for WiFi / Bluetooth
>   MIPS: configs: CI20: Regenerate defconfig
>   MIPS: configs: CI20: Enable WiFi / Bluetooth
> 
>  arch/mips/boot/dts/ingenic/ci20.dts | 148 +++++++++++++++++++---------
>  arch/mips/configs/ci20_defconfig    |  47 ++++++---
>  2 files changed, 133 insertions(+), 62 deletions(-)
> 
> -- 
> 2.39.2

series applied to mips-next.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH 0/9] MIPS: CI20: Add WiFi / Bluetooth support

[H. Nikolaus Schaller]

Hi Thomas,

> Am 09.06.2023 um 10:23 schrieb Thomas Bogendoerfer <tsbogend@alpha.franken.de>:
> 
> On Sun, Jun 04, 2023 at 04:56:33PM +0200, Paul Cercueil wrote:
>> Hi,
>> 
>> Here's a set of patches to add support for the WiFi / Bluetooth chip on
>> the CI20.
>> 
>> WiFi works pretty well, provided it is used with the latest firmware
>> provided by linux-firmware. Bluetooth does not work very well here, as
>> I cannot get my wireless keyboard to pair; but it does detect it, and it
>> does see they key presses when I type the pairing code.
>> 
>> I only tested with a somewhat recent (~2022) Buildroot-based userspace.
>> I enabled WEXT compatibility because the CI20 is typically used with a
>> very old userspace, but I did not try to use it with old tools like
>> ifconfig/iwconfig.
>> 
>> Cheers,
>> -Paul
>> 
>> Paul Cercueil (9):
>>  MIPS: DTS: CI20: Fix regulators
>>  MIPS: DTS: CI20: Fix ACT8600 regulator node names
>>  MIPS: DTS: CI20: Add parent supplies to ACT8600 regulators
>>  MIPS: DTS: CI20: Do not force-enable CIM and WiFi regulators
>>  MIPS: DTS: CI20: Misc. cleanups
>>  MIPS: DTS: CI20: Parent MSCMUX clock to MPLL
>>  MIPS: DTS: CI20: Enable support for WiFi / Bluetooth
>>  MIPS: configs: CI20: Regenerate defconfig
>>  MIPS: configs: CI20: Enable WiFi / Bluetooth
>> 
>> arch/mips/boot/dts/ingenic/ci20.dts | 148 +++++++++++++++++++---------
>> arch/mips/configs/ci20_defconfig    |  47 ++++++---
>> 2 files changed, 133 insertions(+), 62 deletions(-)
>> 
>> -- 
>> 2.39.2
> 
> series applied to mips-next.

I think this was a little too early. Please see my review.

Best regards,
Nikolaus

> 
> Thomas.
> 
> -- 
> Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
> good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH 0/9] MIPS: CI20: Add WiFi / Bluetooth support

[H. Nikolaus Schaller]

Hi Paul,
I was in holidays and could not review this series earlier.


> Am 04.06.2023 um 16:56 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> Hi,
> 
> Here's a set of patches to add support for the WiFi / Bluetooth chip on
> the CI20.
> 
> WiFi works pretty well, provided it is used with the latest firmware
> provided by linux-firmware. Bluetooth does not work very well here, as
> I cannot get my wireless keyboard to pair; but it does detect it, and it
> does see they key presses when I type the pairing code.
> 
> I only tested with a somewhat recent (~2022) Buildroot-based userspace.
> I enabled WEXT compatibility because the CI20 is typically used with a
> very old userspace, but I did not try to use it with old tools like
> ifconfig/iwconfig.

^^^ great since not everyone is using memory hungry latest user-space and
ifconfig/iwconfig is also available on some other OS so users like me
can share scripts.


But I had quite some issues with this series.

1. I could not boot my CI20 V2a board after applying the full series to v6.4-rc6
2. bisecting failed because vcc_33v is used in a patch preceding its definition
   leading to DTC compile abort
3. after fixing I could bisect that "MIPS: DTS: CI20: Fix ACT8600 regulator node names"
   is the first bad commit - although I don't see immediately why

So this series seems to be severely broken and I could not even come to
a test of WiFi and/or Bluetooth which the series claims to support.

Comments to some individual patches follow.

Best regards and looking forward to a v2 for testing,
Nikolaus


> 
> Cheers,
> -Paul
> 
> Paul Cercueil (9):
>  MIPS: DTS: CI20: Fix regulators
>  MIPS: DTS: CI20: Fix ACT8600 regulator node names
>  MIPS: DTS: CI20: Add parent supplies to ACT8600 regulators

^^^ should IMHO be a separate series since it is not directly related to WiFi / Bluetooth

>  MIPS: DTS: CI20: Do not force-enable CIM and WiFi regulators
>  MIPS: DTS: CI20: Misc. cleanups

^^^ these two do not compile
The Misc. cleanups do not belong to this topic.

>  MIPS: DTS: CI20: Parent MSCMUX clock to MPLL

^^^ this is only loosely related to Wifi / Bluetooth

>  MIPS: DTS: CI20: Enable support for WiFi / Bluetooth
>  MIPS: configs: CI20: Regenerate defconfig
>  MIPS: configs: CI20: Enable WiFi / Bluetooth
> 
> arch/mips/boot/dts/ingenic/ci20.dts | 148 +++++++++++++++++++---------
> arch/mips/configs/ci20_defconfig    |  47 ++++++---
> 2 files changed, 133 insertions(+), 62 deletions(-)
> 
> -- 
> 2.39.2
>