Hi, On 23 October 2017 at 01:35, Kever Yang <[email protected]> wrote: > Philipp, > > > On 10/20/2017 05:00 PM, Dr. Philipp Tomsich wrote: >> >> Kever, >> >>> On 20 Oct 2017, at 03:59, Kever Yang <[email protected]> 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", ®) >>> 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 _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
