Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1422197 > unrolled thread

[PATCH v2 net-next v2 12/12] net: dsa: mv88e6xxx: add addressing mode to info

Started byVivien Didelot <vivien.didelot@savoirfairelinux.com>
First post2016-06-14 20:40 +0200
Last post2016-06-15 01:20 +0200
Articles 7 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 net-next v2 12/12] net: dsa: mv88e6xxx: add addressing mode to info Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2016-06-14 20:40 +0200
    Re: [PATCH v2 net-next v2 12/12] net: dsa: mv88e6xxx: add addressing  mode to info Andrew Lunn <andrew@lunn.ch> - 2016-06-14 23:40 +0200
      Re: [PATCH v2 net-next v2 12/12] net: dsa: mv88e6xxx: add addressing mode to info Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2016-06-15 00:00 +0200
    Re: [PATCH v2 net-next v2 12/12] net: dsa: mv88e6xxx: add addressing  mode to info Andrew Lunn <andrew@lunn.ch> - 2016-06-15 00:10 +0200
      Re: [PATCH v2 net-next v2 12/12] net: dsa: mv88e6xxx: add addressing mode to info Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2016-06-15 00:30 +0200
        Re: [PATCH v2 net-next v2 12/12] net: dsa: mv88e6xxx: add addressing  mode to info Andrew Lunn <andrew@lunn.ch> - 2016-06-15 00:50 +0200
          Re: [PATCH v2 net-next v2 12/12] net: dsa: mv88e6xxx: add addressing mode to info Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2016-06-15 01:20 +0200

#1422197 — [PATCH v2 net-next v2 12/12] net: dsa: mv88e6xxx: add addressing mode to info

FromVivien Didelot <vivien.didelot@savoirfairelinux.com>
Date2016-06-14 20:40 +0200
Subject[PATCH v2 net-next v2 12/12] net: dsa: mv88e6xxx: add addressing mode to info
Message-ID<rK1pM-88-19@gated-at.bofh.it>
When the SMI address of the switch chip on the SMI master bus is not
zero, some chips (e.g. 88E6352) use an indirect access through two SMI
Command and Data registers, while others (e.g. 88E6060) still use a
direct access.

Add a capability flag to describe chips supporting the Multi-chip
Addressing Mode.

