Re: OMAP4430 SDP with KS8851: very slow networking

[Tony Lindgren]

* Russell King - ARM Linux  [181207 19:27]:
> You mentioned that edge mode didn't work as well as level mode on
> duovero smsc controller, I think this may help to solve the same
> issue but for edge IRQs - we need a mask_ack_irq function to avoid
> acking while the edge interrupt is masked.  Let me know if that
> lowers the smsc ping latency while in edge mode.

Looks like smsc edge interrupt is still producing varying
ping latencies with this. Seems like the mas_ack_irq is
a nice improvment though.

Regards,

Tony

Re: OMAP4430 SDP with KS8851: very slow networking

[Tony Lindgren]

* Tony Lindgren  [181207 18:14]:
> Hi,
> 
> * Russell King - ARM Linux  [181207 18:01]:
> > Hi Tony,
> > 
> > You know most of what's been going on from IRC, but here's the patch
> > which gets me:
> > 
> > 1) working interrupts for networking
> > 2) solves the stuck-wakeup problem
> > 
> > It also contains some of the debug bits I added.
> 
> This is excellent news :) Will test today.

Yes your patch seems to work great based on brief testing :)

> > I think what this means is that we should strip out ec0daae685b2
> > ("gpio: omap: Add level wakeup handling for omap4 based SoCs").
> 
> Yes the only reason for the wakeup quirk was the stuck wakeup
> state seen on omap4, it can be just dropped if this works.
> Adding Grygorii to Cc too.

I'll post a partial revert for commit ec0daae685b2 ("gpio: omap:
Add level wakeup handling for omap4 based SoCs") shortly.

Thanks,

Tony

Re: OMAP4430 SDP with KS8851: very slow networking

[Tony Lindgren]

Hi,

* Russell King - ARM Linux  [181207 18:01]:
> Hi Tony,
> 
> You know most of what's been going on from IRC, but here's the patch
> which gets me:
> 
> 1) working interrupts for networking
> 2) solves the stuck-wakeup problem
> 
> It also contains some of the debug bits I added.

This is excellent news :) Will test today.

> I think what this means is that we should strip out ec0daae685b2
> ("gpio: omap: Add level wakeup handling for omap4 based SoCs").

Yes the only reason for the wakeup quirk was the stuck wakeup
state seen on omap4, it can be just dropped if this works.
Adding Grygorii to Cc too.

Regards,

Tony

> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 3d021f648c5d..528ffd1b9832 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -11,7 +11,7 @@
>   * it under the terms of the GNU General Public License version 2 as
>   * published by the Free Software Foundation.
>   */
> -
> +#define DEBUG
>  #include 
>  #include 
>  #include 
> @@ -366,10 +366,14 @@ static inline void omap_set_gpio_trigger(struct 
> gpio_bank *bank, int gpio,
> trigger & IRQ_TYPE_LEVEL_LOW);
>   omap_gpio_rmw(base, bank->regs->leveldetect1, gpio_bit,
> trigger & IRQ_TYPE_LEVEL_HIGH);
> + /*
> +  * We need the edge detect enabled for the idle mode detection
> +  * to function on OMAP4430.
> +  */
>   omap_gpio_rmw(base, bank->regs->risingdetect, gpio_bit,
> -   trigger & IRQ_TYPE_EDGE_RISING);
> +   trigger & (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_LEVEL_HIGH));
>   omap_gpio_rmw(base, bank->regs->fallingdetect, gpio_bit,
> -   trigger & IRQ_TYPE_EDGE_FALLING);
> +   trigger & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_LEVEL_LOW));
>  
>   bank->context.leveldetect0 =
>   readl_relaxed(bank->base + bank->regs->leveldetect0);
> @@ -910,14 +914,16 @@ static void omap_gpio_unmask_irq(struct irq_data *d)
>   if (trigger)
>   omap_set_gpio_triggering(bank, offset, trigger);
>  
> + omap_set_gpio_irqenable(bank, offset, 1);
> +
>   /* For level-triggered GPIOs, the clearing must be done after
> -  * the HW source is cleared, thus after the handler has run */
> - if (bank->level_mask & BIT(offset)) {
> - omap_set_gpio_irqenable(bank, offset, 0);
> +  * the HW source is cleared, thus after the handler has run.
> +  * OMAP4 needs this done _after_ enabing the interrupt to clear
> +  * the wakeup status.
> +  */
> + if (bank->level_mask & BIT(offset))
>   omap_clear_gpio_irqstatus(bank, offset);
> - }
>  
> - omap_set_gpio_irqenable(bank, offset, 1);
>   raw_spin_unlock_irqrestore(>lock, flags);
>  }
>  
> @@ -1520,6 +1526,10 @@ static void omap_gpio_idle(struct gpio_bank *bank, 
> bool may_lose_context)
>   struct device *dev = bank->chip.parent;
>   u32 l1 = 0, l2 = 0;
>  
> + dev_dbg(dev, "%s(): ld 0x%08x 0x%08x we 0x%08x\n", __func__,
> + bank->context.leveldetect0, bank->context.leveldetect1,
> + bank->context.wake_en);
> +
>   if (bank->funcs.idle_enable_level_quirk)
>   bank->funcs.idle_enable_level_quirk(bank);
>  
> @@ -1553,6 +1563,10 @@ static void omap_gpio_idle(struct gpio_bank *bank, 
> bool may_lose_context)
>   bank->get_context_loss_count(dev);
>  
>   omap_gpio_dbck_disable(bank);
> +
> + dev_dbg(dev, "%s(): ld 0x%08x 0x%08x we 0x%08x\n", __func__,
> + bank->context.leveldetect0, bank->context.leveldetect1,
> + bank->context.wake_en);
>  }
>  
>  static void omap_gpio_init_context(struct gpio_bank *p);
> @@ -1563,6 +1577,10 @@ static void omap_gpio_unidle(struct gpio_bank *bank)
>   u32 l = 0, gen, gen0, gen1;
>   int c;
>  
> + dev_dbg(dev, "%s(): ld 0x%08x 0x%08x we 0x%08x\n", __func__,
> + bank->context.leveldetect0, bank->context.leveldetect1,
> + bank->context.wake_en);
> +
>   /*
>* On the first resume during the probe, the context has not
>* been initialised and so initialise it now. Also initialise
> @@ -1648,6 +1666,10 @@ static void omap_gpio_unidle(struct gpio_bank *bank)
>   }
>  
>   bank->workaround_enabled = false;
> +
> + dev_dbg(dev, "%s(): ld 0x%08x 0x%08x we 0x%08x\n", __func__,
> + bank->context.leveldetect0, bank->context.leveldetect1,
> + bank->context.wake_en);
>  }
>  
>  static void omap_gpio_init_context(struct gpio_bank *p)
> @@ -1720,6 +1742,7 @@ static int __maybe_unused 
> omap_gpio_runtime_suspend(struct device *dev)
>   error = -EBUSY;
>   goto unlock;
>   }
> + dev_dbg(dev, "%s()\n", __func__);
>   omap_gpio_idle(bank, true);
>   bank->is_suspended = true;
>  unlock:
> @@ -1741,6 +1764,7 @@ static int __maybe_unused 
> omap_gpio_runtime_resume(struct device *dev)
>   

Re: OMAP4430 SDP with KS8851: very slow networking

[Tony Lindgren]

* Russell King - ARM Linux  [181206 18:08]:
> reverted, the problem is still there.  Revert:
> 
> ec0daae685b2 ("gpio: omap: Add level wakeup handling for omap4 based SoCs")
> 
> on top, and networking returns to normal.  So it appears to be this
> last commit causing the issue.
> 
> With that and b764a5863fd8 applied, it still misbehaves.  Then, poking
> at the OMAP4_GPIO_IRQWAKEN0 register, changing it from 0 to 4 with
> devmem2 restores normal behaviour - ping times are normal and NFS is
> happy.
> 
> # devmem2 0x48055044 w 4

OK thanks.

> Given that this GPIO device is not runtime suspended, and is
> permanently active (which is what I think we expect, given that it
> has an IRQ claimed against it) does the hardware still attempt to
> idle the GPIO block - if so, could that be why we need to program
> the wakeup register, so the GPIO block signals that it's active?

Yes we now idle non-irq GPIOs only from CPU_CLUSTER_PM_ENTER
as the selected cpuidle state triggers the domain transitions
with WFI. And that's why runtime_suspended_time does not increase
for a GPIO instance with IRQs.

I can reproduce the long ping latencies on duovero smsc connected
to gpio_44, I'll try to debug it more.

Regards,

Tony

Re: OMAP4430 SDP with KS8851: very slow networking

[Tony Lindgren]

Hi,

* Russell King - ARM Linux  [181206 13:23]:
> It looks very much like a receive problem - in that the board is not
> always aware of a packet having been received until it attempts to
> transmit (eg, in the case of TFTP, when it re-sends the ACK after a
> receive timeout, it _then_ notices that there's a packet waiting.)
> 
> I'm not quite sure when this cropped up as I no longer regularly
> update and run my nightly boot tests, but I think 4.18 was fine.

Sounds like it's some gpio or PM related issue. If it's not caused
by commit b764a5863fd8 ("gpio: omap: Remove custom PM calls and
use cpu_pm instead"), then maybe the changes to probe devices
with ti-sysc interconnect target module driver caused it. Below
is a revert for mcspi that would help in that case.

Also I guess it could be caused by drivers/spi/spi-omap2-mcspi.c
changes since v4.18.

Care to post output of /sys/kernel/debug/pm_debug/count for
a working and non-working kernels?

Regards,

Tony

8< ---
diff --git a/arch/arm/boot/dts/omap4-l4.dtsi b/arch/arm/boot/dts/omap4-l4.dtsi
--- a/arch/arm/boot/dts/omap4-l4.dtsi
+++ b/arch/arm/boot/dts/omap4-l4.dtsi
@@ -2050,25 +2050,7 @@
#address-cells = <1>;
#size-cells = <1>;
ranges = <0x0 0x98000 0x1000>;
-
-   mcspi1: spi@0 {
-   compatible = "ti,omap4-mcspi";
-   reg = <0x0 0x200>;
-   interrupts = ;
-   #address-cells = <1>;
-   #size-cells = <0>;
-   ti,spi-num-cs = <4>;
-   dmas = < 35>,
-  < 36>,
-  < 37>,
-  < 38>,
-  < 39>,
-  < 40>,
-  < 41>,
-  < 42>;
-   dma-names = "tx0", "rx0", "tx1", "rx1",
-   "tx2", "rx2", "tx3", "rx3";
-   };
+   status = "disabled";
};
 
target-module@9a000 {   /* 0x4809a000, ap 51 
2c.0 */
@@ -2089,20 +2071,7 @@
#address-cells = <1>;
#size-cells = <1>;
ranges = <0x0 0x9a000 0x1000>;
-
-   mcspi2: spi@0 {
-   compatible = "ti,omap4-mcspi";
-   reg = <0x0 0x200>;
-   interrupts = ;
-   #address-cells = <1>;
-   #size-cells = <0>;
-   ti,spi-num-cs = <2>;
-   dmas = < 43>,
-  < 44>,
-  < 45>,
-  < 46>;
-   dma-names = "tx0", "rx0", "tx1", "rx1";
-   };
+   status = "disabled";
};
 
target-module@9c000 {   /* 0x4809c000, ap 53 
36.0 */
@@ -2290,17 +2259,7 @@
#address-cells = <1>;
#size-cells = <1>;
ranges = <0x0 0xb8000 0x1000>;
-
-   mcspi3: spi@0 {
-   compatible = "ti,omap4-mcspi";
-   reg = <0x0 0x200>;
-   interrupts = ;
-   #address-cells = <1>;
-   #size-cells = <0>;
-   ti,spi-num-cs = <2>;
-   dmas = < 15>, < 16>;
-   dma-names = "tx0", "rx0";
-   };
+   status = "disabled";
};
 
target-module@ba000 {   /* 0x480ba000, ap 71 
32.0 */
@@ -2321,17 +2280,7 @@
#address-cells = <1>;
#size-cells = <1>;
ranges = <0x0 0xba000 0x1000>;
-
-   mcspi4: spi@0 {
-   compatible = "ti,omap4-mcspi";
-   reg = <0x0 0x200>;
-   interrupts = ;
-   #address-cells = <1>;
-   #size-cells = <0>;
-   ti,spi-num-cs = <1>;
-   dmas = < 70>, < 71>;
-   dma-names = "tx0", "rx0";
-   };
+   status = "disabled";
};
 
target-module@d1000 {   /* 0x480d1000, ap 73 
44.0 */
diff --git 

Re: [PATCH 1/2] dt-bindings: net: cpsw: Document cpsw-phy-sel usage but prefer phandle

[Tony Lindgren]

* Grygorii Strashko  [180830 17:08]:
> On 08/29/2018 07:47 PM, Tony Lindgren wrote:
> > In general, it seems cpsw is just an interconnect instance
> > (L4_FAST) with a control module (CPSW_WR) and a pile of
> > independent other modules. That's described nicely in
> > am437x TRM chapter "2.1.4 L4 Fast Peripheral Memory Map".
> > So from that point of view the binding reg entries right
> > now are all wrong :)
> 
> TRM not consistent - for am5 it's one MMIO region.

Well that same information is there in 57xx TRM in chapter
"Table 26-1454. GMAC_SW Instance Summary". But yeah, all
the cpsw internal devices are stuffed into a single
interconnect target module.

> > In the long run cpsw should be really treated as an
> > interconnect instance with it's control module providing
> > standard Linux framework services such as clock /
> > regulator / phy / pinctrl / iio whatever for the other
> > modules.
> > 
> > Just my 2c based on looking at the interconnect, I'm
> > not too familiar with cpsw otherwise.
> 
> It's not separate modules. this is composite module which have only one
> fck/ick and most of blocks can't even function without each other.
> Above might be the case for Keystone 2, but not omap CPSW.
> Keystone 2 - has packet processor, security accelerator, queue manager in
> addition to its basic switch block.

Yeah there's just one fck/ick as it's all in a single
interconnect module. But you might want to look at the
CPSW_WR device registers and see what gate clocks and other
Linux generic subsystem services CPSW_WR could provide for
the other cpsw internal devices. It might just make your
life easier maintaining all these variants ;)

Regards,

Tony

Re: [PATCH 1/2] dt-bindings: net: cpsw: Document cpsw-phy-sel usage but prefer phandle

[Tony Lindgren]

* Grygorii Strashko  [180830 00:12]:
> Hi Tony,
> 
> On 08/29/2018 10:00 AM, Tony Lindgren wrote:
> > The current cpsw usage for cpsw-phy-sel is undocumented but is used for
> > all the boards using cpsw. And cpsw-phy-sel is not really a child of
> > the cpsw device, it lives in the system control module instead.
> > 
> > Let's document the existing usage, and improve it a bit where we prefer
> > to use a phandle instead of a child device for it. That way we can
> > properly describe the hardware in dts files for things like genpd.
> 
> I'm ok with this series, but I really don't like cpsw-phy-sel in general.

Yeah this binding predates any standards. This series
only fixes the nasty issue of cpsw claiming a module as a
child that's outside it's IO range.

> It was introduced long time back and now I'm thinking about possibility to 
> replace it with
> one of current generic interfaces - for example mux-controller. 
> Each port will control up to 3 muxes (port mode, idmode and rmii_ext_clk) and 
>  
> transform phy-mode => mux states.
> What do you think?

Sure a mux-controller here makes sense.

> Another option is to use phy, but it'd be complicated.

For the port muxes, how about a phy driver just using
a pinctrl driver?

In general, it seems cpsw is just an interconnect instance
(L4_FAST) with a control module (CPSW_WR) and a pile of
independent other modules. That's described nicely in
am437x TRM chapter "2.1.4 L4 Fast Peripheral Memory Map".
So from that point of view the binding reg entries right
now are all wrong :)

In the long run cpsw should be really treated as an
interconnect instance with it's control module providing
standard Linux framework services such as clock /
regulator / phy / pinctrl / iio whatever for the other
modules.

Just my 2c based on looking at the interconnect, I'm
not too familiar with cpsw otherwise.

Regards,

Tony

[PATCH 2/2] net: ethernet: cpsw-phy-sel: prefer phandle for phy sel

[Tony Lindgren]

The cpsw-phy-sel device is not a child of the cpsw interconnect target
module. It lives in the system control module.

Let's fix this issue by trying to use cpsw-phy-sel phandle first if it
exists and if not fall back to current usage of trying to find the
cpsw-phy-sel child. That way the phy sel driver can be a child of the
system control module where it belongs in the device tree.

