| Message ID | 20220914064730.1878211-1-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v3] ravb: Add RZ/G2L MII interface support | expand |
On 9/14/22 9:47 AM, Biju Das wrote: > EMAC IP found on RZ/G2L Gb ethernet supports MII interface. > This patch adds support for selecting MII interface mode. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > v2->v3: > * Documented CXR35_HALFCYC_CLKSW1000 and CXR35_SEL_XMII_MII macros. I definitely didn't mean it done this way... [...] > diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h > index b980bce763d3..058aceac8c92 100644 > --- a/drivers/net/ethernet/renesas/ravb.h > +++ b/drivers/net/ethernet/renesas/ravb.h [...] > @@ -965,6 +966,11 @@ enum CXR31_BIT { > CXR31_SEL_LINK1 = 0x00000008, > }; > > +enum CXR35_BIT { > + CXR35_HALFCYC_CLKSW1000 = 0x03E80000, /* 1000 cycle of clk_chi */ No, please just declare: CXR35_HALFCYC_CLKSW = 0xffff0000, > + CXR35_SEL_XMII_MII = 0x00000002, /* MII interface is used */ All the other register *enum*s are declared from LSB to MSB. The comment is pretty self-obvious here, please remove it. And declare the whole field too: CXR35_SEL_XMII = 0x00000003, CXR35_SEL_XMII_RGMII = 0x00000000, CXR35_SEL_XMII_MII = 0x00000002, [...] > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > index b357ac4c56c5..9a0d06dd5eb6 100644 > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c > @@ -540,7 +540,14 @@ static void ravb_emac_init_gbeth(struct net_device *ndev) > /* E-MAC interrupt enable register */ > ravb_write(ndev, ECSIPR_ICDIP, ECSIPR); > > - ravb_modify(ndev, CXR31, CXR31_SEL_LINK0 | CXR31_SEL_LINK1, CXR31_SEL_LINK0); > + if (priv->phy_interface == PHY_INTERFACE_MODE_MII) { > + ravb_modify(ndev, CXR31, CXR31_SEL_LINK0 | CXR31_SEL_LINK1, 0); > + ravb_write(ndev, CXR35_HALFCYC_CLKSW1000 | CXR35_SEL_XMII_MII, > + CXR35); ravb_write(ndev, (1000 << 16) | CXR35_SEL_XMII_MII, CXR35); [...] MBR, Sergey
Hi Sergey, Thanks for the feedback. > Subject: Re: [PATCH net-next v3] ravb: Add RZ/G2L MII interface support > > On 9/14/22 9:47 AM, Biju Das wrote: > > > EMAC IP found on RZ/G2L Gb ethernet supports MII interface. > > This patch adds support for selecting MII interface mode. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > --- > > v2->v3: > > * Documented CXR35_HALFCYC_CLKSW1000 and CXR35_SEL_XMII_MII macros. > > I definitely didn't mean it done this way... > > [...] > > diff --git a/drivers/net/ethernet/renesas/ravb.h > > b/drivers/net/ethernet/renesas/ravb.h > > index b980bce763d3..058aceac8c92 100644 > > --- a/drivers/net/ethernet/renesas/ravb.h > > +++ b/drivers/net/ethernet/renesas/ravb.h > [...] > > @@ -965,6 +966,11 @@ enum CXR31_BIT { > > CXR31_SEL_LINK1 = 0x00000008, > > }; > > > > +enum CXR35_BIT { > > + CXR35_HALFCYC_CLKSW1000 = 0x03E80000, /* 1000 cycle of clk_chi > */ > > No, please just declare: > > CXR35_HALFCYC_CLKSW = 0xffff0000, Q1) Why do you think we should use this value for setting MII? As per hardware manual the value you suggested is wrong for MII settings. See page 2157 [A] The case which CXR35 SEL_XMII is used for the selection of RGMII/MII in APB Clock 100 MHz. (1) To use RGMII interface, Set ‘H’03E8 0000’ to this register. (2) To use MII interface, Set ‘H’03E8 0002’ to this register. > > > + CXR35_SEL_XMII_MII = 0x00000002, /* MII interface is used > */ > > All the other register *enum*s are declared from LSB to MSB. The > comment is pretty self-obvious here, please remove it. And declare the > whole field too: > > CXR35_SEL_XMII = 0x00000003, Values 1 and 3 are reserved so we cannot use 3. I think the current patch holds good as per the hardware manual for selecting MII interface. Please recheck and correct me if it is wrong. Cheers, Biju > CXR35_SEL_XMII_RGMII = 0x00000000, > CXR35_SEL_XMII_MII = 0x00000002, > > [...] > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c > > b/drivers/net/ethernet/renesas/ravb_main.c > > index b357ac4c56c5..9a0d06dd5eb6 100644 > > --- a/drivers/net/ethernet/renesas/ravb_main.c > > +++ b/drivers/net/ethernet/renesas/ravb_main.c > > @@ -540,7 +540,14 @@ static void ravb_emac_init_gbeth(struct net_device > *ndev) > > /* E-MAC interrupt enable register */ > > ravb_write(ndev, ECSIPR_ICDIP, ECSIPR); > > > > - ravb_modify(ndev, CXR31, CXR31_SEL_LINK0 | CXR31_SEL_LINK1, > CXR31_SEL_LINK0); > > + if (priv->phy_interface == PHY_INTERFACE_MODE_MII) { > > + ravb_modify(ndev, CXR31, CXR31_SEL_LINK0 | CXR31_SEL_LINK1, > 0); > > + ravb_write(ndev, CXR35_HALFCYC_CLKSW1000 | CXR35_SEL_XMII_MII, > > + CXR35); > > ravb_write(ndev, (1000 << 16) | CXR35_SEL_XMII_MII, CXR35); > > [...] > > MBR, Sergey
Hi Sergey, > Subject: RE: [PATCH net-next v3] ravb: Add RZ/G2L MII interface support > > Hi Sergey, > > Thanks for the feedback. > > > Subject: Re: [PATCH net-next v3] ravb: Add RZ/G2L MII interface > > support > > > > On 9/14/22 9:47 AM, Biju Das wrote: > > > > > EMAC IP found on RZ/G2L Gb ethernet supports MII interface. > > > This patch adds support for selecting MII interface mode. > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > > --- > > > v2->v3: > > > * Documented CXR35_HALFCYC_CLKSW1000 and CXR35_SEL_XMII_MII macros. > > > > I definitely didn't mean it done this way... > > > > [...] > > > diff --git a/drivers/net/ethernet/renesas/ravb.h > > > b/drivers/net/ethernet/renesas/ravb.h > > > index b980bce763d3..058aceac8c92 100644 > > > --- a/drivers/net/ethernet/renesas/ravb.h > > > +++ b/drivers/net/ethernet/renesas/ravb.h > > [...] > > > @@ -965,6 +966,11 @@ enum CXR31_BIT { > > > CXR31_SEL_LINK1 = 0x00000008, > > > }; > > > > > > +enum CXR35_BIT { > > > + CXR35_HALFCYC_CLKSW1000 = 0x03E80000, /* 1000 cycle of clk_chi > > */ > > > > No, please just declare: > > > > > > CXR35_HALFCYC_CLKSW = 0xffff0000, > > Q1) Why do you think we should use this value for setting MII? > > As per hardware manual the value you suggested is wrong for MII settings. > See page 2157 > > [A] The case which CXR35 SEL_XMII is used for the selection of RGMII/MII > in APB Clock 100 MHz. > > (1) To use RGMII interface, Set ‘H’03E8 0000’ to this register. > (2) To use MII interface, Set ‘H’03E8 0002’ to this register. > > > > > > + CXR35_SEL_XMII_MII = 0x00000002, /* MII interface is used > > */ > > > > All the other register *enum*s are declared from LSB to MSB. The > > comment is pretty self-obvious here, please remove it. And declare the > > whole field too: > > > > CXR35_SEL_XMII = 0x00000003, > > Values 1 and 3 are reserved so we cannot use 3. > > I think the current patch holds good as per the hardware manual for > selecting MII interface. Please recheck and correct me if it is wrong. > > Cheers, > Biju > > > CXR35_SEL_XMII_RGMII = 0x00000000, > > CXR35_SEL_XMII_MII = 0x00000002, > > > > [...] > > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c > > > b/drivers/net/ethernet/renesas/ravb_main.c > > > index b357ac4c56c5..9a0d06dd5eb6 100644 > > > --- a/drivers/net/ethernet/renesas/ravb_main.c > > > +++ b/drivers/net/ethernet/renesas/ravb_main.c > > > @@ -540,7 +540,14 @@ static void ravb_emac_init_gbeth(struct > > > net_device > > *ndev) > > > /* E-MAC interrupt enable register */ > > > ravb_write(ndev, ECSIPR_ICDIP, ECSIPR); > > > > > > - ravb_modify(ndev, CXR31, CXR31_SEL_LINK0 | CXR31_SEL_LINK1, > > CXR31_SEL_LINK0); > > > + if (priv->phy_interface == PHY_INTERFACE_MODE_MII) { > > > + ravb_modify(ndev, CXR31, CXR31_SEL_LINK0 | CXR31_SEL_LINK1, > > 0); > > > + ravb_write(ndev, CXR35_HALFCYC_CLKSW1000 | CXR35_SEL_XMII_MII, > > > + CXR35); > > > > ravb_write(ndev, (1000 << 16) | CXR35_SEL_XMII_MII, CXR35); Oops. Missed this magic number changes. Will send v4 with above suggestions. Cheers, Biju
On 9/14/22 8:20 PM, Biju Das wrote: [...] >>> EMAC IP found on RZ/G2L Gb ethernet supports MII interface. >>> This patch adds support for selecting MII interface mode. >>> >>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> >>> --- >>> v2->v3: >>> * Documented CXR35_HALFCYC_CLKSW1000 and CXR35_SEL_XMII_MII macros. >> >> I definitely didn't mean it done this way... >> >> [...] >>> diff --git a/drivers/net/ethernet/renesas/ravb.h >>> b/drivers/net/ethernet/renesas/ravb.h >>> index b980bce763d3..058aceac8c92 100644 >>> --- a/drivers/net/ethernet/renesas/ravb.h >>> +++ b/drivers/net/ethernet/renesas/ravb.h >> [...] >>> @@ -965,6 +966,11 @@ enum CXR31_BIT { >>> CXR31_SEL_LINK1 = 0x00000008, >>> }; >>> >>> +enum CXR35_BIT { >>> + CXR35_HALFCYC_CLKSW1000 = 0x03E80000, /* 1000 cycle of clk_chi >> */ >> >> No, please just declare: > > >> >> CXR35_HALFCYC_CLKSW = 0xffff0000, > > Q1) Why do you think we should use this value for setting MII? Where are you seeing me asking for that? This is just the field declaration, correct against the manual... we can safely omit it as well... [...] >>> + CXR35_SEL_XMII_MII = 0x00000002, /* MII interface is used >> */ >> >> All the other register *enum*s are declared from LSB to MSB. The >> comment is pretty self-obvious here, please remove it. And declare the >> whole field too: >> >> CXR35_SEL_XMII = 0x00000003, > > Values 1 and 3 are reserved so we cannot use 3. Again, this is the filed declaration, correct against the manual... > I think the current patch holds good as per the hardware manual > for selecting MII interface. It is incomplete, compared against the manual. And declaring CXR35_HALFCYC_CLKSW1000 just looks completely ridiculous. :-) > Please recheck and correct me > if it is wrong. > Cheers, > Biju [...] MBR, Sergey
Hi Sergey, > Subject: Re: [PATCH net-next v3] ravb: Add RZ/G2L MII interface support > > On 9/14/22 8:20 PM, Biju Das wrote: > > [...] > >>> EMAC IP found on RZ/G2L Gb ethernet supports MII interface. > >>> This patch adds support for selecting MII interface mode. > >>> > >>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > >>> --- > >>> v2->v3: > >>> * Documented CXR35_HALFCYC_CLKSW1000 and CXR35_SEL_XMII_MII macros. > >> > >> I definitely didn't mean it done this way... > >> > >> [...] > >>> diff --git a/drivers/net/ethernet/renesas/ravb.h > >>> b/drivers/net/ethernet/renesas/ravb.h > >>> index b980bce763d3..058aceac8c92 100644 > >>> --- a/drivers/net/ethernet/renesas/ravb.h > >>> +++ b/drivers/net/ethernet/renesas/ravb.h > >> [...] > >>> @@ -965,6 +966,11 @@ enum CXR31_BIT { > >>> CXR31_SEL_LINK1 = 0x00000008, > >>> }; > >>> > >>> +enum CXR35_BIT { > >>> + CXR35_HALFCYC_CLKSW1000 = 0x03E80000, /* 1000 cycle of clk_chi > >> */ > >> > >> No, please just declare: > > > > > >> > >> CXR35_HALFCYC_CLKSW = 0xffff0000, > > > > Q1) Why do you think we should use this value for setting MII? > > Where are you seeing me asking for that? This is just the field > declaration, correct against the manual... we can safely omit it as > well... OK will keep it as field declaration and use actual value during setting. > > [...] > >>> + CXR35_SEL_XMII_MII = 0x00000002, /* MII interface is used > >> */ > >> > >> All the other register *enum*s are declared from LSB to MSB. The > >> comment is pretty self-obvious here, please remove it. And declare > >> the whole field too: > >> > >> CXR35_SEL_XMII = 0x00000003, > > > > Values 1 and 3 are reserved so we cannot use 3. > > Again, this is the filed declaration, correct against the manual... OK. Will add it. > > > I think the current patch holds good as per the hardware manual for > > selecting MII interface. > > It is incomplete, compared against the manual. And declaring > CXR35_HALFCYC_CLKSW1000 just looks completely ridiculous. :-) Ok. All good now. Will send 4. Cheers, Biju
diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h index b980bce763d3..058aceac8c92 100644 --- a/drivers/net/ethernet/renesas/ravb.h +++ b/drivers/net/ethernet/renesas/ravb.h @@ -189,6 +189,7 @@ enum ravb_reg { PSR = 0x0528, PIPR = 0x052c, CXR31 = 0x0530, /* RZ/G2L only */ + CXR35 = 0x0540, /* RZ/G2L only */ MPR = 0x0558, PFTCR = 0x055c, PFRCR = 0x0560, @@ -965,6 +966,11 @@ enum CXR31_BIT { CXR31_SEL_LINK1 = 0x00000008, }; +enum CXR35_BIT { + CXR35_HALFCYC_CLKSW1000 = 0x03E80000, /* 1000 cycle of clk_chi */ + CXR35_SEL_XMII_MII = 0x00000002, /* MII interface is used */ +}; + enum CSR0_BIT { CSR0_TPE = 0x00000010, CSR0_RPE = 0x00000020, diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c index b357ac4c56c5..9a0d06dd5eb6 100644 --- a/drivers/net/ethernet/renesas/ravb_main.c +++ b/drivers/net/ethernet/renesas/ravb_main.c @@ -540,7 +540,14 @@ static void ravb_emac_init_gbeth(struct net_device *ndev) /* E-MAC interrupt enable register */ ravb_write(ndev, ECSIPR_ICDIP, ECSIPR); - ravb_modify(ndev, CXR31, CXR31_SEL_LINK0 | CXR31_SEL_LINK1, CXR31_SEL_LINK0); + if (priv->phy_interface == PHY_INTERFACE_MODE_MII) { + ravb_modify(ndev, CXR31, CXR31_SEL_LINK0 | CXR31_SEL_LINK1, 0); + ravb_write(ndev, CXR35_HALFCYC_CLKSW1000 | CXR35_SEL_XMII_MII, + CXR35); + } else { + ravb_modify(ndev, CXR31, CXR31_SEL_LINK0 | CXR31_SEL_LINK1, + CXR31_SEL_LINK0); + } } static void ravb_emac_init_rcar(struct net_device *ndev)
EMAC IP found on RZ/G2L Gb ethernet supports MII interface. This patch adds support for selecting MII interface mode. Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- v2->v3: * Documented CXR35_HALFCYC_CLKSW1000 and CXR35_SEL_XMII_MII macros. v1->v2: * Fixed spaces->Tab around CXR35 description. --- drivers/net/ethernet/renesas/ravb.h | 6 ++++++ drivers/net/ethernet/renesas/ravb_main.c | 9 ++++++++- 2 files changed, 14 insertions(+), 1 deletion(-)