EDAC on imx8mp and ddr scaling

[Sherry Sun]

> -----Original Message-----
> From: Michael Nazzareno Trimarchi <michael at amarulasolutions.com>
> Sent: Tuesday, February 11, 2025 3:09 PM
> To: Sherry Sun <sherry.sun at nxp.com>
> Cc: Shawn Guo <shawnguo at kernel.org>; linux-arm-kernel <linux-arm-
> kernel at lists.infradead.org>
> Subject: Re: EDAC on imx8mp and ddr scaling
> 
> Hi Sherry
> 
> On Tue, Feb 11, 2025 at 7:34 AM Sherry Sun <sherry.sun at nxp.com> wrote:
> >
> > > -----Original Message-----
> > > From: Michael Nazzareno Trimarchi <michael at amarulasolutions.com>
> > > Sent: Friday, February 7, 2025 5:37 PM
> > > To: Shawn Guo <shawnguo at kernel.org>; Sherry Sun
> <sherry.sun at nxp.com>
> > > Cc: linux-arm-kernel <linux-arm-kernel at lists.infradead.org>
> > > Subject: EDAC on imx8mp and ddr scaling
> > >
> > > Hi Sherry and Shawn
> > >
> > > I have seen that with this patch "arm64: dts: imx8mp: add ddr
> > > controller node to support EDAC on imx8mp" but on the same time the
> > > ddr scaling is coming from arm_smccc_smc DVFS frequency scaling that
> > > is managed from imx8m- ddrc. We have tested it a bit to understand
> > > if the circle to have the code call from linux to ATF was somehow
> > > working. We don't check other than small things.
> > > If the imx8mp is compatible with imx8m-ddrc and synopsys memory
> > > controller, should not needed to merge the support in the synopsys
> > > connected to the board platform data?
> > >
> >
> > Hi Michael,
> >
> > The imx8m-ddrc you reference here is totally different thing with the
> > synopsys_edac that my patch adds.
> > imx8m-ddrc targets dynamic frequency scaling of DRAM, while
> > synopsys_edac targets DDR ECC feature(error detection and correction).
> >
> 
> Ok, that is clear but both insist on the same object, the memory controller.
> Now I need to check better but the memory controller is de-facto coordinate
> by the ATF in:
> - memory frequency scaling
> - suspend/resume
> 
> At the moment I don;t know if  I can consider them independent. What II
> have done quick is:
> 
> diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c index
> 5db541f0eaf1..e923d789757c 100644
> --- a/drivers/clk/imx/clk-imx8mp.c
> +++ b/drivers/clk/imx/clk-imx8mp.c
> @@ -556,8 +556,14 @@ static int imx8mp_clocks_probe(struct
> platform_device *pdev)
> 
>   hws[IMX8MP_CLK_IPG_ROOT] = imx_clk_hw_divider2("ipg_root",
> "ahb_root", ccm_base + 0x9080, 0, 1);
> 
> - hws[IMX8MP_CLK_DRAM_ALT] = imx8m_clk_hw_composite("dram_alt",
> imx8mp_dram_alt_sels, ccm_base + 0xa000);
> - hws[IMX8MP_CLK_DRAM_APB] =
> imx8m_clk_hw_composite_critical("dram_apb", imx8mp_dram_apb_sels,
> ccm_base + 0xa080);
> + /*
> +         * DRAM clocks are manipulated from TF-A outside clock framework.
> +         * The fw_managed helper sets GET_RATE_NOCACHE and clears
> SET_PARENT_GATE
> +         * as div value should always be read from hardware
> +         */
> + hws[IMX8MP_CLK_DRAM_ALT] =
> imx8m_clk_hw_fw_managed_composite("dram_alt", imx8mp_dram_alt_sels,
> ccm_base + 0xa000);
> + hws[IMX8MP_CLK_DRAM_APB] =
> imx8m_clk_hw_fw_managed_composite_critical("dram_apb",
> imx8mp_dram_apb_sels, ccm_base + 0xa080);
> +
>   hws[IMX8MP_CLK_VPU_G1] = imx8m_clk_hw_composite("vpu_g1",
> imx8mp_vpu_g1_sels, ccm_base + 0xa100);
>   hws[IMX8MP_CLK_VPU_G2] = imx8m_clk_hw_composite("vpu_g2",
> imx8mp_vpu_g2_sels, ccm_base + 0xa180);
>   hws[IMX8MP_CLK_CAN1] = imx8m_clk_hw_composite("can1",
> imx8mp_can1_sels, ccm_base + 0xa200);
> 
> And in the board:
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx8mp-icore-mx8mp.dtsi
> b/arch/arm64/boot/dts/freescale/imx8mp-icore-mx8mp.dtsi
> index ffd3672255c0..f96a9d9f64d1 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mp-icore-mx8mp.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mp-icore-mx8mp.dtsi
> @@ -194,3 +194,23 @@ &edacmc {
>   <&clk IMX8MP_CLK_DRAM_ALT>,
>   <&clk IMX8MP_CLK_DRAM_APB>;
>  };
> +
> +&edacmc {
> + operating-points-v2 = <&ddrc_opp_table>;
> +
> + ddrc_opp_table: opp-table {
> + compatible = "operating-points-v2";
> +
> + opp-25000000 {
> + opp-hz = /bits/ 64 <25000000>;
> + };
> +
> + opp-100000000 {
> + opp-hz = /bits/ 64 <100000000>;
> + };
> +
> + opp-900000000 {
> + opp-hz = /bits/ 64 <900000000>;
> + };
> + };
> +};
> 
> +
> +&edacmc {
> + compatible = "fsl,imx8mm-ddrc", "fsl,imx8m-ddrc";  reg = <0x3d400000
> +0x400000>;  clock-names = "core", "pll", "alt", "apb";  clocks = <&clk
> +IMX8MP_CLK_DRAM_CORE>,  <&clk IMX8MP_DRAM_PLL>,  <&clk
> +IMX8MP_CLK_DRAM_ALT>,  <&clk IMX8MP_CLK_DRAM_APB>; };
> 
> I have just a quick copy and paste in the email. This gives me a quick idea of
> the status. So from one side I have on reg = <0x3d400000 0x400000> the
> imx8m-ddrc driver and on the other the synopsys. Do you have a plan on how
> those two functions can be coordinated with each other?
> 

Hi Michael, I am not familiar with the imx8m-ddrc driver, and as I said above, there is no relationship or conflict between the imx8m-ddrc driver and the synopsys_edac driver. Regarding the "memory controller" name you are confused here, we added it because we need to get some memory controller registers for EDAC feature.
It is unclear to me that what you want do on imx8mp, but for EDAC function, if you want to understand more, maybe you can check https://buttersideup.com/mediawiki/index.php/Main_Page and https://www.kernel.org/doc/html/v4.14/driver-api/edac.html. Thanks!

Best Regards
Sherry