Re: [PATCH 09/23] hw/ppc/mpc8544_guts: Populate POR PLL ratio status register

[BALATON Zoltan]

On Mon, 23 Sep 2024, Bernhard Beschow wrote:
> Am 23. September 2024 10:43:19 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
> > On Mon, 23 Sep 2024, Bernhard Beschow wrote:
> > > Populate this read-only register with some arbitrary values which avoids
> > > U-Boot's get_clocks() to hang().
> >
> > Maybe this should be a property settable by the machine as each board 
> > may have different values and it may need to use the correct value for 
> > the machine.
>
> I actually considered this but went with the pragmatic solution to avoid 
> over-engineering. In particular, I wanted to avoid further 
> machine-specitic attributes in the machine class struct. Or do you 
> expect a new e500 machine to be added? In that case I'd set above 
> arbitrary values as default and expect a new machine to override these 
> properties.

Can't override if there's no property for it. There's one machine I may be 
interested in that uses a Freescale e500 SoC. That one seems to use 
0x0606180c for this value which I think corresponds to 0/1 Ratio both 3:1, 
DDR Ratio 12:1 and Plat Ratio 6:1. I think one property to set the 32 bit 
value without individual fields would be enough and we can put comments 
next to the value if needed to note what components it comes from. Or if 
you just need any value here maybe you could take this one then that would 
be good for me as well. (I have some patches adding second i2c bus and SPD 
data that are needed for U-Boot for memory detection but it needs more 
clean up before I can submit it and also waiting for these patches to 
avoid conflict.)

Regards,
BALATON Zoltan

> > > Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> > > ---
> > > hw/ppc/mpc8544_guts.c | 12 ++++++++++++
> > > 1 file changed, 12 insertions(+)
> > >
> > > diff --git a/hw/ppc/mpc8544_guts.c b/hw/ppc/mpc8544_guts.c
> > > index e3540b0281..6688fd44c3 100644
> > > --- a/hw/ppc/mpc8544_guts.c
> > > +++ b/hw/ppc/mpc8544_guts.c
> > > @@ -29,6 +29,12 @@
> > > #define MPC8544_GUTS_RSTCR_RESET      0x02
> > >
> > > #define MPC8544_GUTS_ADDR_PORPLLSR    0x00
> > > +REG32(GUTS_PORPLLSR, 0x00)
> > > +    FIELD(GUTS_PORPLLSR, E500_1_RATIO, 16, 6)
> > > +    FIELD(GUTS_PORPLLSR, E500_0_RATIO, 16, 6)
> > > +    FIELD(GUTS_PORPLLSR, DDR_RATIO, 9, 5)
> > > +    FIELD(GUTS_PORPLLSR, PLAT_RATIO, 1, 5)
> > > +
> > > #define MPC8544_GUTS_ADDR_PORBMSR     0x04
> > > #define MPC8544_GUTS_ADDR_PORIMPSCR   0x08
> > > #define MPC8544_GUTS_ADDR_PORDEVSR    0x0C
> > > @@ -75,6 +81,12 @@ static uint64_t mpc8544_guts_read(void *opaque, hwaddr addr,
> > >
> > >     addr &= MPC8544_GUTS_MMIO_SIZE - 1;
> > >     switch (addr) {
> > > +    case MPC8544_GUTS_ADDR_PORPLLSR:
> > > +        value = FIELD_DP32(value, GUTS_PORPLLSR, E500_1_RATIO, 3); /* 3:2 */
> > > +        value = FIELD_DP32(value, GUTS_PORPLLSR, E500_0_RATIO, 3); /* 3:2 */
> > > +        value = FIELD_DP32(value, GUTS_PORPLLSR, DDR_RATIO, 6); /* 6:1 */
> > > +        value = FIELD_DP32(value, GUTS_PORPLLSR, PLAT_RATIO, 4); /* 4:1 */
> > > +        break;
> > >     case MPC8544_GUTS_ADDR_PVR:
> > >         value = env->spr[SPR_PVR];
> > >         break;