| Message ID | 20211224134347.41812-2-alpernebiyasak@gmail.com |
|---|---|
| State | Accepted |
| Commit | eb0ca6b5ecd8fbbbd55ebdf63a4a622fd6ec907f |
| Delegated to: | Kever Yang |
| Headers | show |
| Series | rockchip: Improve support for Bob chromebook and add support for Kevin | expand |
On Fri, 24 Dec 2021 at 06:44, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote: > > The RK3399 SoC needs to know the voltage value provided by some > regulators, which is done by setting relevant register bits. Configure > these the way other RK3399 boards do, but with the same values as are > set in the equivalent code in coreboot. > > Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com> > Reviewed-by: Kever Yang <kever.yang@rock-chips.com> > --- > There is a driver for this on Rockchip's repo, I managed to forward-port > it and get it working. If that's more desirable than doing it per-board > like this I can send that upstream (but I'd prefer to do it after this). > > Changes in v3: > - Add tag: "Reviewed-by: Kever Yang <kever.yang@rock-chips.com>" > > Changes in v2: > - Drop unnecessary ifdef. > - Clarify commit message regarding 'values set in coreboot'. > > board/google/gru/gru.c | 52 ++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 52 insertions(+) Reviewed-by: Simon Glass <sjg@chromium.org> Tested on bob, kevin Tested-by: Simon Glass <sjg@chromium.org> Yes I think a driver would be a good idea.
On Fri, Dec 24, 2021 at 7:14 PM Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote: > > The RK3399 SoC needs to know the voltage value provided by some > regulators, which is done by setting relevant register bits. Configure > these the way other RK3399 boards do, but with the same values as are > set in the equivalent code in coreboot. > > Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com> > Reviewed-by: Kever Yang <kever.yang@rock-chips.com> > --- > There is a driver for this on Rockchip's repo, I managed to forward-port > it and get it working. If that's more desirable than doing it per-board > like this I can send that upstream (but I'd prefer to do it after this). > > Changes in v3: > - Add tag: "Reviewed-by: Kever Yang <kever.yang@rock-chips.com>" > > Changes in v2: > - Drop unnecessary ifdef. > - Clarify commit message regarding 'values set in coreboot'. > > board/google/gru/gru.c | 52 ++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 52 insertions(+) > > diff --git a/board/google/gru/gru.c b/board/google/gru/gru.c > index 23080c1798b7..cbf62a9427c9 100644 > --- a/board/google/gru/gru.c > +++ b/board/google/gru/gru.c > @@ -6,6 +6,17 @@ > #include <common.h> > #include <dm.h> > #include <init.h> > +#include <syscon.h> > +#include <asm/io.h> > +#include <asm/arch-rockchip/clock.h> > +#include <asm/arch-rockchip/grf_rk3399.h> > +#include <asm/arch-rockchip/hardware.h> > +#include <asm/arch-rockchip/misc.h> > + > +#define GRF_IO_VSEL_BT656_SHIFT 0 > +#define GRF_IO_VSEL_AUDIO_SHIFT 1 > +#define PMUGRF_CON0_VSEL_SHIFT 8 > +#define PMUGRF_CON0_VOL_SHIFT 9 > > #ifdef CONFIG_SPL_BUILD > /* provided to defeat compiler optimisation in board_init_f() */ > @@ -54,3 +65,44 @@ int board_early_init_r(void) > return 0; > } > #endif > + > +static void setup_iodomain(void) > +{ > + struct rk3399_grf_regs *grf = > + syscon_get_first_range(ROCKCHIP_SYSCON_GRF); > + struct rk3399_pmugrf_regs *pmugrf = > + syscon_get_first_range(ROCKCHIP_SYSCON_PMUGRF); > + > + /* BT656 and audio is in 1.8v domain */ > + rk_setreg(&grf->io_vsel, (1 << GRF_IO_VSEL_BT656_SHIFT | > + 1 << GRF_IO_VSEL_AUDIO_SHIFT)); > + > + /* > + * Set GPIO1 1.8v/3.0v source select to PMU1830_VOL > + * and explicitly configure that PMU1830_VOL to be 1.8V > + */ > + rk_setreg(&pmugrf->soc_con0, (1 << PMUGRF_CON0_VSEL_SHIFT | > + 1 << PMUGRF_CON0_VOL_SHIFT)); > +} > + > +int misc_init_r(void) > +{ > + const u32 cpuid_offset = 0x7; > + const u32 cpuid_length = 0x10; > + u8 cpuid[cpuid_length]; > + int ret; > + > + setup_iodomain(); > + > + ret = rockchip_cpuid_from_efuse(cpuid_offset, cpuid_length, cpuid); > + if (ret) > + return ret; > + > + ret = rockchip_cpuid_set(cpuid, cpuid_length); > + if (ret) > + return ret; > + > + ret = rockchip_setup_macaddr(); > + > + return ret; > +} This looks like a board hack code for me, which is difficult to maintain in the long run. Better go with UCLASS_POWER_DOMAIN, I believe it would be a straightforward implementation. Thanks, Jagan.
On 11/03/2022 22:49, Jagan Teki wrote: > On Fri, Dec 24, 2021 at 7:14 PM Alper Nebi Yasak > <alpernebiyasak@gmail.com> wrote: >> >> The RK3399 SoC needs to know the voltage value provided by some >> regulators, which is done by setting relevant register bits. Configure >> these the way other RK3399 boards do, but with the same values as are >> set in the equivalent code in coreboot. >> >> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com> >> Reviewed-by: Kever Yang <kever.yang@rock-chips.com> >> --- >> There is a driver for this on Rockchip's repo, I managed to forward-port >> it and get it working. If that's more desirable than doing it per-board >> like this I can send that upstream (but I'd prefer to do it after this). >> >> [...] > > This looks like a board hack code for me, which is difficult to > maintain in the long run. Better go with UCLASS_POWER_DOMAIN, I > believe it would be a straightforward implementation. Yeah, it's a board hack that several other boards also do. I've got a driver working for this as mentioned above, but that adds a new UCLASS_IO_DOMAIN instead of using UCLASS_POWER_DOMAIN like you suggest. I had been tired of carrying these patches, so wanted to get any working version merged before attempting to upstream the driver and remove the hacky parts from all boards at once. I'm not sure how long would that take, and I didn't want this to be blocked by that. I've got to work on binman things for a while, so I might not be able to get to that and your other suggestions soon enough.
Hi, On Mon, 14 Mar 2022 at 15:32, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote: > > On 11/03/2022 22:49, Jagan Teki wrote: > > On Fri, Dec 24, 2021 at 7:14 PM Alper Nebi Yasak > > <alpernebiyasak@gmail.com> wrote: > >> > >> The RK3399 SoC needs to know the voltage value provided by some > >> regulators, which is done by setting relevant register bits. Configure > >> these the way other RK3399 boards do, but with the same values as are > >> set in the equivalent code in coreboot. > >> > >> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com> > >> Reviewed-by: Kever Yang <kever.yang@rock-chips.com> > >> --- > >> There is a driver for this on Rockchip's repo, I managed to forward-port > >> it and get it working. If that's more desirable than doing it per-board > >> like this I can send that upstream (but I'd prefer to do it after this). > >> > >> [...] > > > > This looks like a board hack code for me, which is difficult to > > maintain in the long run. Better go with UCLASS_POWER_DOMAIN, I > > believe it would be a straightforward implementation. > > Yeah, it's a board hack that several other boards also do. I've got a > driver working for this as mentioned above, but that adds a new > UCLASS_IO_DOMAIN instead of using UCLASS_POWER_DOMAIN like you suggest. > > I had been tired of carrying these patches, so wanted to get any working > version merged before attempting to upstream the driver and remove the > hacky parts from all boards at once. I'm not sure how long would that > take, and I didn't want this to be blocked by that. +1 let's get this merged as I have been waiting two releases for this now. I can't really test anything since the base hardware is not functional on mainline. > > I've got to work on binman things for a while, so I might not be able to > get to that and your other suggestions soon enough. Regards, Simon
diff --git a/board/google/gru/gru.c b/board/google/gru/gru.c index 23080c1798b7..cbf62a9427c9 100644 --- a/board/google/gru/gru.c +++ b/board/google/gru/gru.c @@ -6,6 +6,17 @@ #include <common.h> #include <dm.h> #include <init.h> +#include <syscon.h> +#include <asm/io.h> +#include <asm/arch-rockchip/clock.h> +#include <asm/arch-rockchip/grf_rk3399.h> +#include <asm/arch-rockchip/hardware.h> +#include <asm/arch-rockchip/misc.h> + +#define GRF_IO_VSEL_BT656_SHIFT 0 +#define GRF_IO_VSEL_AUDIO_SHIFT 1 +#define PMUGRF_CON0_VSEL_SHIFT 8 +#define PMUGRF_CON0_VOL_SHIFT 9 #ifdef CONFIG_SPL_BUILD /* provided to defeat compiler optimisation in board_init_f() */ @@ -54,3 +65,44 @@ int board_early_init_r(void) return 0; } #endif + +static void setup_iodomain(void) +{ + struct rk3399_grf_regs *grf = + syscon_get_first_range(ROCKCHIP_SYSCON_GRF); + struct rk3399_pmugrf_regs *pmugrf = + syscon_get_first_range(ROCKCHIP_SYSCON_PMUGRF); + + /* BT656 and audio is in 1.8v domain */ + rk_setreg(&grf->io_vsel, (1 << GRF_IO_VSEL_BT656_SHIFT | + 1 << GRF_IO_VSEL_AUDIO_SHIFT)); + + /* + * Set GPIO1 1.8v/3.0v source select to PMU1830_VOL + * and explicitly configure that PMU1830_VOL to be 1.8V + */ + rk_setreg(&pmugrf->soc_con0, (1 << PMUGRF_CON0_VSEL_SHIFT | + 1 << PMUGRF_CON0_VOL_SHIFT)); +} + +int misc_init_r(void) +{ + const u32 cpuid_offset = 0x7; + const u32 cpuid_length = 0x10; + u8 cpuid[cpuid_length]; + int ret; + + setup_iodomain(); + + ret = rockchip_cpuid_from_efuse(cpuid_offset, cpuid_length, cpuid); + if (ret) + return ret; + + ret = rockchip_cpuid_set(cpuid, cpuid_length); + if (ret) + return ret; + + ret = rockchip_setup_macaddr(); + + return ret; +}