Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <51eda14f-5c37-c3af-84de-b591b8762a63@intel.com>
Date:   Tue, 18 Jan 2022 11:44:20 +0100
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.5.0
Subject: Re: [PATCH] phy: dphy: Correct clk_pre parameter
Content-Language: en-US
To:     Liu Ying <victor.liu@nxp.com>, <dri-devel@lists.freedesktop.org>,
        <linux-kernel@vger.kernel.org>, <linux-phy@lists.infradead.org>,
        <linux-arm-kernel@lists.infradead.org>,
        <linux-amlogic@lists.infradead.org>,
        <linux-rockchip@lists.infradead.org>
CC:     <linux-imx@nxp.com>, Neil Armstrong <narmstrong@baylibre.com>,
        Robert Foss <robert.foss@linaro.org>,
        Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
        Jonas Karlman <jonas@kwiboo.se>,
        "Jernej Skrabec" <jernej.skrabec@gmail.com>,
        David Airlie <airlied@linux.ie>,
        "Daniel Vetter" <daniel@ffwll.ch>,
        Kishon Vijay Abraham I <kishon@ti.com>,
        Vinod Koul <vkoul@kernel.org>,
        Kevin Hilman <khilman@baylibre.com>,
        Jerome Brunet <jbrunet@baylibre.com>,
        Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
        Heiko Stuebner <heiko@sntech.de>,
        Maxime Ripard <mripard@kernel.org>,
        =?UTF-8?Q?Guido_G=c3=bcnther?= <agx@sigxcpu.org>
References: <20220118025918.1444288-1-victor.liu@nxp.com>
 <96cd66cd-8e90-5f68-6009-f48015400aae@intel.com>
 <1d945988f774732c204f9dabdb8fa5795f0eca24.camel@nxp.com>
From:   Andrzej Hajda <andrzej.hajda@intel.com>
Organization: Intel Technology Poland sp. z o.o. - ul. Slowackiego 173, 80-298
 Gdansk - KRS 101882 - NIP 957-07-52-316
