Message-ID: <01829f2e-871e-cdea-afab-0ae1360464a4@ti.com>
Date:   Tue, 4 Dec 2018 11:28:37 +0530
From:   Kishon Vijay Abraham I <kishon@...com>
To:     Maxime Ripard <maxime.ripard@...tlin.com>,
        Sakari Ailus <sakari.ailus@....fi>
CC:     Boris Brezillon <boris.brezillon@...tlin.com>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        <linux-media@...r.kernel.org>,
        Archit Taneja <architt@...eaurora.org>,
        Andrzej Hajda <a.hajda@...sung.com>,
        Chen-Yu Tsai <wens@...e.org>, <linux-kernel@...r.kernel.org>,
        <dri-devel@...ts.freedesktop.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        Krzysztof Witos <kwitos@...ence.com>,
        Rafal Ciepiela <rafalc@...ence.com>
Subject: Re: [PATCH v2 4/9] phy: dphy: Add configuration helpers

Hi Maxime,

On 21/11/18 3:03 PM, Maxime Ripard wrote:
> Hi Sakari,
> 
> Thanks for your review.
> 
> On Mon, Nov 19, 2018 at 03:43:57PM +0200, Sakari Ailus wrote:
>>> +/*
>>> + * Minimum D-PHY timings based on MIPI D-PHY specification. Derived
>>> + * from the valid ranges specified in Section 6.9, Table 14, Page 41
>>> + * of the D-PHY specification (v2.1).
>>
>> I assume these values are compliant with the earlier spec releases.
> 
> I have access to the versions 1.2 and 2.1 of the spec and as far as I
> can tell, they match here. I can't really say for other releases, but
> I wouldn't expect any changes (and it can always be adjusted later on
> if needed).
> 
>>> + */
>>> +int phy_mipi_dphy_get_default_config(unsigned long pixel_clock,
>>
>> How about using the bus frequency instead of the pixel clock? Chances are
>> that the caller already has that information, instead of calculating it
>> here?
> 
> I went for the pixel clock since it's something that all drivers will
> have access too without any computation. The bus frequency can be
> available as well in v4l2, but won't be in DRM, and that would require
> for all drivers to duplicate that computation, which doesn't seem like
> a good choice.
> 
>>> +				     unsigned int bpp,
>>> +				     unsigned int lanes,
>>> +				     struct phy_configure_opts_mipi_dphy *cfg)
>>> +{
>>> +	unsigned long hs_clk_rate;
>>> +	unsigned long ui;
>>> +
>>> +	if (!cfg)
>>> +		return -EINVAL;
>>> +
>>> +	hs_clk_rate = pixel_clock * bpp / lanes;
>>> +	ui = DIV_ROUND_UP(NSEC_PER_SEC, hs_clk_rate);
>>
>> Nanoseconds may not be precise enough for practical computations on these
>> values. At 1 GHz, this ends up being precisely 1. At least Intel hardware
>> has some more precision, I presume others do, too. How about using
>> picoseconds instead?
> 
> Sounds like a good idea.

Would you be fixing this? Or this can be a later patch?

Thanks
Kishon
> 
>>> +
>>> +	cfg->clk_miss = 0;
>>> +	cfg->clk_post = 60 + 52 * ui;
>>> +	cfg->clk_pre = 8;
>>> +	cfg->clk_prepare = 38;
>>> +	cfg->clk_settle = 95;
>>> +	cfg->clk_term_en = 0;
>>> +	cfg->clk_trail = 60;
>>> +	cfg->clk_zero = 262;
>>> +	cfg->d_term_en = 0;
>>> +	cfg->eot = 0;
>>> +	cfg->hs_exit = 100;
>>> +	cfg->hs_prepare = 40 + 4 * ui;
>>> +	cfg->hs_zero = 105 + 6 * ui;
>>> +	cfg->hs_settle = 85 + 6 * ui;
>>> +	cfg->hs_skip = 40;
>>> +
>>> +	/*
>>> +	 * The MIPI D-PHY specification (Section 6.9, v1.2, Table 14, Page 40)
>>> +	 * contains this formula as:
>>> +	 *
>>> +	 *     T_HS-TRAIL = max(n * 8 * ui, 60 + n * 4 * ui)
>>> +	 *
>>> +	 * where n = 1 for forward-direction HS mode and n = 4 for reverse-
>>> +	 * direction HS mode. There's only one setting and this function does
>>> +	 * not parameterize on anything other that ui, so this code will
>>> +	 * assumes that reverse-direction HS mode is supported and uses n = 4.
>>> +	 */
>>> +	cfg->hs_trail = max(4 * 8 * ui, 60 + 4 * 4 * ui);
>>> +
>>> +	cfg->init = 100000;
>>> +	cfg->lpx = 60;
>>> +	cfg->ta_get = 5 * cfg->lpx;
>>> +	cfg->ta_go = 4 * cfg->lpx;
>>> +	cfg->ta_sure = 2 * cfg->lpx;
>>> +	cfg->wakeup = 1000000;
>>> +
>>> +	cfg->hs_clk_rate = hs_clk_rate;
>>
>> How about the LP clock?
>>
>> Frankly, I have worked with MIPI CSI-2 hardware soon a decade, and the very
>> few cases where software has needed to deal with these values has been in
>> form of defaults for a receiver, mostly limiting to clk_settle,
>> clk_term_en, d_term_en as well as hs_settle. On some hardware, the data
>> lane specific values can be at least in theory configured separately on
>> different lanes (but perhaps we could ignore that now).
>>
>> That doesn't say that it'd be useless to convey these values to the PHY
>> though. What I'm a little worried about though is what could be the effect
>> of adding support for this for existing drivers? If you have a new driver,
>> then there is no chance of regressions.
>>
>> I can't help noticing that many of the above values end up being unused in
>> the rest of the patches in the set. I guess that's ok, they come from the
>> standard anyway and some hardware may need them to be configured.
> 
> In order to get these parameters, I went through all the MIPI-DSI and
> MIPI-CSI drivers currently in the tree that could be converted, and
> looked at which parameters they needed to exchange with their PHY.
> 
> I made a summary to Kishon in the previous iteration here:
> https://lwn.net/ml/linux-media/20180919121436.ztjnxofe66quddeq@flea/
> 
> So it looks like the set of parameters on the MIPI-CSI side is indeed
> pretty limited, it really isn't for MIPI-DSI, and the whole point here
> is to support both :/
> 
>> Then there's the question of where should these values originate from.
>> Some drivers appear to have a need to obtain one of these values via
>> firmware, see Documentation/devicetree/bindings/media/samsung-mipi-csis.txt
>> . I presume the defaults should be applicable to most cases, and specific
>> values would need to be defined in the firmware. That means that the
>> defaults have effectively the property of firmware API, meaning that they
>> effectively can never be changed. That suggests we should be pretty sure
>> the defaults are something that should work for the widest possible set of
>> the hardware.
> 
> That function here is made to provide the spec default for those
> values. Any driver is free to change those defaults, as long as they
> remain within the spec boundaries of course. And I'd say that how the
> drivers need to get those non-default values would be driver specific,
> it shouldn't really impact the API here.
> 
> Thanks!
> Maxime
> 

Powered by blists - more mailing lists