On Fri, 2021-01-29 at 02:07 +0100, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
Thanks for your time on reviewing the patches.
>
> > +bool lan937x_is_internal_phy_port(struct ksz_device *dev, int
> > port)
> > +{
> > + /* Check if the port is RGMII */
> > + if (port == LAN937X_RGMII_1_PORT || port ==
> > LAN937X_RGMII_2_PORT)
> > + return false;
> > +
> > + /* Check if the port is SGMII */
> > + if (port == LAN937X_SGMII_PORT &&
> > + GET_CHIP_ID_LSB(dev->chip_id) == CHIP_ID_73)
> > + return false;
> > +
> > + return true;
> > +}
> > +
> > +static u32 lan937x_get_port_addr(int port, int offset)
> > +{
> > + return PORT_CTRL_ADDR(port, offset);
> > +}
> > +
> > +bool lan937x_is_internal_tx_phy_port(struct ksz_device *dev, int
> > port)
> > +{
> > + /* Check if the port is internal tx phy port */
>
> What is an internal TX phy port? Is it actually a conventional t2
> Fast
> Ethernet port, as opposed to a t1 port?
This is 100 Base-Tx phy which is compliant with
802.3/802.3u standards. Two of the SKUs have both T1 and TX integrated
Phys as mentioned in the patch intro mail.
>
> > + if (lan937x_is_internal_phy_port(dev, port) && port ==
> > LAN937X_TXPHY_PORT)
> > + if ((GET_CHIP_ID_LSB(dev->chip_id) == CHIP_ID_71) ||
> > + (GET_CHIP_ID_LSB(dev->chip_id) == CHIP_ID_72))
> > + return true;
> > +
> > + return false;
> > +}
> > +
> > +bool lan937x_is_internal_t1_phy_port(struct ksz_device *dev, int
> > port)
> > +{
> > + /* Check if the port is internal t1 phy port */
> > + if (lan937x_is_internal_phy_port(dev, port) &&
> > + !lan937x_is_internal_tx_phy_port(dev, port))
> > + return true;
> > +
> > + return false;
> > +}
> > +
> > +int lan937x_t1_tx_phy_write(struct ksz_device *dev, int addr,
> > + int reg, u16 val)
> > +{
> > + u16 temp, addr_base;
> > + unsigned int value;
> > + int ret;
> > +
> > + /* Check for internal phy port */
> > + if (!lan937x_is_internal_phy_port(dev, addr))
> > + return 0;
>
> All this t1 and tx is confusing. I think lan937x_internal_phy_write()
> would be better.
Sure, will rename it to lan937x_internal_phy_write()
>
> I also wonder if -EOPNOTSUPP would be better, or -EINVAL?
-EOPNOTSUPP would be better. Currently return value has not checked for
phy_write() calls, i will add that as well in the list
>
> > +
> > + if (lan937x_is_internal_tx_phy_port(dev, addr))
> > + addr_base = REG_PORT_TX_PHY_CTRL_BASE;
> > + else
> > + addr_base = REG_PORT_T1_PHY_CTRL_BASE;
> > +
> > + temp = PORT_CTRL_ADDR(addr, (addr_base + (reg << 2)));
> > +
> > + ksz_write16(dev, REG_VPHY_IND_ADDR__2, temp);
> > +
> > + /* Write the data to be written to the VPHY reg */
> > + ksz_write16(dev, REG_VPHY_IND_DATA__2, val);
> > +
> > + /* Write the Write En and Busy bit */
> > + ksz_write16(dev, REG_VPHY_IND_CTRL__2, (VPHY_IND_WRITE
> > + | VPHY_IND_BUSY));
> > +
> > + ret = regmap_read_poll_timeout(dev->regmap[1],
> > + REG_VPHY_IND_CTRL__2,
> > + value, !(value & VPHY_IND_BUSY), 10,
> > 1000);
> > +
> > + /* failed to write phy register. get out of loop */
> > + if (ret) {
> > + dev_err(dev->dev, "Failed to write phy register\n");
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +int lan937x_t1_tx_phy_read(struct ksz_device *dev, int addr,
> > + int reg, u16 *val)
> > +{
> > + u16 temp, addr_base;
> > + unsigned int value;
> > + int ret;
> > +
> > + if (lan937x_is_internal_phy_port(dev, addr)) {
> > + if (lan937x_is_internal_tx_phy_port(dev, addr))
> > + addr_base = REG_PORT_TX_PHY_CTRL_BASE;
> > + else
> > + addr_base = REG_PORT_T1_PHY_CTRL_BASE;
>
> You could reduce the indentation by doing what you did above:
Sure, will change it.
>
> > + /* Check for internal phy port */
> > + if (!lan937x_is_internal_phy_port(dev, addr))
> > + return 0;
>
> You might want to return 0xffff here, which is what a read on a
> non-existent device on an MDIO bus should return.
Sure, will change it
>
>
> > +
> > + /* get register address based on the logical port */
> > + temp = PORT_CTRL_ADDR(addr, (addr_base + (reg <<
> > 2)));
> > +
> > + ksz_write16(dev, REG_VPHY_IND_ADDR__2, temp);
> > + /* Write Read and Busy bit to start the transaction*/
> > + ksz_write16(dev, REG_VPHY_IND_CTRL__2,
> > VPHY_IND_BUSY);
> > +
> > + ret = regmap_read_poll_timeout(dev->regmap[1],
> > + REG_VPHY_IND_CTRL__2,
> > + value, !(value &
> > VPHY_IND_BUSY), 10, 1000);
> > +
> > + /* failed to read phy register. get out of loop */
> > + if (ret) {
> > + dev_err(dev->dev, "Failed to read phy
> > register\n");
> > + return ret;
> > + }
> > + /* Read the VPHY register which has the PHY data*/
> > + ksz_read16(dev, REG_VPHY_IND_DATA__2, val);
> > + }
> > +
> > + return 0;
> > +}
> > +static void tx_phy_setup(struct ksz_device *dev, int port)
> > +{
> > + u16 data_lo;
> > +
> > + lan937x_t1_tx_phy_read(dev, port, REG_PORT_TX_SPECIAL_MODES,
> > &data_lo);
> > + /* Need to change configuration from 6 to other value. */
> > + data_lo &= TX_PHYADDR_M;
> > +
> > + lan937x_t1_tx_phy_write(dev, port, REG_PORT_TX_SPECIAL_MODES,
> > data_lo);
> > +
> > + /* Need to toggle test_mode bit to enable DSP access. */
> > + lan937x_t1_tx_phy_write(dev, port, REG_PORT_TX_IND_CTRL,
> > TX_TEST_MODE);
> > + lan937x_t1_tx_phy_write(dev, port, REG_PORT_TX_IND_CTRL, 0);
> > +
> > + /* Note TX_TEST_MODE is then always enabled so this is not
> > required. */
> > + lan937x_t1_tx_phy_write(dev, port, REG_PORT_TX_IND_CTRL,
> > TX_TEST_MODE);
> > + lan937x_t1_tx_phy_write(dev, port, REG_PORT_TX_IND_CTRL, 0);
>
> This is only accessing PHY registers, not switch registers. So this
> code belongs in the PHY driver, not the switch driver.
>
> What PHY driver is actually used? The "Microchip LAN87xx T1" driver?
Phy is basically a LAN87xx PHY. But the driver is customized for
LAN937x.
>
> > +static void tx_phy_port_init(struct ksz_device *dev, int port)
> > +{
> > + u32 data;
> > +
> > + /* Software reset. */
> > + lan937x_t1_tx_phy_mod_bits(dev, port, MII_BMCR, BMCR_RESET,
> > true);
> > +
> > + /* tx phy setup */
> > + tx_phy_setup(dev, port);
>
> And which PHY driver is used here? "Microchip LAN88xx"? All this code
> should be in the PHY driver.
As of now, no driver is available in the kernel since its part of
LAN937x.
> s
> > +void lan937x_port_setup(struct ksz_device *dev, int port, bool
> > cpu_port)
> > +{
> > + struct ksz_port *p = &dev->ports[port];
> > + u8 data8, member;
> > + } else {
> > + /* force flow control off*/
> > + lan937x_port_cfg(dev, port, REG_PORT_XMII_CTRL_0,
> > + PORT_FORCE_TX_FLOW_CTRL |
> > PORT_FORCE_RX_FLOW_CTRL,
> > + false);
> > +
> > + lan937x_pread8(dev, port, REG_PORT_XMII_CTRL_1,
> > &data8);
> > +
> > + /* clear MII selection & set it based on interface
> > later */
> > + data8 &= ~PORT_MII_SEL_M;
> > +
> > + /* configure MAC based on p->interface */
> > + switch (p->interface) {
> > + case PHY_INTERFACE_MODE_MII:
> > + lan937x_set_gbit(dev, false, &data8);
> > + data8 |= PORT_MII_SEL;
> > + break;
> > + case PHY_INTERFACE_MODE_RMII:
> > + lan937x_set_gbit(dev, false, &data8);
> > + data8 |= PORT_RMII_SEL;
> > + break;
> > + default:
> > + lan937x_set_gbit(dev, true, &data8);
> > + data8 |= PORT_RGMII_SEL;
> > +
> > + data8 &= ~PORT_RGMII_ID_IG_ENABLE;
> > + data8 &= ~PORT_RGMII_ID_EG_ENABLE;
> > +
> > + if (p->interface ==
> > PHY_INTERFACE_MODE_RGMII_ID ||
> > + p->interface ==
> > PHY_INTERFACE_MODE_RGMII_RXID)
> > + data8 |= PORT_RGMII_ID_IG_ENABLE;
> > +
> > + if (p->interface ==
> > PHY_INTERFACE_MODE_RGMII_ID ||
> > + p->interface ==
> > PHY_INTERFACE_MODE_RGMII_TXID)
> > + data8 |= PORT_RGMII_ID_EG_ENABLE;
>
> Normally, the PHY inserts the delay, not the MAC. If the MAC is doing
> the delay, you need to ensure the PHY knows this, when you call
> phy_connect() you need to pass PHY_INTERFACE_MODE_RGMII, so it does
> not add delays.
Okay, understand.
>
> > + break;
> > + }
> > + lan937x_pwrite8(dev, port, REG_PORT_XMII_CTRL_1,
> > data8);
> > + }
> > +
> > + if (cpu_port)
> > + member = dev->port_mask;
> > + else
> > + member = dev->host_mask | p->vid_member;
> > +
> > + lan937x_cfg_port_member(dev, port, member);
> > +}
> > +
> > +static int lan937x_switch_init(struct ksz_device *dev)
> > +{
> > + int i;
> > +
> > + dev->ds->ops = &lan937x_switch_ops;
> > +
> > + for (i = 0; i < ARRAY_SIZE(lan937x_switch_chips); i++) {
> > + const struct lan937x_chip_data *chip =
> > &lan937x_switch_chips[i];
> > +
> > + if (dev->chip_id == chip->chip_id) {
> > + dev->name = chip->dev_name;
> > + dev->num_vlans = chip->num_vlans;
> > + dev->num_alus = chip->num_alus;
> > + dev->num_statics = chip->num_statics;
> > + dev->port_cnt = chip->port_cnt;
> > + dev->cpu_ports = chip->cpu_ports;
> > + break;
> > + }
> > + }
>
> Please verify that the switch found actually matches the DT
> compatible
> string. With 4 compatible strings, if you don't verify it, you will
> find that 3/4 of the boards have it wrong, but it still works. You
> then get into trouble when you actually need to use the compatible
> string for something.
>
> Or just use a single compatible string.
Okay, will add dt-compatible checks.
>
> > +static int lan937x_get_link_status(struct ksz_device *dev, int
> > port)
> > +{
> > + u16 val1, val2;
> > +
> > + lan937x_t1_tx_phy_read(dev, port, REG_PORT_T1_PHY_M_STATUS,
> > + &val1);
> > +
> > + lan937x_t1_tx_phy_read(dev, port, REG_PORT_T1_MODE_STAT,
> > &val2);
> > +
> > + if (val1 & (PORT_T1_LOCAL_RX_OK | PORT_T1_REMOTE_RX_OK) &&
> > + val2 & (T1_PORT_DSCR_LOCK_STATUS_MSK |
> > T1_PORT_LINK_UP_MSK))
> > + return PHY_LINK_UP;
> > +
> > + return PHY_LINK_DOWN;
> > +}
>
> The PHY driver should tell you if the link is up. You should not
> being
> accessing the PHY directly.
>
> It actually looks like you have your PHY drivers here, embedded
> inside
> this driver. That is wrong, they should move into drivers/net/phy.
> The
> switch driver should just give access to the registers, so that the
> PHY driver, and phylib and drive the PHY.
Sure, i think T1 & Tx phy drivers to be submitted.
>
> > +static void lan937x_port_stp_state_set(struct dsa_switch *ds, int
> > port,
> > + u8 state)
> > +{
> > + struct ksz_device *dev = ds->priv;
> > + struct ksz_port *p = &dev->ports[port];
> > + int forward = dev->member;
> > + int member = -1;
> > + u8 data;
> > +
> > + lan937x_pread8(dev, port, P_STP_CTRL, &data);
> > + data &= ~(PORT_TX_ENABLE | PORT_RX_ENABLE |
> > PORT_LEARN_DISABLE);
> > +
> > + switch (state) {
> > + case BR_STATE_DISABLED:
> > + data |= PORT_LEARN_DISABLE;
> > + if (port != dev->cpu_port)
> > + member = 0;
>
> You can remove all the tests for cpu_port. It should never
> happen. If
> it does, something is broken in the DSA core.
Okay, will remove the checks of cpu_port.
>
> > + break;
> > + case BR_STATE_LISTENING:
> > + data |= (PORT_RX_ENABLE | PORT_LEARN_DISABLE);
> > + if (port != dev->cpu_port &&
> > + p->stp_state == BR_STATE_DISABLED)
> > + member = dev->host_mask | p->vid_member;
> > + break;
> > + case BR_STATE_LEARNING:
> > + data |= PORT_RX_ENABLE;
> > + break;
> > + case BR_STATE_FORWARDING:
> > + data |= (PORT_TX_ENABLE | PORT_RX_ENABLE);
> > +
> > + /* This function is also used internally. */
> > + if (port == dev->cpu_port)
> > + break;
>
> You probably want to refactor this. Move the code which is needed for
> the CPU port into a helper, and call it directly.
Sure, will create one helper function.
>
> > +
> > + member = dev->host_mask | p->vid_member;
> > + mutex_lock(&dev->dev_mutex);
> > +
> > + /* Port is a member of a bridge. */
> > + if (dev->br_member & (1 << port)) {
> > + dev->member |= (1 << port);
> > + member = dev->member;
> > + }
> > + mutex_unlock(&dev->dev_mutex);
> > + break;
> > + case BR_STATE_BLOCKING:
> > + data |= PORT_LEARN_DISABLE;
> > + if (port != dev->cpu_port &&
> > + p->stp_state == BR_STATE_DISABLED)
> > + member = dev->host_mask | p->vid_member;
> > + break;
> > + default:
> > + dev_err(ds->dev, "invalid STP state: %d\n", state);
> > + return;
> > + }
> > +
> > + lan937x_pwrite8(dev, port, P_STP_CTRL, data);
> > +
> > + p->stp_state = state;
> > + mutex_lock(&dev->dev_mutex);
> > +
> > + /* Port membership may share register with STP state. */
> > + if (member >= 0 && member != p->member)
> > + lan937x_cfg_port_member(dev, port, (u8)member);
> > +
> > + /* Check if forwarding needs to be updated. */
> > + if (state != BR_STATE_FORWARDING) {
> > + if (dev->br_member & (1 << port))
> > + dev->member &= ~(1 << port);
> > + }
> > +
> > + /* When topology has changed the function
> > ksz_update_port_member
> > + * should be called to modify port forwarding behavior.
> > + */
> > + if (forward != dev->member)
> > + ksz_update_port_member(dev, port);
>
> Please could you explain more what is going on with membership?
> Generally, STP state is specific to the port, and nothing else
> changes. So it is not clear what this membership is all about.
It updates the membership for the forwarding behavior based on
forwarding state of the port.
>
>
> > + mutex_unlock(&dev->dev_mutex);
> > +}
> > +
> > +static void lan937x_config_cpu_port(struct dsa_switch *ds)
> > +{
> > + struct ksz_device *dev = ds->priv;
> > + struct ksz_port *p;
> > + int i;
> > +
> > + ds->num_ports = dev->port_cnt;
> > +
> > + for (i = 0; i < dev->port_cnt; i++) {
> > + if (dsa_is_cpu_port(ds, i) && (dev->cpu_ports & (1 <<
> > i))) {
> > + phy_interface_t interface;
> > + const char *prev_msg;
> > + const char *prev_mode;
> > +
> > + dev->cpu_port = i;
> > + dev->host_mask = (1 << dev->cpu_port);
> > + dev->port_mask |= dev->host_mask;
> > + p = &dev->ports[i];
> > +
> > + /* Read from XMII register to determine host
> > port
> > + * interface. If set specifically in device
> > tree
> > + * note the difference to help debugging.
> > + */
> > + interface = lan937x_get_interface(dev, i);
> > + if (!p->interface) {
> > + if (dev->compat_interface) {
> > + dev_warn(dev->dev,
> > + "Using legacy switch
> > \"phy-mode\" property, because it is missing on port %d node.
> > Please update your device tree.\n",
> > + i);
>
> Since this is a new driver, there cannot be any legacy DT blobs which
> needs workarounds.
Understand, will remove the warning message.
>
> Andrew
