> -----Original Message-----
> From: Peter Maydell <[email protected]>
> Sent: 02 December 2025 02:22
> To: Gaurav Sharma <[email protected]>
> Cc: [email protected]; [email protected]
> Subject: [EXT] Re: [PATCHv3 02/13] hw/arm/fsl-imx8mm: Implemented
> CCM(Clock Control Module) and Analog IP
>
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report
> this email' button
>
>
> On Wed, 19 Nov 2025 at 13:00, Gaurav Sharma <[email protected]>
> wrote:
> >
> > Implemented Analog device model
> > Implemented CCM device model
> >
> > Signed-off-by: Gaurav Sharma <[email protected]>
>
> Can I ask you to split this one up a bit more, please? Where we add a new
> device, this should be two patches:
> (1) implement the new device
> (2) add the device to the relevant SoC or board
>
> In this case we have two distinct devices, so unless there's some reason I
> missed they should be separate patches, which would make 4 in total.
>
> But: why do we need a new imx8mm_ccm device ? I did a diff against our
> existing imx8mp_ccm source files, and unless I missed something, they're
> exactly identical apart from changing function names etc from 'mp' to 'mm'.
> Can we just use the existing device we have ?
Yes, my original plan was to somehow derive it from the existing imx8mp source
to avoid code duplication. But I wasn't sure about it because of some CCM
differences between imx8mm vs imx8mp, 8MP has additional PLLs for HDMI, Audio,
Video. Also, 8MP adds clocks for ISP,NPU, Media Block control[which is missing
in iMX8MM].
We can very well use the existing device that we have.
> The analog devices also look very similar. In this case there is a tiny
> difference
> in the ANALOG_ARM_PLL_FDIV_CTL0 value.
> But unless we expect these devices to diverge a lot in functionality we
> haven't
> yet implemented, this kind of "almost the same device with minor tweaks" is
> better done by some other method than "copy and paste the source for the
> entire device". A couple of options:
>
> * you can have an abstract base class, which the different
> variants inherit from, with the shared functionality in
> the base class. hw/char/stm32l4x5_usart.c is an example.
>
> * you can have one device, with some properties that the
> SoC sets to configure it. hw/misc/mps2-scc.c is an
> example of this, with uint32 properties for some config
> and ID register reset values.
>
> * you can have one device, with a single property for
> "revision of this h/w" which then drives several different
> unrelated behaviour differences. hw/misc/iotkit-sectl.c
> does this.
>
> Based on the differences I've seen here (i.e. only one register value is
> different), I would suggest the "uint32 properties for register reset values
> that
> differ" approach.
> But you might think one of the other options is more suitable based on better
> knowledge of the actual hardware than I have.
Apologies. I should have executed my earlier plan to maximise code re-use.
Memory map of Analog and the reset values of the registers are almost
identical. we should re-use the 8mp code. I will create another patch revision
that
1. will have code-reuse of imx8mp CCM and Analog
2. will add a uint32 property 'arm_pll_fdiv_ctl0_reset' in imx8mp analog state
struct. imx8mp analog class init will be setting it to its default reset-value.
in fsl-imx8mm we will be overriding this default value with 8mm's reset value.
3. Update the 8mm documentation mentioning the ccm and analog re-use
How does the above sound ?
One question regarding the patch splitting you proposed earlier- Now that we
are re-using ccm and analog of 8mp, will it be like 3 patches ? :-
1 patch that adds CCM device to 8mm in Kconfig
1 patch that adds Analog device to 8mm in Kconfig
1 patch that adds the property 'arm_pll_fdiv_ctl0_reset' in 8mp analog source
Regards
