| Message ID | 20220724092823.24567-1-arun.ramadoss@microchip.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | net: dsa: microchip: add support for phylink mac config and link up | expand |
On Sun, Jul 24, 2022 at 02:58:14PM +0530, Arun Ramadoss wrote: > This patch series add support common phylink mac config and link up for the ksz > series switches. At present, ksz8795 and ksz9477 doesn't implement the phylink > mac config and link up. It configures the mac interface in the port setup hook. > ksz8830 series switch does not mac link configuration. For lan937x switches, in > the part support patch series has support only for MII and RMII configuration. > Some group of switches have some register address and bit fields common and > others are different. So, this patch aims to have common phylink implementation > which configures the register based on the chip id. I don't see something problematic with this patch set. Reviewed-by: Vladimir Oltean <olteanv@gmail.com> > > Changes in v2 > - combined the modification of duplex, tx_pause and rx_pause into single > function. > > Changes in v1 > - Squash the reading rgmii value from dt to patch which apply the rgmii value > - Created the new function ksz_port_set_xmii_speed > - Seperated the namespace values for xmii_ctrl_0 and xmii_ctrl_1 register > - Applied the rgmii delay value based on the rx/tx-internal-delay-ps > > Arun Ramadoss (9): > net: dsa: microchip: add common gigabit set and get function > net: dsa: microchip: add common ksz port xmii speed selection function > net: dsa: microchip: add common duplex and flow control function > net: dsa: microchip: add support for common phylink mac link up > net: dsa: microchip: lan937x: add support for configuing xMII register > net: dsa: microchip: apply rgmii tx and rx delay in phylink mac config > net: dsa: microchip: ksz9477: use common xmii function > net: dsa: microchip: ksz8795: use common xmii function > net: dsa: microchip: add support for phylink mac config > > drivers/net/dsa/microchip/ksz8795.c | 40 --- > drivers/net/dsa/microchip/ksz8795_reg.h | 8 - > drivers/net/dsa/microchip/ksz9477.c | 183 +------------ > drivers/net/dsa/microchip/ksz9477_reg.h | 24 -- > drivers/net/dsa/microchip/ksz_common.c | 312 ++++++++++++++++++++++- > drivers/net/dsa/microchip/ksz_common.h | 54 ++++ > drivers/net/dsa/microchip/lan937x.h | 8 +- > drivers/net/dsa/microchip/lan937x_main.c | 125 +++------ > drivers/net/dsa/microchip/lan937x_reg.h | 32 ++- > 9 files changed, 431 insertions(+), 355 deletions(-) > > > base-commit: 502c6f8cedcce7889ccdefeb88ce36b39acd522f > -- > 2.36.1 >
Hello: This series was applied to netdev/net-next.git (master) by David S. Miller <davem@davemloft.net>: On Sun, 24 Jul 2022 14:58:14 +0530 you wrote: > This patch series add support common phylink mac config and link up for the ksz > series switches. At present, ksz8795 and ksz9477 doesn't implement the phylink > mac config and link up. It configures the mac interface in the port setup hook. > ksz8830 series switch does not mac link configuration. For lan937x switches, in > the part support patch series has support only for MII and RMII configuration. > Some group of switches have some register address and bit fields common and > others are different. So, this patch aims to have common phylink implementation > which configures the register based on the chip id. > > [...] Here is the summary with links: - [net-next,v2,1/9] net: dsa: microchip: add common gigabit set and get function https://git.kernel.org/netdev/net-next/c/46f80fa8981b - [net-next,v2,2/9] net: dsa: microchip: add common ksz port xmii speed selection function https://git.kernel.org/netdev/net-next/c/aa5b8b73d4bd - [net-next,v2,3/9] net: dsa: microchip: add common duplex and flow control function https://git.kernel.org/netdev/net-next/c/8560664fd32a - [net-next,v2,4/9] net: dsa: microchip: add support for common phylink mac link up https://git.kernel.org/netdev/net-next/c/da8cd08520f3 - [net-next,v2,5/9] net: dsa: microchip: lan937x: add support for configuing xMII register https://git.kernel.org/netdev/net-next/c/dc1c596edba5 - [net-next,v2,6/9] net: dsa: microchip: apply rgmii tx and rx delay in phylink mac config https://git.kernel.org/netdev/net-next/c/b19ac41faa3f - [net-next,v2,7/9] net: dsa: microchip: ksz9477: use common xmii function https://git.kernel.org/netdev/net-next/c/0ab7f6bf1675 - [net-next,v2,8/9] net: dsa: microchip: ksz8795: use common xmii function https://git.kernel.org/netdev/net-next/c/c476bede4b0f - [net-next,v2,9/9] net: dsa: microchip: add support for phylink mac config https://git.kernel.org/netdev/net-next/c/f3d890f5f90e You are awesome, thank you!
Hi Arun, starting with this patch set I have following regression on ksz8873 switch. Can you please take a look at it: 8<--- cut here --- Unable to handle kernel NULL pointer dereference at virtual address 00000005 ksz8863-switch gpio-0:00: nonfatal error -34 setting MTU to 1500 on port 0 ... Modules linked in: CPU: 0 PID: 16 Comm: kworker/0:1 Not tainted 6.0.0-rc2-00436-g3da285df1324 #74 Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree) Workqueue: events_power_efficient phylink_resolve PC is at ksz_set_gbit+0x5c/0xa4 LR is at arch_atomic_cmpxchg_relaxed+0x1c/0x38 .... Backtrace: ksz_set_gbit from ksz_phylink_mac_link_up+0x15c/0x1c8 ksz_phylink_mac_link_up from dsa_port_phylink_mac_link_up+0x7c/0x80 dsa_port_phylink_mac_link_up from phylink_resolve+0x304/0x3d0 phylink_resolve from process_one_work+0x214/0x31c process_one_work from worker_thread+0x254/0x2d4 worker_thread from kthread+0xfc/0x108 kthread from ret_from_fork+0x14/0x2c ... ksz8863-switch gpio-0:00 lan2 (uninitialized): PHY [dsa-0.0:01] driver [Micrel KSZ8851 Ethernet MAC or KSZ886X Switch] (irq=POLL) ksz8863-switch gpio-0:00: nonfatal error -34 setting MTU to 1500 on port 1 device eth0 entered promiscuous mode DSA: tree 0 setup ---[ end trace 0000000000000000 ]--- Regards, Oleksij On Sun, Jul 24, 2022 at 02:58:14PM +0530, Arun Ramadoss wrote: > This patch series add support common phylink mac config and link up for the ksz > series switches. At present, ksz8795 and ksz9477 doesn't implement the phylink > mac config and link up. It configures the mac interface in the port setup hook. > ksz8830 series switch does not mac link configuration. For lan937x switches, in > the part support patch series has support only for MII and RMII configuration. > Some group of switches have some register address and bit fields common and > others are different. So, this patch aims to have common phylink implementation > which configures the register based on the chip id. > Changes in v2 > - combined the modification of duplex, tx_pause and rx_pause into single > function. > > Changes in v1 > - Squash the reading rgmii value from dt to patch which apply the rgmii value > - Created the new function ksz_port_set_xmii_speed > - Seperated the namespace values for xmii_ctrl_0 and xmii_ctrl_1 register > - Applied the rgmii delay value based on the rx/tx-internal-delay-ps > > Arun Ramadoss (9): > net: dsa: microchip: add common gigabit set and get function > net: dsa: microchip: add common ksz port xmii speed selection function > net: dsa: microchip: add common duplex and flow control function > net: dsa: microchip: add support for common phylink mac link up > net: dsa: microchip: lan937x: add support for configuing xMII register > net: dsa: microchip: apply rgmii tx and rx delay in phylink mac config > net: dsa: microchip: ksz9477: use common xmii function > net: dsa: microchip: ksz8795: use common xmii function > net: dsa: microchip: add support for phylink mac config > > drivers/net/dsa/microchip/ksz8795.c | 40 --- > drivers/net/dsa/microchip/ksz8795_reg.h | 8 - > drivers/net/dsa/microchip/ksz9477.c | 183 +------------ > drivers/net/dsa/microchip/ksz9477_reg.h | 24 -- > drivers/net/dsa/microchip/ksz_common.c | 312 ++++++++++++++++++++++- > drivers/net/dsa/microchip/ksz_common.h | 54 ++++ > drivers/net/dsa/microchip/lan937x.h | 8 +- > drivers/net/dsa/microchip/lan937x_main.c | 125 +++------ > drivers/net/dsa/microchip/lan937x_reg.h | 32 ++- > 9 files changed, 431 insertions(+), 355 deletions(-) > > > base-commit: 502c6f8cedcce7889ccdefeb88ce36b39acd522f > -- > 2.36.1 >
Hi Oleksij, Is this Bug related to fix in https://lore.kernel.org/lkml/20220829105810.577903823@linuxfoundation.org/ . It is observed in ksz8794 switch. I think after applying this bug fix patch it should work. I don't have ksz8 series to test. I ran the regression only for ksz9 series switches. On Tue, 2022-08-30 at 08:55 +0200, Oleksij Rempel wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > Hi Arun, > > starting with this patch set I have following regression on ksz8873 > switch. Can you please take a look at it: > 8<--- cut here --- > Unable to handle kernel NULL pointer dereference at virtual address > 00000005 > ksz8863-switch gpio-0:00: nonfatal error -34 setting MTU to 1500 on > port 0 > ... > Modules linked in: > CPU: 0 PID: 16 Comm: kworker/0:1 Not tainted 6.0.0-rc2-00436- > g3da285df1324 #74 > Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree) > Workqueue: events_power_efficient phylink_resolve > PC is at ksz_set_gbit+0x5c/0xa4 > LR is at arch_atomic_cmpxchg_relaxed+0x1c/0x38 > .... > Backtrace: > ksz_set_gbit from ksz_phylink_mac_link_up+0x15c/0x1c8 > ksz_phylink_mac_link_up from dsa_port_phylink_mac_link_up+0x7c/0x80 > dsa_port_phylink_mac_link_up from phylink_resolve+0x304/0x3d0 > phylink_resolve from process_one_work+0x214/0x31c > process_one_work from worker_thread+0x254/0x2d4 > worker_thread from kthread+0xfc/0x108 > kthread from ret_from_fork+0x14/0x2c > ... > ksz8863-switch gpio-0:00 lan2 (uninitialized): PHY [dsa-0.0:01] > driver [Micrel KSZ8851 Ethernet MAC or KSZ886X Switch] (irq=POLL) > ksz8863-switch gpio-0:00: nonfatal error -34 setting MTU to 1500 on > port 1 > device eth0 entered promiscuous mode > DSA: tree 0 setup > ---[ end trace 0000000000000000 ]--- > > Regards, > Oleksij > > On Sun, Jul 24, 2022 at 02:58:14PM +0530, Arun Ramadoss wrote: > > This patch series add support common phylink mac config and link up > > for the ksz > > series switches. At present, ksz8795 and ksz9477 doesn't implement > > the phylink > > mac config and link up. It configures the mac interface in the port > > setup hook. > > ksz8830 series switch does not mac link configuration. For lan937x > > switches, in > > the part support patch series has support only for MII and RMII > > configuration. > > Some group of switches have some register address and bit fields > > common and > > others are different. So, this patch aims to have common phylink > > implementation > > which configures the register based on the chip id. > > Changes in v2 > > - combined the modification of duplex, tx_pause and rx_pause into > > single > > function. > > > > Changes in v1 > > - Squash the reading rgmii value from dt to patch which apply the > > rgmii value > > - Created the new function ksz_port_set_xmii_speed > > - Seperated the namespace values for xmii_ctrl_0 and xmii_ctrl_1 > > register > > - Applied the rgmii delay value based on the rx/tx-internal-delay- > > ps > > > > Arun Ramadoss (9): > > net: dsa: microchip: add common gigabit set and get function > > net: dsa: microchip: add common ksz port xmii speed selection > > function > > net: dsa: microchip: add common duplex and flow control function > > net: dsa: microchip: add support for common phylink mac link up > > net: dsa: microchip: lan937x: add support for configuing xMII > > register > > net: dsa: microchip: apply rgmii tx and rx delay in phylink mac > > config > > net: dsa: microchip: ksz9477: use common xmii function > > net: dsa: microchip: ksz8795: use common xmii function > > net: dsa: microchip: add support for phylink mac config > > > > drivers/net/dsa/microchip/ksz8795.c | 40 --- > > drivers/net/dsa/microchip/ksz8795_reg.h | 8 - > > drivers/net/dsa/microchip/ksz9477.c | 183 +------------ > > drivers/net/dsa/microchip/ksz9477_reg.h | 24 -- > > drivers/net/dsa/microchip/ksz_common.c | 312 > > ++++++++++++++++++++++- > > drivers/net/dsa/microchip/ksz_common.h | 54 ++++ > > drivers/net/dsa/microchip/lan937x.h | 8 +- > > drivers/net/dsa/microchip/lan937x_main.c | 125 +++------ > > drivers/net/dsa/microchip/lan937x_reg.h | 32 ++- > > 9 files changed, 431 insertions(+), 355 deletions(-) > > > > > > base-commit: 502c6f8cedcce7889ccdefeb88ce36b39acd522f > > -- > > 2.36.1 > > > > -- > Pengutronix > e.K. | | > Steuerwalder Str. 21 | > http://www.pengutronix.de/e/ | > 31137 Hildesheim, Germany | Phone: +49-5121-206917- > 0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- > 5555 |
Hello, On Tue, Aug 30, 2022 at 08:15:59AM +0000, Arun.Ramadoss@microchip.com wrote: > On Tue, 2022-08-30 at 08:55 +0200, Oleksij Rempel wrote: > > Hi Arun, > > > > starting with this patch set I have following regression on ksz8873 > > switch. Can you please take a look at it: > > 8<--- cut here --- > > Unable to handle kernel NULL pointer dereference at virtual address 00000005 > > ksz8863-switch gpio-0:00: nonfatal error -34 setting MTU to 1500 on port 0 > > ... > > Modules linked in: > > CPU: 0 PID: 16 Comm: kworker/0:1 Not tainted 6.0.0-rc2-00436- > > g3da285df1324 #74 > > Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree) > > Workqueue: events_power_efficient phylink_resolve > > PC is at ksz_set_gbit+0x5c/0xa4 > > LR is at arch_atomic_cmpxchg_relaxed+0x1c/0x38 > > .... > > Backtrace: > > ksz_set_gbit from ksz_phylink_mac_link_up+0x15c/0x1c8 > > ksz_phylink_mac_link_up from dsa_port_phylink_mac_link_up+0x7c/0x80 > > dsa_port_phylink_mac_link_up from phylink_resolve+0x304/0x3d0 > > phylink_resolve from process_one_work+0x214/0x31c > > process_one_work from worker_thread+0x254/0x2d4 > > worker_thread from kthread+0xfc/0x108 > > kthread from ret_from_fork+0x14/0x2c > > ... > > ksz8863-switch gpio-0:00 lan2 (uninitialized): PHY [dsa-0.0:01] driver [Micrel KSZ8851 Ethernet MAC or KSZ886X Switch] (irq=POLL) > > ksz8863-switch gpio-0:00: nonfatal error -34 setting MTU to 1500 on port 1 > > device eth0 entered promiscuous mode > > DSA: tree 0 setup > > ---[ end trace 0000000000000000 ]--- > > Hi Oleksij, > Is this Bug related to fix in > https://lore.kernel.org/lkml/20220829105810.577903823@linuxfoundation.org/ > . > It is observed in ksz8794 switch. I think after applying this bug fix > patch it should work. I don't have ksz8 series to test. I ran the > regression only for ksz9 series switches. I find it unlikely that the cited patch will fix a NULL pointer dereference in ksz_get_gbit(). But rather, some pointer to a structure is NULL, and we then dereference a member located at its offset 0x5, no? My eyes are on this: const u8 *bitval = dev->info->xmii_ctrl1; data8 |= FIELD_PREP(P_GMII_1GBIT_M, bitval[P_GMII_NOT_1GBIT]); ~~~~~~~~~~~~~~~~ this is coincidentally also 5 See, looking at the struct ksz_chip_data[] array element for KSZ8873 that Oleksij mentions as broken, I do not see xmii_ctrl1 and xmii_ctrl2 as being pointers to anything. [KSZ8830] = { .chip_id = KSZ8830_CHIP_ID, .dev_name = "KSZ8863/KSZ8873", .num_vlans = 16, .num_alus = 0, .num_statics = 8, .cpu_ports = 0x4, /* can be configured as cpu port */ .port_cnt = 3, .ops = &ksz8_dev_ops, .mib_names = ksz88xx_mib_names, .mib_cnt = ARRAY_SIZE(ksz88xx_mib_names), .reg_mib_cnt = MIB_COUNTER_NUM, .regs = ksz8863_regs, .masks = ksz8863_masks, .shifts = ksz8863_shifts, .supports_mii = {false, false, true}, .supports_rmii = {false, false, true}, .internal_phy = {true, true, false}, }, Should we point them to ksz8795_xmii_ctrl0 and ksz8795_xmii_ctrl1? I don't know. Could you find out what these should be set to?
On Tue, Aug 30, 2022 at 12:58:30PM +0300, Vladimir Oltean wrote: > Hello, > > On Tue, Aug 30, 2022 at 08:15:59AM +0000, Arun.Ramadoss@microchip.com wrote: ... > > Hi Oleksij, > > Is this Bug related to fix in > > https://lore.kernel.org/lkml/20220829105810.577903823@linuxfoundation.org/ > > . > > It is observed in ksz8794 switch. I think after applying this bug fix > > patch it should work. I don't have ksz8 series to test. I ran the > > regression only for ksz9 series switches. > > I find it unlikely that the cited patch will fix a NULL pointer > dereference in ksz_get_gbit(). But rather, some pointer to a structure > is NULL, and we then dereference a member located at its offset 0x5, no? > > My eyes are on this: > > const u8 *bitval = dev->info->xmii_ctrl1; > > data8 |= FIELD_PREP(P_GMII_1GBIT_M, bitval[P_GMII_NOT_1GBIT]); > ~~~~~~~~~~~~~~~~ > this is coincidentally > also 5 ack. > See, looking at the struct ksz_chip_data[] array element for KSZ8873 > that Oleksij mentions as broken, I do not see xmii_ctrl1 and xmii_ctrl2 > as being pointers to anything. > > [KSZ8830] = { > .chip_id = KSZ8830_CHIP_ID, > .dev_name = "KSZ8863/KSZ8873", > .num_vlans = 16, > .num_alus = 0, > .num_statics = 8, > .cpu_ports = 0x4, /* can be configured as cpu port */ > .port_cnt = 3, > .ops = &ksz8_dev_ops, > .mib_names = ksz88xx_mib_names, > .mib_cnt = ARRAY_SIZE(ksz88xx_mib_names), > .reg_mib_cnt = MIB_COUNTER_NUM, > .regs = ksz8863_regs, > .masks = ksz8863_masks, > .shifts = ksz8863_shifts, > .supports_mii = {false, false, true}, > .supports_rmii = {false, false, true}, > .internal_phy = {true, true, false}, > }, > > Should we point them to ksz8795_xmii_ctrl0 and ksz8795_xmii_ctrl1? I don't know. > Could you find out what these should be set to? xmii_ctrl0/1 are missing and it make no sense to add it. KSZ8873 switch is controlling CPU port MII configuration over global, not port based register. I'll better define separate ops for this chip. Regards, Oleksij
On Tue, Aug 30, 2022 at 06:05:46PM +0200, Oleksij Rempel wrote: > On Tue, Aug 30, 2022 at 12:58:30PM +0300, Vladimir Oltean wrote: > > Hello, > > > > On Tue, Aug 30, 2022 at 08:15:59AM +0000, Arun.Ramadoss@microchip.com wrote: > ... > > > Hi Oleksij, > > > Is this Bug related to fix in > > > https://lore.kernel.org/lkml/20220829105810.577903823@linuxfoundation.org/ > > > . > > > It is observed in ksz8794 switch. I think after applying this bug fix > > > patch it should work. I don't have ksz8 series to test. I ran the > > > regression only for ksz9 series switches. > > > > I find it unlikely that the cited patch will fix a NULL pointer > > dereference in ksz_get_gbit(). But rather, some pointer to a structure > > is NULL, and we then dereference a member located at its offset 0x5, no? > > > > My eyes are on this: > > > > const u8 *bitval = dev->info->xmii_ctrl1; > > > > data8 |= FIELD_PREP(P_GMII_1GBIT_M, bitval[P_GMII_NOT_1GBIT]); > > ~~~~~~~~~~~~~~~~ > > this is coincidentally > > also 5 > > ack. > > > See, looking at the struct ksz_chip_data[] array element for KSZ8873 > > that Oleksij mentions as broken, I do not see xmii_ctrl1 and xmii_ctrl2 > > as being pointers to anything. > > > > [KSZ8830] = { > > .chip_id = KSZ8830_CHIP_ID, > > .dev_name = "KSZ8863/KSZ8873", > > .num_vlans = 16, > > .num_alus = 0, > > .num_statics = 8, > > .cpu_ports = 0x4, /* can be configured as cpu port */ > > .port_cnt = 3, > > .ops = &ksz8_dev_ops, > > .mib_names = ksz88xx_mib_names, > > .mib_cnt = ARRAY_SIZE(ksz88xx_mib_names), > > .reg_mib_cnt = MIB_COUNTER_NUM, > > .regs = ksz8863_regs, > > .masks = ksz8863_masks, > > .shifts = ksz8863_shifts, > > .supports_mii = {false, false, true}, > > .supports_rmii = {false, false, true}, > > .internal_phy = {true, true, false}, > > }, > > > > Should we point them to ksz8795_xmii_ctrl0 and ksz8795_xmii_ctrl1? I don't know. > > Could you find out what these should be set to? > > xmii_ctrl0/1 are missing and it make no sense to add it. > KSZ8873 switch is controlling CPU port MII configuration over global, > not port based register. > > I'll better define separate ops for this chip. Hm, not only KSZ8830/KSZ8863/KSZ8873 are affected. ksz8795 compatible series with defined .xmii_ctrl0/.xmii_ctrl1 are broken too. Because it is writing to the global config register over ksz_pwrite8 function. It means, we are writing to 0xa6 instead of 0x06. And to 0xf6 instead of 0x56.
On Wed, Aug 31, 2022 at 09:43:24AM +0200, Oleksij Rempel wrote: > > > Should we point them to ksz8795_xmii_ctrl0 and ksz8795_xmii_ctrl1? I don't know. > > > Could you find out what these should be set to? > > > > xmii_ctrl0/1 are missing and it make no sense to add it. > > KSZ8873 switch is controlling CPU port MII configuration over global, > > not port based register. > > > > I'll better define separate ops for this chip. > > Hm, not only KSZ8830/KSZ8863/KSZ8873 are affected. ksz8795 compatible > series with defined .xmii_ctrl0/.xmii_ctrl1 are broken too. Because it > is writing to the global config register over ksz_pwrite8 function. It > means, we are writing to 0xa6 instead of 0x06. And to 0xf6 instead of > 0x56. What do you mean by "global config register"? The Is_1Gbps bit is still part of a port register, it's just that this particular register is only defined for the 5th port (port #4, the only xMII port on KSZ7895 AFAIU). That doesn't necessarily make it a "global" register. Datasheet says: Register 22 (0x16): Reserved Register 38 (0x26): Reserved Register 54 (0x36): Reserved Register 70 (0x46): Reserved Register 86 (0x56): Port 5 Interface Control 6 I wonder if it's ok to modify the regs table like this, because we should then only touch P_XMII_CTRL_1 using port 4: static const u16 ksz8795_regs[] = { [REG_IND_CTRL_0] = 0x6E, [REG_IND_DATA_8] = 0x70, [REG_IND_DATA_CHECK] = 0x72, [REG_IND_DATA_HI] = 0x71, [REG_IND_DATA_LO] = 0x75, [REG_IND_MIB_CHECK] = 0x74, [REG_IND_BYTE] = 0xA0, [P_FORCE_CTRL] = 0x0C, [P_LINK_STATUS] = 0x0E, [P_LOCAL_CTRL] = 0x07, [P_NEG_RESTART_CTRL] = 0x0D, [P_REMOTE_STATUS] = 0x08, [P_SPEED_STATUS] = 0x09, [S_TAIL_TAG_CTRL] = 0x0C, [P_STP_CTRL] = 0x02, [S_START_CTRL] = 0x01, [S_BROADCAST_CTRL] = 0x06, [S_MULTICAST_CTRL] = 0x04, [P_XMII_CTRL_0] = 0x06, - [P_XMII_CTRL_1] = 0x56, + [P_XMII_CTRL_1] = 0x06, }; Arun, what is your proposed solution?
On Wed, Aug 31, 2022 at 06:18:59PM +0300, Vladimir Oltean wrote: > On Wed, Aug 31, 2022 at 09:43:24AM +0200, Oleksij Rempel wrote: > > > > Should we point them to ksz8795_xmii_ctrl0 and ksz8795_xmii_ctrl1? I don't know. > > > > Could you find out what these should be set to? > > > > > > xmii_ctrl0/1 are missing and it make no sense to add it. > > > KSZ8873 switch is controlling CPU port MII configuration over global, > > > not port based register. > > > > > > I'll better define separate ops for this chip. > > > > Hm, not only KSZ8830/KSZ8863/KSZ8873 are affected. ksz8795 compatible > > series with defined .xmii_ctrl0/.xmii_ctrl1 are broken too. Because it > > is writing to the global config register over ksz_pwrite8 function. It > > means, we are writing to 0xa6 instead of 0x06. And to 0xf6 instead of > > 0x56. > > What do you mean by "global config register"? The Is_1Gbps bit is still > part of a port register, it's just that this particular register is only > defined for the 5th port (port #4, the only xMII port on KSZ7895 AFAIU). > That doesn't necessarily make it a "global" register. > > Datasheet says: > > Register 22 (0x16): Reserved > Register 38 (0x26): Reserved > Register 54 (0x36): Reserved > Register 70 (0x46): Reserved > Register 86 (0x56): Port 5 Interface Control 6 > > I wonder if it's ok to modify the regs table like this, because we > should then only touch P_XMII_CTRL_1 using port 4: > > static const u16 ksz8795_regs[] = { > [REG_IND_CTRL_0] = 0x6E, > [REG_IND_DATA_8] = 0x70, > [REG_IND_DATA_CHECK] = 0x72, > [REG_IND_DATA_HI] = 0x71, > [REG_IND_DATA_LO] = 0x75, > [REG_IND_MIB_CHECK] = 0x74, > [REG_IND_BYTE] = 0xA0, > [P_FORCE_CTRL] = 0x0C, > [P_LINK_STATUS] = 0x0E, > [P_LOCAL_CTRL] = 0x07, > [P_NEG_RESTART_CTRL] = 0x0D, > [P_REMOTE_STATUS] = 0x08, > [P_SPEED_STATUS] = 0x09, > [S_TAIL_TAG_CTRL] = 0x0C, > [P_STP_CTRL] = 0x02, > [S_START_CTRL] = 0x01, > [S_BROADCAST_CTRL] = 0x06, > [S_MULTICAST_CTRL] = 0x04, > [P_XMII_CTRL_0] = 0x06, > - [P_XMII_CTRL_1] = 0x56, > + [P_XMII_CTRL_1] = 0x06, > }; > Speed configuration on ksz8795 is done over two registers: Register 86 (0x56): Port 5 Interface Control 6: Is_1Gbps - BIT(6) and Register 6 (0x06): Global Control 4: Switch SW5-MII/RMII Speed -BIT(4) both are accessed on wrong offsets. I would prefer to do following steps: - remove everything from ksz_phylink_mac_link_up() except of dev->dev_ops->phylink_mac_link_up - move ksz_duplex_flowctrl(), ksz_port_set_xmii_speed()... to ksz9477.c and rename them. Assign ksz9477_phylink_mac_link_up() dev->dev_ops->phylink_mac_link_up - create separate function ksz8795_phylink_mac_link_up() - use documented, not generic register names.
On Wed, 2022-08-31 at 18:10 +0200, Oleksij Rempel wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On Wed, Aug 31, 2022 at 06:18:59PM +0300, Vladimir Oltean wrote: > > On Wed, Aug 31, 2022 at 09:43:24AM +0200, Oleksij Rempel wrote: > > > > > Should we point them to ksz8795_xmii_ctrl0 and > > > > > ksz8795_xmii_ctrl1? I don't know. > > > > > Could you find out what these should be set to? > > > > > > > > xmii_ctrl0/1 are missing and it make no sense to add it. > > > > KSZ8873 switch is controlling CPU port MII configuration over > > > > global, > > > > not port based register. > > > > > > > > I'll better define separate ops for this chip. > > > > > > Hm, not only KSZ8830/KSZ8863/KSZ8873 are affected. ksz8795 > > > compatible > > > series with defined .xmii_ctrl0/.xmii_ctrl1 are broken too. > > > Because it > > > is writing to the global config register over ksz_pwrite8 > > > function. It > > > means, we are writing to 0xa6 instead of 0x06. And to 0xf6 > > > instead of > > > 0x56. > > What do you mean by "global config register"? The Is_1Gbps bit is > > still > > part of a port register, it's just that this particular register is > > only > > defined for the 5th port (port #4, the only xMII port on KSZ7895 > > AFAIU). > > That doesn't necessarily make it a "global" register. > > > > Datasheet says: > > > > Register 22 (0x16): Reserved > > Register 38 (0x26): Reserved > > Register 54 (0x36): Reserved > > Register 70 (0x46): Reserved > > Register 86 (0x56): Port 5 Interface Control 6 > > > > I wonder if it's ok to modify the regs table like this, because we > > should then only touch P_XMII_CTRL_1 using port 4: > > > > static const u16 ksz8795_regs[] = { > > [REG_IND_CTRL_0] = 0x6E, > > [REG_IND_DATA_8] = 0x70, > > [REG_IND_DATA_CHECK] = 0x72, > > [REG_IND_DATA_HI] = 0x71, > > [REG_IND_DATA_LO] = 0x75, > > [REG_IND_MIB_CHECK] = 0x74, > > [REG_IND_BYTE] = 0xA0, > > [P_FORCE_CTRL] = 0x0C, > > [P_LINK_STATUS] = 0x0E, > > [P_LOCAL_CTRL] = 0x07, > > [P_NEG_RESTART_CTRL] = 0x0D, > > [P_REMOTE_STATUS] = 0x08, > > [P_SPEED_STATUS] = 0x09, > > [S_TAIL_TAG_CTRL] = 0x0C, > > [P_STP_CTRL] = 0x02, > > [S_START_CTRL] = 0x01, > > [S_BROADCAST_CTRL] = 0x06, > > [S_MULTICAST_CTRL] = 0x04, > > [P_XMII_CTRL_0] = 0x06, > > - [P_XMII_CTRL_1] = 0x56, > > + [P_XMII_CTRL_1] = 0x06, > > }; > > > > Speed configuration on ksz8795 is done over two registers: > Register 86 (0x56): Port 5 Interface Control 6: Is_1Gbps - BIT(6) > and > Register 6 (0x06): Global Control 4: Switch SW5-MII/RMII Speed > -BIT(4) > > both are accessed on wrong offsets. > > I would prefer to do following steps: > - remove everything from ksz_phylink_mac_link_up() except of > dev->dev_ops->phylink_mac_link_up > - move ksz_duplex_flowctrl(), ksz_port_set_xmii_speed()... to > ksz9477.c > and rename them. Assign ksz9477_phylink_mac_link_up() > dev->dev_ops->phylink_mac_link_up > - create separate function ksz8795_phylink_mac_link_up() > - use documented, not generic register names. > In the original code, the ksz8795_cpu_interface_select (which does the xmii and speed configuration) is called only ksz87xx switch not for the ksz88x3. During refactoring, I intentionally did not add the P_XMII_CTRL0/1 for the ksz88xx in the chip_data structure. I added check that if switch belong to ksz88x3 then return in the phylink_mac_config function but I missed to add in the phylink_mac_link_up. Thats the mistake I have done. So, for the ksz88xx switch code breakage, it could be fixed by adding the check in the phylink_mac_link_up as well. For the code breakage in ksz8795/ksz8794, the original code has only provision for configuring xmii mode and choosing the 100/1000Mbps speed selection which could be selected using is_1Gbps bit of port5_interface_control6 register (0x56). But it didn't have provision to select speed between 10/100Mbps, flow control and duplex. As per vladimir suggestion, P_XMII_CTRL1 can be changed from 0x56 to 0x06. It fixes the problem for ksz_set_xmii, ksz_set_gbit since this is the port based register not the global register. The global register 0x06 responsibilities are bit 4 for 10/100mbps speed selection, bit 5 for flow control and bit 6 for duplex operation. Since these three are new features added during refactoring I overlooked it. To fix this, either I need to return from the ksz_set_100_10mbit & ksz_duplex_flowctrl function if the chip_id is ksz87xx or add dev- >dev_ops for this alone. Kindly suggest on how to proceed. > -- > Pengutronix > e.K. | | > Steuerwalder Str. 21 | > http://www.pengutronix.de/e/ | > 31137 Hildesheim, Germany | Phone: +49-5121-206917- > 0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- > 5555 |
On Thu, Sep 01, 2022 at 08:51:44AM +0000, Arun.Ramadoss@microchip.com wrote: > On Wed, 2022-08-31 at 18:10 +0200, Oleksij Rempel wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you > > know the content is safe > > > > On Wed, Aug 31, 2022 at 06:18:59PM +0300, Vladimir Oltean wrote: > > > On Wed, Aug 31, 2022 at 09:43:24AM +0200, Oleksij Rempel wrote: > > > > > > Should we point them to ksz8795_xmii_ctrl0 and > > > > > > ksz8795_xmii_ctrl1? I don't know. > > > > > > Could you find out what these should be set to? > > > > > > > > > > xmii_ctrl0/1 are missing and it make no sense to add it. > > > > > KSZ8873 switch is controlling CPU port MII configuration over > > > > > global, > > > > > not port based register. > > > > > > > > > > I'll better define separate ops for this chip. > > > > > > > > Hm, not only KSZ8830/KSZ8863/KSZ8873 are affected. ksz8795 > > > > compatible > > > > series with defined .xmii_ctrl0/.xmii_ctrl1 are broken too. > > > > Because it > > > > is writing to the global config register over ksz_pwrite8 > > > > function. It > > > > means, we are writing to 0xa6 instead of 0x06. And to 0xf6 > > > > instead of > > > > 0x56. > > > What do you mean by "global config register"? The Is_1Gbps bit is > > > still > > > part of a port register, it's just that this particular register is > > > only > > > defined for the 5th port (port #4, the only xMII port on KSZ7895 > > > AFAIU). > > > That doesn't necessarily make it a "global" register. > > > > > > Datasheet says: > > > > > > Register 22 (0x16): Reserved > > > Register 38 (0x26): Reserved > > > Register 54 (0x36): Reserved > > > Register 70 (0x46): Reserved > > > Register 86 (0x56): Port 5 Interface Control 6 > > > > > > I wonder if it's ok to modify the regs table like this, because we > > > should then only touch P_XMII_CTRL_1 using port 4: > > > > > > static const u16 ksz8795_regs[] = { > > > [REG_IND_CTRL_0] = 0x6E, > > > [REG_IND_DATA_8] = 0x70, > > > [REG_IND_DATA_CHECK] = 0x72, > > > [REG_IND_DATA_HI] = 0x71, > > > [REG_IND_DATA_LO] = 0x75, > > > [REG_IND_MIB_CHECK] = 0x74, > > > [REG_IND_BYTE] = 0xA0, > > > [P_FORCE_CTRL] = 0x0C, > > > [P_LINK_STATUS] = 0x0E, > > > [P_LOCAL_CTRL] = 0x07, > > > [P_NEG_RESTART_CTRL] = 0x0D, > > > [P_REMOTE_STATUS] = 0x08, > > > [P_SPEED_STATUS] = 0x09, > > > [S_TAIL_TAG_CTRL] = 0x0C, > > > [P_STP_CTRL] = 0x02, > > > [S_START_CTRL] = 0x01, > > > [S_BROADCAST_CTRL] = 0x06, > > > [S_MULTICAST_CTRL] = 0x04, > > > [P_XMII_CTRL_0] = 0x06, > > > - [P_XMII_CTRL_1] = 0x56, > > > + [P_XMII_CTRL_1] = 0x06, > > > }; > > > > > > > Speed configuration on ksz8795 is done over two registers: > > Register 86 (0x56): Port 5 Interface Control 6: Is_1Gbps - BIT(6) > > and > > Register 6 (0x06): Global Control 4: Switch SW5-MII/RMII Speed > > -BIT(4) > > > > both are accessed on wrong offsets. > > > > I would prefer to do following steps: > > - remove everything from ksz_phylink_mac_link_up() except of > > dev->dev_ops->phylink_mac_link_up > > - move ksz_duplex_flowctrl(), ksz_port_set_xmii_speed()... to > > ksz9477.c > > and rename them. Assign ksz9477_phylink_mac_link_up() > > dev->dev_ops->phylink_mac_link_up > > - create separate function ksz8795_phylink_mac_link_up() > > - use documented, not generic register names. > > > > In the original code, the ksz8795_cpu_interface_select (which does the > xmii and speed configuration) is called only ksz87xx switch not for the > ksz88x3. > During refactoring, I intentionally did not add the P_XMII_CTRL0/1 for > the ksz88xx in the chip_data structure. > I added check that if switch belong to ksz88x3 then return in the > phylink_mac_config function but I missed to add in the > phylink_mac_link_up. Thats the mistake I have done. So, for the ksz88xx > switch code breakage, it could be fixed by adding the check in the > phylink_mac_link_up as well. > > For the code breakage in ksz8795/ksz8794, the original code has only > provision for configuring xmii mode and choosing the 100/1000Mbps speed > selection which could be selected using is_1Gbps bit of > port5_interface_control6 register (0x56). But it didn't have provision > to select speed between 10/100Mbps, flow control and duplex. > > As per vladimir suggestion, P_XMII_CTRL1 can be changed from 0x56 to > 0x06. It fixes the problem for ksz_set_xmii, ksz_set_gbit since this is > the port based register not the global register. > > The global register 0x06 responsibilities are bit 4 for 10/100mbps > speed selection, bit 5 for flow control and bit 6 for duplex > operation. Since these three are new features added during refactoring > I overlooked it. > To fix this, either I need to return from the ksz_set_100_10mbit & > ksz_duplex_flowctrl function if the chip_id is ksz87xx or add dev- > >dev_ops for this alone. > Kindly suggest on how to proceed. Do not worry, every one makes mistakes ;) I just scared how easy it is to make them in this driver. I assume, that initially the good idea to make things more generic in this driver is working against us. The register- and bit-mapping functionally is already biting us: - it is hard to review, due to different register naming schema for different switch families. Especially the old ones. - we are using naming from one documentation to map to other documentation. - some times we need to map some registers multiple times with different names: P_REMOTE_STATUS and P_LINK_STATUS I would prefer to got ops way, to clean things up. Regards, Oleksij
On Thu, Sep 01, 2022 at 01:27:21PM +0200, Oleksij Rempel wrote: > > The global register 0x06 responsibilities are bit 4 for 10/100mbps > > speed selection, bit 5 for flow control and bit 6 for duplex > > operation. Since these three are new features added during refactoring > > I overlooked it. > > To fix this, either I need to return from the ksz_set_100_10mbit & > > ksz_duplex_flowctrl function if the chip_id is ksz87xx or add > > dev->dev_ops for this alone. Kindly suggest on how to proceed. > > I would prefer to got ops way, to clean things up. I can't say that that one approach is better or worse than the other. Indirect function calls are going to be more expensive than conditionals on dev->chip_id, but we aren't in a fast path here, so it doesn't matter too much. Having indirect function calls will in theory help simplify the logic of the main function, but will require good forethought for what constitutes an atom of functionality, in a high enough level such as to abstract switch differences. Whereas conditionals don't require thinking that far, you put them where you need them. Also, indirect function calls will move the bloat somewhere else. I have seen complaints in the past about the mv88e6xxx driver's layered structure, making it difficult to see exactly what gets done for a certain chip. It is probable that we don't want to mix these styles too much within a single driver, so if work has already started towards dev_ops for everything, then dev_ops be it, I guess. Oleksij, are you going to submit patches with your proposal?
Hi Vladimir, On Thu, Sep 01, 2022 at 03:47:37PM +0300, Vladimir Oltean wrote: > On Thu, Sep 01, 2022 at 01:27:21PM +0200, Oleksij Rempel wrote: > > > The global register 0x06 responsibilities are bit 4 for 10/100mbps > > > speed selection, bit 5 for flow control and bit 6 for duplex > > > operation. Since these three are new features added during refactoring > > > I overlooked it. > > > To fix this, either I need to return from the ksz_set_100_10mbit & > > > ksz_duplex_flowctrl function if the chip_id is ksz87xx or add > > > dev->dev_ops for this alone. Kindly suggest on how to proceed. > > > > I would prefer to got ops way, to clean things up. > > I can't say that that one approach is better or worse than the other. > Indirect function calls are going to be more expensive than conditionals > on dev->chip_id, but we aren't in a fast path here, so it doesn't matter > too much. > > Having indirect function calls will in theory help simplify the logic of > the main function, but will require good forethought for what constitutes > an atom of functionality, in a high enough level such as to abstract > switch differences. Whereas conditionals don't require thinking that far, > you put them where you need them. > > Also, indirect function calls will move the bloat somewhere else. I have > seen complaints in the past about the mv88e6xxx driver's layered structure, > making it difficult to see exactly what gets done for a certain chip. > > It is probable that we don't want to mix these styles too much within a > single driver, so if work has already started towards dev_ops for > everything, then dev_ops be it, I guess. > > Oleksij, are you going to submit patches with your proposal? I have send one simple patch for net to make it work. After this one will pop-up in then net-next i'll send other patches depending on this patch. Regards, Oleksij