Hi Sergei,
On Mon, Mar 5, 2018 at 8:12 PM, Sergei Shtylyov
<sergei.shtylyov@xxxxxxxxxxxxxxxxxx> wrote:
> On 03/02/2018 09:47 PM, Geert Uytterhoeven wrote:
>>>>> Add the PFC support for the R8A77980 SoC including pin groups for some
>>>>> on-chip devices such as AVB, CAN-FD, GETHER, [H]SCIF, I2C, INTC-EX, MMC,
>>>>> MSIOF, PWM, and VIN...
>>>>>
>>>>> Based on the original (and large) patch by Vladimir Barinov.
>>>>>
>>>>> Signed-off-by: Vladimir Barinov <vladimir.barinov@xxxxxxxxxxxxxxxxxx>
>>>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@xxxxxxxxxxxxxxxxxx>
>>>>
>>>> Thanks for your patch!
>>>>
>>>> To avoid scaring off potential reviewers, it may be better to not include that
>>>> many pin groups and functions in future initial submissions.
>>>> This also helps if issues are detected during review in some of them (like
>>>> below), delaying queuing of basic functionality and other correct parts.
>>>>
>>>> I only looked at CPU_ALL_PORT(), pins, groups, and functions.
>>>> Comments below.
>>>>
>>>>> --- /dev/null
>>>>> +++ renesas-drivers/drivers/pinctrl/sh-pfc/pfc-r8a77980.c
>>>>
>>>>> +/* - AVB -------------------------------------------------------------------- */
>>>>> +static const unsigned int avb_link_pins[] = {
>>>>> + /* AVB_LINK */
>>>>> + RCAR_GP_PIN(1, 18),
>>>>> +};
>>>>> +static const unsigned int avb_link_mux[] = {
>>>>> + AVB_LINK_MARK,
>>>>> +};
>>>>> +static const unsigned int avb_magic_pins[] = {
>>>>> + /* AVB_MAGIC */
>>>>> + RCAR_GP_PIN(1, 16),
>>>>> +};
>>>>> +static const unsigned int avb_magic_mux[] = {
>>>>> + AVB_MAGIC_MARK,
>>>>> +};
>>>>> +static const unsigned int avb_phy_int_pins[] = {
>>>>> + /* AVB_PHY_INT */
>>>>> + RCAR_GP_PIN(1, 17),
>>>>> +};
>>>>> +static const unsigned int avb_phy_int_mux[] = {
>>>>> + AVB_PHY_INT_MARK,
>>>>> +};
>>>>> +static const unsigned int avb_mdio_pins[] = {
>>>>> + /* AVB_MDC, AVB_MDIO */
>>>>> + RCAR_GP_PIN(1, 15), RCAR_GP_PIN(1, 14),
>>>>> +};
>>>>> +static const unsigned int avb_mdio_mux[] = {
>>>>> + AVB_MDC_MARK, AVB_MDIO_MARK,
>>>>> +};
>>>>
>>>> The grouping is different from other R-Car Gen3 SoCs.
>>>
>>> Not true AFAICS -- only the group naming is different.
>>
>> Oh, so we can have both groups names, and be compatible?
>
> Probably. Just not sure if there's any worth to have the same groups for the not
> pin compatible SoCs. Care to elaborate?
So I have compared all AVB/(G)ETHER pin groups on supported SoCs,
to solve the issue for good, hopefully.
EtherAVB:
--------
R-Car Gen2:
Common (r8a7790/r8a7791/r8a7792/r8a7793/r8a7794) AVB:
link = { LINK }
magic = { MAGIC }
phy_int = { PHY_INT }
mdio = { MDC, MDIO }
mii = { COL, CRS, RX_CLK, RXD[0-3], RX_DV, RX_ER, TX_CLK, TXD[0-3],
TX_EN, TX_ER }
gmii = { COL, CRS, GTX_CLK, GTXREFCLK, RX_CLK, RXD[0-7], RX_DV, RX_ER,
TX_CLK, TXD[0-7], TX_EN, TX_ER }
AVB exceptions:
- r8a7790 "mii" lacks TX_ER (should it?)
- r8a7792 has match = { AVTP_MATCH }
R-Car Gen3:
r8a7795/r8a7795-es1/r8a7796/r8a77965/r8a77995 AVB:
link = { LINK }
magic = { MAGIC }
phy_int = { PHY_INT }
mdc = { MDC, MDIO }
mii = { RD[0-3], RXC, RX_CTL, TD[0-3], TXC, TXCREFCLK, TX_CTL }
avtp_pps = { AVTP_PPS }
avtp_match = { AVTP_MATCH }
avtp_capture = { AVTP_CAPTURE }
Notes:
- { MDC, MDIO } groups is named "mdc" instead of "mdio"
- Supports RGMII, but group is named "mii"
Sergei's r8a77970 AVB proposal:
link = { LINK }
magic = { MAGIC }
phy_int = { PHY_INT }
* mdio = { MDC, MDIO }
mii = { RD[0-3], RXC, RX_CTL, TD[0-3], TXC, TXCREFCLK, TX_CTL }
avtp_pps = { AVTP_PPS }
avtp_match = { AVTP_MATCH }
avtp_capture = { AVTP_CAPTURE }
Notes:
* indicates difference with H3/M3/D3
- Supports RGMII, but group is named "mii"
Sergei's r8a77980 AVB proposal:
link = { LINK }
magic = { MAGIC }
phy_int = { PHY_INT }
* mdio = { MDC, MDIO }
* rgmii = { RD[0-3], RXC, RX_CTL, TD[0-3], TXC, TX_CTL }
* txcrefclk = { TXCREFCLK }
avtp_pps = { AVTP_PPS }
avtp_match = { AVTP_MATCH }
avtp_capture = { AVTP_CAPTURE }
Notes:
* indicates difference with H3/M3/D3
- Supports RGMII, and group is called "rgmii"
- TXCREFCLK has its own group. Why? Similarity with GETHER on V3H?
(G)Ether:
--------
R-Mobile A1 (r8a7740) ETHER:
rmii = { CRS_DV, MDC, MDIO, REF50CK, RXD[01], RX_ER, TXD[01], TX_EN }
mii = { COL, CRS, MDC, MDIO, RX_CLK, RXD[0-3], RX_DV, RX_ER, TX_CLK,
TXD[0-3], TX_EN, TX_ER }
gmii = { COL, CRS, GTX_CLK, MDC, MDIO, REF125CK, RXD[0-7], RX_CLK,
RX_DV, RX_ER, TX_CLK, TXD[0-7], TX_EN, TX_ER }
int = { PHY_INT }
link = { LINK }
wol = { WOL }
Notes:
- "rmii", "mii", and "gmii" include MDC and MDIO
R-Car Gen1:
r8a7778/r8a7779 ETHER:
link = { LINK }
magic = { MAGIC }
rmii = { CRS_DV, MDC, MDIO, REF_CLK, RXD[01], RX_ER, TXD[01], TX_EN }
Notes:
- "rmii" includes MDC and MDIO (there's only one choice of interface)
R-Car Gen2:
r8a7790/r8a7791/r8a7793/r8a7794 ETHER:
link = { LINK }
magic = { MAGIC }
mdio = { MDC, MDIO }
rmii = { CRS_DV, REF_CLK, RXD[01], RX_ER, TXD[01], TX_EN }
R-Car Gen3:
Sergei's r8a77980 GETHER proposal:
link = { LINK }
magic = { MAGIC }
phy_int = { PHY_INT }
mdio = { MDC, MDIO }
rgmii = { RD[0-3], RXC, RX_CTL, TD[0-3], TXC, TX_CTL }
rmii = { CRS_DV, REFCLK, RXD[01], RX_ER, TXD[01], TXD_EN }
txcrefclk = { TXCREFCLK }
txcrefclk_mega = { TXCREFCLK_MEGA }
Notes:
- TXCREFCLK has its own group. I assume because one has to choose
between TXCREFCLK and TXCREFCLK_MEGA?
Actions:
--------
- Add missing TX_ER to "mii" group on r8a7790,
- Rename "mdc" to "mdio" on H3/M3/D3, as the bus is called "Management
Data Input/Output (MDIO)", add alias for backwords compatibility,
https://en.wikipedia.org/wiki/Management_Data_Input/Output
- Should we rename "mii" to "rgmii" on H3/M3/D3?
The group does not contain all RGMII pins anyway.
Thanks for your comments!
>>>>> +/* - HSCIF0 ----------------------------------------------------------------- */
>>>>
>>>>> +static const unsigned int scif_clk_a_pins[] = {
>>>>> + /* SCIF_CLK */
>>>>> + RCAR_GP_PIN(0, 10),
>>>>> +};
>>>>> +static const unsigned int scif_clk_a_mux[] = {
>>>>> + SCIF_CLK_A_MARK,
>>>>> +};
>>>>
>>>>
>>>> [...]
>>>>
>>>>> +static const unsigned int scif_clk_b_pins[] = {
>>>>> + /* SCIF_CLK */
>>>>> + RCAR_GP_PIN(1, 25),
>>>>> +};
>>>>> +static const unsigned int scif_clk_b_mux[] = {
>>>>> + SCIF_CLK_B_MARK,
>>>>> +};
>>>>
>>>> Please move the scif_clk pins to their own section, as they are shared by
>>>> HSCIF and SCIF.
>>>
>>> Again, the same bit in MOD_SEL0...
>>
>> Same story: SCIF_CLK is shared by the HSCIFx and SCIFx modules.
>> Still puts the responsability in the hands of the board designer: he must
>
> Responsibility. :-)
As long as it doesn't end up in a commit message...
>> know not to select SCIF_CLK_A and HSCIF0_B, or SCIF_CLK_B and HSCIF0_A.
>>
>> On this SoC, the limitations seem to be worse than on other members of
>> the same family.
>>
>> However, on other SoCs you have similar limitations. E.g. on H3/M3-W/M3-N
>> you cannot select scif5_data_a and scif5_clk_b, as they are both controlled
>> through sel_scif5,
>
>> although the driver has them in separate groups.
>> But that is done to allow using only RX/TX functionality, and using the
>> CTS/RTS pins for something else (a completely different function, or GPIO).
>
> That's OK as they both are the part of the same function. My case is different,
> sorry for not making this clear...
So, do you agree having a separate SCIF_CLK function, like on all other
R-Car SoCs?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
