Hello!
On 04/17/2018 04:23 PM, Geert Uytterhoeven wrote:
> + Sergei
Sorry for a delay replying, I seem to have ignored large private message and
then it got buried in other mail...
>>> Subject: Re: [PATCH v2 1/2] pinctrl: sh-pfc: Add r8a77470 PFC support
>>> On Wed, Apr 4, 2018 at 5:22 PM, Biju Das <biju.das@xxxxxxxxxxxxxx> wrote:
>>>> Add PFC support for the R8A77470 SoC including pin groups for some
>>>> on-chip devices such as SCIF, AVB and MMC.
>>>>
>>>> Signed-off-by: Biju Das <biju.das@xxxxxxxxxxxxxx>
>>>> Reviewed-by: Fabrizio Castro <fabrizio.castro@xxxxxxxxxxxxxx>
>
>>>> --- /dev/null
>>>> +++ b/drivers/pinctrl/sh-pfc/pfc-r8a77470.c
>
>>>> +/* - AVB
>>>> +-------------------------------------------------------------------- */ static const
>>> unsigned int avb_link_pins[] = {
>>>> + RCAR_GP_PIN(5, 14),
>>>> +};
>>>> +static const unsigned int avb_link_mux[] = {
>>>> + AVB_LINK_MARK,
>>>> +};
>>>> +static const unsigned int avb_magic_pins[] = {
>>>> + RCAR_GP_PIN(5, 15),
>>>> +};
>>>> +static const unsigned int avb_magic_mux[] = {
>>>> + AVB_MAGIC_MARK,
>>>> +};
>>>> +static const unsigned int avb_phy_int_pins[] = {
>>>> + RCAR_GP_PIN(5, 16),
>>>> +};
>>>> +static const unsigned int avb_phy_int_mux[] = {
>>>> + AVB_PHY_INT_MARK,
>>>> +};
>>>> +static const unsigned int avb_mdio_pins[] = {
>>>> + RCAR_GP_PIN(5, 12), RCAR_GP_PIN(5, 13), }; static const
>>>> +unsigned int avb_mdio_mux[] = {
>>>> + AVB_MDC_MARK, AVB_MDIO_MARK,
>>>> +};
>>>> +static const unsigned int avb_mii_pins[] = {
>>>> + RCAR_GP_PIN(3, 14), RCAR_GP_PIN(3, 15), RCAR_GP_PIN(3, 16),
>>>> + RCAR_GP_PIN(3, 27),
>>>> +
>>>> + RCAR_GP_PIN(3, 2), RCAR_GP_PIN(3, 3), RCAR_GP_PIN(3, 4),
>>>> + RCAR_GP_PIN(3, 5),
>>>> +
>>>> + RCAR_GP_PIN(3, 10), RCAR_GP_PIN(3, 0), RCAR_GP_PIN(3, 1),
>>>> + RCAR_GP_PIN(5, 17), RCAR_GP_PIN(3, 13), RCAR_GP_PIN(5, 23),
>>>> + RCAR_GP_PIN(3, 12),
>>>> +};
>>>> +static const unsigned int avb_mii_mux[] = {
>>>> + AVB_TXD0_MARK, AVB_TXD1_MARK, AVB_TXD2_MARK,
>>>> + AVB_TXD3_MARK,
>>>> +
>>>> + AVB_RXD0_MARK, AVB_RXD1_MARK, AVB_RXD2_MARK,
>>>> + AVB_RXD3_MARK,
>>>> +
>>>> + AVB_RX_ER_MARK, AVB_RX_CLK_MARK, AVB_RX_DV_MARK,
>>>> + AVB_CRS_MARK, AVB_TX_EN_MARK, AVB_TX_ER_MARK,
>>>> + AVB_TX_CLK_MARK,
>>>
>>> You forgot AVB_COL, which is GP5_18?
>>
>> GP5_18 is shred between the signals AVB_COL and VI0_CLK. As per the RZ/G1C board schematic /hardware user guide
>> Ethernet Phy collision pin(AVB_COL) pin is not populated on this board by default. It is populated only for
>> Video Input Channel0 pixel clock.
>>
>> Since it is initial submission, I thought of adding only board specific pins first and later
>> add this collision pin alone as a separate pin entry in the avb group. That way we can support existing board
>> as well as any future RZ/G1C board which populates this pin. Are you ok for this approach?
> Oops. That means our grouping is suboptimal. Perhaps we should revisit
> (for all SoCs, while keeping backwards compatibility)?
> After reading some wikipedia, I came up with:
>
> mii_tx
> mii_tx_er (optional)
> mii_rx
I don't see why we should have the separate TX/RX subgroups just because they're described separately in the Wikipedia. Or did you have some special reason to do it?
> mii_col_crs (optional, half duplex)
>
> However, given your board uses AVB_CRS without AVB_COL, that would still
> not be sufficient, so the last group should be split to cover your use case?
> BTW, how does it work with AVB_COL not wired to anything by default, and thus
> floating? Do you enable pull-up/down for that pin in the PFC, or is the pin
> just ignored in full-duplex mode?
Well, I'm seeing the strong possibility the half-duplex mode just doesn't work as
the collision indication isn't available.
>>>> +};
>>>> +static const unsigned int avb_gmii_pins[] = {
>>>> + RCAR_GP_PIN(3, 14), RCAR_GP_PIN(3, 15), RCAR_GP_PIN(3, 16),
>>>> + RCAR_GP_PIN(3, 27), RCAR_GP_PIN(3, 28), RCAR_GP_PIN(3, 29),
>>>> + RCAR_GP_PIN(4, 0), RCAR_GP_PIN(5, 22),
>>>> +
>>>> + RCAR_GP_PIN(3, 2), RCAR_GP_PIN(3, 3), RCAR_GP_PIN(3, 4),
>>>> + RCAR_GP_PIN(3, 5), RCAR_GP_PIN(3, 6), RCAR_GP_PIN(3, 7),
>>>> + RCAR_GP_PIN(3, 8), RCAR_GP_PIN(3, 9),
>>>> +
>>>> + RCAR_GP_PIN(3, 10), RCAR_GP_PIN(3, 0), RCAR_GP_PIN(3, 1),
>>>> + RCAR_GP_PIN(5, 17), RCAR_GP_PIN(4, 1), RCAR_GP_PIN(3, 11),
>>>> + RCAR_GP_PIN(3, 13), RCAR_GP_PIN(5, 23), RCAR_GP_PIN(3, 12), };
>>>> +static const unsigned int avb_gmii_mux[] = {
>>>> + AVB_TXD0_MARK, AVB_TXD1_MARK, AVB_TXD2_MARK,
>>>> + AVB_TXD3_MARK, AVB_TXD4_MARK, AVB_TXD5_MARK,
>>>> + AVB_TXD6_MARK, AVB_TXD7_MARK,
>>>> +
>>>> + AVB_RXD0_MARK, AVB_RXD1_MARK, AVB_RXD2_MARK,
>>>> + AVB_RXD3_MARK, AVB_RXD4_MARK, AVB_RXD5_MARK,
>>>> + AVB_RXD6_MARK, AVB_RXD7_MARK,
>>>> +
>>>> + AVB_RX_ER_MARK, AVB_RX_CLK_MARK, AVB_RX_DV_MARK,
>>>> + AVB_CRS_MARK, AVB_GTX_CLK_MARK, AVB_GTXREFCLK_MARK,
>>>> + AVB_TX_EN_MARK, AVB_TX_ER_MARK, AVB_TX_CLK_MARK,
>>>
>>> You forgot AVB_COL, which is GP5_18?
>>
>> The same comment as above.
>
> Likewise:
>
> gmii_tx
> gmii_rx
We shouldn't divide those IMO...
> gmii_col_crs (optional, half duplex; should this be split?)
No split, in my opinion -- either both or none!
[...]
> Gr{oetje,eeting}s,
>
> Geert
MBR, Sergei
