[Letux-kernel] [PATCH 00/20] A bunch of JZ4730 fixups for letux-kernel

[H. Nikolaus Schaller]

Hi Paul,

> Am 21.12.2020 um 12:40 schrieb Paul Boddie <paul at boddie.org.uk>:
> 
> Nikolaus,
> 
> On Monday, 21 December 2020 09:25:38 CET H. Nikolaus Schaller wrote:
>> 
>> Looking into the code of irq_set_type() there is something suspect:
>> 
>> static void ingenic_gpio_set_bit(struct ingenic_gpio_chip *jzgc,
>> 		u8 reg, u8 offset, bool set)
>> 
>> ...
>> 
>> static void irq_set_type(struct ingenic_gpio_chip *jzgc,
>> ...
>> 	} else if (jzgc->jzpc->info->version == ID_JZ4730) {
>> 		ingenic_gpio_set_bit(jzgc, offset, JZ4730_GPIO_GPDIR, false);
>> 		ingenic_gpio_set_bits(jzgc, offset,
>> 					JZ4730_GPIO_GPIDUR, JZ4730_GPIO_GPIDLR,
>> 					(val2 ? 2 : 0) | (val1 ? 1 : 0));
>> 		return;
>> ...
>> 	if (jzgc->jzpc->info->version >= ID_X1000) {
>> 		ingenic_gpio_shadow_set_bit(jzgc, reg2, offset, val1);
>> 		ingenic_gpio_shadow_set_bit(jzgc, reg1, offset, val2);
>> 		ingenic_gpio_shadow_set_bit_load(jzgc);
>> 	} else {
>> 		ingenic_gpio_set_bit(jzgc, reg2, offset, val1);
>> 		ingenic_gpio_set_bit(jzgc, reg1, offset, val2);
>> 	}
>> 
>> So for the jz4730 we pass the register number(s) as 3rd argument,
>> while for the other SoC we pass as 2nd. And offset is the other one.
> 
> Yes, this is definitely incorrect. Well done for noticing this! It just shows 
> that such things are easy to miss when very tired (and when the same types are 
> used for two different things, assuming that C would even care otherwise).

It also shows that one needs code review AND testing. Neither method alone
is optimal and successful...

> 
>> What I don't understand is how this can wipe out the full register...
>> 
>> But I have tried to swap the offset behind the registers and the result is:
>> 
>> [    2.539027] Waiting for root device /dev/mmcblk1p2...
>> [    2.537757] mmc_gpio_get_ro
>> [    2.540796] ingenic_gpio_get: off=2
>> [    2.544487] ingenic_gpio_read_reg reg=0
>> [    2.530991] ingenic_gpio_get_value: offset=0x2
>> [    2.538372] mmc0: new high speed SDHC card at address 59b4
>> [    2.531623] mmcblk0: mmc0:59b4 USD   7.51 GiB (ro)
>> [    2.547466]  mmcblk0: p1 p2
>> [    3.337335] random: crng init done
>> 
>> Still stuck, but card and partitions are now detected! Yay!
> 
> Well done!
> 
>> It seems to report read-only (ro) but that is a different story (WP GPIO?).
>> It should not stop running the init process.
>> 
>> Patch attached.
> 
> I think you could just apply this. The parameter order was simply wrong.

With this (and no printk) we get what I think is a clean and expected log.

[    0.148321] clk: Not disabling unused clocks
[    0.152855] ALSA device list:
[    0.138537]   No soundcards found.
[    0.144948] jz4740-mmc 10021000.mmc: Got CD GPIO
[    0.150034] jz4740-mmc 10021000.mmc: Got WP GPIO
[    0.155034] jz4740-mmc 10021000.mmc: Looking up vmmc-supply from device tree
[    0.146705] jz4740-mmc 10021000.mmc: Looking up vqmmc-supply from device tree
[    0.146644] jz4740-mmc 10021000.mmc: Ingenic SD/MMC card driver registered
[    0.153793] jz4740-mmc 10021000.mmc: Using PIO, 4-bit mode
[    0.148436] Waiting for root device /dev/mmcblk1p2...
[    0.159135] mmc0: new high speed SDHC card at address 59b4
[    0.152097] mmcblk0: mmc0:59b4 USD   7.51 GiB (ro)
[    0.150519]  mmcblk0: p1 p2

> 
> I also think that the direction reset in ingenic_pinmux_set_pin_fn probably 
> shouldn't be there, either.

This doesn't have a visible effect, but I have added it as well.

Unclear is what the PA21 warning from device tree:

[    0.131441] pinctrl-ingenic 10010000.pin-controller: invalid group "PC0" for function "mmc"
[    0.140276] pinctrl-ingenic 10010000.pin-controller: invalid group "PC2" for function "mmc"
[    0.131627] pinctrl-ingenic 10010000.pin-controller: invalid group "PA21" for function "mmc"

wants to say. Next topic for debugging... And where the kernel hangs after finding
the card and partitions. So it is just the next phase of the journey...

> Sorry for all the inconvenience this has caused!

Well, no need to be sorry. It is life. It is human. And as long as machines
can not find such mistakes automatically, we are needed :)

Best regards,
Nikolaus