Re: [PATCH v3 1/1] regmap: Synchronize cache for the page selector

[Marek Szyprowski <m.szyprowski@samsung.com>]

On 17.01.2025 18:28, Andy Shevchenko wrote:
> On Fri, Jan 17, 2025 at 05:05:42PM +0100, Marek Szyprowski wrote:
>> On 17.01.2025 15:30, Andy Shevchenko wrote:
>>> On Fri, Jan 17, 2025 at 04:09:58PM +0200, Andy Shevchenko wrote:
>>>> On Fri, Jan 17, 2025 at 02:57:52PM +0100, Marek Szyprowski wrote:
>>>>> On 16.01.2025 13:42, Andy Shevchenko wrote:
>>>>>> If the selector register is represented in each page, its value
>>>>>> in accordance to the debugfs is stale because it gets synchronized
>>>>>> only after the real page switch happens. Synchronize cache for
>>>>>> the page selector.
>>>>>>
>>>>>> Before (offset followed by hexdump, the first byte is selector):
>>>>>>
>>>>>>        // Real registers
>>>>>>        18: 05 ff 00 00 ff 0f 00 00 f0 00 00 00
>>>>>>        ...
>>>>>>        // Virtual (per port)
>>>>>>        40: 05 ff 00 00 e0 e0 00 00 00 00 00 1f
>>>>>>        50: 00 ff 00 00 e0 e0 00 00 00 00 00 1f
>>>>>>        60: 01 ff 00 00 ff ff 00 00 00 00 00 00
>>>>>>        70: 02 ff 00 00 cf f3 00 00 00 00 00 0c
>>>>>>        80: 03 ff 00 00 00 00 00 00 00 00 00 ff
>>>>>>        90: 04 ff 00 00 ff 0f 00 00 f0 00 00 00
>>>>>>
>>>>>> After:
>>>>>>
>>>>>>        // Real registers
>>>>>>        18: 05 ff 00 00 ff 0f 00 00 f0 00 00 00
>>>>>>        ...
>>>>>>        // Virtual (per port)
>>>>>>        40: 00 ff 00 00 e0 e0 00 00 00 00 00 1f
>>>>>>        50: 01 ff 00 00 e0 e0 00 00 00 00 00 1f
>>>>>>        60: 02 ff 00 00 ff ff 00 00 00 00 00 00
>>>>>>        70: 03 ff 00 00 cf f3 00 00 00 00 00 0c
>>>>>>        80: 04 ff 00 00 00 00 00 00 00 00 00 ff
>>>>>>        90: 05 ff 00 00 ff 0f 00 00 f0 00 00 00
>>>>>>
>>>>>> Fixes: 6863ca622759 ("regmap: Add support for register indirect addressing.")
>>>>>> Signed-off-by: Andy Shevchenko<andriy.shevchenko@linux.intel.com>
>>>>> This patch landed in linux-next some time ago as commit 1fd60ed1700c
>>>>> ("regmap: Synchronize cache for the page selector"). Today I've noticed
>>>>> that it causes a regression for Lontium LT9611UXC HDMI bridge driver.
>>>> Is there any datasheet link to the HW in question?
>>>>
>>>> (FWIW, I have tested this with the CY8C9540 GPIO I²C expander on Intel Galileo
>>>>    Gen 1 board.)
>>>>
>>>>> With today's linux-next I got the following messages on QCom RB5 board:
>>>>>
>>>>> # dmesg | grep  lt9611uxc
>>>>> [   13.737346] lt9611uxc 5-002b: LT9611 revision: 0x00.00.00
>>>>> [   13.804190] lt9611uxc 5-002b: LT9611 version: 0x00
>>>>> [   13.870564] lt9611uxc 5-002b: FW version 0, enforcing firmware update
>>>>> [   13.877437] lt9611uxc 5-002b: Direct firmware load for
>>>>> lt9611uxc_fw.bin failed with error -2
>>>>> [   13.887517] lt9611uxc 5-002b: probe with driver lt9611uxc failed with
>>>>> error -2
>>>>>
>>>>> after reverting the $subject patch, the driver probes fine on that board.
>>>>>
>>>>> I'm not sure if this is really a bug caused by this change or simply the
>>>>> driver already was aligned to old regmap behavior. Dmitry, could you
>>>>> check the regamp usage and review the changes introduced by this patch?
>>>>> Let me know if there is anything to check on the real hardware to help
>>>>> resolving this issue.
>>>> Yes, see below. And thank you for your report!
> ...
>
>>>>>> +		/*
>>>>>> +		 * If selector register has been just updated, update the respective
>>>>>> +		 * virtual copy as well.
>>>>>> +		 */
>>>>>> +		if (page_chg &&
>>>>>> +		    in_range(range->selector_reg, range->window_start, range->window_len))
>>>>>> +			_regmap_update_bits(map, sel_register, mask, val, NULL, false);
>>>> Can you add a test printk() here to show
>>>>
>>>> page_chg
>>>> range->selector_reg, range->window_start, range->window_len
>>>> sel_register, mask, val
>>>>
>>>> ?
>>>>
>>>> And would commenting these three lines make it work again?
>>> Also try to apply this patch (while having the above lines not commented):
>> This patch applied alone doesn't change anything. Probe still fails.
>>
>> However, with the mentioned 3 lines in the regmap code commented AND
>> this patch applied, lt9611uxc driver probe also fails.
> Does it fail in the same way?

Yes, the hw revision is reported as zero in this case: LT9611 revision: 
0x00.00.00

>> Does it mean that there is really a bug in the driver?
> Without looking at the datasheet it's hard to say. At least what I found so far
> is one page of the I²C traffic dump on Windows as an example how to use their
> evaluation board and software, but it doesn't unveil the bigger picture. At
> least what I think is going on here is that the programming is not so easy as
> just paging. Something is more complicated there.
>
> But at least (and as Mark said) the most of the regmap based drivers got
> the ranges wrong (so, at least there is one bug in the driver).

I can do more experiments if this helps. Do you need a dump of all 
regmap accesses or i2c traffic from this driver?

>>> --- a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
>>> +++ b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
>>> @@ -69,7 +69,7 @@ struct lt9611uxc {
>>>    static const struct regmap_range_cfg lt9611uxc_ranges[] = {
>>>    	{
>>>    		.name = "register_range",
>>> -		.range_min =  0,
>>> +		.range_min = 0x0100,
>>>    		.range_max = 0xd0ff,
>>>    		.selector_reg = LT9611_PAGE_CONTROL,
>>>    		.selector_mask = 0xff,

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland