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

[Dirk Behme]

Hi Magnus,

On 01.06.2016 07:19, Magnus Damm wrote:

Hi Dirk,

On Fri, May 27, 2016 at 4:56 PM, Dirk Behme  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.



Regarding the general removal of hard coded registers:

This breaks virtualization. If you use a virtualization solution, e.g. 
Xen, which does the memory access rights based on the device tree, each 
peripheral memory access done by the Linux kernel is trapped by the 
hypervisor, then. Because it doesn't know anything about the memory 
range accessed.



Regarding the PRODUCTREG specifically, we already see that it's used for 
engineering sample (ES) detection in some drivers. So not having a 
(device tree) based solution for this, does block all driver development 
(upstreaming) where some ES detection might be needed.




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.



See above.



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?



It most probably will work technically, but I see a lot overhead 
regarding boot time and maintenance in the proposed DT compat strings 
solution.


That solution would mean that for each ESx "workaround" we have to do in 
a driver


a) the generic detection code has to be extended to create the driver 
specific *and* ESx specific compatible node


b) the driver has to be extended to contain the new runtime compatible node

c) the driver has to be extended to handle the ESx specific "workaround"

d) the device tree documentation has to be extended for the new compatible

These are the implementation / maintenance steps. Additionally, 
regarding the runtime (boot time does matter!) this does mean that at 
kernel's boot time the ESx information from the hardware register is 
"translated" into a device tree compatible, this is added to the device 
tree at runtime, the device driver has to check the device tree and then 
apply the specific ESx handling.


With my solution, from above list only (c) has to be done. That means 
that you don't have to touch any generic code. And the driver has just 
to call one function.


If you like, compare the mainline i.MX6 way of doing this.


Best regards

Dirk

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

[Dirk Behme]

Hi Geert,

On 01.06.2016 10:26, Geert Uytterhoeven wrote:

Hi Dirk,

On Wed, Jun 1, 2016 at 9:38 AM, Dirk Behme  wrote:

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
},


Are there known differences in the SATA implementations on R-Car H3 ES1, ES2,
and ES3?



It's just an *example* what *might* happen.



BTW having different compatible values in the driver makes it much easier to
audit kernel sources for support for ESx deviations.



Why?

Having these compatible values in the driver doesn't tell you if the 
drivers uses them at all. No?


So you have to look into the driver code (not the compatible values) if 
and how the ESx are handled. And there, I don't think it makes any 
difference if you look for


if (priv->type == RCAR_R8A7790_ES1_SATA)
ap->flags|= ATA_FLAG_NO_DIPM;

or

if (revision_is_rcar2_es1)
ap->flags|= ATA_FLAG_NO_DIPM;

?


Best regards

Dirk

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

[Dirk Behme]

On 01.06.2016 10:40, Geert Uytterhoeven wrote:

Hi Dirk,

On Wed, Jun 1, 2016 at 10:36 AM, Dirk Behme  wrote:

On 01.06.2016 10:26, Geert Uytterhoeven wrote:

On Wed, Jun 1, 2016 at 9:38 AM, Dirk Behme 
wrote:

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
},


Are there known differences in the SATA implementations on R-Car H3 ES1,
ES2,
and ES3?


It's just an *example* what *might* happen.


BTW having different compatible values in the driver makes it much easier
to
audit kernel sources for support for ESx deviations.


Why?

Having these compatible values in the driver doesn't tell you if the drivers
uses them at all. No?


The clue is that we do not add "renesas,*-es%u" compatible values to
drivers, unless ES%u needs to be handled specially.


So you have to look into the driver code (not the compatible values) if and
how the ESx are handled. And there, I don't think it makes any difference if
you look for

if (priv->type == RCAR_R8A7790_ES1_SATA)
ap->flags   |= ATA_FLAG_NO_DIPM;

or

if (revision_is_rcar2_es1)
ap->flags   |= ATA_FLAG_NO_DIPM;

?


... so 'git grep "renesas,.*-es[0-9]"' is all you need.



'git grep "revision_is_rcar2_es[0-9]"'

;)

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

[Geert Uytterhoeven]

Hi Dirk,

