Re: [PATCH v3 3/4] memory: Add LPDDR2 configuration helpers

[Krzysztof Kozlowski]

On 03/10/2021 03:32, Dmitry Osipenko wrote:
> Add helpers for reading and parsing standard LPDDR2 properties.
> 
> Signed-off-by: Dmitry Osipenko <digetx@xxxxxxxxx>
> ---
>  drivers/memory/jedec_ddr.h      | 21 +++++++++++++++++
>  drivers/memory/jedec_ddr_data.c | 42 +++++++++++++++++++++++++++++++++
>  drivers/memory/of_memory.c      | 34 ++++++++++++++++++++++++++
>  drivers/memory/of_memory.h      |  9 +++++++
>  4 files changed, 106 insertions(+)
> 
> diff --git a/drivers/memory/jedec_ddr.h b/drivers/memory/jedec_ddr.h
> index e59ccbd982d0..14cef272559e 100644
> --- a/drivers/memory/jedec_ddr.h
> +++ b/drivers/memory/jedec_ddr.h
> @@ -230,4 +230,25 @@ struct lpddr3_min_tck {
>  	u32 tMRD;
>  };
>  
> +union lpddr2_basic_config4 {
> +	u32 value;
> +
> +	struct {
> +		unsigned int arch_type : 2;
> +		unsigned int density : 4;
> +		unsigned int io_width : 2;
> +	} __packed;
> +};
> +
> +struct lpddr2_configuration {
> +	int arch_type;
> +	int density;
> +	int io_width;
> +	int manufacturer_id;
> +	int revision_id1;
> +	int revision_id2;
> +};
> +
> +const char *lpddr2_jedec_manufacturer(unsigned int manufacturer_id);
> +
>  #endif /* __JEDEC_DDR_H */
> diff --git a/drivers/memory/jedec_ddr_data.c b/drivers/memory/jedec_ddr_data.c
> index ed601d813175..1f214716ac45 100644
> --- a/drivers/memory/jedec_ddr_data.c
> +++ b/drivers/memory/jedec_ddr_data.c
> @@ -7,6 +7,7 @@
>   * Aneesh V <aneesh@xxxxxx>
>   */
>  
> +#include <dt-bindings/memory/lpddr2.h>
>  #include <linux/export.h>
>  
>  #include "jedec_ddr.h"
> @@ -131,3 +132,44 @@ const struct lpddr2_min_tck lpddr2_jedec_min_tck = {
>  	.tFAW		= 8
>  };
>  EXPORT_SYMBOL_GPL(lpddr2_jedec_min_tck);
> +
> +const char *lpddr2_jedec_manufacturer(unsigned int manufacturer_id)
> +{
> +	switch (manufacturer_id) {
> +	case LPDDR2_MANID_SAMSUNG:
> +		return "Samsung";
> +	case LPDDR2_MANID_QIMONDA:
> +		return "Qimonda";
> +	case LPDDR2_MANID_ELPIDA:
> +		return "Elpida";
> +	case LPDDR2_MANID_ETRON:
> +		return "Etron";
> +	case LPDDR2_MANID_NANYA:
> +		return "Nanya";
> +	case LPDDR2_MANID_HYNIX:
> +		return "Hynix";
> +	case LPDDR2_MANID_MOSEL:
> +		return "Mosel";
> +	case LPDDR2_MANID_WINBOND:
> +		return "Winbond";
> +	case LPDDR2_MANID_ESMT:
> +		return "ESMT";
> +	case LPDDR2_MANID_SPANSION:
> +		return "Spansion";
> +	case LPDDR2_MANID_SST:
> +		return "SST";
> +	case LPDDR2_MANID_ZMOS:
> +		return "ZMOS";
> +	case LPDDR2_MANID_INTEL:
> +		return "Intel";
> +	case LPDDR2_MANID_NUMONYX:
> +		return "Numonyx";
> +	case LPDDR2_MANID_MICRON:
> +		return "Micron";
> +	default:
> +		break;
> +	}
> +
> +	return "invalid";
> +}
> +EXPORT_SYMBOL_GPL(lpddr2_jedec_manufacturer);
> diff --git a/drivers/memory/of_memory.c b/drivers/memory/of_memory.c
> index d9f5437d3bce..8aa777f2a090 100644
> --- a/drivers/memory/of_memory.c
> +++ b/drivers/memory/of_memory.c
> @@ -298,3 +298,37 @@ const struct lpddr3_timings
>  	return NULL;
>  }
>  EXPORT_SYMBOL(of_lpddr3_get_ddr_timings);
> +
> +/**
> + * of_lpddr2_get_config() - extracts the lpddr2 chip configuration.
> + * @np: Pointer to device tree node containing configuration
> + * @conf: Configuration updated by this function
> + *
> + * Populates lpddr2_configuration structure by extracting data from device
> + * tree node. Returns 0 on success or error code on failure. If property
> + * is missing in device-tree, then the corresponding @conf value is set to
> + * -ENOENT.
> + */
> +int of_lpddr2_get_config(struct device_node *np,
> +			 struct lpddr2_configuration *conf)
> +{

Interface should be rather like of_get_ddr_timings() - allocate memory
for structure and return it. It's less error-prone.

> +	int err, ret = -ENOENT;
> +
> +#define OF_LPDDR2_READ_U32(prop, dtprop) \
> +	err = of_property_read_u32(np, dtprop, &conf->prop); \
> +	if (err) \
> +		conf->prop = -ENOENT; \
> +	else \
> +		ret = 0
> +
> +	/* at least one property should be parsed */
> +	OF_LPDDR2_READ_U32(manufacturer_id, "jedec,lpddr2-manufacturer-id");
> +	OF_LPDDR2_READ_U32(revision_id1, "jedec,lpddr2-revision-id1");
> +	OF_LPDDR2_READ_U32(revision_id2, "jedec,lpddr2-revision-id2");
> +	OF_LPDDR2_READ_U32(io_width, "jedec,lpddr2-io-width-bits");
> +	OF_LPDDR2_READ_U32(density, "jedec,lpddr2-density-mbits");
> +	OF_LPDDR2_READ_U32(arch_type, "jedec,lpddr2-type");

density and io-width are required properties in existing bindings, so
return code should not be overridden.

> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(of_lpddr2_get_config);
> diff --git a/drivers/memory/of_memory.h b/drivers/memory/of_memory.h
> index 4a99b232ab0a..95eccc251b04 100644
> --- a/drivers/memory/of_memory.h
> +++ b/drivers/memory/of_memory.h
> @@ -20,6 +20,9 @@ const struct lpddr3_min_tck *of_lpddr3_get_min_tck(struct device_node *np,
>  const struct lpddr3_timings *
>  of_lpddr3_get_ddr_timings(struct device_node *np_ddr,
>  			  struct device *dev, u32 device_type, u32 *nr_frequencies);
> +
> +int of_lpddr2_get_config(struct device_node *np,
> +			 struct lpddr2_configuration *conf);
>  #else
>  static inline const struct lpddr2_min_tck
>  	*of_get_min_tck(struct device_node *np, struct device *dev)
> @@ -46,6 +49,12 @@ static inline const struct lpddr3_timings
>  {
>  	return NULL;
>  }
> +
> +static int of_lpddr2_get_config(struct device_node *np,
> +				struct lpddr2_configuration *conf)
> +{
> +	return -ENOENT;
> +}
>  #endif /* CONFIG_OF && CONFIG_DDR */
>  
>  #endif /* __LINUX_MEMORY_OF_REG_ */
> 


Best regards,
Krzysztof