Without this fix, we cannot have a proper interconnect target module
hierarchy in device tree for things like genpd.

Note that deferred probe is mostly not supported by cpsw and this patch
does not attempt to fix that. In case deferred probe support is needed,
this could be added to cpsw_slave_open() and phy_connect() so they start
handling and returning errors.

For documenting it, looks like the cpsw-phy-sel is used for all cpsw device
tree nodes. It's missing the related binding documentation, so let's also
update the binding documentation accordingly.

Cc: devicet...@vger.kernel.org
Cc: Andrew Lunn 
Cc: Grygorii Strashko 
Cc: Ivan Khoronzhuk 
Cc: Mark Rutland 
Cc: Murali Karicheri 
Cc: Rob Herring 
Signed-off-by: Tony Lindgren 
---
 drivers/net/ethernet/ti/cpsw-phy-sel.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw-phy-sel.c 
b/drivers/net/ethernet/ti/cpsw-phy-sel.c
--- a/drivers/net/ethernet/ti/cpsw-phy-sel.c
+++ b/drivers/net/ethernet/ti/cpsw-phy-sel.c
@@ -170,10 +170,13 @@ void cpsw_phy_sel(struct device *dev, phy_interface_t 
phy_mode, int slave)
struct device_node *node;
struct cpsw_phy_sel_priv *priv;
 
-   node = of_get_child_by_name(dev->of_node, "cpsw-phy-sel");
+   node = of_parse_phandle(dev->of_node, "cpsw-phy-sel", 0);
if (!node) {
-   dev_err(dev, "Phy mode driver DT not found\n");
-   return;
+   node = of_get_child_by_name(dev->of_node, "cpsw-phy-sel");
+   if (!node) {
+   dev_err(dev, "Phy mode driver DT not found\n");
+   return;
+   }
}
 
dev = bus_find_device(_bus_type, NULL, node, match);
-- 
2.18.0

[PATCH 1/2] dt-bindings: net: cpsw: Document cpsw-phy-sel usage but prefer phandle

[Tony Lindgren]

The current cpsw usage for cpsw-phy-sel is undocumented but is used for
all the boards using cpsw. And cpsw-phy-sel is not really a child of
the cpsw device, it lives in the system control module instead.

Let's document the existing usage, and improve it a bit where we prefer
to use a phandle instead of a child device for it. That way we can
properly describe the hardware in dts files for things like genpd.

Cc: devicet...@vger.kernel.org
Cc: Andrew Lunn 
Cc: Grygorii Strashko 
Cc: Ivan Khoronzhuk 
Cc: Mark Rutland 
Cc: Murali Karicheri 
Cc: Rob Herring 
Signed-off-by: Tony Lindgren 
---
 Documentation/devicetree/bindings/net/cpsw.txt | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/cpsw.txt 
b/Documentation/devicetree/bindings/net/cpsw.txt
--- a/Documentation/devicetree/bindings/net/cpsw.txt
+++ b/Documentation/devicetree/bindings/net/cpsw.txt
@@ -19,6 +19,10 @@ Required properties:
 - slaves   : Specifies number for slaves
 - active_slave : Specifies the slave to use for time stamping,
  ethtool and SIOCGMIIPHY
+- cpsw-phy-sel : Specifies the phandle to the CPSW phy mode selection
+ device. See also cpsw-phy-sel.txt for it's binding.
+ Note that in legacy cases cpsw-phy-sel may be
+ a child device instead of a phandle.
 
 Optional properties:
 - ti,hwmods: Must be "cpgmac0"
@@ -75,6 +79,7 @@ Examples:
cpts_clock_mult = <0x8000>;
cpts_clock_shift = <29>;
syscon = <>;
+   cpsw-phy-sel = <_sel>;
cpsw_emac0: slave@0 {
phy_id = <_mdio>, <0>;
phy-mode = "rgmii-txid";
@@ -103,6 +108,7 @@ Examples:
cpts_clock_mult = <0x8000>;
cpts_clock_shift = <29>;
syscon = <>;
+   cpsw-phy-sel = <_sel>;
cpsw_emac0: slave@0 {
phy_id = <_mdio>, <0>;
phy-mode = "rgmii-txid";
-- 
2.18.0

Re: [PATCH] net: ethernet: cpsw-phy-sel: prefer phandle for phy sel and update binding

[Tony Lindgren]

* Tony Lindgren  [180808 13:52]:
> * Andrew Lunn  [180808 12:02]:
> > 
> > Do you need to handle EPROBE_DEFER here? The phandle points to a
> > device which has not yet been loaded? I'm not sure exactly where it
> > will be returned, maybe it is bus_find_device(), but i expect to see
> > some handling of it somewhere in this function.

If no device is found the driver just produces a warning currently.
And in that case cpsw attempts to continue with bootloader settings.

And looking at the caller function cpsw_slave_open() it also just
produces warnings for phy_connect() too..

I agree that in general this this whole pile of cpsw related drivers sure
could use some better error handling. Starting with making cpsw_slave_open()
and cpsw_phy_sel() return errors instead of just ignoring them might be a
good start.

Grygorii, care to add that note of things to do into your cpsw maintainer
hat?

> With the proper interconnect hierarchy in the device tree there should be
> no EPROBE_DEFER happening here as the interconnects are probed in the
> right order with the always on interrupt with system control module first :)
>
> But then again, adding support for EPROBE_DEFER here won't hurt either,
> will take a look.

I'll just add some notes about that to the patch description considering
the above.

Regards,

Tony

Re: [PATCH] net: ethernet: cpsw-phy-sel: prefer phandle for phy sel and update binding

[Tony Lindgren]

* Andrew Lunn  [180808 12:02]:
> > +++ b/Documentation/devicetree/bindings/net/cpsw.txt
> > @@ -19,6 +19,9 @@ Required properties:
> >  - slaves   : Specifies number for slaves
> >  - active_slave : Specifies the slave to use for time stamping,
> >   ethtool and SIOCGMIIPHY
> > +- cpsw-phy-sel : Specifies the phandle to the CPSW phy mode 
> > selection
> > + device. Note that in legacy cases cpsw-phy-sel may be
> > + a child device instead of a phandle.
> 
> Hi Tony
> 
> It would be good to reference cpsw-phy-sel.txt.

OK will add.

> > --- a/drivers/net/ethernet/ti/cpsw-phy-sel.c
> > +++ b/drivers/net/ethernet/ti/cpsw-phy-sel.c
> > @@ -170,10 +170,13 @@ void cpsw_phy_sel(struct device *dev, phy_interface_t 
> > phy_mode, int slave)
> > struct device_node *node;
> > struct cpsw_phy_sel_priv *priv;
> >  
> > -   node = of_get_child_by_name(dev->of_node, "cpsw-phy-sel");
> > +   node = of_parse_phandle(dev->of_node, "cpsw-phy-sel", 0);
> > if (!node) {
> 
> Do you need to handle EPROBE_DEFER here? The phandle points to a
> device which has not yet been loaded? I'm not sure exactly where it
> will be returned, maybe it is bus_find_device(), but i expect to see
> some handling of it somewhere in this function.

With the proper interconnect hierarchy in the device tree there should be
no EPROBE_DEFER happening here as the interconnects are probed in the
right order with the always on interrupt with system control module first :)

But then again, adding support for EPROBE_DEFER here won't hurt either,
will take a look.

Regards,

Tony

[PATCH] net: ethernet: cpsw-phy-sel: prefer phandle for phy sel and update binding

[Tony Lindgren]

The cpsw-phy-sel device is not a child of the cpsw interconnect target
module. It lives in the system control module.

Let's fix this issue by trying to use cpsw-phy-sel phandle first if it
exists and if not fall back to current usage of trying to find the
cpsw-phy-sel child. That way the phy sel driver can be a child of the
system control module where it belongs in the device tree.

Without this fix, we cannot have a proper interconnect target module
hierarchy in device tree for things like genpd.

For documenting it, looks like the cpsw-phy-sel is used for all cpsw device
tree nodes. It's missing the related binding documentation, so let's also
update the binding documentation accordingly.

Cc: devicet...@vger.kernel.org
Cc: Grygorii Strashko 
Cc: Ivan Khoronzhuk 
Cc: Mark Rutland 
Cc: Murali Karicheri 
Cc: Rob Herring 
Signed-off-by: Tony Lindgren 
---
 Documentation/devicetree/bindings/net/cpsw.txt | 5 +
 drivers/net/ethernet/ti/cpsw-phy-sel.c | 9 ++---
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/cpsw.txt 
b/Documentation/devicetree/bindings/net/cpsw.txt
--- a/Documentation/devicetree/bindings/net/cpsw.txt
+++ b/Documentation/devicetree/bindings/net/cpsw.txt
@@ -19,6 +19,9 @@ Required properties:
 - slaves   : Specifies number for slaves
 - active_slave : Specifies the slave to use for time stamping,
  ethtool and SIOCGMIIPHY
+- cpsw-phy-sel : Specifies the phandle to the CPSW phy mode selection
+ device. Note that in legacy cases cpsw-phy-sel may be
+ a child device instead of a phandle.
 
 Optional properties:
 - ti,hwmods: Must be "cpgmac0"
@@ -75,6 +78,7 @@ Examples:
cpts_clock_mult = <0x8000>;
cpts_clock_shift = <29>;
syscon = <>;
+   cpsw-phy-sel = <_sel>;
cpsw_emac0: slave@0 {
phy_id = <_mdio>, <0>;
phy-mode = "rgmii-txid";
@@ -103,6 +107,7 @@ Examples:
cpts_clock_mult = <0x8000>;
cpts_clock_shift = <29>;
syscon = <>;
+   cpsw-phy-sel = <_sel>;
cpsw_emac0: slave@0 {
phy_id = <_mdio>, <0>;
phy-mode = "rgmii-txid";
diff --git a/drivers/net/ethernet/ti/cpsw-phy-sel.c 
b/drivers/net/ethernet/ti/cpsw-phy-sel.c
--- a/drivers/net/ethernet/ti/cpsw-phy-sel.c
+++ b/drivers/net/ethernet/ti/cpsw-phy-sel.c
@@ -170,10 +170,13 @@ void cpsw_phy_sel(struct device *dev, phy_interface_t 
phy_mode, int slave)
struct device_node *node;
struct cpsw_phy_sel_priv *priv;
 
-   node = of_get_child_by_name(dev->of_node, "cpsw-phy-sel");
+   node = of_parse_phandle(dev->of_node, "cpsw-phy-sel", 0);
if (!node) {
-   dev_err(dev, "Phy mode driver DT not found\n");
-   return;
+   node = of_get_child_by_name(dev->of_node, "cpsw-phy-sel");
+   if (!node) {
+   dev_err(dev, "Phy mode driver DT not found\n");
+   return;
+   }
}
 
dev = bus_find_device(_bus_type, NULL, node, match);
-- 
2.17.1

Re: Fwd: DA850-evm MAC Address is random

[Tony Lindgren]

* Adam Ford  [170828 13:33]:
> On Mon, Aug 28, 2017 at 1:54 PM, Grygorii Strashko
>  wrote:
> > Cc: Sekhar
> >
> > On 08/28/2017 10:32 AM, Adam Ford wrote:
> >>
> >> The davinvi_emac MAC address seems to attempt a call to
> >> ti_cm_get_macid in cpsw-common.c but it returns the message
> >> 'davinci_emac davinci_emac.1: incompatible machine/device type for
> >> reading mac address ' and then generates a random MAC address.
> >>
> >> The function appears to lookup varions boards using
> >> 'of_machine_is_compaible' and supports dm8148, am33xx, am3517, dm816,
> >> am4372 and dra7.  I don't see the ti,davinci-dm6467-emac which is
> >> what's shown in the da850 device tree.
> >>
> >> Is there a patch somewhere for supporting the da850-evm?
> >
> >
> > Not sure if MAC address can be read from Control module.
> > May be Sekhar can say more?
> 
> My understanding is that the MAC address is programmed by Logic PD
> into the SPI flash.  The Bootloader reads this from either SPI or its
> env variables.  Looking at the partition info listed in the
> da850-evm.dts file, it appears as if they've reserved space for it.
> Unfortunately, I don't see any code that reads it out.  I was hoping
> there might be a way to just pass cmdline parameter from the
> bootloader to the kernel to accept the MAC address.
> 
> >
> >>
> >> If not, is there a way to pass the MAC address from U-Boot to the
> >> driver so it doesn't generate a random MAC?
> >
> >
> > "local-mac-address" dt porp
> 
> The downside here, is that we'd have to have the Bootloader modify the
> device tree.

That piece of code exists somewhere in u-boot already. Note how
we are populating the mac address for USB Ethernet drivers in
u-boot and then the Ethernet driver code parses it. See commit
055d31de7158 ("ARM: omap3: beagleboard-xm: dt: Add ethernet to
the device tree") for some more information.

I think u-boot needs the ethernet alias for finding the interface.

Regards,

Tony

Re: [RFC/RFT PATCH 4/4] [debug] ARM: am335x: illustrate hwstamp

[Tony Lindgren]

* Grygorii Strashko <grygorii.stras...@ti.com> [170613 16:20]:
> This patch allows to test CPTS HW_TS_PUSH functionality on am335x boards
> 
> below sequence of commands will enable Timer7 to trigger 1sec
> periodic pulses on CPTS HW4_TS_PUSH input pin:
> 
>  # echo 10 > /sys/class/pwm/pwmchip0/pwm0/period
>  # echo 5 > /sys/class/pwm/pwmchip0/pwm0/duty_cycle
>  # echo 1 > /sys/class/pwm/pwmchip0/pwm0/enable
>  # ./ptp/testptp -e 10 -i 3
> external time stamp request okay
> event index 3 at 1493259028.376600798
> event index 3 at 1493259029.377170898
> event index 3 at 1493259030.377741039
> event index 3 at 1493259031.378311139
> event index 3 at 1493259032.378881279
> event index 3 at 1493259033.379451424
> event index 3 at 1493259034.380021520
> event index 3 at 1493259035.380591660
> event index 3 at 1493259036.381161765
> event index 3 at 1493259037.381731909

Cool :)

Acked-by: Tony Lindgren <t...@atomide.com>

Re: phy: cpcap-usb: Add CPCAP PMIC USB support

[Tony Lindgren]

* Colin Ian King  [170605 10:58]:
> Hi Tony,
> 
> While running static analysis on linux-next, CoverityScan picked up a
> NULL pointer deference on ddata->pins when calling pinctrl_lookup_state:
> 
> 466ddata->pins = devm_pinctrl_get(ddata->dev);
> 
>1. Condition IS_ERR(ddata->pins), taking true branch.
> 
> 467if (IS_ERR(ddata->pins)) {
> 468dev_info(ddata->dev, "default pins not configured:
> %ld\n",
> 469 PTR_ERR(ddata->pins));
> 
>2. assign_zero: Assigning: ddata->pins = NULL.
> 
> 470ddata->pins = NULL;
> 471}
> 472
> 
>CID 1440453 (#1 of 1): Explicit null dereferenced (FORWARD_NULL)3.
> var_deref_model: Passing null pointer ddata->pins to
> pinctrl_lookup_state, which dereferences it. [show details]
> 
> 473ddata->pins_ulpi = pinctrl_lookup_state(ddata->pins, "ulpi");
> 
> 
> I suspect the IS_ERROR() check should return with some error return
> rather than continuing.

OK thanks for letting me know. I'll take a look and will
send a patch. The pinctrl_lookup_state() probably need to
happen only if ddata->pins as they are optional.

Regards,

Tony

Re: [PATCH] [4.11 regression] cpsw/netcp: refine cpts dependency

[Tony Lindgren]

* Arnd Bergmann <a...@arndb.de> [170428 08:06]:
> Tony Lindgren reports a kernel oops that resulted from my compile-time
> fix on the default config. This shows two problems:
> 
> a) configurations that did not already enable PTP_1588_CLOCK will
>now miss the cpts driver
> 
> b) when cpts support is disabled, the driver crashes. This is a
>preexisting problem that we did not notice before my patch.
> 
> While the second problem is still being investigated, this modifies
> the dependencies again, getting us back to the original state, with
> another 'select NET_PTP_CLASSIFY' added in to avoid the original
> link error we got, and the 'depends on POSIX_TIMERS' to hide
> the CPTS support when turning it on would be useless.
> 
> Cc: sta...@vger.kernel.org # 4.11 needs this
> Fixes: 07fef3623407 ("cpsw/netcp: cpts depends on posix_timers")
> Signed-off-by: Arnd Bergmann <a...@arndb.de>
> ---
> Adding the Cc: stable in case this doesn't make it into 4.11
> any more.

Thanks this fixes the oops I'm seeing:

Tested-by: Tony Lindgren <t...@atomide.com>

>  drivers/net/ethernet/ti/Kconfig | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/ti/Kconfig b/drivers/net/ethernet/ti/Kconfig
> index 9e631952b86f..48a541eb0af2 100644
> --- a/drivers/net/ethernet/ti/Kconfig
> +++ b/drivers/net/ethernet/ti/Kconfig
> @@ -76,7 +76,7 @@ config TI_CPSW
>  config TI_CPTS
>   bool "TI Common Platform Time Sync (CPTS) Support"
>   depends on TI_CPSW || TI_KEYSTONE_NETCP
> - depends on PTP_1588_CLOCK
> + depends on POSIX_TIMERS
>   ---help---
> This driver supports the Common Platform Time Sync unit of
> the CPSW Ethernet Switch and Keystone 2 1g/10g Switch Subsystem.
> @@ -87,6 +87,8 @@ config TI_CPTS_MOD
>   tristate
>   depends on TI_CPTS
>   default y if TI_CPSW=y || TI_KEYSTONE_NETCP=y
> + select NET_PTP_CLASSIFY
> + imply PTP_1588_CLOCK
>   default m
>  
>  config TI_KEYSTONE_NETCP
> -- 
> 2.9.0
> 

Re: cpsw regression in mainline with "cpsw/netcp: cpts depends on posix_timers"

[Tony Lindgren]

* Arnd Bergmann <a...@arndb.de> [170424 11:14]:
> On Mon, Apr 24, 2017 at 7:44 PM, Tony Lindgren <t...@atomide.com> wrote:
> > * Arnd Bergmann <a...@arndb.de> [170424 10:38]:
> >> On Mon, Apr 24, 2017 at 6:51 PM, Tony Lindgren <t...@atomide.com> wrote:
> >> > Hi,
> >> >
> >> > Looks like commit 07fef3623407 ("cpsw/netcp: cpts depends on 
> >> > posix_timers")
> >> > in mainline started triggering the following oops at least on j5eco-evm.
> >> >
> >> > Adding CONFIG_PTP_1588_CLOCK to .config solves it, but the oops hints
> >> > something is wrong with the dependencies.. CONFIG_TI_CPTS defaults to N
> >> > and not selecting it causes the oops.
> >> >
> >> > Any ideas what's needed to properly fix this?
> >>
> >> Does your configuration have POSIX_TIMERS enabled? If not, then CPTS
> >> is now also disabled. There are two issues here that we need to solve:
> >>
> >> a) find out why POSIX_TIMERS got disabled, and make sure it's always
> >> turned on for normal users
> >
> > $ make omap2plus_defconfig
> > ...
> > CONFIG_POSIX_TIMERS=y
> > # CONFIG_PTP_1588_CLOCK is not set
> >
> > So CONFIG_TI_CPTS is unselectable.
> 
> Ok, so at least this one is easy to understand, it's a direct consequence
> of my change, as nothing else will select CONFIG_PTP_1588_CLOCK
> now.
> 
> My first try would be this one:
> 
> diff --git a/drivers/net/ethernet/ti/Kconfig b/drivers/net/ethernet/ti/Kconfig
> index 9e631952b86f..8042a7626056 100644
> --- a/drivers/net/ethernet/ti/Kconfig
> +++ b/drivers/net/ethernet/ti/Kconfig
> @@ -76,7 +76,7 @@ config TI_CPSW
>  config TI_CPTS
>   bool "TI Common Platform Time Sync (CPTS) Support"
>   depends on TI_CPSW || TI_KEYSTONE_NETCP
> - depends on PTP_1588_CLOCK
> + depends on POSIX_TIMERS
>   ---help---
>This driver supports the Common Platform Time Sync unit of
>the CPSW Ethernet Switch and Keystone 2 1g/10g Switch Subsystem.
> @@ -87,6 +87,7 @@ config TI_CPTS_MOD
>   tristate
>   depends on TI_CPTS
>   default y if TI_CPSW=y || TI_KEYSTONE_NETCP=y
> + imply PTP_1588_CLOCK
>   default m
> 
>  config TI_KEYSTONE_NETCP

Yup this produces a booting .config for me:

Tested-by: Tony Lindgren <t...@atomide.com>

Regards,

Tony

Re: cpsw regression in mainline with "cpsw/netcp: cpts depends on posix_timers"

[Tony Lindgren]

* Arnd Bergmann <a...@arndb.de> [170424 10:38]:
> On Mon, Apr 24, 2017 at 6:51 PM, Tony Lindgren <t...@atomide.com> wrote:
> > Hi,
> >
> > Looks like commit 07fef3623407 ("cpsw/netcp: cpts depends on posix_timers")
> > in mainline started triggering the following oops at least on j5eco-evm.
> >
> > Adding CONFIG_PTP_1588_CLOCK to .config solves it, but the oops hints
> > something is wrong with the dependencies.. CONFIG_TI_CPTS defaults to N
> > and not selecting it causes the oops.
> >
> > Any ideas what's needed to properly fix this?
> 
> Does your configuration have POSIX_TIMERS enabled? If not, then CPTS
> is now also disabled. There are two issues here that we need to solve:
> 
> a) find out why POSIX_TIMERS got disabled, and make sure it's always
> turned on for normal users

$ make omap2plus_defconfig
...
CONFIG_POSIX_TIMERS=y
# CONFIG_PTP_1588_CLOCK is not set

So CONFIG_TI_CPTS is unselectable.

> b) find out what's wrong with the driver when CPTS is disabled. This would
> be an existing problem that you just never saw because CPTS was
> always enabled so far.

