Hi Andrew, On 09/05/22 18:02, Andrew Lunn wrote: >>>> +static void icssg_init_emac_mode(struct prueth *prueth) >>>> +{ >>>> + u8 mac[ETH_ALEN] = { 0 }; >>>> + >>>> + if (prueth->emacs_initialized) >>>> + return; >>>> + >>>> + regmap_update_bits(prueth->miig_rt, FDB_GEN_CFG1, SMEM_VLAN_OFFSET_MASK, 0); >>>> + regmap_write(prueth->miig_rt, FDB_GEN_CFG2, 0); >>>> + /* Clear host MAC address */ >>>> + icssg_class_set_host_mac_addr(prueth->miig_rt, mac); >>> >>> Seems an odd thing to do, set it to 00:00:00:00:00:00. You probably >>> want to add a comment why you do this odd thing. >> >> Actually, this is when the device is configured as a bridge, the host >> mac address has to be set to zero to while bringing it back to emac >> mode. I will add a comment to explain this. > > I don't see any switchdev interface. How does it get into bridge mode? I will be sending patches to add the switch mode support after this series gets merged. > >>>> + } else if (emac->link) { >>>> + new_state = true; >>>> + emac->link = 0; >>>> + /* defaults for no link */ >>>> + >>>> + /* f/w should support 100 & 1000 */ >>>> + emac->speed = SPEED_1000; >>>> + >>>> + /* half duplex may not supported by f/w */ >>>> + emac->duplex = DUPLEX_FULL; >>> >>> Why set speed and duplex when you have just lost the link? They are >>> meaningless until the link comes back. >> >> These were just the default values that we added. >> What do you suggest I put here? > > Nothing. If the link is down, they are meaningless. If something is > accessing them when the link is down, that code is broken. So i > suppose you could give them poison values to help find your broken > code. Okay, I will remove it in next version. > >>>> + for_each_child_of_node(eth_ports_node, eth_node) { >>>> + u32 reg; >>>> + >>>> + if (strcmp(eth_node->name, "port")) >>>> + continue; >>>> + ret = of_property_read_u32(eth_node, "reg", ®); >>>> + if (ret < 0) { >>>> + dev_err(dev, "%pOF error reading port_id %d\n", >>>> + eth_node, ret); >>>> + } >>>> + >>>> + if (reg == 0) >>>> + eth0_node = eth_node; >>>> + else if (reg == 1) >>>> + eth1_node = eth_node; >>> >>> and if reg == 4 >>> >>> Or reg 0 appears twice? >> >> In both of the cases that you mentioned, the device tree schema check >> will fail, hence, we can safely assume that this will be 0 and 1 only. > > Nothing forces you to run the scheme checker. It is not run by the > kernel before it starts accessing the DT blob. You should assume it is > invalid until you have proven it to be valid. I will add error checking here to make sure it is handled. > > Andrew Thanks, Puranjay Mohan