[PATCH v2] PM / devfreq: rk3399_dmc: Fix kernel oops when rockchip,pmu is absent

[PATCH v2] PM / devfreq: rk3399_dmc: Fix kernel oops when rockchip,pmu is absent

[Marc Zyngier]

Booting a recent kernel on a rk3399-based system (nanopc-t4),
equipped with a recent u-boot and ATF results in the following:

[    5.607431] Unable to handle kernel NULL pointer dereference at virtual address 00000000000001e4
[    5.608219] Mem abort info:
[    5.608469]   ESR = 0x96000004
[    5.608749]   EC = 0x25: DABT (current EL), IL = 32 bits
[    5.609223]   SET = 0, FnV = 0
[    5.609600]   EA = 0, S1PTW = 0
[    5.609891] Data abort info:
[    5.610149]   ISV = 0, ISS = 0x00000004
[    5.610489]   CM = 0, WnR = 0
[    5.610757] user pgtable: 4k pages, 48-bit VAs, pgdp=00000000e62fb000
[    5.611326] [00000000000001e4] pgd=0000000000000000, p4d=0000000000000000
[    5.611931] Internal error: Oops: 96000004 [#1] SMP
[    5.612363] Modules linked in: rockchip_thermal(E+) rk3399_dmc(E+) soundcore(E) dw_wdt(E) rockchip_dfi(E) nvmem_rockchip_efuse(E) pwm_rockchip(E) cfg80211(E+) rockchip_saradc(E) industrialio(E) rfkill(E) cpufreq_dt(E) ip_tables(E) x_tables(E) autofs4(E) ext4(E) crc32c_generic(E) crc16(E) mbcache(E) jbd2(E) realtek(E) nvme(E) nvme_core(E) t10_pi(E) xhci_plat_hcd(E) xhci_hcd(E) rtc_rk808(E) rk808_regulator(E) clk_rk808(E) dwc3(E) udc_core(E) roles(E) ulpi(E) rk808(E) fan53555(E) rockchipdrm(E) analogix_dp(E) dw_hdmi(E) cec(E) dw_mipi_dsi(E) fixed(E) dwc3_of_simple(E) phy_rockchip_emmc(E) gpio_keys(E) drm_kms_helper(E) phy_rockchip_inno_usb2(E) ehci_platform(E) dwmac_rk(E) stmmac_platform(E) phy_rockchip_pcie(E) ohci_platform(E) ohci_hcd(E) rockchip_io_domain(E) stmmac(E) phy_rockchip_typec(E) ehci_hcd(E) sdhci_of_arasan(E) mdio_xpcs(E) sdhci_pltfm(E) cqhci(E) drm(E) sdhci(E) phylink(E) of_mdio(E) usbcore(E) i2c_rk3x(E) dw_mmc_rockchip(E) dw_mmc_pltfm(E) dw_mmc(E) fixed_phy(E) libphy(E)
[    5.612454]  pl330(E)
[    5.620255] CPU: 1 PID: 270 Comm: systemd-udevd Tainted: G            E     5.7.0-13692-g83ae758d8b22 #1157
[    5.621110] Hardware name: rockchip evb_rk3399/evb_rk3399, BIOS 2020.07-rc4-00023-g10d4cafe0f 06/10/2020
[    5.621947] pstate: 40000005 (nZcv daif -PAN -UAO BTYPE=--)
[    5.622446] pc : regmap_read+0x1c/0x80
[    5.622787] lr : rk3399_dmcfreq_probe+0x6a4/0x8c0 [rk3399_dmc]
[    5.623299] sp : ffff8000126cb8a0
[    5.623594] x29: ffff8000126cb8a0 x28: ffff8000126cbdb0
[    5.624063] x27: ffff0000f22dac40 x26: ffff0000f6779800
[    5.624533] x25: ffff0000f6779810 x24: 00000000ffffffea
[    5.625002] x23: 00000000ffffffea x22: ffff0000f65b74c8
[    5.625471] x21: ffff0000f783ca08 x20: ffff0000f65b7480
[    5.625941] x19: 0000000000000000 x18: 0000000000000001
[    5.626410] x17: 0000000000000000 x16: 0000000000000000
[    5.626878] x15: ffff0000f22db138 x14: ffffffffffffffff
[    5.627347] x13: 0000000000000018 x12: ffff80001106a8c7
[    5.627817] x11: 0000000000000003 x10: 0101010101010101
[    5.627861] systemd[1]: Found device SPCC M.2 PCIE SSD 3.
[    5.628286] x9 : ffff800008d7c89c x8 : 7f7f7f7f7f7f7f7f
[    5.629238] x7 : fefefeff646c606d x6 : 1c0e0e0ee3e8e9f0
[    5.629709] x5 : 706968630e0e0e1c x4 : 8080808000000000
[    5.630178] x3 : 937b1b5b1b434b80 x2 : ffff8000126cb944
[    5.630648] x1 : 0000000000000308 x0 : 0000000000000000
[    5.631119] Call trace:
[    5.631346]  regmap_read+0x1c/0x80
[    5.631654]  rk3399_dmcfreq_probe+0x6a4/0x8c0 [rk3399_dmc]
[    5.632142]  platform_drv_probe+0x5c/0xb0
[    5.632500]  really_probe+0xe4/0x448
[    5.632819]  driver_probe_device+0xfc/0x168
[    5.633191]  device_driver_attach+0x7c/0x88
[    5.633567]  __driver_attach+0xac/0x178
[    5.633914]  bus_for_each_dev+0x78/0xc8
[    5.634261]  driver_attach+0x2c/0x38
[    5.634582]  bus_add_driver+0x14c/0x230
[    5.634925]  driver_register+0x6c/0x128
[    5.635269]  __platform_driver_register+0x50/0x60
[    5.635692]  rk3399_dmcfreq_driver_init+0x2c/0x1000 [rk3399_dmc]
[    5.636226]  do_one_initcall+0x50/0x230
[    5.636569]  do_init_module+0x60/0x248
[    5.636902]  load_module+0x21f8/0x28d8
[    5.637237]  __do_sys_finit_module+0xb0/0x118
[    5.637627]  __arm64_sys_finit_module+0x28/0x38
[    5.638031]  el0_svc_common.constprop.0+0x7c/0x1f8
[    5.638456]  do_el0_svc+0x2c/0x98
[    5.638754]  el0_svc+0x18/0x48
[    5.639029]  el0_sync_handler+0x8c/0x2d4
[    5.639378]  el0_sync+0x158/0x180
[    5.639680] Code: a9bd7bfd 910003fd a90153f3 aa0003f3 (b941e400)
[    5.640221] ---[ end trace 63675fe5d0021970 ]---

This turns out to be due to the rk3399-dmc driver looking for
an *undocumented* property (rockchip,pmu), and happily using
a NULL pointer when the property isn't there.

Instead, make most of what was brought in with 9173c5ceb035
("PM / devfreq: rk3399_dmc: Pass ODT and auto power down parameters
to TF-A.") conditioned on finding this property in the device-tree,
preventing the driver from exploding.

Fixes: 9173c5ceb035 ("PM / devfreq: rk3399_dmc: Pass ODT and auto power down parameters to TF-A.")
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/devfreq/rk3399_dmc.c | 42 ++++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 19 deletions(-)

diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c
index 24f04f78285b..027769e39f9b 100644
--- a/drivers/devfreq/rk3399_dmc.c
+++ b/drivers/devfreq/rk3399_dmc.c
@@ -95,18 +95,20 @@ static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq,
 
 	mutex_lock(&dmcfreq->lock);
 
-	if (target_rate >= dmcfreq->odt_dis_freq)
-		odt_enable = true;
-
-	/*
-	 * This makes a SMC call to the TF-A to set the DDR PD (power-down)
-	 * timings and to enable or disable the ODT (on-die termination)
-	 * resistors.
-	 */
-	arm_smccc_smc(ROCKCHIP_SIP_DRAM_FREQ, dmcfreq->odt_pd_arg0,
-		      dmcfreq->odt_pd_arg1,
-		      ROCKCHIP_SIP_CONFIG_DRAM_SET_ODT_PD,
-		      odt_enable, 0, 0, 0, &res);
+	if (dmcfreq->regmap_pmu) {
+		if (target_rate >= dmcfreq->odt_dis_freq)
+			odt_enable = true;
+
+		/*
+		 * This makes a SMC call to the TF-A to set the DDR PD
+		 * (power-down) timings and to enable or disable the
+		 * ODT (on-die termination) resistors.
+		 */
+		arm_smccc_smc(ROCKCHIP_SIP_DRAM_FREQ, dmcfreq->odt_pd_arg0,
+			      dmcfreq->odt_pd_arg1,
+			      ROCKCHIP_SIP_CONFIG_DRAM_SET_ODT_PD,
+			      odt_enable, 0, 0, 0, &res);
+	}
 
 	/*
 	 * If frequency scaling from low to high, adjust voltage first.
@@ -371,13 +373,14 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev)
 	}
 
 	node = of_parse_phandle(np, "rockchip,pmu", 0);
-	if (node) {
-		data->regmap_pmu = syscon_node_to_regmap(node);
-		of_node_put(node);
-		if (IS_ERR(data->regmap_pmu)) {
-			ret = PTR_ERR(data->regmap_pmu);
-			goto err_edev;
-		}
+	if (!node)
+		goto no_pmu;
+
+	data->regmap_pmu = syscon_node_to_regmap(node);
+	of_node_put(node);
+	if (IS_ERR(data->regmap_pmu)) {
+		ret = PTR_ERR(data->regmap_pmu);
+		goto err_edev;
 	}
 
 	regmap_read(data->regmap_pmu, RK3399_PMUGRF_OS_REG2, &val);
@@ -399,6 +402,7 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev)
 		goto err_edev;
 	};
 
+no_pmu:
 	arm_smccc_smc(ROCKCHIP_SIP_DRAM_FREQ, 0, 0,
 		      ROCKCHIP_SIP_CONFIG_DRAM_INIT,
 		      0, 0, 0, 0, &res);
-- 
2.26.2

Re: [PATCH v2] PM / devfreq: rk3399_dmc: Fix kernel oops when rockchip,pmu is absent

[Chanwoo Choi]

Hi Marc,

On 6/23/20 12:28 AM, Marc Zyngier wrote:
> Booting a recent kernel on a rk3399-based system (nanopc-t4),
> equipped with a recent u-boot and ATF results in the following:
> 
> [    5.607431] Unable to handle kernel NULL pointer dereference at virtual address 00000000000001e4
> [    5.608219] Mem abort info:
> [    5.608469]   ESR = 0x96000004
> [    5.608749]   EC = 0x25: DABT (current EL), IL = 32 bits
> [    5.609223]   SET = 0, FnV = 0
> [    5.609600]   EA = 0, S1PTW = 0
> [    5.609891] Data abort info:
> [    5.610149]   ISV = 0, ISS = 0x00000004
> [    5.610489]   CM = 0, WnR = 0
> [    5.610757] user pgtable: 4k pages, 48-bit VAs, pgdp=00000000e62fb000
> [    5.611326] [00000000000001e4] pgd=0000000000000000, p4d=0000000000000000
> [    5.611931] Internal error: Oops: 96000004 [#1] SMP
> [    5.612363] Modules linked in: rockchip_thermal(E+) rk3399_dmc(E+) soundcore(E) dw_wdt(E) rockchip_dfi(E) nvmem_rockchip_efuse(E) pwm_rockchip(E) cfg80211(E+) rockchip_saradc(E) industrialio(E) rfkill(E) cpufreq_dt(E) ip_tables(E) x_tables(E) autofs4(E) ext4(E) crc32c_generic(E) crc16(E) mbcache(E) jbd2(E) realtek(E) nvme(E) nvme_core(E) t10_pi(E) xhci_plat_hcd(E) xhci_hcd(E) rtc_rk808(E) rk808_regulator(E) clk_rk808(E) dwc3(E) udc_core(E) roles(E) ulpi(E) rk808(E) fan53555(E) rockchipdrm(E) analogix_dp(E) dw_hdmi(E) cec(E) dw_mipi_dsi(E) fixed(E) dwc3_of_simple(E) phy_rockchip_emmc(E) gpio_keys(E) drm_kms_helper(E) phy_rockchip_inno_usb2(E) ehci_platform(E) dwmac_rk(E) stmmac_platform(E) phy_rockchip_pcie(E) ohci_platform(E) ohci_hcd(E) rockchip_io_domain(E) stmmac(E) phy_rockchip_typec(E) ehci_hcd(E) sdhci_of_arasan(E) mdio_xpcs(E) sdhci_pltfm(E) cqhci(E) drm(E) sdhci(E) phylink(E) of_mdio(E) usbcore(E) i2c_rk3x(E) dw_mmc_rockchip(E) dw_mmc_pltfm(E) dw_mmc(E) fixed_phy(E) libphy(E)
> [    5.612454]  pl330(E)
> [    5.620255] CPU: 1 PID: 270 Comm: systemd-udevd Tainted: G            E     5.7.0-13692-g83ae758d8b22 #1157
> [    5.621110] Hardware name: rockchip evb_rk3399/evb_rk3399, BIOS 2020.07-rc4-00023-g10d4cafe0f 06/10/2020
> [    5.621947] pstate: 40000005 (nZcv daif -PAN -UAO BTYPE=--)
> [    5.622446] pc : regmap_read+0x1c/0x80
> [    5.622787] lr : rk3399_dmcfreq_probe+0x6a4/0x8c0 [rk3399_dmc]
> [    5.623299] sp : ffff8000126cb8a0
> [    5.623594] x29: ffff8000126cb8a0 x28: ffff8000126cbdb0
> [    5.624063] x27: ffff0000f22dac40 x26: ffff0000f6779800
> [    5.624533] x25: ffff0000f6779810 x24: 00000000ffffffea
> [    5.625002] x23: 00000000ffffffea x22: ffff0000f65b74c8
> [    5.625471] x21: ffff0000f783ca08 x20: ffff0000f65b7480
> [    5.625941] x19: 0000000000000000 x18: 0000000000000001
> [    5.626410] x17: 0000000000000000 x16: 0000000000000000
> [    5.626878] x15: ffff0000f22db138 x14: ffffffffffffffff
> [    5.627347] x13: 0000000000000018 x12: ffff80001106a8c7
> [    5.627817] x11: 0000000000000003 x10: 0101010101010101
> [    5.627861] systemd[1]: Found device SPCC M.2 PCIE SSD 3.
> [    5.628286] x9 : ffff800008d7c89c x8 : 7f7f7f7f7f7f7f7f
> [    5.629238] x7 : fefefeff646c606d x6 : 1c0e0e0ee3e8e9f0
> [    5.629709] x5 : 706968630e0e0e1c x4 : 8080808000000000
> [    5.630178] x3 : 937b1b5b1b434b80 x2 : ffff8000126cb944
> [    5.630648] x1 : 0000000000000308 x0 : 0000000000000000
> [    5.631119] Call trace:
> [    5.631346]  regmap_read+0x1c/0x80
> [    5.631654]  rk3399_dmcfreq_probe+0x6a4/0x8c0 [rk3399_dmc]
> [    5.632142]  platform_drv_probe+0x5c/0xb0
> [    5.632500]  really_probe+0xe4/0x448
> [    5.632819]  driver_probe_device+0xfc/0x168
> [    5.633191]  device_driver_attach+0x7c/0x88
> [    5.633567]  __driver_attach+0xac/0x178
> [    5.633914]  bus_for_each_dev+0x78/0xc8
> [    5.634261]  driver_attach+0x2c/0x38
> [    5.634582]  bus_add_driver+0x14c/0x230
> [    5.634925]  driver_register+0x6c/0x128
> [    5.635269]  __platform_driver_register+0x50/0x60
> [    5.635692]  rk3399_dmcfreq_driver_init+0x2c/0x1000 [rk3399_dmc]
> [    5.636226]  do_one_initcall+0x50/0x230
> [    5.636569]  do_init_module+0x60/0x248
> [    5.636902]  load_module+0x21f8/0x28d8
> [    5.637237]  __do_sys_finit_module+0xb0/0x118
> [    5.637627]  __arm64_sys_finit_module+0x28/0x38
> [    5.638031]  el0_svc_common.constprop.0+0x7c/0x1f8
> [    5.638456]  do_el0_svc+0x2c/0x98
> [    5.638754]  el0_svc+0x18/0x48
> [    5.639029]  el0_sync_handler+0x8c/0x2d4
> [    5.639378]  el0_sync+0x158/0x180
> [    5.639680] Code: a9bd7bfd 910003fd a90153f3 aa0003f3 (b941e400)
> [    5.640221] ---[ end trace 63675fe5d0021970 ]---
> 
> This turns out to be due to the rk3399-dmc driver looking for
> an *undocumented* property (rockchip,pmu), and happily using
> a NULL pointer when the property isn't there.
> 
> Instead, make most of what was brought in with 9173c5ceb035
> ("PM / devfreq: rk3399_dmc: Pass ODT and auto power down parameters
> to TF-A.") conditioned on finding this property in the device-tree,
> preventing the driver from exploding.
> 
> Fixes: 9173c5ceb035 ("PM / devfreq: rk3399_dmc: Pass ODT and auto power down parameters to TF-A.")
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  drivers/devfreq/rk3399_dmc.c | 42 ++++++++++++++++++++----------------
>  1 file changed, 23 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c
> index 24f04f78285b..027769e39f9b 100644
> --- a/drivers/devfreq/rk3399_dmc.c
> +++ b/drivers/devfreq/rk3399_dmc.c
> @@ -95,18 +95,20 @@ static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq,
>  
>  	mutex_lock(&dmcfreq->lock);
>  
> -	if (target_rate >= dmcfreq->odt_dis_freq)
> -		odt_enable = true;
> -
> -	/*
> -	 * This makes a SMC call to the TF-A to set the DDR PD (power-down)
> -	 * timings and to enable or disable the ODT (on-die termination)
> -	 * resistors.
> -	 */
> -	arm_smccc_smc(ROCKCHIP_SIP_DRAM_FREQ, dmcfreq->odt_pd_arg0,
> -		      dmcfreq->odt_pd_arg1,
> -		      ROCKCHIP_SIP_CONFIG_DRAM_SET_ODT_PD,
> -		      odt_enable, 0, 0, 0, &res);
> +	if (dmcfreq->regmap_pmu) {
> +		if (target_rate >= dmcfreq->odt_dis_freq)
> +			odt_enable = true;
> +
> +		/*
> +		 * This makes a SMC call to the TF-A to set the DDR PD
> +		 * (power-down) timings and to enable or disable the
> +		 * ODT (on-die termination) resistors.
> +		 */
> +		arm_smccc_smc(ROCKCHIP_SIP_DRAM_FREQ, dmcfreq->odt_pd_arg0,
> +			      dmcfreq->odt_pd_arg1,
> +			      ROCKCHIP_SIP_CONFIG_DRAM_SET_ODT_PD,
> +			      odt_enable, 0, 0, 0, &res);
> +	}
>  
>  	/*
>  	 * If frequency scaling from low to high, adjust voltage first.
> @@ -371,13 +373,14 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev)
>  	}
>  
>  	node = of_parse_phandle(np, "rockchip,pmu", 0);
> -	if (node) {
> -		data->regmap_pmu = syscon_node_to_regmap(node);
> -		of_node_put(node);
> -		if (IS_ERR(data->regmap_pmu)) {
> -			ret = PTR_ERR(data->regmap_pmu);
> -			goto err_edev;
> -		}
> +	if (!node)
> +		goto no_pmu;
> +
> +	data->regmap_pmu = syscon_node_to_regmap(node);
> +	of_node_put(node);
> +	if (IS_ERR(data->regmap_pmu)) {
> +		ret = PTR_ERR(data->regmap_pmu);
> +		goto err_edev;
>  	}
>  
>  	regmap_read(data->regmap_pmu, RK3399_PMUGRF_OS_REG2, &val);
> @@ -399,6 +402,7 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev)
>  		goto err_edev;
>  	};
>  
> +no_pmu:
>  	arm_smccc_smc(ROCKCHIP_SIP_DRAM_FREQ, 0, 0,
>  		      ROCKCHIP_SIP_CONFIG_DRAM_INIT,
>  		      0, 0, 0, 0, &res);
> 

It looks good to me. But, I think that it is not necessary
fully kernel panic log about NULL pointer. It is enoughspsp
just mentioning the NULL pointer issue without full kernel panic log.

So, how about editing the patch description as following or others simply?
and we need to add 'stable@vger.kernel.org' to Cc list for applying it
to stable branch.


  PM / devfreq: rk3399_dmc: Fix kernel oops when rockchip,pmu is absent

    Booting a recent kernel on a rk3399-based system (nanopc-t4),
    equipped with a recent u-boot and ATF results in the kernel panic
    about NULL pointer issue.

    This turns out to be due to the rk3399-dmc driver looking for
    an *undocumented* property (rockchip,pmu), and happily using
    a NULL pointer when the property isn't there.

    Instead, make most of what was brought in with 9173c5ceb035
    ("PM / devfreq: rk3399_dmc: Pass ODT and auto power down parameters
    to TF-A.") conditioned on finding this property in the device-tree,
    preventing the driver from exploding.

    Fixes: 9173c5ceb035 ("PM / devfreq: rk3399_dmc: Pass ODT and auto power down parameters to TF-A.")
    Signed-off-by: Marc Zyngier <maz@kernel.org>
    Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2] PM / devfreq: rk3399_dmc: Fix kernel oops when rockchip,pmu is absent

[Marc Zyngier]

Hi Chanwoo,

On Mon, 29 Jun 2020 03:43:37 +0100,
Chanwoo Choi <cw00.choi@samsung.com> wrote:
> 
> Hi Marc,
> 
> On 6/23/20 12:28 AM, Marc Zyngier wrote:

[...]

> It looks good to me. But, I think that it is not necessary
> fully kernel panic log about NULL pointer. It is enoughspsp
> just mentioning the NULL pointer issue without full kernel panic log.

I personally find the backtrace useful as it allows people with the
same issue to trawl the kernel log and find whether it has already be
fixed upstream. But it's only me, and I'm not attached to it.

> So, how about editing the patch description as following or others simply?
> and we need to add 'stable@vger.kernel.org' to Cc list for applying it
> to stable branch.

Looks good to me.

> 
> 
>   PM / devfreq: rk3399_dmc: Fix kernel oops when rockchip,pmu is absent
> 
>     Booting a recent kernel on a rk3399-based system (nanopc-t4),
>     equipped with a recent u-boot and ATF results in the kernel panic
>     about NULL pointer issue.

nit: "results in a kernel panic on dereferencing a NULL pointer".

> 
>     This turns out to be due to the rk3399-dmc driver looking for
>     an *undocumented* property (rockchip,pmu), and happily using
>     a NULL pointer when the property isn't there.
>
>     Instead, make most of what was brought in with 9173c5ceb035
>     ("PM / devfreq: rk3399_dmc: Pass ODT and auto power down parameters
>     to TF-A.") conditioned on finding this property in the device-tree,
>     preventing the driver from exploding.
> 
>     Fixes: 9173c5ceb035 ("PM / devfreq: rk3399_dmc: Pass ODT and auto power down parameters to TF-A.")
>     Signed-off-by: Marc Zyngier <maz@kernel.org>
>     Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>


Note that the biggest issue is still there: the driver is using an
undocumented property, and this patch is just papering over it.
Since I expect this property to be useful for something, it would be
good for whoever knows what it does to document it.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v2] PM / devfreq: rk3399_dmc: Fix kernel oops when rockchip,pmu is absent

[Chanwoo Choi]

Hi Enric,

Could you check this issue? Your patch[1] causes this issue.
As Marc mentioned, although rk3399-dmc.c handled 'rockchip,pmu'
as the mandatory property, your patch[1] didn't add the 'rockchip,pmu'
property to the documentation. 

[1] 9173c5ceb035 ("PM / devfreq: rk3399_dmc: Pass ODT
and auto power down parameters to TF-A.")


On 6/29/20 5:18 PM, Marc Zyngier wrote:
> Hi Chanwoo,
> 
> On Mon, 29 Jun 2020 03:43:37 +0100,
> Chanwoo Choi <cw00.choi@samsung.com> wrote:
>>
>> Hi Marc,
>>
>> On 6/23/20 12:28 AM, Marc Zyngier wrote:
> 
> [...]
> 
>> It looks good to me. But, I think that it is not necessary
>> fully kernel panic log about NULL pointer. It is enoughspsp
>> just mentioning the NULL pointer issue without full kernel panic log.
> 
> I personally find the backtrace useful as it allows people with the
> same issue to trawl the kernel log and find whether it has already be
> fixed upstream. But it's only me, and I'm not attached to it.
> 
>> So, how about editing the patch description as following or others simply?
>> and we need to add 'stable@vger.kernel.org' to Cc list for applying it
>> to stable branch.
> 
> Looks good to me.
> 
>>
>>
>>   PM / devfreq: rk3399_dmc: Fix kernel oops when rockchip,pmu is absent
>>
>>     Booting a recent kernel on a rk3399-based system (nanopc-t4),
>>     equipped with a recent u-boot and ATF results in the kernel panic
>>     about NULL pointer issue.
> 
> nit: "results in a kernel panic on dereferencing a NULL pointer".
> 
>>
>>     This turns out to be due to the rk3399-dmc driver looking for
>>     an *undocumented* property (rockchip,pmu), and happily using
>>     a NULL pointer when the property isn't there.
>>
>>     Instead, make most of what was brought in with 9173c5ceb035
>>     ("PM / devfreq: rk3399_dmc: Pass ODT and auto power down parameters
>>     to TF-A.") conditioned on finding this property in the device-tree,
>>     preventing the driver from exploding.
>>
>>     Fixes: 9173c5ceb035 ("PM / devfreq: rk3399_dmc: Pass ODT and auto power down parameters to TF-A.")
>>     Signed-off-by: Marc Zyngier <maz@kernel.org>
>>     Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> 
> 
> Note that the biggest issue is still there: the driver is using an
> undocumented property, and this patch is just papering over it.
> Since I expect this property to be useful for something, it would be
> good for whoever knows what it does to document it.

Hi Marc,

You are right. We have to do two step:
1. Add missing explanation of 'rockchip,pmu' property to dt-binding document
2. If possible, add 'rockchip,pmu' property node to rk3399_dmc dt node.

When I tried to find usage example of 'rockchip,pmu' property,
I found them as following: The 'rockchip,pmu' property[2] indicates
'PMU (Power Management Unit)'. 

$ grep -rn "rockchip,pmu" arch/arm64/boot/dts/
arch/arm64/boot/dts/rockchip/px30.dtsi:1211:		rockchip,pmu = <&pmugrf>;
arch/arm64/boot/dts/rockchip/rk3399.dtsi:1909:		rockchip,pmu = <&pmugrf>;
arch/arm64/boot/dts/rockchip/rk3368.dtsi:807:		rockchip,pmu = <&pmugrf>;

[2] the description of 'rockchip,pmu' property
- https://elixir.bootlin.com/linux/v5.7.2/source/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.txt#L40


If don't receive the any reply, I'll add as following:

cwchoi00@chan-linux-pc:~/kernel/git.kernel/linux.chanwoo$ d
diff --git a/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt b/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt
index 0ec68141f85a..161e60ea874b 100644
--- a/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt
+++ b/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt
@@ -18,6 +18,8 @@ Optional properties:
                         format depends on the interrupt controller.
                         It should be a DCF interrupt. When DDR DVFS finishes
                         a DCF interrupt is triggered.
+- rockchip,pmu:                 Phandle to the syscon managing the "pmu general
+                        register files".
 
 Following properties relate to DDR timing:
 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2] PM / devfreq: rk3399_dmc: Fix kernel oops when rockchip,pmu is absent

[Enric Balletbo i Serra]

Hi Chanwoo and Marc,

On 29/6/20 13:09, Chanwoo Choi wrote:
> Hi Enric,
> 
> Could you check this issue? Your patch[1] causes this issue.
> As Marc mentioned, although rk3399-dmc.c handled 'rockchip,pmu'
> as the mandatory property, your patch[1] didn't add the 'rockchip,pmu'
> property to the documentation. 
> 

I think the problem is that the DT binding patch, for some reason, was missed
and didn't land. The patch seems to have all the required reviews and acks.

  https://patchwork.kernel.org/patch/10901593/

Sorry because I didn't notice this issue when 9173c5ceb035 landed. And thanks
for fixing the issue.

Best regards,
 Enric

> [1] 9173c5ceb035 ("PM / devfreq: rk3399_dmc: Pass ODT
> and auto power down parameters to TF-A.")
> 
> 
> On 6/29/20 5:18 PM, Marc Zyngier wrote:
>> Hi Chanwoo,
>>
>> On Mon, 29 Jun 2020 03:43:37 +0100,
>> Chanwoo Choi <cw00.choi@samsung.com> wrote:
>>>
>>> Hi Marc,
>>>
>>> On 6/23/20 12:28 AM, Marc Zyngier wrote:
>>
>> [...]
>>
>>> It looks good to me. But, I think that it is not necessary
>>> fully kernel panic log about NULL pointer. It is enoughspsp
>>> just mentioning the NULL pointer issue without full kernel panic log.
>>
>> I personally find the backtrace useful as it allows people with the
>> same issue to trawl the kernel log and find whether it has already be
>> fixed upstream. But it's only me, and I'm not attached to it.
>>
>>> So, how about editing the patch description as following or others simply?
>>> and we need to add 'stable@vger.kernel.org' to Cc list for applying it
>>> to stable branch.
>>
>> Looks good to me.
>>
>>>
>>>
>>>   PM / devfreq: rk3399_dmc: Fix kernel oops when rockchip,pmu is absent
>>>
>>>     Booting a recent kernel on a rk3399-based system (nanopc-t4),
>>>     equipped with a recent u-boot and ATF results in the kernel panic
>>>     about NULL pointer issue.
>>
>> nit: "results in a kernel panic on dereferencing a NULL pointer".
>>
>>>
>>>     This turns out to be due to the rk3399-dmc driver looking for
>>>     an *undocumented* property (rockchip,pmu), and happily using
>>>     a NULL pointer when the property isn't there.
>>>
>>>     Instead, make most of what was brought in with 9173c5ceb035
>>>     ("PM / devfreq: rk3399_dmc: Pass ODT and auto power down parameters
>>>     to TF-A.") conditioned on finding this property in the device-tree,
>>>     preventing the driver from exploding.
>>>
>>>     Fixes: 9173c5ceb035 ("PM / devfreq: rk3399_dmc: Pass ODT and auto power down parameters to TF-A.")
>>>     Signed-off-by: Marc Zyngier <maz@kernel.org>
>>>     Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>
>>
>> Note that the biggest issue is still there: the driver is using an
>> undocumented property, and this patch is just papering over it.
>> Since I expect this property to be useful for something, it would be
>> good for whoever knows what it does to document it.
> 
> Hi Marc,
> 
> You are right. We have to do two step:
> 1. Add missing explanation of 'rockchip,pmu' property to dt-binding document
> 2. If possible, add 'rockchip,pmu' property node to rk3399_dmc dt node.
> 
> When I tried to find usage example of 'rockchip,pmu' property,
> I found them as following: The 'rockchip,pmu' property[2] indicates
> 'PMU (Power Management Unit)'. 
> 
> $ grep -rn "rockchip,pmu" arch/arm64/boot/dts/
> arch/arm64/boot/dts/rockchip/px30.dtsi:1211:		rockchip,pmu = <&pmugrf>;
> arch/arm64/boot/dts/rockchip/rk3399.dtsi:1909:		rockchip,pmu = <&pmugrf>;
> arch/arm64/boot/dts/rockchip/rk3368.dtsi:807:		rockchip,pmu = <&pmugrf>;
> 
> [2] the description of 'rockchip,pmu' property
> - https://elixir.bootlin.com/linux/v5.7.2/source/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.txt#L40
> 
> 
> If don't receive the any reply, I'll add as following:
> 
> cwchoi00@chan-linux-pc:~/kernel/git.kernel/linux.chanwoo$ d
> diff --git a/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt b/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt
> index 0ec68141f85a..161e60ea874b 100644
> --- a/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt
> +++ b/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt
> @@ -18,6 +18,8 @@ Optional properties:
>                          format depends on the interrupt controller.
>                          It should be a DCF interrupt. When DDR DVFS finishes
>                          a DCF interrupt is triggered.
> +- rockchip,pmu:                 Phandle to the syscon managing the "pmu general
> +                        register files".
>  
>  Following properties relate to DDR timing:
>  
> 
> 

Re: [PATCH v2] PM / devfreq: rk3399_dmc: Fix kernel oops when rockchip,pmu is absent

[Chanwoo Choi]

Hi Enric and Mark,

On 6/29/20 8:05 PM, Enric Balletbo i Serra wrote:
> Hi Chanwoo and Marc,
> 
> On 29/6/20 13:09, Chanwoo Choi wrote:
>> Hi Enric,
>>
>> Could you check this issue? Your patch[1] causes this issue.
>> As Marc mentioned, although rk3399-dmc.c handled 'rockchip,pmu'
>> as the mandatory property, your patch[1] didn't add the 'rockchip,pmu'
>> property to the documentation. 
>>
> 
> I think the problem is that the DT binding patch, for some reason, was missed
> and didn't land. The patch seems to have all the required reviews and acks.
> 
>   https://patchwork.kernel.org/patch/10901593/
> 
> Sorry because I didn't notice this issue when 9173c5ceb035 landed. And thanks
> for fixing the issue.

If the 'rockchip,pmu' propery is mandatory, instead of Mark's patch,
we better to require the merge of patch[1] to DT maintainer.

[1] https://patchwork.kernel.org/patch/10901593/

> 
> Best regards,
>  Enric
> 
>> [1] 9173c5ceb035 ("PM / devfreq: rk3399_dmc: Pass ODT
>> and auto power down parameters to TF-A.")
>>
>>
>> On 6/29/20 5:18 PM, Marc Zyngier wrote:
>>> Hi Chanwoo,
>>>
>>> On Mon, 29 Jun 2020 03:43:37 +0100,
>>> Chanwoo Choi <cw00.choi@samsung.com> wrote:
>>>>
>>>> Hi Marc,
>>>>
>>>> On 6/23/20 12:28 AM, Marc Zyngier wrote:
>>>
>>> [...]
>>>
>>>> It looks good to me. But, I think that it is not necessary
>>>> fully kernel panic log about NULL pointer. It is enoughspsp
>>>> just mentioning the NULL pointer issue without full kernel panic log.
>>>
>>> I personally find the backtrace useful as it allows people with the
>>> same issue to trawl the kernel log and find whether it has already be
>>> fixed upstream. But it's only me, and I'm not attached to it.
>>>
>>>> So, how about editing the patch description as following or others simply?
>>>> and we need to add 'stable@vger.kernel.org' to Cc list for applying it
>>>> to stable branch.
>>>
>>> Looks good to me.
>>>
>>>>
>>>>
>>>>   PM / devfreq: rk3399_dmc: Fix kernel oops when rockchip,pmu is absent
>>>>
>>>>     Booting a recent kernel on a rk3399-based system (nanopc-t4),
>>>>     equipped with a recent u-boot and ATF results in the kernel panic
>>>>     about NULL pointer issue.
>>>
>>> nit: "results in a kernel panic on dereferencing a NULL pointer".
>>>
>>>>
>>>>     This turns out to be due to the rk3399-dmc driver looking for
>>>>     an *undocumented* property (rockchip,pmu), and happily using
>>>>     a NULL pointer when the property isn't there.
>>>>
>>>>     Instead, make most of what was brought in with 9173c5ceb035
>>>>     ("PM / devfreq: rk3399_dmc: Pass ODT and auto power down parameters
>>>>     to TF-A.") conditioned on finding this property in the device-tree,
>>>>     preventing the driver from exploding.
>>>>
>>>>     Fixes: 9173c5ceb035 ("PM / devfreq: rk3399_dmc: Pass ODT and auto power down parameters to TF-A.")
>>>>     Signed-off-by: Marc Zyngier <maz@kernel.org>
>>>>     Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>
>>>
>>> Note that the biggest issue is still there: the driver is using an
>>> undocumented property, and this patch is just papering over it.
>>> Since I expect this property to be useful for something, it would be
>>> good for whoever knows what it does to document it.
>>
>> Hi Marc,
>>
>> You are right. We have to do two step:
>> 1. Add missing explanation of 'rockchip,pmu' property to dt-binding document
>> 2. If possible, add 'rockchip,pmu' property node to rk3399_dmc dt node.
>>
>> When I tried to find usage example of 'rockchip,pmu' property,
>> I found them as following: The 'rockchip,pmu' property[2] indicates
>> 'PMU (Power Management Unit)'. 
>>
>> $ grep -rn "rockchip,pmu" arch/arm64/boot/dts/
>> arch/arm64/boot/dts/rockchip/px30.dtsi:1211:		rockchip,pmu = <&pmugrf>;
>> arch/arm64/boot/dts/rockchip/rk3399.dtsi:1909:		rockchip,pmu = <&pmugrf>;
>> arch/arm64/boot/dts/rockchip/rk3368.dtsi:807:		rockchip,pmu = <&pmugrf>;
>>
>> [2] the description of 'rockchip,pmu' property
>> - https://protect2.fireeye.com/url?k=e55f0ba3-b8384f85-e55e80ec-0cc47a31384a-d9c5f6b28aba9be6&q=1&u=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv5.7.2%2Fsource%2FDocumentation%2Fdevicetree%2Fbindings%2Fpinctrl%2Frockchip%2Cpinctrl.txt%23L40
>>
>>
>> If don't receive the any reply, I'll add as following:
>>
>> cwchoi00@chan-linux-pc:~/kernel/git.kernel/linux.chanwoo$ d
>> diff --git a/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt b/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt
>> index 0ec68141f85a..161e60ea874b 100644
>> --- a/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt
>> +++ b/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt
>> @@ -18,6 +18,8 @@ Optional properties:
>>                          format depends on the interrupt controller.
>>                          It should be a DCF interrupt. When DDR DVFS finishes
>>                          a DCF interrupt is triggered.
>> +- rockchip,pmu:                 Phandle to the syscon managing the "pmu general
>> +                        register files".
>>  
>>  Following properties relate to DDR timing:
>>  
>>
>>
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2] PM / devfreq: rk3399_dmc: Fix kernel oops when rockchip,pmu is absent

[Enric Balletbo i Serra]

Hi Chanwoo,

On 29/6/20 13:29, Chanwoo Choi wrote:
> Hi Enric and Mark,
> 
> On 6/29/20 8:05 PM, Enric Balletbo i Serra wrote:
>> Hi Chanwoo and Marc,
>>
>> On 29/6/20 13:09, Chanwoo Choi wrote:
>>> Hi Enric,
>>>
>>> Could you check this issue? Your patch[1] causes this issue.
>>> As Marc mentioned, although rk3399-dmc.c handled 'rockchip,pmu'
>>> as the mandatory property, your patch[1] didn't add the 'rockchip,pmu'
>>> property to the documentation. 
>>>
>>
>> I think the problem is that the DT binding patch, for some reason, was missed
>> and didn't land. The patch seems to have all the required reviews and acks.
>>
>>   https://patchwork.kernel.org/patch/10901593/
>>
>> Sorry because I didn't notice this issue when 9173c5ceb035 landed. And thanks
>> for fixing the issue.
> 
> If the 'rockchip,pmu' propery is mandatory, instead of Mark's patch,
> we better to require the merge of patch[1] to DT maintainer.
> 
> [1] https://patchwork.kernel.org/patch/10901593/
> 

Give me some time to double check, because I think that at this point, is needed
on some devices with old firmware but not now. It's been a while since I worked
on this, but I suspect that being optional is the right way.

Maybe Heiko, who IIRC worked on TF-A has a more clear thought on this?

Thanks,
 Enric

>>
>> Best regards,
>>  Enric
>>
>>> [1] 9173c5ceb035 ("PM / devfreq: rk3399_dmc: Pass ODT
>>> and auto power down parameters to TF-A.")
>>>
>>>
>>> On 6/29/20 5:18 PM, Marc Zyngier wrote:
>>>> Hi Chanwoo,
>>>>
>>>> On Mon, 29 Jun 2020 03:43:37 +0100,
>>>> Chanwoo Choi <cw00.choi@samsung.com> wrote:
>>>>>
>>>>> Hi Marc,
>>>>>
>>>>> On 6/23/20 12:28 AM, Marc Zyngier wrote:
>>>>
>>>> [...]
>>>>
>>>>> It looks good to me. But, I think that it is not necessary
>>>>> fully kernel panic log about NULL pointer. It is enoughspsp
>>>>> just mentioning the NULL pointer issue without full kernel panic log.
>>>>
>>>> I personally find the backtrace useful as it allows people with the
>>>> same issue to trawl the kernel log and find whether it has already be
>>>> fixed upstream. But it's only me, and I'm not attached to it.
>>>>
>>>>> So, how about editing the patch description as following or others simply?
>>>>> and we need to add 'stable@vger.kernel.org' to Cc list for applying it
>>>>> to stable branch.
>>>>
>>>> Looks good to me.
>>>>
>>>>>
>>>>>
>>>>>   PM / devfreq: rk3399_dmc: Fix kernel oops when rockchip,pmu is absent
>>>>>
>>>>>     Booting a recent kernel on a rk3399-based system (nanopc-t4),
>>>>>     equipped with a recent u-boot and ATF results in the kernel panic
>>>>>     about NULL pointer issue.
>>>>
>>>> nit: "results in a kernel panic on dereferencing a NULL pointer".
>>>>
>>>>>
>>>>>     This turns out to be due to the rk3399-dmc driver looking for
>>>>>     an *undocumented* property (rockchip,pmu), and happily using
>>>>>     a NULL pointer when the property isn't there.
>>>>>
>>>>>     Instead, make most of what was brought in with 9173c5ceb035
>>>>>     ("PM / devfreq: rk3399_dmc: Pass ODT and auto power down parameters
>>>>>     to TF-A.") conditioned on finding this property in the device-tree,
>>>>>     preventing the driver from exploding.
>>>>>
>>>>>     Fixes: 9173c5ceb035 ("PM / devfreq: rk3399_dmc: Pass ODT and auto power down parameters to TF-A.")
>>>>>     Signed-off-by: Marc Zyngier <maz@kernel.org>
>>>>>     Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>>
>>>>
>>>> Note that the biggest issue is still there: the driver is using an
>>>> undocumented property, and this patch is just papering over it.
>>>> Since I expect this property to be useful for something, it would be
>>>> good for whoever knows what it does to document it.
>>>
>>> Hi Marc,
>>>
>>> You are right. We have to do two step:
>>> 1. Add missing explanation of 'rockchip,pmu' property to dt-binding document
>>> 2. If possible, add 'rockchip,pmu' property node to rk3399_dmc dt node.
>>>
>>> When I tried to find usage example of 'rockchip,pmu' property,
>>> I found them as following: The 'rockchip,pmu' property[2] indicates
>>> 'PMU (Power Management Unit)'. 
>>>
>>> $ grep -rn "rockchip,pmu" arch/arm64/boot/dts/
>>> arch/arm64/boot/dts/rockchip/px30.dtsi:1211:		rockchip,pmu = <&pmugrf>;
>>> arch/arm64/boot/dts/rockchip/rk3399.dtsi:1909:		rockchip,pmu = <&pmugrf>;
>>> arch/arm64/boot/dts/rockchip/rk3368.dtsi:807:		rockchip,pmu = <&pmugrf>;
>>>
>>> [2] the description of 'rockchip,pmu' property
>>> - https://protect2.fireeye.com/url?k=e55f0ba3-b8384f85-e55e80ec-0cc47a31384a-d9c5f6b28aba9be6&q=1&u=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv5.7.2%2Fsource%2FDocumentation%2Fdevicetree%2Fbindings%2Fpinctrl%2Frockchip%2Cpinctrl.txt%23L40
>>>
>>>
>>> If don't receive the any reply, I'll add as following:
>>>
>>> cwchoi00@chan-linux-pc:~/kernel/git.kernel/linux.chanwoo$ d
>>> diff --git a/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt b/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt
>>> index 0ec68141f85a..161e60ea874b 100644
>>> --- a/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt
>>> +++ b/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt
>>> @@ -18,6 +18,8 @@ Optional properties:
>>>                          format depends on the interrupt controller.
>>>                          It should be a DCF interrupt. When DDR DVFS finishes
>>>                          a DCF interrupt is triggered.
>>> +- rockchip,pmu:                 Phandle to the syscon managing the "pmu general
>>> +                        register files".
>>>  
>>>  Following properties relate to DDR timing:
>>>  
>>>
>>>
>>
>>
> 
> 

Re: [PATCH v2] PM / devfreq: rk3399_dmc: Fix kernel oops when rockchip,pmu is absent

[Chanwoo Choi]

Hi Enric,

On 6/29/20 8:26 PM, Enric Balletbo i Serra wrote:
> Hi Chanwoo,
> 
> On 29/6/20 13:29, Chanwoo Choi wrote:
>> Hi Enric and Mark,
>>
>> On 6/29/20 8:05 PM, Enric Balletbo i Serra wrote:
>>> Hi Chanwoo and Marc,
>>>
>>> On 29/6/20 13:09, Chanwoo Choi wrote:
>>>> Hi Enric,
>>>>
>>>> Could you check this issue? Your patch[1] causes this issue.
>>>> As Marc mentioned, although rk3399-dmc.c handled 'rockchip,pmu'
>>>> as the mandatory property, your patch[1] didn't add the 'rockchip,pmu'
>>>> property to the documentation. 
>>>>
>>>
>>> I think the problem is that the DT binding patch, for some reason, was missed
>>> and didn't land. The patch seems to have all the required reviews and acks.
>>>
>>>   https://patchwork.kernel.org/patch/10901593/
>>>
>>> Sorry because I didn't notice this issue when 9173c5ceb035 landed. And thanks
>>> for fixing the issue.
>>
>> If the 'rockchip,pmu' propery is mandatory, instead of Mark's patch,
>> we better to require the merge of patch[1] to DT maintainer.
>>
>> [1] https://patchwork.kernel.org/patch/10901593/
>>
> 
> Give me some time to double check, because I think that at this point, is needed
> on some devices with old firmware but not now. It's been a while since I worked
> on this, but I suspect that being optional is the right way.

OK. Thanks for your reply.

> 
> Maybe Heiko, who IIRC worked on TF-A has a more clear thought on this?
> 
> Thanks,
>  Enric
> 
>>>
>>> Best regards,
>>>  Enric
>>>
>>>> [1] 9173c5ceb035 ("PM / devfreq: rk3399_dmc: Pass ODT
>>>> and auto power down parameters to TF-A.")
>>>>
>>>>
>>>> On 6/29/20 5:18 PM, Marc Zyngier wrote:
>>>>> Hi Chanwoo,
>>>>>
>>>>> On Mon, 29 Jun 2020 03:43:37 +0100,
>>>>> Chanwoo Choi <cw00.choi@samsung.com> wrote:
>>>>>>
>>>>>> Hi Marc,
>>>>>>
>>>>>> On 6/23/20 12:28 AM, Marc Zyngier wrote:
>>>>>
>>>>> [...]
>>>>>
>>>>>> It looks good to me. But, I think that it is not necessary
>>>>>> fully kernel panic log about NULL pointer. It is enoughspsp
>>>>>> just mentioning the NULL pointer issue without full kernel panic log.
>>>>>
>>>>> I personally find the backtrace useful as it allows people with the
>>>>> same issue to trawl the kernel log and find whether it has already be
>>>>> fixed upstream. But it's only me, and I'm not attached to it.
>>>>>
>>>>>> So, how about editing the patch description as following or others simply?
>>>>>> and we need to add 'stable@vger.kernel.org' to Cc list for applying it
>>>>>> to stable branch.
>>>>>
>>>>> Looks good to me.
>>>>>
>>>>>>
>>>>>>
>>>>>>   PM / devfreq: rk3399_dmc: Fix kernel oops when rockchip,pmu is absent
>>>>>>
>>>>>>     Booting a recent kernel on a rk3399-based system (nanopc-t4),
>>>>>>     equipped with a recent u-boot and ATF results in the kernel panic
>>>>>>     about NULL pointer issue.
>>>>>
>>>>> nit: "results in a kernel panic on dereferencing a NULL pointer".
>>>>>
>>>>>>
>>>>>>     This turns out to be due to the rk3399-dmc driver looking for
>>>>>>     an *undocumented* property (rockchip,pmu), and happily using
>>>>>>     a NULL pointer when the property isn't there.
>>>>>>
>>>>>>     Instead, make most of what was brought in with 9173c5ceb035
>>>>>>     ("PM / devfreq: rk3399_dmc: Pass ODT and auto power down parameters
>>>>>>     to TF-A.") conditioned on finding this property in the device-tree,
>>>>>>     preventing the driver from exploding.
>>>>>>
>>>>>>     Fixes: 9173c5ceb035 ("PM / devfreq: rk3399_dmc: Pass ODT and auto power down parameters to TF-A.")
>>>>>>     Signed-off-by: Marc Zyngier <maz@kernel.org>
>>>>>>     Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>>>
>>>>>
>>>>> Note that the biggest issue is still there: the driver is using an
>>>>> undocumented property, and this patch is just papering over it.
>>>>> Since I expect this property to be useful for something, it would be
>>>>> good for whoever knows what it does to document it.
>>>>
>>>> Hi Marc,
>>>>
>>>> You are right. We have to do two step:
>>>> 1. Add missing explanation of 'rockchip,pmu' property to dt-binding document
>>>> 2. If possible, add 'rockchip,pmu' property node to rk3399_dmc dt node.
>>>>
>>>> When I tried to find usage example of 'rockchip,pmu' property,
>>>> I found them as following: The 'rockchip,pmu' property[2] indicates
>>>> 'PMU (Power Management Unit)'. 
>>>>
>>>> $ grep -rn "rockchip,pmu" arch/arm64/boot/dts/
>>>> arch/arm64/boot/dts/rockchip/px30.dtsi:1211:		rockchip,pmu = <&pmugrf>;
>>>> arch/arm64/boot/dts/rockchip/rk3399.dtsi:1909:		rockchip,pmu = <&pmugrf>;
>>>> arch/arm64/boot/dts/rockchip/rk3368.dtsi:807:		rockchip,pmu = <&pmugrf>;
>>>>
>>>> [2] the description of 'rockchip,pmu' property
>>>> - https://protect2.fireeye.com/url?k=e55f0ba3-b8384f85-e55e80ec-0cc47a31384a-d9c5f6b28aba9be6&q=1&u=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv5.7.2%2Fsource%2FDocumentation%2Fdevicetree%2Fbindings%2Fpinctrl%2Frockchip%2Cpinctrl.txt%23L40
>>>>
>>>>
>>>> If don't receive the any reply, I'll add as following:
>>>>
>>>> cwchoi00@chan-linux-pc:~/kernel/git.kernel/linux.chanwoo$ d
>>>> diff --git a/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt b/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt
>>>> index 0ec68141f85a..161e60ea874b 100644
>>>> --- a/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt
>>>> +++ b/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt
>>>> @@ -18,6 +18,8 @@ Optional properties:
>>>>                          format depends on the interrupt controller.
>>>>                          It should be a DCF interrupt. When DDR DVFS finishes
>>>>                          a DCF interrupt is triggered.
>>>> +- rockchip,pmu:                 Phandle to the syscon managing the "pmu general
>>>> +                        register files".
>>>>  
>>>>  Following properties relate to DDR timing:
>>>>  
>>>>
>>>>
>>>
>>>
>>
>>
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2] PM / devfreq: rk3399_dmc: Fix kernel oops when rockchip,pmu is absent

[Marc Zyngier]

On 2020-06-29 12:29, Chanwoo Choi wrote:
> Hi Enric and Mark,
> 
> On 6/29/20 8:05 PM, Enric Balletbo i Serra wrote:
>> Hi Chanwoo and Marc,
>> 
>> On 29/6/20 13:09, Chanwoo Choi wrote:
>>> Hi Enric,
>>> 
>>> Could you check this issue? Your patch[1] causes this issue.
>>> As Marc mentioned, although rk3399-dmc.c handled 'rockchip,pmu'
>>> as the mandatory property, your patch[1] didn't add the 
>>> 'rockchip,pmu'
>>> property to the documentation.
>>> 
>> 
>> I think the problem is that the DT binding patch, for some reason, was 
>> missed
>> and didn't land. The patch seems to have all the required reviews and 
>> acks.
>> 
>>   https://patchwork.kernel.org/patch/10901593/
>> 
>> Sorry because I didn't notice this issue when 9173c5ceb035 landed. And 
>> thanks
>> for fixing the issue.
> 
> If the 'rockchip,pmu' propery is mandatory, instead of Mark's patch,
> we better to require the merge of patch[1] to DT maintainer.

It is way too late. Firmware exists (mainline u-boot, for one) that
do not expose the new property, and you can't demand that people
upgrade. This is an ABI bug, and we now have to live with it.

So, yes to fixing the DT, and no to *only* fixing the DT.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2] PM / devfreq: rk3399_dmc: Fix kernel oops when rockchip,pmu is absent

[Chanwoo Choi]

Hi Marc,
Hi Marc,

On 6/29/20 10:22 PM, Marc Zyngier wrote:
> On 2020-06-29 12:29, Chanwoo Choi wrote:
>> Hi Enric and Mark,
>>
>> On 6/29/20 8:05 PM, Enric Balletbo i Serra wrote:
>>> Hi Chanwoo and Marc,
>>>
>>> On 29/6/20 13:09, Chanwoo Choi wrote:
>>>> Hi Enric,
>>>>
>>>> Could you check this issue? Your patch[1] causes this issue.
>>>> As Marc mentioned, although rk3399-dmc.c handled 'rockchip,pmu'
>>>> as the mandatory property, your patch[1] didn't add the 'rockchip,pmu'
>>>> property to the documentation.
>>>>
>>>
>>> I think the problem is that the DT binding patch, for some reason, was missed
>>> and didn't land. The patch seems to have all the required reviews and acks.
>>>
>>>   https://patchwork.kernel.org/patch/10901593/
>>>
>>> Sorry because I didn't notice this issue when 9173c5ceb035 landed. And thanks
>>> for fixing the issue.
>>
>> If the 'rockchip,pmu' propery is mandatory, instead of Mark's patch,
>> we better to require the merge of patch[1] to DT maintainer.
> 
> It is way too late. Firmware exists (mainline u-boot, for one) that
> do not expose the new property, and you can't demand that people
> upgrade. This is an ABI bug, and we now have to live with it.

As you commented, it is proper that rk3399-dmc.c treats 'rockchip,pmu'
property as optional. Could you send v3 with edited patch descritpion
and adding stable mailing list to Cc?

> 
> So, yes to fixing the DT, and no to *only* fixing the DT.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics