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.
Thanks,
- Kever
We will follow the coding style requirement in all other normal controller like
I2C/SPI/USB/SDMMC/DISPLAY and etc.
Thanks,
- Kever
_______________________________________________
U-Boot mailing list
[email protected]
https://lists.denx.de/listinfo/u-boot
