Hi Russell,
Thanks for your comments.
On Tue, 21 Nov 2023 at 17:45, Russell King (Oracle)
<linux@xxxxxxxxxxxxxxx> wrote:
>
> On Tue, Nov 21, 2023 at 05:17:33PM +0200, Tomer Maimon wrote:
> > Add Nuvoton NPCM BMC SoCs support to STMMAC dwmac driver.
> >
> > And modify MAINTAINERS to add a new F: entry for this driver.
> >
> > Signed-off-by: Tomer Maimon <tmaimon77@xxxxxxxxx>
>
> A few comments on this...
>
> > +#define IND_AC_BA_REG 0x1FE
> > +#define SR_MII_CTRL 0x3E0000
> > +
> > +#define PCS_SR_MII_CTRL_REG 0x0
> > +#define PCS_SPEED_SELECT6 BIT(6)
> > +#define PCS_AN_ENABLE BIT(12)
> > +#define PCS_SPEED_SELECT13 BIT(13)
> > +#define PCS_RST BIT(15)
>
> include/uapi/linux/mii.h:
>
> #define BMCR_SPEED1000 0x0040 /* MSB of Speed (1000) */
> #define BMCR_ANENABLE 0x1000 /* Enable auto negotiation */
> #define BMCR_SPEED100 0x2000 /* Select 100Mbps */
> #define BMCR_RESET 0x8000 /* Reset to default state */
>
> Look familiar? Maybe use the standard definitions for a standardised
> register?
>
> > +void npcm_dwmac_pcs_init(struct npcm_dwmac *dwmac, struct device *dev,
> > + struct plat_stmmacenet_data *plat_dat)
> > +{
> > + u16 val;
> > +
> > + iowrite16((u16)(SR_MII_CTRL >> 9), dwmac->reg + IND_AC_BA_REG);
> > + val = ioread16(dwmac->reg + PCS_SR_MII_CTRL_REG);
> > + val |= PCS_RST;
> > + iowrite16(val, dwmac->reg + PCS_SR_MII_CTRL_REG);
> > +
> > + while (val & PCS_RST)
> > + val = ioread16(dwmac->reg + PCS_SR_MII_CTRL_REG);
>
> What if the PCS never clears its reset bit? Maybe use
> read_poll_timeout() ?
>
> > +
> > + val &= ~(PCS_AN_ENABLE);
> > + iowrite16(val, dwmac->reg + PCS_SR_MII_CTRL_REG);
> > +}
>
> Also, maybe it's time to require new stmmac platform support to start
> making use of the phylink PCS support rather than continuing to code its
> own?
>
> I notice, however, that you always disable inband signalling - please
> explain why. Also, what protocol does the PCS use when communicating
> with the PHY?
With disable inband signalling you mean disable auto negotiation? if
yes it is because the PCS sgmii is connected to the external phy AN
and is not working between the PCS and external phy.
accessing the PCS registers is indirect. The top 13 bits (bits 21-9)
of the offset have to be written to Indirect Access Base register
bits 12:0 before indirectly accessing the target register with the
offset of the bottom 9 bits (8:0) of the offset
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Thanks,
Tomer
