| Message ID | 148999976116.21495.8298900753704826597.sendpatchset@little-apple (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | e9f0089b2d8a3d450b8ec02eccfb92b950110fbe |
| Headers | show |
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
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
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.
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
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
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
--- 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>;