Re: [PATCH V4 4/8] memory: tegra: Add Tegra210 EMC clock driver

[Dmitry Osipenko <digetx@gmail.com>]

26.02.2020 19:57, Thierry Reding пишет:
> On Thu, May 30, 2019 at 10:45:01AM +0800, Joseph Lo wrote:
>> On 5/29/19 9:26 PM, Dmitry Osipenko wrote:
>>> 29.05.2019 11:21, Joseph Lo пишет:
>>>> This is the initial patch for Tegra210 EMC clock driver, which doesn't
>>>> include the support code and detail sequence for clock scaling yet.
>>>>
>>>> The driver is designed to support LPDDR4 SDRAM. Because of the LPDDR4
>>>> devices need to do initial time training before it can be used, the
>>>> firmware will help to do that at early boot stage. Then, the trained
>>>> table of the rates we support will pass to the kernel via DT. So the
>>>> driver can get the trained table for clock scaling support.
>>>>
>>>> For the higher rate support (above 800MHz), the periodic training is
>>>> needed for the timing compensation. So basically, two methodologies for
>>>> clock scaling are supported, one is following the clock changing
>>>> sequence to update the EMC table to EMC registers and another is if the
>>>> rate needs periodic training, then we will start a timer to do that
>>>> periodically until it scales to the lower rate.
>>>>
>>>> Based on the work of Peter De Schrijver <pdeschrijver@nvidia.com>.
>>>>
>>>> Signed-off-by: Joseph Lo <josephl@nvidia.com>
>>>> ---
>>>> v4:
>>>> - remove the statistic data in debugfs
>>>> - add tegra210_clk_register_emc API to make it compatible with the case
>>>>    if the kernel still uses the older DTB which doesn't have EMC node.
>>>>    And the MC and EMC clock can still be registered successfully.
>>>> v3:
>>>> - address almost all the comments from the previous version
>>>> - remove the DT parser of EMC table
>>>> - The EMC table is passing as a binary blob now.
>>>> ---
>>>>   drivers/memory/tegra/Kconfig        |  10 +
>>>>   drivers/memory/tegra/Makefile       |   1 +
>>>>   drivers/memory/tegra/tegra210-emc.c | 671 ++++++++++++++++++++++++++++
>>>>   drivers/memory/tegra/tegra210-emc.h | 156 +++++++
>>>>   include/soc/tegra/emc.h             |   2 +
>>>>   5 files changed, 840 insertions(+)
>>>>   create mode 100644 drivers/memory/tegra/tegra210-emc.c
>>>>   create mode 100644 drivers/memory/tegra/tegra210-emc.h
>>>>
>>>> diff --git a/drivers/memory/tegra/Kconfig b/drivers/memory/tegra/Kconfig
>>>> index 4680124ddcab..9d051bcdbee3 100644
>>>> --- a/drivers/memory/tegra/Kconfig
>>>> +++ b/drivers/memory/tegra/Kconfig
>>>> @@ -26,3 +26,13 @@ config TEGRA124_EMC
>>>>   	  Tegra124 chips. The EMC controls the external DRAM on the board.
>>>>   	  This driver is required to change memory timings / clock rate for
>>>>   	  external memory.
>>>> +
>>>> +config TEGRA210_EMC
>>>> +	bool "NVIDIA Tegra210 External Memory Controller driver"
>>>> +	default y
>>>
>>> This is not enough since you're leaving possibility to disable
>>> compilation of the driver, but the compilation will fail because of the
>>> unresolved symbol (tegra210_clk_register_emc).
>>>
>>>> +	depends on TEGRA_MC && ARCH_TEGRA_210_SOC
>>>> +	help
>>>> +	  This driver is for the External Memory Controller (EMC) found on
>>>> +	  Tegra210 chips. The EMC controls the external DRAM on the board.
>>>> +	  This driver is required to change memory timings / clock rate for
>>>> +	  external memory.
>>>
>>> Either TEGRA210_EMC Kconfig option shall be always force-selected for
>>> T210 or you should move all the clk-related code into drivers/clk/tegra/.
>>>
>>> Could you please give a rationale for having EMC clock code within the
>>> EMC driver?
>>
>> I didn't have a specific reason for that initially, just wanted the clock
>> code and EMC driver together for easier maintenance.
>>
>> But considering the fix in v4, that makes it backward compatible with the
>> case if the kernel uses the older DT without EMC node, I think it's better
>> to move the clock code into the clk folder now.
> 
> I looked into this a bit and I don't think this is actually worth it.
> The problem is that, as opposed to Tegra124 and earlier, the sequence
> for changing the EMC frequency is much more entangled. The bulk of the
> programming will be on the EMC side, with the code occasionally calling
> into CAR code to set the parent clock and some other flags.
> 
> So there's going to be some interdependencies regardless of where the
> clock code actually lives. I can try to split this apart, but I don't
> have very high hopes that the end result will be any cleaner than the
> version here.

I'm vaguely recalling that there was another reason than just to "make
things cleaner"..

https://patchwork.kernel.org/patch/10938389/#22641053

Secondly, if you're going to use the CCF API for the clock changes, then
I'm not sure that having couple custom clock-API functions sounds too bad.

Lastly, in regards to the cleanup.. at minimum you should strip out all
the unused parts from this code, make a generic cleanup to make it all
look better, address previous comments.