[PATCH] arm64: dts: r8a7795: Correct SATA device size to 2MiB

[PATCH] arm64: dts: r8a7795: Correct SATA device size to 2MiB

[Magnus Damm]

From: Magnus Damm <damm+renesas@opensource.se>

Update the r8a7795 SATA device node to use a 2MiB I/O space as specified
in the "72. Serial-ATA" section of R-Car-Gen3-rev0.52E.pdf

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 Developed on top of renesas-devel-20170313-v4.11-rc2

 arch/arm64/boot/dts/renesas/r8a7795.dtsi |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- 0001/arch/arm64/boot/dts/renesas/r8a7795.dtsi
+++ work/arch/arm64/boot/dts/renesas/r8a7795.dtsi	2017-03-20 17:41:36.390607110 +0900
@@ -1209,7 +1209,7 @@
 
 		sata: sata@ee300000 {
 			compatible = "renesas,sata-r8a7795";
-			reg = <0 0xee300000 0 0x1fff>;
+			reg = <0 0xee300000 0 0x200000>;
 			interrupts = <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&cpg CPG_MOD 815>;
 			power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;

Re: [PATCH] arm64: dts: r8a7795: Correct SATA device size to 2MiB

[Geert Uytterhoeven]

Hi Magnus,

On Mon, Mar 20, 2017 at 9:49 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
> From: Magnus Damm <damm+renesas@opensource.se>
>
> Update the r8a7795 SATA device node to use a 2MiB I/O space as specified
> in the "72. Serial-ATA" section of R-Car-Gen3-rev0.52E.pdf
>
> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

> --- 0001/arch/arm64/boot/dts/renesas/r8a7795.dtsi
> +++ work/arch/arm64/boot/dts/renesas/r8a7795.dtsi       2017-03-20 17:41:36.390607110 +0900
> @@ -1209,7 +1209,7 @@
>
>                 sata: sata@ee300000 {
>                         compatible = "renesas,sata-r8a7795";
> -                       reg = <0 0xee300000 0 0x1fff>;
> +                       reg = <0 0xee300000 0 0x200000>;

While the datasheet does mention the 2 MiB area, it also says no (write)
access should be made to registers not listed in the table, while these are
all covered by the existing area?

BTW, what about the Reference Clock Source Select Register, which lies
in a further undocumented area?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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] arm64: dts: r8a7795: Correct SATA device size to 2MiB

[Magnus Damm]

Hi Geert,

On Mon, Mar 20, 2017 at 7:57 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> Hi Magnus,
>
> On Mon, Mar 20, 2017 at 9:49 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
>> From: Magnus Damm <damm+renesas@opensource.se>
>>
>> Update the r8a7795 SATA device node to use a 2MiB I/O space as specified
>> in the "72. Serial-ATA" section of R-Car-Gen3-rev0.52E.pdf
>>
>> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thanks.

>> --- 0001/arch/arm64/boot/dts/renesas/r8a7795.dtsi
>> +++ work/arch/arm64/boot/dts/renesas/r8a7795.dtsi       2017-03-20 17:41:36.390607110 +0900
>> @@ -1209,7 +1209,7 @@
>>
>>                 sata: sata@ee300000 {
>>                         compatible = "renesas,sata-r8a7795";
>> -                       reg = <0 0xee300000 0 0x1fff>;
>> +                       reg = <0 0xee300000 0 0x200000>;
>
> While the datasheet does mention the 2 MiB area, it also says no (write)
> access should be made to registers not listed in the table, while these are
> all covered by the existing area?

That bit about not writing to non-listed registers seems like just
common sense to me, but perhaps there is more to the story than just
that? Like you say it is probably possible to use the driver with the
existing 8K-1 size, but in my mind we should use window sizes defined
in the data sheet for DT?

> BTW, what about the Reference Clock Source Select Register, which lies
> in a further undocumented area?

Yeah, no idea. This would be good task for the I/O or Core group to
figure out how to handle. I'm surprised that the SATA DT device nodes
with the strange and incorrect 0x1fff size got merged upstream without
anyone thinking about that register that you are mentioning. Seems
like supporting that should be part of SATA development for R-Car
Gen3?

Thanks,

/ magnus

Re: [PATCH] arm64: dts: r8a7795: Correct SATA device size to 2MiB

[Geert Uytterhoeven]

Hi Magnus,

On Tue, Mar 21, 2017 at 8:17 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
> On Mon, Mar 20, 2017 at 7:57 PM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>> On Mon, Mar 20, 2017 at 9:49 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
>>> --- 0001/arch/arm64/boot/dts/renesas/r8a7795.dtsi
>>> +++ work/arch/arm64/boot/dts/renesas/r8a7795.dtsi       2017-03-20 17:41:36.390607110 +0900
>>> @@ -1209,7 +1209,7 @@
>>>
>>>                 sata: sata@ee300000 {
>>>                         compatible = "renesas,sata-r8a7795";
>>> -                       reg = <0 0xee300000 0 0x1fff>;
>>> +                       reg = <0 0xee300000 0 0x200000>;
>>
>> While the datasheet does mention the 2 MiB area, it also says no (write)
>> access should be made to registers not listed in the table, while these are
>> all covered by the existing area?
>
> That bit about not writing to non-listed registers seems like just
> common sense to me, but perhaps there is more to the story than just

Sure.

> that? Like you say it is probably possible to use the driver with the
> existing 8K-1 size, but in my mind we should use window sizes defined
> in the data sheet for DT?

The 2 MiB window size is a lot larger than needed to cover all documented
rregisters. Either there are more undocumented registers, or the hardware
engineers were lazy and just decoded the full 2 MiB block.

>> BTW, what about the Reference Clock Source Select Register, which lies
>> in a further undocumented area?
>
> Yeah, no idea. This would be good task for the I/O or Core group to

SATA is I/O.

> figure out how to handle. I'm surprised that the SATA DT device nodes
> with the strange and incorrect 0x1fff size got merged upstream without
> anyone thinking about that register that you are mentioning. Seems
> like supporting that should be part of SATA development for R-Car
> Gen3?

Probably it was just an oversight. I almost missed it myself when
reviewing your patch.

The register is not present (not documented) on R-Car H1 and Gen2.
The IP core is derived from SH-Navi2G (sh7775), but no datasheet.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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] arm64: dts: r8a7795: Correct SATA device size to 2MiB

[Magnus Damm]

Hi Geert,

On Tue, Mar 21, 2017 at 6:30 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> Hi Magnus,
>
> On Tue, Mar 21, 2017 at 8:17 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
>> On Mon, Mar 20, 2017 at 7:57 PM, Geert Uytterhoeven
>> <geert@linux-m68k.org> wrote:
>>> On Mon, Mar 20, 2017 at 9:49 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
>>>> --- 0001/arch/arm64/boot/dts/renesas/r8a7795.dtsi
>>>> +++ work/arch/arm64/boot/dts/renesas/r8a7795.dtsi       2017-03-20 17:41:36.390607110 +0900
>>>> @@ -1209,7 +1209,7 @@
>>>>
>>>>                 sata: sata@ee300000 {
>>>>                         compatible = "renesas,sata-r8a7795";
>>>> -                       reg = <0 0xee300000 0 0x1fff>;
>>>> +                       reg = <0 0xee300000 0 0x200000>;
>>>
>>> While the datasheet does mention the 2 MiB area, it also says no (write)
>>> access should be made to registers not listed in the table, while these are
>>> all covered by the existing area?
>>
>> That bit about not writing to non-listed registers seems like just
>> common sense to me, but perhaps there is more to the story than just
>
> Sure.
>
>> that? Like you say it is probably possible to use the driver with the
>> existing 8K-1 size, but in my mind we should use window sizes defined
>> in the data sheet for DT?
>
> The 2 MiB window size is a lot larger than needed to cover all documented
> rregisters. Either there are more undocumented registers, or the hardware
> engineers were lazy and just decoded the full 2 MiB block.

Yeah, and on top of that it looks to me like the size is not aligned
to the offset either. I would expect a 2MiB block to be aligned to a
2MiB boundary...

>>> BTW, what about the Reference Clock Source Select Register, which lies
>>> in a further undocumented area?
>>
>> Yeah, no idea. This would be good task for the I/O or Core group to
>
> SATA is I/O.

For sure, however I wonder how the external clock register REFSEL is
supposed to be handled. It looks like an external hardware block
somehow.

>> figure out how to handle. I'm surprised that the SATA DT device nodes
>> with the strange and incorrect 0x1fff size got merged upstream without
>> anyone thinking about that register that you are mentioning. Seems
>> like supporting that should be part of SATA development for R-Car
>> Gen3?
>
> Probably it was just an oversight. I almost missed it myself when
> reviewing your patch.

Probably yes.

> The register is not present (not documented) on R-Car H1 and Gen2.
> The IP core is derived from SH-Navi2G (sh7775), but no datasheet.

On R-Car Gen3 there seem to be a chance that random glue hardware for
other devices may also be present in the 0xe65exxxx range however
documentations seem to be lacking...

/ magnus

Re: [PATCH] arm64: dts: r8a7795: Correct SATA device size to 2MiB

[Geert Uytterhoeven]

Hi Magnus,

On Thu, Mar 23, 2017 at 9:30 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
> On Tue, Mar 21, 2017 at 6:30 PM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>> On Tue, Mar 21, 2017 at 8:17 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
>>> On Mon, Mar 20, 2017 at 7:57 PM, Geert Uytterhoeven
>>> <geert@linux-m68k.org> wrote:
>>>> On Mon, Mar 20, 2017 at 9:49 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
>>>>> --- 0001/arch/arm64/boot/dts/renesas/r8a7795.dtsi
>>>>> +++ work/arch/arm64/boot/dts/renesas/r8a7795.dtsi       2017-03-20 17:41:36.390607110 +0900
>>>>> @@ -1209,7 +1209,7 @@
>>>>>
>>>>>                 sata: sata@ee300000 {
>>>>>                         compatible = "renesas,sata-r8a7795";
>>>>> -                       reg = <0 0xee300000 0 0x1fff>;
>>>>> +                       reg = <0 0xee300000 0 0x200000>;
>>>>
>>>> While the datasheet does mention the 2 MiB area, it also says no (write)
>>>> access should be made to registers not listed in the table, while these are
>>>> all covered by the existing area?
>>>
>>> That bit about not writing to non-listed registers seems like just
>>> common sense to me, but perhaps there is more to the story than just
>>
>> Sure.
>>
>>> that? Like you say it is probably possible to use the driver with the
>>> existing 8K-1 size, but in my mind we should use window sizes defined
>>> in the data sheet for DT?
>>
>> The 2 MiB window size is a lot larger than needed to cover all documented
>> rregisters. Either there are more undocumented registers, or the hardware
>> engineers were lazy and just decoded the full 2 MiB block.
>
> Yeah, and on top of that it looks to me like the size is not aligned
> to the offset either. I would expect a 2MiB block to be aligned to a
> 2MiB boundary...

The base address was aligned to 2 miB on R-Car H1.
As of R-Car Gen2, it's no longer aligned. Too much copy-'n-paste...

>>>> BTW, what about the Reference Clock Source Select Register, which lies
>>>> in a further undocumented area?
>>>
>>> Yeah, no idea. This would be good task for the I/O or Core group to
>>
>> SATA is I/O.
>
> For sure, however I wonder how the external clock register REFSEL is
> supposed to be handled. It looks like an external hardware block
> somehow.

It's a single bit to select between internal and external clock.
No documention about which external clock.

>>> figure out how to handle. I'm surprised that the SATA DT device nodes
>>> with the strange and incorrect 0x1fff size got merged upstream without
>>> anyone thinking about that register that you are mentioning. Seems
>>> like supporting that should be part of SATA development for R-Car
>>> Gen3?
>>
>> Probably it was just an oversight. I almost missed it myself when
>> reviewing your patch.
>
> Probably yes.

It was up-ported from a patch in the BSP, which may predate the datasheet
revision that documented REFSEL.

>> The register is not present (not documented) on R-Car H1 and Gen2.
>> The IP core is derived from SH-Navi2G (sh7775), but no datasheet.
>
> On R-Car Gen3 there seem to be a chance that random glue hardware for
> other devices may also be present in the 0xe65exxxx range however
> documentations seem to be lacking...

And without documentation...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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] arm64: dts: r8a7795: Correct SATA device size to 2MiB

[Simon Horman]

On Mon, Mar 20, 2017 at 11:57:55AM +0100, Geert Uytterhoeven wrote:
> Hi Magnus,
> 
> On Mon, Mar 20, 2017 at 9:49 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
> > From: Magnus Damm <damm+renesas@opensource.se>
> >
> > Update the r8a7795 SATA device node to use a 2MiB I/O space as specified
> > in the "72. Serial-ATA" section of R-Car-Gen3-rev0.52E.pdf
> >
> > Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

...

Thanks, applied.