Re: [PATCH 0/4] ARM: Renesas: R-Car3: Add product register support

[Dirk Behme <dirk.behme@de.bosch.com>]

Hi Geert,

On 01.06.2016 09:27, Geert Uytterhoeven wrote:
> Hi Dirk,
>
> On Wed, Jun 1, 2016 at 9:19 AM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
>> On 01.06.2016 07:19, Magnus Damm wrote:
>>> On Fri, May 27, 2016 at 4:56 PM, Dirk Behme <dirk.behme@de.bosch.com>
>>> wrote:
>>>> On 27.05.2016 05:13, Magnus Damm wrote:
>>>>> I don't think anyone disagrees that it makes sense to be able to
>>>>> determine ES version during runtime. The questions in my mind are how
>>>>> to do it
>>>>
>>>> I've made a proposal ;) And I'm happy to discuss technically about it.
>>>
>>> Thanks! It would be interesting to know the reason behind your
>>> interest in these things. For instance, if you are interested in
>>> reducing run time memory usage, or general source code duplication
>>> reduction. Do you have any target platform that you can upstream board
>>> support for?
>>>
>>>>> and the urgency.
>>>>
>>>> Regarding the urgency: Someone has accepted the hard coded PRODUCT
>>>> register
>>>> (and MODEMR) being in renesas-drivers, now:
>>>>
>>>> https://git.kernel.org/cgit/linux/kernel/git/geert/renesas-drivers.git/tree/drivers/gpu/drm/rcar-du/rcar_du_crtc.c?h=topic/gen3-latest#n177
>>>>
>>>> This does really hurts us.
>>>>
>>>> Therefore, I try to get this removed as soon as possible to hopefully get
>>>> a
>>>> Renesas BSP without this, the next time.
>>>>
>>>> If anybody finds an other way to remove that, that would be fine, too.
>>>
>>> Please share the underlying issue so we can fix that.
>>>
>>>>> So far we have decided to use the compatible string since it is a
>>>>> common driver model abstraction already used by device drivers and
>>>>> hardware description in DT. Using DT compat string matching we can
>>>>> have logic in the drivers to handle ES differences if needed. As for
>>>>> how to enable the workaround, my opinion is that the missing piece
>>>>> consists of ES workaround code that appends ES suffix information to
>>>>> the DT compat strings automatically during boot.
>>>>
>>>> Technically this sounds slightly too complicated to me. As I have to
>>>> advocate my proposal (inspired from an other mainline SoC family) I'd
>>>> think
>>>> that my proposal is easier.
>>>
>>> It may indeed be easier in the short term, but I feel we need to
>>> consider how to support a wide range of SoCs in a consistent way.
>>>
>>>>> I suppose the workaround is not yet implemented because no one has
>>>>> deemed this topic as particularly urgent. Until it becomes urgent or a
>>>>> new ES version appears the affected users can simply adjust the DT
>>>>> binding themselves. This is what happened to the "sata-r8a7790-es1"
>>>>> case above.
>>>
>>> Can you see any technical reason why using DT compat strings wouldn't
>>> solve your case?
>>
>> I'll try to explain my understanding. Please re-ask or correct me if I fail
>> ;)
>>
>> First of all, I can talk only about R-Car3. There, at the moment, we have
>> ES1, ES2 and ES3 documented, already. We don't know, yet, if these are
>> really needed in the code, but it might be.
>>
>> Taking the example above, we then would have
>>
>> compatible = "renesas,sata-r8a7790", "sata-r8a7790-es1", "sata-r8a7790-es2",
>> "sata-r8a7790-es3";
>>
>> in the device tree. Do we want that? No! Because its no run time detection.
>
> You would only have "renesas,sata-r8a7790" for normal parts.
> ES1 needs "renesas,sata-r8a7790-es1".
> "esX" compatible values are the exception, not the rule.


Yes.


>> If we don't want that, and we want runtime detection, I think anywhere in
>> this thread it was mentioned that the "es1" string is appended to
>> sata-r8a7790 at runtime? If so, could you point us to the code for that?
>
> The PoC is on my local SSD. Will send out soon.
>
>> Then we have anything like
>>
>> if(PRODUCT_REG & ES1 == ES1)
>>         append ES1 to the compatible string
>>
>> in the boot code. This is one additional indirection to my proposal, needing
>> boot time. In the driver you then add
>>
>>         {
>>                 .compatible = "renesas,sata-r8a7790-es1",
>>                 .data = (void *)RCAR_R8A7790_ES1_SATA
>>         },
>>         {
>>                 .compatible = "renesas,sata-r8a7790-es2",
>>                 .data = (void *)RCAR_R8A7790_ES2_SATA
>>         },
>>         {
>>                 .compatible = "renesas,sata-r8a7790-es3",
>>                 .data = (void *)RCAR_R8A7790_ES3_SATA
>>         },
>
> No, only "renesas,sata-r8a7790" and "renesas,sata-r8a7790-es1", like we
> already have.


Sorry, if that was unclear. I took the example and transferred it to 
R-Car3 where we have ES1 - ES3.

So, taking this example, on R-Car3 we might end up with

	{
		.compatible = "renesas,sata-r8a7795",
		.data = (void *)RCAR_GEN2_SATA
	},
	{
		.compatible = "renesas,sata-r8a7795-es1",
		.data = (void *)RCAR_R8A7795_ES1_SATA
	},
	{
		.compatible = "renesas,sata-r8a7795-es2",
		.data = (void *)RCAR_R8A7795_ES2_SATA
	},
	{
		.compatible = "renesas,sata-r8a7795-es3",
		.data = (void *)RCAR_R8A7795_ES3_SATA
	},

?

Best regards

Dirk