Re: [PATCH net-next 3/3] net: dsa: mv88e6xxx: make global2 code optional

2016-09-02 Thread Vivien Didelot 
Hi Andrew,

Andrew Lunn  writes:

>> I agree. Does the following snippet looks OK?
>> 
>> 
>> #ifndef CONFIG_NET_DSA_MV88E6XXX_GLOBAL2
>> if (mv88e6xxx_has(chip, MV88E6XXX_FLAG_GLOBAL2)) {
>> dev_err(chip->dev, "Missing support for Global 2 
>> registers\n");
>
> I would include the name of the option which needs enabling. Also it
> is not really missing. It has not been enabled.
>
> "The required compile time options needed to support this switch have
> not been enabled. Please enable: CONFIG_NET_DSA_MV88E6XXX_GLOBAL2"

That is much better indeed, respining.

Thanks!

Vivien

Re: [PATCH net-next 3/3] net: dsa: mv88e6xxx: make global2 code optional

2016-09-02 Thread Vivien Didelot 
Hi Andrew,

Andrew Lunn  writes:

>> I agree. Does the following snippet looks OK?
>> 
>> 
>> #ifndef CONFIG_NET_DSA_MV88E6XXX_GLOBAL2
>> if (mv88e6xxx_has(chip, MV88E6XXX_FLAG_GLOBAL2)) {
>> dev_err(chip->dev, "Missing support for Global 2 
>> registers\n");
>
> I would include the name of the option which needs enabling. Also it
> is not really missing. It has not been enabled.
>
> "The required compile time options needed to support this switch have
> not been enabled. Please enable: CONFIG_NET_DSA_MV88E6XXX_GLOBAL2"

That is much better indeed, respining.

Thanks!

Vivien

Re: [PATCH net-next 3/3] net: dsa: mv88e6xxx: make global2 code optional

2016-09-02 Thread Andrew Lunn 
> I agree. Does the following snippet looks OK?
> 
> 
> #ifndef CONFIG_NET_DSA_MV88E6XXX_GLOBAL2
> if (mv88e6xxx_has(chip, MV88E6XXX_FLAG_GLOBAL2)) {
> dev_err(chip->dev, "Missing support for Global 2 
> registers\n");

I would include the name of the option which needs enabling. Also it
is not really missing. It has not been enabled.

"The required compile time options needed to support this switch have
not been enabled. Please enable: CONFIG_NET_DSA_MV88E6XXX_GLOBAL2"

  Andrew

Re: [PATCH net-next 3/3] net: dsa: mv88e6xxx: make global2 code optional

2016-09-02 Thread Andrew Lunn 
> I agree. Does the following snippet looks OK?
> 
> 
> #ifndef CONFIG_NET_DSA_MV88E6XXX_GLOBAL2
> if (mv88e6xxx_has(chip, MV88E6XXX_FLAG_GLOBAL2)) {
> dev_err(chip->dev, "Missing support for Global 2 
> registers\n");

I would include the name of the option which needs enabling. Also it
is not really missing. It has not been enabled.

"The required compile time options needed to support this switch have
not been enabled. Please enable: CONFIG_NET_DSA_MV88E6XXX_GLOBAL2"

  Andrew

Re: [PATCH net-next 3/3] net: dsa: mv88e6xxx: make global2 code optional

2016-09-02 Thread Vivien Didelot 
Hi Andrew,

Andrew Lunn  writes:

>> What do you think?
>
> I think the probe() needs to fail with a very obvious error message
> saying you need to recompile your kernel with option XYZ enabled in
> order to support this switch, when the optional stuff is not
> optional...

I agree. Does the following snippet looks OK?


#ifndef CONFIG_NET_DSA_MV88E6XXX_GLOBAL2
if (mv88e6xxx_has(chip, MV88E6XXX_FLAG_GLOBAL2)) {
dev_err(chip->dev, "Missing support for Global 2 
registers\n");
return -EOPNOTSUPP;
}
#endif /* CONFIG_NET_DSA_MV88E6XXX_GLOBAL2 */


Thanks,

Vivien

Re: [PATCH net-next 3/3] net: dsa: mv88e6xxx: make global2 code optional

2016-09-02 Thread Vivien Didelot 
Hi Andrew,

Andrew Lunn  writes:

>> What do you think?
>
> I think the probe() needs to fail with a very obvious error message
> saying you need to recompile your kernel with option XYZ enabled in
> order to support this switch, when the optional stuff is not
> optional...

I agree. Does the following snippet looks OK?


#ifndef CONFIG_NET_DSA_MV88E6XXX_GLOBAL2
if (mv88e6xxx_has(chip, MV88E6XXX_FLAG_GLOBAL2)) {
dev_err(chip->dev, "Missing support for Global 2 
registers\n");
return -EOPNOTSUPP;
}
#endif /* CONFIG_NET_DSA_MV88E6XXX_GLOBAL2 */


Thanks,

Vivien

Re: [PATCH net-next 3/3] net: dsa: mv88e6xxx: make global2 code optional

2016-09-02 Thread Andrew Lunn 
Hi Vivien

> What do you think?

I think the probe() needs to fail with a very obvious error message
saying you need to recompile your kernel with option XYZ enabled in
order to support this switch, when the optional stuff is not
optional...

Andrew

Re: [PATCH net-next 3/3] net: dsa: mv88e6xxx: make global2 code optional

2016-09-02 Thread Andrew Lunn 
Hi Vivien

> What do you think?

I think the probe() needs to fail with a very obvious error message
saying you need to recompile your kernel with option XYZ enabled in
order to support this switch, when the optional stuff is not
optional...

Andrew

Re: [PATCH net-next 3/3] net: dsa: mv88e6xxx: make global2 code optional

2016-09-02 Thread Vivien Didelot 
Hi Andrew,

Andrew Lunn  writes:

> On Fri, Sep 02, 2016 at 08:08:19AM -0400, Vivien Didelot wrote:
>> Since not every chip has a Global2 set of registers, make its support
>> optional, in which case the related functions will return -EOPNOTSUPP.
>> 
>> This also allows to reduce the size of the mv88e6xxx driver for devices
>> such as home routers embedding Ethernet chips without Global2 support.
>
> I've no problems with splitting this out.

Yes, I think it is important to split support for independent internal
SMI devices (Globals, PHY, TCAM, ...).

> However, making it optional? I don't think that is a good idea.
>
> 1) If you don't have this, and you should, it looks like the PHYs stop
> working, and since you cannot set the switch MAC address, maybe Pause
> frames are broken.
>
> 2) Distros won't build a kernel per target hardware. They probably
> build a kernel per SoC vendor. That means, they will have this
> optional code built all the time. Very few builds will leave it out.
>
> So the only way i think making this optional, is if it is a kernel
> module, and it gets loaded if needed.

Your points are correct. But unless I'm mistaken, I purposely default
this suppor to 'y'. Distros will compile everything, which is good.

Only people who know their chips and what they are doing will eventually
go there and disable the support for some devices.

I plan to merge mv88e6060.c into the mv88e6xxx driver, these chips don't
have Global2. And we will soon add support for TCAM, which is huge. The
majority of devices supported by mv88e6xxx don't have a TCAM device.

So my point is that I'd like to disable support for unnecessary internal
devices, even if it doesn't make much difference on our huge dev boards,
but it does make a difference for the many embedded devices out there
(think home routers) with some 88E6060, 88E6176 or 88E6185 chips.

What do you think?

Thanks,

Vivien

Re: [PATCH net-next 3/3] net: dsa: mv88e6xxx: make global2 code optional

2016-09-02 Thread Vivien Didelot 
Hi Andrew,

Andrew Lunn  writes:

> On Fri, Sep 02, 2016 at 08:08:19AM -0400, Vivien Didelot wrote:
>> Since not every chip has a Global2 set of registers, make its support
>> optional, in which case the related functions will return -EOPNOTSUPP.
>> 
>> This also allows to reduce the size of the mv88e6xxx driver for devices
>> such as home routers embedding Ethernet chips without Global2 support.
>
> I've no problems with splitting this out.

Yes, I think it is important to split support for independent internal
SMI devices (Globals, PHY, TCAM, ...).

> However, making it optional? I don't think that is a good idea.
>
> 1) If you don't have this, and you should, it looks like the PHYs stop
> working, and since you cannot set the switch MAC address, maybe Pause
> frames are broken.
>
> 2) Distros won't build a kernel per target hardware. They probably
> build a kernel per SoC vendor. That means, they will have this
> optional code built all the time. Very few builds will leave it out.
>
> So the only way i think making this optional, is if it is a kernel
> module, and it gets loaded if needed.

Your points are correct. But unless I'm mistaken, I purposely default
this suppor to 'y'. Distros will compile everything, which is good.

Only people who know their chips and what they are doing will eventually
go there and disable the support for some devices.

I plan to merge mv88e6060.c into the mv88e6xxx driver, these chips don't
have Global2. And we will soon add support for TCAM, which is huge. The
majority of devices supported by mv88e6xxx don't have a TCAM device.

So my point is that I'd like to disable support for unnecessary internal
devices, even if it doesn't make much difference on our huge dev boards,
but it does make a difference for the many embedded devices out there
(think home routers) with some 88E6060, 88E6176 or 88E6185 chips.

What do you think?

Thanks,

Vivien

Re: [PATCH net-next 3/3] net: dsa: mv88e6xxx: make global2 code optional

2016-09-02 Thread Andrew Lunn 
On Fri, Sep 02, 2016 at 08:08:19AM -0400, Vivien Didelot wrote:
> Since not every chip has a Global2 set of registers, make its support
> optional, in which case the related functions will return -EOPNOTSUPP.
> 
> This also allows to reduce the size of the mv88e6xxx driver for devices
> such as home routers embedding Ethernet chips without Global2 support.

Hi Vivien

I've no problems with splitting this out.

However, making it optional? I don't think that is a good idea.

1) If you don't have this, and you should, it looks like the PHYs stop
working, and since you cannot set the switch MAC address, maybe Pause
frames are broken.

2) Distros won't build a kernel per target hardware. They probably
build a kernel per SoC vendor. That means, they will have this
optional code built all the time. Very few builds will leave it out.

So the only way i think making this optional, is if it is a kernel
module, and it gets loaded if needed.

Andrew

Re: [PATCH net-next 3/3] net: dsa: mv88e6xxx: make global2 code optional

2016-09-02 Thread Andrew Lunn 
On Fri, Sep 02, 2016 at 08:08:19AM -0400, Vivien Didelot wrote:
> Since not every chip has a Global2 set of registers, make its support
> optional, in which case the related functions will return -EOPNOTSUPP.
> 
> This also allows to reduce the size of the mv88e6xxx driver for devices
> such as home routers embedding Ethernet chips without Global2 support.

Hi Vivien

I've no problems with splitting this out.

However, making it optional? I don't think that is a good idea.

1) If you don't have this, and you should, it looks like the PHYs stop
working, and since you cannot set the switch MAC address, maybe Pause
frames are broken.

2) Distros won't build a kernel per target hardware. They probably
build a kernel per SoC vendor. That means, they will have this
optional code built all the time. Very few builds will leave it out.

So the only way i think making this optional, is if it is a kernel
module, and it gets loaded if needed.

Andrew

[PATCH net-next 3/3] net: dsa: mv88e6xxx: make global2 code optional

2016-09-02 Thread Vivien Didelot 
Since not every chip has a Global2 set of registers, make its support
optional, in which case the related functions will return -EOPNOTSUPP.

This also allows to reduce the size of the mv88e6xxx driver for devices
such as home routers embedding Ethernet chips without Global2 support.

It is present on most recent chips, thus enable its support by default.

Signed-off-by: Vivien Didelot 
---
 drivers/net/dsa/mv88e6xxx/Kconfig | 11 ++
 drivers/net/dsa/mv88e6xxx/Makefile|  2 +-
 drivers/net/dsa/mv88e6xxx/mv88e6xxx.h | 40 +++
 3 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mv88e6xxx/Kconfig 
b/drivers/net/dsa/mv88e6xxx/Kconfig
index ac77737..4ba9d5a 100644
--- a/drivers/net/dsa/mv88e6xxx/Kconfig
+++ b/drivers/net/dsa/mv88e6xxx/Kconfig
@@ -6,3 +6,14 @@ config NET_DSA_MV88E6XXX
help
  This driver adds support for most of the Marvell 88E6xxx models of
  Ethernet switch chips, except 88E6060.
+
+config NET_DSA_MV88E6XXX_GLOBAL2
+   bool "Switch Global 2 Registers support"
+   default y
+   depends on NET_DSA_MV88E6XXX
+   help
+ This registers set at internal SMI address 0x1C provides extended
+ features like indirect PHY access, EEPROM interface, trunking, etc.
+
+ It is required on most chips. If the chip you compile the support for
+ doesn't have such registers set, say N here. In doubt, say Y.
diff --git a/drivers/net/dsa/mv88e6xxx/Makefile 
b/drivers/net/dsa/mv88e6xxx/Makefile
index 5a42066..6971039 100644
--- a/drivers/net/dsa/mv88e6xxx/Makefile
+++ b/drivers/net/dsa/mv88e6xxx/Makefile
@@ -1,3 +1,3 @@
 obj-$(CONFIG_NET_DSA_MV88E6XXX) += mv88e6xxx.o
 mv88e6xxx-objs := chip.o
-mv88e6xxx-objs += global2.o
+mv88e6xxx-$(CONFIG_NET_DSA_MV88E6XXX_GLOBAL2) += global2.o
diff --git a/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h 
b/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
index 151f0af..18924c2 100644
--- a/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
@@ -726,6 +726,7 @@ int mv88e6xxx_update(struct mv88e6xxx_chip *chip, int addr, 
int reg,
 int mv88e6xxx_wait(struct mv88e6xxx_chip *chip, int addr, int reg, u16 mask);
 
 /* global2.c */
+#ifdef CONFIG_NET_DSA_MV88E6XXX_GLOBAL2
 int mv88e6xxx_g2_smi_phy_read(struct mv88e6xxx_chip *chip, int addr, int reg,
  u16 *val);
 int mv88e6xxx_g2_smi_phy_write(struct mv88e6xxx_chip *chip, int addr, int reg,
@@ -736,4 +737,43 @@ int mv88e6xxx_g2_get_eeprom16(struct mv88e6xxx_chip *chip,
 int mv88e6xxx_g2_set_eeprom16(struct mv88e6xxx_chip *chip,
  struct ethtool_eeprom *eeprom, u8 *data);
 int mv88e6xxx_g2_setup(struct mv88e6xxx_chip *chip);
+#else
+static inline int mv88e6xxx_g2_smi_phy_read(struct mv88e6xxx_chip *chip,
+   int addr, int reg, u16 *val)
+{
+   return -EOPNOTSUPP;
+}
+
+static inline int mv88e6xxx_g2_smi_phy_write(struct mv88e6xxx_chip *chip,
+int addr, int reg, u16 val)
+{
+   return -EOPNOTSUPP;
+}
+
+static inline int mv88e6xxx_g2_set_switch_mac(struct mv88e6xxx_chip *chip,
+ u8 *addr)
+{
+   return -EOPNOTSUPP;
+}
+
+static inline int mv88e6xxx_g2_get_eeprom16(struct mv88e6xxx_chip *chip,
+   struct ethtool_eeprom *eeprom,
+   u8 *data)
+{
+   return -EOPNOTSUPP;
+}
+
+static inline int mv88e6xxx_g2_set_eeprom16(struct mv88e6xxx_chip *chip,
+   struct ethtool_eeprom *eeprom,
+   u8 *data)
+{
+   return -EOPNOTSUPP;
+}
+
+static inline int mv88e6xxx_g2_setup(struct mv88e6xxx_chip *chip)
+{
+   return 0;
+}
+#endif /* CONFIG_NET_DSA_MV88E6XXX_GLOBAL2 */
+
 #endif
-- 
2.9.3

[PATCH net-next 3/3] net: dsa: mv88e6xxx: make global2 code optional

2016-09-02 Thread Vivien Didelot 
Since not every chip has a Global2 set of registers, make its support
optional, in which case the related functions will return -EOPNOTSUPP.

This also allows to reduce the size of the mv88e6xxx driver for devices
such as home routers embedding Ethernet chips without Global2 support.

It is present on most recent chips, thus enable its support by default.

Signed-off-by: Vivien Didelot 
---
 drivers/net/dsa/mv88e6xxx/Kconfig | 11 ++
 drivers/net/dsa/mv88e6xxx/Makefile|  2 +-
 drivers/net/dsa/mv88e6xxx/mv88e6xxx.h | 40 +++
 3 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mv88e6xxx/Kconfig 
b/drivers/net/dsa/mv88e6xxx/Kconfig
index ac77737..4ba9d5a 100644
--- a/drivers/net/dsa/mv88e6xxx/Kconfig
+++ b/drivers/net/dsa/mv88e6xxx/Kconfig
@@ -6,3 +6,14 @@ config NET_DSA_MV88E6XXX
help
  This driver adds support for most of the Marvell 88E6xxx models of
  Ethernet switch chips, except 88E6060.
+
+config NET_DSA_MV88E6XXX_GLOBAL2
+   bool "Switch Global 2 Registers support"
+   default y
+   depends on NET_DSA_MV88E6XXX
+   help
+ This registers set at internal SMI address 0x1C provides extended
+ features like indirect PHY access, EEPROM interface, trunking, etc.
+
+ It is required on most chips. If the chip you compile the support for
+ doesn't have such registers set, say N here. In doubt, say Y.
diff --git a/drivers/net/dsa/mv88e6xxx/Makefile 
b/drivers/net/dsa/mv88e6xxx/Makefile
index 5a42066..6971039 100644
--- a/drivers/net/dsa/mv88e6xxx/Makefile
+++ b/drivers/net/dsa/mv88e6xxx/Makefile
@@ -1,3 +1,3 @@
 obj-$(CONFIG_NET_DSA_MV88E6XXX) += mv88e6xxx.o
 mv88e6xxx-objs := chip.o
-mv88e6xxx-objs += global2.o
+mv88e6xxx-$(CONFIG_NET_DSA_MV88E6XXX_GLOBAL2) += global2.o
diff --git a/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h 
b/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
index 151f0af..18924c2 100644
--- a/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
@@ -726,6 +726,7 @@ int mv88e6xxx_update(struct mv88e6xxx_chip *chip, int addr, 
int reg,
 int mv88e6xxx_wait(struct mv88e6xxx_chip *chip, int addr, int reg, u16 mask);
 
 /* global2.c */
+#ifdef CONFIG_NET_DSA_MV88E6XXX_GLOBAL2
 int mv88e6xxx_g2_smi_phy_read(struct mv88e6xxx_chip *chip, int addr, int reg,
  u16 *val);
 int mv88e6xxx_g2_smi_phy_write(struct mv88e6xxx_chip *chip, int addr, int reg,
@@ -736,4 +737,43 @@ int mv88e6xxx_g2_get_eeprom16(struct mv88e6xxx_chip *chip,
 int mv88e6xxx_g2_set_eeprom16(struct mv88e6xxx_chip *chip,
  struct ethtool_eeprom *eeprom, u8 *data);
 int mv88e6xxx_g2_setup(struct mv88e6xxx_chip *chip);
+#else
+static inline int mv88e6xxx_g2_smi_phy_read(struct mv88e6xxx_chip *chip,
+   int addr, int reg, u16 *val)
+{
+   return -EOPNOTSUPP;
+}
+
+static inline int mv88e6xxx_g2_smi_phy_write(struct mv88e6xxx_chip *chip,
+int addr, int reg, u16 val)
+{
+   return -EOPNOTSUPP;
+}
+
+static inline int mv88e6xxx_g2_set_switch_mac(struct mv88e6xxx_chip *chip,
+ u8 *addr)
+{
+   return -EOPNOTSUPP;
+}
+
+static inline int mv88e6xxx_g2_get_eeprom16(struct mv88e6xxx_chip *chip,
+   struct ethtool_eeprom *eeprom,
+   u8 *data)
+{
+   return -EOPNOTSUPP;
+}
+
+static inline int mv88e6xxx_g2_set_eeprom16(struct mv88e6xxx_chip *chip,
+   struct ethtool_eeprom *eeprom,
+   u8 *data)
+{
+   return -EOPNOTSUPP;
+}
+
+static inline int mv88e6xxx_g2_setup(struct mv88e6xxx_chip *chip)
+{
+   return 0;
+}
+#endif /* CONFIG_NET_DSA_MV88E6XXX_GLOBAL2 */
+
 #endif
-- 
2.9.3