Use the SMI indirect access ops only for switches with this flag and
change the direct SMI direct access ops to support non-zero chip
address.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6xxx.c |  6 +++---
 drivers/net/dsa/mv88e6xxx.h | 16 +++++++++++++++-
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index fc28a6c..8e12246 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -52,7 +52,7 @@ static int mv88e6xxx_smi_direct_read(struct mii_bus *bus, int sw_addr,
 {
 	int ret;
 
-	ret = mdiobus_read_nested(bus, addr, reg);
+	ret = mdiobus_read_nested(bus, sw_addr + addr, reg);
 	if (ret < 0)
 		return ret;
 
@@ -66,7 +66,7 @@ static int mv88e6xxx_smi_direct_write(struct mii_bus *bus, int sw_addr,
 {
 	int ret;
 
-	ret = mdiobus_write_nested(bus, addr, reg, val);
+	ret = mdiobus_write_nested(bus, sw_addr + addr, reg, val);
 	if (ret < 0)
 		return ret;
 
@@ -3681,7 +3681,7 @@ mv88e6xxx_smi_detect(struct device *dev, struct mii_bus *bus, int sw_addr,
 	u16 id;
 
 	ops = &mv88e6xxx_smi_direct_ops;
-	if (sw_addr > 0)
+	if (sw_addr > 0 && info->flags & MV88E6XXX_FLAG_MULTI_CHIP)
 		ops = &mv88e6xxx_smi_indirect_ops;
 
 	if (ops->read(bus, sw_addr, info->port_base_addr, PORT_SWITCH_ID, &id))
diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
index 100e4e6..3a5ef6a 100644
--- a/drivers/net/dsa/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx.h
@@ -387,6 +387,12 @@ enum mv88e6xxx_cap {
 	 */
 	MV88E6XXX_CAP_EEPROM,
 
+	/* Multi-chip Addressing Mode.
+	 * Some chips require an indirect SMI access when their SMI device
+	 * address is not zero. to See SMI Command and Data Registers.
+	 */
+	MV88E6XXX_CAP_MULTI_CHIP,
+
 	/* Port State Filtering for 802.1D Spanning Tree.
 	 * See PORT_CONTROL_STATE_* values in the PORT_CONTROL register.
 	 */
@@ -439,6 +445,7 @@ enum mv88e6xxx_cap {
 #define MV88E6XXX_FLAG_ATU		BIT(MV88E6XXX_CAP_ATU)
 #define MV88E6XXX_FLAG_EEE		BIT(MV88E6XXX_CAP_EEE)
 #define MV88E6XXX_FLAG_EEPROM		BIT(MV88E6XXX_CAP_EEPROM)
+#define MV88E6XXX_FLAG_MULTI_CHIP	BIT(MV88E6XXX_CAP_MULTI_CHIP)
 #define MV88E6XXX_FLAG_PORTSTATE	BIT(MV88E6XXX_CAP_PORTSTATE)
 #define MV88E6XXX_FLAG_PPU		BIT(MV88E6XXX_CAP_PPU)
 #define MV88E6XXX_FLAG_PPU_ACTIVE	BIT(MV88E6XXX_CAP_PPU_ACTIVE)
@@ -452,25 +459,29 @@ enum mv88e6xxx_cap {
 
 #define MV88E6XXX_FLAGS_FAMILY_6095	\
 	(MV88E6XXX_FLAG_ATU |		\
+	 MV88E6XXX_FLAG_MULTI_CHIP |	\
 	 MV88E6XXX_FLAG_PPU |		\
 	 MV88E6XXX_FLAG_VLANTABLE |	\
 	 MV88E6XXX_FLAG_VTU)
 
 #define MV88E6XXX_FLAGS_FAMILY_6097	\
 	(MV88E6XXX_FLAG_ATU |		\
+	 MV88E6XXX_FLAG_MULTI_CHIP |	\
 	 MV88E6XXX_FLAG_PPU |		\
 	 MV88E6XXX_FLAG_STU |		\
 	 MV88E6XXX_FLAG_VLANTABLE |	\
 	 MV88E6XXX_FLAG_VTU)
 
 #define MV88E6XXX_FLAGS_FAMILY_6165	\
-	(MV88E6XXX_FLAG_STU |		\
+	(MV88E6XXX_FLAG_MULTI_CHIP |	\
+	 MV88E6XXX_FLAG_STU |		\
 	 MV88E6XXX_FLAG_SWITCH_MAC |	\
 	 MV88E6XXX_FLAG_TEMP |		\
 	 MV88E6XXX_FLAG_VTU)
 
 #define MV88E6XXX_FLAGS_FAMILY_6185	\
 	(MV88E6XXX_FLAG_ATU |		\
+	 MV88E6XXX_FLAG_MULTI_CHIP |	\
 	 MV88E6XXX_FLAG_PPU |		\
 	 MV88E6XXX_FLAG_VLANTABLE |	\
 	 MV88E6XXX_FLAG_VTU)
@@ -479,6 +490,7 @@ enum mv88e6xxx_cap {
 	(MV88E6XXX_FLAG_ATU |		\
 	 MV88E6XXX_FLAG_EEE |		\
 	 MV88E6XXX_FLAG_EEPROM |	\
+	 MV88E6XXX_FLAG_MULTI_CHIP |	\
 	 MV88E6XXX_FLAG_PORTSTATE |	\
 	 MV88E6XXX_FLAG_PPU_ACTIVE |	\
 	 MV88E6XXX_FLAG_SMI_PHY |	\
@@ -490,6 +502,7 @@ enum mv88e6xxx_cap {
 
 #define MV88E6XXX_FLAGS_FAMILY_6351	\
 	(MV88E6XXX_FLAG_ATU |		\
+	 MV88E6XXX_FLAG_MULTI_CHIP |	\
 	 MV88E6XXX_FLAG_PORTSTATE |	\
 	 MV88E6XXX_FLAG_PPU_ACTIVE |	\
 	 MV88E6XXX_FLAG_SMI_PHY |	\
@@ -503,6 +516,7 @@ enum mv88e6xxx_cap {
 	(MV88E6XXX_FLAG_ATU |		\
 	 MV88E6XXX_FLAG_EEE |		\
 	 MV88E6XXX_FLAG_EEPROM |	\
+	 MV88E6XXX_FLAG_MULTI_CHIP |	\
 	 MV88E6XXX_FLAG_PORTSTATE |	\
 	 MV88E6XXX_FLAG_PPU_ACTIVE |	\
 	 MV88E6XXX_FLAG_SMI_PHY |	\
-- 
2.8.3

[toc] | [next] | [standalone]

#1422361 — Re: [PATCH v2 net-next v2 12/12] net: dsa: mv88e6xxx: add addressing mode to info

FromAndrew Lunn <andrew@lunn.ch>
Date2016-06-14 23:40 +0200
SubjectRe: [PATCH v2 net-next v2 12/12] net: dsa: mv88e6xxx: add addressing mode to info
Message-ID<rK4dY-1WX-15@gated-at.bofh.it>
In reply to#1422197
> @@ -3681,7 +3681,7 @@ mv88e6xxx_smi_detect(struct device *dev, struct mii_bus *bus, int sw_addr,
>  	u16 id;
>  
>  	ops = &mv88e6xxx_smi_direct_ops;
> -	if (sw_addr > 0)
> +	if (sw_addr > 0 && info->flags & MV88E6XXX_FLAG_MULTI_CHIP)
>  		ops = &mv88e6xxx_smi_indirect_ops;

Hi Vivien

Is sw_addr is > 0 and MV88E6XXX_FLAG_MULTI_CHIP is not set, you should
return -EINVAL. The device tree is invalid.

       Andrew

[toc] | [prev] | [next] | [standalone]

#1422369

FromVivien Didelot <vivien.didelot@savoirfairelinux.com>
Date2016-06-15 00:00 +0200
Message-ID<rK4xp-23y-17@gated-at.bofh.it>
In reply to#1422361
Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

>> @@ -3681,7 +3681,7 @@ mv88e6xxx_smi_detect(struct device *dev, struct mii_bus *bus, int sw_addr,
>>  	u16 id;
>>  
>>  	ops = &mv88e6xxx_smi_direct_ops;
>> -	if (sw_addr > 0)
>> +	if (sw_addr > 0 && info->flags & MV88E6XXX_FLAG_MULTI_CHIP)
>>  		ops = &mv88e6xxx_smi_indirect_ops;
>
> Is sw_addr is > 0 and MV88E6XXX_FLAG_MULTI_CHIP is not set, you should
> return -EINVAL. The device tree is invalid.

OK, I'll change this snippet for the following until we explicitly add
support for such device with non-zero address and direct SMI access:

    if (sw_addr == 0)
        ops = &mv88e6xxx_smi_direct_ops;
    else if (info->flags & MV88E6XXX_FLAG_MULTI_CHIP)
        ops = &mv88e6xxx_smi_indirect_ops;
    else
        return NULL;

Thanks,

        Vivien

[toc] | [prev] | [next] | [standalone]

#1422372 — Re: [PATCH v2 net-next v2 12/12] net: dsa: mv88e6xxx: add addressing mode to info

FromAndrew Lunn <andrew@lunn.ch>
Date2016-06-15 00:10 +0200
SubjectRe: [PATCH v2 net-next v2 12/12] net: dsa: mv88e6xxx: add addressing mode to info
Message-ID<rK4GZ-2lJ-1@gated-at.bofh.it>
In reply to#1422197
On Tue, Jun 14, 2016 at 02:31:53PM -0400, Vivien Didelot wrote:
> When the SMI address of the switch chip on the SMI master bus is not
> zero, some chips (e.g. 88E6352) use an indirect access through two SMI
> Command and Data registers, while others (e.g. 88E6060) still use a
> direct access.
> 
> Add a capability flag to describe chips supporting the Multi-chip
> Addressing Mode.
> 
> Use the SMI indirect access ops only for switches with this flag and
> change the direct SMI direct access ops to support non-zero chip
> address.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
>  drivers/net/dsa/mv88e6xxx.c |  6 +++---
>  drivers/net/dsa/mv88e6xxx.h | 16 +++++++++++++++-
>  2 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
> index fc28a6c..8e12246 100644
> --- a/drivers/net/dsa/mv88e6xxx.c
> +++ b/drivers/net/dsa/mv88e6xxx.c
> @@ -52,7 +52,7 @@ static int mv88e6xxx_smi_direct_read(struct mii_bus *bus, int sw_addr,
>  {
>  	int ret;
>  
> -	ret = mdiobus_read_nested(bus, addr, reg);
> +	ret = mdiobus_read_nested(bus, sw_addr + addr, reg);
>  	if (ret < 0)
>  		return ret;

If we are doing direct access, doesn't it means sw_addr is 0?

So isn't this pointless?

   Andrew

[toc] | [prev] | [next] | [standalone]

#1422403

FromVivien Didelot <vivien.didelot@savoirfairelinux.com>
Date2016-06-15 00:30 +0200
Message-ID<rK50m-2si-15@gated-at.bofh.it>
In reply to#1422372
Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

>> -	ret = mdiobus_read_nested(bus, addr, reg);
>> +	ret = mdiobus_read_nested(bus, sw_addr + addr, reg);
>>  	if (ret < 0)
>>  		return ret;
>
> If we are doing direct access, doesn't it means sw_addr is 0?
>
> So isn't this pointless?

6060 has no indirect access and directly responds to 16 SMI addresses,
regardless its chip address which can be strapped to either 0 or 16.

If we want to add support for it in mv88e6xxx someday (which is likely),
the code is ready for that.

Question 1) given this, should I still consider your first comment on
this patch about the mv88e6xxx_smi_ops assignment?

Question 2) is MV88E6XXX_FLAG_MULTI_CHIP confusing? I took a short name
for style but maybe a longer MV88E6XXX_FLAG_MULTI_CHIP_ADDRESSING or
MV88E6XXX_FLAG_MULTI_CHIP_MODE would be clearer to make to distinction
between "Single-chip Addressing Mode" and "Multi-chip Addressing Mode".

Thanks,

        Vivien

[toc] | [prev] | [next] | [standalone]

#1422426 — Re: [PATCH v2 net-next v2 12/12] net: dsa: mv88e6xxx: add addressing mode to info

FromAndrew Lunn <andrew@lunn.ch>
Date2016-06-15 00:50 +0200
SubjectRe: [PATCH v2 net-next v2 12/12] net: dsa: mv88e6xxx: add addressing mode to info
Message-ID<rK5jI-2zl-27@gated-at.bofh.it>
In reply to#1422403
On Tue, Jun 14, 2016 at 06:24:17PM -0400, Vivien Didelot wrote:
> Hi Andrew,
> 
> Andrew Lunn <andrew@lunn.ch> writes:
> 
> >> -	ret = mdiobus_read_nested(bus, addr, reg);
> >> +	ret = mdiobus_read_nested(bus, sw_addr + addr, reg);
> >>  	if (ret < 0)
> >>  		return ret;
> >
> > If we are doing direct access, doesn't it means sw_addr is 0?
> >
> > So isn't this pointless?
> 
> 6060 has no indirect access and directly responds to 16 SMI addresses,
> regardless its chip address which can be strapped to either 0 or 16.

Ah! O.K.

wnr854t-setup.c uses 0.
rd88f6183ap-ge-setup.c uses 0.
wrt350n-v2-setup.c uses 0.
rd88f5181l-fxo-setup.c uses 0.
rd88f5181l-ge-setup.c uses 0.
mach-bf518/boards/ezbrd.c uses 0.

The 6060 is a very old device. I doubt we will get any new boards
contributed using it. We are also going to have trouble actually
finding a device with one in order to test a merged mv88e6xxx and
mv88e6060 driver.

So i say we ignore the possibility of an 6060 on 16, until one really
comes along.

> Question 2) is MV88E6XXX_FLAG_MULTI_CHIP confusing?

No, i think it is fine.

    Andrew

[toc] | [prev] | [next] | [standalone]

#1422451

FromVivien Didelot <vivien.didelot@savoirfairelinux.com>
Date2016-06-15 01:20 +0200
Message-ID<rK5MK-2ZU-7@gated-at.bofh.it>
In reply to#1422426
Hi,

Andrew Lunn <andrew@lunn.ch> writes:

> On Tue, Jun 14, 2016 at 06:24:17PM -0400, Vivien Didelot wrote:
>> Hi Andrew,
>> 
>> Andrew Lunn <andrew@lunn.ch> writes:
>> 
>> >> -	ret = mdiobus_read_nested(bus, addr, reg);
>> >> +	ret = mdiobus_read_nested(bus, sw_addr + addr, reg);
>> >>  	if (ret < 0)
>> >>  		return ret;
>> >
>> > If we are doing direct access, doesn't it means sw_addr is 0?
>> >
>> > So isn't this pointless?
>> 
>> 6060 has no indirect access and directly responds to 16 SMI addresses,
>> regardless its chip address which can be strapped to either 0 or 16.
>
> Ah! O.K.
>
> wnr854t-setup.c uses 0.
> rd88f6183ap-ge-setup.c uses 0.
> wrt350n-v2-setup.c uses 0.
> rd88f5181l-fxo-setup.c uses 0.
> rd88f5181l-ge-setup.c uses 0.
> mach-bf518/boards/ezbrd.c uses 0.
>
> The 6060 is a very old device. I doubt we will get any new boards
> contributed using it. We are also going to have trouble actually
> finding a device with one in order to test a merged mv88e6xxx and
> mv88e6060 driver.
>
> So i say we ignore the possibility of an 6060 on 16, until one really
> comes along.

Sounds good to me!

Thanks,

        Vivien

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web