16.05.2019 10:52, Joseph Lo пишет:
> On 5/15/19 11:25 PM, Dmitry Osipenko wrote:
>> 15.05.2019 11:42, Joseph Lo пишет:
>>> On 5/15/19 1:04 AM, Dmitry Osipenko wrote:
>>>> 10.05.2019 11:47, 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 for 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@xxxxxxxxxx>.
>>>>>
>>>>> Signed-off-by: Joseph Lo <josephl@xxxxxxxxxx>
>>>>> ---
> snip.
>>>>> + if (!seq->set_clock) {
>>>>> + seq = NULL;
>>>>> + dev_err(&pdev->dev, "Invalid EMC sequence for table Rev.
>>>>> %d\n",
>>>>> + emc->emc_table[0].rev);
>>>>> + goto emc_clk_register;
>>>>
>>>> Why do you want to register EMC clock if something fails? KMSG will be
>>>> flooded with errors coming from clk_set_rate.
>>>>
>>>
>>> See patch 7 in the series, the legacy EMC clock will be removed later,
>>> so we need to register the EMC clock whether the table is ready or
>>> not> In that case, I mean if the table is not available, it will still
>>> register EMC clock at the rate that boot loader configured before kernel
>>> booting. So the MC clock can still work as expected, which is under EMC
>>> clock.
>>>
>>> And I did test that, couldn't observe any KMSG in that case.
>>
>> Looks like it kinda should work in the end.
>>
>> Although it's not good that now MC driver relies on the EMC driver
>> presence. Maybe it's not the best variant with moving the clock stuff
>> into the EMC driver?
>>
>> What about the backwards compatibility for DT that doesn't have the EMC
>> node?
>>
>> What if EMC driver is disabled in the kernel's config?
>
> The three questions above are actually one problem here. It's not about
> MC clock, because it's still available after these changes. And MC
> driver can still get it in the probe function even the EMC driver isn't
> there.
No, these are separate problems. MC driver queries the clock rate during
the probe to configure memory arbitration. In your case the clock rate
is always zero for MC.
> The problem is that without EMC driver after these changes. The PLLM
> will have no client under it, which will cause the PLLM to be disabled
> in the late init call of "clk_disable_unused". So the system will be stuck.
This and the above are very compelling reasons to *NOT* register the
clock from the EMC driver. You shall move all the clock stuff into the
clock driver. Please see T124 EMC driver and what is done for the
upcoming T30 EMC driver [1] for the example, borrow parts that fit best
for T210.
[1] https://patchwork.ozlabs.org/project/linux-tegra/list/?series=102688
>>
>> And lastly.. what stops the MC driver to probe before the EMC? Looks
>> like MC driver is already in trouble because it's on arch level and the
>> EMC is on subsys, hence MC will get the orphaned clock and won't
>> initialize hardware properly on probe.
>
> After this moving, the EMC driver will be always enabled by default. And
> the DT change is necessary as well. The blob of EMC table is not
> necessary, because it needs a firmware update. We will update the
> firmware accordingly after the review settled and release it later.
>
> In case of no EMC table blob, the driver can still be registered, but no
> scaling function provided.
Anyone could change the default kernel configuration. Kernel shall boot
and work fine with older device-tree's and any custom config after
update. You should not break someones established setup without a good
reason and you don't have one here.
>>
>> BTW, how are you testing the EMC driver? Is there T210 devfreq patches
>> in works? Or what's the user of the EMC on T210?
>>
>
> 1. Currently, via debugfs.
> 2. No, we prefer to use Interconnect framework for that. The evaluation
> is ongoing.
> 3. With Interconnect, the devices or peripherals can register on it to
> request the BW. So we can fine-tune the BW requirements with the latency
> allowance registers altogether to get better efficiency.
Devfreq is the driver for the ACTMON hardware unit. This unit tells the
driver when memory clock rate need to go higher or lower, depending on
overall memory clients activity. Currently the tegra-devfreq driver
supports T124 only, it will support T30 starting with v5.3. AFAIK, it
shouldn't be difficult to add support for T210 as well.
IIRC, the new Interconnect API is another away of conveying different
requirements between devices. All Tegra's have knobs for memory
configuration tuning, but there is no real need to change the default
good-enough configuration for the time being in upstream.
AFAIK, there is only one memory client that is really very sensitive to
available memory bandwidth - Display Controller. The PM QoS memory
bandwidth API is more than enough for the easy start and it's all
internal to kernel, hence it will be possible to replace the PM API with
something more advanced later on by as-needed basis. That's what I'm
currently targeting for T20-T124. The PM QoS API could coexist with the
Interconnect API (or whatever else) without any troubles, so it won't be
a problem if you'll decide to pull into other direction for T210.
AFAIK, the latency allowance config should be similar on all Tegra's
starting from T30. Will be awesome if you'll do all the hard job of
bringing up the fresh new API for T210, it always easier to follow by
example.