On Wed, Jun 1, 2016 at 10:36 AM, Dirk Behme  wrote:
> On 01.06.2016 10:26, Geert Uytterhoeven wrote:
>> On Wed, Jun 1, 2016 at 9:38 AM, Dirk Behme 
>> wrote:
>>> 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
>>> },
>>
>> Are there known differences in the SATA implementations on R-Car H3 ES1,
>> ES2,
>> and ES3?
>
> It's just an *example* what *might* happen.
>
>> BTW having different compatible values in the driver makes it much easier
>> to
>> audit kernel sources for support for ESx deviations.
>
> Why?
>
> Having these compatible values in the driver doesn't tell you if the drivers
> uses them at all. No?

The clue is that we do not add "renesas,*-es%u" compatible values to
drivers, unless ES%u needs to be handled specially.

> So you have to look into the driver code (not the compatible values) if and
> how the ESx are handled. And there, I don't think it makes any difference if
> you look for
>
> if (priv->type == RCAR_R8A7790_ES1_SATA)
> ap->flags   |= ATA_FLAG_NO_DIPM;
>
> or
>
> if (revision_is_rcar2_es1)
> ap->flags   |= ATA_FLAG_NO_DIPM;
>
> ?

... so 'git grep "renesas,.*-es[0-9]"' is all you need.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

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

[Geert Uytterhoeven]

Hi Dirk,

On Wed, Jun 1, 2016 at 9:38 AM, Dirk Behme  wrote:
> 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
> },

Are there known differences in the SATA implementations on R-Car H3 ES1, ES2,
and ES3? If not, just "renesas,sata-r8a7795" will do.

BTW having different compatible values in the driver makes it much easier to
audit kernel sources for support for ESx deviations.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

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

[Dirk Behme]

Hi Geert,

On 01.06.2016 09:27, Geert Uytterhoeven wrote:

Hi Dirk,

On Wed, Jun 1, 2016 at 9:19 AM, Dirk Behme  wrote:

On 01.06.2016 07:19, Magnus Damm wrote:

On Fri, May 27, 2016 at 4:56 PM, Dirk Behme 
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

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

[Geert Uytterhoeven]

On Fri, May 27, 2016 at 10:44 AM, Dirk Behme  wrote:
> Btw, what's about the MODEMR patch series? As it's from you and there have
> been no further comments with v3, I'm somehow hoping to get it integrated.

I'll be working on it soon.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

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

[Dirk Behme]

On 27.05.2016 10:39, Geert Uytterhoeven wrote:

On Fri, May 27, 2016 at 9:56 AM, Dirk Behme  wrote:

Given the use of PRR you saw is in a patch that's definitely not ready for
upstream, I think adding a full-fledged PRR driver is a bit premature.



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.


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


I will drop the topic branch topic/salvator-x-hdmi-prototype-v2-rebased1
from next renesas-drivers release, as it's definitely not ready.

People who want to use that branch can still merge it themselves.



Ok, fine, thanks. But maybe it makes sense to continue to discuss about 
the SoC/revision detection independently, then. Maybe with less urgency ;)



Btw, what's about the MODEMR patch series? As it's from you and there 
have been no further comments with v3, I'm somehow hoping to get it 
integrated.



Best regards

Dirk

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

[Magnus Damm]

Hi Dirk,

On Thu, May 26, 2016 at 4:48 PM, Dirk Behme  wrote:
> Hi Geert,
>
> On 26.05.2016 09:14, Geert Uytterhoeven wrote:
>>
>> Hi Dirk,
>>
>> On Wed, May 25, 2016 at 10:58 AM, Dirk Behme 
>> wrote:
>>>
>>> Instead of hard coding the product register in rcar_du_crtc.c,
>>> read it based on the device tree.
>>>
>>> This patch series is based on
>>>
>>>
>>> https://git.kernel.org/cgit/linux/kernel/git/geert/renesas-drivers.git/log/?h=topic/gen3-latest
>>> renesas-drivers-2016-05-17-v4.6
>>> 4717ef6d59c3204e385c
>>> Revert "ASoC: simple-card: Add pm callbacks to platform driver"
>>>
>>> It's boot tested on r8a7795 Salvator-X and checked that the same
>>> product register value is read without and with this patch series.
>>
>>
>> Thanks for your series!
>>
>> Given the use of PRR you saw is in a patch that's definitely not ready for
>> upstream, I think adding a full-fledged PRR driver is a bit premature.
>>
>> So far we've always planned to handle differences in ES versions using the
>> compatible value, cfr. "renesas,sata-r8a7790-es1".
>> But in general we target the latest (production) version in upstream.
>
> Well, there are several issues ;)
>
> First, regarding "handle differences in ES versions using the compatible
> value, cfr. "renesas,sata-r8a7790-es1": I can't talk about r8a7790. But for
> r8a7795 & r8a7796 we are able to auto-detect this by the PRR. And I'd think
> what can be auto detected, should be auto detected. And not handled via the
> device tree.
>
> Second: Other SoCs families show us that we definitely need such kind of
> cpu_is() and revision_is() logic.

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 and the urgency.

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.

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.

