Message-Id: <RIL92R.MLAZ6CTO865E1@crapouillou.net>
Date:   Mon, 08 Nov 2021 17:49:39 +0000
From:   Paul Cercueil <paul@...pouillou.net>
To:     "H. Nikolaus Schaller" <hns@...delico.com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Kees Cook <keescook@...omium.org>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Miquel Raynal <miquel.raynal@...tlin.com>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Neil Armstrong <narmstrong@...libre.com>,
        Robert Foss <robert.foss@...aro.org>,
        Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        Harry Wentland <harry.wentland@....com>,
        Sam Ravnborg <sam@...nborg.org>,
        Maxime Ripard <maxime@...no.tech>,
        Hans Verkuil <hverkuil-cisco@...all.nl>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Paul Boddie <paul@...die.org.uk>,
        OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS 
        <devicetree@...r.kernel.org>,
        linux-mips <linux-mips@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Discussions about the Letux Kernel 
        <letux-kernel@...nphoenux.org>, Jonas Karlman <jonas@...boo.se>,
        dri-devel <dri-devel@...ts.freedesktop.org>
Subject: Re: [PATCH v5 2/7] drm/ingenic: Add support for JZ4780 and HDMI
 output

Hi,

Le lun., nov. 8 2021 at 18:22:58 +0100, H. Nikolaus Schaller 
<hns@...delico.com> a écrit :
> Hi Paul,
> 
>>  Am 08.11.2021 um 17:30 schrieb Paul Cercueil <paul@...pouillou.net>:
>> 
>>  Hi,
>> 
>>  Le lun., nov. 8 2021 at 16:29:11 +0100, H. Nikolaus Schaller 
>> <hns@...delico.com> a écrit :
>>>  Bnjour Paul,
>>>>  Am 08.11.2021 um 13:20 schrieb Paul Cercueil 
>>>> <paul@...pouillou.net>:
>>>>  Hi,
>>>>>  e.g. jz4770.dtsi:
>>>>>  	lcd: lcd-controller@...50000 {
>>>>>  		compatible = "ingenic,jz4770-lcd";
>>>>>  		reg = <0x13050000 0x300>;
>>>>>  or jz4725b.dtsi:
>>>>>  	lcd: lcd-controller@...50000 {
>>>>>  		compatible = "ingenic,jz4725b-lcd";
>>>>>  		reg = <0x13050000 0x1000>;
>>>>>  So max_register becomes 0x300 or 0x1000 but not
>>>>>  #define JZ_REG_LCD_SIZE1	0x12c
>>>>>  	.max_reg = JZ_REG_LCD_SIZE1,
>>>>>  And therefore wastes a lot of regmap memory.
>>>>  "regmap memory"? ...
>>>  regmap allocates memory for its cache. Usually the total amount 
>>> specified in the reg property.
>> 
>>  We are not using any register cache here.
>> 
>>>>>  Do you want this? DTS should not be reduced (DTS should be kept 
>>>>> as stable as possible), since the reg property describes address 
>>>>> mapping - not how many bytes are really used by registers or how 
>>>>> big a cache should be allocated (cache allocation size 
>>>>> requirements are not hardware description).
>>>>  The DTS should list the address and size of the register area. If 
>>>> your last register is at address 0x12c and there's nothing above, 
>>>> then the size in DTS should be 0x130.
>>>  If I look into some .dtsi it is sometimes that way but sometimes 
>>> not. There seems to be no consistent rule.
>>>  So does this mean you allow me to modify jz4740.dtsi, jz4770.dtsi 
>>> and jz4725b.dtsi as well (as mentioned above: this is beyond the 
>>> scope of my project)?
>> 
>>  You could update them if you wanted to, but there is no need to do 
>> it here.
> 
> Hm. Then we are changing the .max_register initialization to a much 
> bigger value.
> 
>> 
>>>>>  But here are good news:
>>>>>  I have a simpler and less invasive proposal. We keep the 
>>>>> devm_regmap_init_mmio code as is and just increase its 
>>>>> .max_register from JZ_REG_LCD_SIZE1 to JZ_REG_LCD_PCFG when 
>>>>> introducing the jz4780. This wastes a handful bytes for all 
>>>>> non-jz4780 chips but less than using the DTS memory region size. 
>>>>> And is less code (no entry in soc_info tables, no modifyable 
>>>>> copy) and faster code execution than all other proposals.
>>>>>  This is then just a single-line change when introducing the 
>>>>> jz4780. And no "preparation for adding jz4780" patch is needed at 
>>>>> all. No patch to split out for separate review.
>>>>>  Let's go this way to get it eventually finalized. Ok?
>>>>  No.
>>>  Look friend, if you explain your "no" and what is wrong with my 
>>> arguments, it helps to understand your decisions and learn 
>>> something from them. A plain "no" does not help anyone.
>> 
>>  I answered just "no" because I felt like I explained already what I 
>> wanted to see in the previous email.
>> 
>>  By using a huge number as the .max_register, we do *not* waste 
>> additional memory. Computing the value of the .max_register field 
>> does not add any overhead, either.
>> 
>>  The .max_register is only used for boundary checking. To make sure 
>> that you're not calling regmap_write() with an invalid register. 
>> That's all there is to it.
> 
> Ah, now I understand our disconnect. So far I have used regmaps 
> mainly for i2c devices and there is caching to avoid redundant i2c 
> traffic...
> 
> So I just assumed wrongly that the regmap driver also allocates some 
> buffer/cache here. Although it does not initialize .cache_type 
> (default: REGCACHE_NONE).
> 
>> 
>>>  So to summarize: if you prefer something which I consider worse, 
>>> it is ok for me... In the end you are right - you are the 
>>> maintainer, not me. So you have to live with your proposals.
>>>  Therefore, I have prepared new variants so you can choose which 
>>> one is easier to maintain for you.
>>>  Note that they are both preparing for full jz4780-lcdc/hdmi 
>>> support but in very different ways:
>>>  Variant 1 already adds some jz4780 stuff while Variant 2 just 
>>> prepares for it.
>>>  Variant 2 is not tested (except to compile). So it needs some 
>>> Tested-by: from someone with access to hardware. IMHO it is more 
>>> invasive.
>>>  And don't forget: DTB could be in ROM or be provided by a separate 
>>> bootloader... So we should not change it too often (I had such 
>>> discussions some years ago with maintainers when I thought it is 
>>> easier to change DTS instead of code).
>>>  Variant 3 would be to not separate this. As proposed in [PATCH v5 
>>> 2/7].
>>>  (Finally, a Variant 3b would be to combine the simple change from 
>>> Variant 1 with Variant 3).
>>>  So what is your choice?
>> 
>>  Variant 4: the variant #2 without the changes to the DTSI files.
> 
> Hm. If there is no cache and we can safely remove tight boundary 
> checking (by JZ_REG_LCD_SIZE1) for jz4725/40/70 (by not fixing DTSI) 
> why do we still need the max_register calculation from DTSI 
> specifically for jz4780 and at all?

It's better to have the .max_register actually set to the proper value. 
Then reading the registers from debugfs (/sys/kernel/debug/regmap/) 
will print the actual list of registers without bogus values. If 
.max_register is set too high, it will end up reading outside the 
registers area. On Ingenic SoCs such reads just return 0, but on some 
other SoCs it can lock up the system.

So the best way forward is to have .max_register computed from the 
register area's size, and fix the DTSI with the proper sizes. Since 
your JZ4780 code needs to update .max_register anyway it's a good 
moment to add this patch, and the DTSI files can be fixed later (by me 
or whoever is up to the task).

Fixing the DTS is not a problem in any way, btw. We just need to ensure 
that the drivers still work with old DTB files, which will be the case 
here.

-Paul

> So what about:
> 
> Variant 5: set .max_register = 0x1800, i.e. "big enough for everyone" 
> (includes z4780 gamma and vee registers) + no DTSI changes (+ no 
> jz4780 register constants like Variant 1)
> 
> + no DTSI changes
> + no calculation from DTSI needed
> + single separate patch to prepare for jz4780 but not included in 
> jz4780 patch
> 
> BR and thanks,
> Nikolaus
> 
> 

Powered by blists - more mailing lists