Re: [PATCH 2/2] pinctrl: sh-pfc: add R8A77980 PFC support

[Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>]

On 03/06/2018 01:59 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@cogentembedded.com>
>>>>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>>>>
>>>>> 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,

   Oops! GTXREFCLK is not a part of GMII (at least according to Wikipedia) ...

>              TX_CLK, TXD[0-7], TX_EN, TX_ER }
> 
>   AVB exceptions:
>     - r8a7790 "mii" lacks TX_ER (should it?)

   This signal is optional.

>     - 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 }

   Oops again! TXREFCLK doen't belong in MII...

>     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"

   Oops... I blame Renesas! :-)

>   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"

   Oops, needs fixing...

>   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?

   Because that signal doesn't belong to RGMII (according to Wikipedia).

> (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,

   Oops, REF125CK doesn't belong to GMII...

>              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

   That sucketh! :-)

> 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)

   That too...

> 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?

   No, because they're not a part of either RGMII or RMII interfaces.

> Actions:
> --------
> 
>   - Add missing TX_ER to "mii" group on r8a7790,

   Remembering it's optional...

>   - 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

   Agreed.

>   - Should we rename "mii" to "rgmii" on H3/M3/D3?
>     The group does not contain all RGMII pins anyway.

   It does!

> 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?

   I agree as long as that works...

> Gr{oetje,eeting}s,
> 
>                         Geert

MBR, Sergei