Thanks,

/ magnus

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

[Dirk Behme]

Hi Geert,

On 26.05.2016 09:14, Geert Uytterhoeven wrote:

Hi Dirk,

On Wed, May 25, 2016 at 10:58 AM, Dirk Behme  wrote:

Instead of hard coding the product register in rcar_du_crtc.c,
read it based on the device tree.

This patch series is based on

https://git.kernel.org/cgit/linux/kernel/git/geert/renesas-drivers.git/log/?h=topic/gen3-latest
renesas-drivers-2016-05-17-v4.6
4717ef6d59c3204e385c
Revert "ASoC: simple-card: Add pm callbacks to platform driver"

It's boot tested on r8a7795 Salvator-X and checked that the same
product register value is read without and with this patch series.


Thanks for your series!

Given the use of PRR you saw is in a patch that's definitely not ready for
upstream, I think adding a full-fledged PRR driver is a bit premature.

So far we've always planned to handle differences in ES versions using the
compatible value, cfr. "renesas,sata-r8a7790-es1".
But in general we target the latest (production) version in upstream.



Well, there are several issues ;)


First, regarding "handle differences in ES versions using the compatible 
value, cfr. "renesas,sata-r8a7790-es1": I can't talk about r8a7790. But 
for r8a7795 & r8a7796 we are able to auto-detect this by the PRR. And 
I'd think what can be auto detected, should be auto detected. And not 
handled via the device tree.



Second: Other SoCs families show us that we definitely need such kind of 
cpu_is() and revision_is() logic.



Third: Regarding "But in general we target the latest (production) 
version in upstream": Please keep in mind how Renesas is doing their 
BSPs. For the Renesas BSPs, Renesas picks your renesas-drivers (and not 
upstream!). I.e. what ever is in your renesas-drivers might end up in a 
customer BSP. And this is what really hurts us (as a customer) here.


Now, we have a Renesas BSP, based on your renesas-drivers, which has a 
hard coded MODEMR and PRODUCT_REG (and I think ADVFS, but that seems to 
be from Renesas and not from renesas-drivers). And this really hurts us. 
Therefore I try to fix this in your renesas-drivers, to hopefully have 
it fixed in the next Renesas BSP. *And* discuss with the community how 
to further improve it.


I.e. in a frist step, I'm mainly thinking about getting renesas-drivers 
"fixed" and with this the next Renesas BSP.


Best regards

Dirk

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

[Geert Uytterhoeven]

Hi Dirk,

On Wed, May 25, 2016 at 10:58 AM, Dirk Behme  wrote:
> Instead of hard coding the product register in rcar_du_crtc.c,
> read it based on the device tree.
>
> This patch series is based on
>
> https://git.kernel.org/cgit/linux/kernel/git/geert/renesas-drivers.git/log/?h=topic/gen3-latest
> renesas-drivers-2016-05-17-v4.6
> 4717ef6d59c3204e385c
> Revert "ASoC: simple-card: Add pm callbacks to platform driver"
>
> It's boot tested on r8a7795 Salvator-X and checked that the same
> product register value is read without and with this patch series.

Thanks for your series!

Given the use of PRR you saw is in a patch that's definitely not ready for
upstream, I think adding a full-fledged PRR driver is a bit premature.

So far we've always planned to handle differences in ES versions using the
compatible value, cfr. "renesas,sata-r8a7790-es1".
But in general we target the latest (production) version in upstream.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds