Re: [PATCH v2 5/5] memory: tegra: Introduce Tegra20 EMC driver

[Thierry Reding]

On Mon, Jun 11, 2018 at 04:06:41PM +0300, Dmitry Osipenko wrote:
> On Monday, 11 June 2018 14:35:03 MSK Thierry Reding wrote:
> > On Wed, Jun 06, 2018 at 04:42:01PM +0300, Dmitry Osipenko wrote:
> > > On 06.06.2018 14:02, Thierry Reding wrote:
> > > > On Mon, Jun 04, 2018 at 01:36:54AM +0300, Dmitry Osipenko wrote:
[...]
> > > >> +	} else {
> > > >> +		init_completion(&emc->clk_handshake_complete);
> > > >> +
> > > >> +		err = devm_request_irq(&pdev->dev, emc->irq, tegra_emc_isr, 
> 0,
> > > >> +				       dev_name(&pdev->dev), emc);
> > > >> +		if (err < 0) {
> > > >> +			dev_err(&pdev->dev, "failed to request IRQ#%u: %d\n",
> > > >> +				emc->irq, err);
> > > >> +			return err;
> > > >> +		}
> > > >> +	}
> > > >> +
> > > >> +	emc->pll_m = clk_get_sys(NULL, "pll_m");
> > > >> +	if (IS_ERR(emc->pll_m)) {
> > > >> +		err = PTR_ERR(emc->pll_m);
> > > >> +		dev_err(&pdev->dev, "failed to get pll_m: %d\n", err);
> > > >> +		return err;
> > > >> +	}
> > > >> +
> > > >> +	emc->backup_clk = clk_get_sys(NULL, "pll_p");
> > > >> +	if (IS_ERR(emc->backup_clk)) {
> > > >> +		err = PTR_ERR(emc->backup_clk);
> > > >> +		dev_err(&pdev->dev, "failed to get pll_p: %d\n", err);
> > > >> +		goto put_pll_m;
> > > >> +	}
> > > >> +
> > > >> +	emc->clk = clk_get_sys(NULL, "emc");
> > > >> +	if (IS_ERR(emc->clk)) {
> > > >> +		err = PTR_ERR(emc->clk);
> > > >> +		dev_err(&pdev->dev, "failed to get emc: %d\n", err);
> > > >> +		goto put_backup;
> > > >> +	}
> > > > 
> > > > Instead of using clk_get_sys(), why not specify these in the DT with
> > > > proper names for context ("emc", "pll", "backup")? Again, I don't think
> > > > we have to worry about backwards-compatibility here since there can be
> > > > no regression.
> > > 
> > > I don't think that "pll" and "backup" could be placed in DT because it is
> > > a pure software-driver descriptio     in
> > DT. There are other cases, like for display, where we list clocks in the
> > DT that aren't strictly inputs to a hardware block. But they are related
> > to the functionality of the block, so I think it makes sense to list
> > them as well.
> > 
> > In this particular case, the PLL is what drives the memory banks, which
> > is the think that the EMC controls, right? So that itself is reason
> > enough, in my opinion, to list it in DT. Much the same goes for the
> > backup clock, which is really just the PLL for some transient state
> > where the normal PLL is being reconfigured.
> 
> PLL itself shouldn't really matter. EMC doesn't control PLL, but only 
> interacts with the Clock-and-Reset controller. This means that we could use 
> PLL_C instead of PLL_M if we wanted. Though PLL_M is meant to be a "memory 
> PLL".
> 
> To me a selection of a parent clock is a pure software description that is 
> wrong to be placed in DT.

I think of it as configuration rather than software description. The
problem that we're trying to solve with DT is primarily one of
configuration and parameterization. The idea is that we describe the
interfaces of some hardware module and then specify which resources
to use as inputs and/or outputs for those interfaces.

So in this case we can define that in order to perform its function the
EMC driver needs to somehow control a PLL that drives the memory. Even
if the EMC hardware itself doesn't control a PLL, not controlling a PLL
would make the EMC driver rather pointless. Actually, if the hardware
did control the PLL itself, there'd be no need to describe any aspect of
that PLL in the DT. We only need to describe it in DT because we've got
several possibilities and we want to make sure we pick one that is best
for our specific use-case.

> I'm not sure that specifying parent clock for display in DT is correct. That 
> simply doesn't make sense because there are four possible parent clocks for 
> the display. Selection of a parent clock in DT is a pure software description 
> that is only relevant for the upstream Linux DRM driver, it's a mistake to me.

I disagree. While it is certainly true that there are multiple possible
PLLs for display, which one to use depends on the use-case. In most SoC
generations we have two display PLLs where pll_d2 is "more capable" than
pll_d because it can run at higher frequencies. pll_d for example can in
many cases not be used to drive an HDMI output at 1080p resolutions. For
DisplayPort output, pll_d can usually also not be used. It therefore
makes sense to define in DT which PLL to use for a particular use-case.

Now, I'll grant you that this somewhat blurs the lines of hardware
descriptions vs. software description. But the alternative would be that
we don't describe the PLLs in DT, which would imply that we have to have
some logic in the driver that either knows which PLLs exist on the given
SoC and can query their capabilities in order to determine which one to
use for any given use-case.

It also means that the driver becomes much less generic, because we have
to put a lot of SoC specific knowledge into the driver. This is perhaps
not a big issue for SoCs like Tegra that are very closely integrated
anyway, but imagine if you take the same approach with IP that can be
licensed from a third party and therefore could appear in many more
different combinations.

I think having the parent PLL defined in DT is a good compromise. If you
say that strictly only hardware interfaces can be represented in DT, you
take away a lot of the flexibility and in turn put a lot of the data
back into drivers.

> Moreover PLL isn't a "device" clock, but specifically a "system" clock. So 
> describing it in DT as a "device" clock is wrong to me too.
> 
> Putting all together, the PLL's should be left as-is they are now in v2, 
> please let me know if you disagree.

I don't care very strongly in this case because the driver is unlikely
to ever be reused anywhere other than on Tegra20. But looking up the
system clock by a string is not something that is typically encouraged
because it limits portability.

> The "emc" clock could be placed in DT since we won't bother with the DT 
> backwards compatibility, I'll change it in the next revision of the patchset.

Frankly, if you don't have the other clocks in DT there's little sense
in having the EMC clock in DT.

Thierry

Attachment: signature.asc
Description: PGP signature