RE: [EXT] Re: [PATCHv3 02/13] hw/arm/fsl-imx8mm: Implemented CCM(Clock Control Module) and Analog IP
> -Original Message- > From: Peter Maydell > Sent: 02 December 2025 15:17 > To: Gaurav Sharma > Cc: [email protected]; [email protected] > Subject: Re: [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 Tue, 2 Dec 2025 at 09:32, Gaurav Sharma > wrote: > > 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 > > You don't need to mention this in the user-facing documentation; it's just an > implementation detail. Otherwise this sounds OK. Okay. > > 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 > > You can structure it like this: > patch 1: add the new property to the analog device patch 2: add the analog > device to the 8mm board (setting > the property) > patch 3: add the CCM device to the 8mm board > Understood
Re: [EXT] Re: [PATCHv3 02/13] hw/arm/fsl-imx8mm: Implemented CCM(Clock Control Module) and Analog IP
On Tue, 2 Dec 2025 at 09:32, Gaurav Sharma wrote: > 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 You don't need to mention this in the user-facing documentation; it's just an implementation detail. Otherwise this sounds OK. > 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 You can structure it like this: patch 1: add the new property to the analog device patch 2: add the analog device to the 8mm board (setting the property) patch 3: add the CCM device to the 8mm board -- PMM
RE: [EXT] Re: [PATCHv3 02/13] hw/arm/fsl-imx8mm: Implemented CCM(Clock Control Module) and Analog IP
> -Original Message- > From: Peter Maydell > Sent: 02 December 2025 02:22 > To: Gaurav Sharma > 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 > wrote: > > > > Implemented Analog device model > > Implemented CCM device model > > > > Signed-off-by: Gaurav Sharma > > 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
Re: [PATCHv3 02/13] hw/arm/fsl-imx8mm: Implemented CCM(Clock Control Module) and Analog IP
On Wed, 19 Nov 2025 at 13:00, Gaurav Sharma wrote: > > Implemented Analog device model > Implemented CCM device model > > Signed-off-by: Gaurav Sharma 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 ? 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. thanks -- PMM
