Re: [PATCH] phy: nxp-c45: add driver for tja1103

["Radu Nicolae Pirea (NXP OSS)" <radu-nicolae.pirea@oss.nxp.com>]

On Mon, 2021-04-12 at 10:50 +0100, Russell King - ARM Linux admin
wrote:
> On Fri, Apr 09, 2021 at 09:41:06PM +0300, Radu Pirea (NXP OSS) wrote:
> > +#define B100T1_PMAPMD_CTL              0x0834
> > +#define B100T1_PMAPMD_CONFIG_EN                BIT(15)
> > +#define B100T1_PMAPMD_MASTER           BIT(14)
> > +#define MASTER_MODE                    (B100T1_PMAPMD_CONFIG_EN |
> > B100T1_PMAPMD_MASTER)
> > +#define SLAVE_MODE                     (B100T1_PMAPMD_CONFIG_EN)
> > +
> > +#define DEVICE_CONTROL                 0x0040
> > +#define DEVICE_CONTROL_RESET           BIT(15)
> > +#define DEVICE_CONTROL_CONFIG_GLOBAL_EN        BIT(14)
> > +#define DEVICE_CONTROL_CONFIG_ALL_EN   BIT(13)
> > +#define RESET_POLL_NS                  (250 * NSEC_PER_MSEC)
> > +
> > +#define PHY_CONTROL                    0x8100
> > +#define PHY_CONFIG_EN                  BIT(14)
> > +#define PHY_START_OP                   BIT(0)
> > +
> > +#define PHY_CONFIG                     0x8108
> > +#define PHY_CONFIG_AUTO                        BIT(0)
> > +
> > +#define SIGNAL_QUALITY                 0x8320
> > +#define SQI_VALID                      BIT(14)
> > +#define SQI_MASK                       GENMASK(2, 0)
> > +#define MAX_SQI                                SQI_MASK
> > +
> > +#define CABLE_TEST                     0x8330
> > +#define CABLE_TEST_ENABLE              BIT(15)
> > +#define CABLE_TEST_START               BIT(14)
> > +#define CABLE_TEST_VALID               BIT(13)
> > +#define CABLE_TEST_OK                  0x00
> > +#define CABLE_TEST_SHORTED             0x01
> > +#define CABLE_TEST_OPEN                        0x02
> > +#define CABLE_TEST_UNKNOWN             0x07
> > +
> > +#define PORT_CONTROL                   0x8040
> > +#define PORT_CONTROL_EN                        BIT(14)
> > +
> > +#define PORT_INFRA_CONTROL             0xAC00
> > +#define PORT_INFRA_CONTROL_EN          BIT(14)
> > +
> > +#define VND1_RXID                      0xAFCC
> > +#define VND1_TXID                      0xAFCD
> > +#define ID_ENABLE                      BIT(15)
> > +
> > +#define ABILITIES                      0xAFC4
> > +#define RGMII_ID_ABILITY               BIT(15)
> > +#define RGMII_ABILITY                  BIT(14)
> > +#define RMII_ABILITY                   BIT(10)
> > +#define REVMII_ABILITY                 BIT(9)
> > +#define MII_ABILITY                    BIT(8)
> > +#define SGMII_ABILITY                  BIT(0)
> > +
> > +#define MII_BASIC_CONFIG               0xAFC6
> > +#define MII_BASIC_CONFIG_REV           BIT(8)
> > +#define MII_BASIC_CONFIG_SGMII         0x9
> > +#define MII_BASIC_CONFIG_RGMII         0x7
> > +#define MII_BASIC_CONFIG_RMII          0x5
> > +#define MII_BASIC_CONFIG_MII           0x4
> > +
> > +#define SYMBOL_ERROR_COUNTER           0x8350
> > +#define LINK_DROP_COUNTER              0x8352
> > +#define LINK_LOSSES_AND_FAILURES       0x8353
> > +#define R_GOOD_FRAME_CNT               0xA950
> > +#define R_BAD_FRAME_CNT                        0xA952
> > +#define R_RXER_FRAME_CNT               0xA954
> > +#define RX_PREAMBLE_COUNT              0xAFCE
> > +#define TX_PREAMBLE_COUNT              0xAFCF
> > +#define RX_IPG_LENGTH                  0xAFD0
> > +#define TX_IPG_LENGTH                  0xAFD1
> > +#define COUNTERS_EN                    BIT(15)
> > +
> > +#define CLK_25MHZ_PS_PERIOD            40000UL
> > +#define PS_PER_DEGREE                  (CLK_25MHZ_PS_PERIOD / 360)
> > +#define MIN_ID_PS                      8222U
> > +#define MAX_ID_PS                      11300U
> 
> Maybe include some prefix as to which MMD each of these registers is
> located?
I will add the MMD as prefix. Thank you.
> 
> > +static bool nxp_c45_can_sleep(struct phy_device *phydev)
> > +{
> > +       int reg;
> > +
> > +       reg = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_STAT1);
> > +       if (reg < 0)
> > +               return false;
> > +
> > +       return !!(reg & MDIO_STAT1_LPOWERABLE);
> > +}
> 
> This looks like it could be useful as a generic helper function -
> nothing in this function is specific to this PHY.
> 
> > +static int nxp_c45_resume(struct phy_device *phydev)
> > +{
> > +       int reg;
> > +
> > +       if (!nxp_c45_can_sleep(phydev))
> > +               return -EOPNOTSUPP;
> > +
> > +       reg = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1);
> > +       reg &= ~MDIO_CTRL1_LPOWER;
> > +       phy_write_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1, reg);
> > +
> > +       return 0;
> > +}
> > +
> > +static int nxp_c45_suspend(struct phy_device *phydev)
> > +{
> > +       int reg;
> > +
> > +       if (!nxp_c45_can_sleep(phydev))
> > +               return -EOPNOTSUPP;
> > +
> > +       reg = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1);
> > +       reg |= MDIO_CTRL1_LPOWER;
> > +       phy_write_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1, reg);
> > +
> > +       return 0;
> > +}
> 
> These too look like potential generic helper functions.
That's true.
Should I implement them as genphy_c45_pma_suspend/resume? Given that we
can also have PCS suspend/resume too.

However, in my case, PMA low power bit will enable low power for PCS as
well.