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

[Simon Glass]

Hi,

On 23 October 2017 at 01:35, Kever Yang <kever.yang at rock-chips.com> wrote:
> 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.

I wonder if those drivers are asking for a particular function from
the grf? If so, we can name that function and add it as an ioctl to
the SYSCON uclass, perhaps?

Regards,
Simon