In-Reply-To: <1d945988f774732c204f9dabdb8fa5795f0eca24.camel@nxp.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?elJRR0JKKzBpcGdUbzZpK0VxMkljL1RVaE5hZFFMV2x3ZU5FOWNkQmNIN01n?=
 =?utf-8?B?SU9HRm80R0hPdmFETXNRMVV3SUxMVC90Rkk0Q1kwb3FES1VnL2pYaDRDZWI4?=
 =?utf-8?B?c2ZzcmZoaFplTU8rTmJUbVkxNHNObG9PNlM2MEhsWTUxWkJQT3pIQVBNYWlN?=
 =?utf-8?B?SGEyQUN6bkdVb2xvZUdDRi9lc3FqWTdNRWJXVWxmWnJNOUdBaGpoY25IREF6?=
 =?utf-8?B?QWY0amp1RnpWS0NvcXFVOU5rU0dCUGtlWEhpanRoRnQvQjgwRTZ5U3lNNlRD?=
 =?utf-8?B?S3BFL3RIeS9RWkY3K2FLMnR6M2dBTHE5NnhqNnJIbGtaclZweldOSkJ6MXcv?=
 =?utf-8?B?ZDZHQ2ZITy9QdjJIaHJtWmlGVlZEeWJ6SkdhTkR6YVZuVDJ2TWkxdmljQisr?=
 =?utf-8?B?QndHd1ZaK2dSU3VGKzVPWTltSlZxR0EyNXVqV1V4Z290czNNci9ncGswWWxF?=
 =?utf-8?B?a3hTRnhtN01ObGltRmdkTDIyamRlSDRmNDNLeTRaSVRhSHNEYWRQbk9naEs0?=
 =?utf-8?B?dGtPTk1xUUVLNC93TGFMMHp4aFJxcU9BM042UTFRRlRhWi9xckF2L2xnSHlw?=
 =?utf-8?B?V3Q2cE0wMGdLMXduU2M2VzNFWjh1d0dxUUlhS1FacCtMc1R1QTIwNThBd3pG?=
 =?utf-8?B?cm5xRmpjR0lBYlFzVngrTEZoTHlMdUh2YUNmU09wQzQrM3p5QjFUOVRmUCtR?=
 =?utf-8?B?NmFiWU1lbkwyRzZUSDRWMnhpZVV6cG15ZDRyUkdRSkFSaEZOQ0VSMkZuSzN6?=
 =?utf-8?B?TW9ZZHJGZzl4Q1VUZVF3Znplb1BvYVh6a3BhUkhjTEx4TVRDdDFvOXY1WkVj?=
 =?utf-8?B?M2VBZzBOaG9qYTdaNWF4aWIvNlhUYUx3QjNXcVVDY2ZMSXovZ1JsZjYxZ3p1?=
 =?utf-8?B?WEQ1MjJjR1ZydU0xUTcvNXo1ME0zcEp3L2pXTEtZNllEWmZqZ2VnTUFtZTlQ?=
 =?utf-8?B?dXFSUWxqOHZIdnRvTEVHOWNaZ2pKSjVlVERkbWFyNDdTdnZsNFV0Q1VIVEJP?=
 =?utf-8?B?KzA1ckhZSU5BMWpZV3FVV3VSbEIzWis3QUkyN3k1VmRzUUhhMzlOdGxCTUgw?=
 =?utf-8?B?c2tseGQvUzNwYW9QMk85Q3BCcjQ4Y0xpSk04eDBBUGRUV2Qwa21HQXNiODRK?=
 =?utf-8?B?TTdUOW1RcXJobWpMZEFJbjIxM1dpMXNkQUE2MU9HQ2FNRmNEamRZS09KdVov?=
 =?utf-8?B?UU1BOFp6QlpLRmVzSitLUm9YWGhxNG90MFAxTStEZU5HaldPYzBVc2xDdTJB?=
 =?utf-8?B?Zk5jWEM5ZUt4VE5ocHF2ME00d0NPdkFoSTZyRXBIa2tPM214d3k2K2tvd0Q4?=
 =?utf-8?B?bVp6cTJkU3F5UkxaSGFFRkVWbkdoNEkvaHA4YWFhTkVzN2lDclluK3V1dElV?=
 =?utf-8?B?TnI5SUJFSExrZS9URUh3cnlacURxYmhtSllwbkd6RDBEdXMxZW9xTUYwTFlW?=
 =?utf-8?B?QXk0WTNWQVYvKzJXVDVoeDZLbmlXVFRady9KTUtuZ2hyNzBndVNBRkdmTXR2?=
 =?utf-8?B?dzZsQVZQTzRBMWU2Zyt3Vm1lSzVVSVJicndkVnMxTnllSXIvdHBkMmRKQ1ZQ?=
 =?utf-8?B?VlhZbkpNQ0hjR1A4V2F3NWk0b3owYStHanpPWkVuelZtYWY4a1IweEVmNjJD?=
 =?utf-8?B?aE5sT3RSWit3QWFUaFd5ck9GTjF0eUphYkFJSHBJbEd3NGpOcGkzMDJtalhv?=
 =?utf-8?B?YmxHSDhYa2I4MytNeWZCVElIcEQvb1NyTWt4c2tKSFZxRkFEcmpHYmwremdO?=
 =?utf-8?B?OHYvdE1qMGxucVptWDB6Qi9IK1VESXQ2V2FuSEhkRE0rWG1aanF0V29nZHpT?=
 =?utf-8?B?ME56SGgrUGR2eG5CSi9SNzNZbzZMYVNXMHRkbzNPOUIzdVNySmN6cld6ZEV1?=
 =?utf-8?B?RTQ1dmwzVUtqdWRsdi9Ha2ZFOWUydmZEWGZQcW9odmE3dTFYQ1ZwaE1GMGtX?=
 =?utf-8?B?RFVVbHhxb05SaHFmUjAraU5xaGp5M2REMjJNQlRRVks5bEZ5ejE2OFBzVTEw?=
 =?utf-8?B?NHA2WEhXWitYaGJKbjlUOCtoRlcvOGo3UXdXcjdOWExYMU9mVnZlRXY2bGRt?=
 =?utf-8?B?bjdrc2pDd3R2Rkh3d0JiVzlkNWR6dGh2ZFBvbVdFRHFoZ2hhdTJLQXVIV2VV?=
 =?utf-8?B?SlkvVWdqM3lhSU55VzA2WTYzRzVYZ2JUQ2NXc0FZZlpqL2FJZEt4T1Z4d2lJ?=
 =?utf-8?Q?Vdcd6xrp5Y9Q+OROdCmksG0=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 4d35f4f2-bd16-41ab-e45a-08d9da6f804b
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3180.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Jan 2022 10:44:28.0818
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 2vF+NL2XyEzj+Mc3/EdSzl2jeCuDsT8LvuOZN/XSLZOoVPwmoTj5hQcdaZw8AwCHAA9i5dQcYKBbvumDbx6JRw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB2617
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 18.01.2022 11:14, Liu Ying wrote:
> Hi Andrzej,
>
> Thanks for your review.
>
> On Tue, 2022-01-18 at 10:24 +0100, Andrzej Hajda wrote:
>> Hi,
>>
>> On 18.01.2022 03:59, Liu Ying wrote:
>>> The D-PHY specification (v1.2) explicitly mentions that the T-CLK-PRE
>>> parameter's unit is Unit Interval(UI) and the minimum value is 8.  Also,
>>> kernel doc of the 'clk_pre' member of struct phy_configure_opts_mipi_dphy
>>> mentions that it should be in UI.  However, the dphy core driver wrongly
>>> sets 'clk_pre' to 8000, which seems to hint that it's in picoseconds.
>>> And, the kernel doc of the 'clk_pre' member wrongly says the minimum value
>>> is '8 UI', instead of 8.
>>>
>>> So, let's fix both the dphy core driver and the kernel doc of the 'clk_pre'
>>> member to correctly reflect the T-CLK-PRE parameter's unit and the minimum
>>> value according to the D-PHY specification.
>>>
>>> I'm assuming that all impacted custom drivers shall program values in
>>> TxByteClkHS cycles into hardware for the T-CLK-PRE parameter.  The D-PHY
>>> specification mentions that the frequency of TxByteClkHS is exactly 1/8
>>> the High-Speed(HS) bit rate(each HS bit consumes one UI).  So, relevant
>>> custom driver code is changed to program those values as
>>> DIV_ROUND_UP(cfg->clk_pre, MIPI_DPHY_UI_PER_TXBYTECLKHS_PERIOD), then.
>>>
>>> Note that I've only tested the patch with RM67191 DSI panel on i.MX8mq EVK.
>>> Help is needed to test with other i.MX8mq, Meson and Rockchip platforms,
>>> as I don't have the hardwares.
>>>
>>> Fixes: 2ed869990e14 ("phy: Add MIPI D-PHY configuration options")
>>> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
>>> Cc: Neil Armstrong <narmstrong@baylibre.com>
>>> Cc: Robert Foss <robert.foss@linaro.org>
>>> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
>>> Cc: Jonas Karlman <jonas@kwiboo.se>
>>> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
>>> Cc: David Airlie <airlied@linux.ie>
>>> Cc: Daniel Vetter <daniel@ffwll.ch>
>>> Cc: Kishon Vijay Abraham I <kishon@ti.com>
>>> Cc: Vinod Koul <vkoul@kernel.org>
>>> Cc: Kevin Hilman <khilman@baylibre.com>
>>> Cc: Jerome Brunet <jbrunet@baylibre.com>
>>> Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>>> Cc: Heiko Stuebner <heiko@sntech.de>
>>> Cc: Maxime Ripard <mripard@kernel.org>
>>> Cc: Guido Günther <agx@sigxcpu.org>
>>> Tested-by: Liu Ying <victor.liu@nxp.com> # RM67191 DSI panel on i.MX8mq EVK
>>> Signed-off-by: Liu Ying <victor.liu@nxp.com>
>>> ---
>>>    drivers/gpu/drm/bridge/nwl-dsi.c                 | 7 ++-----
>>>    drivers/phy/amlogic/phy-meson-axg-mipi-dphy.c    | 3 ++-
>>>    drivers/phy/phy-core-mipi-dphy.c                 | 4 ++--
>>>    drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c | 3 ++-
>>>    include/linux/phy/phy-mipi-dphy.h                | 4 +++-
>>>    5 files changed, 11 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/nwl-dsi.c b/drivers/gpu/drm/bridge/nwl-dsi.c
>>> index a7389a0facfb..f1fdcdf763ee 100644
>>> --- a/drivers/gpu/drm/bridge/nwl-dsi.c
>>> +++ b/drivers/gpu/drm/bridge/nwl-dsi.c
>>> @@ -196,12 +196,9 @@ static u32 ps2bc(struct nwl_dsi *dsi, unsigned long long ps)
>>>    /*
>>>     * ui2bc - UI time periods to byte clock cycles
>>>     */
>>> -static u32 ui2bc(struct nwl_dsi *dsi, unsigned long long ui)
>>> +static u32 ui2bc(struct nwl_dsi *dsi, unsigned int ui)
>>>    {
>>> -	u32 bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
>>> -
>>> -	return DIV64_U64_ROUND_UP(ui * dsi->lanes,
>>> -				  dsi->mode.clock * 1000 * bpp);
>>> +	return DIV_ROUND_UP(ui, MIPI_DPHY_UI_PER_TXBYTECLKHS_PERIOD);
>> I have some doubts here. According to specs:
>>
>>       UI - duration of any HS state on the Clock Lane,
>>
>>       TxByteClkHS - exactly 1/8 the High-Speed(HS) bit rate
>>
>> I'd like to emphasize "BIT RATE" above (not clock rate).
>>
>> In such case I would expect that ui2bc would take into account number of
>> lanes:
>>
>> byte clock cycles = div_round_up(ui * dsi->lanes, 8)
>>
>> So in case of one lane we need 8 ticks to get byte, and in case of 4
>> lanes 2 ticks are enough.
>>
>> Am I correct, or I've missed sth?
> Sorry, I think you are wrong.
>
> The spec also mentions that it is recommended that all transmitting
> Data Lane Modules share one TxByteClkHS signal.  So, usually,
> TxByteClkHS has nothing to do with data lane number, but only UI - one
> bit period is HS state.  I think the NWL DSI follows the recommended
> implementation.
>
> 'Table 20 HS Transmitter AC Specifications' in the spec notes
> that Applicable when supporting maximum HS bit rates ≤ 1 Gbps (UI ≥ 1
> ns).  This hints that HS bit rate is 1/UI.


OK, apparently "bit rate" in case of PHYs usually means "bit rate per lane".


Regards

Andrzej