RE: [PATCH] ARM: dts: socfpga: remove syscon compatible string for sysmgr node

["Rabara, Niravkumar L" <niravkumar.l.rabara@xxxxxxxxx>]

Hi Dinh,

> -----Original Message-----
> From: Dinh Nguyen <dinguyen@xxxxxxxxxx>
> Sent: Tuesday, 11 February, 2025 12:15 PM
> To: Rabara, Niravkumar L <niravkumar.l.rabara@xxxxxxxxx>; Rob Herring
> <robh@xxxxxxxxxx>; Krzysztof Kozlowski <krzk+dt@xxxxxxxxxx>; Conor Dooley
> <conor+dt@xxxxxxxxxx>; devicetree@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Cc: lkp <lkp@xxxxxxxxx>
> Subject: Re: [PATCH] ARM: dts: socfpga: remove syscon compatible string for
> sysmgr node
> 
> > Yes, I have tested this using NFS boot, however I didn't observe any
> > issue with SD/MMC driver.
> >
> > => fdt print /soc/mmc@ff808000
> > mmc@ff808000 {
> > 	#address-cells = <0x00000001>;
> > 	#size-cells = <0x00000000>;
> > 	compatible = "altr,socfpga-dw-mshc";
> > 	reg = <0xff808000 0x00001000>;
> > 	interrupts = <0x00000000 0x00000062 0x00000004>;
> > 	fifo-depth = <0x00000400>;
> > 	clocks = <0x0000001a 0x00000024>;
> > 	clock-names = "biu", "ciu";
> > 	resets = <0x00000006 0x00000027>;
> > 	altr,sysmgr-syscon = <0x0000001c 0x00000028 0x00000004>;
> > 	status = "okay";
> > 	cap-sd-highspeed;
> > 	cap-mmc-highspeed;
> > 	broken-cd;
> > 	bus-width = <0x00000004>;
> > 	clk-phase-sd-hs = <0x00000000 0x00000087>;
> > 	phandle = <0x00000029>;
> > };
> > => fdt print /soc/sysmgr@ffd06000
> > sysmgr@ffd06000 {
> > 	compatible = "altr,sys-mgr";
> > 	reg = <0xffd06000 0x00000300>;
> > 	cpu1-start-addr = <0xffd06230>;
> > 	phandle = <0x0000001c>;
> > };
> >
> > [    1.095784] mmc_host mmc0: Bus speed (slot 0) = 50000000Hz (slot req
> 50000000Hz, actual 50000000HZ div = 0)
> > [    1.105692] mmc0: new high speed SDHC card at address 0001
> > [    1.108238] at24 0-0051: supply vcc not found, using dummy regulator
> > [    1.111817] mmcblk0: mmc0:0001 SD32G 29.1 GiB
> > [    1.118872] at24 0-0051: 4096 byte 24c32 EEPROM, writable, 32
> bytes/write
> > [    1.129186]  mmcblk0: p1 p2 p3
> >
> > .
> >
> > root@arria10:~# ls /dev/mmcblk0*
> > /dev/mmcblk0    /dev/mmcblk0p1  /dev/mmcblk0p2  /dev/mmcblk0p3
> > root@arria10:~# mount /dev/mmcblk0p1 /tmp/ root@arria10:~# ls /tmp/
> > extlinux                         socfpga_arria10_socdk_sdmmc.dtb  zImage
> > fit_spl_fpga.itb                 u-boot.img
> >
> >
> 
> You didn't really test anything. There's a register in the System Manager that has
> set the SD/MMC clk-phase in U-Boot. So you won't see the failure unless you
> explicitly change the value in that register and then boot Linux, then you will see
> the failure. If you look at drivers/mmc/host/dw_mmc-pltfm.c and look at the
> function dw_mci_socfpga_priv_init(), you can see that work in action. If you
> remove the syscon property, then that portion of the driver will fail.
> Also the ethernet driver is using the system manager's to set the correct PHY
> mode through syscon. I think you need to test this patch more thoroughly.
> 
> DInh

Altera System Manager driver (drivers/mfd/altera-sysmgr.c) is enabled
in "socfpga_defconfig" - i.e. CONFIG_MFD_ALTERA_SYSMGR=y

So, SoCFPGA always using drivers/mfd/altera-sysmgr.c for System Manager
register access, not the generic "syscon" drivers/mfd/syscon.c.
That's why we do not need "syscon" compatible for fall back mechanism.  

	sysmgr: sysmgr@ffd08000 {
-		compatible = "altr,sys-mgr", "syscon";
+		compatible = "altr,sys-mgr";
 		reg = <0xffd08000 0x4000>;
 	};
	mmc: mmc@ff808000 {
		…
		altr,sysmgr-syscon = <&sysmgr 0x28 4>;
		clk-phase-sd-hs = <0>, <135>;
		…
	};
	gmac0: ethernet@ff800000 {
		…
		altr,sysmgr-syscon = <&sysmgr 0x44 0>;
		…
	};


Even the sdmmc driver you mentioned is using "drivers/mfd/altera-sysmgr.c"
not the generic "syscon" drivers/mfd/syscon.c. Same thing for ethernet driver
as well.  

dw_mci_socfpga_priv_init()  {
	...
	rc = of_property_read_variable_u32_array(np, "clk-phase-sd-hs", &clk_phase[0], 2, 0);
	if (rc < 0)
		return 0;

	sys_mgr_base_addr = altr_sysmgr_regmap_lookup_by_phandle(np, "altr,sysmgr-syscon");
	if (IS_ERR(sys_mgr_base_addr)) {
		dev_warn(host->dev, "clk-phase-sd-hs was specified, but failed to find altr,sys-mgr regmap!\n");
		return 0;
	}

	of_property_read_u32_index(np, "altr,sysmgr-syscon", 1, &reg_offset);
	of_property_read_u32_index(np, "altr,sysmgr-syscon", 2, &reg_shift);
	...
}

Please let me know if my understanding is incorrect. 

Thanks,
Nirav