Hi Philipp, On 24 July 2017 at 10:49, Dr. Philipp Tomsich <philipp.toms...@theobroma-systems.com> wrote: > >> On 24 Jul 2017, at 18:43, Simon Glass <s...@chromium.org> wrote: >> >> Hi Philipp, >> >> On 24 July 2017 at 07:10, Dr. Philipp Tomsich >> <philipp.toms...@theobroma-systems.com> wrote: >>> >>> >>>> On 24 Jul 2017, at 14:45, Kever Yang <kever.y...@rock-chips.com> wrote: >>>> >>>> Hi Philipp, >>>> >>>> >>>> On 07/24/2017 05:11 PM, Dr. Philipp Tomsich wrote: >>>>>> On 24 Jul 2017, at 10:34, Andy Yan <andy....@rock-chips.com> wrote: >>>>>> >>>>>> Hi Philipp: >>>>>> >>>>>> >>>>>> On 2017年07月21日 16:34, Dr. Philipp Tomsich wrote: >>>>>>>> On 21 Jul 2017, at 05:50, Andy Yan <andy....@rock-chips.com> wrote: >>>>>>>> >>>>>>>> Hi Philipp: >>>>>>>> >>>>>>>> >>>>>>>> On 2017年07月19日 04:36, Philipp Tomsich wrote: >>>>>>>>> The RK3368 GRF header was still defines with a shifted-mask but with >>>>>>>>> non-shifted function selectors for the IOMUX defines. As the RK3368 >>>>>>>>> support is still fresh enough to allow a quick rename, we do this now >>>>>>>>> before having more code use this. >>>>>>>>> >>>>>>>>> As some of the downstream drivers (e.g. the Designware GMAC wrapper) >>>>>>>>> may need to include the grf-header of multiple devices, we rename the >>>>>>>>> various defines for the RK3368 by prefixing them with the device name. >>>>>>>>> This avoids future trouble during driver integration. >>>>>>>> Is that really necessary to add such a prefix for all the register >>>>>>>> definition, just to fit the GMAC dirver? >>>>>>>> Maybe the gmac driver can define the platform specific macro in its >>>>>>>> own code like dwmac-rk in the kernel. Then we can keep the register >>>>>>>> header file a little tidy. >>>>>>> The problem is not the GMAC driver (although it was the motivation for >>>>>>> doing >>>>>>> this change), but rather how easy it is to pick up the wrong grf-file >>>>>>> and refer >>>>>>> to the wrong definitions/values… the conflicts are not through the >>>>>>> GMAC-specifc >>>>>>> defines but rather through things like the IOMUX masks or (before I >>>>>>> moved >>>>>>> this to the DDR-driver) things like the defines to enable DRAM in GRF. >>>>>>> >>>>>>> Until we find a way to consistently structure things in such a way that >>>>>>> (a) common constructs can be shared (e.g. the masks in the IOMUX) >>>>>>> (b) specific definitions (e.g. the pin-out for a specific device) is >>>>>>> not visible outside of its driver >>>>>>> I prefer the names to explicitly refer to each device. >>>>>>> That said, the longer-term plan (especially with a larger number of >>>>>>> devices >>>>>>> being supported) needs to include a consistent rework of how (and where) >>>>>>> we manage all these definitions from the grf-header files. >>>>>> It's true that many devices use GRF, we also face this condition in the >>>>>> kernel land.And all the drivers in kernel handle this device specific >>>>>> definition in the driver itself(by of_device_id->data or MACRO). And >>>>>> there is also no need to use the grf constructs, the common syscon api >>>>>> will provide you the grf base address. I think this keeps things >>>>>> simple, we don't need to rely on too much of the grf or cru header, >>>>>> maybe some day we can remove the header file, just as clean as the >>>>>> kernel. >>>>> The problem is not the base-address of GRF, but rather the need to pull >>>>> in the >>>>> register structure (so we can refer to individual registers by name >>>>> instead of by >>>>> hard-coded offset): as we pull this in today, the definitions for the >>>>> various IOMUX >>>>> entries will also be pulled in, which will cause duplicate definitions. >>>> >>>> I really don't like the idea of add the prefix, which end with longer >>>> MACRO but no other improve. >>>> I think the GRF is used mostly for the IOMUX, in this case the better >>>> solution is >>>> port the kernel rockchip-pinctrl driver in uboot, which not rely on the >>>> dtsi instead of header file. >>>> >>>> If we do this, we can remove most of the GRF header definition for IOMUX, >>>> I can ask David Wu >>>> to do this, but maybe few weeks later. >>> >>> As indicated earlier: this is a temporary solution (although we have a >>> local saying that >>> "nothing is more permanent than a temporary solution”) and should remain >>> limited to >>> the RK3368 until we can clean this up across all boards. >> >> Can we drop the prefix for now? I do worry about the precedent this >> sets. If this is needed anywhere then it suggests to me that we have >> not split up the drivers correctly. We should not be including two >> different GRF headers into the same file. E.g. there should be >> different pinctrl drivers for each SoC. > > If we drop it, we can’t get RK3368-support merged into the GMAC driver for > now. > Splitting the Rockchip-wrapper of the GMAC-driver doesn’t appear like a > workable > solution either, as this would create a new file for each chip-variant in > drivers/net. > > I’ll just pull the RK3368 GMAC (driver) support for now and split it off this > series.
My view of GRF is that we should try to have a driver for the various operations that each thing needs (e.g. using the ioctl model). Imagine a world where we have a U-Boot that supports running on all available Rockchip SoCs and can boot on any board. In that case we probably want a GRF driver with operations that can be implemented by each SoC. I have not sketched this out or tried it though. It needs more thought / prototyping. I am concerned about #ifdefs in the code and including private SoC-specific RK header files in generic Rockchip drivers. To me that is the wrong approach. > >>> Pulling this into the pinctrl drivers is an excellent choice. >>> Please watch out for board_debug_uart_init(…), which also directly accesses >>> the IOMUX >>> constants and will need to be restructured together with pinctrl drivers. >>> >>> Thanks! >>> —Philipp. >>> >>>> Thanks, >>>> - Kever >>>>> >>>>>> On the other hand, add such a prefix of rk3368 breaks the consistency >>>>>> with other rockchip devices. >>>>> I do agree that we need to clean this up across all the boards and drivers >>>>> supported today. As of today, I can not resolve this on a RK3368-only >>>>> basis, >>>>> as I’d need to clean up the RK3288 at the same time (and don’t want to do >>>>> this >>>>> in a RK3368-focused series that already has way too much going on). >>>>> >>>>> Let’s try to clean this up for the next (i.e. the one after this) merge >>>>> window. >>>>> Once we have bit-definitions out of the grf-header, I’ll remove the >>>>> RK3368_ >>>>> prefixes... >>>>> >>>>>>>>> Signed-off-by: Philipp Tomsich <philipp.toms...@theobroma-systems.com> >>>>>>>>> --- >>>>>>>>> Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