See a) above, yes seems there are two problems here.

Regards,

Tony

cpsw regression in mainline with "cpsw/netcp: cpts depends on posix_timers"

[Tony Lindgren]

Hi,

Looks like commit 07fef3623407 ("cpsw/netcp: cpts depends on posix_timers")
in mainline started triggering the following oops at least on j5eco-evm.

Adding CONFIG_PTP_1588_CLOCK to .config solves it, but the oops hints
something is wrong with the dependencies.. CONFIG_TI_CPTS defaults to N
and not selecting it causes the oops.

Any ideas what's needed to properly fix this?

Regards,

Tony

8< -
Unhandled fault: external abort on non-linefetch (0x1008) at 0xf09a3018
pgd = c0004000
[f09a3018] *pgd=ae82c811, *pte=4a103653, *ppte=4a103453
Internal error: : 1008 [#1] SMP ARM
Modules linked in:
CPU: 0 PID: 14 Comm: kworker/0:1 Not tainted 4.11.0-rc8 #418
Hardware name: Generic ti814x (Flattened Device Tree)
Workqueue: rpciod rpc_async_schedule
task: ee8f41c0 task.stack: ee93c000
PC is at cpdma_chan_submit+0x154/0x304
LR is at __dma_page_cpu_to_dev+0x28/0xac
pc : []lr : []psr: a093
sp : ee93d958  ip : ef6f9000  fp : 
r10: eeebee40  r9 : 8013  r8 : eed956f0
r7 :   r6 : 003c  r5 : f09a3000  r4 : eed956d0
r3 : f09a3018  r2 : aeebb302  r1 : aeebb33e  r0 : ee950410
Flags: NzCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment none
Control: 10c5387d  Table: 80004019  DAC: 0051
Process kworker/0:1 (pid: 14, stack limit = 0xee93c218)
Stack: (0xee93d958 to 0xee93e000)
d940:   0001 
d960: ee8f47f0  eeebee40 eeebee40 eedfc000 eedf4810  eed956d0
d980:  eee01800 ee93c000 c063e35c  c01975d8 eeebee40 eedfc000
d9a0:  eedfc000  c06b4c6c  eee01840 eeebee40 002a
d9c0: eedfc000 ee93d9f4 eeeb2e50 eeeb2e00 eeebee40 eee01800 eedfc000 
d9e0: eeeb2e50  0008 c06db9f4 eeeb2e00 0010  eeeb2e00
da00:    c06b574c eeeb2e50 0001 c072af98 6013
da20:  c072af98 0878     eeeb2e80
da40: fff4  e000 eeebee40 c0db2600 c0dbe34a  c0dbe34a
da60: c06c06bc 0003 c0dbe34a c072af98   c072ae2c 
da80: 3e6fa8c0 426fa8c0 c06c06bc 0003 c0dbe34a c072b048 3e6fa8c0 eedfc21c
daa0:  eedfc000  3e6fa8c0 426fa8c0 c072bdb8 3e6fa8c0 
dac0: eeded088   c01b424c c0d59933 c029ed04 ef6db580 c080d190
dae0: eeebdc68 6013 c06c06a0 0003 01080020 eeebdc14 eeebef00 eeebdc00
db00: eeebef00 0001 eeeac000 eeebdc38 eeeac000   c06c06bc
db20: eeebdc00 eeebdc14 0001 c06c4144 8d80 eeebdc00 eeeac000 
db40: c0dbe2e3 c06f3550 eeeac000   c06c4f78 c0db6e78 c0d07da0
db60:   ee8f41c0 0006 eeebde10 eeebdc00 eedfc000 426fa8c0
db80: c0dbe2e3 c0db2600 eeeac000   c06f3550  
dba0: c06f32c0   0010 c06f6b6c 6013 c06f6b44 426fa8c0
dbc0: e000  eeeac000 c0dbe2e3 c0db2600 0001 c06f75b0 0074
dbe0:  c06f6b6c   c06f6a70  eedfc000 eea36040
dc00: ee93dc54 c06f51b0 ee93ddb8 3e6fa8c0 eea36340 c06f777c ee8f4e30 eeeac000
dc20: c0db2600 c0db2600 eea36040 eeebde24 0071 ee93dd00 c0725248 3e6fa8c0
dc40: eeeac000 c06f75b0   ee93dd04 ee93dc54 ee93dc54 eeeac000
dc60: eea36040 eeebde24 0071 ee93dd00  3e6fa8c0 eeeac000 c0725248
dc80: eea36040 ee93ddb8 ee93dd00 426fa8c0 0058 0060 3e6fa8c0 c0726f3c
dca0: 0060 0008 ee93dce4 ee93dce0 4040 6013 c07222fc 
dcc0:  0058  c06f51b0 6f00 c0691c74 c0db2600 ee93de5c
dce0:  ee8f47f0 c0ee 426fa8c0    0004
dd00: 0002 0001  0011  0007  
dd20:  ee93de5c 3e6fa8c0 426fa8c0 d5866f00 ee8f41c0 0001 eeeb100c
dd40:  c019a0d8 0001 e000 c0733718 c013e764 eea36040 ee93ddb8
dd60: 0058 c0733718 eea36040 c0734310   1944 
dd80: ee32a900 0010 eeeb100c  eeebb404 ee93de5c c0dc5a04 c0691c74
dda0:  c07d8460 0058  eeeb6804 0058 eeeb100c 0010
ddc0: 0003   ee93ddb8    
dde0: 4040   0001  eeebb404 ee32a900 0058
de00:  c07d84d4   c1566b80  c1566bc4 c01c7b3c
de20: 0001 eeeb1000 eeebb400 eeeac0c0 eeebb474 eeeb1344  c0dc5874
de40: c0dc5a04 c07d8a10  0001 ee93de5c eeeac0c0 eeebb474 
de60: eeeb1000 eeebb400 eeeac0c0 c07d5240 eeeb685c eeebb404 eeeb6954 eeeac0c0
de80: eeebb400 eeebb400 0681 fef5f746 c0d0796c c0dc5874 ee93c000 c07d1fe4
dea0: eee4c800 c01975d8 eeeac0c0 ee8ff080 ef6dfa40 c07d1e8c c07d1e8c c07dde98
dec0:  0681   e000  6013 eeeac0e4
dee0: ee8ff080 ef6dfa40 ee93df20 ff7f0c00 c0d0796c c0dc2d54 c0d4fa8e c0154dac
df00: 0001  c0154cf4 0001   

Re: [PATCH 0/2] ARM: am335x-icev2: Add ethernet support

[Tony Lindgren]

* Roger Quadros  [170330 05:37]:
> Hi Tony & Dave,
> 
> On 13/03/17 15:42, Roger Quadros wrote:
> > Hi,
> > 
> > This series adds ethernet support to am335x-icev2 board.
> > 
> > The ethernet PHYs on the board need an explicit GPIO reset pulse
> > to ensure they bootstrap to the correct mode. Without the
> > GPIO reset they just don't work.
> > 
> > cheers,
> > -roger
> 
> Any comments on this series. Patch 1 is at version 2.

I think you meant patch 2/3 is at version2. I've picked
patches 2 and 3 for v4.12 into dt and defconfig branches.

You may need to resend the davinci_mdio.c patch alone
for Dave as he usually won't pick individual patches I
think.

Regards,

Tony

> > Roger Quadros (2):
> >   net: davinci_mdio: add GPIO reset logic
> >   ARM: dts: am335x-icev2: Add CPSW ethernet0 and ethernet1
> > 
> >  .../devicetree/bindings/net/davinci-mdio.txt   |   2 +
> >  arch/arm/boot/dts/am335x-icev2.dts | 113 
> > +
> >  drivers/net/ethernet/ti/davinci_mdio.c |  68 +++--
> >  3 files changed, 175 insertions(+), 8 deletions(-)
> > 
> 
> cheers,
> -roger

Re: [PATCH] net: qmi_wwan: Add USB IDs for MDM6600 modem on Motorola Droid 4

[Tony Lindgren]

* Bjørn Mork <bj...@mork.no> [170319 10:23]:
> Tony Lindgren <t...@atomide.com> writes:
> > And the v3.8 kernel also has drivers/usb/serial/mdm6600.c:
> >
> > +static const struct usb_device_id mdm6600_id_table[] = {
> > +   { USB_DEVICE_AND_INTERFACE_INFO(0x22b8, 0x2a70, 0xff, 0xff, 0xff) },
> > +/* MDM9600 */
> > +   { USB_DEVICE_AND_INTERFACE_INFO(0x22b8, 0x2e0a, 0xff, 0xff, 0xff) },
> > +   { USB_DEVICE_AND_INTERFACE_INFO(0x22b8, 0x900e, 0xff, 0xff, 0xff) },
> > +   { },
> > +};
> 
> 
> Yes, looks like they consitently use ff/ff/ff for serial functions and
> ff/fb/ff for QMI functions.  So adding a vendor rule seems appropriate.

OK

> > And then in drivers/usb/serial/moto_flashqsc.c:
> >
> > +static struct usb_device_id id_table[] = {
> > +   {USB_DEVICE_AND_INTERFACE_INFO(0x22b8, 0x2a63, 0x0a, 0, 0)},
> > +   {USB_DEVICE_AND_INTERFACE_INFO(0x22b8, 0x4281, 0x0a, 0, 0xfc)},
> > +   {USB_DEVICE_AND_INTERFACE_INFO(0x22b8, 0x2db4, 0x0a, 0, 0xfc)},
> > +   {USB_DEVICE(0x22b8, 0x4260)},
> > +   {USB_DEVICE(0x22b8, 0x426D)},
> > +   {},
> > +};
> 
> This on the other hand, is something I hope I don't have to review :)
> The 0x0a class (CDC Data) is always part of a multi-interface function,
> and you would normally match on the control interface.

No idea about that one.. Presumably we can ignore it for now :)

> > Where the 0x4260 and 0x426d seem to be for the flash mode of the
> > Wrigley3GLTE modem.
> >
> > See also lsusb -v output below. No idea if there's a Windows driver
> > .inf file for this. Most likely whatever Windows driver is just using the
> > generic Android USB driver(s). I know the USB on droid 4 can be multiplexed
> > to have MDM6600 directly accessed, but I think that's only used for
> > debugging the modem as that mode needs to be selected in the bootloader
> > temporarily using volume keys.
> >
> > With the configuration in my patch, modprobe of qmi_wwan produces four
> > wwan interfaces if that matters.
> 
> And I assume they all work?

Yeah the network scan shows networks on all four interfaces.

> >> The reason I ask is that I'd hate to have reports of other Motorola
> >> devices where ff/fb/ff was used for some other USB function.  Yes, that
> >> would be stupid.  But still... Experience shows that we cannot rule out
> >> stupid when we consider USB descriptors.
> >
> > Yes thanks for checking. If you prefer to set it up in some other
> > way, or need more info, please let me know.
> 
> No, it all looks very sane to me, given your explanation. Just wanted to
> be absolutely sure. Thanks a lot.
> 
> 
> Acked-by: Bjørn Mork <bj...@mork.no>

OK thanks,

Tony

Re: [PATCH] net: qmi_wwan: Add USB IDs for MDM6600 modem on Motorola Droid 4

