| Message ID | 20200825162718.5838-2-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | iWave G21D-Q7 enable PCIe, flash, CAN and SD2 LED | expand |
Hi Prabhakar, On Tue, Aug 25, 2020 at 6:28 PM Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > Enable PCIe Controller and set PCIe bus clock frequency. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > Reviewed-by: Chris Paterson <Chris.Paterson2@renesas.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> i.e. will queue in renesas-devel for v5.10. One thing to double-check below. > --- a/arch/arm/boot/dts/r8a7742-iwg21d-q7.dts > +++ b/arch/arm/boot/dts/r8a7742-iwg21d-q7.dts > @@ -238,6 +238,18 @@ > /* status = "okay"; */ > }; > > +&pcie_bus_clk { > + clock-frequency = <100000000>; > +}; > + > +&pciec { > + /* SW2[6] determines which connector is activated > + * ON = PCIe X4 (connector-J7) > + * OFF = mini-PCIe (connector-J26) The table on page 14 says it's the other way around. According to the CBTL02042ABQ datasheet, PCIe_SEL = low selects the first channel (PCIe x4), while PCIe_SEL = high selects the second channel (mini-PCIe). Enabling the switch ties the signal low, so the table must be wrong. > + */ > + status = "okay"; > +}; > + > &pfc { > avb_pins: avb { > groups = "avb_mdio", "avb_gmii"; Gr{oetje,eeting}s, Geert
Hi Geert, Thank you for the review. On Thu, Sep 3, 2020 at 11:18 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Prabhakar, > > On Tue, Aug 25, 2020 at 6:28 PM Lad Prabhakar > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > Enable PCIe Controller and set PCIe bus clock frequency. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Reviewed-by: Chris Paterson <Chris.Paterson2@renesas.com> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > i.e. will queue in renesas-devel for v5.10. > > One thing to double-check below. > > > --- a/arch/arm/boot/dts/r8a7742-iwg21d-q7.dts > > +++ b/arch/arm/boot/dts/r8a7742-iwg21d-q7.dts > > @@ -238,6 +238,18 @@ > > /* status = "okay"; */ > > }; > > > > +&pcie_bus_clk { > > + clock-frequency = <100000000>; > > +}; > > + > > +&pciec { > > + /* SW2[6] determines which connector is activated > > + * ON = PCIe X4 (connector-J7) > > + * OFF = mini-PCIe (connector-J26) > > The table on page 14 says it's the other way around. > > According to the CBTL02042ABQ datasheet, PCIe_SEL = low > selects the first channel (PCIe x4), while PCIe_SEL = high selects the > second channel (mini-PCIe). > Enabling the switch ties the signal low, so the table must be wrong. > Referring to [1] page 3: SEL = LOW: A↔B SEL = HIGH: A↔C And as per the schematic iW-PREJD-CS-01-R2.0-REL1.5.pdf channel B is J7 (PCIe X 4) and channel C is J26 (mini PCIe slot). Enabling the switch SW2[6] (ON) ties SEL to LOW -> channel B is J7 (PCIe X 4) Disabling the switch SW2[6] (OFF) ties SEL to HIGH -> channel C is J26 (mini PCIe) Also iW-PREJD-CS-01-R2.0-REL1.5.pdf page 14 (General purpose table DIP Switch) mentions the above. [1] https://www.mouser.co.uk/datasheet/2/302/CBTL02042A_CBTL02042B-1126164.pdf Cheers, Prabhakar > > + */ > > + status = "okay"; > > +}; > > + > > &pfc { > > avb_pins: avb { > > groups = "avb_mdio", "avb_gmii"; > > 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 Prabhakar, On Thu, Sep 3, 2020 at 1:18 PM Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote: > On Thu, Sep 3, 2020 at 11:18 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Tue, Aug 25, 2020 at 6:28 PM Lad Prabhakar > > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > > Enable PCIe Controller and set PCIe bus clock frequency. > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > Reviewed-by: Chris Paterson <Chris.Paterson2@renesas.com> > > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > i.e. will queue in renesas-devel for v5.10. > > > > One thing to double-check below. > > > > > --- a/arch/arm/boot/dts/r8a7742-iwg21d-q7.dts > > > +++ b/arch/arm/boot/dts/r8a7742-iwg21d-q7.dts > > > @@ -238,6 +238,18 @@ > > > /* status = "okay"; */ > > > }; > > > > > > +&pcie_bus_clk { > > > + clock-frequency = <100000000>; > > > +}; > > > + > > > +&pciec { > > > + /* SW2[6] determines which connector is activated > > > + * ON = PCIe X4 (connector-J7) > > > + * OFF = mini-PCIe (connector-J26) > > > > The table on page 14 says it's the other way around. > > > > According to the CBTL02042ABQ datasheet, PCIe_SEL = low > > selects the first channel (PCIe x4), while PCIe_SEL = high selects the > > second channel (mini-PCIe). > > Enabling the switch ties the signal low, so the table must be wrong. > > > Referring to [1] page 3: > > SEL = LOW: A↔B > SEL = HIGH: A↔C > > And as per the schematic iW-PREJD-CS-01-R2.0-REL1.5.pdf channel B is > J7 (PCIe X 4) and channel C is J26 (mini PCIe slot). > > Enabling the switch SW2[6] (ON) ties SEL to LOW -> channel B is J7 (PCIe X 4) > Disabling the switch SW2[6] (OFF) ties SEL to HIGH -> channel C is J26 > (mini PCIe) > > Also iW-PREJD-CS-01-R2.0-REL1.5.pdf page 14 (General purpose table DIP > Switch) mentions the above. Oh right, I looked at the old document, and they fixed it in the newer one. Gr{oetje,eeting}s, Geert
Hi! > +&pciec { > + /* SW2[6] determines which connector is activated > + * ON = PCIe X4 (connector-J7) > + * OFF = mini-PCIe (connector-J26) > + */ > + status = "okay"; > +}; Note this is wrong comment style for non-network parts of kernel. Best regards, Pavel
Hi Pavel, Thank you for the review. On Fri, Oct 9, 2020 at 8:23 AM Pavel Machek <pavel@denx.de> wrote: > > Hi! > > > +&pciec { > > + /* SW2[6] determines which connector is activated > > + * ON = PCIe X4 (connector-J7) > > + * OFF = mini-PCIe (connector-J26) > > + */ > > + status = "okay"; > > +}; > > Note this is wrong comment style for non-network parts of kernel. > Good point, i'll fix that. Cheers, Prabhakar
diff --git a/arch/arm/boot/dts/r8a7742-iwg21d-q7.dts b/arch/arm/boot/dts/r8a7742-iwg21d-q7.dts index 9bf4fbd9c736..73300ab46ea6 100644 --- a/arch/arm/boot/dts/r8a7742-iwg21d-q7.dts +++ b/arch/arm/boot/dts/r8a7742-iwg21d-q7.dts @@ -238,6 +238,18 @@ /* status = "okay"; */ }; +&pcie_bus_clk { + clock-frequency = <100000000>; +}; + +&pciec { + /* SW2[6] determines which connector is activated + * ON = PCIe X4 (connector-J7) + * OFF = mini-PCIe (connector-J26) + */ + status = "okay"; +}; + &pfc { avb_pins: avb { groups = "avb_mdio", "avb_gmii";