| Message ID | 20200331234807.432637-2-marex@denx.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/4,RFC] ARM: stm32: Implement board coding on AV96 | expand |
Hi Marek, > From: Marek Vasut <marex at denx.de> > Sent: mercredi 1 avril 2020 01:48 > > Add support for multiple DRAM configuration subnodes, while retaining the > support for a single flat DRAM configuration node. This is useful on systems > which can be manufactured in multiple configurations and where the DRAM > configuration can be determined at runtime. > > The code is augmented by a function which can be overridden on board level, > allowing a match on the configuration node name, very much like the fitImage > configuration node name matching works. The default match is on the single top- > level DRAM configuration, if matching on subnodes is required, then this > board_stm32mp1_ddr_config_name_match() must be overridden. > > Signed-off-by: Marek Vasut <marex at denx.de> > Cc: Manivannan Sadhasivam <manivannan.sadhasivam at linaro.org> > Cc: Patrick Delaunay <patrick.delaunay at st.com> > Cc: Patrice Chotard <patrice.chotard at st.com> > --- > drivers/ram/stm32mp1/stm32mp1_ram.c | 36 +++++++++++++++++++++++++--- > - > 1 file changed, 32 insertions(+), 4 deletions(-) > > diff --git a/drivers/ram/stm32mp1/stm32mp1_ram.c > b/drivers/ram/stm32mp1/stm32mp1_ram.c > index eb78f1198d..d6821eacd7 100644 > --- a/drivers/ram/stm32mp1/stm32mp1_ram.c > +++ b/drivers/ram/stm32mp1/stm32mp1_ram.c > @@ -57,6 +57,33 @@ int stm32mp1_ddr_clk_enable(struct ddr_info *priv, > uint32_t mem_speed) > return 0; > } > > +__weak int board_stm32mp1_ddr_config_name_match(struct udevice *dev, > + const char *name) > +{ > + return 0; /* Always match */ > +} > + > +static ofnode stm32mp1_ddr_get_ofnode(struct udevice *dev) { > + const char *name; > + ofnode node; > + > + node = dev_ofnode(dev); > + name = ofnode_get_name(node); > + if (!board_stm32mp1_ddr_config_name_match(dev, name)) > + return node; Compare with name of the node or with name of DDR configuration ? For me " st,mem-name" is same than "description" in FIT config. name = ofnode_read_string(node, "st,mem-name"); if (name && !board_stm32mp1_ddr_config_name_match(dev, name)) return node; > + > + dev_for_each_subnode(node, dev) { > + name = ofnode_get_name(node); > + > + if (!board_stm32mp1_ddr_config_name_match(dev, name)) > + return node; > + } > + > + return ofnode_null(); > +} > + > static __maybe_unused int stm32mp1_ddr_setup(struct udevice *dev) { > struct ddr_info *priv = dev_get_priv(dev); @@ -64,6 +91,7 @@ static > __maybe_unused int stm32mp1_ddr_setup(struct udevice *dev) > unsigned int idx; > struct clk axidcg; > struct stm32mp1_ddr_config config; > + ofnode node = stm32mp1_ddr_get_ofnode(dev); > > #define PARAM(x, y) \ > { x,\ > @@ -87,9 +115,9 @@ static __maybe_unused int stm32mp1_ddr_setup(struct > udevice *dev) > PHY_PARAM(cal) > }; > > - config.info.speed = dev_read_u32_default(dev, "st,mem-speed", 0); > - config.info.size = dev_read_u32_default(dev, "st,mem-size", 0); > - config.info.name = dev_read_string(dev, "st,mem-name"); > + config.info.speed = ofnode_read_u32_default(node, "st,mem-speed", 0); > + config.info.size = ofnode_read_u32_default(node, "st,mem-size", 0); > + config.info.name = ofnode_read_string(node, "st,mem-name"); > if (!config.info.name) { > debug("%s: no st,mem-name\n", __func__); > return -EINVAL; > @@ -97,7 +125,7 @@ static __maybe_unused int stm32mp1_ddr_setup(struct > udevice *dev) > printf("RAM: %s\n", config.info.name); > > for (idx = 0; idx < ARRAY_SIZE(param); idx++) { > - ret = dev_read_u32_array(dev, param[idx].name, > + ret = ofnode_read_u32_array(node, param[idx].name, > (void *)((u32)&config + > param[idx].offset), > param[idx].size); > -- > 2.25.1
On 4/7/20 3:04 PM, Patrick DELAUNAY wrote: > Hi Marek, Hi, [...] >> +__weak int board_stm32mp1_ddr_config_name_match(struct udevice *dev, >> + const char *name) >> +{ >> + return 0; /* Always match */ >> +} >> + >> +static ofnode stm32mp1_ddr_get_ofnode(struct udevice *dev) { >> + const char *name; >> + ofnode node; >> + >> + node = dev_ofnode(dev); >> + name = ofnode_get_name(node); >> + if (!board_stm32mp1_ddr_config_name_match(dev, name)) >> + return node; > > Compare with name of the node or with name of DDR configuration ? > > For me " st,mem-name" is same than "description" in FIT config. > > name = ofnode_read_string(node, "st,mem-name"); > > if (name && !board_stm32mp1_ddr_config_name_match(dev, name)) > return node; st,mem-name contains the version string, which makes it not very usable, see: arch/arm/dts/stm32mp15-ddr3-1x4Gb-1066-binG.dtsi:#define DDR_MEM_NAME "DDR3-1066/888 bin G 1x4Gb 533MHz v1.45" arch/arm/dts/stm32mp15-ddr3-2x4Gb-1066-binG.dtsi:#define DDR_MEM_NAME "DDR3-1066/888 bin G 2x4Gb 533MHz v1.45" That "v1.45" part. [...]
Hi Marek, > From: Marek Vasut <marex at denx.de> > Sent: mardi 7 avril 2020 21:58 > > On 4/7/20 3:04 PM, Patrick DELAUNAY wrote: > > Hi Marek, > > Hi, > > [...] > > >> +__weak int board_stm32mp1_ddr_config_name_match(struct udevice *dev, > >> + const char *name) > >> +{ > >> + return 0; /* Always match */ > >> +} > >> + > >> +static ofnode stm32mp1_ddr_get_ofnode(struct udevice *dev) { > >> + const char *name; > >> + ofnode node; > >> + > >> + node = dev_ofnode(dev); > >> + name = ofnode_get_name(node); > >> + if (!board_stm32mp1_ddr_config_name_match(dev, name)) > >> + return node; > > > > Compare with name of the node or with name of DDR configuration ? > > > > For me " st,mem-name" is same than "description" in FIT config. > > > > name = ofnode_read_string(node, "st,mem-name"); > > > > if (name && !board_stm32mp1_ddr_config_name_match(dev, name)) > > return node; > > st,mem-name contains the version string, which makes it not very usable, > see: > > arch/arm/dts/stm32mp15-ddr3-1x4Gb-1066-binG.dtsi:#define DDR_MEM_NAME > "DDR3-1066/888 bin G 1x4Gb 533MHz v1.45" > arch/arm/dts/stm32mp15-ddr3-2x4Gb-1066-binG.dtsi:#define DDR_MEM_NAME > "DDR3-1066/888 bin G 2x4Gb 533MHz v1.45" > > That "v1.45" part. It is the version of first internal tools used to generated the ddr file , which define register values according type, timing , size, frequency. I kept it with when I upstream the file (for U-Boot and TF-A) but it was a bad idea. I align these 2 files with the files generated by the official tools = CubeMX and this version indication disappear (but DDR_MEM_NAME change) in http://patchwork.ozlabs.org/patch/1264835/ [14/16] ARM: dts: stm32mp15: use DDR3 files generated by STM32CubeMX NB: You can also compare reg if you are OK with my proposal (config at 2 / config at 3) Patrick > [...]
On 4/8/20 12:17 PM, Patrick DELAUNAY wrote: > Hi Marek, Hi, >> From: Marek Vasut <marex at denx.de> >> Sent: mardi 7 avril 2020 21:58 >> >> On 4/7/20 3:04 PM, Patrick DELAUNAY wrote: >>> Hi Marek, >> >> Hi, >> >> [...] >> >>>> +__weak int board_stm32mp1_ddr_config_name_match(struct udevice *dev, >>>> + const char *name) >>>> +{ >>>> + return 0; /* Always match */ >>>> +} >>>> + >>>> +static ofnode stm32mp1_ddr_get_ofnode(struct udevice *dev) { >>>> + const char *name; >>>> + ofnode node; >>>> + >>>> + node = dev_ofnode(dev); >>>> + name = ofnode_get_name(node); >>>> + if (!board_stm32mp1_ddr_config_name_match(dev, name)) >>>> + return node; >>> >>> Compare with name of the node or with name of DDR configuration ? >>> >>> For me " st,mem-name" is same than "description" in FIT config. >>> >>> name = ofnode_read_string(node, "st,mem-name"); >>> >>> if (name && !board_stm32mp1_ddr_config_name_match(dev, name)) >>> return node; >> >> st,mem-name contains the version string, which makes it not very usable, >> see: >> >> arch/arm/dts/stm32mp15-ddr3-1x4Gb-1066-binG.dtsi:#define DDR_MEM_NAME >> "DDR3-1066/888 bin G 1x4Gb 533MHz v1.45" >> arch/arm/dts/stm32mp15-ddr3-2x4Gb-1066-binG.dtsi:#define DDR_MEM_NAME >> "DDR3-1066/888 bin G 2x4Gb 533MHz v1.45" >> >> That "v1.45" part. > > It is the version of first internal tools used to generated the ddr file , which define register > values according type, timing , size, frequency. > I kept it with when I upstream the file (for U-Boot and TF-A) but it was a bad idea. No, that's a good idea, at least we know which version is used. That could be important. > I align these 2 files with the files generated by the official tools = CubeMX > and this version indication disappear (but DDR_MEM_NAME change) > in http://patchwork.ozlabs.org/patch/1264835/ > > [14/16] ARM: dts: stm32mp15: use DDR3 files generated by STM32CubeMX > > NB: You can also compare reg if you are OK with my proposal (config at 2 / config at 3) That is absolutely error-prone. We need some unique unambiguous identifier, maybe a compatible string for the memory config ?
diff --git a/drivers/ram/stm32mp1/stm32mp1_ram.c b/drivers/ram/stm32mp1/stm32mp1_ram.c index eb78f1198d..d6821eacd7 100644 --- a/drivers/ram/stm32mp1/stm32mp1_ram.c +++ b/drivers/ram/stm32mp1/stm32mp1_ram.c @@ -57,6 +57,33 @@ int stm32mp1_ddr_clk_enable(struct ddr_info *priv, uint32_t mem_speed) return 0; } +__weak int board_stm32mp1_ddr_config_name_match(struct udevice *dev, + const char *name) +{ + return 0; /* Always match */ +} + +static ofnode stm32mp1_ddr_get_ofnode(struct udevice *dev) +{ + const char *name; + ofnode node; + + node = dev_ofnode(dev); + name = ofnode_get_name(node); + + if (!board_stm32mp1_ddr_config_name_match(dev, name)) + return node; + + dev_for_each_subnode(node, dev) { + name = ofnode_get_name(node); + + if (!board_stm32mp1_ddr_config_name_match(dev, name)) + return node; + } + + return ofnode_null(); +} + static __maybe_unused int stm32mp1_ddr_setup(struct udevice *dev) { struct ddr_info *priv = dev_get_priv(dev); @@ -64,6 +91,7 @@ static __maybe_unused int stm32mp1_ddr_setup(struct udevice *dev) unsigned int idx; struct clk axidcg; struct stm32mp1_ddr_config config; + ofnode node = stm32mp1_ddr_get_ofnode(dev); #define PARAM(x, y) \ { x,\ @@ -87,9 +115,9 @@ static __maybe_unused int stm32mp1_ddr_setup(struct udevice *dev) PHY_PARAM(cal) }; - config.info.speed = dev_read_u32_default(dev, "st,mem-speed", 0); - config.info.size = dev_read_u32_default(dev, "st,mem-size", 0); - config.info.name = dev_read_string(dev, "st,mem-name"); + config.info.speed = ofnode_read_u32_default(node, "st,mem-speed", 0); + config.info.size = ofnode_read_u32_default(node, "st,mem-size", 0); + config.info.name = ofnode_read_string(node, "st,mem-name"); if (!config.info.name) { debug("%s: no st,mem-name\n", __func__); return -EINVAL; @@ -97,7 +125,7 @@ static __maybe_unused int stm32mp1_ddr_setup(struct udevice *dev) printf("RAM: %s\n", config.info.name); for (idx = 0; idx < ARRAY_SIZE(param); idx++) { - ret = dev_read_u32_array(dev, param[idx].name, + ret = ofnode_read_u32_array(node, param[idx].name, (void *)((u32)&config + param[idx].offset), param[idx].size);
Add support for multiple DRAM configuration subnodes, while retaining the support for a single flat DRAM configuration node. This is useful on systems which can be manufactured in multiple configurations and where the DRAM configuration can be determined at runtime. The code is augmented by a function which can be overridden on board level, allowing a match on the configuration node name, very much like the fitImage configuration node name matching works. The default match is on the single top-level DRAM configuration, if matching on subnodes is required, then this board_stm32mp1_ddr_config_name_match() must be overridden. Signed-off-by: Marek Vasut <marex at denx.de> Cc: Manivannan Sadhasivam <manivannan.sadhasivam at linaro.org> Cc: Patrick Delaunay <patrick.delaunay at st.com> Cc: Patrice Chotard <patrice.chotard at st.com> --- drivers/ram/stm32mp1/stm32mp1_ram.c | 36 +++++++++++++++++++++++++---- 1 file changed, 32 insertions(+), 4 deletions(-)