[PATCH 3/3] board: rockchip: Fix panel detection for mainline A-TF

[Chris Morgan]

On Mon, Sep 30, 2024 at 04:59:41PM +0800, Kever Yang wrote:
> 
> On 2024/9/19 21:15, Chris Morgan wrote:
> > On Thu, Sep 19, 2024 at 09:48:58AM +0800, Kever Yang wrote:
> > > Hi Chris,
> > > 
> > > On 2024/9/18 21:38, Chris Morgan wrote:
> > > > On Wed, Sep 18, 2024 at 11:06:34AM +0800, Kever Yang wrote:
> > > > > Hi Chris,
> > > > > 
> > > > > Please update the subject with something like "Enable the VO PD before
> > > > > driver access",
> > > > > 
> > > > > and the commit message for the change reason is enough.
> > > > > 
> > > > > and the config is more soc level, I think you can add this to
> > > > > arch_cpu_init() for we don't have a power domain driver.
> > > > To my knowledge I'm the only board trying to access the video before
> > > > Linux proper. Do you want me to add this for all rk3568 boards then
> > > > (from within rk3568.c under arch_cpu_init() as you say)?
> > > Yes, please move it to rk3568.c in case other board need it.
> > > >    I'm thinking
> > > > if I do though I can at least get rid of the memory barrier.
> > > > 
> > > > > On 2024/9/17 05:01, Chris Morgan wrote:
> > > > > > From: Chris Morgan <macromorgan at hotmail.com>
> > > > > > 
> > > > > > The current panel detection logic crashes when the device boots with
> > > > > > mainline A-TF, causing a reboot loop. It turns out mainline A-TF
> > > > > > doesn't enable the VO power domain like the BSP A-TF did.
> > > > > > 
> > > > > > Set the VO domain on and use a memory barrier to ensure it is powered
> > > > > > up before we attempt to do the panel detection.
> > > > > > 
> > > > > > Signed-off-by: Chris Morgan <macromorgan at hotmail.com>
> > > > > > ---
> > > > > >     board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c | 13 +++++++++++++
> > > > > >     configs/anbernic-rgxx3-rk3566_defconfig    |  2 --
> > > > > >     2 files changed, 13 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c b/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c
> > > > > > index c1d1826fd14..f4e7c1ab360 100644
> > > > > > --- a/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c
> > > > > > +++ b/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c
> > > > > > @@ -22,6 +22,10 @@
> > > > > >     DECLARE_GLOBAL_DATA_PTR;
> > > > > > +#define PMU_BASE_ADDR		0xfdd90000
> > > > > > +#define PMU_PWR_GATE_SFTCON	0xa0
> > > > > > +#define PMU_PD_VO_DWN_ENA	BIT(7)
> > > > > > +
> > > > > >     #define GPIO0_BASE		0xfdd60000
> > > > > >     #define GPIO4_BASE		0xfe770000
> > > > > >     #define GPIO_SWPORT_DR_L	0x0000
> > > > > > @@ -311,6 +315,15 @@ int rgxx3_detect_display(void)
> > > > > >     	int i;
> > > > > >     	u8 panel_id[2];
> > > > > > +	/* Disable VO power domain power-down */
> > > > > Disable?
> > > > It's weird verbiage but the data sheet says "Enable power down PD_VO"
> > > > by writing a 1, so I figure I'm disabling it by writing a 0.
> > > OK, understand now.
> > > 
> > > To make it simple and clear, could you just say "Enable PD_VO for display"?
> > > 
> > > > > > +	writel((PMU_PD_VO_DWN_ENA << 16),
> > > > > > +	       PMU_BASE_ADDR + PMU_PWR_GATE_SFTCON);
> > > > > > +	/*
> > > > > > +	 * System will crash if the power domain isn't enabled before
> > > > > > +	 * we start trying to talk to the DSI panel.
> > > > > > +	 */
> > > > > > +	wmb();
> > > > > > +
> > > > > >     	/*
> > > > > >     	 * Take panel out of reset status.
> > > > > >     	 * Set GPIO4_A0 to output.
> > > > > > diff --git a/configs/anbernic-rgxx3-rk3566_defconfig b/configs/anbernic-rgxx3-rk3566_defconfig
> > > > > > index f5e5470df8c..49d3766613e 100644
> > > > > > --- a/configs/anbernic-rgxx3-rk3566_defconfig
> > > > > > +++ b/configs/anbernic-rgxx3-rk3566_defconfig
> > > > > > @@ -49,8 +49,6 @@ CONFIG_SPL_REGMAP=y
> > > > > >     CONFIG_SPL_SYSCON=y
> > > > > >     CONFIG_SPL_ADC=y
> > > > > >     CONFIG_SPL_CLK=y
> > > > > > -CONFIG_ARM_SMCCC_FEATURES=y
> > > > > > -CONFIG_SCMI_FIRMWARE=y
> > > > > This should be a separate patch, and what's the change?
> > > > > 
> > > > > 
> > > > Mainline A-TF seems to get angry with U-Boot when these are enabled.
> > > > By angry I mean it freezes. It's all a related change to allowing
> > > > mainline A-TF, though I should probably call it out better as a
> > > > required change. Again I also specify though in the commit message
> > > > that mainline A-TF still doesn't work with Linux proper because of
> > > > the missing psci clk and psci reset bits, but at least with this
> > > > change U-Boot won't be contributing to the problem (for this board).
> > > The scmi clock is missing on rk3568 mainline A-TF now, but it should return
> > > error with something like
> > > 
> > > NOT SUPPORT instead of hang inside.
> > > 
> > > Any way, this is a separate change, we don't need it if the ATF supports
> > > scmi clock later.
> > > 
> > > Thanks,
> > > 
> > > - Kever
> > I'll opt to disable this for now anyway (but in a commit by itself),
> > if that's okay. It makes no difference for booting when using binary
> > A-TF, but it does make a difference when using the *current* mainline
> > A-TF, even if we do get SCMI later.
> 
> Could you try with below  patch for rk3568 SCMI support:
> 
> https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/31265

I did. We still need this specific patch to call the panel in U-Boot
(so I can detect it and fix-up the device tree). I noted a few issues
I identified on that commit; they may or may not be related to the
specific commit at hand though since it was the first time I could
boot my devices with mainline A-TF.

Thank you,
Chris

> 
> 
> Thanks,
> 
> - Kever
> 
> > 
> > Thank you,
> > Chris
> > 
> > > > > Thanks,
> > > > > 
> > > > > - Kever
> > > > > 
> > > > > >     CONFIG_ROCKCHIP_GPIO=y
> > > > > >     CONFIG_SYS_I2C_ROCKCHIP=y
> > > > > >     CONFIG_MISC=y
> > > > Thank you,
> > > > Chris