| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <427ccaf425fe68190e22fb23e2918bd300679323.camel@oss.nxp.com>
Date: Tue, 13 Apr 2021 16:44:15 +0300
From: "Radu Nicolae Pirea (NXP OSS)" <radu-nicolae.pirea@....nxp.com>
To: Russell King - ARM Linux admin <linux@...linux.org.uk>
Cc: andrew@...n.ch, hkallweit1@...il.com, davem@...emloft.net,
kuba@...nel.org, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] phy: nxp-c45: add driver for tja1103
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.
Powered by blists - more mailing lists