Re: [PATCH 2/2] ram: k3-ddrss: Fix register definitions

[Dave Gerlach <d-gerlach@ti.com>]

Hi,
On 3/22/22 07:11, Dominic Rath wrote:
> The K3 DDRSS used by AM64x and J721e (and probably J7200 in the future)
> used to define only two register ranges: one labeled "cfg" that was
> internally referenced as "DDRSS wrapper", and the LPDDR4 FSP handshake
> registers in the CTRL_MMR register space, labeled "ctrl_mmr_lp4".
> 
> The address specified as the "cfg" register space was actually the
> DDR controller's configuration registers, not the wrapper.
> 
> The "cfg" registers were mostly used correctly as the address passed
> to Cadence code that actually configured the DDR controller and
> subsystems, but the code also attempted to use this register space when
> it actually wanted to write the DDRSS_V2A_CTL_REG and DDRSS_ECC_CTRL_REG
> registers.
> 
> This patch adds a third register property "ddrss", uses that for the
> DDRSS wrapper, and continues to use the "cfg" property for the actual
> controller configuration registers.
> 
> The "cfg" register space was also configured too small at 16 KB.
> The TRMs say the range is 32KB, and the PHY registers actually start
> at the 16KB offset.
> 
> Signed-off-by: Dominic Rath <rath@ibv-augsburg.net>
> ---

I just introduced this same change last week here to add ecc support for
am64x:

https://lore.kernel.org/u-boot/20220317170346.31162-6-d-gerlach@ti.com/
https://lore.kernel.org/u-boot/20220317170346.31162-8-d-gerlach@ti.com/

Major difference in approach is that I made this an optional region, as
J721e does not require it.

Regards,
Dave


>   arch/arm/dts/k3-am64-ddr.dtsi   |  5 +++--
>   arch/arm/dts/k3-j721e-ddr.dtsi  |  5 +++--
>   drivers/ram/k3-ddrss/k3-ddrss.c | 12 ++++++++++--
>   3 files changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm/dts/k3-am64-ddr.dtsi b/arch/arm/dts/k3-am64-ddr.dtsi
> index 026a547f0e..967bd55e75 100644
> --- a/arch/arm/dts/k3-am64-ddr.dtsi
> +++ b/arch/arm/dts/k3-am64-ddr.dtsi
> @@ -6,9 +6,10 @@
>   / {
>   	memorycontroller: memorycontroller@f300000 {
>   		compatible = "ti,am64-ddrss";
> -		reg = <0x00 0x0f308000 0x00 0x4000>,
> +		reg = <0x00 0x0f300000 0x00 0x200>,
> +		      <0x00 0x0f308000 0x00 0x8000>,
>   		      <0x00 0x43014000 0x00 0x100>;
> -		reg-names = "cfg", "ctrl_mmr_lp4";
> +		reg-names = "ddrss", "cfg", "ctrl_mmr_lp4";
>   		power-domains = <&k3_pds 138 TI_SCI_PD_SHARED>,
>   			<&k3_pds 55 TI_SCI_PD_SHARED>;
>   		clocks = <&k3_clks 138 0>, <&k3_clks 16 4>;
> diff --git a/arch/arm/dts/k3-j721e-ddr.dtsi b/arch/arm/dts/k3-j721e-ddr.dtsi
> index 21d63802a5..8f1b792849 100644
> --- a/arch/arm/dts/k3-j721e-ddr.dtsi
> +++ b/arch/arm/dts/k3-j721e-ddr.dtsi
> @@ -6,9 +6,10 @@
>   / {
>   	memorycontroller: memorycontroller@0298e000 {
>   		compatible = "ti,j721e-ddrss";
> -		reg = <0x0 0x02990000 0x0 0x4000>,
> +		reg = <0x0 0x02980000 0x0 0x200>,
> +		      <0x0 0x02990000 0x0 0x8000>,
>   		      <0x0 0x0114000 0x0 0x100>;
> -		reg-names = "cfg", "ctrl_mmr_lp4";
> +		reg-names = "ddrss", "cfg", "ctrl_mmr_lp4";
>   		power-domains = <&k3_pds 47 TI_SCI_PD_SHARED>,
>   			<&k3_pds 90 TI_SCI_PD_SHARED>;
>   		clocks = <&k3_clks 47 2>, <&k3_clks 30 9>;
> diff --git a/drivers/ram/k3-ddrss/k3-ddrss.c 
> b/drivers/ram/k3-ddrss/k3-ddrss.c
> index 261ba64506..7e2a4c5811 100644
> --- a/drivers/ram/k3-ddrss/k3-ddrss.c
> +++ b/drivers/ram/k3-ddrss/k3-ddrss.c
> @@ -105,6 +105,7 @@ struct k3_msmc {
>   struct k3_ddrss_desc {
>   	struct udevice *dev;
>   	void __iomem *ddrss_ss_cfg;
> +	void __iomem *ddrss_ctrl_cfg;
>   	void __iomem *ddrss_ctrl_mmr;
>   	struct power_domain ddrcfg_pwrdmn;
>   	struct power_domain ddrdata_pwrdmn;
> @@ -314,13 +315,20 @@ static int k3_ddrss_ofdata_to_priv(struct udevice 
> *dev)
>    	debug("%s(dev=%p)\n", __func__, dev);
>   -	reg = dev_read_addr_name(dev, "cfg");
> +	reg = dev_read_addr_name(dev, "ddrss");
>   	if (reg == FDT_ADDR_T_NONE) {
>   		dev_err(dev, "No reg property for DDRSS wrapper logic\n");
>   		return -EINVAL;
>   	}
>   	ddrss->ddrss_ss_cfg = (void *)reg;
>   +	reg = dev_read_addr_name(dev, "cfg");
> +	if (reg == FDT_ADDR_T_NONE) {
> +		dev_err(dev, "No reg property for controller registers\n");
> +		return -EINVAL;
> +	}
> +	ddrss->ddrss_ctrl_cfg = (void *)reg;
> +
>   	reg = dev_read_addr_name(dev, "ctrl_mmr_lp4");
>   	if (reg == FDT_ADDR_T_NONE) {
>   		dev_err(dev, "No reg property for CTRL MMR\n");
> @@ -403,7 +411,7 @@ void k3_lpddr4_init(struct k3_ddrss_desc *ddrss)
>   		hang();
>   	}
>   -	config->ctlbase = (struct lpddr4_ctlregs_s *)ddrss->ddrss_ss_cfg;
> +	config->ctlbase = (struct lpddr4_ctlregs_s *)ddrss->ddrss_ctrl_cfg;
>   	config->infohandler = (lpddr4_infocallback) k3_lpddr4_info_handler;
>    	status = driverdt->init(pd, config);