| Message ID | 20180104200150.11352-4-chris.brandt@renesas.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Thu, Jan 4, 2018 at 9:01 PM, Chris Brandt <chris.brandt@renesas.com> wrote: > Add USB device support. > > Signed-off-by: Chris Brandt <chris.brandt@renesas.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Minor nits below. > --- a/arch/arm/boot/dts/r7s72100.dtsi > +++ b/arch/arm/boot/dts/r7s72100.dtsi > @@ -667,4 +667,24 @@ > power-domains = <&cpg_clocks>; > status = "disabled"; > }; > + > + usbhs0: usbhs@e8010000 { > + compatible = "renesas,usbhs-r7s72100"; > + reg = <0xe8010000 0x1A0>; 0x1a0 > + interrupts = <GIC_SPI (73-32) IRQ_TYPE_LEVEL_HIGH>; "41", all other interrupt properties already have the SPI offset subtracted? > + clocks = <&mstp7_clks R7S72100_CLK_USB0>; > + renesas,buswait = <4>; > + power-domains = <&cpg_clocks>; > + status = "disabled"; > + }; > + > + usbhs1: usbhs@e8207000 { > + compatible = "renesas,usbhs-r7s72100"; > + reg = <0xe8207000 0x1A0>; 0x1a0 > + interrupts = <GIC_SPI (74-32) IRQ_TYPE_LEVEL_HIGH>; "42", all other interrupt properties already have the SPI offset subtracted? 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
Hello! On 1/4/2018 11:01 PM, Chris Brandt wrote: > Add USB device support. > > Signed-off-by: Chris Brandt <chris.brandt@renesas.com> > --- > arch/arm/boot/dts/r7s72100.dtsi | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/arch/arm/boot/dts/r7s72100.dtsi b/arch/arm/boot/dts/r7s72100.dtsi > index ab9645a42eca..eb414e735185 100644 > --- a/arch/arm/boot/dts/r7s72100.dtsi > +++ b/arch/arm/boot/dts/r7s72100.dtsi > @@ -667,4 +667,24 @@ > power-domains = <&cpg_clocks>; > status = "disabled"; > }; > + > + usbhs0: usbhs@e8010000 { The node names should be generic, i.e "usb@e8010000". > + compatible = "renesas,usbhs-r7s72100"; > + reg = <0xe8010000 0x1A0>; Lowercase in the hex values, please be consistent... > + interrupts = <GIC_SPI (73-32) IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&mstp7_clks R7S72100_CLK_USB0>; > + renesas,buswait = <4>; > + power-domains = <&cpg_clocks>; > + status = "disabled"; > + }; > + > + usbhs1: usbhs@e8207000 { > + compatible = "renesas,usbhs-r7s72100"; > + reg = <0xe8207000 0x1A0>; > + interrupts = <GIC_SPI (74-32) IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&mstp7_clks R7S72100_CLK_USB1>; > + renesas,buswait = <4>; > + power-domains = <&cpg_clocks>; > + status = "disabled"; > + }; > }; The same comments for the 2nd device. MBR, Sergei
On Thu, Jan 04, 2018 at 03:01:50PM -0500, Chris Brandt wrote: > Add USB device support. > > Signed-off-by: Chris Brandt <chris.brandt@renesas.com> Hi Chris, it looks like there have been some requests for (minor) changes to this patch. Please consider addressing those and reposting.
On Friday, January 05, 2018, Sergei Shtylyov wrote: > > + usbhs0: usbhs@e8010000 { > > The node names should be generic, i.e "usb@e8010000". > > > + compatible = "renesas,usbhs-r7s72100"; > > + reg = <0xe8010000 0x1A0>; > > Lowercase in the hex values, please be consistent... > > > + interrupts = <GIC_SPI (73-32) IRQ_TYPE_LEVEL_HIGH>; > > + clocks = <&mstp7_clks R7S72100_CLK_USB0>; > > + renesas,buswait = <4>; > > + power-domains = <&cpg_clocks>; > > + status = "disabled"; > > + }; > > + > > + usbhs1: usbhs@e8207000 { > > + compatible = "renesas,usbhs-r7s72100"; > > + reg = <0xe8207000 0x1A0>; > > + interrupts = <GIC_SPI (74-32) IRQ_TYPE_LEVEL_HIGH>; > > + clocks = <&mstp7_clks R7S72100_CLK_USB1>; > > + renesas,buswait = <4>; > > + power-domains = <&cpg_clocks>; > > + status = "disabled"; > > + }; > > }; > > The same comments for the 2nd device. Thank you for the review. I'll make the changes and resubmit. Chris
Hi Geert, Thanks for the review. On Friday, January 05, 2018, Geert Uytterhoeven wrote: > > + interrupts = <GIC_SPI (73-32) IRQ_TYPE_LEVEL_HIGH>; > > "41", all other interrupt properties already have the SPI offset > subtracted? The actual HW vector number in the hardware manual is '73'. As you know, you need to subtract 32 for the number you use in the device tree. But then...the number goes back to '73' when you look at it in /proc/interrupts. So it was confusing to people on what number you needed to use. Therefore in the RZ/A1 BSP, I was doing (xx-32) so at least people could see the relationship between what's in the hardware manual and what gets put into the device tree. So, if doing (73-32) looks wrong, I can change it back to '41' for the upstream version. Any opinions???? Chris
Hi Chris, On Fri, Jan 5, 2018 at 1:54 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote: > On Friday, January 05, 2018, Geert Uytterhoeven wrote: >> > + interrupts = <GIC_SPI (73-32) IRQ_TYPE_LEVEL_HIGH>; >> >> "41", all other interrupt properties already have the SPI offset >> subtracted? > > The actual HW vector number in the hardware manual is '73'. > As you know, you need to subtract 32 for the number you use in the > device tree. > But then...the number goes back to '73' when you look at it in > /proc/interrupts. Having an identical number in /proc/interrupts is a coincidence. These numbers are virtual, and may change even across reboots. > So it was confusing to people on what number you needed to use. > > Therefore in the RZ/A1 BSP, I was doing (xx-32) so at least people could > see the relationship between what's in the hardware manual and what > gets put into the device tree. > > So, if doing (73-32) looks wrong, I can change it back to '41' for the > upstream version. > > Any opinions???? Not really, except that no single .dts(i) file seems to have "- 32". Note that e.g. the R-Car Gen3 manuals do list both "interrupt ID" and "SGI, PPI, or SPI No" in the documentation for INTC-AP. 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 Friday, January 05, 2018, Geert Uytterhoeven wrote: > > But then...the number goes back to '73' when you look at it in > > /proc/interrupts. > > Having an identical number in /proc/interrupts is a coincidence. > These numbers are virtual, and may change even across reboots. I'm not talking about the id number (the 1st column). I'm talking about the 4th column. $ uname -rs Linux 4.15.0-rc5-00008-g366d22ea091b $ cat /proc/interrupts CPU0 17: 2365 GIC-0 135 Edge ostm 18: 0 GIC-0 230 Level e8008000.serial:rx err 19: 45 GIC-0 231 Level e8008000.serial:rx full 20: 194 GIC-0 232 Level e8008000.serial:tx empty 21: 0 GIC-0 229 Level e8008000.serial:break 24: 0 GIC-0 189 Level riic-tend 25: 0 GIC-0 190 Edge riic-rdrf 26: 0 GIC-0 191 Edge riic-tdre 27: 0 GIC-0 192 Level riic-stop 29: 0 GIC-0 194 Level riic-nack 32: 160 GIC-0 213 Level riic-tend 33: 160 GIC-0 214 Edge riic-rdrf 34: 480 GIC-0 215 Edge riic-tdre 35: 240 GIC-0 216 Level riic-stop 37: 0 GIC-0 218 Level riic-nack 40: 0 GIC-0 139 Level fcff0000.timer 42: 0 GIC-0 305 Level e804e800.sd 43: 52 GIC-0 306 Level e804e800.sd 44: 0 GIC-0 307 Level e804e800.sd 45: 0 GIC-0 308 Edge sh-rtc period 46: 0 GIC-0 309 Edge sh-rtc carry 47: 0 GIC-0 310 Edge sh-rtc alarm 48: 0 GIC-0 73 Level e8010000.usbhs Err: 0 > > Any opinions???? > > Not really, except that no single .dts(i) file seems to have "- 32". Then I can be a pioneer of new software!! ;) I'll just change it back to a single number when I upstream code just to be consistent. Thanks, Chris
diff --git a/arch/arm/boot/dts/r7s72100.dtsi b/arch/arm/boot/dts/r7s72100.dtsi index ab9645a42eca..eb414e735185 100644 --- a/arch/arm/boot/dts/r7s72100.dtsi +++ b/arch/arm/boot/dts/r7s72100.dtsi @@ -667,4 +667,24 @@ power-domains = <&cpg_clocks>; status = "disabled"; }; + + usbhs0: usbhs@e8010000 { + compatible = "renesas,usbhs-r7s72100"; + reg = <0xe8010000 0x1A0>; + interrupts = <GIC_SPI (73-32) IRQ_TYPE_LEVEL_HIGH>; + clocks = <&mstp7_clks R7S72100_CLK_USB0>; + renesas,buswait = <4>; + power-domains = <&cpg_clocks>; + status = "disabled"; + }; + + usbhs1: usbhs@e8207000 { + compatible = "renesas,usbhs-r7s72100"; + reg = <0xe8207000 0x1A0>; + interrupts = <GIC_SPI (74-32) IRQ_TYPE_LEVEL_HIGH>; + clocks = <&mstp7_clks R7S72100_CLK_USB1>; + renesas,buswait = <4>; + power-domains = <&cpg_clocks>; + status = "disabled"; + }; };
Add USB device support. Signed-off-by: Chris Brandt <chris.brandt@renesas.com> --- arch/arm/boot/dts/r7s72100.dtsi | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)