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

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Sergei,

On Mon, Feb 26, 2018 at 5:53 PM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> On 02/26/2018 03:42 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?

>>> +/* - CANFD0 ----------------------------------------------------------------- */
>>> +static const unsigned int canfd0_data_a_pins[] = {
>>> +       /* CANFD0_TX, CANFD0_RX */
>>> +       RCAR_GP_PIN(1, 21), RCAR_GP_PIN(1, 22),
>>> +};
>>> +static const unsigned int canfd0_data_a_mux[] = {
>>> +       CANFD0_TX_A_MARK, CANFD0_RX_A_MARK,
>>> +};
>>> +static const unsigned int canfd_clk_a_pins[] = {
>>> +       /* CANFD_CLK */
>>> +       RCAR_GP_PIN(1, 25),
>>> +};
>>> +static const unsigned int canfd_clk_a_mux[] = {
>>> +       CANFD_CLK_A_MARK,
>>> +};
>>> +static const unsigned int canfd0_data_b_pins[] = {
>>> +       /* CANFD0_TX, CANFD0_RX */
>>> +       RCAR_GP_PIN(3, 6), RCAR_GP_PIN(3, 7),
>>> +};
>>> +static const unsigned int canfd0_data_b_mux[] = {
>>> +       CANFD0_TX_B_MARK, CANFD0_RX_B_MARK,
>>> +};
>>> +static const unsigned int canfd_clk_b_pins[] = {
>>> +       /* CANFD_CLK */
>>> +       RCAR_GP_PIN(3, 8),
>>> +};
>>> +static const unsigned int canfd_clk_b_mux[] = {
>>> +       CANFD_CLK_B_MARK,
>>> +};
>>
>> Please move the canfd_clk pins to their own section.
>
>    Are you aware the CANFD_CLK is controlled by MOD_SEL0.SEL_CANFD0? If it's allowed
> to have the pins controlled by a single MOD_SEL field in the diff groups, I'll place
> CANFD_CLK into a separate group...

SEL_CANFD0 switches between the A and B groups for CANFD0_TX, CANFD0_RX,
and CANFD0_CLK together.
So all three pins must use either group A, or group B. That's something the
user must do correctly anyway.

However, the CANFD_CLK pin is shared between the CANFD0 and CANFD1
modules.  So a user who uses CANFD1, and not CANFD0, still has to configure
pinmuxing for CANFD_CLK.

>>> +/* - 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
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).

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