Message-ID: <b2f909e3-bdee-4b00-97b9-20859c22f60c@linaro.org>
Date: Wed, 22 Jan 2025 23:29:53 +0200
From: Vladimir Zapolskiy <vladimir.zapolskiy@...aro.org>
To: Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
 Robert Foss <rfoss@...nel.org>, Todor Tomov <todor.too@...il.com>,
 Mauro Carvalho Chehab <mchehab@...nel.org>
Cc: linux-media@...r.kernel.org, linux-arm-msm@...r.kernel.org,
 linux-kernel@...r.kernel.org, Depeng Shao <quic_depengs@...cinc.com>,
 Vikram Sharma <quic_vikramsa@...cinc.com>
Subject: Re: [PATCH 6/7] media: qcom: camss: csiphy-3ph: Add 4nm CSIPHY 2ph
 5Gbps DPHY v2.1.2 init sequence

On 1/22/25 15:41, Bryan O'Donoghue wrote:
> On 22/01/2025 00:29, Vladimir Zapolskiy wrote:
>> Hi Bryan.
>>
>> On 1/20/25 17:47, Bryan O'Donoghue wrote:
>>> For various SoC skews at 4nm CSIPHY 2.1.2 is used. Add in the init
>>> sequence
>>> with base control reg offset of 0x1000.
>>>
>>> This initial version will support X1E80100. Take the silicon verification
>>> PHY init parameters as a first/best guess pass.
>>>
>>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
>>> ---
>>>    .../platform/qcom/camss/camss-csiphy-3ph-1-0.c     | 126 +++++++++++
>>> ++++++++++
>>>    1 file changed, 126 insertions(+)
>>>
>>> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
>>> b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
>>> index b44939686e4bb..fc624a3da1c43 100644
>>> --- a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
>>> +++ b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
>>> @@ -55,6 +55,7 @@
>>>    #define CSIPHY_DNP_PARAMS        4
>>>    #define CSIPHY_2PH_REGS            5
>>>    #define CSIPHY_3PH_REGS            6
>>> +#define CSIPHY_SKEW_CAL            7
>>
>> This one is not needed, having CSIPHY_DNP_PARAMS only is good enough.
>>
>>>    struct csiphy_lane_regs {
>>>        s32 reg_addr;
>>> @@ -423,6 +424,130 @@ csiphy_lane_regs lane_regs_sm8550[] = {
>>>        {0x0C64, 0x7F, 0x00, CSIPHY_DEFAULT_PARAMS},
>>>    };
>>> +/* 4nm 2PH v 2.1.2 2p5Gbps 4 lane DPHY mode */
>>> +static const struct
>>> +csiphy_lane_regs lane_regs_x1e80100[] = {
>>> +    /* Power up lanes 2ph mode */
>>> +    {0x1014, 0xD5, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x101C, 0x7A, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x1018, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +
>>> +    {0x0094, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x00A0, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0090, 0x0f, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0098, 0x08, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0094, 0x07, 0x01, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0030, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0000, 0x8E, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0038, 0xFE, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x002C, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0034, 0x0F, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x001C, 0x0A, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0014, 0x60, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x003C, 0xB8, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0004, 0x0C, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0020, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0008, 0x10, 0x00, CSIPHY_SETTLE_CNT_LOWER_BYTE},
>>> +    {0x0010, 0x52, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0094, 0xD7, 0x00, CSIPHY_SKEW_CAL},
>>> +    {0x005C, 0x00, 0x00, CSIPHY_SKEW_CAL},
>>> +    {0x0060, 0xBD, 0x00, CSIPHY_SKEW_CAL},
>>> +    {0x0064, 0x7F, 0x00, CSIPHY_SKEW_CAL},
>>> +    {0x0064, 0x7F, 0x00, CSIPHY_SKEW_CAL},
>>
>> Double write record, which is anyway ignored, but one should
>> be enough.
> 
> Yes except having the SKEW_CAL definition allows us to import the
> downstream init sequence unmodified.

It's not a technical benefit at all, the expectation is that the upstream
code shall be better than the downstream code.



>>> +
>>> +    {0x0E94, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0EA0, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0E90, 0x0f, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0E98, 0x08, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0E94, 0x07, 0x01, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0E30, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0E28, 0x04, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0E00, 0x80, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0E0C, 0xFF, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0E38, 0x1F, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0E2C, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0E34, 0x0F, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0E1C, 0x0A, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0E14, 0x60, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0E3C, 0xB8, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0E04, 0x0C, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0E20, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0E08, 0x10, 0x00, CSIPHY_SETTLE_CNT_LOWER_BYTE},
>>> +    {0x0E10, 0x52, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0E10, 0x52, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0E10, 0x52, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0E10, 0x52, 0x00, CSIPHY_DEFAULT_PARAMS},
>>
>> Writing the same value to a register 4 times in a row, apparently
>> it's not needed, one time write is sufficient.
> 
> To be honest I just took the downstream sequence verbatim.
> 

I believe this could be a support to my words above, quite often
the downstream code is not good enough quality to be copied blindly.

> I'll see if the 4 x has an effect though.
> 

Thank you in advance!

>>> +
>>> +    {0x0494, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x04A0, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0490, 0x0f, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0498, 0x08, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0494, 0x07, 0x01, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0430, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0400, 0x8E, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0438, 0xFE, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x042C, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0434, 0x0F, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x041C, 0x0A, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0414, 0x60, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x043C, 0xB8, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0404, 0x0C, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0420, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0408, 0x10, 0x00, CSIPHY_SETTLE_CNT_LOWER_BYTE},
>>> +    {0x0410, 0x52, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0494, 0xD7, 0x00, CSIPHY_SKEW_CAL},
>>> +    {0x045C, 0x00, 0x00, CSIPHY_SKEW_CAL},
>>> +    {0x0460, 0xBD, 0x00, CSIPHY_SKEW_CAL},
>>> +    {0x0464, 0x7F, 0x00, CSIPHY_SKEW_CAL},
>>> +    {0x0464, 0x7F, 0x00, CSIPHY_SKEW_CAL},
>>
>> Two equal "ignored" writes.
> 
> Again I think these init sequences "do no harm" and its at least
> possible we can improve the logic of our upstream init sequences to make
> these NOPs mean more...
> 
> At they very least they consume time in the APSS wrt the next writes..

I disagree about "do no harm" part, since it caused a severe confusion
on my reader's side, and the next reader will return with the same
currently highlighted problem, if there is a mistake or what the hidden
sense is here.

Here I ask to remove the unused code along with the confusion it brings.

>>
>>> +
>>> +    {0x0894, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x08A0, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0890, 0x0f, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0898, 0x08, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0894, 0x07, 0x01, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0830, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0800, 0x8E, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0838, 0xFE, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x082C, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0834, 0x0F, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x081C, 0x0A, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0814, 0x60, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x083C, 0xB8, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0804, 0x0C, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0820, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0808, 0x10, 0x00, CSIPHY_SETTLE_CNT_LOWER_BYTE},
>>> +    {0x0810, 0x52, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0894, 0xD7, 0x00, CSIPHY_SKEW_CAL},
>>> +    {0x085C, 0x00, 0x00, CSIPHY_SKEW_CAL},
>>> +    {0x0860, 0xBD, 0x00, CSIPHY_SKEW_CAL},
>>> +    {0x0864, 0x7F, 0x00, CSIPHY_SKEW_CAL},
>>> +    {0x0864, 0x7F, 0x00, CSIPHY_SKEW_CAL},
>>
>> Two equal "ignored" writes.
>>
>>> +
>>> +    {0x0C94, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0CA0, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0C90, 0x0f, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0C98, 0x08, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0C94, 0x07, 0x01, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0C30, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0C00, 0x8E, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0C38, 0xFE, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0C2C, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0C34, 0x0F, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0C1C, 0x0A, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0C14, 0x60, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0C3C, 0xB8, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0C04, 0x0C, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0C20, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0C08, 0x10, 0x00, CSIPHY_SETTLE_CNT_LOWER_BYTE},
>>> +    {0x0C10, 0x52, 0x00, CSIPHY_DEFAULT_PARAMS},
>>> +    {0x0C94, 0xD7, 0x00, CSIPHY_SKEW_CAL},
>>> +    {0x0C5C, 0x00, 0x00, CSIPHY_SKEW_CAL},
>>> +    {0x0C60, 0xBD, 0x00, CSIPHY_SKEW_CAL},
>>> +    {0x0C64, 0x7F, 0x00, CSIPHY_SKEW_CAL},
>>> +    {0x0C64, 0x7F, 0x00, CSIPHY_SKEW_CAL},
>>
>> Two equal "ignored" writes.
>>
>>> +};
>>> +
>>>    static void csiphy_hw_version_read(struct csiphy_device *csiphy,
>>>                       struct device *dev)
>>>    {
>>> @@ -594,6 +719,7 @@ static void csiphy_gen2_config_lanes(struct
>>> csiphy_device *csiphy,
>>>                val = settle_cnt & 0xff;
>>>                break;
>>>            case CSIPHY_DNP_PARAMS:
>>> +        case CSIPHY_SKEW_CAL:
>>
>> Having CSIPHY_DNP_PARAMS is good enough, no need to add another
>> "dummy" write type.
> 
> True but, I'd like to be able to bring in unmodified init sequences from
> downstream.

It might be called reluctance, it's not a good enough reason for the
upstream code, I believe.

> I think there is value in being able to setup the PHYs in the exact same
> configuration.

Removing non-writes and/or using everywhere only one of two types
CSIPHY_DNP_PARAMS/CSIPHY_SKEW_CAL does not change the given by you
point, it's still a setup of "PHYs in the exact same configuration".

> So, I think we should keep the SKEW_CAL support and I'm open to
> experiment reducing repeated DNP/SKEW downwards, perhaps defining a real
> number for the delay instead.
> 

I care a lot about making quite low quality source code of the driver more
simple and comprehensible, while optimization of time needed to copy code
from the downstream could be a movement in quite the opposite direction.

I won't object, if you replace CSIPHY_DNP_PARAMS with CSIPHY_SKEW_CAL in
the driver, but there is no technical reason to keep both types. Again,
I do recognize the non-technical reason to simplify copying from the
downstream, I just claim it as a non-technical reason, which is out of
the engineering duty.

--
Best wishes,
Vladimir

Powered by blists - more mailing lists