[U-Boot] Rockchip GRF/PMUGRF/CRU reg access in U-Boot

[Kever Yang]

Philipp,


On 10/20/2017 05:00 PM, Dr. Philipp Tomsich wrote:
> Kever,
>
>> On 20 Oct 2017, at 03:59, Kever Yang <kever.yang at rock-chips.com> wrote:
>>
>> Hi Simon and Philipp.
>>
>>      I would like to include Heiko who is maintainer of upstream kernel Rockchip SoC for this topic.
>>
>> For the driver related to grf, which including pinctrl, usb phy, gmac and many other modules, we would like to sync with kernel, so that we can re-use the dtsi from kernel. The use of grf register have many discuss on upstream kernel already, and all modules use grf with a &grf reference and offset in dts.
>>
>> U-Boot coding style says "Use structures for I/O access", in my opinion, this is good for most case, but not for registers like Rockchip grf, because it's related for many modules, and it's different in different SoC, use "base addr + offset" for grf reg in dtsi and decode it in driver is much more reasonable for grf related driver setting. Just like the line size 80 is max, we follow the rule in most case, but in some case, more than 80 characters is better for read, then we use more than 80 characters. We can merge pinctrl driver into one file like kernel if we can use this feature, and the same with sysreset, soft reset...We are not going to follow the dead rules for all cases, we have to use the one which is better for us.
>>
>> To make it more clear, here is an example:
>>
>> Looking at drivers/sysreset/sysreset_rk3xxx.c(there are 8 files and more to come), the source code are almost the same, the only difference is the reg address of glb_srst_snd_value and glb_srst_fst_value in cru,  we can very easy to decode this from dts in one common driver if we use 'base + offset'. If we insist on using structure for this kind of reg access, to merge them into one common driver we have to including all different SoC cru definition and use different variable for different SoC, there would be many '#ifdef ... #else' in the file like this
>>
> In the sysreset-case there shouldn’t be a change to the DTSI necessary.
> The sysreset-drivers are always instantiated from the clock-drivers, as in the following example:
> 	static int rk3399_clk_bind(struct udevice *dev)
> 	{
> 	        int ret;
> 	
> 	        /* The reset driver does not have a device node, so bind it here */
> 	        ret = device_bind_driver(gd->dm_root, "rk3399_sysreset", "reset", &dev);
> 	        if (ret)
> 	                printf("Warning: No RK3399 reset driver: ret=%d\n", ret);
> 	
> 	        return 0;
> 	}
>
> Given this, the offset of the sysreset register in the GRF can be easily passed in during binding the driver by changing this code (the following is just pseudocode w/o error handling) to:
> 	struct driver *drv = lists_driver_lookup_name(“rk3399_sysreset”);
> 	ulong drv_data = offsetof(grf_struct, sysreset_reg);
> 	device_bind_with_driver_data(parent, drv, dev_name, drv_data, gd->dm_root, &dev);

Yes, this can be resolved by the API you mentioned, but this is one of a 
typical case if grf reg if there we have to add one dts node for 
sysreset driver.
> 	
>
> And: thank you for getting around to look at the sysreset situation and starting work on deduplicating this.
>
>> #ifdef CONFIG_ROCKCHIP_RK3036
>>
>> #include <asm/arch/cru_rk3036.h>
>>
>> #else ifdef CONFIG_ROCKCHIP_RK3188
>>
>> #include <asm/arch/cru_rk3188.h>
>>
>> #else ifdef CONFIG_ROCKCHIP_RK322x
>>
>> #include <asm/arch/cru_rk322x.h>
>>
>> #else ifdef CONFIG_ROCKCHIP_RK3288
>>
>> #include <asm/arch/cru_rk3288.h>
>>
>> #else ifdef CONFIG_ROCKCHIP_RK3328
>>
>> #include <asm/arch/cru_rk3288.h>
>>
>> #else ifdef CONFIG_ROCKCHIP_RK3368
>>
>> #include <asm/arch/cru_rk3368.h>
>>
>> ...
>> #endif
>>
>> and also this is need when we define and use the cru variant.
>>
>>
>> I would like to use two dts decode instead like this for all SoCs:
>> rphy->grf_base = syscon_get_first_range(ROCKCHIP_SYSCON_GRF);
>> ofnode_read_u32(dev_ofnode(dev), "reg", &reg)
>> sysreset_reg = rphy->grf_base + reg;
>>
>> and dts for different SoC like this:
>>          rockchip,grf = <&grf>;
>>          reg = <0x017c>;
>>
> The proper coding style in U-Boot would be to use the syscon-functions to perform a read/write to an offset.
> In other words
> 	regmap_read(grf, reg, &valp)
> should be used to read the location for the above example.
>
> Note that his works only, if the layout within these registers is the same across devices.

Well, great, that means we can re-use the driver and dts definition from 
kernel for grf regs.
>
>> We wish upstream U-Boot can accept this kind of coding, I believe when the coding style was made, it does not met the controller reg like Rockchip GRF.
> In fact the regmap and syscon device classes have always been intended to work exactly for such cases.
>
> There is one convenience layer (on top of) regmap missing to write bitfields in or across symbolically named
> registers.  However, this may be an intentional omission as such functionality will most likely need to be
> handled by misc-devices (which is the reason why I finally started work to get our GMAC-related control
> offloaded into chip-specific misc device).

I'm not clear about your 'chip-specific misc-devices', some of grf reg 
setting for module are one-time init,
but some others need to modify during runtime, so there will be some 
drivers with grf access.
For example, when you search "grf" in rk3288.dtsi, there are more then 
10 device node using grf or sgrf,
they include the reference to grf because there driver use the grf.

Thanks,
- Kever
>
>> We will follow the coding style requirement in all other normal controller like I2C/SPI/USB/SDMMC/DISPLAY and etc.
>>
>> Thanks,
>> - Kever
>