> Date: Wed, 26 Aug 2020 21:54:30 +0000 > From: a...@sdf.org > > > > reg |= H3_CPUX_CLK_SRC_SEL_OSC24M; > > > SXIWRITE4(sc, H3_CPUX_AXI_CFG_REG, reg); > > > + /* Must wait at least 8 cycles of the current clock. */ > > > + delay(1); > > > > > > error = sxiccmu_h3_set_frequency(sc, H3_CLK_PLL_CPUX, freq); > > > > > > @@ -1673,6 +1686,7 @@ sxiccmu_h3_set_frequency(struct sxiccmu_ > > > reg &= ~H3_CPUX_CLK_SRC_SEL; > > > reg |= H3_CPUX_CLK_SRC_SEL_PLL_CPUX; > > > SXIWRITE4(sc, H3_CPUX_AXI_CFG_REG, reg); > > > + delay(1); > > > > Is this delay really necessary? It makes very little sense to me to > > add a delay here. We've switched back to the PLL, and whether we spin > > for a bit or actually execute useful code whouldn't matter... > > Hello, > > In the datasheet, in the description of CPUX_AXI_CFG_REG it reads > "if the clock source is changed, at most to wait for 8 present > running clock cycles." It doesn't say why do you have to wait, > nor _for_what_ do you have to wait. So I assume it refers to not > interact with other components of the soc until at least 8 nop > operations. I put those delay() to notice this, although as I said > before I don't know if this would be the best choice. For cases > like this maybe it would be better to define a delay macro in cycles > with some inline assembly. > > I don't know if this is really necessary here, but if you don't > include such a delay I think it would be a good idea to notice this > in a comment, so in the future if the code gets more complicated > a possible bug could be sprayed.
Ok, missed that. And it seems Linux has the delay as well. And there is a comment for the first delay(1) where we switch to the 24MHz clock. Probably good to repeat that comment when we switch back. With that fixed, this diff is ok kettenis@ Thanks for figuring this stuff out.
