> 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. >> 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