[Tony Lindgren]

* Tony Lindgren <t...@atomide.com> [170319 10:05]:
> * Bjørn Mork <bj...@mork.no> [170319 09:33]:
> > This is a bit unusual, so I'd like to verify that it is correct.  Do you
> > happen to have a "lsusb -v" or /sys/kernel/debug/usb/devices dump for
> > this device?  Is this usage of vendor + class consistent with the
> > Windows driver *.inf data?  Are you sure that the ff/fb/ff class is only
> > used for QMI functions by this vendor ID? 
> 
> Well this is based on what the earlier Motorola mapphone v3.8 kernel has
> for in drivers/net/usb/qcusbnet/qcusbnet.c driver:
> 
> +   { /* Motorola Xoom */
> +   USB_DEVICE_AND_INTERFACE_INFO(0x22B8, 0x2A70, 0xff, 0xfb, 0xff),
> +   .driver_info = (unsigned long)_qc_netinfo
> +   },
> +   { /* MDM9600 */
> +   USB_DEVICE_AND_INTERFACE_INFO(0x22B8, 0x2E0A, 0xff, 0xfb, 0xff),
> +   .driver_info = (unsigned long)_qc_netinfo
> +   },
> 
> Where the "Motorola Xoom" id is also used for all mapphone devices
> with MDM6600 variants.
> 
> Then xoom_qc_netinfo in the mapphone kernel has:
> 
> +static const struct driver_info xoom_qc_netinfo = {
> +   .description   = "Xoom QCUSBNet Ethernet Device",
> +   .flags = FLAG_ETHER|FLAG_SEND_ZLP,
> +   .bind  = xoom_qcnet_bind,
> +   .unbind= qcnet_unbind,
> +   .data  = 0,
> +   .manage_power  = qcnet_manage_power,
> +};
> 
> And the v3.8 kernel also has drivers/usb/serial/mdm6600.c:
> 
> +static const struct usb_device_id mdm6600_id_table[] = {
> +   { USB_DEVICE_AND_INTERFACE_INFO(0x22b8, 0x2a70, 0xff, 0xff, 0xff) },
> +/* MDM9600 */
> +   { USB_DEVICE_AND_INTERFACE_INFO(0x22b8, 0x2e0a, 0xff, 0xff, 0xff) },
> +   { USB_DEVICE_AND_INTERFACE_INFO(0x22b8, 0x900e, 0xff, 0xff, 0xff) },
> +   { },
> +};
> 
> And then in drivers/usb/serial/moto_flashqsc.c:
> 
> +static struct usb_device_id id_table[] = {
> +   {USB_DEVICE_AND_INTERFACE_INFO(0x22b8, 0x2a63, 0x0a, 0, 0)},
> +   {USB_DEVICE_AND_INTERFACE_INFO(0x22b8, 0x4281, 0x0a, 0, 0xfc)},
> +   {USB_DEVICE_AND_INTERFACE_INFO(0x22b8, 0x2db4, 0x0a, 0, 0xfc)},
> +   {USB_DEVICE(0x22b8, 0x4260)},
> +   {USB_DEVICE(0x22b8, 0x426D)},
> +   {},
> +};
> 
> Where the 0x4260 and 0x426d seem to be for the flash mode of the
> Wrigley3GLTE modem.

Oh found one more entry grepping the old patch, this time in
drivers/usb/serial/qsc6085_modem.c:

+static struct usb_device_id id_table[] = {
+   {USB_DEVICE(0x22b8, 0x2a6e)},   /* Sholes CDMA BP modem */
+   {USB_DEVICE(0x22b8, 0x2a6f)},   /* Sholes CDMA BP modem */
+   {USB_DEVICE_AND_INTERFACE_INFO(0x22b8, 0x2a70, 0xff, 0xff, 0xff)},
+   {USB_DEVICE_AND_INTERFACE_INFO(0x22b8, 0x2e0a, 0xff, 0xff, 0xff)}, /* 
MDM9600 */
+   {USB_DEVICE_AND_INTERFACE_INFO(0x22b8, 0x900e, 0xff, 0xff, 0xff)},
+   {},
+};

Regards,

Tony

Re: [PATCH] net: qmi_wwan: Add USB IDs for MDM6600 modem on Motorola Droid 4

[Tony Lindgren]

* Bjørn Mork <bj...@mork.no> [170319 09:33]:
> Tony Lindgren <t...@atomide.com> writes:
> 
> > This gets qmicli working with the MDM6600 modem.
> >
> > Cc: Bjørn Mork <bj...@mork.no>
> > Reviewed-by: Sebastian Reichel <s...@kernel.org>
> > Tested-by: Sebastian Reichel <s...@kernel.org>
> > Signed-off-by: Tony Lindgren <t...@atomide.com>
> > ---
> >  drivers/net/usb/qmi_wwan.c | 4 
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
> > --- a/drivers/net/usb/qmi_wwan.c
> > +++ b/drivers/net/usb/qmi_wwan.c
> > @@ -580,6 +580,10 @@ static const struct usb_device_id products[] = {
> > USB_VENDOR_AND_INTERFACE_INFO(HUAWEI_VENDOR_ID, 
> > USB_CLASS_VENDOR_SPEC, 0x01, 0x69),
> > .driver_info= (unsigned long)_wwan_info,
> > },
> > +   {   /* Motorola Mapphone devices with MDM6600 */
> > +   USB_VENDOR_AND_INTERFACE_INFO(0x22b8, USB_CLASS_VENDOR_SPEC, 
> > 0xfb, 0xff),
> 
> 
> This is a bit unusual, so I'd like to verify that it is correct.  Do you
> happen to have a "lsusb -v" or /sys/kernel/debug/usb/devices dump for
> this device?  Is this usage of vendor + class consistent with the
> Windows driver *.inf data?  Are you sure that the ff/fb/ff class is only
> used for QMI functions by this vendor ID? 

Well this is based on what the earlier Motorola mapphone v3.8 kernel has
for in drivers/net/usb/qcusbnet/qcusbnet.c driver:

+   { /* Motorola Xoom */
+   USB_DEVICE_AND_INTERFACE_INFO(0x22B8, 0x2A70, 0xff, 0xfb, 0xff),
+   .driver_info = (unsigned long)_qc_netinfo
+   },
+   { /* MDM9600 */
+   USB_DEVICE_AND_INTERFACE_INFO(0x22B8, 0x2E0A, 0xff, 0xfb, 0xff),
+   .driver_info = (unsigned long)_qc_netinfo
+   },

Where the "Motorola Xoom" id is also used for all mapphone devices
with MDM6600 variants.

Then xoom_qc_netinfo in the mapphone kernel has:

+static const struct driver_info xoom_qc_netinfo = {
+   .description   = "Xoom QCUSBNet Ethernet Device",
+   .flags = FLAG_ETHER|FLAG_SEND_ZLP,
+   .bind  = xoom_qcnet_bind,
+   .unbind= qcnet_unbind,
+   .data  = 0,
+   .manage_power  = qcnet_manage_power,
+};

And the v3.8 kernel also has drivers/usb/serial/mdm6600.c:

+static const struct usb_device_id mdm6600_id_table[] = {
+   { USB_DEVICE_AND_INTERFACE_INFO(0x22b8, 0x2a70, 0xff, 0xff, 0xff) },
+/* MDM9600 */
+   { USB_DEVICE_AND_INTERFACE_INFO(0x22b8, 0x2e0a, 0xff, 0xff, 0xff) },
+   { USB_DEVICE_AND_INTERFACE_INFO(0x22b8, 0x900e, 0xff, 0xff, 0xff) },
+   { },
+};

And then in drivers/usb/serial/moto_flashqsc.c:

+static struct usb_device_id id_table[] = {
+   {USB_DEVICE_AND_INTERFACE_INFO(0x22b8, 0x2a63, 0x0a, 0, 0)},
+   {USB_DEVICE_AND_INTERFACE_INFO(0x22b8, 0x4281, 0x0a, 0, 0xfc)},
+   {USB_DEVICE_AND_INTERFACE_INFO(0x22b8, 0x2db4, 0x0a, 0, 0xfc)},
+   {USB_DEVICE(0x22b8, 0x4260)},
+   {USB_DEVICE(0x22b8, 0x426D)},
+   {},
+};

Where the 0x4260 and 0x426d seem to be for the flash mode of the
Wrigley3GLTE modem.

See also lsusb -v output below. No idea if there's a Windows driver
.inf file for this. Most likely whatever Windows driver is just using the
generic Android USB driver(s). I know the USB on droid 4 can be multiplexed
to have MDM6600 directly accessed, but I think that's only used for
debugging the modem as that mode needs to be selected in the bootloader
temporarily using volume keys.

With the configuration in my patch, modprobe of qmi_wwan produces four
wwan interfaces if that matters.

> The reason I ask is that I'd hate to have reports of other Motorola
> devices where ff/fb/ff was used for some other USB function.  Yes, that
> would be stupid.  But still... Experience shows that we cannot rule out
> stupid when we consider USB descriptors.

Yes thanks for checking. If you prefer to set it up in some other
way, or need more info, please let me know.

Regards,

Tony

8< 
Bus 002 Device 002: ID 22b8:2a70  
Device Descriptor:
  bLength18
  bDescriptorType 1
  bcdUSB   2.00
  bDeviceClass0 
  bDeviceSubClass 0 
  bDeviceProtocol 0 
  bMaxPacketSize064
  idVendor   0x22b8 
  idProduct  0x2a70 
  bcdDevice0.00
  iManufacturer   1 Motorola, Incorporated
  iProduct2 Flash MZ600
  iSerial 0 
  bNumConfigurations  1
  Configuration Descriptor:
bLength 9
bDescriptorType 2
wTotalLength  251
bNumInterfaces  9
bConfigurationValue 1
iConfiguration  3 Motorola Configuration
bmAttributes

[PATCH] net: qmi_wwan: Add USB IDs for MDM6600 modem on Motorola Droid 4

[Tony Lindgren]

This gets qmicli working with the MDM6600 modem.

Cc: Bjørn Mork <bj...@mork.no>
Reviewed-by: Sebastian Reichel <s...@kernel.org>
Tested-by: Sebastian Reichel <s...@kernel.org>
Signed-off-by: Tony Lindgren <t...@atomide.com>
---
 drivers/net/usb/qmi_wwan.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
--- a/drivers/net/usb/qmi_wwan.c
+++ b/drivers/net/usb/qmi_wwan.c
@@ -580,6 +580,10 @@ static const struct usb_device_id products[] = {
USB_VENDOR_AND_INTERFACE_INFO(HUAWEI_VENDOR_ID, 
USB_CLASS_VENDOR_SPEC, 0x01, 0x69),
.driver_info= (unsigned long)_wwan_info,
},
+   {   /* Motorola Mapphone devices with MDM6600 */
+   USB_VENDOR_AND_INTERFACE_INFO(0x22b8, USB_CLASS_VENDOR_SPEC, 
0xfb, 0xff),
+   .driver_info= (unsigned long)_wwan_info,
+   },
 
/* 2. Combined interface devices matching on class+protocol */
{   /* Huawei E367 and possibly others in "Windows mode" */
-- 
2.11.1

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Tony Lindgren]

* Kalle Valo <kv...@codeaurora.org> [170130 22:36]:
> Tony Lindgren <t...@atomide.com> writes:
> 
> > * Pavel Machek <pa...@ucw.cz> [170127 11:41]:
> >> On Fri 2017-01-27 17:23:07, Kalle Valo wrote:
> >> > Pali Rohár <pali.ro...@gmail.com> writes:
> >> > 
> >> > > On Friday 27 January 2017 14:26:22 Kalle Valo wrote:
> >> > >> Pali Rohár <pali.ro...@gmail.com> writes:
> >> > >> 
> >> > >> > 2) It was already tested that example NVS data can be used for N900 
> >> > >> > e.g.
> >> > >> > for SSH connection. If real correct data are not available it is 
> >> > >> > better
> >> > >> > to use at least those example (and probably log warning message) so 
> >> > >> > user
> >> > >> > can connect via SSH and start investigating where is problem.
> >> > >> 
> >> > >> I disagree. Allowing default calibration data to be used can be
> >> > >> unnoticed by user and left her wondering why wifi works so badly.
> >> > >
> >> > > So there are only two options:
> >> > >
> >> > > 1) Disallow it and so these users will have non-working wifi.
> >> > >
> >> > > 2) Allow those data to be used as fallback mechanism.
> >> > >
> >> > > And personally I'm against 1) because it will break wifi support for
> >> > > *all* Nokia N900 devices right now.
> >> > 
> >> > All two of them? :)
> >> 
> >> Umm. You clearly want a flock of angry penguins at your doorsteps :-).
> >
> > Well this silly issue of symlinking and renaming nvs files in a standard
> > Linux distro was also hitting me on various devices with wl12xx/wl18xx
> > trying to use the same rootfs.
> >
> > Why don't we just set a custom compatible property for n900 that then
> > picks up some other nvs file instead of the default?
> 
> Please don't. An ugly kernel workaround in kernel because of user space
> problems is a bad idea. wl1251 should just ask for NVS file from user
> space, it shouldn't care if it's a "default" file or something else.
> That's a user space policy decision.

Grr I keep forgetting it needs to be for each device manufactured so
yeah that won't work.

The names of standard distro files are hardcoded into the kernel
driver so it's also a kernel problem though :p

How about a custom devices tree property saying "needs-custom-firmware"?

Something that would prevent anything being loaded until user space
loads the firmware. It could also be set in the driver automatically
based on the compatible flag if we always want it enabled. And we could
have some cmdline option to ignore it. Or the other way around whatever
makes sense.

> Why can't you do something like this:
> 
> * rename the NVS file linux-firmware to wl1251-nvs.bin.example

As that name is hardcoded in the kernel and that file is provided by
all standard distros, let's assume we just have to deal with that ABI
forever.

> * before distro updates linux-firmware create yours own deb/rpm/whatever
>   package "wl1251-firmware" which installs your flavor of nvs file (or
>   the user fallback helper if more dynamic functionality is preferred)

And that won't work when using the same file system on other machines.

Think NFSroot for example. At least I'm using the same NFSroot across
about 15 different machines including one n900 macro board with smc91x
Ethernet.

Regards,

Tony

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Tony Lindgren]

* Pavel Machek  [170127 11:41]:
> On Fri 2017-01-27 17:23:07, Kalle Valo wrote:
> > Pali Rohár  writes:
> > 
> > > On Friday 27 January 2017 14:26:22 Kalle Valo wrote:
> > >> Pali Rohár  writes:
> > >> 
> > >> > 2) It was already tested that example NVS data can be used for N900 
> > >> > e.g.
> > >> > for SSH connection. If real correct data are not available it is better
> > >> > to use at least those example (and probably log warning message) so 
> > >> > user
> > >> > can connect via SSH and start investigating where is problem.
> > >> 
> > >> I disagree. Allowing default calibration data to be used can be
> > >> unnoticed by user and left her wondering why wifi works so badly.
> > >
> > > So there are only two options:
> > >
> > > 1) Disallow it and so these users will have non-working wifi.
> > >
> > > 2) Allow those data to be used as fallback mechanism.
> > >
> > > And personally I'm against 1) because it will break wifi support for
> > > *all* Nokia N900 devices right now.
> > 
> > All two of them? :)
> 
> Umm. You clearly want a flock of angry penguins at your doorsteps :-).

Well this silly issue of symlinking and renaming nvs files in a standard
Linux distro was also hitting me on various devices with wl12xx/wl18xx
trying to use the same rootfs.

Why don't we just set a custom compatible property for n900 that then
picks up some other nvs file instead of the default?

Regards,

Tony

Re: [PATCH] ARM: dts: am57xx-beagle-x15: implement errata "Ethernet RGMII2 Limited to 10/100 Mbps"

[Tony Lindgren]

* Grygorii Strashko  [170112 09:15]:
> According to errata i880 description the speed of Ethernet port 1 on AM572x
> SoCs rev 1.1 shuld be limited to 10/100Mbps, because RGMII2 Switching
> Characteristics are not compatible with 1000 Mbps operation [1].
> The issue is fixed with Rev 2.0 silicon.
> 
> Hence, rework Beagle-X15 and Begale-X15-revb1 to use phy-handle instead of
> phy_id and apply corresponding limitation to the Ethernet Phy 1.
> 
> [1] http://www.ti.com/lit/er/sprz429j/sprz429j.pdf

Applying into omap-for-v4.11/dt thanks.

Tony

Re: [PATCH 2/2] ARM: dts: dra72-evm-revc: enable irqs for dp83867 eth phys

[Tony Lindgren]

