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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <f775891b-12c4-ae6c-364c-abaa7ea3f249@gmail.com>
Date:   Tue, 21 Dec 2021 18:34:41 +0300
From:   Dmitry Osipenko <digetx@...il.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>,
        Thierry Reding <thierry.reding@...il.com>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Svyatoslav Ryhel <clamor95@...il.com>
Cc:     linux-kernel@...r.kernel.org, linux-tegra@...r.kernel.org
Subject: Re: [PATCH v1] memory: tegra30-emc: Print additional memory info

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.