[PATCH] power: pmic: rk8xx: Workaround pmic failure when probed before relocation

[PATCH] power: pmic: rk8xx: Workaround pmic failure when probed before relocation

[Michal Suchanek]

When the sysreset is added as child of the pmic the pmic is probed
before relocation. That probe fails, and subsequent attempts to probe
after reloaction fail as well.

As a workaround do not bind the sysreset before relocation.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
 drivers/power/pmic/rk8xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/power/pmic/rk8xx.c b/drivers/power/pmic/rk8xx.c
index a239a18674..d12263c4f2 100644
--- a/drivers/power/pmic/rk8xx.c
+++ b/drivers/power/pmic/rk8xx.c
@@ -131,7 +131,7 @@ static int rk8xx_read(struct udevice *dev, uint reg, uint8_t *buff, int len)
 
 static int rk8xx_bind(struct udevice *dev)
 {
-	if (CONFIG_IS_ENABLED(SYSRESET)) {
+	if (CONFIG_IS_ENABLED(SYSRESET) && (gd->flags & GD_FLG_RELOC)) {
 		device_bind_driver(dev, "rk8xx_sysreset",
 				   "rk8xx_sysreset", NULL);
 	}
-- 
2.37.1

Re: [PATCH] power: pmic: rk8xx: Workaround pmic failure when probed before relocation

[Simon Glass]

Hi Michal,

On Fri, 5 Aug 2022 at 05:32, Michal Suchanek <msuchanek@suse.de> wrote:
>
> When the sysreset is added as child of the pmic the pmic is probed
> before relocation. That probe fails, and subsequent attempts to probe
> after reloaction fail as well.
>
> As a workaround do not bind the sysreset before relocation.
>
> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> ---
>  drivers/power/pmic/rk8xx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/power/pmic/rk8xx.c b/drivers/power/pmic/rk8xx.c
> index a239a18674..d12263c4f2 100644
> --- a/drivers/power/pmic/rk8xx.c
> +++ b/drivers/power/pmic/rk8xx.c
> @@ -131,7 +131,7 @@ static int rk8xx_read(struct udevice *dev, uint reg, uint8_t *buff, int len)
>
>  static int rk8xx_bind(struct udevice *dev)
>  {
> -       if (CONFIG_IS_ENABLED(SYSRESET)) {
> +       if (CONFIG_IS_ENABLED(SYSRESET) && (gd->flags & GD_FLG_RELOC)) {
>                 device_bind_driver(dev, "rk8xx_sysreset",
>                                    "rk8xx_sysreset", NULL);
>         }
> --
> 2.37.1
>

I think it is OK to avoid starting a device before relocation, or make
that device do something different. I really don't like the binding
being optional though...we have the 'u-boot,dm-pre-reloc' for that.

But missing from your commit message is exactly what fails?

Regards,
Simon

Re: [PATCH] power: pmic: rk8xx: Workaround pmic failure when probed before relocation

[Michal Suchánek]

On Fri, Aug 05, 2022 at 10:48:26AM -0600, Simon Glass wrote:
> Hi Michal,
> 
> On Fri, 5 Aug 2022 at 05:32, Michal Suchanek <msuchanek@suse.de> wrote:
> >
> > When the sysreset is added as child of the pmic the pmic is probed
> > before relocation. That probe fails, and subsequent attempts to probe
> > after reloaction fail as well.
> >
> > As a workaround do not bind the sysreset before relocation.
> >
> > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > ---
> >  drivers/power/pmic/rk8xx.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/power/pmic/rk8xx.c b/drivers/power/pmic/rk8xx.c
> > index a239a18674..d12263c4f2 100644
> > --- a/drivers/power/pmic/rk8xx.c
> > +++ b/drivers/power/pmic/rk8xx.c
> > @@ -131,7 +131,7 @@ static int rk8xx_read(struct udevice *dev, uint reg, uint8_t *buff, int len)
> >
> >  static int rk8xx_bind(struct udevice *dev)
> >  {
> > -       if (CONFIG_IS_ENABLED(SYSRESET)) {
> > +       if (CONFIG_IS_ENABLED(SYSRESET) && (gd->flags & GD_FLG_RELOC)) {
> >                 device_bind_driver(dev, "rk8xx_sysreset",
> >                                    "rk8xx_sysreset", NULL);
> >         }
> > --
> > 2.37.1
> >
> 
> I think it is OK to avoid starting a device before relocation, or make
> that device do something different. I really don't like the binding
> being optional though...we have the 'u-boot,dm-pre-reloc' for that.

So perhaps the flag should be dropped from rk8xx then.

> 
> But missing from your commit message is exactly what fails?

The pmic is an i2c device, all i2c tranferss fail if initialized
pre-relocation. I supect it's the i2c bus that fails if initialized
before reloc - the regulatorss that share the i2c bus also report i2c
transfer failure.

Thanks

Michal

Re: [PATCH] power: pmic: rk8xx: Workaround pmic failure when probed before relocation

[Simon Glass]

Hi Michal,

On Fri, 5 Aug 2022 at 14:07, Michal Suchánek <msuchanek@suse.de> wrote:
>
> On Fri, Aug 05, 2022 at 10:48:26AM -0600, Simon Glass wrote:
> > Hi Michal,
> >
> > On Fri, 5 Aug 2022 at 05:32, Michal Suchanek <msuchanek@suse.de> wrote:
> > >
> > > When the sysreset is added as child of the pmic the pmic is probed
> > > before relocation. That probe fails, and subsequent attempts to probe
> > > after reloaction fail as well.
> > >
> > > As a workaround do not bind the sysreset before relocation.
> > >
> > > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > > ---
> > >  drivers/power/pmic/rk8xx.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/power/pmic/rk8xx.c b/drivers/power/pmic/rk8xx.c
> > > index a239a18674..d12263c4f2 100644
> > > --- a/drivers/power/pmic/rk8xx.c
> > > +++ b/drivers/power/pmic/rk8xx.c
> > > @@ -131,7 +131,7 @@ static int rk8xx_read(struct udevice *dev, uint reg, uint8_t *buff, int len)
> > >
> > >  static int rk8xx_bind(struct udevice *dev)
> > >  {
> > > -       if (CONFIG_IS_ENABLED(SYSRESET)) {
> > > +       if (CONFIG_IS_ENABLED(SYSRESET) && (gd->flags & GD_FLG_RELOC)) {
> > >                 device_bind_driver(dev, "rk8xx_sysreset",
> > >                                    "rk8xx_sysreset", NULL);
> > >         }
> > > --
> > > 2.37.1
> > >
> >
> > I think it is OK to avoid starting a device before relocation, or make
> > that device do something different. I really don't like the binding
> > being optional though...we have the 'u-boot,dm-pre-reloc' for that.
>
> So perhaps the flag should be dropped from rk8xx then.
>
> >
> > But missing from your commit message is exactly what fails?
>
> The pmic is an i2c device, all i2c tranferss fail if initialized
> pre-relocation. I supect it's the i2c bus that fails if initialized
> before reloc - the regulatorss that share the i2c bus also report i2c
> transfer failure.

OK, I wonder why i2c fails? It works OK on the rockchip devices I'm
familiar with, e.g. chromebooks.

Regards,
Simon

Re: [PATCH] power: pmic: rk8xx: Workaround pmic failure when probed before relocation

[Michal Suchánek]

On Sat, Aug 06, 2022 at 12:21:29PM -0600, Simon Glass wrote:
> Hi Michal,
> 
> On Fri, 5 Aug 2022 at 14:07, Michal Suchánek <msuchanek@suse.de> wrote:
> >
> > On Fri, Aug 05, 2022 at 10:48:26AM -0600, Simon Glass wrote:
> > > Hi Michal,
> > >
> > > On Fri, 5 Aug 2022 at 05:32, Michal Suchanek <msuchanek@suse.de> wrote:
> > > >
> > > > When the sysreset is added as child of the pmic the pmic is probed
> > > > before relocation. That probe fails, and subsequent attempts to probe
> > > > after reloaction fail as well.
> > > >
> > > > As a workaround do not bind the sysreset before relocation.
> > > >
> > > > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > > > ---
> > > >  drivers/power/pmic/rk8xx.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/power/pmic/rk8xx.c b/drivers/power/pmic/rk8xx.c
> > > > index a239a18674..d12263c4f2 100644
> > > > --- a/drivers/power/pmic/rk8xx.c
> > > > +++ b/drivers/power/pmic/rk8xx.c
> > > > @@ -131,7 +131,7 @@ static int rk8xx_read(struct udevice *dev, uint reg, uint8_t *buff, int len)
> > > >
> > > >  static int rk8xx_bind(struct udevice *dev)
> > > >  {
> > > > -       if (CONFIG_IS_ENABLED(SYSRESET)) {
> > > > +       if (CONFIG_IS_ENABLED(SYSRESET) && (gd->flags & GD_FLG_RELOC)) {
> > > >                 device_bind_driver(dev, "rk8xx_sysreset",
> > > >                                    "rk8xx_sysreset", NULL);
> > > >         }
> > > > --
> > > > 2.37.1
> > > >
> > >
> > > I think it is OK to avoid starting a device before relocation, or make
> > > that device do something different. I really don't like the binding
> > > being optional though...we have the 'u-boot,dm-pre-reloc' for that.
> >
> > So perhaps the flag should be dropped from rk8xx then.
> >
> > >
> > > But missing from your commit message is exactly what fails?
> >
> > The pmic is an i2c device, all i2c tranferss fail if initialized
> > pre-relocation. I supect it's the i2c bus that fails if initialized
> > before reloc - the regulatorss that share the i2c bus also report i2c
> > transfer failure.
> 
> OK, I wonder why i2c fails? It works OK on the rockchip devices I'm
> familiar with, e.g. chromebooks.

It worksss fine here as well - so long as it is not initialized before
relocation.

Thanks

Michal

Re: [PATCH] power: pmic: rk8xx: Workaround pmic failure when probed before relocation

[Simon Glass]

Hi Michal,

On Sat, 6 Aug 2022 at 13:54, Michal Suchánek <msuchanek@suse.de> wrote:
>
> On Sat, Aug 06, 2022 at 12:21:29PM -0600, Simon Glass wrote:
> > Hi Michal,
> >
> > On Fri, 5 Aug 2022 at 14:07, Michal Suchánek <msuchanek@suse.de> wrote:
> > >
> > > On Fri, Aug 05, 2022 at 10:48:26AM -0600, Simon Glass wrote:
> > > > Hi Michal,
> > > >
> > > > On Fri, 5 Aug 2022 at 05:32, Michal Suchanek <msuchanek@suse.de> wrote:
> > > > >
> > > > > When the sysreset is added as child of the pmic the pmic is probed
> > > > > before relocation. That probe fails, and subsequent attempts to probe
> > > > > after reloaction fail as well.
> > > > >
> > > > > As a workaround do not bind the sysreset before relocation.
> > > > >
> > > > > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > > > > ---
> > > > >  drivers/power/pmic/rk8xx.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/power/pmic/rk8xx.c b/drivers/power/pmic/rk8xx.c
> > > > > index a239a18674..d12263c4f2 100644
> > > > > --- a/drivers/power/pmic/rk8xx.c
> > > > > +++ b/drivers/power/pmic/rk8xx.c
> > > > > @@ -131,7 +131,7 @@ static int rk8xx_read(struct udevice *dev, uint reg, uint8_t *buff, int len)
> > > > >
> > > > >  static int rk8xx_bind(struct udevice *dev)
> > > > >  {
> > > > > -       if (CONFIG_IS_ENABLED(SYSRESET)) {
> > > > > +       if (CONFIG_IS_ENABLED(SYSRESET) && (gd->flags & GD_FLG_RELOC)) {
> > > > >                 device_bind_driver(dev, "rk8xx_sysreset",
> > > > >                                    "rk8xx_sysreset", NULL);
> > > > >         }
> > > > > --
> > > > > 2.37.1
> > > > >
> > > >
> > > > I think it is OK to avoid starting a device before relocation, or make
> > > > that device do something different. I really don't like the binding
> > > > being optional though...we have the 'u-boot,dm-pre-reloc' for that.
> > >
> > > So perhaps the flag should be dropped from rk8xx then.
> > >
> > > >
> > > > But missing from your commit message is exactly what fails?
> > >
> > > The pmic is an i2c device, all i2c tranferss fail if initialized
> > > pre-relocation. I supect it's the i2c bus that fails if initialized
> > > before reloc - the regulatorss that share the i2c bus also report i2c
> > > transfer failure.
> >
> > OK, I wonder why i2c fails? It works OK on the rockchip devices I'm
> > familiar with, e.g. chromebooks.
>
> It worksss fine here as well - so long as it is not initialized before
> relocation.

I wonder if the clock driver is doing something different then, or has
a limited number of clocks?

Regards,
Simon

Re: [PATCH] power: pmic: rk8xx: Workaround pmic failure when probed before relocation

[Michal Suchánek]

On Mon, Aug 08, 2022 at 01:26:23PM -0600, Simon Glass wrote:
> Hi Michal,
> 
> On Sat, 6 Aug 2022 at 13:54, Michal Suchánek <msuchanek@suse.de> wrote:
> >
> > On Sat, Aug 06, 2022 at 12:21:29PM -0600, Simon Glass wrote:
> > > Hi Michal,
> > >
> > > On Fri, 5 Aug 2022 at 14:07, Michal Suchánek <msuchanek@suse.de> wrote:
> > > >
> > > > On Fri, Aug 05, 2022 at 10:48:26AM -0600, Simon Glass wrote:
> > > > > Hi Michal,
> > > > >
> > > > > On Fri, 5 Aug 2022 at 05:32, Michal Suchanek <msuchanek@suse.de> wrote:
> > > > > >
> > > > > > When the sysreset is added as child of the pmic the pmic is probed
> > > > > > before relocation. That probe fails, and subsequent attempts to probe
> > > > > > after reloaction fail as well.
> > > > > >
> > > > > > As a workaround do not bind the sysreset before relocation.
> > > > > >
> > > > > > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > > > > > ---
> > > > > >  drivers/power/pmic/rk8xx.c | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/power/pmic/rk8xx.c b/drivers/power/pmic/rk8xx.c
> > > > > > index a239a18674..d12263c4f2 100644
> > > > > > --- a/drivers/power/pmic/rk8xx.c
> > > > > > +++ b/drivers/power/pmic/rk8xx.c
> > > > > > @@ -131,7 +131,7 @@ static int rk8xx_read(struct udevice *dev, uint reg, uint8_t *buff, int len)
> > > > > >
> > > > > >  static int rk8xx_bind(struct udevice *dev)
> > > > > >  {
> > > > > > -       if (CONFIG_IS_ENABLED(SYSRESET)) {
> > > > > > +       if (CONFIG_IS_ENABLED(SYSRESET) && (gd->flags & GD_FLG_RELOC)) {
> > > > > >                 device_bind_driver(dev, "rk8xx_sysreset",
> > > > > >                                    "rk8xx_sysreset", NULL);
> > > > > >         }
> > > > > > --
> > > > > > 2.37.1
> > > > > >
> > > > >
> > > > > I think it is OK to avoid starting a device before relocation, or make
> > > > > that device do something different. I really don't like the binding
> > > > > being optional though...we have the 'u-boot,dm-pre-reloc' for that.
> > > >
> > > > So perhaps the flag should be dropped from rk8xx then.
> > > >
> > > > >
> > > > > But missing from your commit message is exactly what fails?
> > > >
> > > > The pmic is an i2c device, all i2c tranferss fail if initialized
> > > > pre-relocation. I supect it's the i2c bus that fails if initialized
> > > > before reloc - the regulatorss that share the i2c bus also report i2c
> > > > transfer failure.
> > >
> > > OK, I wonder why i2c fails? It works OK on the rockchip devices I'm
> > > familiar with, e.g. chromebooks.
> >
> > It worksss fine here as well - so long as it is not initialized before
> > relocation.
> 
> I wonder if the clock driver is doing something different then, or has
> a limited number of clocks?

Looks like it is not the clocks.

There is something slightly fishy going on there with them:

In the broken case the i2c clock is not initialized before reloc

TICE:  BL31: v2.6(debug):
NOTICE:  BL31: Built : 14:50:40, Jul  1 2022
INFO:    GICv3 with legacy support detected.
INFO:    ARM GICv3 driver initialized in EL3
INFO:    Maximum SPI INTID supported: 287
INFO:    plat_rockchip_pmu_init(1624): pd status 3e
INFO:    BL31: Initializing runtime services
INFO:    BL31: cortex_a53: CPU workaround for 855873 was applied
WARNING: BL31: cortex_a53: CPU workaround for 1530924 was missing!
INFO:    BL31: Preparing for EL3 exit to normal world
INFO:    Entry point address = 0x200000
INFO:    SPSR = 0x3c9
rk8xx_bind
global flags 0 relocaddr 0
rk8xx_sysreset_bind
rk8xx_bind: 'pmic@1b' - found regulators subnode
rk3399_clk_get_rate: clk clock-controller@ff760000 83
clock-controller@ff760000 83: clock rate 24000000


U-Boot 2022.07-00038-g61e11a8e9f-dirty (Aug 07 2022 - 19:53:20 +0200)

U-Boot PLL at 00000000ff750000: fbdiv=112, refdiv=2, postdiv1=2, postdiv2=1, vco=1344000 khz, output=672000 khz
i2c@ff3c0000: Got clock pmu-clock-controller@ff750000 9
rockchip_i2c_probe: clk: pmu-clock-controller@ff750000 9 rate 0 count 0 flags 0
rockchip_i2c_set_bus_speed: 400000
rk3399_i2c_get_pmuclk: clk 9, con 771, div 3
rk3399_pmuclk_get_rate: clk pmu-clock-controller@ff750000 9 -> 169000000 <<< different freqency
pmu-clock-controller@ff750000 9: clock rate 169000000
rk_i2c_set_clk: clk pmu-clock-controller@ff750000 9, i2c rate = 169000000, scl rate = 400000
set i2c clk div = 51, divh = 26, divl = 25
set clk(I2C_CLKDIV: 0x001a0019)
rk8xx_probe
original ID register
i2c_xfer: 2 messages
i2c_xfer: chip=0x1b, len=0x1
rk_i2c_write: chip = 27, reg = 0, r_len = 0, b_len = 1
I2c Send Start bit.
I2C Send Start Bit Timeout
i2c_con: 0x00010009
i2c_clkdiv: 0x001a0019
i2c_mrxaddr: 0x00000000
i2c_mrxraddR: 0x00000000
i2c_mtxcnt: 0x00000000
i2c_mrxcnt: 0x00000000
i2c_ien: 0x00000010
i2c_ipd: 0x00000000
i2c_fcnt: 0x00000000
i2c_txdata0: 0x00000000
i2c_txdata1: 0x00000000
i2c_txdata2: 0x00000000
i2c_txdata3: 0x00000000
i2c_txdata4: 0x00000000
i2c_txdata5: 0x00000000
i2c_txdata6: 0x00000000
i2c_txdata7: 0x00000000
i2c_rxdata0: 0x00000000
i2c_rxdata1: 0x00000000
i2c_rxdata2: 0x00000000
i2c_rxdata3: 0x00000000
i2c_rxdata4: 0x00000000
i2c_rxdata5: 0x00000000
i2c_rxdata6: 0x00000000
i2c_rxdata7: 0x00000000
i2c_write: error sending
read from device: 00000000002fcff0 register: 0x17 -> -121
SoC: Rockchip rk3399
Reset cause: RST
Model: Pine64 Pinebook Pro
DRAM:  3.9 GiB
rk8xx_bind
global flags 301 relocaddr f6f1d000
rk8xx_sysreset_bind
rk8xx_bind: 'pmic@1b' - found regulators subnode
U-Boot PLL at 00000000ff750000: fbdiv=112, refdiv=2, postdiv1=2, postdiv2=1, vco=1344000 khz, output=672000 khz
i2c@ff3c0000: Got clock pmu-clock-controller@ff750000 9
rk3399_i2c_set_pmuclk: clk 9, hz 200000000, div 3
rk3399_pmuclk_set_rate: clk pmu-clock-controller@ff750000 9 rate 200000000 -> 169000000 <<< here the old frequency is returned
rockchip_i2c_probe: clk: pmu-clock-controller@ff750000 9 rate 0 count 0 flags 0
rockchip_i2c_set_bus_speed: 400000
rk3399_i2c_get_pmuclk: clk 9, con 770, div 2
rk3399_pmuclk_get_rate: clk pmu-clock-controller@ff750000 9 -> 225333333 <<< but here the frequency is reported higher
pmu-clock-controller@ff750000 9: clock rate 225333333
rk_i2c_set_clk: clk pmu-clock-controller@ff750000 9, i2c rate = 225333333, scl rate = 400000
set i2c clk div = 69, divh = 35, divl = 34
set clk(I2C_CLKDIV: 0x00230022)
rk8xx_probe
original ID register
i2c_xfer: 2 messages
i2c_xfer: chip=0x1b, len=0x1
rk_i2c_write: chip = 27, reg = 0, r_len = 0, b_len = 1
I2c Send Start bit.
I2C Send Start Bit Timeout

Basically the old 169000000 and the new 225333333 frequency are both
different rounding of 200000000 for lower and higher divisor and
generally not all that different.

It is fishy that get_clock_rate and set_clock_rate report different
frequency using the same frequency calcualtion macro. Nonetheless,
forcing the set_clock_rate call from the i2c driver does not help, the
i2c transfers fail anyway:

rk8xx_bind
global flags 0 relocaddr 0
rk8xx_sysreset_bind
rk8xx_bind: 'pmic@1b' - found regulators subnode
rk3399_clk_get_rate: clk clock-controller@ff760000 83


U-Boot 2022.07-00043-g395bdd35fe-dirty (Aug 08 2022 - 22:09:19 +0200)

U-Boot PLL at 00000000ff750000: fbdiv=112, refdiv=2, postdiv1=2, postdiv2=1, vco=1344000 khz, output=672000 khz
i2c@ff3c0000: Got clock pmu-clock-controller@ff750000 9
rk3399_i2c_get_pmuclk: clk 9, con 771, div 3
rk3399_pmuclk_get_rate: clk pmu-clock-controller@ff750000 9 -> 169000000
rockchip_i2c_probe: clk: pmu-clock-controller@ff750000 9 rate 0 count 0 flags 0
rk3399_i2c_set_pmuclk: clk 9, hz 200000000, div 3
rk3399_pmuclk_set_rate: clk pmu-clock-controller@ff750000 9 rate 200000000 -> 169000000
rockchip_i2c_probe: clk: pmu-clock-controller@ff750000 9 rate 0 count 0 flags 0
rockchip_i2c_set_bus_speed: 400000
rk3399_i2c_get_pmuclk: clk 9, con 770, div 2
rk3399_pmuclk_get_rate: clk pmu-clock-controller@ff750000 9 -> 225333333
rk_i2c_set_clk: clk pmu-clock-controller@ff750000 9, i2c rate = 225333333, scl rate = 400000
set i2c clk div = 69, divh = 35, divl = 34
set clk(I2C_CLKDIV: 0x00230022)
rk8xx_probe
original ID register
i2c_xfer: 2 messages
i2c_xfer: chip=0x1b, len=0x1
rk_i2c_write: chip = 27, reg = 0, r_len = 0, b_len = 1
I2c Send Start bit.
I2C Send Start Bit Timeout
i2c_con: 0x00010009
i2c_clkdiv: 0x00230022
i2c_mrxaddr: 0x00000000
i2c_mrxraddR: 0x00000000
i2c_mtxcnt: 0x00000000
i2c_mrxcnt: 0x00000000
i2c_ien: 0x00000010
i2c_ipd: 0x00000000
i2c_fcnt: 0x00000000
i2c_txdata0: 0x00000000
i2c_txdata1: 0x00000000
i2c_txdata2: 0x00000000
i2c_txdata3: 0x00000000
i2c_txdata4: 0x00000000
i2c_txdata5: 0x00000000
i2c_txdata6: 0x00000000
i2c_txdata7: 0x00000000
i2c_rxdata0: 0x00000000
i2c_rxdata1: 0x00000000
i2c_rxdata2: 0x00000000
i2c_rxdata3: 0x00000000
i2c_rxdata4: 0x00000000
i2c_rxdata5: 0x00000000
i2c_rxdata6: 0x00000000
i2c_rxdata7: 0x00000000

It looks like it is not clock related after all, although the i2c clock
getting set post-relocation and not pre-relocation might be source of
problems if the frequency was significantly different without the
set_clock_rate call.

Thanks

Michal

Re: [PATCH] power: pmic: rk8xx: Workaround pmic failure when probed before relocation

[Simon Glass]

Hi Michal,

On Mon, 8 Aug 2022 at 14:27, Michal Suchánek <msuchanek@suse.de> wrote:
>
> On Mon, Aug 08, 2022 at 01:26:23PM -0600, Simon Glass wrote:
> > Hi Michal,
> >
> > On Sat, 6 Aug 2022 at 13:54, Michal Suchánek <msuchanek@suse.de> wrote:
> > >
> > > On Sat, Aug 06, 2022 at 12:21:29PM -0600, Simon Glass wrote:
> > > > Hi Michal,
> > > >
> > > > On Fri, 5 Aug 2022 at 14:07, Michal Suchánek <msuchanek@suse.de> wrote:
> > > > >
> > > > > On Fri, Aug 05, 2022 at 10:48:26AM -0600, Simon Glass wrote:
> > > > > > Hi Michal,
> > > > > >
> > > > > > On Fri, 5 Aug 2022 at 05:32, Michal Suchanek <msuchanek@suse.de> wrote:
> > > > > > >
> > > > > > > When the sysreset is added as child of the pmic the pmic is probed
> > > > > > > before relocation. That probe fails, and subsequent attempts to probe
> > > > > > > after reloaction fail as well.
> > > > > > >
> > > > > > > As a workaround do not bind the sysreset before relocation.
> > > > > > >
> > > > > > > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > > > > > > ---
> > > > > > >  drivers/power/pmic/rk8xx.c | 2 +-
> > > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/drivers/power/pmic/rk8xx.c b/drivers/power/pmic/rk8xx.c
> > > > > > > index a239a18674..d12263c4f2 100644
> > > > > > > --- a/drivers/power/pmic/rk8xx.c
> > > > > > > +++ b/drivers/power/pmic/rk8xx.c
> > > > > > > @@ -131,7 +131,7 @@ static int rk8xx_read(struct udevice *dev, uint reg, uint8_t *buff, int len)
> > > > > > >
> > > > > > >  static int rk8xx_bind(struct udevice *dev)
> > > > > > >  {
> > > > > > > -       if (CONFIG_IS_ENABLED(SYSRESET)) {
> > > > > > > +       if (CONFIG_IS_ENABLED(SYSRESET) && (gd->flags & GD_FLG_RELOC)) {
> > > > > > >                 device_bind_driver(dev, "rk8xx_sysreset",
> > > > > > >                                    "rk8xx_sysreset", NULL);
> > > > > > >         }
> > > > > > > --
> > > > > > > 2.37.1
> > > > > > >
> > > > > >
> > > > > > I think it is OK to avoid starting a device before relocation, or make
> > > > > > that device do something different. I really don't like the binding
> > > > > > being optional though...we have the 'u-boot,dm-pre-reloc' for that.
> > > > >
> > > > > So perhaps the flag should be dropped from rk8xx then.
> > > > >
> > > > > >
> > > > > > But missing from your commit message is exactly what fails?
> > > > >
> > > > > The pmic is an i2c device, all i2c tranferss fail if initialized
> > > > > pre-relocation. I supect it's the i2c bus that fails if initialized
> > > > > before reloc - the regulatorss that share the i2c bus also report i2c
> > > > > transfer failure.
> > > >
> > > > OK, I wonder why i2c fails? It works OK on the rockchip devices I'm
> > > > familiar with, e.g. chromebooks.
> > >
> > > It worksss fine here as well - so long as it is not initialized before
> > > relocation.
> >
> > I wonder if the clock driver is doing something different then, or has
> > a limited number of clocks?
>
> Looks like it is not the clocks.
>
> There is something slightly fishy going on there with them:
>
> In the broken case the i2c clock is not initialized before reloc
>
> TICE:  BL31: v2.6(debug):
> NOTICE:  BL31: Built : 14:50:40, Jul  1 2022
> INFO:    GICv3 with legacy support detected.
> INFO:    ARM GICv3 driver initialized in EL3
> INFO:    Maximum SPI INTID supported: 287
> INFO:    plat_rockchip_pmu_init(1624): pd status 3e
> INFO:    BL31: Initializing runtime services
> INFO:    BL31: cortex_a53: CPU workaround for 855873 was applied
> WARNING: BL31: cortex_a53: CPU workaround for 1530924 was missing!
> INFO:    BL31: Preparing for EL3 exit to normal world
> INFO:    Entry point address = 0x200000
> INFO:    SPSR = 0x3c9
> rk8xx_bind
> global flags 0 relocaddr 0
> rk8xx_sysreset_bind
> rk8xx_bind: 'pmic@1b' - found regulators subnode
> rk3399_clk_get_rate: clk clock-controller@ff760000 83
> clock-controller@ff760000 83: clock rate 24000000
>
>
> U-Boot 2022.07-00038-g61e11a8e9f-dirty (Aug 07 2022 - 19:53:20 +0200)
>
> U-Boot PLL at 00000000ff750000: fbdiv=112, refdiv=2, postdiv1=2, postdiv2=1, vco=1344000 khz, output=672000 khz
> i2c@ff3c0000: Got clock pmu-clock-controller@ff750000 9
> rockchip_i2c_probe: clk: pmu-clock-controller@ff750000 9 rate 0 count 0 flags 0
> rockchip_i2c_set_bus_speed: 400000
> rk3399_i2c_get_pmuclk: clk 9, con 771, div 3
> rk3399_pmuclk_get_rate: clk pmu-clock-controller@ff750000 9 -> 169000000 <<< different freqency
> pmu-clock-controller@ff750000 9: clock rate 169000000
> rk_i2c_set_clk: clk pmu-clock-controller@ff750000 9, i2c rate = 169000000, scl rate = 400000
> set i2c clk div = 51, divh = 26, divl = 25
> set clk(I2C_CLKDIV: 0x001a0019)
> rk8xx_probe
> original ID register
> i2c_xfer: 2 messages
> i2c_xfer: chip=0x1b, len=0x1
> rk_i2c_write: chip = 27, reg = 0, r_len = 0, b_len = 1
> I2c Send Start bit.
> I2C Send Start Bit Timeout
> i2c_con: 0x00010009
> i2c_clkdiv: 0x001a0019
> i2c_mrxaddr: 0x00000000
> i2c_mrxraddR: 0x00000000
> i2c_mtxcnt: 0x00000000
> i2c_mrxcnt: 0x00000000
> i2c_ien: 0x00000010
> i2c_ipd: 0x00000000
> i2c_fcnt: 0x00000000
> i2c_txdata0: 0x00000000
> i2c_txdata1: 0x00000000
> i2c_txdata2: 0x00000000
> i2c_txdata3: 0x00000000
> i2c_txdata4: 0x00000000
> i2c_txdata5: 0x00000000
> i2c_txdata6: 0x00000000
> i2c_txdata7: 0x00000000
> i2c_rxdata0: 0x00000000
> i2c_rxdata1: 0x00000000
> i2c_rxdata2: 0x00000000
> i2c_rxdata3: 0x00000000
> i2c_rxdata4: 0x00000000
> i2c_rxdata5: 0x00000000
> i2c_rxdata6: 0x00000000
> i2c_rxdata7: 0x00000000
> i2c_write: error sending
> read from device: 00000000002fcff0 register: 0x17 -> -121
> SoC: Rockchip rk3399
> Reset cause: RST
> Model: Pine64 Pinebook Pro
> DRAM:  3.9 GiB
> rk8xx_bind
> global flags 301 relocaddr f6f1d000
> rk8xx_sysreset_bind
> rk8xx_bind: 'pmic@1b' - found regulators subnode
> U-Boot PLL at 00000000ff750000: fbdiv=112, refdiv=2, postdiv1=2, postdiv2=1, vco=1344000 khz, output=672000 khz
> i2c@ff3c0000: Got clock pmu-clock-controller@ff750000 9
> rk3399_i2c_set_pmuclk: clk 9, hz 200000000, div 3
> rk3399_pmuclk_set_rate: clk pmu-clock-controller@ff750000 9 rate 200000000 -> 169000000 <<< here the old frequency is returned
> rockchip_i2c_probe: clk: pmu-clock-controller@ff750000 9 rate 0 count 0 flags 0
> rockchip_i2c_set_bus_speed: 400000
> rk3399_i2c_get_pmuclk: clk 9, con 770, div 2
> rk3399_pmuclk_get_rate: clk pmu-clock-controller@ff750000 9 -> 225333333 <<< but here the frequency is reported higher
> pmu-clock-controller@ff750000 9: clock rate 225333333
> rk_i2c_set_clk: clk pmu-clock-controller@ff750000 9, i2c rate = 225333333, scl rate = 400000
> set i2c clk div = 69, divh = 35, divl = 34
> set clk(I2C_CLKDIV: 0x00230022)
> rk8xx_probe
> original ID register
> i2c_xfer: 2 messages
> i2c_xfer: chip=0x1b, len=0x1
> rk_i2c_write: chip = 27, reg = 0, r_len = 0, b_len = 1
> I2c Send Start bit.
> I2C Send Start Bit Timeout
>
> Basically the old 169000000 and the new 225333333 frequency are both
> different rounding of 200000000 for lower and higher divisor and
> generally not all that different.
>
> It is fishy that get_clock_rate and set_clock_rate report different
> frequency using the same frequency calcualtion macro. Nonetheless,
> forcing the set_clock_rate call from the i2c driver does not help, the
> i2c transfers fail anyway:
>
> rk8xx_bind
> global flags 0 relocaddr 0
> rk8xx_sysreset_bind
> rk8xx_bind: 'pmic@1b' - found regulators subnode
> rk3399_clk_get_rate: clk clock-controller@ff760000 83
>
>
> U-Boot 2022.07-00043-g395bdd35fe-dirty (Aug 08 2022 - 22:09:19 +0200)
>
> U-Boot PLL at 00000000ff750000: fbdiv=112, refdiv=2, postdiv1=2, postdiv2=1, vco=1344000 khz, output=672000 khz
> i2c@ff3c0000: Got clock pmu-clock-controller@ff750000 9
> rk3399_i2c_get_pmuclk: clk 9, con 771, div 3
> rk3399_pmuclk_get_rate: clk pmu-clock-controller@ff750000 9 -> 169000000
> rockchip_i2c_probe: clk: pmu-clock-controller@ff750000 9 rate 0 count 0 flags 0
> rk3399_i2c_set_pmuclk: clk 9, hz 200000000, div 3
> rk3399_pmuclk_set_rate: clk pmu-clock-controller@ff750000 9 rate 200000000 -> 169000000
> rockchip_i2c_probe: clk: pmu-clock-controller@ff750000 9 rate 0 count 0 flags 0
> rockchip_i2c_set_bus_speed: 400000
> rk3399_i2c_get_pmuclk: clk 9, con 770, div 2
> rk3399_pmuclk_get_rate: clk pmu-clock-controller@ff750000 9 -> 225333333
> rk_i2c_set_clk: clk pmu-clock-controller@ff750000 9, i2c rate = 225333333, scl rate = 400000
> set i2c clk div = 69, divh = 35, divl = 34
> set clk(I2C_CLKDIV: 0x00230022)
> rk8xx_probe
> original ID register
> i2c_xfer: 2 messages
> i2c_xfer: chip=0x1b, len=0x1
> rk_i2c_write: chip = 27, reg = 0, r_len = 0, b_len = 1
> I2c Send Start bit.
> I2C Send Start Bit Timeout
> i2c_con: 0x00010009
> i2c_clkdiv: 0x00230022
> i2c_mrxaddr: 0x00000000
> i2c_mrxraddR: 0x00000000
> i2c_mtxcnt: 0x00000000
> i2c_mrxcnt: 0x00000000
> i2c_ien: 0x00000010
> i2c_ipd: 0x00000000
> i2c_fcnt: 0x00000000
> i2c_txdata0: 0x00000000
> i2c_txdata1: 0x00000000
> i2c_txdata2: 0x00000000
> i2c_txdata3: 0x00000000
> i2c_txdata4: 0x00000000
> i2c_txdata5: 0x00000000
> i2c_txdata6: 0x00000000
> i2c_txdata7: 0x00000000
> i2c_rxdata0: 0x00000000
> i2c_rxdata1: 0x00000000
> i2c_rxdata2: 0x00000000
> i2c_rxdata3: 0x00000000
> i2c_rxdata4: 0x00000000
> i2c_rxdata5: 0x00000000
> i2c_rxdata6: 0x00000000
> i2c_rxdata7: 0x00000000
>
> It looks like it is not clock related after all, although the i2c clock
> getting set post-relocation and not pre-relocation might be source of
> problems if the frequency was significantly different without the
> set_clock_rate call.

I suppose the other option is pinmux?

Regards,
Simon