Re: [PATCH v1] memory: tegra30-emc: Print additional memory info

[Dmitry Osipenko <digetx@gmail.com>]

21.12.2021 11:14, Krzysztof Kozlowski пишет:
>>>> +static void emc_read_lpddr_sdram_info(struct tegra_emc *emc,
>>>> +				      unsigned int emem_dev)
>>>> +{
>>>> +	union lpddr2_basic_config4 basic_conf4;
>>>> +	unsigned int manufacturer_id;
>>>> +	unsigned int revision_id1;
>>>> +	unsigned int revision_id2;
>>>> +
>>>> +	/* these registers are standard for all LPDDR JEDEC memory chips */
>>>> +	emc_read_lpddr_mode_register(emc, emem_dev, 5, &manufacturer_id);
>>>> +	emc_read_lpddr_mode_register(emc, emem_dev, 6, &revision_id1);
>>>> +	emc_read_lpddr_mode_register(emc, emem_dev, 7, &revision_id2);
>>>> +	emc_read_lpddr_mode_register(emc, emem_dev, 8, &basic_conf4.value);
>>>> +
>>>> +	dev_info(emc->dev, "SDRAM[dev%u]: manufacturer: 0x%x (%s) rev1: 0x%x rev2: 0x%x prefetch: S%u density: %uMbit iowidth: %ubit\n",
>>>> +		 emem_dev, manufacturer_id,
>>>> +		 lpddr2_jedec_manufacturer(manufacturer_id),
>>>> +		 revision_id1, revision_id2,
>>>> +		 4 >> basic_conf4.arch_type,
>>>> +		 64 << basic_conf4.density,
>>>> +		 32 >> basic_conf4.io_width);
>>>> +}
>>>> +
>>> Quickly looking, these two functions are exactly the same as ones in
>>> tegra20-emc.c
>>> . Later you might come up with another set for other SoCs, so it looks
>>> it is worth to share these.
>> Should be too much trouble for not much gain, IMO. How many bytes will
>> be shared in the end? There is no much code here, we may lose more than
>> win. All these Tegra EMC drivers can be compiled as a loadable modules,
>> that's what distro kernels usually do. There are no plans for other SoCs
>> for today.
> It's not about the bytes but source code lines to maintain and fix (if
> there is something to fix). But if you don't plan to make a third copy
> of it, it is okay.

Only Tegra114 SoC potentially supports LPDDR2, later SoCs dropped LPDDR2
support. We don't even have memory driver for T114 at all in the today's
mainline. I also doubt that there were any consumer T114 devices sold
with LPDDR2. Hence we shouldn't have a need for the third copy anytime
soon, likely ever.

>> I don't see how that sharing could be done easily and nicely. Please
>> tell if you see.
> Since it is not about duplicated object code, but code for review, it is
> pretty straightforward:
> 
> 1. Create tegra-emc-common.[ch]
> 2. In Makefile:
> 
> +tegra20_emc-y += tegra20-emc.o tegra-emc-common.o
> 
> +obj-$(CONFIG_TEGRA20_EMC)  += tegra20_emc.o
> 
> +
> 
> +tegra30_emc-y += tegra30-emc.o tegra-emc-common.o
> 
> +obj-$(CONFIG_TEGRA30_EMC)  += tegra30_emc.o
> 

The problem that struct tegra_emc isn't shareable and this common code
should introduce messiness to the Tegra EMC drivers. I'd prefer not to
share anything for now and get back to this option with sharing later
on, if will be another good reason.