| Message ID | 20190524085921.11108-6-rasmus.villemoes@prevas.dk |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: dsa: support for mv88e6250 | expand |
> @@ -4841,6 +4910,10 @@ static const struct of_device_id mv88e6xxx_of_match[] = { > .compatible = "marvell,mv88e6190", > .data = &mv88e6xxx_table[MV88E6190], > }, > + { > + .compatible = "marvell,mv88e6250", > + .data = &mv88e6xxx_table[MV88E6250], > + }, > { /* sentinel */ }, > }; Ah, yes. I had not thought about that. A device at address 0 would be found, but a device at address 16 would be missed. Please add this compatible string to Documentation/devicetree/bindings/net/dsa/marvell.txt > +++ b/drivers/net/dsa/mv88e6xxx/global1.c > @@ -182,6 +182,25 @@ int mv88e6185_g1_reset(struct mv88e6xxx_chip *chip) > return mv88e6185_g1_wait_ppu_polling(chip); > } > > +int mv88e6250_g1_reset(struct mv88e6xxx_chip *chip) > +{ > + u16 val; > + int err; > + > + /* Set the SWReset bit 15 */ > + err = mv88e6xxx_g1_read(chip, MV88E6XXX_G1_CTL1, &val); > + if (err) > + return err; > + > + val |= MV88E6XXX_G1_CTL1_SW_RESET; > + > + err = mv88e6xxx_g1_write(chip, MV88E6XXX_G1_CTL1, val); > + if (err) > + return err; > + > + return mv88e6xxx_g1_wait_init_ready(chip); > +} It looks like you could refactor mv88e6352_g1_reset() to call this function, and then mv88e6352_g1_wait_ppu_polling(chip); Otherwise, this looks good. Andrew
Hi Rasmus, On Fri, 24 May 2019 09:00:31 +0000, Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote: > This is a very rough attempt at adding support for the Marvell > 88E6250. The _info and _ops structures are based on those for 6240 (as > I have data sheets for both the 6240 and 6250), fixing the things that > I have determined to be different for the two chips - but some things > are almost certain to still be wrong. The idea is that for things that you're not certain about, simply don't add the corresponding ops. The driver would simply return -EOPNOTSUPP for the related features (if it doesn't behave like this, we must fix this.) Thanks, Vivien
On 24/05/2019 16.27, Andrew Lunn wrote: >> @@ -4841,6 +4910,10 @@ static const struct of_device_id mv88e6xxx_of_match[] = { >> .compatible = "marvell,mv88e6190", >> .data = &mv88e6xxx_table[MV88E6190], >> }, >> + { >> + .compatible = "marvell,mv88e6250", >> + .data = &mv88e6xxx_table[MV88E6250], >> + }, >> { /* sentinel */ }, >> }; > > Ah, yes. I had not thought about that. A device at address 0 would be > found, but a device at address 16 would be missed. Eh, no? The port registers are at offset 0x8, i.e. at either SMI address 8 or 24, so I don't think a 6250 at address 0 could be detected using either of the existing families? > Please add this compatible string to Documentation/devicetree/bindings/net/dsa/marvell.txt Will do. >> +++ b/drivers/net/dsa/mv88e6xxx/global1.c >> @@ -182,6 +182,25 @@ int mv88e6185_g1_reset(struct mv88e6xxx_chip *chip) >> return mv88e6185_g1_wait_ppu_polling(chip); >> } >> >> +int mv88e6250_g1_reset(struct mv88e6xxx_chip *chip) >> +{ >> + u16 val; >> + int err; >> + >> + /* Set the SWReset bit 15 */ >> + err = mv88e6xxx_g1_read(chip, MV88E6XXX_G1_CTL1, &val); >> + if (err) >> + return err; >> + >> + val |= MV88E6XXX_G1_CTL1_SW_RESET; >> + >> + err = mv88e6xxx_g1_write(chip, MV88E6XXX_G1_CTL1, val); >> + if (err) >> + return err; >> + >> + return mv88e6xxx_g1_wait_init_ready(chip); >> +} > > It looks like you could refactor mv88e6352_g1_reset() to call > this function, and then mv88e6352_g1_wait_ppu_polling(chip); Yes, I actually deliberately moved the 6250 reset function further up in v2 to allow that. I'll add that refactoring as a separate patch. Thanks, Rasmus
On Mon, Jun 03, 2019 at 08:52:38AM +0000, Rasmus Villemoes wrote: > On 24/05/2019 16.27, Andrew Lunn wrote: > >> @@ -4841,6 +4910,10 @@ static const struct of_device_id mv88e6xxx_of_match[] = { > >> .compatible = "marvell,mv88e6190", > >> .data = &mv88e6xxx_table[MV88E6190], > >> }, > >> + { > >> + .compatible = "marvell,mv88e6250", > >> + .data = &mv88e6xxx_table[MV88E6250], > >> + }, > >> { /* sentinel */ }, > >> }; > > > > Ah, yes. I had not thought about that. A device at address 0 would be > > found, but a device at address 16 would be missed. > > Eh, no? The port registers are at offset 0x8, i.e. at either SMI address > 8 or 24, so I don't think a 6250 at address 0 could be detected using > either of the existing families? Even better. The real problem is, people keep trying to add new compatible strings here when they should not. The compatible string is about being able to read the ID registers, not to list every single switch chip family. This is one case where it really is needed, and i had not thought about that. Andrew
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index 28414db979b0..8cc9a76559ab 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -3448,6 +3448,51 @@ static const struct mv88e6xxx_ops mv88e6240_ops = { .phylink_validate = mv88e6352_phylink_validate, }; +static const struct mv88e6xxx_ops mv88e6250_ops = { + /* MV88E6XXX_FAMILY_6250 */ + .ieee_pri_map = mv88e6085_g1_ieee_pri_map, + .ip_pri_map = mv88e6085_g1_ip_pri_map, + .irl_init_all = mv88e6352_g2_irl_init_all, + .get_eeprom = mv88e6xxx_g2_get_eeprom16, + .set_eeprom = mv88e6xxx_g2_set_eeprom16, + .set_switch_mac = mv88e6xxx_g2_set_switch_mac, + .phy_read = mv88e6xxx_g2_smi_phy_read, + .phy_write = mv88e6xxx_g2_smi_phy_write, + .port_set_link = mv88e6xxx_port_set_link, + .port_set_duplex = mv88e6xxx_port_set_duplex, + .port_set_rgmii_delay = mv88e6352_port_set_rgmii_delay, + .port_set_speed = mv88e6352_port_set_speed, + .port_tag_remap = mv88e6095_port_tag_remap, + .port_set_frame_mode = mv88e6351_port_set_frame_mode, + .port_set_egress_floods = mv88e6352_port_set_egress_floods, + .port_set_ether_type = mv88e6351_port_set_ether_type, + .port_set_jumbo_size = mv88e6165_port_set_jumbo_size, + .port_egress_rate_limiting = mv88e6097_port_egress_rate_limiting, + .port_pause_limit = mv88e6097_port_pause_limit, + .port_disable_learn_limit = mv88e6xxx_port_disable_learn_limit, + .port_disable_pri_override = mv88e6xxx_port_disable_pri_override, + .port_link_state = mv88e6352_port_link_state, + .port_get_cmode = mv88e6352_port_get_cmode, + .stats_snapshot = mv88e6320_g1_stats_snapshot, + .stats_set_histogram = mv88e6095_g1_stats_set_histogram, + .stats_get_sset_count = mv88e6095_stats_get_sset_count, + .stats_get_strings = mv88e6095_stats_get_strings, + .stats_get_stats = mv88e6095_stats_get_stats, + .set_cpu_port = mv88e6095_g1_set_cpu_port, + .set_egress_port = mv88e6095_g1_set_egress_port, + .watchdog_ops = &mv88e6250_watchdog_ops, + .mgmt_rsvd2cpu = mv88e6352_g2_mgmt_rsvd2cpu, + .pot_clear = mv88e6xxx_g2_pot_clear, + .reset = mv88e6250_g1_reset, + .rmu_disable = mv88e6352_g1_rmu_disable, + .vtu_getnext = mv88e6250_g1_vtu_getnext, + .vtu_loadpurge = mv88e6250_g1_vtu_loadpurge, + .gpio_ops = &mv88e6352_gpio_ops, + .avb_ops = &mv88e6352_avb_ops, + .ptp_ops = &mv88e6352_ptp_ops, + .phylink_validate = mv88e6352_phylink_validate, +}; + static const struct mv88e6xxx_ops mv88e6290_ops = { /* MV88E6XXX_FAMILY_6390 */ .setup_errata = mv88e6390_setup_errata, @@ -4233,6 +4278,30 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .ops = &mv88e6240_ops, }, + [MV88E6250] = { + .prod_num = MV88E6XXX_PORT_SWITCH_ID_PROD_6250, + .family = MV88E6XXX_FAMILY_6250, + .name = "Marvell 88E6250", + .num_databases = 64, + .num_ports = 7, + .num_internal_phys = 5, + .num_gpio = 8, + .max_vid = 4095, + .port_base_addr = 0x08, + .phy_base_addr = 0x00, + .global1_addr = 0x0f, + .global2_addr = 0x07, + .age_time_coeff = 15000, + .g1_irqs = 9, + .g2_irqs = 10, + .atu_move_port_mask = 0xf, + .pvt = false, + .dual_chip = true, + .tag_protocol = DSA_TAG_PROTO_DSA, + .ptp_support = true, + .ops = &mv88e6250_ops, + }, + [MV88E6290] = { .prod_num = MV88E6XXX_PORT_SWITCH_ID_PROD_6290, .family = MV88E6XXX_FAMILY_6390, @@ -4841,6 +4910,10 @@ static const struct of_device_id mv88e6xxx_of_match[] = { .compatible = "marvell,mv88e6190", .data = &mv88e6xxx_table[MV88E6190], }, + { + .compatible = "marvell,mv88e6250", + .data = &mv88e6xxx_table[MV88E6250], + }, { /* sentinel */ }, }; diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h index 74777c3bc313..2fbe72b7587b 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.h +++ b/drivers/net/dsa/mv88e6xxx/chip.h @@ -62,6 +62,7 @@ enum mv88e6xxx_model { MV88E6190X, MV88E6191, MV88E6240, + MV88E6250, MV88E6290, MV88E6320, MV88E6321, @@ -80,6 +81,7 @@ enum mv88e6xxx_family { MV88E6XXX_FAMILY_6097, /* 6046 6085 6096 6097 */ MV88E6XXX_FAMILY_6165, /* 6123 6161 6165 */ MV88E6XXX_FAMILY_6185, /* 6108 6121 6122 6131 6152 6155 6182 6185 */ + MV88E6XXX_FAMILY_6250, /* 6250 */ MV88E6XXX_FAMILY_6320, /* 6320 6321 */ MV88E6XXX_FAMILY_6341, /* 6141 6341 */ MV88E6XXX_FAMILY_6351, /* 6171 6175 6350 6351 */ diff --git a/drivers/net/dsa/mv88e6xxx/global1.c b/drivers/net/dsa/mv88e6xxx/global1.c index 38e399e0f30e..b373feeed06c 100644 --- a/drivers/net/dsa/mv88e6xxx/global1.c +++ b/drivers/net/dsa/mv88e6xxx/global1.c @@ -182,6 +182,25 @@ int mv88e6185_g1_reset(struct mv88e6xxx_chip *chip) return mv88e6185_g1_wait_ppu_polling(chip); } +int mv88e6250_g1_reset(struct mv88e6xxx_chip *chip) +{ + u16 val; + int err; + + /* Set the SWReset bit 15 */ + err = mv88e6xxx_g1_read(chip, MV88E6XXX_G1_CTL1, &val); + if (err) + return err; + + val |= MV88E6XXX_G1_CTL1_SW_RESET; + + err = mv88e6xxx_g1_write(chip, MV88E6XXX_G1_CTL1, val); + if (err) + return err; + + return mv88e6xxx_g1_wait_init_ready(chip); +} + int mv88e6352_g1_reset(struct mv88e6xxx_chip *chip) { u16 val; diff --git a/drivers/net/dsa/mv88e6xxx/global1.h b/drivers/net/dsa/mv88e6xxx/global1.h index b205b0bba158..e1b7a6b68365 100644 --- a/drivers/net/dsa/mv88e6xxx/global1.h +++ b/drivers/net/dsa/mv88e6xxx/global1.h @@ -259,6 +259,7 @@ int mv88e6xxx_g1_set_switch_mac(struct mv88e6xxx_chip *chip, u8 *addr); int mv88e6185_g1_reset(struct mv88e6xxx_chip *chip); int mv88e6352_g1_reset(struct mv88e6xxx_chip *chip); +int mv88e6250_g1_reset(struct mv88e6xxx_chip *chip); int mv88e6185_g1_ppu_enable(struct mv88e6xxx_chip *chip); int mv88e6185_g1_ppu_disable(struct mv88e6xxx_chip *chip); diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h index 39c85e98fb92..541ef5c3f1d1 100644 --- a/drivers/net/dsa/mv88e6xxx/port.h +++ b/drivers/net/dsa/mv88e6xxx/port.h @@ -112,6 +112,7 @@ #define MV88E6XXX_PORT_SWITCH_ID_PROD_6191 0x1910 #define MV88E6XXX_PORT_SWITCH_ID_PROD_6185 0x1a70 #define MV88E6XXX_PORT_SWITCH_ID_PROD_6240 0x2400 +#define MV88E6XXX_PORT_SWITCH_ID_PROD_6250 0x2500 #define MV88E6XXX_PORT_SWITCH_ID_PROD_6290 0x2900 #define MV88E6XXX_PORT_SWITCH_ID_PROD_6321 0x3100 #define MV88E6XXX_PORT_SWITCH_ID_PROD_6141 0x3400
This is a very rough attempt at adding support for the Marvell 88E6250. The _info and _ops structures are based on those for 6240 (as I have data sheets for both the 6240 and 6250), fixing the things that I have determined to be different for the two chips - but some things are almost certain to still be wrong. The chip uses the new dual_chip option, and since its port registers start at SMI address 0x08 or 0x18 (i.e., always 0x08 + sw_addr), we need to introduce it as a new family in order for the auto-identification in mv88e6xxx_detect() to work. Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> --- drivers/net/dsa/mv88e6xxx/chip.c | 73 +++++++++++++++++++++++++++++ drivers/net/dsa/mv88e6xxx/chip.h | 2 + drivers/net/dsa/mv88e6xxx/global1.c | 19 ++++++++ drivers/net/dsa/mv88e6xxx/global1.h | 1 + drivers/net/dsa/mv88e6xxx/port.h | 1 + 5 files changed, 96 insertions(+)