* Grygorii Strashko <grygorii.stras...@ti.com> [170109 11:26]:
> 
> 
> On 01/06/2017 03:54 PM, Tony Lindgren wrote:
> > * Grygorii Strashko <grygorii.stras...@ti.com> [170106 12:56]:
> > > TI DRA72-EVM Rev C has two DP83867 ethernet phys which support IRQ
> > > generation in case of phy/link status changes. The INT/PWDN lines from 
> > > both
> > > DP83867 phys are wired to DRA7 gpio6.16, so reflect the same in DT.
> > 
> > Hmm not seeing the patch 1/2 here.. Can this one be queued separately?
> > Is it for v4.11 or a fix?
> 
> This is for next (v4.11) and there is just a mistake in subj, Sry.

OK applying into omap-for-v4.11/dt thanks.

Tony

Re: [PATCH 2/2] ARM: dts: dra72-evm-revc: enable irqs for dp83867 eth phys

[Tony Lindgren]

* Grygorii Strashko  [170106 12:56]:
> TI DRA72-EVM Rev C has two DP83867 ethernet phys which support IRQ
> generation in case of phy/link status changes. The INT/PWDN lines from both
> DP83867 phys are wired to DRA7 gpio6.16, so reflect the same in DT.

Hmm not seeing the patch 1/2 here.. Can this one be queued separately?
Is it for v4.11 or a fix?

Regards,

Tony

> Signed-off-by: Grygorii Strashko 
> ---
>  arch/arm/boot/dts/dra72-evm-revc.dts | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/dra72-evm-revc.dts 
> b/arch/arm/boot/dts/dra72-evm-revc.dts
> index c3d939c..3ecac56 100644
> --- a/arch/arm/boot/dts/dra72-evm-revc.dts
> +++ b/arch/arm/boot/dts/dra72-evm-revc.dts
> @@ -68,6 +68,8 @@
>   ti,tx-internal-delay = ;
>   ti,fifo-depth = ;
>   ti,min-output-impedance;
> + interrupt-parent = <>;
> + interrupts = <16 IRQ_TYPE_EDGE_FALLING>;
>   };
>  
>   dp83867_1: ethernet-phy@3 {
> @@ -75,6 +77,8 @@
>   ti,rx-internal-delay = ;
>   ti,tx-internal-delay = ;
>   ti,fifo-depth = ;
> - ti,min-output-imepdance;
> + ti,min-output-impedance;
> + interrupt-parent = <>;
> + interrupts = <16 IRQ_TYPE_EDGE_FALLING>;
>   };
>  };
> -- 
> 2.10.1.dirty
> 

Re: wl1251 & mac address & calibration data

[Tony Lindgren]

* Kalle Valo <kv...@codeaurora.org> [161220 09:12]:
> Tony Lindgren <t...@atomide.com> writes:
> 
> > * Kalle Valo <kv...@codeaurora.org> [161220 03:47]:
> >> Arend Van Spriel <arend.vanspr...@broadcom.com> writes:
> >> 
> >> > On 18-12-2016 13:09, Pali Rohár wrote:
> >> >
> >> >> File wl1251-nvs.bin is provided by linux-firmware package and contains 
> >> >> default data which should be overriden by model specific calibrated 
> >> >> data.
> >> >
> >> > Ah. Someone thought it was a good idea to provide the "one ring to rule
> >> > them all". Nice.
> >> 
> >> Yes, that was a bad idea. wl1251-nvs.bin in linux-firmware.git should be
> >> renamed to wl1251-nvs.bin.example, or something like that, as it should
> >> be only installed to a real system only if there's no real calibration
> >> data available (only for developers to use, not real users).
> >
> > Makes sense to me. Note that with the recent changes to wlcore, we can
> > now easily provide board specific calibration firmware simply by adding a
> > new compatible value. So for n900, we could have something like
> > compatible = "ti,wl1251-n900" and have it point to n900 specific calibration
> > file wl1251-nvs-n900.bin. Of course this won't help with the mac address,
> > or any of the device specific data..
> >
> > That is assuming the calibration values are the same for each similar
> > device and don't have to be generated for each device. And naturally wl1251
> > needs simlar changes done to make use of devices specific calibration files.
> 
> No, these are unique per each sold device. Every N900 was calibrated at
> the factory and they all have different calibration data which is stored
> to the flash. So when N900 boots (and in _every_ boot) it has to load
> the calibration data from the flash and provide it to the wl1251 driver
> somehow.

Urgh, OK. So much for that idea then.

Thanks,

Tony

Re: wl1251 & mac address & calibration data

[Tony Lindgren]

* Kalle Valo  [161220 03:47]:
> Arend Van Spriel  writes:
> 
> > On 18-12-2016 13:09, Pali Rohár wrote:
> >
> >> File wl1251-nvs.bin is provided by linux-firmware package and contains 
> >> default data which should be overriden by model specific calibrated 
> >> data.
> >
> > Ah. Someone thought it was a good idea to provide the "one ring to rule
> > them all". Nice.
> 
> Yes, that was a bad idea. wl1251-nvs.bin in linux-firmware.git should be
> renamed to wl1251-nvs.bin.example, or something like that, as it should
> be only installed to a real system only if there's no real calibration
> data available (only for developers to use, not real users).

Makes sense to me. Note that with the recent changes to wlcore, we can
now easily provide board specific calibration firmware simply by adding a
new compatible value. So for n900, we could have something like
compatible = "ti,wl1251-n900" and have it point to n900 specific calibration
file wl1251-nvs-n900.bin. Of course this won't help with the mac address,
or any of the device specific data..

That is assuming the calibration values are the same for each similar
device and don't have to be generated for each device. And naturally wl1251
needs simlar changes done to make use of devices specific calibration files.

Regards,

Tony

Re: wl1251 & mac address & calibration data

[Tony Lindgren]

* Pali Rohár  [161126 09:21]:
> On Thursday 24 November 2016 19:46:01 Aaro Koskinen wrote:
> > Hi,
> > 
> > On Thu, Nov 24, 2016 at 04:20:45PM +0100, Pali Rohár wrote:
> > > Proprietary, signed and closed bootloader NOLO does not support DT.
> > > So for booting you need to append DTS file to kernel image.
> > > 
> > > U-Boot is optional and can be used as intermediate bootloader
> > > between NOLO and kernel. But still it has problems with reading
> > > from nand, so cannot read NVS data nor MAC address.
> > 
> > You could use kexec to pass the fixed DT.
> > 
> > A.
> 
> IIRC it was broken for N900/omap3, no idea if somebody fixed it.

FYI, at least in next-20161205 kexec works on omap3 for me.

Regards,

Tony

Re: [PATCH v2] net: cpsw: fix obtaining mac address for am3517

[Tony Lindgren]

* Jeroen Hofstee <jhofs...@victronenergy.com> [161028 11:19]:
> Hello Tony,
> 
> On 28-10-16 17:52, Tony Lindgren wrote:
> > * Jeroen Hofstee <jhofs...@victronenergy.com> [161028 08:33]:
> > > Commit b6745f6e4e63 ("drivers: net: cpsw: davinci_emac: move reading mac
> > > id to common file") did not only move the code for an am3517, it also
> > > added the slave parameter, resulting in an invalid (all zero) mac address
> > > being returned for an am3517, since it only has a single emac and the 
> > > slave
> > > parameter is pointing to the second. So simply always read the first and
> > > valid mac-address for a ti,am3517-emac.
> > And others davinci_emac.c users can have more than one. So is the
> > reason the slave parameter points to the second instance because
> > of the location in the hardware?
> 
> Sort of, the slave parameter gets determined by the fact if there is one
> or two register range(s) associated with the davinci_emac. In davinci_emac.c
> 
> res_ctrl = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> ...
> rc = davinci_emac_try_get_mac(pdev, res_ctrl ? 0 : 1,
>   priv->mac_addr);
> 
> So it there are two ranges, the slave param becomes 0. It there is only one,
> it
> will be 1. Since the am3517 only has a single regs entry it ends up with
> slave 1,
> while there is only a single davinci_emac.

OK thanks for clarifying it:

Acked-by: Tony Lindgren <t...@atomide.com>

Re: [PATCH v2] net: cpsw: fix obtaining mac address for am3517

[Tony Lindgren]

* Jeroen Hofstee  [161028 08:33]:
> Commit b6745f6e4e63 ("drivers: net: cpsw: davinci_emac: move reading mac
> id to common file") did not only move the code for an am3517, it also
> added the slave parameter, resulting in an invalid (all zero) mac address
> being returned for an am3517, since it only has a single emac and the slave
> parameter is pointing to the second. So simply always read the first and
> valid mac-address for a ti,am3517-emac.

And others davinci_emac.c users can have more than one. So is the
reason the slave parameter points to the second instance because
of the location in the hardware?

Regards,

Tony

Re: [PATCH] net: cpsw: fix obtaining mac address for am3517

[Tony Lindgren]

* Jeroen Hofstee  [161021 02:31]:
> Aaah, lets wait a sec. I just saw there is another user of this function,
> so above is simply not true
> 
> if (of_machine_is_compatible("ti,dra7"))
> return davinci_emac_3517_get_macid(dev, 0x514, slave, mac_addr);

Oh OK, then this will produce duplicate macs.

> So let me check if I don't break that one. or better, let me send a v2,
> which
> changes the caller to pass slave as 0 here?
> 
> if (of_device_is_compatible(dev->of_node, "ti,am3517-emac"))
> return davinci_emac_3517_get_macid(dev, 0x110, slave, mac_addr);
> 

Yeah that sounds good to me.

Regards,

Tony

Re: [PATCH] net: cpsw: fix obtaining mac address for am3517

[Tony Lindgren]

* Jeroen Hofstee <jhofs...@victronenergy.com> [161021 00:37]:
> Hello Tony,
> 
> On 21-10-16 08:38, Tony Lindgren wrote:
> > * Jeroen Hofstee <jhofs...@victronenergy.com> [161020 12:57]:
> > > Commit b6745f6e4e63 ("drivers: net: cpsw: davinci_emac: move reading mac
> > > id to common file") did not only move the code for an am3517, it also
> > > added the slave parameter, resulting in a invalid (all zero) mac address
> > > being returned. So change it back to always read from slave zero, so it
> > > works again.
> > Hmm doesn't this now break it for cpsw with two instances?
> > 
> 
> Yes, well, they get the same mac address at least. But does it matter?
> This changes davinci_emac_3517_get_macid, the only way to get there
> is:
> 
> if (of_device_is_compatible(dev->of_node, "ti,am3517-emac"))
> return davinci_emac_3517_get_macid(dev, 0x110, slave, mac_addr)
> 
> and the only user of ti,am3517-emac is arch/arm/boot/dts/am3517.dtsi,
> which only has one emac. So the change is already am3517 specific.
> 
> > We may need am3517 specific quirk flag instead?
> 
> Given above, it is already am3517 specific. Let me know if you prefer this
> route then I will have a look at it.

Oh OK thanks for explaining it :) As it's already am3517 specific:

Acked-by: Tony Lindgren <t...@atomide.com>

Re: [PATCH] net: cpsw: fix obtaining mac address for am3517

[Tony Lindgren]

* Jeroen Hofstee  [161020 12:57]:
> Commit b6745f6e4e63 ("drivers: net: cpsw: davinci_emac: move reading mac
> id to common file") did not only move the code for an am3517, it also
> added the slave parameter, resulting in a invalid (all zero) mac address
> being returned. So change it back to always read from slave zero, so it
> works again.

Hmm doesn't this now break it for cpsw with two instances? We may need
am3517 specific quirk flag instead?

Regards,

Tony

Re: [PATCH] davinci_emac: fix setting the mac from DT

[Tony Lindgren]

* Jeroen Hofstee <jhofs...@victronenergy.com> [161019 12:39]:
> commit 9120bd6e9f77 ("net: davinci_emac: Get device dm816x MAC address
> using the cpsw code") sets the mac address to the one stored in the chip
> unconditionally, overwritten the one already set from the device tree.
> This patch makes sure the mac from DT is preserved.
> 
> On a am3517 this address is incorrectly read as all zeros, making it
> impossible to set a valid mac address without this patch.

OK, at least I don't have better ideas for fixing this:

Acked-by: Tony Lindgren <t...@atomide.com>

Re: [PATCH 3/3 v2] net: smsc911x: add wake-up event interrupt support

[Tony Lindgren]

* Linus Walleij <linus.wall...@linaro.org> [160824 06:00]:
> The SMSC911x have a line out of the chip called "PME",
> Power Management Event. When connected to an asynchronous
> interrupt controller this is able to wake the system up
> from sleep in response to certain network events.
> 
> This is the first attempt to support this in the Linux
> driver: the Qualcomm APQ8060 Dragonboard has this line
> routed to a GPIO line on the primary SoC padring, and as
> such it can be armed as a wakeup interrupt.
> 
> The patch is inspired by the wakeup code in the RTC
> subsystem.
> 
> The code looks for an additional interrupt - apart from the
> ordinary device interrupt - and in case that is present,
> we register an interrupt handler to respons to this,
> and flag the device and this interrupt as a wakeup.
> 
> Cc: Sudeep Holla <sudeep.ho...@arm.com>
> Cc: Tony Lindgren <t...@atomide.com>
> Cc: Florian Fainelli <f.faine...@gmail.com>
> Signed-off-by: Linus Walleij <linus.wall...@linaro.org>
> ---
> ChangeLog v1->v2:
> - Call pm_wakeup_event() in the wakeup IRQ thread to
>   account for the wakeup event.
> - Drop the enable/disable_irq_wake() calls from suspend/resume:
>   this is handled from the irq core when you call
>   dev_pm_set_wake_irq() as we do.

Looks OK to me:

Acked-by: Tony Lindgren <t...@atomide.com>

Re: [PATCH 3/3] RFC: net: smsc911x: add wake-up event interrupt support

[Tony Lindgren]

* Linus Walleij  [160708 02:10]:
> The SMSC911x have a line out of the chip called "PME",
> Power Management Event. When connected to an asynchronous
> interrupt controller this is able to wake the system up
> from sleep in response to certain network events.

Cool, so far have not found any boards here with that
connected. Should be possible to solder that on at least
omap3-evm at some point though.

> +static irqreturn_t smsc911x_pme_irq_thread(int irq, void *dev_id)
> +{
> + struct net_device *dev = dev_id;
> + struct smsc911x_data *pdata __maybe_unused = netdev_priv(dev);
> +
> + SMSC_TRACE(pdata, pm, "wakeup event");
> + /* This signal is active for 50 ms, wait for it to deassert */
> + usleep_range(5, 10);
> + return IRQ_HANDLED;
> +}

This probably adds an extra 50 ms latency before we wake up
the smsc911x driver. Maybe we can eventually come up with
some nice solution to that too. Anyways, no objections on
my side for this patch as it provides a nice GPIO wakeirq test
case that can be automated with just ping -c 1 :)

Regards,

Tony

Re: [PATCH 15/15] ARM: dts: am335x/am437x/dra7: use new "ti,cpsw-mdio" compat string

[Tony Lindgren]

* Grygorii Strashko <grygorii.stras...@ti.com> [160615 05:05]:
> Add "ti,cpsw-mdio" for am335x/am437x/dra7 SoCs where MDIO is
> implemented as part of TI CPSW and, this way, enable PM runtime auto
> suspend for Davinci MDIO driver on these paltforms.

This one should not cause merge conflicts, please feel free
to merge along with the other CPSW driver patches after the
pending comments are dealt with:

Acked-by: Tony Lindgren <t...@atomide.com>

Re: NFSroot hangs with bad unlock balance in Linux next

[Tony Lindgren]

* Al Viro <v...@zeniv.linux.org.uk> [160509 08:41]:
> On Mon, May 09, 2016 at 08:21:38AM -0700, Tony Lindgren wrote:
> 
> > Looks like with both patches applied I still also get this eventually:
> > 
> > =
> > [ BUG: bad unlock balance detected! ]
> > 4.6.0-rc7-next-20160509+ #1264 Not tainted
> > -
> 
> Lockdep warnings are noise.  To make them STFU try the following incremental;
> I'll fold it into #work.lookups and #for-next.  Note that it will do nothing
> to hangs - those are completely unrelated and you need Eric's patch to deal
> with them.

OK yeah that helps thanks:

Tested-by: Tony Lindgren <t...@atomide.com>

> diff --git a/fs/nfs/unlink.c b/fs/nfs/unlink.c
> index d367b06..1868246 100644
> --- a/fs/nfs/unlink.c
> +++ b/fs/nfs/unlink.c
> @@ -64,7 +64,7 @@ static void nfs_async_unlink_release(void *calldata)
>   struct dentry *dentry = data->dentry;
>   struct super_block *sb = dentry->d_sb;
>  
> - up_read(_I(d_inode(dentry->d_parent))->rmdir_sem);
> + up_read_non_owner(_I(d_inode(dentry->d_parent))->rmdir_sem);
>   d_lookup_done(dentry);
>   nfs_free_unlinkdata(data);
>   dput(dentry);
> @@ -117,10 +117,10 @@ static int nfs_call_unlink(struct dentry *dentry, 
> struct nfs_unlinkdata *data)
>   struct inode *dir = d_inode(dentry->d_parent);
>   struct dentry *alias;
>  
> - down_read(_I(dir)->rmdir_sem);
> + down_read_non_owner(_I(dir)->rmdir_sem);
>   alias = d_alloc_parallel(dentry->d_parent, >args.name, >wq);
>   if (IS_ERR(alias)) {
> - up_read(_I(dir)->rmdir_sem);
> + up_read_non_owner(_I(dir)->rmdir_sem);
>   return 0;
>   }
>   if (!d_in_lookup(alias)) {
> @@ -142,7 +142,7 @@ static int nfs_call_unlink(struct dentry *dentry, struct 
> nfs_unlinkdata *data)
>   ret = 0;
>   spin_unlock(>d_lock);
>   dput(alias);
> - up_read(_I(dir)->rmdir_sem);
> + up_read_non_owner(_I(dir)->rmdir_sem);
>   /*
>* If we'd displaced old cached devname, free it.  At that
>* point dentry is definitely not a root, so we won't need

Re: NFSroot hangs with bad unlock balance in Linux next

[Tony Lindgren]

* Tony Lindgren <t...@atomide.com> [160509 08:15]:
> * Eric Dumazet <eduma...@google.com> [160509 07:16]:
> > On Mon, May 9, 2016 at 12:32 AM, Al Viro <v...@zeniv.linux.org.uk> wrote:
> > > On Sun, May 08, 2016 at 03:16:29PM +0100, Al Viro wrote:
> > >
> > >> Very strange.  We grab that rwsem at the entry into nfs_call_unlink()
> > >> and then either release it there and return or call nfs_do_call_unlink().
> > >> Which arranges for eventual call of nfs_async_unlink_release() (via
> > >> ->rpc_release); nfs_async_unlink_release() releases the rwsem.  Nobody 
> > >> else
> > >> releases it (on the read side, that is).
> > >>
> > >> The only kinda-sorta possibility I see here is that the inode we are
> > >> unlocking in that nfs_async_unlink_release() is not the one we'd locked
> > >> in nfs_call_unlink() that has lead to it.  That really shouldn't happen,
> > >> though...  Just to verify whether that's what we are hitting, could you
> > >> try to reproduce that thing with the patch below on top of -next and see
> > >> if it triggers any of those WARN_ON?
> 
> Thanks no warnings with that patch though.
> 
> > > D'oh...  Lockdep warnings are easy to trigger (and, AFAICS, bogus).
> > > up_read/down_read in fs/nfs/unlink.c should be replaced with
> > > up_read_non_owner/down_read_non_owner, lest the lockdep gets confused.
> > > Hangs are different - I've no idea what's triggering those.  I've seen
> > > something similar on that -next, but not on work.lookups.
> > >
> > > The joy of bisecting -next...  
> > > 9317bb69824ec8d078b0b786b6971aedb0af3d4f is the first bad commit
> > > commit 9317bb69824ec8d078b0b786b6971aedb0af3d4f
> > > Author: Eric Dumazet <eduma...@google.com>
> > > Date:   Mon Apr 25 10:39:32 2016 -0700
> > >
> > > net: SOCKWQ_ASYNC_NOSPACE optimizations
> > >
> > > Reverting changes to sk_set_bit/sk_clear_bit gets rid of the hangs.  Plain
> > > revert gives a conflict, since there had been additional change in
> > > "net: SOCKWQ_ASYNC_WAITDATA optimizations"; removing both fixed the hangs.
> > >
> > > Note that hangs appear without any fs/nfs/unlink.c modifications being
> > > there.  When the hang happens it affects NFS traffic; ssh session still
> > > works fine until it steps on a filesystem operation on NFS (i.e. you
> > > can use builtins, access procfs, etc.)
> > 
> > Yeah, the issue was reported last week (
> > http://www.spinics.net/lists/netdev/msg375777.html ),
> > and I could not convince myself to add a new sock flag,  like
> > SOCK_FASYNC_STICKY.
> > 
> > (Just in case NFS would ever call sock_fasync() with an empty
> > fasync_list, and SOCK_FASYNC would be cleared again.
> 
> Yeah applying the test patch from the url above makes things work
> for me again.

Looks like with both patches applied I still also get this eventually:

=
[ BUG: bad unlock balance detected! ]
4.6.0-rc7-next-20160509+ #1264 Not tainted
-
kworker/0:1/18 is trying to release lock (>rmdir_sem) at:
[] nfs_async_unlink_release+0x3c/0xc0
but there are no more locks to release!

   other info that might help us debug this:
2 locks held by kworker/0:1/18:
 #0:  ("nfsiod"){.+.+..}, at: [] process_one_work+0x120/0x6bc
 #1:  ((>u.tk_work)#2){+.+...}, at: [] 
process_one_work+0x120/0x6bc

   stack backtrace:
CPU: 0 PID: 18 Comm: kworker/0:1 Not tainted 4.6.0-rc7-next-20160509+ #1264
Hardware name: Generic OMAP5 (Flattened Device Tree)
Workqueue: nfsiod rpc_async_release
[] (unwind_backtrace) from [] (show_stack+0x10/0x14)
[] (show_stack) from [] (dump_stack+0xb0/0xe4)
[] (dump_stack) from [] 
(print_unlock_imbalance_bug+0xb0/0xe0)
[] (print_unlock_imbalance_bug) from [] 
(lock_release+0x2ec/0x4c0)
[] (lock_release) from [] (up_read+0x18/0x58)
[] (up_read) from [] (nfs_async_unlink_release+0x3c/0xc0)
[] (nfs_async_unlink_release) from [] 
(rpc_free_task+0x24/0x44)
[] (rpc_free_task) from [] (process_one_work+0x1e8/0x6bc)
[] (process_one_work) from [] (worker_thread+0x144/0x4e8)
[] (worker_thread) from [] (kthread+0xdc/0xf8)
[] (kthread) from [] (ret_from_fork+0x14/0x24)

After the warning, NFSroot keeps working with Eric's patch.

Regards,

Tony

Re: NFSroot hangs with bad unlock balance in Linux next

[Tony Lindgren]

* Eric Dumazet  [160509 07:16]:
> On Mon, May 9, 2016 at 12:32 AM, Al Viro  wrote:
> > On Sun, May 08, 2016 at 03:16:29PM +0100, Al Viro wrote:
> >
> >> Very strange.  We grab that rwsem at the entry into nfs_call_unlink()
> >> and then either release it there and return or call nfs_do_call_unlink().
> >> Which arranges for eventual call of nfs_async_unlink_release() (via
> >> ->rpc_release); nfs_async_unlink_release() releases the rwsem.  Nobody else
> >> releases it (on the read side, that is).
> >>
> >> The only kinda-sorta possibility I see here is that the inode we are
> >> unlocking in that nfs_async_unlink_release() is not the one we'd locked
> >> in nfs_call_unlink() that has lead to it.  That really shouldn't happen,
> >> though...  Just to verify whether that's what we are hitting, could you
> >> try to reproduce that thing with the patch below on top of -next and see
> >> if it triggers any of those WARN_ON?

Thanks no warnings with that patch though.

> > D'oh...  Lockdep warnings are easy to trigger (and, AFAICS, bogus).
> > up_read/down_read in fs/nfs/unlink.c should be replaced with
> > up_read_non_owner/down_read_non_owner, lest the lockdep gets confused.
> > Hangs are different - I've no idea what's triggering those.  I've seen
> > something similar on that -next, but not on work.lookups.
> >
> > The joy of bisecting -next...  
> > 9317bb69824ec8d078b0b786b6971aedb0af3d4f is the first bad commit
> > commit 9317bb69824ec8d078b0b786b6971aedb0af3d4f
> > Author: Eric Dumazet 
> > Date:   Mon Apr 25 10:39:32 2016 -0700
> >
> > net: SOCKWQ_ASYNC_NOSPACE optimizations
> >
> > Reverting changes to sk_set_bit/sk_clear_bit gets rid of the hangs.  Plain
> > revert gives a conflict, since there had been additional change in
> > "net: SOCKWQ_ASYNC_WAITDATA optimizations"; removing both fixed the hangs.
> >
> > Note that hangs appear without any fs/nfs/unlink.c modifications being
> > there.  When the hang happens it affects NFS traffic; ssh session still
> > works fine until it steps on a filesystem operation on NFS (i.e. you
> > can use builtins, access procfs, etc.)
> 
> Yeah, the issue was reported last week (
> http://www.spinics.net/lists/netdev/msg375777.html ),
> and I could not convince myself to add a new sock flag,  like
> SOCK_FASYNC_STICKY.
> 
> (Just in case NFS would ever call sock_fasync() with an empty
> fasync_list, and SOCK_FASYNC would be cleared again.

Yeah applying the test patch from the url above makes things work
for me again.

Regards,

Tony

Re: [PATCH net v3 0/5] drivers: net: cpsw: phy-handle fixes

[Tony Lindgren]

* David Rivshin (Allworx) <drivshin.allw...@gmail.com> [160427 18:13]:
> From: David Rivshin <drivs...@allworx.com>
> 
> This series fixes a number of related issues around using phy-handle
> properties in cpsw emac nodes.
> 
> Patch 1 fixes a bug if more than one slave is used, and either
> slave uses the phy-handle property in the devicetree.
> 
> Patch 2 fixes a NULL pointer dereference which can occur if a
> phy-handle property is used and of_phy_connect() return NULL,
> such as with a bad devicetree.
> 
> Patch 3 fixes an issue where the phy-mode property would be ignored
> if a phy-handle property was used. This also fixes a bogus error
> message that would be emitted.
> 
> Patch 4 fixes makes the binding documentation more explicit that
> exactly one PHY property should be used, and also marks phy_id as
> deprecated.
> 
> Patch 5 cleans up the fixed-link case to work like the now-fixed
> phy-handle case.
> 
> I have tested on the following hardware configurations:
>  - (EVMSK) dual emac, phy_id property in both slaves
>  - (EVMSK) dual emac, phy-handle property in both slaves
>  - (EVMSK) a bad phy-handle property pointing to 
>  - (EVMSK) phy_id property with incorrect PHY address
>  - (BeagleBoneBlack) single emac, phy_id property
>  - (custom) single emac, fixed-link subnode
> 
> Andrew Goodbody reported testing v2 on a board that doesn't use
> dual_emac mode, but with 2 PHYs using phy-handle properties [1].
> 
> Nicolas Chauvet reported testing v2 on an HP t410 (dm8148).
> 
> Markus Brunner reported testing v1 on the following [2]:
>  - emac0 with phy_id and emac1 with fixed phy
>  - emac0 with phy-handle and emac1 with fixed phy
>  - emac0 with fixed phy and emac1 with fixed phy

Quickly boot tested these against next on dra62x-j5eco EVM:

Tested-by: Tony Lindgren <t...@atomide.com>

Re: [PATCH v2] MAINTAINERS: net: add entry for TI Ethernet Switch drivers

[Tony Lindgren]

* Mugunthan V N <mugunthan...@ti.com> [160421 04:19]:
> On Thursday 21 April 2016 03:43 PM, Grygorii Strashko wrote:
> > Add record for TI Ethernet Switch Driver CPSW/CPDMA/MDIO HW
> > (am33/am43/am57/dr7/davinci) to ensure that related patches
> > will go through dedicated linux-omap list.
> > 
> > Also add Mugunthan as maintainer and myself as the reviewer.
> > 
> > Cc: "David S. Miller" <da...@davemloft.net>
> > Cc: Tony Lindgren <t...@atomide.com>
> > Cc: Mugunthan V N <mugunthan...@ti.com>
> > Cc: Richard Cochran <richardcoch...@gmail.com>
> > Signed-off-by: Grygorii Strashko <grygorii.stras...@ti.com>
> 
> Acked-by: Mugunthan V N <mugunthan...@ti.com>

Looks good to me too thanks:

Acked-by: Tony Lindgren <t...@atomide.com>

Re: [PATCH] MAINTAINERS: net: add entry for TI Ethernet Switch drivers

[Tony Lindgren]

* Grygorii Strashko <grygorii.stras...@ti.com> [160420 09:19]:
> On 04/20/2016 05:23 PM, Tony Lindgren wrote:
> > * Grygorii Strashko <grygorii.stras...@ti.com> [160420 04:26]:
> >> Add record for TI Ethernet Switch Driver CPSW/CPDMA/MDIO HW
> >> (am33/am43/am57/dr7/davinci) to ensure that related patches
> >> will go through dedicated linux-omap list.
> >>
> >> Also add Mugunthan as maintainer and myself as the reviewer.
> >>
> >> Cc: "David S. Miller" <da...@davemloft.net>
> >> Cc: Mugunthan V N <mugunthan...@ti.com>
> >> Cc: Richard Cochran <richardcoch...@gmail.com>
> >> Signed-off-by: Grygorii Strashko <grygorii.stras...@ti.com>
> >> ---
> >>   MAINTAINERS | 8 
> >>   1 file changed, 8 insertions(+)
> >>
> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> index 1d5b4be..aca864d 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -11071,6 +11071,14 @@ S:Maintained
> >>   F:   drivers/clk/ti/
> >>   F:   include/linux/clk/ti.h
> >>   
> >> +TI ETHERNET SWITCH DRIVER (CPSW)
> >> +M:Mugunthan V N <mugunthan...@ti.com>
> >> +R:Grygorii Strashko <grygorii.stras...@ti.com>
> >> +L:linux-o...@vger.kernel.org
> >> +S:Maintained
> >> +F:drivers/net/ethernet/ti/cpsw*
> >> +F:drivers/net/ethernet/ti/davinci*
> >> +
> >>   TI FLASH MEDIA INTERFACE DRIVER
> >>   M:   Alex Dubov <oa...@yahoo.com>
> >>   S:   Maintained
> >> -- 
> > 
> > Please add netdev list also there as the primary list:
> > 
> > L:  netdev@vger.kernel.org
> > L:  linux-o...@vger.kernel.org
> > 
> > Then we can easily review and ack the patches for Dave to apply.
> > 
> 
> I can, but want clarify if it really necessary, because get_maintainer.pl
> automatically adds netdev@vger.kernel.org:

Well it may not be obvious from reading MAINTAINERS file though :)

Tony

Re: Regression in next for smsc911x with tigthen lockdep checks

[Tony Lindgren]

* Hannes Frederic Sowa <han...@stressinduktion.org> [160420 08:24]:
> Hi,
> 
> On 20.04.2016 17:01, Tony Lindgren wrote:
> > Looks like commit fafc4e1ea1a4 ("sock: tigthen lockdep checks for
> > sock_owned_by_user") in next causes a regression at least for
> > smsc911x with CONFIG_LOCKDEP. It keeps spamming with the following
> > message. Any ideas?
> 
> Not yet, can you quickly send me your config?

It's just the arch/arm/configs/omap2plus_defconfig I'm using.

Tony

Re: [PATCH 1/1] Revert "Prevent NUll pointer dereference with two PHYs on cpsw"

[Tony Lindgren]

* Andrew Goodbody <andrew.goodb...@cambrionix.com> [160420 07:51]:
> This reverts commit cfe255600154f0072d4a8695590dbd194dfd1aeb
> 
> This can result in a "Unable to handle kernel paging request"
> during boot. This was due to using an uninitialised struct member,
> data->slaves.

Missing Signed-off-by?

This gets cpsw boards working in next for me again:

Tested-by: Tony Lindgren <t...@atomide.com>

Re: Regression in next for smsc911x with tigthen lockdep checks

[Tony Lindgren]

* Tony Lindgren <t...@atomide.com> [160420 08:02]:
> Hi,
> 
> Looks like commit fafc4e1ea1a4 ("sock: tigthen lockdep checks for
> sock_owned_by_user") in next causes a regression at least for
> smsc911x with CONFIG_LOCKDEP. It keeps spamming with the following
> message. Any ideas?

Sorry forgot to add Steve to Cc, added now.

> Regards,
> 
> Tony
> 
> 8< 
> WARNING: CPU: 0 PID: 0 at include/net/sock.h:1408 
> udp_queue_rcv_skb+0x398/0x640
> Modules linked in:
> CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.6.0-rc4-next-20160420 #1087
> Hardware name: Generic OMAP36xx (Flattened Device Tree)
> [] (unwind_backtrace) from [] (show_stack+0x10/0x14)
> [] (show_stack) from [] (dump_stack+0xb0/0xe4)
> [] (dump_stack) from [] (__warn+0xd8/0x104)
> [] (__warn) from [] (warn_slowpath_null+0x20/0x28)
> [] (warn_slowpath_null) from [] 
> (udp_queue_rcv_skb+0x398/0x640)
> [] (udp_queue_rcv_skb) from [] 
> (__udp4_lib_rcv+0x4cc/0xc0c)
> [] (__udp4_lib_rcv) from [] 
> (ip_local_deliver_finish+0xcc/0x4f4)
> [] (ip_local_deliver_finish) from [] 
> (ip_local_deliver+0xcc/0xd8)
> [] (ip_local_deliver) from [] (ip_rcv_finish+0xbc/0x700)
> [] (ip_rcv_finish) from [] (ip_rcv+0x48c/0x6d4)
> [] (ip_rcv) from [] (__netif_receive_skb_core+0x380/0xa10)
> [] (__netif_receive_skb_core) from [] 
> (netif_receive_skb_internal+
> 0x74/0x1ec)
> [] (netif_receive_skb_internal) from [] 
> (smsc911x_poll+0xd8/0x228)
> [] (smsc911x_poll) from [] (net_rx_action+0x124/0x470)
> [] (net_rx_action) from [] (__do_softirq+0xc8/0x54c)
> [] (__do_softirq) from [] (irq_exit+0xbc/0x130)
> [] (irq_exit) from [] (__handle_domain_irq+0x6c/0xdc)
> [] (__handle_domain_irq) from [] (__irq_svc+0x58/0x78)
> [] (__irq_svc) from [] (cpuidle_enter_state+0xc4/0x3d4)
> [] (cpuidle_enter_state) from [] 
> (cpu_startup_entry+0x198/0x3a0)
> [] (cpu_startup_entry) from [] (start_kernel+0x350/0x3c8)
> [] (start_kernel) from [<8000807c>] (0x8000807c)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Regression in next for smsc911x with tigthen lockdep checks

[Tony Lindgren]

Hi,

Looks like commit fafc4e1ea1a4 ("sock: tigthen lockdep checks for
sock_owned_by_user") in next causes a regression at least for
smsc911x with CONFIG_LOCKDEP. It keeps spamming with the following
message. Any ideas?

Regards,

Tony

8< 
WARNING: CPU: 0 PID: 0 at include/net/sock.h:1408 udp_queue_rcv_skb+0x398/0x640
Modules linked in:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.6.0-rc4-next-20160420 #1087
Hardware name: Generic OMAP36xx (Flattened Device Tree)
[] (unwind_backtrace) from [] (show_stack+0x10/0x14)
[] (show_stack) from [] (dump_stack+0xb0/0xe4)
[] (dump_stack) from [] (__warn+0xd8/0x104)
[] (__warn) from [] (warn_slowpath_null+0x20/0x28)
[] (warn_slowpath_null) from [] 
(udp_queue_rcv_skb+0x398/0x640)
[] (udp_queue_rcv_skb) from [] (__udp4_lib_rcv+0x4cc/0xc0c)
[] (__udp4_lib_rcv) from [] 
(ip_local_deliver_finish+0xcc/0x4f4)
[] (ip_local_deliver_finish) from [] 
(ip_local_deliver+0xcc/0xd8)
[] (ip_local_deliver) from [] (ip_rcv_finish+0xbc/0x700)
[] (ip_rcv_finish) from [] (ip_rcv+0x48c/0x6d4)
[] (ip_rcv) from [] (__netif_receive_skb_core+0x380/0xa10)
[] (__netif_receive_skb_core) from [] 
(netif_receive_skb_internal+
0x74/0x1ec)
[] (netif_receive_skb_internal) from [] 
(smsc911x_poll+0xd8/0x228)
[] (smsc911x_poll) from [] (net_rx_action+0x124/0x470)
[] (net_rx_action) from [] (__do_softirq+0xc8/0x54c)
[] (__do_softirq) from [] (irq_exit+0xbc/0x130)
[] (irq_exit) from [] (__handle_domain_irq+0x6c/0xdc)
[] (__handle_domain_irq) from [] (__irq_svc+0x58/0x78)
[] (__irq_svc) from [] (cpuidle_enter_state+0xc4/0x3d4)
[] (cpuidle_enter_state) from [] 
(cpu_startup_entry+0x198/0x3a0)
[] (cpu_startup_entry) from [] (start_kernel+0x350/0x3c8)
[] (start_kernel) from [<8000807c>] (0x8000807c)

Re: [PATCH] MAINTAINERS: net: add entry for TI Ethernet Switch drivers

[Tony Lindgren]

* Grygorii Strashko  [160420 04:26]:
> Add record for TI Ethernet Switch Driver CPSW/CPDMA/MDIO HW
> (am33/am43/am57/dr7/davinci) to ensure that related patches
> will go through dedicated linux-omap list.
> 
> Also add Mugunthan as maintainer and myself as the reviewer.
> 
> Cc: "David S. Miller" 
> Cc: Mugunthan V N 
> Cc: Richard Cochran 
> Signed-off-by: Grygorii Strashko 
> ---
>  MAINTAINERS | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1d5b4be..aca864d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11071,6 +11071,14 @@ S:   Maintained
>  F:   drivers/clk/ti/
>  F:   include/linux/clk/ti.h
>  
> +TI ETHERNET SWITCH DRIVER (CPSW)
> +M:   Mugunthan V N 
> +R:   Grygorii Strashko 
> +L:   linux-o...@vger.kernel.org
> +S:   Maintained
> +F:   drivers/net/ethernet/ti/cpsw*
> +F:   drivers/net/ethernet/ti/davinci*
> +
>  TI FLASH MEDIA INTERFACE DRIVER
>  M:   Alex Dubov 
>  S:   Maintained
> -- 

Please add netdev list also there as the primary list:

L:  netdev@vger.kernel.org
L:  linux-o...@vger.kernel.org

Then we can easily review and ack the patches for Dave to apply.

Regards,

Tony

[PATCH] net: cpsw: Fix ethernet regression for dm814x

[Tony Lindgren]

Commit b6745f6e4e63 ("drivers: net: cpsw: davinci_emac: move reading mac
id to common file") started using of_machine_is_compatible for detecting
type but missed at dm8148 causing Ethernet to stop working.

Let's fix the issue by adding handling for dm814x.

Cc: Mugunthan V N <mugunthan...@ti.com>
Signed-off-by: Tony Lindgren <t...@atomide.com>
---
 drivers/net/ethernet/ti/cpsw-common.c | 3 +++
 1 file changed, 3 insertions(+)

--- a/drivers/net/ethernet/ti/cpsw-common.c
+++ b/drivers/net/ethernet/ti/cpsw-common.c
@@ -78,6 +78,9 @@ static int cpsw_am33xx_cm_get_macid(struct device *dev, u16 
offset, int slave,
 
 int ti_cm_get_macid(struct device *dev, int slave, u8 *mac_addr)
 {
+   if (of_machine_is_compatible("ti,dm8148"))
+   return cpsw_am33xx_cm_get_macid(dev, 0x630, slave, mac_addr);
+
if (of_machine_is_compatible("ti,am33xx"))
return cpsw_am33xx_cm_get_macid(dev, 0x630, slave, mac_addr);
 
-- 
2.6.2

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] net: davinci_emac: Add support for fixed-link PHY

[Tony Lindgren]

* Neil Armstrong <narmstr...@baylibre.com> [150922 02:01]:
> In case the DaVinci Emac is directly connected to a
> non-mdio PHY/device, it should be possible to provide
> a fixed link configuration in the DT.
> 
> Signed-off-by: Neil Armstrong <narmstr...@baylibre.com>

Ethernet works for me with this patch:

Tested-by: Tony Lindgren <t...@atomide.com>

> ---
>  drivers/net/ethernet/ti/davinci_emac.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/davinci_emac.c 
> b/drivers/net/ethernet/ti/davinci_emac.c
> index aeebc0a..6521dfb 100644
> --- a/drivers/net/ethernet/ti/davinci_emac.c
> +++ b/drivers/net/ethernet/ti/davinci_emac.c
> @@ -1861,8 +1861,12 @@ davinci_emac_of_get_pdata(struct platform_device 
> *pdev, struct emac_priv *priv)
>   pdata->no_bd_ram = of_property_read_bool(np, "ti,davinci-no-bd-ram");
> 
>   priv->phy_node = of_parse_phandle(np, "phy-handle", 0);
> - if (!priv->phy_node)
> - pdata->phy_id = NULL;
> + if (!priv->phy_node) {
> + if (!of_phy_is_fixed_link(np))
> + pdata->phy_id = NULL;
> + else if (of_phy_register_fixed_link(np) >= 0)
> + priv->phy_node = of_node_get(np);
> + }
> 
>   auxdata = pdev->dev.platform_data;
>   if (auxdata) {
> -- 
> 1.9.1
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [net-next PATCH v3] drivers: net: cpsw: Add support to drive gpios for ethernet to be functional

[Tony Lindgren]

* Mugunthan V N <mugunthan...@ti.com> [150907 02:50]:
> In DRA72x EVM, by default slave 1 is connected to the onboard
> phy, but slave 2 pins are also muxed with video input module
> which is controlled by pcf857x gpio and currently to select slave
> 0 to connect to phy gpio hogging is used, but with
> omap2plus_defconfig the pcf857x gpio is built as module. So when
> using NFS on DRA72x EVM, board doesn't boot as gpio hogging do
> not set proper gpio state to connect slave 0 to phy as it is
> built as module and you do not see any errors for not setting
> gpio and just mentions dhcp reply not got.
> 
> To solve this issue, introducing "mode-gpios" in DT when gpio
> based muxing is required. This will throw a warning when gpio
> get fails and returns probe defer. When gpio-pcf857x module is
> installed, cpsw probes again and ethernet becomes functional.
> Verified this on DRA72x with pcf as module and ramdisk.
> 
> Signed-off-by: Mugunthan V N <mugunthan...@ti.com>

Acked-by: Tony Lindgren <t...@atomide.com>

> ---
> 
> Changes from v2:
> * Used mode-gpios, so that the driver is generic enough to handle
>   multiple gpios
> 
> This patch is tested on DRA72x, Logs [1] and pushed a branch [2]
> 
> [1]: http://pastebin.ubuntu.com/12306224/
> [2]: git://git.ti.com/~mugunthanvnm/ti-linux-kernel/linux.git 
> cpsw-gpio-optional-v3
> 
> ---
>  Documentation/devicetree/bindings/net/cpsw.txt | 7 +++
>  drivers/net/ethernet/ti/cpsw.c | 9 +
>  2 files changed, 16 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/cpsw.txt 
> b/Documentation/devicetree/bindings/net/cpsw.txt
> index a9df21a..676ecf6 100644
> --- a/Documentation/devicetree/bindings/net/cpsw.txt
> +++ b/Documentation/devicetree/bindings/net/cpsw.txt
> @@ -30,6 +30,13 @@ Optional properties:
>  - dual_emac  : Specifies Switch to act as Dual EMAC
>  - syscon : Phandle to the system control device node, which is
> the control module device of the am33x
> +- mode-gpios : Should be added if one/multiple gpio lines are
> +   required to be driven so that cpsw data lines
> +   can be connected to the phy via selective mux.
> +   For example in dra72x-evm, pcf gpio has to be
> +   driven low so that cpsw slave 0 and phy data
> +   lines are connected via mux.
> +
>  
>  Slave Properties:
>  Required properties:
> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index 8fc90f1..c670317 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -29,6 +29,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -2207,6 +2208,7 @@ static int cpsw_probe(struct platform_device *pdev)
>   void __iomem*ss_regs;
>   struct resource *res, *ss_res;
>   const struct of_device_id   *of_id;
> + struct gpio_descs   *mode;
>   u32 slave_offset, sliver_offset, slave_size;
>   int ret = 0, i;
>   int irq;
> @@ -2232,6 +2234,13 @@ static int cpsw_probe(struct platform_device *pdev)
>   goto clean_ndev_ret;
>   }
>  
> + mode = devm_gpiod_get_array_optional(>dev, "mode", GPIOD_OUT_LOW);
> + if (IS_ERR(mode)) {
> + ret = PTR_ERR(mode);
> + dev_err(>dev, "gpio request failed, ret %d\n", ret);
> + goto clean_ndev_ret;
> + }
> +
>   /*
>* This may be required here for child devices.
>*/
> -- 
> 2.6.0.rc0.24.gec371ff
> 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [net-next PATCH v2] drivers: net: cpsw: Add support to make gpio drive which slave connected to phy

[Tony Lindgren]

* Mugunthan V N  [150902 23:05]:
> In DRA72x EVM, by default slave 1 is connected to the onboard
> phy, but slave 2 pins are also muxed with video input module
> which is controlled by pcf857x gpio and currently to select slave
> 0 to connect to phy gpio hogging is used, but with
> omap2plus_defconfig the pcf857x gpio is built as module. So when
> using NFS on DRA72x EVM, board doesn't boot as gpio hogging do
> not set proper gpio state to connect slave 0 to phy as it is
> built as module and you do not see any errors for not setting
> gpio and just mentions dhcp reply not got.
> 
> To solve this issue, introducing "mode-gpio" in DT when gpio
> based muxing is required. This will throw a warning when gpio
> get fails and returns probe defer. When gpio-pcf857x module is
> installed, cpsw probes again and ethernet becomes functional.
> Verified this on DRA72x with pcf as module and ramdisk.

Hmm you might be able to make it even a little bit more generic.
The gpios can be an array.. So typically they're named "-gpios":

[linux] $ git grep "\-gpio " arch/arm/boot/dts/*.dts* | wc -l
219
[linux] $ git grep "\-gpios " arch/arm/boot/dts/*.dts* | wc -l
704

So I'd use mode-gpios even though there's just one gpio in
this case. Up to you though, and should be retested after
the change naturally. At some point gpio code was not parsing
"gpio" or "gpios" properly.. But that's probably been fixed
a long time ago.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [net-next PATCH] drivers: net: cpsw: Add support to make gpio drive which slave connected to phy

[Tony Lindgren]

* Mugunthan V N  [150901 04:28]:
> --- a/Documentation/devicetree/bindings/net/cpsw.txt
> +++ b/Documentation/devicetree/bindings/net/cpsw.txt
> @@ -26,6 +26,9 @@ Optional properties:
>  - dual_emac  : Specifies Switch to act as Dual EMAC
>  - syscon : Phandle to the system control device node, which is
> the control module device of the am33x
> +- select-slave-gpio  : Should be added if a gpio line is required to
> +   select which slave is connected to phy
> +

How about using something more generic here for the name?
Something like mode-gpios?

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] net/smsc911x: Fix deferred probe for interrupt

[Tony Lindgren]

The interrupt handler may not be available when smsc911x probes if the
interrupt handler is a GPIO controller for example. Let's fix that
by adding handling for -EPROBE_DEFER.

Cc: Steve Glendinning steve.glendinn...@shawell.net
Signed-off-by: Tony Lindgren t...@atomide.com
---
 drivers/net/ethernet/smsc/smsc911x.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/smsc/smsc911x.c 
b/drivers/net/ethernet/smsc/smsc911x.c
index 959aeea..cb9f166f 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -2435,7 +2435,10 @@ static int smsc911x_drv_probe(struct platform_device 
*pdev)
res_size = resource_size(res);
 
irq = platform_get_irq(pdev, 0);
-   if (irq = 0) {
+   if (irq == -EPROBE_DEFER) {
+   retval = -EPROBE_DEFER;
+   goto out_0;
+   } else if (irq = 0) {
pr_warn(Could not allocate irq resource\n);
retval = -ENODEV;
goto out_0;
-- 
2.1.4

--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH -next] smsc911x: Fix crash seen if neither ACPI nor OF is configured or used

[Tony Lindgren]

* Guenter Roeck li...@roeck-us.net [150826 11:37]:
 On 08/26/2015 10:04 AM, Tony Lindgren wrote:
 Hi,
 
 * Guenter Roeck li...@roeck-us.net [150817 13:48]:
 Commit 0b50dc4fc971 (Convert smsc911x to use ACPI as well as DT) makes
 the call to smsc911x_probe_config() unconditional, and no longer fails if
 there is no device node. device_get_phy_mode() is called unconditionally,
 and if there is no phy node configured returns an error code. This error
 code is assigned to phy_interface, and interpreted elsewhere in the code
 as valid phy mode. This in turn causes qemu to crash when running a
 variant of realview_pb_defconfig.
 
 qemu: hardware error: lan9118_read: Bad reg 0x86
 
 Fixes: 0b50dc4fc971 (Convert smsc911x to use ACPI as well as DT)
 Cc: Jeremy Linton jeremy.lin...@arm.com
 Cc Graeme Gregory graeme.greg...@linaro.org
 Signed-off-by: Guenter Roeck li...@roeck-us.net
 ---
   drivers/net/ethernet/smsc/smsc911x.c | 7 ++-
   1 file changed, 6 insertions(+), 1 deletion(-)
 
 diff --git a/drivers/net/ethernet/smsc/smsc911x.c 
 b/drivers/net/ethernet/smsc/smsc911x.c
 index 0f21aa3bb537..34f97684506b 100644
 --- a/drivers/net/ethernet/smsc/smsc911x.c
 +++ b/drivers/net/ethernet/smsc/smsc911x.c
 @@ -2367,12 +2367,17 @@ static const struct smsc911x_ops 
 shifted_smsc911x_ops = {
   static int smsc911x_probe_config(struct smsc911x_platform_config *config,
  struct device *dev)
   {
 +   int phy_interface;
 u32 width = 0;
 
 if (!dev)
 return -ENODEV;
 
 -   config-phy_interface = device_get_phy_mode(dev);
 +   phy_interface = device_get_phy_mode(dev);
 +   if (phy_interface  0)
 +   return phy_interface;
 +
 +   config-phy_interface = phy_interface;
 
 device_get_mac_address(dev, config-mac, ETH_ALEN);
 
 Looks like this change makes at least omap boards using smsc911x
 fail with -22 for me in Linux next.
 
 Do any of the the device tree configured smsc911x devices actually
 have a phy configured?
 
 
 Ok, this is more subtle than I thought.
 
 Previously, the code would not attempt any devicetree configuration
 if devicetree was not configured.
 
 Now it does.
 
 The error return from device_get_phy_mode() isn't the actual problem.
 Apparently it doesn't really matter if a nonsensical value is assigned
 to phy_interface.
 
 The problem is that the reg-io-width property is obviously not present
 in the non-dt and non-acpi case. This overwrites the existing platform data
 configuration and selects 16 bit mode, to which the (simulated) hardware
 obviously reacts less than enthusiastic.
 
 Fixing this properly won't be easy. If the reg-io-width property
 is not present or wrong, the default register width is 16 bit. Obviously,
 if neither DT nor ACPI is available, it won't be present. This causes
 the crash I had observed.

Heh OK :)
 
 Bad part is that there does not seem to be a reliable means to detect
 that platform data should be used in that situation. Other device_get_XXX
 functions return -ENXIO if that happens, but not device_property_read_u32().
 It is _supposed_ to return it per its API, but it doesn't (it returns
 -ENODATA).
 
 We may need two separate patches, one to fix up device_property_read_u32()
 to return -ENXIO, and one to fix smsc911x_probe_config() to ignore the error
 from device_get_phy_mode(), and to bail out if device_property_read_u32()
 returns -ENXIO.

I guess the device_property_read_u32() change needs to be discussed
separately.. So probably best to fix up the regression to smsc911x
first.
 
 The simpler alternative would be to check the return value from
 device_property_read_u32() for both -ENXIO and -ENODATA.
 This would make the code independent of the necessary core changes
 (which may take a while). I tested this variant, and it works, at least
 for the non-DT case.
 
 Does this make sense ?

Yeh I think that would allow fixing up the smsc911x regression while
discussing the device_property_read_u32() change. Got a test patch
for me to try?

Regards,

Tony
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH -next] smsc911x: Fix crash seen if neither ACPI nor OF is configured or used

[Tony Lindgren]

Hi,

* Guenter Roeck li...@roeck-us.net [150817 13:48]:
 Commit 0b50dc4fc971 (Convert smsc911x to use ACPI as well as DT) makes
 the call to smsc911x_probe_config() unconditional, and no longer fails if
 there is no device node. device_get_phy_mode() is called unconditionally,
 and if there is no phy node configured returns an error code. This error
 code is assigned to phy_interface, and interpreted elsewhere in the code
 as valid phy mode. This in turn causes qemu to crash when running a
 variant of realview_pb_defconfig.
 
   qemu: hardware error: lan9118_read: Bad reg 0x86
 
 Fixes: 0b50dc4fc971 (Convert smsc911x to use ACPI as well as DT)
 Cc: Jeremy Linton jeremy.lin...@arm.com
 Cc Graeme Gregory graeme.greg...@linaro.org
 Signed-off-by: Guenter Roeck li...@roeck-us.net
 ---
  drivers/net/ethernet/smsc/smsc911x.c | 7 ++-
  1 file changed, 6 insertions(+), 1 deletion(-)
 
 diff --git a/drivers/net/ethernet/smsc/smsc911x.c 
 b/drivers/net/ethernet/smsc/smsc911x.c
 index 0f21aa3bb537..34f97684506b 100644
 --- a/drivers/net/ethernet/smsc/smsc911x.c
 +++ b/drivers/net/ethernet/smsc/smsc911x.c
 @@ -2367,12 +2367,17 @@ static const struct smsc911x_ops shifted_smsc911x_ops 
 = {
  static int smsc911x_probe_config(struct smsc911x_platform_config *config,
struct device *dev)
  {
 + int phy_interface;
   u32 width = 0;
  
   if (!dev)
   return -ENODEV;
  
 - config-phy_interface = device_get_phy_mode(dev);
 + phy_interface = device_get_phy_mode(dev);
 + if (phy_interface  0)
 + return phy_interface;
 +
 + config-phy_interface = phy_interface;
  
   device_get_mac_address(dev, config-mac, ETH_ALEN);

Looks like this change makes at least omap boards using smsc911x
fail with -22 for me in Linux next.

Do any of the the device tree configured smsc911x devices actually
have a phy configured?

Regards,

Tony
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH -next] smsc911x: Fix crash seen if neither ACPI nor OF is configured or used

[Tony Lindgren]

* Jeremy Linton jeremy.lin...@arm.com [150826 10:35]:
 On 08/26/2015 12:04 PM, Tony Lindgren wrote:
 * Guenter Roeck li...@roeck-us.net [150817 13:48]:
 Commit 0b50dc4fc971 (Convert smsc911x to use ACPI as well as DT) makes
 Looks like this change makes at least omap boards using smsc911x
 fail with -22 for me in Linux next.
 
 Do any of the the device tree configured smsc911x devices actually
 have a phy configured?
 
 Tony,
 
   Looks like all the ones in the kernel boot/dts directory have a phy
 including the omap3-lilly except for the ste-snowball.dts.
 
   Do you have smsc,force-internal-phy set instead?

Hmm most of them are using omap-gpmc-smsc911x.dtsi and
omap-gpmc-smsc9221.dtsi which are set up the same way as
omap3-lilly. So no phy.

Regards,

Tony
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH -next] smsc911x: Fix crash seen if neither ACPI nor OF is configured or used

[Tony Lindgren]

* Guenter Roeck li...@roeck-us.net [150826 10:40]:
 Hi Tony,
 
 On 08/26/2015 10:04 AM, Tony Lindgren wrote:
 Hi,
 
 * Guenter Roeck li...@roeck-us.net [150817 13:48]:
 Commit 0b50dc4fc971 (Convert smsc911x to use ACPI as well as DT) makes
 the call to smsc911x_probe_config() unconditional, and no longer fails if
 there is no device node. device_get_phy_mode() is called unconditionally,
 and if there is no phy node configured returns an error code. This error
 code is assigned to phy_interface, and interpreted elsewhere in the code
 as valid phy mode. This in turn causes qemu to crash when running a
 variant of realview_pb_defconfig.
 
 qemu: hardware error: lan9118_read: Bad reg 0x86
 
 Fixes: 0b50dc4fc971 (Convert smsc911x to use ACPI as well as DT)
 Cc: Jeremy Linton jeremy.lin...@arm.com
 Cc Graeme Gregory graeme.greg...@linaro.org
 Signed-off-by: Guenter Roeck li...@roeck-us.net
 ---
   drivers/net/ethernet/smsc/smsc911x.c | 7 ++-
   1 file changed, 6 insertions(+), 1 deletion(-)
 
 diff --git a/drivers/net/ethernet/smsc/smsc911x.c 
 b/drivers/net/ethernet/smsc/smsc911x.c
 index 0f21aa3bb537..34f97684506b 100644
 --- a/drivers/net/ethernet/smsc/smsc911x.c
 +++ b/drivers/net/ethernet/smsc/smsc911x.c
 @@ -2367,12 +2367,17 @@ static const struct smsc911x_ops 
 shifted_smsc911x_ops = {
   static int smsc911x_probe_config(struct smsc911x_platform_config *config,
  struct device *dev)
   {
 +   int phy_interface;
 u32 width = 0;
 
 if (!dev)
 return -ENODEV;
 
 -   config-phy_interface = device_get_phy_mode(dev);
 +   phy_interface = device_get_phy_mode(dev);
 +   if (phy_interface  0)
 +   return phy_interface;
 +
 +   config-phy_interface = phy_interface;
 
 device_get_mac_address(dev, config-mac, ETH_ALEN);
 
 Looks like this change makes at least omap boards using smsc911x
 fail with -22 for me in Linux next.
 
 
 What do you see if you revert my patch ? It should assign -22, or its
 unsigned representation, to phy_interface, which isn't such a good idea
 either.

If I revert patch smsc911x: Fix crash seen if neither ACPI nor OF is
configured or used things work as just assign config-phy_interface
directly without returning early. It's -22 in that case also.
 
 Do any of the the device tree configured smsc911x devices actually
 have a phy configured?
 
 Good question, and beats me. Looking into the original code,
 it didn't check for an error return from of_get_phy_mode() either,
 and thus _would_ dutifully assign the error code to phy_interface.
 Wonder how was this supposed to work to start with.
 
 I'll do some debugging and try to find out what exactly is going on.

Looks like adding smsc,force-internal-phy to omap-gpmc-smsc9221.dtsi
does not help. Somehow the default behavior is now different.

Regards,

Tony
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH -next v2 2/2] smsc911x: Ignore error return from device_get_phy_mode()

[Tony Lindgren]

* Guenter Roeck li...@roeck-us.net [150826 13:24]:
 Commit 62ee783bf1f8 (smsc911x: Fix crash seen if neither ACPI nor OF is
 configured or used) introduces an error check for the return value from
 device_get_phy_mode() and bails out if there is an error. Unfortunately,
 there are configurations where no phy is configured. Those configurations
 now fail.
 
 To fix the problem, accept error returns from device_get_phy_mode(),
 and use the return value from device_property_read_u32() to determine
 if there is a suitable firmware interface to read the configuration.
 
 Fixes: 62ee783bf1f8 (smsc911x: Fix crash seen if neither ACPI nor OF is 
 configured or used)
 Signed-off-by: Guenter Roeck li...@roeck-us.net
 ---
 v2: Dropped RFC
 Removed check for -ENODATA
 Depends on patch 1/2
 
 Tested with non-devicetree configuration. Should be tested with ACPI
 and FDT configurations.

Thanks this fixes smsc911x regression in Linux next for me with FDT:

Tested-by: Tony Lindgren t...@atomide.com
 
  drivers/net/ethernet/smsc/smsc911x.c | 14 --
  1 file changed, 8 insertions(+), 6 deletions(-)
 
 diff --git a/drivers/net/ethernet/smsc/smsc911x.c 
 b/drivers/net/ethernet/smsc/smsc911x.c
 index 6eef3251d833..c8b26259c9cf 100644
 --- a/drivers/net/ethernet/smsc/smsc911x.c
 +++ b/drivers/net/ethernet/smsc/smsc911x.c
 @@ -2369,23 +2369,25 @@ static int smsc911x_probe_config(struct 
 smsc911x_platform_config *config,
  {
   int phy_interface;
   u32 width = 0;
 + int err;
  
   phy_interface = device_get_phy_mode(dev);
   if (phy_interface  0)
 - return phy_interface;
 -
 + phy_interface = PHY_INTERFACE_MODE_NA;
   config-phy_interface = phy_interface;
  
   device_get_mac_address(dev, config-mac, ETH_ALEN);
  
 - device_property_read_u32(dev, reg-shift, config-shift);
 -
 - device_property_read_u32(dev, reg-io-width, width);
 - if (width == 4)
 + err = device_property_read_u32(dev, reg-io-width, width);
 + if (err == -ENXIO)
 + return err;
 + if (!err  width == 4)
   config-flags |= SMSC911X_USE_32BIT;
   else
   config-flags |= SMSC911X_USE_16BIT;
  
 + device_property_read_u32(dev, reg-shift, config-shift);
 +
   if (device_property_present(dev, smsc,irq-active-high))
   config-irq_polarity = SMSC911X_IRQ_POLARITY_ACTIVE_HIGH;
  
 -- 
 2.1.4
 
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH -next] smsc911x: Fix crash seen if neither ACPI nor OF is configured or used

[Tony Lindgren]

* Guenter Roeck li...@roeck-us.net [150826 13:58]:
 Hi Tony,
 
 On 08/26/2015 01:16 PM, Tony Lindgren wrote:
 [ ... ]
 
 We may need two separate patches, one to fix up device_property_read_u32()
 to return -ENXIO, and one to fix smsc911x_probe_config() to ignore the error
 from device_get_phy_mode(), and to bail out if device_property_read_u32()
 returns -ENXIO.
 
 I guess the device_property_read_u32() change needs to be discussed
 separately.. So probably best to fix up the regression to smsc911x
 first.
 
 Not sure myself. Jeremy has a point - we don't really know for sure how
 safe it is to check for -ENODATA (in addition to -ENXIO). Also, fixing
 device_property_read_u32() turned out to be much easier than I thought.
 
 The simpler alternative would be to check the return value from
 device_property_read_u32() for both -ENXIO and -ENODATA.
 This would make the code independent of the necessary core changes
 (which may take a while). I tested this variant, and it works, at least
 for the non-DT case.
 
 Does this make sense ?
 
 Yeh I think that would allow fixing up the smsc911x regression while
 discussing the device_property_read_u32() change. Got a test patch
 for me to try?
 
 
 You should have two by now to choose from.

Acked the second version thanks :)

Tony
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [net-next PATCH 2/3] ARM: dts: dra7: update cpsw compatible

[Tony Lindgren]

* Mugunthan V N mugunthan...@ti.com [150812 02:56]:
 CPSW driver has been updated with compatibles for enabling errata
 workarounds. So updating cpsw compatibles.
 
 Signed-off-by: Mugunthan V N mugunthan...@ti.com

Please feel free to merge this one via net tree once the changes
are reviewed:

Acked-by: Tony Lindgren t...@atomide.com

 ---
  arch/arm/boot/dts/dra7.dtsi | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
 index 8f1e25b..b4fdd10 100644
 --- a/arch/arm/boot/dts/dra7.dtsi
 +++ b/arch/arm/boot/dts/dra7.dtsi
 @@ -1398,7 +1398,7 @@
   };
  
   mac: ethernet@4a10 {
 - compatible = ti,cpsw;
 + compatible = ti,dra7-cpsw,ti,cpsw;
   ti,hwmods = gmac;
   clocks = dpll_gmac_ck, gmac_gmii_ref_clk_div;
   clock-names = fck, cpts;
 -- 
 2.5.0.234.gefc8a62
 
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [net-next PATCH 3/3] ARM: dts: am33xx: update cpsw compatible

[Tony Lindgren]

* Mugunthan V N mugunthan...@ti.com [150812 02:56]:
 CPSW driver has been updated with compatibles for enabling errata
 workarounds. So updating cpsw compatibles.
 
 Signed-off-by: Mugunthan V N mugunthan...@ti.com

This too:

Acked-by: Tony Lindgren t...@atomide.com

 ---
  arch/arm/boot/dts/am33xx.dtsi | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
 index 21fcc44..8b59c86 100644
 --- a/arch/arm/boot/dts/am33xx.dtsi
 +++ b/arch/arm/boot/dts/am33xx.dtsi
 @@ -700,7 +700,7 @@
   };
  
   mac: ethernet@4a10 {
 - compatible = ti,cpsw;
 + compatible = ti,am335x-cpsw,ti,cpsw;
   ti,hwmods = cpgmac0;
   clocks = cpsw_125mhz_gclk, cpsw_cpts_rft_clk;
   clock-names = fck, cpts;
 -- 
 2.5.0.234.gefc8a62
 
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html