Re: [PATCH net-next v2 0/2] of: mdio: Fall back to mdiobus_register() with NULL device_node

[Andrew Lunn]

On Wed, May 16, 2018 at 10:54:12AM +0200, Geert Uytterhoeven wrote:
> Hi Florian,
> 
> Thanks for your series!
> I like the effect on simplifying drivers.
> 
> On Wed, May 16, 2018 at 1:56 AM, Florian Fainelli  
> wrote:
> > This patch series updates of_mdiobus_register() such that when the 
> > device_node
> > argument is NULL, it calls mdiobus_register() directly. This is consistent 
> > with
> > the behavior of of_mdiobus_register() when CONFIG_OF=n.
> 
> IMHO the CONFIG_OF=n behavior of of_mdiobus_register() (which I wasn't
> aware of) is inconsistent with the behavior of other of_*() functions,
> which are just empty stubs.
> 
> So I'm wondering if you should do it the other way around, and let
> mdiobus_register() call of_mdiobus_register() if dev->of_node exists?

Hi Geert

dev->of_node is often not the correct OF node. The mdio properties are
often embedded inside a MAC driver, and use an 'mdio' container
node. This container node is needed, not the device node.

> I haven't looked at the ACPI handling, but perhaps this can be moved
> inside mdiobus_register() as well?

The ACPI binding for MDIO and PHYs has not been defined yet.

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

Re: [PATCH v3 3/3] dt-bindings: Document the DT bindings for lan78xx

[Andrew Lunn]

On Thu, Apr 19, 2018 at 03:32:05PM +0100, Phil Elwell wrote:
> The Microchip LAN78XX family of devices are Ethernet controllers with
> a USB interface. Despite being discoverable devices it can be useful to
> be able to configure them from Device Tree, particularly in low-cost
> applications without an EEPROM or programmed OTP.
> 
> Document the supported properties in a bindings file.
> 
> Signed-off-by: Phil Elwell <p...@raspberrypi.org>

Reviewed-by: Andrew Lunn <and...@lunn.ch>

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

Re: [PATCH v3 2/3] lan78xx: Read LED states from Device Tree

[Andrew Lunn]

> @@ -2077,6 +2085,28 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
>   mii_adv = (u32)mii_advertise_flowctrl(dev->fc_request_control);
>   phydev->advertising |= mii_adv_to_ethtool_adv_t(mii_adv);
>  
> + if (phydev->mdio.dev.of_node) {
> + u32 reg;
> + int len;
> +
> + len = of_property_count_elems_of_size(phydev->mdio.dev.of_node,
> +   "microchip,led-modes",
> +   sizeof(u32));
> + if (len >= 0) {
> + /* Ensure the appropriate LEDs are enabled */
> + lan78xx_read_reg(dev, HW_CFG, );
> + reg &= ~(HW_CFG_LED0_EN_ |
> +  HW_CFG_LED1_EN_ |
> +  HW_CFG_LED2_EN_ |
> +  HW_CFG_LED3_EN_);
> + reg |= (len > 0) * HW_CFG_LED0_EN_ |
> + (len > 1) * HW_CFG_LED1_EN_ |
> + (len > 2) * HW_CFG_LED2_EN_ |
> + (len > 3) * HW_CFG_LED3_EN_;
> + lan78xx_write_reg(dev, HW_CFG, reg);
> + }
> + }
> +

Humm. Not nice. But i cannot think of a cleaner way of doing this.

Reviewed-by: Andrew Lunn <and...@lunn.ch>

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

Re: [PATCH v2 2/3] lan78xx: Read LED states from Device Tree

[Andrew Lunn]

On Wed, Apr 18, 2018 at 04:45:22PM +0100, Phil Elwell wrote:
> Add support for DT property "microchip,led-modes", a vector of zero
> to four cells (u32s) in the range 0-15, each of which sets the mode
> for one of the LEDs. Some possible values are:
> 
> 0=link/activity  1=link1000/activity
> 2=link100/activity   3=link10/activity
> 4=link100/1000/activity  5=link10/1000/activity
> 6=link10/100/activity14=off15=on
> 
> These values are given symbolic constants in a dt-bindings header.
> 
> Also use the presence of the DT property to indicate that the
> LEDs should be enabled - necessary in the event that no valid OTP
> or EEPROM is available.

Hi Phil

As i said last week, these are PHY properties, so should be in the PHY
node in device tree. It should be the PHY driver which parses these
properties and configures the LEDs, not the MAC.

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

Re: [PATCH 3/4] lan78xx: Read LED modes from Device Tree

[Andrew Lunn]

> @@ -2097,6 +2098,25 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
>   (void)lan78xx_set_eee(dev->net, );
>   }
>  
> + if (!of_property_read_u32_array(dev->udev->dev.of_node,
> + "microchip,led-modes",
> + led_modes, ARRAY_SIZE(led_modes))) {
> + u32 reg;
> + int i;
> +
> + reg = phy_read(phydev, 0x1d);
> + for (i = 0; i < ARRAY_SIZE(led_modes); i++) {
> + reg &= ~(0xf << (i * 4));
> + reg |= (led_modes[i] & 0xf) << (i * 4);
> + }
> + (void)phy_write(phydev, 0x1d, reg);

Poking PHY registers directly from the MAC driver is not always a good
idea. This MAC driver does that in a few places :-(

What do we know about the PHY? It is built into the device or is it
external? If it is external, how do you know the LED register is at
0x1d?

The safest place to do this is in the PHY driver, and place these OF
properties into the PHY node.

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

Re: [PATCH 4/4] dt-bindings: Document the DT bindings for lan78xx

[Andrew Lunn]

On Thu, Apr 12, 2018 at 02:55:36PM +0100, Phil Elwell wrote:
> The Microchip LAN78XX family of devices are Ethernet controllers with
> a USB interface. Despite being discoverable devices it can be useful to
> be able to configure them from Device Tree, particularly in low-cost
> applications without an EEPROM or programmed OTP.

It would be good to document what happens when there is an EEPROM. Is
OF used in preference to the EEPROM?
   
Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] lan78xx: Read LED modes from Device Tree

[Andrew Lunn]

On Thu, Apr 12, 2018 at 02:55:35PM +0100, Phil Elwell wrote:
> Add support for DT property "microchip,led-modes", a vector of two
> cells (u32s) in the range 0-15, each of which sets the mode for one
> of the two LEDs. Some possible values are:
> 
> 0=link/activity  1=link1000/activity
> 2=link100/activity   3=link10/activity
> 4=link100/1000/activity  5=link10/1000/activity
> 6=link10/100/activity14=off15=on
> 
> Also use the presence of the DT property to indicate that the
> LEDs should be enabled - necessary in the event that no valid OTP
> or EEPROM is available.

I'm not a fan of this, but at the moment, we don't have anything
better.

Please follow what mscc does, add a header file for the LED settings.

   Andrew

> 
> Signed-off-by: Phil Elwell 
> ---
>  drivers/net/usb/lan78xx.c | 20 
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index d98397b..ffb483d 100644
> --- a/drivers/net/usb/lan78xx.c
> +++ b/drivers/net/usb/lan78xx.c
> @@ -2008,6 +2008,7 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
>  {
>   int ret;
>   u32 mii_adv;
> + u32 led_modes[2];
>   struct phy_device *phydev;
>  
>   phydev = phy_find_first(dev->mdiobus);
> @@ -2097,6 +2098,25 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
>   (void)lan78xx_set_eee(dev->net, );
>   }
>  
> + if (!of_property_read_u32_array(dev->udev->dev.of_node,
> + "microchip,led-modes",
> + led_modes, ARRAY_SIZE(led_modes))) {
> + u32 reg;
> + int i;
> +
> + reg = phy_read(phydev, 0x1d);
> + for (i = 0; i < ARRAY_SIZE(led_modes); i++) {
> + reg &= ~(0xf << (i * 4));
> + reg |= (led_modes[i] & 0xf) << (i * 4);
> + }

Please add range checks for led_modes[i] and return -EINVAL if the
check fails.

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

Re: [PATCH 4/4] dt-bindings: Document the DT bindings for lan78xx

[Andrew Lunn]

On Thu, Apr 12, 2018 at 03:10:57PM +0100, Phil Elwell wrote:
> Hi Andrew,
> 
> On 12/04/2018 15:04, Andrew Lunn wrote:
> > On Thu, Apr 12, 2018 at 02:55:36PM +0100, Phil Elwell wrote:
> >> The Microchip LAN78XX family of devices are Ethernet controllers with
> >> a USB interface. Despite being discoverable devices it can be useful to
> >> be able to configure them from Device Tree, particularly in low-cost
> >> applications without an EEPROM or programmed OTP.
> >>
> >> Document the supported properties in a bindings file, adding it to
> >> MAINTAINERS at the same time.
> > 
> > Hi Phil
> > 
> > How you link an OF node to a USB device is not obvious. Could you
> > please include either a pointer to some binding documentation, or make
> > your example show it.
> 
> Thanks for the feedback. Would you consider this (lifted from the Pi 3B+ 
> Device Tree)
> a sufficient example?

Yes, this is good.

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

Re: [PATCH 2/4] lan78xx: Read initial EEE setting from Device Tree

[Andrew Lunn]

On Thu, Apr 12, 2018 at 02:55:34PM +0100, Phil Elwell wrote:
> Add two new Device Tree properties:
> * microchip,eee-enabled  - a boolean to enable EEE
> * microchip,tx-lpi-timer - time in microseconds to wait after TX goes
>idle before entering the low power state
>(default 600)

Hi Phil

This looks wrong.

What should happen is that the MAC driver calls phy_init_eee() to find
out if the PHY supports EEE. There should be no need to look in device
tree.

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

Re: [PATCH 1/4] lan78xx: Read MAC address from DT if present

[Andrew Lunn]

Hi Phil

> - ret = lan78xx_write_reg(dev, RX_ADDRL, addr_lo);
> - ret = lan78xx_write_reg(dev, RX_ADDRH, addr_hi);
> + mac_addr = of_get_mac_address(dev->udev->dev.of_node);

It might be better to use the higher level eth_platform_get_mac_address().

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

Re: [PATCH 4/4] dt-bindings: Document the DT bindings for lan78xx

[Andrew Lunn]

On Thu, Apr 12, 2018 at 02:55:36PM +0100, Phil Elwell wrote:
> The Microchip LAN78XX family of devices are Ethernet controllers with
> a USB interface. Despite being discoverable devices it can be useful to
> be able to configure them from Device Tree, particularly in low-cost
> applications without an EEPROM or programmed OTP.
> 
> Document the supported properties in a bindings file, adding it to
> MAINTAINERS at the same time.

Hi Phil

How you link an OF node to a USB device is not obvious. Could you
please include either a pointer to some binding documentation, or make
your example show it.

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

Re: [PATCH] lan78xx: Correctly indicate invalid OTP

[Andrew Lunn]

On Wed, Apr 11, 2018 at 10:59:17AM +0100, Phil Elwell wrote:
> lan78xx_read_otp tries to return -EINVAL in the event of invalid OTP
> content, but the value gets overwritten before it is returned and the
> read goes ahead anyway. Make the read conditional as it should be
> and preserve the error code.

Hi Phil

Do you know that the Fixes: tag should be for this? When did it break?

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

Re: [Question] MFD driver that handles clocks/resets and populates child nodes

[Andrew Lunn]

On Mon, Apr 02, 2018 at 10:21:01PM +0900, Masahiro Yamada wrote:
> 2018-04-02 21:04 GMT+09:00 Andrew Lunn <and...@lunn.ch>:
> >> The maintainer of DWC3, Felipe Balbi, requested to
> >> split the glue layer driver into small parts such as
> >> reset, regulator, phy, etc.
> >
> > What exactly did Felipe ask for? Did he ask that the patch be split
> > up, one patch per reset, regulator, phy etc?
> 
> 
> Yeah.  That is what we understood from his comments.
> 
> 
> These are the feed-backs from him.
> 
> https://lkml.org/lkml/2018/1/23/298
> https://lkml.org/lkml/2018/1/24/352

> > Are all these resources used just by the DWC3? Or is it a true MFD,
> > multiple functions?
> 
> I do not think this is a real MFD.
> 
> This is a DWC3 glue layer, i.e.
> a collection of misc registers that control
> the DWC3 IP.
> 
> 
> Just splitting it into small pieces
> to use PHY, reset, regulator framework in Linux.
> 
> Of course, the price of this approach
> is so cluttered Device Tree,
> honestly I do not like it much.

This however the correct way to do this. You should have a phy driver,
and a regulator driver, and a reset driver. The DWC3 then uses
phandles to these drivers.

How is the IO map area 65b0 split up. Can you cleanly separate it
into sub areas, which do not overlap, so you have a sub-area for the
PHY driver, a sub-area for the regulator driver and a sub-area for the
reset area? If you can cleanly split it up, you don't need an MFD. If
however the registers are in overlapping areas, you do need an
MFD. The MFD core provides access to the registers, while its children
implement PHY, reset, regulator etc.

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

Re: [Question] MFD driver that handles clocks/resets and populates child nodes

[Andrew Lunn]

> The maintainer of DWC3, Felipe Balbi, requested to
> split the glue layer driver into small parts such as
> reset, regulator, phy, etc.

What exactly did Felipe ask for? Did he ask that the patch be split
up, one patch per reset, regulator, phy etc?

Are all these resources used just by the DWC3? Or is it a true MFD,
multiple functions?

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

Re: [PATCH] lan78xx: Connect phy early

[Andrew Lunn]

> @@ -2082,8 +2082,6 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
>  
>   dev->fc_autoneg = phydev->autoneg;
>  
> - phy_start(phydev);
> -
>   netif_dbg(dev, ifup, dev->net, "phy initialised successfully");
>  
>   return 0;
> @@ -2512,9 +2510,7 @@ static int lan78xx_open(struct net_device *net)
>   if (ret < 0)
>   goto done;
>  
> - ret = lan78xx_phy_init(dev);
> - if (ret < 0)
> - goto done;
> + phy_start(net->phydev);

Should the debug message be moved as well?

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

Re: [PATCH 0/2] net/usb/ax88179_178a: Adjustments for ax88179_chk_eee()

[Andrew Lunn]

On Sat, Mar 10, 2018 at 07:22:55PM +0100, SF Markus Elfring wrote:
> From: Markus Elfring 
> Date: Sat, 10 Mar 2018 19:05:45 +0100
> 
> Two update suggestions were taken into account
> from static source code analysis.

Hi Markus

How about re-writing this driver to use phylib. The whole of
ax88179_chk_eee() would then go away, since it is a duplication of
phy_init_eee().

Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" 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/{mii,smsc}: Make mii_ethtool_get_link_ksettings and smc_netdev_get_ecmd return void

[Andrew Lunn]

> diff --git a/drivers/net/cris/eth_v10.c b/drivers/net/cris/eth_v10.c
> index da02041..017f48c 100644
> --- a/drivers/net/cris/eth_v10.c
> +++ b/drivers/net/cris/eth_v10.c
> @@ -1417,10 +1417,9 @@ static int e100_get_link_ksettings(struct net_device 
> *dev,
>  {
>   struct net_local *np = netdev_priv(dev);
>   u32 supported;
> - int err;
>  
>   spin_lock_irq(>lock);
> - err = mii_ethtool_get_link_ksettings(>mii_if, cmd);
> + mii_ethtool_get_link_ksettings(>mii_if, cmd);
>   spin_unlock_irq(>lock);
>  
>   /* The PHY may support 1000baseT, but the Etrax100 does not.  */
> @@ -1432,7 +1431,7 @@ static int e100_get_link_ksettings(struct net_device 
> *dev,
>   ethtool_convert_legacy_u32_to_link_mode(cmd->link_modes.supported,
>   supported);
>  
> - return err;
> + return 0;
>  }

How far are going planning on going? It seems like
*_get_link_ksettings() now all return a useless 0. Do you plan to
change ethtool_ops and make if void all the way up?

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

Re: [PATCH v3] smsc95xx: Add comments to the registers definition

[Andrew Lunn]

On Wed, Apr 12, 2017 at 11:24:05AM +0200, Martin Wetterwald wrote:
> This chip is used by a lot of embedded devices and also by the Raspberry
> Pi 1, 2 & 3 which were created to promote the study of computer
> sciences. Students wanting to learn kernel / network device driver
> programming through those devices can only rely on the Linux kernel
> driver source to make their own.
> 
> This commit adds a lot of comments to the registers definition to expand
> the register names.
> 
> Cc: Steve Glendinning <steve.glendinn...@shawell.net>
> Cc: Microchip Linux Driver Support <unglinuxdri...@microchip.com>
> CC: David Miller <da...@davemloft.net>
> Signed-off-by: Martin Wetterwald <mar...@wetterwald.eu>

Reviewed-by: Andrew Lunn <and...@lunn.ch>

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

Re: [PATCH] smsc95xx: Add comments to the registers definition

[Andrew Lunn]

Hi Martin

> @@ -2032,7 +2032,7 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet 
> *dev,
>   skb_push(skb, 4);
>   tx_cmd_b = (u32)(skb->len - 4);
>   if (csum)
> - tx_cmd_b |= TX_CMD_B_CSUM_ENABLE;
> + tx_cmd_b |= TX_CMD_B_CSUM_EN;

This changed seems a step backwards, ENABLE is much more readable than EN.

>  
> -#define TX_CMD_B_CSUM_ENABLE (0x4000)
> -#define TX_CMD_B_ADD_CRC_DISABLE_(0x2000)
> -#define TX_CMD_B_DISABLE_PADDING_(0x1000)
> -#define TX_CMD_B_PKT_BYTE_LENGTH_(0x07FF)
> +#define TX_CMD_B_CSUM_EN (0x4000)/* TX Checksum Enable */

And there is space for ABLE here.

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

Re: [PATCH RFC 0/7] phylib MMD accessor cleanups

[Andrew Lunn]

> Thanks.  When I posted this last time around (19th Jan) I mentioned
> about marking the old _indirect() accessors with __deprecated - is
> that still something we want to do?
> 
> I haven't tested this against net-next yet, so I don't know if there
> are any new users of the indirect accessors - going down the deprecated
> route would avoid breakage, but means having to submit a patch later to
> actually remove them.
> 
> How would people want this handled?

Hi Russell

We can get patches into net-next very quickly. So i suggest you rebase
and resubmit and get it in. If something breaks, we add followup
patches to fix it.

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

Re: [PATCH RFC 3/7] net: lan78xx: update for phy_(read|write)_mmd_indirect() removal

[Andrew Lunn]

On Sun, Mar 19, 2017 at 11:00:29AM +, Russell King wrote:
> lan78xx appears to use phylib in a rather weird way, accessing the PHY
> partly through phylib, and partly by makign direct accesses to it,

Hi Russell

s/makign/making

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

Re: [PATCH RFC 0/7] phylib MMD accessor cleanups

[Andrew Lunn]

On Sun, Mar 19, 2017 at 10:59:44AM +, Russell King - ARM Linux wrote:
> Hi,
> 
> This series cleans up the phylib MMD accessors.  We have two accessors
> at present, phy_(read|write)_mmd() and phy_(read|write)_mmd_indirect()
> 
> The _indirect methods access the MMD registers via a clause 22 phy,
> whereas the non-_indirect methods acess via clause 45 accesses.
> 
> Current PHY-independent parts of phylib (such as EEE) access the MMD
> registers via the _indirect methods, which is no good if you have a
> clause 45 PHY that doesn't respond to clause 22 accesses.
> 
> In order to make these features available, we need to rework these
> accessors such that they can access the MMD registers using a method
> dependent on the clause that the PHY conforms with.
> 
> This series of patches does exactly that - we merge the functionality
> of the indirect accesses into the clause 45 accessors, and use these
> exclusively to access MMD registers.  Internally, the new clause
> independent MMD accessors indirect via the PHY drivers read_mmd/write_mmd
> methods if present, otherwise fall back to using clause 45 accesses if
> the PHY is a clause 45 phy, or clause 22 indirect accesses if the PHY
> is clause 22.
> 
> Note: confusingly, phy_read_mmd_indirect() vs phy_read_mmd() switches
> the order of prtad and devad, which means that converting between the
> two is not a simple matter of just replacing the function name.  Same
> applies for the write methods.

Hi Russell

This all looks good, apart from the one typo i spotted.

Reviewed-by: Andrew Lunn <and...@lunn.ch>

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

Re: [PATCH v2 0/3] Add EHCI support for Armada 37xx

[Andrew Lunn]

On Thu, Mar 09, 2017 at 12:04:21PM +0100, Gregory CLEMENT wrote:
> Hi,
> 
> The EHCI controller in the Armada 37xx SoCs is the one used on many
> other mvebu SoCs such as the orion5x, the kirkwood, or the
> armada. However, for Armada 37xx an extra initialization step is
> needed: this is the purpose of the first patch.
> 
> The second patch allows to build the driver for the ARM64 Armada SoCs.
> 
> The last one enables the EHCI in the device tree.
> 
> This second version takes into account the review from Andrew Lunn and
> Thomas Petazzoni.

Hi Gregory

Thanks for making the changes.

Reviewed-by: Andrew Lunn <and...@lunn.ch>

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

Re: [PATCH 2/3] usb: host: Allow to build ehci orion with mvebu SoCs

[Andrew Lunn]

On Wed, Mar 08, 2017 at 05:24:22PM +0100, Gregory CLEMENT wrote:
> The mvebu ARM64 SoCs no more select PLAT_ORION but some of them as the
> Armada 37xx use the EHCI orion controller. This patch allow to build
> the driver when ARCH_MVEBU is selected.

The mvebu ARM64 SoCs no longer selects PLAT_ORION.  However Armada
37xx use the Orion EHCI controller. This patch allows the Orion EHCI
driver to be built when ARCH_MVEBU is selected.

> Signed-off-by: Gregory CLEMENT 
> ---
>  drivers/usb/host/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> index 407d947b34ea..870c42d89298 100644
> --- a/drivers/usb/host/Kconfig
> +++ b/drivers/usb/host/Kconfig
> @@ -188,7 +188,7 @@ config USB_EHCI_HCD_OMAP
>  
>  config USB_EHCI_HCD_ORION
>   tristate  "Support for Marvell EBU on-chip EHCI USB controller"
> - depends on USB_EHCI_HCD && PLAT_ORION
> + depends on USB_EHCI_HCD && PLAT_ORION || ARCH_MVEBU

I don't know the Kconfig language too well. Should this be:

> + depends on USB_EHCI_HCD && (PLAT_ORION || ARCH_MVEBU)

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

Re: [PATCH 1/3] usb: orion-echi: Add support for the Armada 3700

[Andrew Lunn]

Hi Gregory

> - Add a new compatoble string for the Armada 3700 SoCs

compatible

> 
> - add sbuscfg support for orion usb controller driver. For the SoCs
>   without hlock, need to program BAWR/BARD/AHBBRST fields in the sbuscfg
>   register to guarantee the AHB master's burst would not overrun or
>   underrun the FIFO.
> 
> - the sbuscfg register has to be set after the usb controller reset,
>   otherwise the value would be overridden to 0. In order to do this, the
>   reset callback is registered.
> 
> [gregory.clem...@free-electrons.com: - reword commit and comments
>- fix checkpatch warning]
> Signed-off-by: jinghua 
> Signed-off-by: Gregory CLEMENT 
> ---
>  .../devicetree/bindings/usb/ehci-orion.txt |  4 ++-
>  drivers/usb/host/ehci-orion.c  | 39 
> ++
>  2 files changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/usb/ehci-orion.txt 
> b/Documentation/devicetree/bindings/usb/ehci-orion.txt
> index 17c3bc858b86..9dfffc9dffec 100644
> --- a/Documentation/devicetree/bindings/usb/ehci-orion.txt
> +++ b/Documentation/devicetree/bindings/usb/ehci-orion.txt
> @@ -1,7 +1,9 @@
>  * EHCI controller, Orion Marvell variants
>  
>  Required properties:
> -- compatible: must be "marvell,orion-ehci"
> +- compatible: could be one of the following

must, not could.

> + "marvell,orion-ehci"
> + "marvell,armada-3700-ehci"
>  - reg: physical base address of the controller and length of memory mapped
>region.
>  - interrupts: The EHCI interrupt
> diff --git a/drivers/usb/host/ehci-orion.c b/drivers/usb/host/ehci-orion.c
> index ee8d5faa0194..cf778e166b90 100644
> --- a/drivers/usb/host/ehci-orion.c
> +++ b/drivers/usb/host/ehci-orion.c
> @@ -47,6 +47,21 @@
>  #define USB_PHY_IVREF_CTRL   0x440
>  #define USB_PHY_TST_GRP_CTRL 0x450
>  
> +#define USB_SBUSCFG  0x90
> +#define  USB_SBUSCFG_BAWR0x6
> +#define  USB_SBUSCFG_BARD0x3
> +#define  USB_SBUSCFG_AHBBRST 0x0

These three are all shifts. So i would suggest adding _SHIFT to the
end.

> +
> +/* BAWR = BARD = 3 : Align read/write bursts packets larger than 128 bytes */
> +#define USB_SBUSCFG_BAWR_ALIGN_128B  0x3
> +#define USB_SBUSCFG_BARD_ALIGN_128B  0x3
> +/* AHBBRST = 3  : Align AHB Burst to INCR16 (64 bytes) */
> +#define USB_SBUSCFG_AHBBRST_INCR16   0x3

You can then apply the shift here.

> +
> +#define USB_SBUSCFG_DEF_VAL ((USB_SBUSCFG_BAWR_ALIGN_128B << 
> USB_SBUSCFG_BAWR) \
> +  | (USB_SBUSCFG_BARD_ALIGN_128B << USB_SBUSCFG_BARD) \
> +  | (USB_SBUSCFG_AHBBRST_INCR16 << USB_SBUSCFG_AHBBRST))

and this is then shorted.

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

Re: [PATCH v6] r8152: Add support for setting pass through MAC address on RTL8153-AD

[Andrew Lunn]

> > It is same, how to handle two network cards which tell us, that they
> > have same MAC addresses.
> > 
> 
> The kernel handles this just fine.  In doing this patch I checked to see
> what it does in that scenario.  Two devices are made.  systemd doesn't
> rename the second device via the MAC name (eg enxAABBCCDDEEFF).

What does you dhcp server do? Does it gives out the same IP address?
You then have two interfaces on the same network, with the same MAC
address and IP address. Then what happens?

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

Re: [PATCH v6] r8152: Add support for setting pass through MAC address on RTL8153-AD

[Andrew Lunn]

On Fri, Jun 10, 2016 at 10:51:56PM -0700, David Miller wrote:
> From: Mario Limonciello <mario_limoncie...@dell.com>
> Date: Tue,  7 Jun 2016 13:22:37 -0500
> 
> > The RTL8153-AD supports a persistent system specific MAC address.
> > This means a device plugged into two different systems with host side
> > support will show different (but persistent) MAC addresses.
> > 
> > This information for the system's persistent MAC address is burned in when
> > the system HW is built and available under \_SB.AMAC in the DSDT at runtime.
> > 
> > This technology is currently implemented in the Dell TB15 and WD15 Type-C
> > docks.  More information is available here:
> > http://www.dell.com/support/article/us/en/04/SLN301147
> > 
> > Signed-off-by: Mario Limonciello <mario_limoncie...@dell.com>
> > ---
> > Changes from v5:
> >  * Correct return value if hex2bin succesful but invalid ether addr
> 
> Have things calmed down enough now that I can apply this?

Hi David

I think the code has reaching the level of maturity needed for
acceptance. So for the code quality:

Reviewed-by: Andrew Lunn <and...@lunn.ch>

What is still open is do we want to accept it at all? Do we accept the
concept of putting the same MAC address on multiple interfaces at
hotplug time? Do we trust BIOS vendors to not keep changing DSDT
property name, since it is not standardised?

Do we want this at all should be decided by somebody more senior then
those passing comments on the code.

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

Re: [PATCH v3] r8152: Add support for setting pass through MAC address on RTL8153-AD

[Andrew Lunn]

> + /* returns _AUXMAC_#AABBCCDDEEFF# */
> + status = acpi_evaluate_object(NULL, "\\_SB.AMAC", NULL, );
> + obj = (union acpi_object *)buffer.pointer;
> + if (ACPI_SUCCESS(status)) {
> + if (obj->type != ACPI_TYPE_BUFFER ||
> + obj->string.length != 0x17) {
> + pr_warn("r8152: get_passthru_addr: Invalid buffer");

Please don't use pr_warn() when you can use the better alternatives
like dev_warn().

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

Re: [PATCH v2] r8152: Add support for setting MAC to system's Auxiliary MAC address

[Andrew Lunn]

On Thu, Jun 02, 2016 at 07:04:32PM +, mario_limoncie...@dell.com wrote:
> > -Original Message-
> > From: Andrew Lunn [mailto:and...@lunn.ch]
> > Sent: Thursday, June 2, 2016 2:03 PM
> > To: Limonciello, Mario <mario_limoncie...@dell.com>
> > Cc: gre...@linuxfoundation.org; hayesw...@realtek.com; linux-
> > ker...@vger.kernel.org; net...@vger.kernel.org; linux-
> > u...@vger.kernel.org; pali.ro...@gmail.com; anthony.w...@canonical.com
> > Subject: Re: [PATCH v2] r8152: Add support for setting MAC to system's
> > Auxiliary MAC address
> > 
> > > > And you want to check this for all Dell devices?  Please be model
> > > > specific, I doubt a bunch of Dell servers wants to run this code...
> > > >
> > >
> > > Tracking model specific is really going to turn into a giant list never 
> > > ending
> > list.
> > > To drill down more specifically, I can match on chassis too.
> > 
> > Does Dell happen to use its own USB Vendor ID for the USB device in
> > the dock? You could go at this problem from the other direction if it
> > does have a unique vendor ID.
> > 
> >  Andrew
> 
> Unfortunately it's not a Dell specific VID/PID.  I'm asking around to find out
> if there is something else identifiable about this dock's NIC (maybe that 
> r8152 
> can query).

lsusb -v

I assume there is a USB hub in the dock, maybe that has a Dell VID?
Going one level up the USB tree hierarchy should not be too hard.

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

Re: [PATCH v2] r8152: Add support for setting MAC to system's Auxiliary MAC address

[Andrew Lunn]

> > And you want to check this for all Dell devices?  Please be model
> > specific, I doubt a bunch of Dell servers wants to run this code...
> > 
> 
> Tracking model specific is really going to turn into a giant list never 
> ending list.
> To drill down more specifically, I can match on chassis too.

Does Dell happen to use its own USB Vendor ID for the USB device in
the dock? You could go at this problem from the other direction if it
does have a unique vendor ID.

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

Re: [PATCH] r8152: Add support for setting MAC to system's Auxiliary MAC address

[Andrew Lunn]

> > 
> > > + pr_info("r8152: Using system auxiliary MAC address");
> > 
> > It would be great to write also mac address into that pr_info

And since there could be multiple r8152 in the system, it would be
good to indicate which of them is having its MAC changed. So
netdev_info() or dev_info().

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

Re: [PATCH] r8152: Add support for setting MAC to system's Auxiliary MAC address

[Andrew Lunn]

On Wed, Jun 01, 2016 at 04:50:44PM -0500, Mario Limonciello wrote:
> Dell systems with Type-C ports have support for a persistent system
> specific MAC address when used with Dell Type-C docks and dongles.
> This means a dock plugged into two different systems will show different
> (but persistent) MAC addresses.  Dell Type-C docks and dongles use the
> r8152 driver.
> 
> This information for the system's persistent MAC address is burned in when
> the HW is built and avilable under _SB\AMAC in the DSDT at runtime.
> 
> More information about the technology is available here:
> http://www.dell.com/support/article/us/en/04/SLN301147
> 
> Signed-off-by: Mario Limonciello 
> ---
>  drivers/net/usb/Kconfig |  1 +
>  drivers/net/usb/r8152.c | 37 +
>  2 files changed, 38 insertions(+)
> 
> diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig
> index cdde590..c320930 100644
> --- a/drivers/net/usb/Kconfig
> +++ b/drivers/net/usb/Kconfig
> @@ -98,6 +98,7 @@ config USB_RTL8150
>  config USB_RTL8152
>   tristate "Realtek RTL8152/RTL8153 Based USB Ethernet Adapters"
>   select MII
> + depends on ACPI

Hi Mario

That seems a bit heavy handed. What about ARM or MIPS machines which
don't use ACPI but do have USB ports where i could plug in a USB
dongle with this chipset.

I think it would be better to make use of ACPI if it is available, but
don't require it in order the build the driver.

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

Re: [REGRESSION] asix: Lots of asix_rx_fixup() errors and slow transmissions

[Andrew Lunn]

> In other words, the full-speed hub is restricting the USB to
> Ethernet Adaptor to a 12Mbps (half-duplex) bandwidth to support
> Ethernet 100Mbps (full-duplex) traffic. That is not going to work
> very well because Ethernet frames (perhaps partial Ethernet frames)
> need to be discarded within the USB link.

If that really is true, the design is broken. I would expect the
adaptor to reliably transfer whole frames over USB, and drop whole
frames from its receive queue when the USB is congested. TCP is also
going to see the USB bottleneck as just like any bottleneck in the
network and back off. So TCP streams should not cause major congestion
on the USB link. Going over a 12Mbps USB link should be no different
to hitting an old Ethernet hub which can only do 10/Half.

> Therefore please retest with a working high-speed USB hub or remove
> the full-speed USB hub from the test environment and directly
> connect the USB to Ethernet Adaptor to the root hub of the USB port.
> Then repeat the tests to see whether anything improved.
> 
> In other words, you need to eliminate the dmesg messages saying "not
> running at top speed; connect to a high speed hub".

I would also suggest testing with the Ethernet at 10/half. You should
be able to use Ethtool to set that up. Your USB and Ethernet bandwidth
become more equal. If you still see errors, it suggests a protocol
implementation error somewhere.

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

Re: [PATCH 1/3] usb: core: add power sequence for USB devices

[Andrew Lunn]

> So, would you like to accept the generic solution like below:
> 
> - Create a generic power sequence driver, and it will be probed
> according to compatible string at device tree. At its probe,
> we can create a power sequence structure, and let this structure
> as the private data for this power sequence device.

I'm not sure a separate driver is required. Why not consider it more
like pinctrl properties? They are listed in the devices node. Have the
bus enumerate code first walk all children and run their on sequence.
Bus shutdown would again walk the children and run the off sequence.

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

Re: [PATCH 1/3] usb: core: add power sequence for USB devices

[Andrew Lunn]

On Fri, Mar 04, 2016 at 10:02:42AM +0800, Peter Chen wrote:
> On Thu, Mar 03, 2016 at 02:54:55PM -0600, Rob Herring wrote:
> > On Thu, Mar 3, 2016 at 4:01 AM, Peter Chen  wrote:
> > > Some hard-wired USB devices need to do power sequence to let the
> > > device work normally, the typical power sequence like: enable USB
> > > PHY clock, toggle reset pin, etc. But current Linux USB driver
> > > lacks of such code to do it, it may cause some hard-wired USB devices
> > > works abnormal or can't be recognized by controller at all.
> > >
> > > In this patch, it will do power on sequence at hub's probe for all
> > > devices under this hub (includes root hub) if this device is described
> > > at dts and there is a phandle "usb-pwrseq" for it. At hub_disconnect,
> > > it will do power off sequence.
> > >
> > > Signed-off-by: Peter Chen 
> > > ---
> > >  .../devicetree/bindings/usb/usb-device.txt |  41 +-
> > >  drivers/usb/core/Makefile  |   2 +-
> > >  drivers/usb/core/hub.c |  32 +
> > >  drivers/usb/core/pwrseq.c  | 149 
> > > +
> > >  include/linux/usb/of.h |  10 ++
> > >  5 files changed, 232 insertions(+), 2 deletions(-)
> > >  create mode 100644 drivers/usb/core/pwrseq.c
> > >
> > > diff --git a/Documentation/devicetree/bindings/usb/usb-device.txt 
> > > b/Documentation/devicetree/bindings/usb/usb-device.txt
> > > index 1c35e7b..c7a298c 100644
> > > --- a/Documentation/devicetree/bindings/usb/usb-device.txt
> > > +++ b/Documentation/devicetree/bindings/usb/usb-device.txt
> > > @@ -13,8 +13,37 @@ Required properties:
> > >  - reg: the port number which this device is connecting to, the range
> > >is 1-31.
> > >
> > > +Optional properties:
> > > +- usb-pwrseq: the power sequence handler which need to do before this USB
> > > +  device can work.
> > > +- clocks: the input clock for USB device.
> > > +- clock-frequency: the frequency for device's clock.
> > > +- reset-gpios: Should specify the GPIO for reset.
> > > +- reset-duration-us: the duration in microsecond for assert reset signal.
> > 
> > So we sorted out how to describe USB devices in DT, but now we don't
> > use it?
> 
> We only know USB device after USB bus finds it, but without power
> on sequence, the USB bus can't find it.

Not really true. Device tree says it exists, you just cannot see it
yet. The fact it is in device tree means it is soldered on the board
and really is there. So when the host controller enumerates the bus,
it should run the power sequence of all child nodes to make them
appear.

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

Re: [PATCH 2/3] usb: chipidea: host: let the hcd know's parent device node

[Andrew Lunn]

> > > diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
> > > index 053bac9..55120ef 100644
> > > --- a/drivers/usb/chipidea/host.c
> > > +++ b/drivers/usb/chipidea/host.c
> > > @@ -109,15 +109,25 @@ static int host_start(struct ci_hdrc *ci)
> > >   struct ehci_hcd *ehci;
> > >   struct ehci_ci_priv *priv;
> > >   int ret;
> > > + struct device *dev = ci->dev;
> > >  
> > > - if (usb_disabled())
> > > + if (usb_disabled() || !dev)
> > >   return -ENODEV;
> > >  
> > > - hcd = usb_create_hcd(_ehci_hc_driver, ci->dev, dev_name(ci->dev));
> > > + /*
> > > +  * USB Core will try to get child node under roothub,
> > > +  * but chipidea core has no of_node, and the child node
> > > +  * for controller is located at glue layer's node which
> > > +  * is chipidea core's parent.
> > > +  */
> > > + if (dev->parent && dev->parent->of_node)
> > > + dev->of_node = dev->parent->of_node;
> > 
> > Is this a good idea? Two devices with the same of_node?
> > 
> 
> This is only for chipidea driver whose host controller device
> doesn't have entry at dts, but other host controller driver which
> supports device tree should have its entry at dts.
> 
> > I know the networking code assumes of_node values are unique, and uses
> > it to find a device. Are you 100% sure the USB code does not make this
> > assumption.
> > 
> 
> The controller device is the root for USB device, the common
> USB code will not touch its glue layer device (controller's parent).

I'm just thinking about code like:

of_find_spi_master_by_node(), of_find_net_device_by_node(),
of_find_backlight_by_node(), etc.

If somebody was to implement an of_find_usb_host_by_node() are you
100% sure the right node will be found? This seems like a bug waiting
to happen.

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

Re: [PATCH 2/3] usb: chipidea: host: let the hcd know's parent device node

[Andrew Lunn]

On Thu, Mar 03, 2016 at 06:01:15PM +0800, Peter Chen wrote:
> From: Peter Chen 
> 
> Since the hcd (chipidea core device) has no device node, so
> if we want to describe the child node under the hcd, we had
> to put it under its parent's node (glue layer device), and
> in the code, we need to let the hcd knows glue layer's code,
> then the USB core can handle this node.
> 
> Signed-off-by: Peter Chen 
> ---
>  drivers/usb/chipidea/host.c | 18 ++
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
> index 053bac9..55120ef 100644
> --- a/drivers/usb/chipidea/host.c
> +++ b/drivers/usb/chipidea/host.c
> @@ -109,15 +109,25 @@ static int host_start(struct ci_hdrc *ci)
>   struct ehci_hcd *ehci;
>   struct ehci_ci_priv *priv;
>   int ret;
> + struct device *dev = ci->dev;
>  
> - if (usb_disabled())
> + if (usb_disabled() || !dev)
>   return -ENODEV;
>  
> - hcd = usb_create_hcd(_ehci_hc_driver, ci->dev, dev_name(ci->dev));
> + /*
> +  * USB Core will try to get child node under roothub,
> +  * but chipidea core has no of_node, and the child node
> +  * for controller is located at glue layer's node which
> +  * is chipidea core's parent.
> +  */
> + if (dev->parent && dev->parent->of_node)
> + dev->of_node = dev->parent->of_node;

Is this a good idea? Two devices with the same of_node?

I know the networking code assumes of_node values are unique, and uses
it to find a device. Are you 100% sure the USB code does not make this
assumption.

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

Re: [PATCH] asix: do not free array priv->mdio->irq

[Andrew Lunn]

On Thu, Mar 03, 2016 at 01:27:56PM +, Colin King wrote:
> From: Colin Ian King <colin.k...@canonical.com>
> 
> Used to be allocated and required freeing, but now
> priv->mdio->irq is now a fixed sized array and should no longer be
> free'd.
> 
> Issue detected using static analysis with CoverityScan
> 
> Fixes: e7f4dc3536a400 ("mdio: Move allocation of interrupts into core")
> Signed-off-by: Colin Ian King <colin.k...@canonical.com>

Reviewed-by: Andrew Lunn <and...@lunn.ch>

Thanks
Andrew

> ---
>  drivers/net/usb/ax88172a.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/net/usb/ax88172a.c b/drivers/net/usb/ax88172a.c
> index 224e7d8..cf77f2d 100644
> --- a/drivers/net/usb/ax88172a.c
> +++ b/drivers/net/usb/ax88172a.c
> @@ -134,7 +134,6 @@ static void ax88172a_remove_mdio(struct usbnet *dev)
>  
>   netdev_info(dev->net, "deregistering mdio bus %s\n", priv->mdio->id);
>   mdiobus_unregister(priv->mdio);
> - kfree(priv->mdio->irq);
>   mdiobus_free(priv->mdio);
>  }
>  
> -- 
> 2.7.0
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [ehci-orion] ETIMEOUT with ehci_setup()'s ehci_halt()

[Andrew Lunn]

 One final update on this topic: the kernel config was missing the
 CONFIG_USB_EHCI_ROOT_HUB_TT option. It turns out that the USB IP embeded a
 Transaction Translator and there is a check in ehci_halt() that aborts the
 routine in case of a Transaction Translator in the ehci controller. The check
 however only works in the above option is enabled.

Maybe it would be worth detecting this and issuing a warning?

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

Re: [ehci-orion] ETIMEOUT with ehci_setup()'s ehci_halt()

[Andrew Lunn]

On Fri, Jun 05, 2015 at 04:34:54PM +0200, Valentin Longchamp wrote:
 Hello,
 
 I am currently bringing up the USB 2.0 Host controller of the Bobcat's 
 (98DX4122
 Marvell switch) internal kirkwood CPU on a variation of Keymile's km_kirwood
 hardware (kirkwood-km_kirkwood.dts).
 
 When the driver registers its hcd, it fails with a timemout as it can be seen 
 in
 the below log:
 
  root@kmcoge5un:~# dmesg -n 8
  root@kmcoge5un:~# insmod ehci-orion.ko 
  ehci-orion: EHCI orion driver
  Initializing Orion-SoC USB Host Controller
  orion-ehci f105.ehci: EHCI Host Controller
  orion-ehci f105.ehci: new USB bus registered, assigned bus number 1
  orion-ehci f105.ehci: reset hcs_params 0x10011 dbg=0 ind cc=0 pcc=0 
  ordered ports=1
  orion-ehci f105.ehci: reset hcc_params 0006 thresh 0 uframes 
  256/512/1024 park
  orion-ehci f105.ehci: park 0
  orion-ehci f105.ehci: reset command 0080002 (park)=0 ithresh=8 
  period=1024 Reset HALT
  orion-ehci f105.ehci: can't setup
  orion-ehci f105.ehci: USB bus 1 deregistered
  orion-ehci f105.ehci: init f105.ehci fail, -110
  orion-ehci: probe of f105.ehci failed with error -110
 
 This is very similar to this problem, except that it always happens:
 https://lkml.org/lkml/2012/6/4/381
 
 I have cornered it out to the last handshake() call of the ehci_halt() call,
 that should set the controller in the HALT state. If I comment this 
 handshake()
 call out, the registration is fine and the controller then looks OK (I don't
 have a hardware with a real USB bus to further test it yet).
 
 My current idea is that maybe a reset or an initialization may be missing, 
 but I
 wanted to ask if anybody already has seen a similar behavior with the various
 hardware platform supported by ehci-orion ?

Hi Valentin

I don't remember seeing any problems like this on any kirkwood
devices.

Do you know what phy the 98DX4122 uses? The ehci-orion.c has some code
for handling the Orion5X USB phy. I also think u-boot might also have
some code. Sebastian might know more, i think he implemented USB
support for barebox.

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

Re: [RFC PATCH 0/3] Enable connecting DSA-based switch to the USB RMII interface.

[Andrew Lunn]

On Wed, Apr 22, 2015 at 04:14:33PM +, Jan Kaisrlik wrote:
 2015-04-21 17:51 GMT+00:00 Florian Fainelli f.faine...@gmail.com:
  On 21/04/15 10:39, Andrew Lunn wrote:
  I would however say that sysfs is the wrong API. The linux network
  stack uses netlink for most configuration activities. So i would
  suggest adding a netlink binding to DSA, and place the code in
  net/dsa/, not within an MDIO driver.
 
  I suppose we could do that, but that sounds like a pretty radical change
  in how DSA is currently configured (that is statically at boot time),
  part in order to allow booting from DSA-enabled network devices (e.g:
  nfsroot).
 
  We would keep both DT and platform device. But statically at boot does
  not work for a USB hotpluggable switch!
 
  Is the switch really hotpluggable, or it is the USB-Ethernet adapter
  connecting to it? If the former, then I agree, if not, I would imagine
  that there is nothing that prevents creating the switch device first,
  and wait for its master_netdev to show up later before it starts doing
  anything useful?
  --
  Florian
 
 Thank you for your quick and helpful answers.
 
 The goal of this project is to extend embeded modules without integreted
 MII to add possibility to connect ethernet switch.
 
 Current version of switch is hotplugable but this feature is not required.
 In my humble opinion, hotplugable switch seems to be pretty interesting idea.

Hi Jan

Thanks for the extra information.

I don't know this USB device. Can you change the product:vendor ID?
Could you imply from the USB product:vendor ID what the DSA
configuration is? So have a wrapper driver around the asix driver
which installs a dsa platform device and then instantiates the asix
driver? That eliminates all your DSA changes, no need for a user space
API, etc.

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

Re: [RFC PATCH 0/3] Enable connecting DSA-based switch to the USB RMII interface.

[Andrew Lunn]

  I would however say that sysfs is the wrong API. The linux network
  stack uses netlink for most configuration activities. So i would
  suggest adding a netlink binding to DSA, and place the code in
  net/dsa/, not within an MDIO driver.
 
 I suppose we could do that, but that sounds like a pretty radical change
 in how DSA is currently configured (that is statically at boot time),
 part in order to allow booting from DSA-enabled network devices (e.g:
 nfsroot).

We would keep both DT and platform device. But statically at boot does
not work for a USB hotpluggable switch!

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

Re: [RFC PATCH 0/3] Enable connecting DSA-based switch to the USB RMII interface.

[Andrew Lunn]

 My goal in reworking this weird DSA device/driver model is that you
 could just register your switch devices as an enhanced
 phy_driver/spi_driver/pci_driver etc..., such that libphy-ready drivers
 could just take advantage of that when they scan/detect their MDIO buses
 and find a switch. We are not quite there yet, but some help could be
 welcome, here are the WIP patches (tested with platform_driver only so far):

We are hijacking another thread, but...

I don't understand you here. Who calls dsa_switch_register()?

I know of a board coming soon which has three switch chips on
it. There is one MDIO device in the Soc, but there is an external MDIO
multiplexor controlled via gpio lines, such that each switch has its
own MDIO bus. The DT binding does not support this currently, but the
underlying data structures do.

How do you envisage dsa_switch_register() to work in such a setup?

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

Re: [RFC PATCH 0/3] Enable connecting DSA-based switch to the USB RMII interface.

[Andrew Lunn]

Hi Jan

Interesting work, but i think the architecture is wrong.

DSA needs an Ethernet device, an MDIO bus, and information about ports
on the switch. The MDIO bus and the Ethernet need no knowledge of
DSA. So putting your DSA configuration code in the MDIO driver is
wrong.

The problem you have is where the put the configuration data. There
are the currently two choices, using a platform driver, which you can
find some examples of in arch/arm/mach-orion5x, or via device tree. Or
you need a new method.

Part of your problem is hotplug, since you have a USB device, and no
stable names for the ethernet device nor the MDIO device. Your
hardware is not fixed, you could hang any switch off the USB
device. So it does sound like you need a user space API.

I would however say that sysfs is the wrong API. The linux network
stack uses netlink for most configuration activities. So i would
suggest adding a netlink binding to DSA, and place the code in
net/dsa/, not within an MDIO driver.

Device tree overlays might be a solution, if you can dynamically load
a blob as part of a USB hotplug event. What makes it easier is that
both the Ethernet device and MDIO bus are on the same USB device, so
all your phandles are within the blob.

What is your long term goal? Is this just a development tool? Are you
thinking of making a product which integrates both the switch and the
USB ethernet onto a USB dongle? This could also change the
architecture, since it makes the configuration more fixed.

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

Re: [PATCH v2 0/3] ARM: mvebu: Enable XHCI on the Armada 385 AP

[Andrew Lunn]

On Tue, Jan 20, 2015 at 09:30:28PM +0100, Maxime Ripard wrote:
 Hi Andrew,
 
 On Mon, Jan 19, 2015 at 10:35:07PM +0100, Andrew Lunn wrote:
  On Mon, Jan 19, 2015 at 02:01:11PM +0100, Maxime Ripard wrote:
   Hi all,
   
   This serie enables the Armada 385 AP XHCI controller.
   
   Since the controller uses a GPIO-controlled VBUS, we used the
   phy-generic driver, and made the needed additions to the xhci-plat
   driver to retrieve a USB phy.
   
   Unfortunately, some glitches were also found along the way, mostly
   because of the probe deferring that was introduced by this phy
   retrieval.
   
   Since the introduction of the Armada 38x support in 3.16, the driver
   was attempting to write into registers while the clock wasn't enabled
   yet. This was working because the bootloader left it enabled, but in
   the case of a deferred probing, the clock would have been disabled by
   the error path of our driver, and this would fail. This should go in
   3.19, and any stable kernel for 3.16+.
   
   The two patches remaining are regular patches, and are aimed at
   3.20. The last patch depend on my previous serie to introduce support
   for the the A385 AP board.
  
  Hi Maxime
  
  I assume you want me to take 3/3? Any other route is not simple, since
  this file only exists in mvebu/dt and maybe a staging branch of
  arm-soc.
 
  What route do you think the other patches will take?
 
 There should be no merge dependency, but merging the third patch alone
 will probably result on a boot breakage. I don't think it really
 matters though, since this is a new board, so I guess it can go
 through the USB-PHY tree.

Hi Maxime

Humm, maybe i'm wrong, but i think

arch/arm/boot/dts/armada-385-db-ap.dts

only exists in the mvebu tree?

At least, i don't see it here:

https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/tree/arch/arm/boot/dts?h=next

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

Re: [PATCH v2 0/3] ARM: mvebu: Enable XHCI on the Armada 385 AP

[Andrew Lunn]

On Mon, Jan 19, 2015 at 02:01:11PM +0100, Maxime Ripard wrote:
 Hi all,
 
 This serie enables the Armada 385 AP XHCI controller.
 
 Since the controller uses a GPIO-controlled VBUS, we used the
 phy-generic driver, and made the needed additions to the xhci-plat
 driver to retrieve a USB phy.
 
 Unfortunately, some glitches were also found along the way, mostly
 because of the probe deferring that was introduced by this phy
 retrieval.
 
 Since the introduction of the Armada 38x support in 3.16, the driver
 was attempting to write into registers while the clock wasn't enabled
 yet. This was working because the bootloader left it enabled, but in
 the case of a deferred probing, the clock would have been disabled by
 the error path of our driver, and this would fail. This should go in
 3.19, and any stable kernel for 3.16+.
 
 The two patches remaining are regular patches, and are aimed at
 3.20. The last patch depend on my previous serie to introduce support
 for the the A385 AP board.

Hi Maxime

I assume you want me to take 3/3? Any other route is not simple, since
this file only exists in mvebu/dt and maybe a staging branch of
arm-soc.

What route do you think the other patches will take?

Thanks

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

Re: [PATCH v3 01/12] reset: add the Berlin reset controller driver

[Andrew Lunn]

On Wed, Jul 16, 2014 at 10:25:55AM +0200, Antoine Ténart wrote:
 Add a reset controller for Marvell Berlin SoCs which is used by the
 USB PHYs drivers (for now).
 
 Signed-off-by: Antoine Ténart antoine.ten...@free-electrons.com
 Signed-off-by: Sebastian Hesselbarth sebastian.hesselba...@gmail.com
 Acked-by: Philipp Zabel p.za...@pengutronix.de
 ---
  drivers/reset/Makefile   |   1 +
  drivers/reset/reset-berlin.c | 131 
 +++
  2 files changed, 132 insertions(+)
  create mode 100644 drivers/reset/reset-berlin.c
 
 diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
 index 60fed3d7820b..157d421f755b 100644
 --- a/drivers/reset/Makefile
 +++ b/drivers/reset/Makefile
 @@ -1,4 +1,5 @@
  obj-$(CONFIG_RESET_CONTROLLER) += core.o
  obj-$(CONFIG_ARCH_SOCFPGA) += reset-socfpga.o
 +obj-$(CONFIG_ARCH_BERLIN) += reset-berlin.o
  obj-$(CONFIG_ARCH_SUNXI) += reset-sunxi.o
  obj-$(CONFIG_ARCH_STI) += sti/
 diff --git a/drivers/reset/reset-berlin.c b/drivers/reset/reset-berlin.c
 new file mode 100644
 index ..7b894047a81d
 --- /dev/null
 +++ b/drivers/reset/reset-berlin.c
 @@ -0,0 +1,131 @@
 +/*
 + * Copyright (C) 2014 Marvell Technology Group Ltd.
 + *
 + * Antoine Ténart antoine.ten...@free-electrons.com
 + * Sebastian Hesselbarth sebastian.hesselba...@gmail.com
 + *
 + * This file is licensed under the terms of the GNU General Public
 + * License version 2. This program is licensed as is without any
 + * warranty of any kind, whether express or implied.
 + */
 +
 +#include linux/delay.h
 +#include linux/io.h
 +#include linux/module.h
 +#include linux/of.h
 +#include linux/of_address.h
 +#include linux/platform_device.h
 +#include linux/reset-controller.h
 +#include linux/slab.h
 +#include linux/types.h
 +
 +#define BERLIN_MAX_RESETS32
 +
 +#define to_berlin_reset_priv(p)  \
 + container_of((p), struct berlin_reset_priv, rcdev)
 +
 +struct berlin_reset_priv {
 + void __iomem*base;
 + unsigned intsize;
 + struct reset_controller_dev rcdev;
 +};
 +
 +static int berlin_reset_reset(struct reset_controller_dev *rcdev,
 +   unsigned long id)
 +{
 + struct berlin_reset_priv *priv = to_berlin_reset_priv(rcdev);
 + int offset = id  8;
 + int mask = BIT(id  0xff);

nit: 

The parameter to BIT() should be 0-31. So you want to  with 5. Given
your xlate function, it should never happen that it is greater than
31. Hence it is only a nit. If you need to respin, you can change
this, but don't bother to respin just because of this.

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

Re: [PATCH 2/5] Documentation: dt-bindings: document the Armada 375 USB cluster binding

[Andrew Lunn]

On Fri, May 23, 2014 at 02:54:02PM +0530, Kishon Vijay Abraham I wrote:
 Hi,
 
 On Friday 16 May 2014 09:52 PM, Gregory CLEMENT wrote:
  Armada 375 comes with an USB2 host and device controller and an USB3
  controller. The USB cluster control register allows to manage common
  features of both USB controllers. This commit adds the Device Tree
  binding documentation for this piece of hardware.
 
 Pls re-order so that the Documentation patch comes before the driver patch..
  
  Signed-off-by: Gregory CLEMENT gregory.clem...@free-electrons.com
  ---
   .../bindings/phy/armada-375-usb-phy-cluster.txt  | 20 
  
   1 file changed, 20 insertions(+)
   create mode 100644 
  Documentation/devicetree/bindings/phy/armada-375-usb-phy-cluster.txt
  
  diff --git 
  a/Documentation/devicetree/bindings/phy/armada-375-usb-phy-cluster.txt 
  b/Documentation/devicetree/bindings/phy/armada-375-usb-phy-cluster.txt
 
 simpler file name? armada-phy?

Armada is a collection of families of SoCs. This driver is very
specific to one SoC, in one family of SoCs.

Do you want one .txt file per driver, or can we combine binding
documentations into one file? There should already be a mvebu-phy.txt,
which contains the sata phy usable on some of the Armada SoC families.
This binding could be appended to it.

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

Re: [PATCH 2/5] Documentation: dt-bindings: document the Armada 375 USB cluster binding

[Andrew Lunn]

 Do you want one .txt file per driver, or can we combine binding
 documentations into one file? There should already be a mvebu-phy.txt,
 which contains the sata phy usable on some of the Armada SoC families.
 This binding could be appended to it.

Ah. Humm, well! It seems the patch adding the mvebu-phy.txt fell
through a crack when adding the driver.

Kishon could you take
http://www.spinics.net/lists/devicetree/msg17018.html into your tree?
It should of been taken the same time you took the actual driver,
http://www.spinics.net/lists/devicetree/msg17011.html into your tree.

I can resend that one missing patch if you want.

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

Re: [PATCH 2/5] Documentation: dt-bindings: document the Armada 375 USB cluster binding

[Andrew Lunn]

On Fri, May 23, 2014 at 07:22:48PM +0530, Kishon Vijay Abraham I wrote:
 hI,
 
 On Friday 23 May 2014 07:06 PM, Andrew Lunn wrote:
  Do you want one .txt file per driver, or can we combine binding
  documentations into one file? There should already be a mvebu-phy.txt,
  which contains the sata phy usable on some of the Armada SoC families.
  This binding could be appended to it.
  
  Ah. Humm, well! It seems the patch adding the mvebu-phy.txt fell
  through a crack when adding the driver.
  
  Kishon could you take
  http://www.spinics.net/lists/devicetree/msg17018.html into your tree?
  It should of been taken the same time you took the actual driver,
  http://www.spinics.net/lists/devicetree/msg17011.html into your tree.
  
  I can resend that one missing patch if you want.
 
 yes please. But it's already too late to go in the next merge window.

Gregory, could you pick it up and append your 375 binding to it? We
can avoid merge conflicts that way.

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

Re: [PATCH v3 02/20] usb: ehci-orion: Add the optional PHY support

[Andrew Lunn]

On Wed, May 07, 2014 at 11:40:06AM +0200, Thomas Petazzoni wrote:
 Dear Andrew Lunn,
 
 On Tue, 6 May 2014 15:33:41 +0200, Andrew Lunn wrote:
 
   + priv-phy = devm_phy_get(pdev-dev, usb);
   + if (!IS_ERR(priv-phy)) {
   + err = phy_init(priv-phy);
   + if (err)
   + goto err2;
   +
   + err = phy_power_on(priv-phy);
   + if (err)
   + goto err3;
   + }
  
  Hi Gregory
  
  What about EPROBE_DEFERRED?
 
 In v4 (to be submitted soon), I've changed this to:
 
   priv-phy = devm_phy_optional_get(pdev-dev, usb);
   if (IS_ERR(priv-phy)) {
   err = PTR_ERR(priv-phy);
   goto err_phy_get;
   } else {
   err = phy_init(priv-phy);
   if (err)
   goto err_phy_init;
 
   err = phy_power_on(priv-phy);
   if (err)
   goto err_phy_power_on;
   }
 
 Thanks to devm_phy_optional_get(), the fact of not having a PHY in the
 DT is not considered an error. So on any error from
 devm_phy_optional_get() (including -EPROBE_DEFER), we simply bail out.
 Does this looks good?

Hi Thomas

That looks good.

To avoid the SATA phy problems we had last time, i would like to test
this on a few systems. Please could you let me know what branch v4 is
in when it is ready.

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

Re: [PATCH v3 02/20] usb: ehci-orion: Add the optional PHY support

[Andrew Lunn]

On Tue, May 06, 2014 at 02:13:57AM +0200, Gregory CLEMENT wrote:
 This commit allows to use the PHY provided through the device tree. It
 will be useful for the Armada 375 SoCs. if no PHY is provided then the
 behavior of the driver is unchanged.
 
 Signed-off-by: Gregory CLEMENT gregory.clem...@free-electrons.com
 ---
  drivers/usb/host/ehci-orion.c | 25 +++--
  1 file changed, 23 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/usb/host/ehci-orion.c b/drivers/usb/host/ehci-orion.c
 index d6c19c37c76b..f6d9eb2e33cd 100644
 --- a/drivers/usb/host/ehci-orion.c
 +++ b/drivers/usb/host/ehci-orion.c
 @@ -15,6 +15,7 @@
  #include linux/clk.h
  #include linux/platform_data/usb-ehci-orion.h
  #include linux/of.h
 +#include linux/phy/phy.h
  #include linux/of_device.h
  #include linux/of_irq.h
  #include linux/usb.h
 @@ -46,6 +47,7 @@
  
  struct orion_ehci_hcd {
   struct clk *clk;
 + struct phy *phy;
  };
  
  static const char hcd_name[] = ehci-orion;
 @@ -224,6 +226,18 @@ static int ehci_orion_drv_probe(struct platform_device 
 *pdev)
   if (!IS_ERR(priv-clk))
   clk_prepare_enable(priv-clk);
  
 +
 + priv-phy = devm_phy_get(pdev-dev, usb);
 + if (!IS_ERR(priv-phy)) {
 + err = phy_init(priv-phy);
 + if (err)
 + goto err2;
 +
 + err = phy_power_on(priv-phy);
 + if (err)
 + goto err3;
 + }

Hi Gregory

What about EPROBE_DEFERRED?

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

Re: [PATCH v3 08/20] ARM: mvebu: Add Device Tree description of xHCI hosts on Armada 38x

[Andrew Lunn]

On Tue, May 06, 2014 at 02:14:03AM +0200, Gregory CLEMENT wrote:
 The Marvell Armada 38x SoCs contains two xHCI host. This commit adds
 the Device Tree description of those interfaces at the SoC level, and
 also enables the two USB3 ports on the Armada 385 DB platform and one
 USB3 port on the Armada 385 RD platform.
 
 Signed-off-by: Gregory CLEMENT gregory.clem...@free-electrons.com
 ---
  arch/arm/boot/dts/armada-385-db.dts |  8 
  arch/arm/boot/dts/armada-385-rd.dts |  4 
  arch/arm/boot/dts/armada-38x.dtsi   | 17 +
  3 files changed, 29 insertions(+)
 
 diff --git a/arch/arm/boot/dts/armada-385-db.dts 
 b/arch/arm/boot/dts/armada-385-db.dts
 index 6828d77696a6..d5db1466da82 100644
 --- a/arch/arm/boot/dts/armada-385-db.dts
 +++ b/arch/arm/boot/dts/armada-385-db.dts
 @@ -101,6 +101,14 @@
   reg = 0x100 0x3f00;
   };
   };
 +
 + usb3@f {
 + status = okay;
 + };
 +
 + usb3@f8000 {
 + status = okay;
 + };
   };
  
   pcie-controller {
 diff --git a/arch/arm/boot/dts/armada-385-rd.dts 
 b/arch/arm/boot/dts/armada-385-rd.dts
 index 45250c88814b..a505fe94ff37 100644
 --- a/arch/arm/boot/dts/armada-385-rd.dts
 +++ b/arch/arm/boot/dts/armada-385-rd.dts
 @@ -77,6 +77,10 @@
   reg = 1;
   };
   };
 +
 + usb3@f {
 + status = okay;
 + };
   };
  
   pcie-controller {
 diff --git a/arch/arm/boot/dts/armada-38x.dtsi 
 b/arch/arm/boot/dts/armada-38x.dtsi
 index a064f59da02d..5913ce1cc601 100644
 --- a/arch/arm/boot/dts/armada-38x.dtsi
 +++ b/arch/arm/boot/dts/armada-38x.dtsi
 @@ -355,6 +355,23 @@
   clocks = coredivclk 0;
   status = disabled;
   };
 +
 + usb3@f {
 + compatible = marvell,armada-380-xhci;
 + reg = 0xf 0x3fff,0xf4000 0x3fff;

0x3fff?

I would expect that to be 0x4000.

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

Re: [PATCH v3 17/20] phy: Add support for USB cluster on the Armada 375 SoC

[Andrew Lunn]

On Tue, May 06, 2014 at 02:14:12AM +0200, Gregory CLEMENT wrote:
 The Armada 375 SoC comes with an USB2 host and device controller and
 an USB3 controller. The USB cluster control register allows to manage
 common features of both USB controllers. It uses the generic PHY
 framework
 
 Signed-off-by: Gregory CLEMENT gregory.clem...@free-electrons.com
 ---
  drivers/phy/Kconfig  |   5 ++
  drivers/phy/Makefile |   1 +
  drivers/phy/phy-armada375-usb2.c | 154 
 +++
  3 files changed, 160 insertions(+)
  create mode 100644 drivers/phy/phy-armada375-usb2.c
 
 diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
 index 3bb05f17b9b4..cdf3e2c24e3a 100644
 --- a/drivers/phy/Kconfig
 +++ b/drivers/phy/Kconfig
 @@ -15,6 +15,11 @@ config GENERIC_PHY
 phy users can obtain reference to the PHY. All the users of this
 framework should select this config.
  
 +config ARMADA375_USBCLUSTER_PHY
 + def_bool y
 + depends on OF
 + select GENERIC_PHY
 +
  config PHY_EXYNOS_MIPI_VIDEO
   tristate S5P/EXYNOS SoC series MIPI CSI-2/DSI PHY driver
   depends on HAS_IOMEM
 diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
 index 2faf78edc864..47d5a86807b6 100644
 --- a/drivers/phy/Makefile
 +++ b/drivers/phy/Makefile
 @@ -3,6 +3,7 @@
  #
  
  obj-$(CONFIG_GENERIC_PHY)+= phy-core.o
 +obj-$(CONFIG_ARMADA375_USBCLUSTER_PHY)   += phy-armada375-usb2.o
  obj-$(CONFIG_BCM_KONA_USB2_PHY)  += phy-bcm-kona-usb2.o
  obj-$(CONFIG_PHY_EXYNOS_DP_VIDEO)+= phy-exynos-dp-video.o
  obj-$(CONFIG_PHY_EXYNOS_MIPI_VIDEO)  += phy-exynos-mipi-video.o
 diff --git a/drivers/phy/phy-armada375-usb2.c 
 b/drivers/phy/phy-armada375-usb2.c
 new file mode 100644
 index ..8bbac45e72c8
 --- /dev/null
 +++ b/drivers/phy/phy-armada375-usb2.c
 @@ -0,0 +1,154 @@
 +/*
 + * USB cluster support for Armada 375 platform.
 + *
 + * Copyright (C) 2014 Marvell
 + *
 + * Gregory CLEMENT gregory.clem...@free-electrons.com
 + *
 + * This file is licensed under the terms of the GNU General Public
 + * License version 2 or later. This program is licensed as is
 + * without any warranty of any kind, whether express or implied.
 + *
 + * Armada 375 comes with an USB2 host and device controller and an
 + * USB3 controller. The USB cluster control register allows to manage
 + * common features of both USB controller.
 + */
 +
 +#include linux/init.h
 +#include linux/io.h
 +#include linux/kernel.h
 +#include linux/module.h
 +#include linux/of_address.h
 +#include linux/phy/phy.h
 +#include linux/platform_device.h
 +#include linux/slab.h
 +
 +#define USB2_PHY_CONFIG_ENABLE BIT(0) /* active low */
 +
 +/* the USB cluster allow to choose between two PHYs*/
 +#define NB_PHY 2
 +
 +enum {
 + PHY_USB2 = 0,
 + PHY_USB3 = 1,
 +};
 +
 +struct armada375_cluster_phy {
 + struct phy *phy;
 + void __iomem *reg;
 + bool enable;
 + bool use_usb3;

Hi Gregory

nit: How about using the enum you just defined?

 +};
 +
 +struct armada375_cluster_phy usb_cluster_phy[NB_PHY];
 +
 +static int armada375_usb_phy_init(struct phy *phy)
 +{
 + struct armada375_cluster_phy *cluster_phy = phy_get_drvdata(phy);
 + u32 reg;
 +
 + if (cluster_phy-enable) {
 + reg = readl(cluster_phy-reg);
 + if (cluster_phy-use_usb3)
 + reg |= USB2_PHY_CONFIG_ENABLE;
 + else
 + reg = ~USB2_PHY_CONFIG_ENABLE;
 + writel(reg, cluster_phy-reg);
 +
 + return 0;
 + } else {
 + return -ENODEV;
 + }
 +}
 +
 +static struct phy_ops armada375_usb_phy_ops = {
 + .init = armada375_usb_phy_init,
 + .owner  = THIS_MODULE,
 +};
 +
 +static struct phy *armada375_usb_phy_xlate(struct device *dev,
 + struct of_phandle_args *args)
 +{
 + if (WARN_ON(args-args[0] = NB_PHY))
 + return ERR_PTR(-ENODEV);
 +
 + return usb_cluster_phy[args-args[0]].phy;
 +}
 +
 +static int armada375_usb_phy_probe(struct platform_device *pdev)
 +{
 + struct device *dev = pdev-dev;
 + struct phy *phy;
 + struct device_node *np = dev-of_node;
 + struct phy_provider *phy_provider;
 + void __iomem *usb_cluster_base;
 + struct device_node *xhci_node;
 + int i;
 +
 + usb_cluster_base = of_iomap(np, 0);

devm_ API?

Check the return value for an error?

 + BUG_ON(!usb_cluster_base);
 +
 + for (i = 0; i  NB_PHY; i++) {
 + phy = devm_phy_create(dev, armada375_usb_phy_ops, NULL);
 + if (IS_ERR(phy))
 + dev_err(dev, failed to create PHY n%d\n, i);
 +
 + usb_cluster_phy[i].phy = phy;
 + usb_cluster_phy[i].reg = usb_cluster_base;
 + usb_cluster_phy[i].enable = false;
 + phy_set_drvdata(phy, usb_cluster_phy[i]);
 + }
 +
 + usb_cluster_phy[PHY_USB2].use_usb3 = false;
 + 

Re: [PATCH v2 02/18] usb: host: xhci-plat: Add clocks support

[Andrew Lunn]

On Fri, Apr 25, 2014 at 04:07:00PM +0200, Gregory CLEMENT wrote:
 Some platform (such as the Armada 38x ones) can gate the clock of
 their USB controller. This patch add the support for the clock, by
 enabling them during probe and disabling them on remove.
 
 As not all platforms have clock support then enabling and disabling
 the clocks have been placed in separate functions. Then if the clocks
 are not supported we still can use the same calls, and there is no
 
 Signed-off-by: Gregory CLEMENT gregory.clem...@free-electrons.com
 ---
  drivers/usb/host/xhci-plat.c | 52 
 ++--
  1 file changed, 50 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
 index f5351af4b2c5..bb5d563f729c 100644
 --- a/drivers/usb/host/xhci-plat.c
 +++ b/drivers/usb/host/xhci-plat.c
 @@ -11,6 +11,7 @@
   * version 2 as published by the Free Software Foundation.
   */
  
 +#include linux/clk.h
  #include linux/dma-mapping.h
  #include linux/module.h
  #include linux/of.h
 @@ -85,6 +86,42 @@ static const struct hc_driver xhci_plat_xhci_driver = {
   .bus_resume =   xhci_bus_resume,
  };
  
 +#if defined(CONFIG_HAVE_CLK)

Hi Gregory

You probably don't need to do this conditional on CONFIG_HAVE_CLK.
There are stub functions in clk.h for when CONFIG_HAVE_CLK is not
defined. So the compiler knows devm_clk_get() will return NULL, it can
evaluate IS_ERR is false, clk_prepare() returns 0 and so the whole
function collapses to return 0;

 +static int try_enable_clk(struct platform_device *pdev)
 +{
 + struct clk *clk = devm_clk_get(pdev-dev, NULL);
 +
 + /* Not all platforms have a clk so it is not an error if the clock
 +does not exists. */
 + if (!IS_ERR(clk))
 + if (clk_prepare_enable(clk))
 + return  -ENODEV;

Is ENODEV the correct error code? It would probably be better to
return the return value of clk_prepare_enable()

 + return 0;
 +}
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 14/18] ARM: mvebu: Add support for USB cluster on the Armada 375 SoC

[Andrew Lunn]

  +* We can't use usb2 and usb3 in the same time, so let's
  +* disbale usb2 and complain about it to the user askinf
 
 typos: disable, asking

And it should be at the same time, not in.

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

Re: [PATCH v2 05/18] ARM: mvebu: Add Device Tree description of xHCI hosts on Armada 38x

[Andrew Lunn]

On Fri, Apr 25, 2014 at 04:07:03PM +0200, Gregory CLEMENT wrote:
 The Marvell Armada 38x SoCs contain two xHCI host. This commit adds
 the Device Tree description of those interfaces at the SoC level, and
 also enables the two USB3 ports on the Armada 385 DB platform and one
 USB3 port on the Armada 385 RD platforms.
 
 Signed-off-by: Gregory CLEMENT gregory.clem...@free-electrons.com
 ---
  arch/arm/boot/dts/armada-385-db.dts |  8 
  arch/arm/boot/dts/armada-385-rd.dts |  4 
  arch/arm/boot/dts/armada-38x.dtsi   | 17 +
  3 files changed, 29 insertions(+)
 
 diff --git a/arch/arm/boot/dts/armada-385-db.dts 
 b/arch/arm/boot/dts/armada-385-db.dts
 index 6828d77696a6..d5db1466da82 100644
 --- a/arch/arm/boot/dts/armada-385-db.dts
 +++ b/arch/arm/boot/dts/armada-385-db.dts
 @@ -101,6 +101,14 @@
   reg = 0x100 0x3f00;
   };
   };
 +
 + usb3@f {
 + status = okay;
 + };
 +
 + usb3@f8000 {
 + status = okay;
 + };
   };
  
   pcie-controller {
 diff --git a/arch/arm/boot/dts/armada-385-rd.dts 
 b/arch/arm/boot/dts/armada-385-rd.dts
 index 45250c88814b..a505fe94ff37 100644
 --- a/arch/arm/boot/dts/armada-385-rd.dts
 +++ b/arch/arm/boot/dts/armada-385-rd.dts
 @@ -77,6 +77,10 @@
   reg = 1;
   };
   };
 +
 + usb3@f {
 + status = okay;
 + };
   };
  
   pcie-controller {
 diff --git a/arch/arm/boot/dts/armada-38x.dtsi 
 b/arch/arm/boot/dts/armada-38x.dtsi
 index a064f59da02d..5913ce1cc601 100644
 --- a/arch/arm/boot/dts/armada-38x.dtsi
 +++ b/arch/arm/boot/dts/armada-38x.dtsi
 @@ -355,6 +355,23 @@
   clocks = coredivclk 0;
   status = disabled;
   };
 +
 + usb3@f {
 + compatible = marvell,armada-380-xhci;
 + reg = 0xf 0x3fff,0xf4000 0x3fff;

s/0x3fff/0x4000/g  ?

Andrew 

 + interrupts = GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH;
 + clocks = gateclk 9;
 + status = disabled;
 + };
 +
 + usb3@f8000 {
 + compatible = marvell,armada-380-xhci;
 + reg = 0xf8000 0x3fff,0xfc000 0x3fff;
 + interrupts = GIC_SPI 17 IRQ_TYPE_LEVEL_HIGH;
 + clocks = gateclk 10;
 + status = disabled;
 + };
 +
   };
   };
  
 -- 
 1.8.1.2
 
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 14/18] ARM: mvebu: Add support for USB cluster on the Armada 375 SoC

[Andrew Lunn]

On Fri, Apr 25, 2014 at 04:07:12PM +0200, Gregory CLEMENT wrote:
 The Armada 375 SoC comes with an USB2 host and device controller and
 an USB3 controller. The USB cluster control register allows to manage
 common features of both USB controllers.
 
 Signed-off-by: Gregory CLEMENT gregory.clem...@free-electrons.com
 ---
  arch/arm/mach-mvebu/Makefile  |  2 +-
  arch/arm/mach-mvebu/usb-cluster.c | 96 
 +++
  2 files changed, 97 insertions(+), 1 deletion(-)
  create mode 100644 arch/arm/mach-mvebu/usb-cluster.c
 
 diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile
 index a63e43b6b451..dec05e7e1802 100644
 --- a/arch/arm/mach-mvebu/Makefile
 +++ b/arch/arm/mach-mvebu/Makefile
 @@ -4,7 +4,7 @@ ccflags-$(CONFIG_ARCH_MULTIPLATFORM) := 
 -I$(srctree)/$(src)/include \
  AFLAGS_coherency_ll.o:= -Wa,-march=armv7-a
  
  obj-y += system-controller.o mvebu-soc-id.o
 -obj-$(CONFIG_MACH_MVEBU_V7)  += board-v7.o
 +obj-$(CONFIG_MACH_MVEBU_V7)  += board-v7.o usb-cluster.o
  obj-$(CONFIG_MACH_DOVE)   += dove.o
  obj-$(CONFIG_ARCH_MVEBU)  += coherency.o coherency_ll.o pmsu.o
  obj-$(CONFIG_SMP)+= platsmp.o headsmp.o
 diff --git a/arch/arm/mach-mvebu/usb-cluster.c 
 b/arch/arm/mach-mvebu/usb-cluster.c
 new file mode 100644
 index ..4c15d282db23
 --- /dev/null
 +++ b/arch/arm/mach-mvebu/usb-cluster.c
 @@ -0,0 +1,96 @@
 +/*
 + * USB cluster support for Armada 375 platform.
 + *
 + * Copyright (C) 2014 Marvell
 + *
 + * Gregory CLEMENT gregory.clem...@free-electrons.com
 + *
 + * This file is licensed under the terms of the GNU General Public
 + * License version 2.  This program is licensed as is without any
 + * warranty of any kind, whether express or implied.
 + *
 + * Armada 375 comes with an USB2 host and device controller and an
 + * USB3 controller. The USB cluster control register allows to manage
 + * common features of both USB controller.
 + */
 +
 +#include linux/kernel.h
 +#include linux/init.h
 +#include linux/of_address.h
 +#include linux/io.h
 +#include linux/slab.h
 +
 +#define USB2_PHY_CONFIG_ENABLE BIT(0) /* active low */
 +
 +static struct of_device_id of_usb_cluster_table[] = {
 + { .compatible = marvell,armada-375-usb-cluster, },
 + { /* end of list */ },
 +};
 +
 +static int __init mvebu_usb_cluster_init(void)
 +{
 + struct device_node *np;
 +
 + np = of_find_matching_node(NULL, of_usb_cluster_table);
 + if (np) {
 + void __iomem *usb_cluster_base;
 + u32 reg;
 + struct device_node *ehci_node, *xhci_node;
 + struct property *ehci_status;
 + bool use_usb3 = false;
 +
 + usb_cluster_base = of_iomap(np, 0);
 + BUG_ON(!usb_cluster_base);
 +
 + xhci_node = of_find_compatible_node(NULL, NULL,
 + marvell,armada-375-xhci);
 +
 + if (xhci_node  of_device_is_available(xhci_node))
 + use_usb3 = true;
 +
 + ehci_node = of_find_compatible_node(NULL, NULL,
 + marvell,orion-ehci);
 +
 + if (ehci_node  of_device_is_available(ehci_node)
 +  use_usb3) {
 + /*
 +  * We can't use usb2 and usb3 in the same time, so let's
 +  * disbale usb2 and complain about it to the user askinf
 +  * to fix the device tree.
 +  */
 +
 + ehci_status = kzalloc(sizeof(struct property),
 + GFP_KERNEL);
 + WARN_ON(!ehci_status);
 +
 + ehci_status-value = kstrdup(disabled, GFP_KERNEL);
 + WARN_ON(!ehci_status-value);
 +
 + ehci_status-length = 8;
 + ehci_status-name = kstrdup(status, GFP_KERNEL);
 + WARN_ON(!ehci_status-name);
 +
 + of_update_property(ehci_node, ehci_status);
 + pr_err(%s: armada-375-xhci and orion-ehci are 
 incompatible for this SoC.\n,
 + __func__);
 + pr_err(Please fix your dts!\n);
 + pr_err(orion-ehci have been disabled by default...\n);
 +
 + }
 +
 + reg = readl(usb_cluster_base);
 + if (use_usb3)
 + reg |= USB2_PHY_CONFIG_ENABLE;
 + else
 + reg = ~USB2_PHY_CONFIG_ENABLE;
 + writel(reg, usb_cluster_base);

Am i right in saying this in the end is about enabling the USB2 phy or
a USB3 why? So why not just implement a phy driver?

  Andrew
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  

Re: [PATCH v8 2/2] usb-serial: Moxa UPORT 12XX/14XX/16XX driver

[Andrew Lunn]

 I hope it's OK that I submit the patch with the below changes to Greg
 for inclusion in v3.14. Let me know if you prefer to respin yourself.

Hi Johan

The patch looks good.

Acked-by: Andrew Lunn and...@lunn.ch

 Thanks for all your work with fixing up and mainlining this driver!

And thanks for all the reviews.

Andrew

 
 Happy new year,
 Johan
 
 
 diff --git a/drivers/usb/serial/mxuport.c b/drivers/usb/serial/mxuport.c
 index 4b7a5bf57526..17e4f40b6306 100644
 --- a/drivers/usb/serial/mxuport.c
 +++ b/drivers/usb/serial/mxuport.c
 @@ -229,13 +229,14 @@ static int mxuport_recv_ctrl_urb(struct usb_serial 
 *serial,
USB_CTRL_GET_TIMEOUT);
   if (status  0) {
   dev_err(serial-interface-dev,
 - %s - recv usb_control_msg failed. (%d)\n,
 + %s - usb_control_msg failed (%d)\n,
   __func__, status);
   return status;
   }
  
   if (status != size) {
 - dev_err(serial-interface-dev, %s - sort read (%d / %zd)\n,
 + dev_err(serial-interface-dev,
 + %s - short read (%d / %zd)\n,
   __func__, status, size);
   return -EIO;
   }
 @@ -260,13 +261,14 @@ static int mxuport_send_ctrl_data_urb(struct usb_serial 
 *serial,
USB_CTRL_SET_TIMEOUT);
   if (status  0) {
   dev_err(serial-interface-dev,
 - %s - send usb_control_msg failed. (%d)\n,
 + %s - usb_control_msg failed (%d)\n,
   __func__, status);
   return status;
   }
  
   if (status != size) {
 - dev_err(serial-interface-dev, %s - sort write (%d / %zd)\n,
 + dev_err(serial-interface-dev,
 + %s - short write (%d / %zd)\n,
   __func__, status, size);
   return -EIO;
   }
 @@ -974,8 +976,10 @@ static int mxuport_get_fw_version(struct usb_serial 
 *serial, u32 *version)
   /* Get firmware version from SDRAM */
   err = mxuport_recv_ctrl_urb(serial, RQ_VENDOR_GET_VERSION, 0, 0,
   ver_buf, 4);
 - if (err != 4)
 + if (err != 4) {
 + err = -EIO;
   goto out;
 + }
  
   *version = (ver_buf[0]  16) | (ver_buf[1]  8) | ver_buf[2];
   err = 0;
 @@ -1137,7 +1141,10 @@ static int mxuport_port_probe(struct usb_serial_port 
 *port)
   err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_INTERFACE,
   MX_INT_RS232,
   port-port_number);
 - return err;
 + if (err)
 + return err;
 +
 + return 0;
  }
  
  static int mxuport_alloc_write_urb(struct usb_serial *serial,
 @@ -1240,13 +1247,14 @@ static int mxuport_attach(struct usb_serial *serial)
   /*
* All data from the ports is received on the first bulk in
* endpoint, with a multiplex header. The second bulk in is
 -  * used for events. Start to read serial data from the device
 +  * used for events.
 +  *
 +  * Start to read from the device.
*/
   err = usb_serial_generic_submit_read_urbs(port0, GFP_KERNEL);
   if (err)
   return err;
  
 - /* Endpoints on Port1 is used for events */
   err = usb_serial_generic_submit_read_urbs(port1, GFP_KERNEL);
   if (err) {
   usb_serial_generic_close(port0);
 @@ -1304,11 +1312,8 @@ static void mxuport_break_ctl(struct tty_struct *tty, 
 int break_state)
  {
   struct usb_serial_port *port = tty-driver_data;
   struct usb_serial *serial = port-serial;
 - struct mxuport_port *mxport;
   int enable;
  
 - mxport = usb_get_serial_port_data(port);
 -
   if (break_state == -1) {
   enable = 1;
   dev_dbg(port-dev, %s - sending break\n, __func__);
 @@ -1384,11 +1389,6 @@ static struct usb_serial_driver *const 
 serial_drivers[] = {
  
  module_usb_serial_driver(serial_drivers, mxuport_idtable);
  
 -/*
 - *  Module Information
 - */
  MODULE_AUTHOR(Andrew Lunn and...@lunn.ch);
  MODULE_AUTHOR(supp...@moxa.com);
  MODULE_LICENSE(GPL);
 -
 -
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v8 2/2] usb-serial: Moxa UPORT 12XX/14XX/16XX driver

[Andrew Lunn]

Add a driver which supports the following Moxa USB to serial converters:
*   2 ports : UPort 1250, UPort 1250I
*   4 ports : UPort 1410, UPort 1450, UPort 1450I
*   8 ports : UPort 1610-8, UPort 1650-8
*  16 ports : UPort 1610-16, UPort 1650-16

The UPORT devices don't directy fit the USB serial model. USB serial
assumes a bulk in/out endpoint pair per serial port. Thus a dual port
USB serial device is expected to have two bulk in/out pairs. The Moxa
UPORT only has one pair for data transfer and places a header on each
transfer over the endpoint indicating for which port the transfer
relates to. There is a second endpoint pair for events, such as modem
control lines changing state, setting baud rates etc. Again, a
multiplexing header is used on these endpoints.

Some ports need to have a kfifo explicitily allocated since the
framework does not allocate one if there is no associated endpoints.
The framework will however free it on unload of the module.

All data transfers are made on port0, yet the locks are taken on PortN.
urb-context points to PortN, even though the URB is for port0.

Where possible, code from the generic driver is called. However
mxuport_process_read_urb_data() is mostly a cut/paste of
usb_serial_generic_process_read_urb().

The driver will attempt to load firmware from userspace and compare
the available version and the running version. If the available
version is newer, it will be download into RAM of the device and
started. This is optional and the driver appears to work O.K. with
older firmware in the devices ROM.

This driver is based on the MOXA driver and retains MOXAs copyright.

Signed-off-by: Andrew Lunn and...@lunn.ch

---

v1-v2

Remove debug module parameter.
Add missing \n to dev_dbg() strings.
Consistent dev_dbg(%s - message in lowercase\n, __func__);
Don't log failed allocations.
Remove defensive checks for NULL mx_port.
Use snprintf() instead of sprintf().
Use port-icount and usb_serial_generic_get_icount().
Break firmware download into smaller functions.
Don't DMA to/from buffers on the stack.
Use more of the generic write functions.
Make use of port_probe() and port_remove().
Indent the device structure.
Minor cleanups in mxuport_close()
__u8 - u8, __u16 - u16.
remove mxuport_wait_until_sent() and implemented mxuport_tx_empty()
Remove mxuport_write_room() and use the generic equivelent.
Remove unneeded struct mx_uart_config members
Make use of generic functions for receiving data and events.
Name mxport consistently.
Use usb_serial_generic_tiocmiwait()
Let line discipline handle TCFLSH ioctl.
Import contents of mxuport.h into mxuport.c
Use shifts and ORs to create version number.
Use port_number, not minor

Clarify the meaning of interface in mx_uart_config.
Remove buffer from mxuport_send_async_ctrl_urb().
No need to multiply USB_CTRL_SET_TIMEOUT by HZ.
Simplify buffer allocation and free.
Swap (un)throttle to synchronous interface, remove async code.
Pass usb_serial to mxuport_[recv|send]_ctrl_urb()
Add missing {} on if else statement.
Use unsigned char type, remove casts.
Replace constants with defines.
Add error handling when disabling HW flow control.
Verify port is open before passing it recieved data
Verify contents of received data  events.
Remove broken support for alternative baud rates.
Fix B0 and modem line handling. Remove uart_cfg structure.
Remove hold_reason, which was set, but never used.
s/is/if/
Move parts of port open into port probe.
Remove mxport-port and pass usb_serial_port instead.
Remove mxport-flags which is no longer used.
Allocate buffers before starting firmware download.
Remove redundent detected debug message.
Use devm_kzalloc() to simply memory handling.
Remove repeated debug message when sending break.
Implement mxuport_dtr_rts().
Add locking around mcr_state.
Remove unused lsr_state from mxuport_port.
Support 485 via official IOCTL and sysfs.
Use usleep_range() instread of mdelay().
Simplify the comments.
Use port0 as a template when setting up bulk out endpoints.
Set bulk_in_size for unused endpoints to 0.
Rebase to v3.12-rc2

v3-v4
Remove support for rs485 - No hardware to test it on.
Fix formatting of multi line comments.
Use HEADER_SIZE instead of hard coded 4.
Use EIO instread of ECOMM.
Drop use of spinlocks on  mxuport_{un}throttle().
Remove unneeded parenthesis.
Split mxuport_process_read_urb_event() into a number of functions.
Check for zero bytes of data.
Don't needlessly initialize variables.
Fold mxuport_port_init() into mxuport_port_open()
Correctly handle the on parameter in mxuport_dtr_rts().
Drop mxuport_get_serial_info and the now empty ioctl handler.
Splitup mxuport_set_termios() into smaller functions
Make use of C_* macros for cflags.
Terminate the firmware download on error.
Remove some unneeded dev_dbg()'s.
Let usb-serial core fill .write function in usb_serial_driver.
Change usb_serial_driver.name to match filename.
Move tiocmget() and tiocmset() next to each other.
Only one declaration per line

[PATCH v8 1/2] tty: Add C_CMSPAR(tty)

[Andrew Lunn]

Add the missing C_CMSPAR(tty) macro.

Signed-off-by: Andrew Lunn and...@lunn.ch
---
 include/linux/tty.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/tty.h b/include/linux/tty.h
index 97d660ed70c1..e53e90ed3f19 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -137,6 +137,7 @@ struct tty_bufhead {
 #define C_CLOCAL(tty)  _C_FLAG((tty), CLOCAL)
 #define C_CIBAUD(tty)  _C_FLAG((tty), CIBAUD)
 #define C_CRTSCTS(tty) _C_FLAG((tty), CRTSCTS)
+#define C_CMSPAR(tty)  _C_FLAG((tty), CMSPAR)
 
 #define L_ISIG(tty)_L_FLAG((tty), ISIG)
 #define L_ICANON(tty)  _L_FLAG((tty), ICANON)
-- 
1.8.5.1

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

Re: [PATCH v7 2/2] usb-serial: Moxa UPORT 12XX/14XX/16XX driver

[Andrew Lunn]

  +* endpoint, with a multiplex header. The second bulk in is
  +* used for events. Throw away all but the first two bulk in
  +* urbs.
  +*/
  +   for (i = 2; i  serial-num_bulk_in; ++i) {
  +   port = serial-port[i];
  +   for (j = 0; j  ARRAY_SIZE(port-read_urbs); ++j) {
  +   usb_free_urb(port-read_urbs[j]);
  +   kfree(port-bulk_in_buffers[j]);
  +   port-read_urbs[j] = NULL;
  +   port-bulk_in_buffers[j] = NULL;
  +   port-bulk_in_size = 0;
  +   }
  +   }
 
 Is this really necessary? Why not simple leave these urbs and buffers
 allocated (if there are any devices with more than to bulk-in
 endpoints at all)? You never submit them (and neither does usb-serial
 core since you override resume) so could be left allocated.

Hi Johan

It is not really necessary. The hardware i have has 4 bulk-in
endpoints, of which the driver only uses the first two. I've no idea
what the other two are used for. Freeing the buffers does save a
little bit of memory, but i could drop this code block and skip the
memory saved?

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

[PATCH v7 1/2] tty: Add C_CMSPAR(tty)

[Andrew Lunn]

Add the missing C_CMSPAR(tty) macro.

Signed-off-by: Andrew Lunn and...@lunn.ch
---
 include/linux/tty.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/tty.h b/include/linux/tty.h
index 97d660ed70c1..e53e90ed3f19 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -137,6 +137,7 @@ struct tty_bufhead {
 #define C_CLOCAL(tty)  _C_FLAG((tty), CLOCAL)
 #define C_CIBAUD(tty)  _C_FLAG((tty), CIBAUD)
 #define C_CRTSCTS(tty) _C_FLAG((tty), CRTSCTS)
+#define C_CMSPAR(tty)  _C_FLAG((tty), CMSPAR)
 
 #define L_ISIG(tty)_L_FLAG((tty), ISIG)
 #define L_ICANON(tty)  _L_FLAG((tty), ICANON)
-- 
1.8.5.1

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

[PATCH v7 2/2] usb-serial: Moxa UPORT 12XX/14XX/16XX driver

[Andrew Lunn]

Add a driver which supports the following Moxa USB to serial converters:
*   2 ports : UPort 1250, UPort 1250I
*   4 ports : UPort 1410, UPort 1450, UPort 1450I
*   8 ports : UPort 1610-8, UPort 1650-8
*  16 ports : UPort 1610-16, UPort 1650-16

The UPORT devices don't directy fit the USB serial model. USB serial
assumes a bulk in/out endpoint pair per serial port. Thus a dual port
USB serial device is expected to have two bulk in/out pairs. The Moxa
UPORT only has one pair for data transfer and places a header on each
transfer over the endpoint indicating for which port the transfer
relates to. There is a second endpoint pair for events, such as modem
control lines changing state, setting baud rates etc. Again, a
multiplexing header is used on these endpoints.

Some ports need to have a kfifo explicitily allocated since the
framework does not allocate one if there is no associated endpoints.
The framework will however free it on unload of the module.

All data transfers are made on port0, yet the locks are taken on PortN.
urb-context points to PortN, even though the URB is for port0.

Where possible, code from the generic driver is called. However
mxuport_process_read_urb_data() is mostly a cut/paste of
usb_serial_generic_process_read_urb().

The driver will attempt to load firmware from userspace and compare
the available version and the running version. If the available
version is newer, it will be download into RAM of the device and
started. This is optional and the driver appears to work O.K. with
older firmware in the devices ROM.

This driver is based on the MOXA driver and retains MOXAs copyright.

Signed-off-by: Andrew Lunn and...@lunn.ch

---

v1-v2

Remove debug module parameter.
Add missing \n to dev_dbg() strings.
Consistent dev_dbg(%s - message in lowercase\n, __func__);
Don't log failed allocations.
Remove defensive checks for NULL mx_port.
Use snprintf() instead of sprintf().
Use port-icount and usb_serial_generic_get_icount().
Break firmware download into smaller functions.
Don't DMA to/from buffers on the stack.
Use more of the generic write functions.
Make use of port_probe() and port_remove().
Indent the device structure.
Minor cleanups in mxuport_close()
__u8 - u8, __u16 - u16.
remove mxuport_wait_until_sent() and implemented mxuport_tx_empty()
Remove mxuport_write_room() and use the generic equivelent.
Remove unneeded struct mx_uart_config members
Make use of generic functions for receiving data and events.
Name mxport consistently.
Use usb_serial_generic_tiocmiwait()
Let line discipline handle TCFLSH ioctl.
Import contents of mxuport.h into mxuport.c
Use shifts and ORs to create version number.
Use port_number, not minor

Clarify the meaning of interface in mx_uart_config.
Remove buffer from mxuport_send_async_ctrl_urb().
No need to multiply USB_CTRL_SET_TIMEOUT by HZ.
Simplify buffer allocation and free.
Swap (un)throttle to synchronous interface, remove async code.
Pass usb_serial to mxuport_[recv|send]_ctrl_urb()
Add missing {} on if else statement.
Use unsigned char type, remove casts.
Replace constants with defines.
Add error handling when disabling HW flow control.
Verify port is open before passing it recieved data
Verify contents of received data  events.
Remove broken support for alternative baud rates.
Fix B0 and modem line handling. Remove uart_cfg structure.
Remove hold_reason, which was set, but never used.
s/is/if/
Move parts of port open into port probe.
Remove mxport-port and pass usb_serial_port instead.
Remove mxport-flags which is no longer used.
Allocate buffers before starting firmware download.
Remove redundent detected debug message.
Use devm_kzalloc() to simply memory handling.
Remove repeated debug message when sending break.
Implement mxuport_dtr_rts().
Add locking around mcr_state.
Remove unused lsr_state from mxuport_port.
Support 485 via official IOCTL and sysfs.
Use usleep_range() instread of mdelay().
Simplify the comments.
Use port0 as a template when setting up bulk out endpoints.
Set bulk_in_size for unused endpoints to 0.
Rebase to v3.12-rc2

v3-v4
Remove support for rs485 - No hardware to test it on.
Fix formatting of multi line comments.
Use HEADER_SIZE instead of hard coded 4.
Use EIO instread of ECOMM.
Drop use of spinlocks on  mxuport_{un}throttle().
Remove unneeded parenthesis.
Split mxuport_process_read_urb_event() into a number of functions.
Check for zero bytes of data.
Don't needlessly initialize variables.
Fold mxuport_port_init() into mxuport_port_open()
Correctly handle the on parameter in mxuport_dtr_rts().
Drop mxuport_get_serial_info and the now empty ioctl handler.
Splitup mxuport_set_termios() into smaller functions
Make use of C_* macros for cflags.
Terminate the firmware download on error.
Remove some unneeded dev_dbg()'s.
Let usb-serial core fill .write function in usb_serial_driver.
Change usb_serial_driver.name to match filename.
Move tiocmget() and tiocmset() next to each other.
Only one declaration per line

[PATCH v6 2/2] usb-serial: Moxa UPORT 12XX/14XX/16XX driver

[Andrew Lunn]

Add a driver which supports the following Moxa USB to serial converters:
*   2 ports : UPort 1250, UPort 1250I
*   4 ports : UPort 1410, UPort 1450, UPort 1450I
*   8 ports : UPort 1610-8, UPort 1650-8
*  16 ports : UPort 1610-16, UPort 1650-16

The UPORT devices don't directy fit the USB serial model. USB serial
assumes a bulk in/out endpoint pair per serial port. Thus a dual port
USB serial device is expected to have two bulk in/out pairs. The Moxa
UPORT only has one pair for data transfer and places a header on each
transfer over the endpoint indicating for which port the transfer
relates to. There is a second endpoint pair for events, such as modem
control lines changing state, setting baud rates etc. Again, a
multiplexing header is used on these endpoints.

Some ports need to have a kfifo explicitily allocated since the
framework does not allocate one if there is no associated endpoints.
The framework will however free it on unload of the module.

All data transfers are made on port0, yet the locks are taken on PortN.
urb-context points to PortN, even though the URB is for port0.

Where possible, code from the generic driver is called. However
mxuport_process_read_urb_data() is mostly a cut/paste of
usb_serial_generic_process_read_urb().

The driver will attempt to load firmware from userspace and compare
the available version and the running version. If the available
version is newer, it will be download into RAM of the device and
started. This is optional and the driver appears to work O.K. with
older firmware in the devices ROM.

This driver is based on the MOXA driver and retains MOXAs copyright.

Signed-off-by: Andrew Lunn and...@lunn.ch

---

v1-v2

Remove debug module parameter.
Add missing \n to dev_dbg() strings.
Consistent dev_dbg(%s - message in lowercase\n, __func__);
Don't log failed allocations.
Remove defensive checks for NULL mx_port.
Use snprintf() instead of sprintf().
Use port-icount and usb_serial_generic_get_icount().
Break firmware download into smaller functions.
Don't DMA to/from buffers on the stack.
Use more of the generic write functions.
Make use of port_probe() and port_remove().
Indent the device structure.
Minor cleanups in mxuport_close()
__u8 - u8, __u16 - u16.
remove mxuport_wait_until_sent() and implemented mxuport_tx_empty()
Remove mxuport_write_room() and use the generic equivelent.
Remove unneeded struct mx_uart_config members
Make use of generic functions for receiving data and events.
Name mxport consistently.
Use usb_serial_generic_tiocmiwait()
Let line discipline handle TCFLSH ioctl.
Import contents of mxuport.h into mxuport.c
Use shifts and ORs to create version number.
Use port_number, not minor

Clarify the meaning of interface in mx_uart_config.
Remove buffer from mxuport_send_async_ctrl_urb().
No need to multiply USB_CTRL_SET_TIMEOUT by HZ.
Simplify buffer allocation and free.
Swap (un)throttle to synchronous interface, remove async code.
Pass usb_serial to mxuport_[recv|send]_ctrl_urb()
Add missing {} on if else statement.
Use unsigned char type, remove casts.
Replace constants with defines.
Add error handling when disabling HW flow control.
Verify port is open before passing it recieved data
Verify contents of received data  events.
Remove broken support for alternative baud rates.
Fix B0 and modem line handling. Remove uart_cfg structure.
Remove hold_reason, which was set, but never used.
s/is/if/
Move parts of port open into port probe.
Remove mxport-port and pass usb_serial_port instead.
Remove mxport-flags which is no longer used.
Allocate buffers before starting firmware download.
Remove redundent detected debug message.
Use devm_kzalloc() to simply memory handling.
Remove repeated debug message when sending break.
Implement mxuport_dtr_rts().
Add locking around mcr_state.
Remove unused lsr_state from mxuport_port.
Support 485 via official IOCTL and sysfs.
Use usleep_range() instread of mdelay().
Simplify the comments.
Use port0 as a template when setting up bulk out endpoints.
Set bulk_in_size for unused endpoints to 0.
Rebase to v3.12-rc2

v3-v4
Remove support for rs485 - No hardware to test it on.
Fix formatting of multi line comments.
Use HEADER_SIZE instead of hard coded 4.
Use EIO instread of ECOMM.
Drop use of spinlocks on  mxuport_{un}throttle().
Remove unneeded parenthesis.
Split mxuport_process_read_urb_event() into a number of functions.
Check for zero bytes of data.
Don't needlessly initialize variables.
Fold mxuport_port_init() into mxuport_port_open()
Correctly handle the on parameter in mxuport_dtr_rts().
Drop mxuport_get_serial_info and the now empty ioctl handler.
Splitup mxuport_set_termios() into smaller functions
Make use of C_* macros for cflags.
Terminate the firmware download on error.
Remove some unneeded dev_dbg()'s.
Let usb-serial core fill .write function in usb_serial_driver.
Change usb_serial_driver.name to match filename.
Move tiocmget() and tiocmset() next to each other.
Only one declaration per line

[PATCH v6 1/2] tty: Add C_CMSPAR(tty)

[Andrew Lunn]

Add the missing C_CMSPAR(tty) macro.

Signed-off-by: Andrew Lunn and...@lunn.ch
---
 include/linux/tty.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/tty.h b/include/linux/tty.h
index 64f864651d86..8d1ba896070a 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -137,6 +137,7 @@ struct tty_bufhead {
 #define C_CLOCAL(tty)  _C_FLAG((tty), CLOCAL)
 #define C_CIBAUD(tty)  _C_FLAG((tty), CIBAUD)
 #define C_CRTSCTS(tty) _C_FLAG((tty), CRTSCTS)
+#define C_CMSPAR(tty)  _C_FLAG((tty), CMSPAR)
 
 #define L_ISIG(tty)_L_FLAG((tty), ISIG)
 #define L_ICANON(tty)  _L_FLAG((tty), ICANON)
-- 
1.8.4.rc3

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

Re: [PATCH v5 2/2] usb-serial: Moxa UPORT 12XX/14XX/16XX driver

[Andrew Lunn]

Hi Johan

Thanks for the quick response.

  +static int mxuport_dtr(struct usb_serial_port *port, int on)
  +{
  +   struct mxuport_port *mxport = usb_get_serial_port_data(port);
  +   int err;
  +
  +   mutex_lock(mxport-mutex);
  +
  +   if (on)
  +   mxport-mcr_state |= UART_MCR_DTR;
  +   else
  +   mxport-mcr_state = ~UART_MCR_DTR;
  +
  +   err = mxuport_set_mcr(port, mxport-mcr_state);
  +
  +   mutex_unlock(mxport-mutex);
  +
  +   return err;
  +}
 
 You should probably use SET_DTR (and rename the function
 mxuport_set_dtr).

I considered mxuport_set_dtr(), but it does not really fit with the
usb-serial naming. tty_port_operations has dtr_rts, not set_dtr_rts.
usb_serial_driver also has dtr_rts, not set_dtr_rts.

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

[PATCH v5 1/2] tty: Add C_CMSPAR(tty)

[Andrew Lunn]

Add the missing C_CMSPAR(tty) macro.

Signed-off-by: Andrew Lunn and...@lunn.ch
---
 include/linux/tty.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/tty.h b/include/linux/tty.h
index 64f864651d86..8d1ba896070a 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -137,6 +137,7 @@ struct tty_bufhead {
 #define C_CLOCAL(tty)  _C_FLAG((tty), CLOCAL)
 #define C_CIBAUD(tty)  _C_FLAG((tty), CIBAUD)
 #define C_CRTSCTS(tty) _C_FLAG((tty), CRTSCTS)
+#define C_CMSPAR(tty)  _C_FLAG((tty), CMSPAR)
 
 #define L_ISIG(tty)_L_FLAG((tty), ISIG)
 #define L_ICANON(tty)  _L_FLAG((tty), ICANON)
-- 
1.8.4.rc3

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

[PATCH v5 2/2] usb-serial: Moxa UPORT 12XX/14XX/16XX driver

[Andrew Lunn]

Add a driver which supports the following Moxa USB to serial converters:
*   2 ports : UPort 1250, UPort 1250I
*   4 ports : UPort 1410, UPort 1450, UPort 1450I
*   8 ports : UPort 1610-8, UPort 1650-8
*  16 ports : UPort 1610-16, UPort 1650-16

The UPORT devices don't directy fit the USB serial model. USB serial
assumes a bulk in/out endpoint pair per serial port. Thus a dual port
USB serial device is expected to have two bulk in/out pairs. The Moxa
UPORT only has one pair for data transfer and places a header on each
transfer over the endpoint indicating for which port the transfer
relates to. There is a second endpoint pair for events, such as modem
control lines changing state, setting baud rates etc. Again, a
multiplexing header is used on these endpoints.

Some ports need to have a kfifo explicitily allocated since the
framework does not allocate one if there is no associated endpoints.
The framework will however free it on unload of the module.

All data transfers are made on port0, yet the locks are taken on PortN.
urb-context points to PortN, even though the URB is for port0.

Where possible, code from the generic driver is called. However
mxuport_process_read_urb_data() is mostly a cut/paste of
usb_serial_generic_process_read_urb().

The driver will attempt to load firmware from userspace and compare
the available version and the running version. If the available
version is newer, it will be download into RAM of the device and
started. This is optional and the driver appears to work O.K. with
older firmware in the devices ROM.

This driver is based on the MOXA driver and retains MOXAs copyright.

Signed-off-by: Andrew Lunn and...@lunn.ch

---

v1-v2

Remove debug module parameter.
Add missing \n to dev_dbg() strings.
Consistent dev_dbg(%s - message in lowercase\n, __func__);
Don't log failed allocations.
Remove defensive checks for NULL mx_port.
Use snprintf() instead of sprintf().
Use port-icount and usb_serial_generic_get_icount().
Break firmware download into smaller functions.
Don't DMA to/from buffers on the stack.
Use more of the generic write functions.
Make use of port_probe() and port_remove().
Indent the device structure.
Minor cleanups in mxuport_close()
__u8 - u8, __u16 - u16.
remove mxuport_wait_until_sent() and implemented mxuport_tx_empty()
Remove mxuport_write_room() and use the generic equivelent.
Remove unneeded struct mx_uart_config members
Make use of generic functions for receiving data and events.
Name mxport consistently.
Use usb_serial_generic_tiocmiwait()
Let line discipline handle TCFLSH ioctl.
Import contents of mxuport.h into mxuport.c
Use shifts and ORs to create version number.
Use port_number, not minor

Clarify the meaning of interface in mx_uart_config.
Remove buffer from mxuport_send_async_ctrl_urb().
No need to multiply USB_CTRL_SET_TIMEOUT by HZ.
Simplify buffer allocation and free.
Swap (un)throttle to synchronous interface, remove async code.
Pass usb_serial to mxuport_[recv|send]_ctrl_urb()
Add missing {} on if else statement.
Use unsigned char type, remove casts.
Replace constants with defines.
Add error handling when disabling HW flow control.
Verify port is open before passing it recieved data
Verify contents of received data  events.
Remove broken support for alternative baud rates.
Fix B0 and modem line handling. Remove uart_cfg structure.
Remove hold_reason, which was set, but never used.
s/is/if/
Move parts of port open into port probe.
Remove mxport-port and pass usb_serial_port instead.
Remove mxport-flags which is no longer used.
Allocate buffers before starting firmware download.
Remove redundent detected debug message.
Use devm_kzalloc() to simply memory handling.
Remove repeated debug message when sending break.
Implement mxuport_dtr_rts().
Add locking around mcr_state.
Remove unused lsr_state from mxuport_port.
Support 485 via official IOCTL and sysfs.
Use usleep_range() instread of mdelay().
Simplify the comments.
Use port0 as a template when setting up bulk out endpoints.
Set bulk_in_size for unused endpoints to 0.
Rebase to v3.12-rc2

v3-v4
Remove support for rs485 - No hardware to test it on.
Fix formatting of multi line comments.
Use HEADER_SIZE instead of hard coded 4.
Use EIO instread of ECOMM.
Drop use of spinlocks on  mxuport_{un}throttle().
Remove unneeded parenthesis.
Split mxuport_process_read_urb_event() into a number of functions.
Check for zero bytes of data.
Don't needlessly initialize variables.
Fold mxuport_port_init() into mxuport_port_open()
Correctly handle the on parameter in mxuport_dtr_rts().
Drop mxuport_get_serial_info and the now empty ioctl handler.
Splitup mxuport_set_termios() into smaller functions
Make use of C_* macros for cflags.
Terminate the firmware download on error.
Remove some unneeded dev_dbg()'s.
Let usb-serial core fill .write function in usb_serial_driver.
Change usb_serial_driver.name to match filename.
Move tiocmget() and tiocmset() next to each other.
Only one declaration per line

Re: [PATCH] usb-serial: Moxa UPORT 12XX/14XX/16XX driver

[Andrew Lunn]

 Hopefully my comments will suffice as guidance for mxuport, but we could
 iterate the flow-control handling separate from the other changes if you
 want.

Hi Johan

I just posted v5. It would be good if you could take a look at the
flow-control changes first.

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

Re: [PATCH] usb-serial: Moxa UPORT 12XX/14XX/16XX driver

[Andrew Lunn]

Hi Johan

Thanks for the review.

I have a few questions, points to raise.

  +   /* Submit the vendor request */
  +   buf[0] = data_bits;
  +   buf[1] = parity;
  +   buf[2] = stop_bits;
  +   buf[3] = 0;
  +
  +   err = mxuport_send_ctrl_data_urb(serial, RQ_VENDOR_SET_LINE,
  +0, port-port_number, buf, 4);
  +
  +   if (err != 4)
  +   goto out;
  +
  +   if (C_BAUD(tty)) {
  +   if (old_termios  (old_termios-c_cflag  CBAUD) == B0) {
  +   /* Raise DTR/RTS */
  +   err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_DTR,
  +   1, port-port_number);
  +   if (err)
  +   goto out;
  +
  +   mxport-mcr_state |= (UART_MCR_DTR | UART_MCR_RTS);
  +   } else {
  +   /* Drop DTR/RTS */
  +   err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_DTR,
  +   0, port-port_number);
  +   if (err)
  +   goto out;
  +   mxport-mcr_state = ~(UART_MCR_DTR | UART_MCR_RTS);
  +   }
  +   err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_MCR,
  +   mxport-mcr_state,
  +   port-port_number);
  +   if (err)
  +   goto out;
  +   }
 
 This isn't quite right. You want to raise DTR and (possibly) RTS when
 transitioning from B0 (or when !old_termios), and drop DTR and RTS when
 B0 is requested. That is, something like:
 
   if (C_BAUD(tty)) {
 if (old_termios  old_termios-c_cflag  CBAUD == B0)
 /* raise DTR/RTS */
 } else {
 /* drop DTR/RTS */
 }

Which driver is a good example to copy. I looked at a few, and they
all seem to do this differently.

Maybe something you can put onto you long term TODO list if move most
of the logic for this into the generic code, and call the dtr_rts()
function of the driver?

  +static int mxuport_open(struct tty_struct *tty, struct usb_serial_port 
  *port)
  +{
  +   struct mxuport_port *mxport = usb_get_serial_port_data(port);
  +   struct usb_serial *serial = port-serial;
  +   int err;
  +
  +   err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_OPEN,
  +   1, port-port_number);
  +   if (err)
  +   return err;
  +
  +   /* Send vendor request - set receive host (enable) */
  +   err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_RX_HOST_EN,
  +   1, port-port_number);
 
 Do you want to enable reception before opening?
 
  +
  +   if (err) {
  +   mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_OPEN, 0,
  + port-port_number);
  +
  +   return err;
  +   }
  +
  +   /* Initial port termios */
  +   mxuport_set_termios(tty, port, NULL);
  +
  +   /* Initial private parameters of port */
  +   mxport-msr_state = 0;
 
 Again, why not use RQ_VENDOR_GET_MSR here?

Sorry, i should of put a note in the change log. I looked at this.
The problem is, i've no idea what RQ_VENDOR_GET_MSR returns. The Moxa
vendor driver never uses it. So setting it to 0 seems safer than
wrongly decoding what i get back from the hardware.

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

[PATCH] usb-serial: Moxa UPORT 12XX/14XX/16XX driver

[Andrew Lunn]

Add a driver which supports the following Moxa USB to serial converters:
*   2 ports : UPort 1250, UPort 1250I
*   4 ports : UPort 1410, UPort 1450, UPort 1450I
*   8 ports : UPort 1610-8, UPort 1650-8
*  16 ports : UPort 1610-16, UPort 1650-16

The UPORT devices don't directy fit the USB serial model. USB serial
assumes a bulk in/out endpoint pair per serial port. Thus a dual port
USB serial device is expected to have two bulk in/out pairs. The Moxa
UPORT only has one pair for data transfer and places a header on each
transfer over the endpoint indicating for which port the transfer
relates to. There is a second endpoint pair for events, just as modem
control lines changing state, setting baud rates etc. Again, a
multiplexing header is used on these endpoints.

Some ports need to have a kfifo explicitily allocated since the
framework does not allocate one if there is no associated endpoints.
The framework will however free it on unload of the module.

All data transfers are made on port0, yet the locks are taken on PortN.
urb-context points to PortN, even though the URB is for port0.

Where possible, code from the generic driver is called. However
mxuport_process_read_urb_data() is mostly a cut/paste of
usb_serial_generic_process_read_urb().

The driver will attempt to load firmware from userspace and compare
the available version and the running version. If the available
version is newer, it will be download into RAM of the device and
started. This is optional and the driver appears to work O.K. with
older firmware in the devices ROM.

This driver is based on the MOXA driver and retains MOXAs copyright.

Signed-off-by: Andrew Lunn and...@lunn.ch

---

v1-v2

Remove debug module parameter.
Add missing \n to dev_dbg() strings.
Consistent dev_dbg(%s - message in lowercase\n, __func__);
Don't log failed allocations.
Remove defensive checks for NULL mx_port.
Use snprintf() instead of sprintf().
Use port-icount and usb_serial_generic_get_icount().
Break firmware download into smaller functions.
Don't DMA to/from buffers on the stack.
Use more of the generic write functions.
Make use of port_probe() and port_remove().
Indent the device structure.
Minor cleanups in mxuport_close()
__u8 - u8, __u16 - u16.
remove mxuport_wait_until_sent() and implemented mxuport_tx_empty()
Remove mxuport_write_room() and use the generic equivelent.
Remove unneeded struct mx_uart_config members
Make use of generic functions for receiving data and events.
Name mxport consistently.
Use usb_serial_generic_tiocmiwait()
Let line discipline handle TCFLSH ioctl.
Import contents of mxuport.h into mxuport.c
Use shifts and ORs to create version number.
Use port_number, not minor

v2-v3

Clarify the meaning of interface in mx_uart_config.
Remove buffer from mxuport_send_async_ctrl_urb().
No need to multiply USB_CTRL_SET_TIMEOUT by HZ.
Simplify buffer allocation and free.
Swap (un)throttle to synchronous interface, remove async code.
Pass usb_serial to mxuport_[recv|send]_ctrl_urb()
Add missing {} on if else statement.
Use unsigned char type, remove casts.
Replace constants with defines.
Add error handling when disabling HW flow control.
Verify port is open before passing it recieved data
Verify contents of received data  events.
Remove broken support for alternative baud rates.
Fix B0 and modem line handling. Remove uart_cfg structure.
Remove hold_reason, which was set, but never used.
s/is/if/
Move parts of port open into port probe.
Remove mxport-port and pass usb_serial_port instead.
Remove mxport-flags which is no longer used.
Allocate buffers before starting firmware download.
Remove redundent detected debug message.
Use devm_kzalloc() to simply memory handling.
Remove repeated debug message when sending break.
Implement mxuport_dtr_rts().
Add locking around mcr_state.
Remove unused lsr_state from mxuport_port.
Support 485 via official IOCTL and sysfs.
Use usleep_range() instread of mdelay().
Simplify the comments.
Use port0 as a template when setting up bulk out endpoints.
Set bulk_in_size for unused endpoints to 0.
Rebase to v3.12-rc2

v3-v4

Remove support for rs485 - No hardware to test it on.
Fix formatting of multi line comments.
Use HEADER_SIZE instead of hard coded 4.
Use EIO instread of ECOMM.
Drop use of spinlocks on  mxuport_{un}throttle().
Remove unneeded parenthesis.
Split mxuport_process_read_urb_event() into a number of functions.
Check for zero bytes of data.
Don't needlessly initialize variables.
Fold mxuport_port_init() into mxuport_port_open()
Correctly handle the on parameter in mxuport_dtr_rts().
Drop mxuport_get_serial_info and the now empty ioctl handler.
Splitup mxuport_set_termios() into smaller functions
Make use of C_* macros for cflags.
Terminate the firmware download on error.
Remove some unneeded dev_dbg()'s.
Let usb-serial core fill .write function in usb_serial_driver.
Change usb_serial_driver.name to match filename.
Move tiocmget() and tiocmset() next to each other.
Only one declaration

Re: [PATCH v3] usb-serial: Moxa UPORT 12XX/14XX/16XX driver

[Andrew Lunn]

On Tue, Oct 15, 2013 at 08:30:51AM +, Danny Lin (林政易) wrote:
 Hi Andrew,
 
 We can do the interface testing but I want to know how to get the
 latest source. We need about 5 days to do the testing for all models
 and give you a testing report.

Hi Danny

Thanks for the offer of testing. However, I already removed the
support from my working tree.

How about we leave it out for the moment, get the driver accepted and
included in mainline, and then add back the interface support?

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

Re: [PATCH v3] usb-serial: Moxa UPORT 12XX/14XX/16XX driver

[Andrew Lunn]

On Wed, Oct 09, 2013 at 07:27:42PM +0200, Johan Hovold wrote:
 On Wed, Oct 09, 2013 at 12:57:45PM +0200, Johan Hovold wrote:
  On Wed, Sep 25, 2013 at 11:53:00AM +0200, Andrew Lunn wrote:
 
 [...]
 
   +#define MX_INT_RS232 0
   +#define MX_INT_2W_RS485  1
   +#define MX_INT_RS422 2
   +#define MX_INT_4W_RS485  3
 
   +static ssize_t show_four_wire(struct device *dev,
   +   struct device_attribute *attr, char *buf)
   +{
   + struct usb_serial_port *port = to_usb_serial_port(dev);
   + struct mxuport_port *mxport = usb_get_serial_port_data(port);
   +
   + return sprintf(buf, %i\n, mxport-four_wire_rs485);
  
  Use scnprintf (and PAGE_SIZE) even if it's a bool.
  
   +}
   +
   +static ssize_t store_four_wire(struct device *dev,
   +struct device_attribute *attr,
   +const char *buf, size_t size)
   +{
   + struct usb_serial_port *port = to_usb_serial_port(dev);
   + struct mxuport_port *mxport = usb_get_serial_port_data(port);
   +
   + if (strtobool(buf, mxport-four_wire_rs485)  0)
   + return -EINVAL;
   +
   + return size;
   +}
   +
   +static DEVICE_ATTR(4_wire_rs485, S_IWUSR | S_IRUGO,
   +show_four_wire, store_four_wire);
  
  Use DEVICE_ATTR_RW.
  
  You dropped RS422-support from v3. Was that intentional?
  
  Hmmm. I've been thinking a bit how best to handle this, and I think we
  should consider adding a SER_RS485_4_WIRE flag to serial_rs485 instead
  of having a custom attribute. That still leaves RS422, which could
  possibly be enabled using the RS485-ioctl as well (or we use a
  rs422-attribute for now).
  
  I'll get back to you on this.
 
 After giving this some more thought I don't really think the
 TIOCSRS485-ioctl interface is appropriate after all. The
 RQ_VENDOR_SET_INTERFACE-request doesn't just enable 2-wire-rs485-style
 signalling, but also changes which pins on the DB9-connector that are
 used.
 
 This should probably remain a device-specific parameter. We already have
 the MOXA mxser-driver with similar configuration options. This one uses
 a custom ioctl to set the interface, but I think we should use a sysfs
 parameter for this.
 
 What do you say about a simple interface-parameter representing the
 four MX_INT-values above? A string parameter would perhaps be even
 better if you combine it with an interface_available-parameter, for
 example:
 
   # cat /sys/bus/usb-serial/devices/ttyUSB0/interface_available
   rs232 rs422 rs485-2w rs485-4w
 
 for devices which can use all four modes.
 
 Sorry about the confusion.

Hi Johan

No problems.

I'm actually tempted to drop the support for anything over than RS232.
That is all the hardware i have can do. So anything we do add i cannot
test.

If somebody does have the necessary hardware and the need for these
extra modes we can come back to this.

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

[PATCH v3] usb-serial: Moxa UPORT 12XX/14XX/16XX driver

[Andrew Lunn]

Add a driver which supports the following Moxa USB to serial converters:
*   2 ports : UPort 1250, UPort 1250I
*   4 ports : UPort 1410, UPort 1450, UPort 1450I
*   8 ports : UPort 1610-8, UPort 1650-8
*  16 ports : UPort 1610-16, UPort 1650-16

The UPORT devices don't directy fit the USB serial model. USB serial
assumes a bulk in/out endpoint pair per serial port. Thus a dual port
USB serial device is expected to have two bulk in/out pairs. The Moxa
UPORT only has one pair for data transfer and places a header on each
transfer over the endpoint indicating for which port the transfer
relates to. There is a second endpoint pair for events, just as modem
control lines changing state, setting baud rates etc. Again, a
multiplexing header is used on these endpoints.

This difference to the model results in some additional code which
other drivers don't have:

Some ports need to have a kfifo explicitily allocated since the
framework does not allocate one if there is no associated endpoints.
The framework will however free it on unload of the module.

All data transfers are made on port0, yet the locks are taken on PortN.
urb-context points to PortN, even though the URB is for port0.

Where possible, code from the generic driver is called. However
mxuport_process_read_urb_data() is mostly a cut/paste of
usb_serial_generic_process_read_urb().

The driver will attempt to load firmware from userspace and compare
the available version and the running version. If the available
version is newer, it will be download into RAM of the device and
started. This is optional and the driver appears to work O.K. with
older firmware in the devices ROM.

This driver is based on the MOXA driver and retains MOXAs copyright.

Signed-off-by: Andrew Lunn and...@lunn.ch

---

v1-v2

Remove debug module parameter.
Add missing \n to dev_dbg() strings.
Consistent dev_dbg(%s - message in lowercase\n, __func__);
Don't log failed allocations.
Remove defensive checks for NULL mx_port.
Use snprintf() instead of sprintf().
Use port-icount and usb_serial_generic_get_icount().
Break firmware download into smaller functions.
Don't DMA to/from buffers on the stack.
Use more of the generic write functions.
Make use of port_probe() and port_remove().
Indent the device structure.
Minor cleanups in mxuport_close()
__u8 - u8, __u16 - u16.
remove mxuport_wait_until_sent() and implemented mxuport_tx_empty()
Remove mxuport_write_room() and use the generic equivelent.
Remove unneeded struct mx_uart_config members
Make use of generic functions for receiving data and events.
Name mxport consistently.
Use usb_serial_generic_tiocmiwait()
Let line discipline handle TCFLSH ioctl.
Import contents of mxuport.h into mxuport.c
Use shifts and ORs to create version number.
Use port_number, not minor

v2-v3

Clarify the meaning of interface in mx_uart_config.
Remove buffer from mxuport_send_async_ctrl_urb().
No need to multiply USB_CTRL_SET_TIMEOUT by HZ.
Simplify buffer allocation and free.
Swap (un)throttle to synchronous interface, remove async code.
Pass usb_serial to mxuport_[recv|send]_ctrl_urb()
Add missing {} on if else statement.
Use unsigned char type, remove casts.
Replace constants with defines.
Add error handling when disabling HW flow control.
Verify port is open before passing it recieved data
Verify contents of received data  events.
Remove broken support for alternative baud rates.
Fix B0 and modem line handling. Remove uart_cfg structure.
Remove hold_reason, which was set, but never used.
s/is/if/
Move parts of port open into port probe.
Remove mxport-port and pass usb_serial_port instead.
Remove mxport-flags which is no longer used.
Allocate buffers before starting firmware download.
Remove redundent detected debug message.
Use devm_kzalloc() to simply memory handling.
Remove repeated debug message when sending break.
Implement mxuport_dtr_rts().
Add locking around mcr_state.
Remove unused lsr_state from mxuport_port.
Support 485 via official IOCTL and sysfs.
Use usleep_range() instread of mdelay().
Simplify the comments.
Use port0 as a template when setting up bulk out endpoints.
Set bulk_in_size for unused endpoints to 0.
Rebase to v3.12-rc2

TODO
Custom resume function

---
 drivers/usb/serial/Kconfig   |   29 +
 drivers/usb/serial/Makefile  |1 +
 drivers/usb/serial/mxuport.c | 1611 ++
 3 files changed, 1641 insertions(+)
 create mode 100644 drivers/usb/serial/mxuport.c

diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig
index ddb9c51..36bfd48 100644
--- a/drivers/usb/serial/Kconfig
+++ b/drivers/usb/serial/Kconfig
@@ -472,6 +472,35 @@ config USB_SERIAL_MOS7840
  To compile this driver as a module, choose M here: the
  module will be called mos7840.  If unsure, choose N.
 
+config USB_SERIAL_MOXA_UPORT
+   tristate USB Moxa UPORT USB Serial Driver
+   ---help---
+ Say Y here if you want to use a MOXA UPort Serial hub

Re: [PATCH v2] usb-serial: Moxa UPORT 12XX/14XX/16XX driver

[Andrew Lunn]

  I need 115200, since that is what most of my development systems use
  for the serial console. Will the normal path, tty_get_baud_rate(tty),
  handle baud rates above 57600?
 
 Yes, cfsetospeed can be used up to 4Mbaud for example.
 
  The hardware is also capable of custom divisor although you don't
  pass the divisor you need, but the actual baud rate. The moxa vendor
  driver had a custom IOCTL call for that, which i ripped out, but i've
  not yet looked how to implement it correctly.
 
 Isn't that the way you do it today with RQ_VENDOR_SET_BAUD?

Yes, that part i know. I've only used custom divisors once, on an FTDI
chipset. As far as i remember, you pass the divisor you want, not the
baud rate. I've no idea what the base clock is, before the
divider. I've also no idea how the device maps the baud rate to a
divisor. So i would need to experiment with different values passed to
RQ_VENDOR_SET_BAUD and see what the hardware actually does.

For the moment i won't support it.
 
  So you are assuming the endpoints all have the same buffer size?  At
  least for the one device i have, that is true. But is it guaranteed?
  What would happen if the event endpoint actually had a smaller size?
 
 No, remember that we're setting up all ports to use the first bulk-out
 endpoint, so the fields can be retrieved from port0.

Doh! I've been away from this code too long. 
 
Thanks
Andrew
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] usb-serial: Moxa UPORT 12XX/14XX/16XX driver

[Andrew Lunn]

Hi Johan

Thanks for your quick comments. I hopefully have more time to work on
this code now, so v3 will not take as long as v2 did.

  All data transfers are made on port0, yet the locks are taken on PortN.
  urb-context points to PortN, even though the URL is for port0.
 
 You probably meant URB here.

Yep. Changelog fixed. 


  +/* This structure holds all of the local port information */
  +struct mxuport_port {
  +   int flags;  /* for async_struct and serial_struct flags
  +  field */
  +   int set_B0;
  +
  +   u8 mcr_state;   /* last MCR state */
  +
  +   u8 msr_state;   /* last MSR state */
  +   u8 lsr_state;   /* last LSR state */
  +   unsigned long hold_reason;
 
 You go to great lengths to update this hold_reason but I can't see that
 you ever use it?

Its left over from the old moxa vendor driver. It seems i ripped out
all the users of it. The vendor driver has some flow control between
the host and the device. The device has quite big buffers, both read
and write. So if the host is writing data out fast, it gets buffered,
rather than pushing back and slowing down the application generating
the data. Similarly, if the application is being slow reading what has
been received, it will get buffered in the device, rather than trigger
flow control over the serial link. For my application, using the 4
port device for consoles onto my ARM development systems, these
buffers are not an issue. My usage it interactive. However if somebody
were to run PPP over the ports, they are likely to see quite bad
buffer bloat issues.

I will probably rip out the rest of this hold_reason code. If the
buffers do turn out to be an issue for somebody, we can see how to
cleanly add back the flow control. Its messy because of the
multiplexing.
 
  +
  +   struct usb_serial_port *port;   /* loop back to the owner of this
  +  object */
  +   struct mx_uart_config *uart_cfg;/* configuration of UART */
  +};
  +
  +/* Configuration of UART */
  +struct mx_uart_config {
  +   u8 data_bits;   /* 5..8 - data bits per character   */
  +   u8 parity;  /* parity settings  */
  +   u8 stop_bits;   /* stop bits settings   */
  +   u8 flow_ctrl;   /* flow control settings*/
  +   int interface;  /* interface is defined */
 
 This description isn't very clear.

Agreed. After reading the code, it indicates RS232, 485 etc. I will
change the comment.
 
  +/*
  + *  mxuport_control_callback
  + *
  + *  This is the callback function for when we have finished sending
  + *  control urb on the control pipe.
  + */
  +static void mxuport_control_callback(struct urb *urb)
  +{
  +   struct usb_ctrlrequest *req = (struct usb_ctrlrequest *)urb-context;
 
 You should pass the usb_serial_port (or usb_serial) rather than the
 ctrlrequest as context in order to get uniform and more informative
 error messages if anything goes wrong. You can still access the
 ctrlrequest as urb-setup_packet.
 
  +   struct device *dev = urb-dev-dev;
 
 Use port-dev (or serial-interface-dev).
 
  +
  +   switch (urb-status) {
  +   case 0:
  +   break;
  +
  +   case -ECONNRESET:
  +   case -ENOENT:
  +   case -ESHUTDOWN:
  +   /* This urb is terminated, clean up */
  +   dev_dbg(dev, %s - urb shutting down with status: %d\n,
  +   __func__, urb-status);
  +   break;
  +
  +   default:
  +   dev_err(dev, %s - nonzero control status received: %d\n,
  +   __func__, urb-status);
  +   break;
  +   }
  +
  +   kfree(req);
  +   usb_free_urb(urb);
 
 You don't free the transfer_buffer even though you allow control
 requests to pass a buffer. You don't use that part of the interface at
 the moment, so either disallow buffers to be passed below or make sure
 to free them here.

I will remove data and size. As you say, the current callers don't
actually pass any data.

  +}
  +
  +/*
  + * mxuport_send_aysnc_ctrl_urb - send asynchronously a vendor request with
  + * data
  + *
  + * This function writes the given buffer out to the control pipe, but
  + * does not wait around for it to complete.
  + */
  +static void mxuport_send_async_ctrl_urb(struct usb_device *udev,
  +   u8 request,
  +   u16 value,
  +   u16 index, u8 *data, size_t size)

...

 If you think this could be useful in other contexts than
 throttle/unthrottle where you currently call it while holding a
 spinlock, I'd pass the memory flag as a parameter (instead of always
 using GFP_ATOMIC).
 
At the moment i don't see any other potential users. We can add the
memory flag parameter later if needed.
 
  +   int status = usb_control_msg(udev,
  +usb_rcvctrlpipe(udev, 0),
  + 

[Pull request] Fix moxa firmware filenames

[Andrew Lunn]

Hi David, Ben.

I messed up with the Moxa firmware. I have the filenames wrong, due to
one too many decimal to hex conversions of the USB product ID. I could
work around this in the driver, but it would be a lot better to rename
the files. Here is a pull request to fix the issue.

Thanks
Andrew



The following changes since commit e7c85b24a5e5107e9911c6b80f536b6bbc5d465d:

  moxa: Fix firmware file names. (2013-09-05 13:18:07 +0200)

are available in the git repository at:

  g...@github.com:lunn/linux-firmware.git moxa

for you to fetch changes up to e7c85b24a5e5107e9911c6b80f536b6bbc5d465d:

  moxa: Fix firmware file names. (2013-09-05 13:18:07 +0200)


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

Re: [PULL] Moxa UPort firmware

[Andrew Lunn]

On Thu, Aug 08, 2013 at 11:00:20PM +0200, Ben Hutchings wrote:
 On Thu, 2013-08-08 at 12:20 +0200, Andrew Lunn wrote:
  Hi Ben, David
  
  Here is a pull request for firmware for Moxa USB-Serial hub devices.
  
  Thanks
  Andrew
  
  The following changes since commit 931e4469dc254df66a2c990ff1a8723685759eb4:
  
radeon: add ucode for KABINI GPUs (2013-07-28 22:39:47 +0100)
  
  are available in the git repository at:
  
https://github.com/lunn/linux-firmware.git moxa
  
  for you to fetch changes up to 6eed67a9f372a5ade30431af9af0aff8df6f678f:
  
moxa: Add firmware for some of the Moxa USB-Serial hubs (2013-08-08 
  12:04:52 +0200)
 [...]
 
 Pulled and pushed out, thanks.

Great thanks.

Is it too late to add the Acked-by from Moxa? It would help make it
clear that Moxa fully agree and helped.

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

[PULL] Moxa UPort firmware

[Andrew Lunn]

Hi Ben, David

Here is a pull request for firmware for Moxa USB-Serial hub devices.

Thanks
Andrew

The following changes since commit 931e4469dc254df66a2c990ff1a8723685759eb4:

  radeon: add ucode for KABINI GPUs (2013-07-28 22:39:47 +0100)

are available in the git repository at:

  https://github.com/lunn/linux-firmware.git moxa

for you to fetch changes up to 6eed67a9f372a5ade30431af9af0aff8df6f678f:

  moxa: Add firmware for some of the Moxa USB-Serial hubs (2013-08-08 12:04:52 
+0200)


Andrew Lunn (1):
  moxa: Add firmware for some of the Moxa USB-Serial hubs

 WHENCE|   32 
 moxa/moxa-04e2.fw |  Bin 0 - 33681 bytes
 moxa/moxa-04e3.fw |  Bin 0 - 33685 bytes
 moxa/moxa-0582.fw |  Bin 0 - 33521 bytes
 moxa/moxa-05aa.fw |  Bin 0 - 33521 bytes
 moxa/moxa-05ab.fw |  Bin 0 - 33525 bytes
 moxa/moxa-064d.fw |  Bin 0 - 33529 bytes
 moxa/moxa-0652.fw |  Bin 0 - 33525 bytes
 moxa/moxa-0675.fw |  Bin 0 - 33529 bytes
 moxa/moxa-067a.fw |  Bin 0 - 33525 bytes
 10 files changed, 32 insertions(+)
 create mode 100644 moxa/moxa-04e2.fw
 create mode 100644 moxa/moxa-04e3.fw
 create mode 100644 moxa/moxa-0582.fw
 create mode 100644 moxa/moxa-05aa.fw
 create mode 100644 moxa/moxa-05ab.fw
 create mode 100644 moxa/moxa-064d.fw
 create mode 100644 moxa/moxa-0652.fw
 create mode 100644 moxa/moxa-0675.fw
 create mode 100644 moxa/moxa-067a.fw
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] usb-serial: Moxa UPORT 12XX/14XX/16XX driver

[Andrew Lunn]

On Sun, May 26, 2013 at 06:04:24PM +0200, Johan Hovold wrote:
 On Sun, May 26, 2013 at 01:00:51PM +0200, Andrew Lunn wrote:
  +/*
  + * mxuport_write_room
  + *
  + * Return how much space is available in the buffer.
  + */
  +static int mxuport_write_room(struct tty_struct *tty)
  +{
  +   struct usb_serial_port *port = tty-driver_data;
  +   unsigned long flags;
  +   int room;
  +
  +   spin_lock_irqsave(port-lock, flags);
  +   room = kfifo_avail(port-write_fifo);
  +   spin_unlock_irqrestore(port-lock, flags);
  +
  +   dev_dbg(port-dev, %s - returns %d\n, __func__, room);
  +   return room;
  +}


 
 It seems it could be possible to reuse the generic write implementations
 of all functions but prepare_write_buffer above rather than the modified
 copies.

Hi Johan

If there a reason why usb_serial_generic_write_room() is not exported
as a GPL symbol. It looks like i should be able to use it once i sort
my 'fake' ports/endpoints out.

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

Re: [PATCH 2/2] usb-serial: Moxa UPORT 12XX/14XX/16XX driver

[Andrew Lunn]

On Tue, May 28, 2013 at 11:08:54PM +0200, Johan Hovold wrote:
 On Tue, May 28, 2013 at 09:41:08PM +0200, Andrew Lunn wrote:
  On Sun, May 26, 2013 at 06:04:24PM +0200, Johan Hovold wrote:
   On Sun, May 26, 2013 at 01:00:51PM +0200, Andrew Lunn wrote:
+/*
+ * mxuport_write_room
+ *
+ * Return how much space is available in the buffer.
+ */
+static int mxuport_write_room(struct tty_struct *tty)
+{
+   struct usb_serial_port *port = tty-driver_data;
+   unsigned long flags;
+   int room;
+
+   spin_lock_irqsave(port-lock, flags);
+   room = kfifo_avail(port-write_fifo);
+   spin_unlock_irqrestore(port-lock, flags);
+
+   dev_dbg(port-dev, %s - returns %d\n, __func__, room);
+   return room;
+}
  
  
   
   It seems it could be possible to reuse the generic write implementations
   of all functions but prepare_write_buffer above rather than the modified
   copies.
  
  Hi Johan
  
  If there a reason why usb_serial_generic_write_room() is not exported
  as a GPL symbol. It looks like i should be able to use it once i sort
  my 'fake' ports/endpoints out.
 
 No, I don't think so, but perhaps it should be. Most usb-serial drivers
 still use it. Simply leave the operation unset and usb-serial core will
 set it for you.

Hi Johan

Ah, i fell into the trap. I grep'd for usb_serial_generic_write_room
and did not get any hits. The code is obfusticated/grep unfriendly:

set_to_generic_if_null(device, write_room);

I will leave it unset.

  Andrew

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

[PATCH 0/2] MOXA UPORT Serial USB driver

[Andrew Lunn]

These two patches add support for MOXA 12XX/14XX/16XX USB serial
devices. The first patch exports a function from the generic usb
serial driver, which the driver in the second patch would like to use.

Andrew Lunn (2):
  USB: Serial: export usb_serial_generic_submit_read_urb()
  usb-serial: Moxa UPORT 12XX/14XX/16XX driver

 drivers/usb/serial/Kconfig   |   30 +
 drivers/usb/serial/Makefile  |1 +
 drivers/usb/serial/generic.c |5 +-
 drivers/usb/serial/mxuport.c | 2005 ++
 drivers/usb/serial/mxuport.h |  184 
 include/linux/usb/serial.h   |2 +
 6 files changed, 2225 insertions(+), 2 deletions(-)
 create mode 100644 drivers/usb/serial/mxuport.c
 create mode 100644 drivers/usb/serial/mxuport.h

-- 
1.7.10.4

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

[PATCH 1/2] USB: Serial: export usb_serial_generic_submit_read_urb()

[Andrew Lunn]

USB Serial drivers can currently use the generic function
usb_serial_generic_submit_read_urbs() to submit a group of urbs when
starting up the device. The read bulk callback needs to be able to
submit a single urb. usb_serial_generic_submit_read_urb() does this,
but it is not exported and so only usable within generic functions,
in particular usb_serial_generic_read_bulk_callback().

Export this function so drivers can use it in there own read bulk
callback routine.

Signed-off-by: Andrew Lunn and...@lunn.ch
---
 drivers/usb/serial/generic.c |5 +++--
 include/linux/usb/serial.h   |2 ++
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c
index 297665f..13e9b3b 100644
--- a/drivers/usb/serial/generic.c
+++ b/drivers/usb/serial/generic.c
@@ -253,8 +253,8 @@ int usb_serial_generic_chars_in_buffer(struct tty_struct 
*tty)
 }
 EXPORT_SYMBOL_GPL(usb_serial_generic_chars_in_buffer);
 
-static int usb_serial_generic_submit_read_urb(struct usb_serial_port *port,
-   int index, gfp_t mem_flags)
+int usb_serial_generic_submit_read_urb(struct usb_serial_port *port,
+  int index, gfp_t mem_flags)
 {
int res;
 
@@ -276,6 +276,7 @@ static int usb_serial_generic_submit_read_urb(struct 
usb_serial_port *port,
 
return 0;
 }
+EXPORT_SYMBOL_GPL(usb_serial_generic_submit_read_urb);
 
 int usb_serial_generic_submit_read_urbs(struct usb_serial_port *port,
gfp_t mem_flags)
diff --git a/include/linux/usb/serial.h b/include/linux/usb/serial.h
index b9b0f7b4..a85e35b 100644
--- a/include/linux/usb/serial.h
+++ b/include/linux/usb/serial.h
@@ -337,6 +337,8 @@ extern int usb_serial_generic_get_icount(struct tty_struct 
*tty,
struct serial_icounter_struct *icount);
 extern int usb_serial_generic_register(void);
 extern void usb_serial_generic_deregister(void);
+extern int usb_serial_generic_submit_read_urb(struct usb_serial_port *port,
+ int index, gfp_t mem_flags);
 extern int usb_serial_generic_submit_read_urbs(struct usb_serial_port *port,
 gfp_t mem_flags);
 extern void usb_serial_generic_process_read_urb(struct urb *urb);
-- 
1.7.10.4

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

Re: [PATCH 2/2] usb-serial: Moxa UPORT 12XX/14XX/16XX driver

[Andrew Lunn]

On Sun, May 26, 2013 at 06:04:24PM +0200, Johan Hovold wrote:
 On Sun, May 26, 2013 at 01:00:51PM +0200, Andrew Lunn wrote:
  Add a driver which supports the following Moxa USB to serial converters:
  *   2 ports : UPort 1250, UPort 1250I
  *   4 ports : UPort 1410, UPort 1450, UPort 1450I
  *   8 ports : UPort 1610-8, UPort 1650-8
  *  16 ports : UPort 1610-16, UPort 1650-16
  
  The UPORT devices don't directy fit the USB serial model. USB serial
  assumes a bulk in/out endpoint pair per serial port. Thus a dual port
  USB serial device is expected to have two bulk in/out pairs. The Moxa
  UPORT only has one pair for data transfer and places a header on each
  transfer over the endpoint indicating for which port the transfer
  relates to. There is a second endpoint pair for events, just as modem
  control lines changing state, setting baud rates etc. Again, a
  multiplexing header is used on these endpoints.
 
  This difference to the model results in some additional code which
  other drivers don't have:
  
  Some ports need to have a kfifo explicitily allocated since the
  framework does not allocate one if there is no associated endpoints.
  The framework will however free it on unload of the module.
  
  All data transfers are made on port0, yet the locks are taken on PortN.
  urb-context points to PortN, even though the URL is for port0.
 
 First, thanks for submitting the driver.

And thanks for the quick review.

 As you mention, it does not fit
 the usb-serial model directly, but it seems that there's still quite a
 bit more of generic code that could be reused rather than copied or
 reimplemented.

I will take another look. I was not happy with cut/pasting code from
the generic driver, so tried to minimize it. I will take a deeper look
at your suggestions with the endpoints. 
 
 Consider my comments as a first round of review, there's probably some
 things I've missed and more minor things I'll get back to later.

Sure, no problem. 
 
  The driver will attempt to load firmware from userspace and compare
  the available version and the running version. If the available
  version is newer, it will be download into RAM of the device and
  started. This is optional and currently the firmware blobs are not yet
  in linux-firmware. However MOXA have agreed that the blobs can be
  distributed.
  
  This driver is based on the MOXA driver and retains MOXAs copyright.
 
 The driver is also quite heavily based on the generic driver and much
 code is copied more or less verbatim. Please make sure to mention in
 this in the header as well.

O.K.

Also, most of the paranoid checks for NULL pointers and closed ports
is from the original MOXA driver. Maybe in the past they have had
problems. I've already take out a lot of dead code, and converted to
generic where i could. I'm happy to continue this.

  +/* Global variables */
  +static bool debug;
 
 Please drop the debug module parameter. usb_serial_debug_data is
 supposed to be controlled using dynamic debugging.

O.K.
 
  +
  +/* Table of devices that work with this driver */
  +static struct usb_device_id mxuport_idtable[] = {
  +   {USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1250_PID)},
  +   {USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1251_PID)},
  +   {USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1410_PID)},
  +   {USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1450_PID)},
  +   {USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1451_PID)},
  +   {USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1618_PID)},
  +   {USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1658_PID)},
  +   {USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1613_PID)},
  +   {USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1653_PID)},
  +   {}  /* Terminating entry */
  +};
  +
  +MODULE_DEVICE_TABLE(usb, mxuport_idtable);
  +
  +/*
  + * mxuport_prepare_write_buffer - fill in the buffer, ready for
  + * sending
  + *
  + * Add a four byte header containing the port number and the number of
  + * bytes of data in the message. Return the number of bytes in the
  + * buffer.
  + */
  +static int mxuport_prepare_write_buffer(struct usb_serial_port *port,
  +   void *dest, size_t size)
  +{
  +   struct mxuport_port *mx_port = usb_get_serial_port_data(port);
  +   unsigned char *buf = dest;
  +   int count;
  +
  +   count = kfifo_out_locked(port-write_fifo, buf + 4, size - 4,
  +port-lock);
  +
  +   put_unaligned_be16(mx_port-portno, buf);
  +   put_unaligned_be16(count, buf + 2);
  +
  +   dev_dbg(port-dev, %s - port %d, size %d count %d, __func__,
  +   mx_port-portno, size, count);
  +
  +   return count + 4;
  +}
  +
  +/*
  + * mxuport_write_start - kick off an URB write
  + * @port:  Pointer to the struct usb_serial_port data
  + *
  + * Returns zero on success, or a negative errno value
  + */
  +static int mxuport_write_start(struct usb_serial_port *port)
  +{
  +   struct usb_serial *serial = port-serial;
  +   struct urb *urb;
  +   int count, result

Re: [PATCH 2/2] USB: EHCI: make ehci-orion a separate driver

[Andrew Lunn]

On Mon, Feb 18, 2013 at 05:34:35PM -0500, Alan Stern wrote:
 On Fri, 15 Feb 2013, Arnd Bergmann wrote:
 
  From: Manjunath Goudar manjunath.gou...@linaro.org
  
  With the multiplatform changes in arm-soc tree, it becomes
  possible to enable the mvebu platform (which uses
  ehci-orion) at the same time as other platforms that require
  a conflicting EHCI bus glue. At the moment, this results
  in a warning like
  
  drivers/usb/host/ehci-hcd.c:1297:0: warning: PLATFORM_DRIVER redefined 
  [enabled by default]
  drivers/usb/host/ehci-hcd.c:1277:0: note: this is the location of the 
  previous definition
  drivers/usb/host/ehci-orion.c:334:31: warning: 'ehci_orion_driver' defined 
  but not used [-Wunused-variable]
  
  and an ehci driver that only works on one of them.
  
  With the infrastructure added by Alan Stern in patch 3e0232039
  USB: EHCI: prepare to make ehci-hcd a library module, we can
  avoid this problem by turning a bus glue into a separate
  module, as we do here for the orion bus glue.
 
  --- a/drivers/usb/host/Makefile
  +++ b/drivers/usb/host/Makefile
  @@ -30,6 +30,7 @@ obj-$(CONFIG_USB_EHCI_MXC)+= ehci-mxc.o
   
   obj-$(CONFIG_USB_OXU210HP_HCD) += oxu210hp-hcd.o
   obj-$(CONFIG_USB_EHCI_HCD_VT8500)+= ehci-vt8500.o
  +obj-$(CONFIG_USB_EHCI_HCD_ORION)+= ehci-orion.o
 
 Both of these two new lines should be formatted like the other lines in
 this file (i.e., with tabs at the corresponding places), and they
 should come before the OXU210HP_HCD entry so that they are next to the
 other EHCI-related lines.
 
  --- a/drivers/usb/host/ehci-orion.c
  +++ b/drivers/usb/host/ehci-orion.c
  @@ -17,6 +17,13 @@
   #include linux/of.h
   #include linux/of_device.h
   #include linux/of_irq.h
  +#include linux/usb.h
  +#include linux/usb/hcd.h
  +#include linux/io.h
  +#include linux/dma-mapping.h
 
 Is this line really needed?
 
  @@ -34,6 +41,17 @@
   #define USB_PHY_IVREF_CTRL 0x440
   #define USB_PHY_TST_GRP_CTRL   0x450
   
  +#define DRIVER_DESC EHCI orion driver
  +
  +static const char hcd_name[] = ehci-orion;
  +
  +static struct hc_driver __read_mostly ehci_orion_hc_driver;
  +
  +static const struct ehci_driver_overrides orion_overrides __initdata = {
  +   .reset = ehci_setup,
  +};
 
 This is not necessary; ehci_setup is the default value anyway.  This 
 structure can be omitted.
 
  @@ -323,8 +296,6 @@ static int __exit ehci_orion_drv_remove(struct 
  platform_device *pdev)
  return 0;
   }
   
  -MODULE_ALIAS(platform:orion-ehci);
  -
   static const struct of_device_id ehci_orion_dt_ids[] = {
  { .compatible = marvell,orion-ehci, },
  {},
  @@ -336,8 +307,31 @@ static struct platform_driver ehci_orion_driver = {
  .remove = __exit_p(ehci_orion_drv_remove),
  .shutdown   = usb_hcd_platform_shutdown,
  .driver = {
  -   .name   = orion-ehci,
  +   .name   = hcd_name,
 
 Is this really what you want -- changing the driver name from 
 orion-ehci to ehci-orion?  Is that liable to cause trouble?
 
  +MODULE_DESCRIPTION(DRIVER_DESC);
  +MODULE_ALIAS(platform:ehci-orion);
 
 And is this really what you want -- changing the alias from 
 platform:orion-ehci to platform:ehci-orion?

Hi Manjunath

I can confirm that this breaks non DT based kirkwood systems. The
driver does not get loaded.

Sorry for not testing and finding this case earlier, i just tested a
DT based system.

GregKH: Please can you drop this patch from usb-next. It breaks more
than it fixes.

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

Re: [PATCH 2/2] USB: EHCI: make ehci-orion a separate driver

[Andrew Lunn]

On Fri, Feb 15, 2013 at 11:12:29PM +0100, Arnd Bergmann wrote:
 From: Manjunath Goudar manjunath.gou...@linaro.org
 
 With the multiplatform changes in arm-soc tree, it becomes
 possible to enable the mvebu platform (which uses
 ehci-orion) at the same time as other platforms that require
 a conflicting EHCI bus glue. At the moment, this results
 in a warning like
 
 drivers/usb/host/ehci-hcd.c:1297:0: warning: PLATFORM_DRIVER redefined 
 [enabled by default]
 drivers/usb/host/ehci-hcd.c:1277:0: note: this is the location of the 
 previous definition
 drivers/usb/host/ehci-orion.c:334:31: warning: 'ehci_orion_driver' defined 
 but not used [-Wunused-variable]
 
 and an ehci driver that only works on one of them.
 
 With the infrastructure added by Alan Stern in patch 3e0232039
 USB: EHCI: prepare to make ehci-hcd a library module, we can
 avoid this problem by turning a bus glue into a separate
 module, as we do here for the orion bus glue.
 
 Signed-off-by: Manjunath Goudar manjunath.gou...@linaro.org
 Signed-off-by: Arnd Bergmann a...@arndb.de
 Cc: Jason Cooper ja...@lakedaemon.net
 Cc: Andrew Lunn and...@lunn.ch
 ---
  drivers/usb/host/Kconfig  |  8 
  drivers/usb/host/Makefile |  1 +
  drivers/usb/host/ehci-hcd.c   |  6 +--
  drivers/usb/host/ehci-orion.c | 90 
 ---
  4 files changed, 52 insertions(+), 53 deletions(-)

Tested-by: Andrew Lunn and...@lunn.ch

I booted with this patch on a Kirkwood based QNAP NAS box. The USB bus
enumerated as expected.

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

[PATCH v2 2/2] ARM: Kirkwood: Convert all DT boards to EHCI via DT.

[Andrew Lunn]

Now that the EHCI driver has DT support, drop old style configuration
of it and add DT in its place. Since all the boards enable the EHCI,
enable it by default in kirkwood.dtsi. Any new boards which don't have
USB can specifically disable it.

Signed-off-by: Andrew Lunn and...@lunn.ch
---
 arch/arm/boot/dts/kirkwood.dtsi   |7 +++
 arch/arm/mach-kirkwood/board-dnskw.c  |1 -
 arch/arm/mach-kirkwood/board-dockstar.c   |1 -
 arch/arm/mach-kirkwood/board-dreamplug.c  |1 -
 arch/arm/mach-kirkwood/board-dt.c |1 +
 arch/arm/mach-kirkwood/board-goflexnet.c  |1 -
 arch/arm/mach-kirkwood/board-ib62x0.c |1 -
 arch/arm/mach-kirkwood/board-iconnect.c   |1 -
 arch/arm/mach-kirkwood/board-iomega_ix2_200.c |2 --
 arch/arm/mach-kirkwood/board-km_kirkwood.c|1 -
 arch/arm/mach-kirkwood/board-lsxl.c   |1 -
 arch/arm/mach-kirkwood/board-ts219.c  |1 -
 12 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/arch/arm/boot/dts/kirkwood.dtsi b/arch/arm/boot/dts/kirkwood.dtsi
index 4e5b815..4fc7a8e 100644
--- a/arch/arm/boot/dts/kirkwood.dtsi
+++ b/arch/arm/boot/dts/kirkwood.dtsi
@@ -77,6 +77,13 @@
status = okay;
};
 
+   ehci@5 {
+   compatible = marvell,orion-ehci;
+   reg = 0x5 0x1000;
+   interrupts = 19;
+   status = okay;
+   };
+
sata@8 {
compatible = marvell,orion-sata;
reg = 0x8 0x5000;
diff --git a/arch/arm/mach-kirkwood/board-dnskw.c 
b/arch/arm/mach-kirkwood/board-dnskw.c
index 43d16d6..2ac6c60 100644
--- a/arch/arm/mach-kirkwood/board-dnskw.c
+++ b/arch/arm/mach-kirkwood/board-dnskw.c
@@ -78,7 +78,6 @@ void __init dnskw_init(void)
 {
kirkwood_mpp_conf(dnskw_mpp_config);
 
-   kirkwood_ehci_init();
kirkwood_ge00_init(dnskw_ge00_data);
 
/* Register power-off GPIO. */
diff --git a/arch/arm/mach-kirkwood/board-dockstar.c 
b/arch/arm/mach-kirkwood/board-dockstar.c
index f2fbb02..e94782d 100644
--- a/arch/arm/mach-kirkwood/board-dockstar.c
+++ b/arch/arm/mach-kirkwood/board-dockstar.c
@@ -55,7 +55,6 @@ void __init dockstar_dt_init(void)
if (gpio_request(29, USB Power Enable) != 0 ||
gpio_direction_output(29, 1) != 0)
pr_err(can't setup GPIO 29 (USB Power Enable)\n);
-   kirkwood_ehci_init();
 
kirkwood_ge00_init(dockstar_ge00_data);
 }
diff --git a/arch/arm/mach-kirkwood/board-dreamplug.c 
b/arch/arm/mach-kirkwood/board-dreamplug.c
index 20af53a..acdc04a 100644
--- a/arch/arm/mach-kirkwood/board-dreamplug.c
+++ b/arch/arm/mach-kirkwood/board-dreamplug.c
@@ -64,7 +64,6 @@ void __init dreamplug_init(void)
 */
kirkwood_mpp_conf(dreamplug_mpp_config);
 
-   kirkwood_ehci_init();
kirkwood_ge00_init(dreamplug_ge00_data);
kirkwood_ge01_init(dreamplug_ge01_data);
kirkwood_sdio_init(dreamplug_mvsdio_data);
diff --git a/arch/arm/mach-kirkwood/board-dt.c 
b/arch/arm/mach-kirkwood/board-dt.c
index 70c5a28..ccb91b8 100644
--- a/arch/arm/mach-kirkwood/board-dt.c
+++ b/arch/arm/mach-kirkwood/board-dt.c
@@ -34,6 +34,7 @@ struct of_dev_auxdata kirkwood_auxdata_lookup[] __initdata = {
OF_DEV_AUXDATA(marvell,orion-sata, 0xf108, sata_mv.0, NULL),
OF_DEV_AUXDATA(marvell,orion-nand, 0xf400, orion_nand, NULL),
OF_DEV_AUXDATA(marvell,orion-crypto, 0xf103, mv_crypto, NULL),
+   OF_DEV_AUXDATA(marvell,orion-ehci, 0xf105, orion-ehci.0, NULL),
{},
 };
 
diff --git a/arch/arm/mach-kirkwood/board-goflexnet.c 
b/arch/arm/mach-kirkwood/board-goflexnet.c
index 001ca8c..d388bea 100644
--- a/arch/arm/mach-kirkwood/board-goflexnet.c
+++ b/arch/arm/mach-kirkwood/board-goflexnet.c
@@ -65,7 +65,6 @@ void __init goflexnet_init(void)
if (gpio_request(29, USB Power Enable) != 0 ||
gpio_direction_output(29, 1) != 0)
pr_err(can't setup GPIO 29 (USB Power Enable)\n);
-   kirkwood_ehci_init();
 
kirkwood_ge00_init(goflexnet_ge00_data);
 }
diff --git a/arch/arm/mach-kirkwood/board-ib62x0.c 
b/arch/arm/mach-kirkwood/board-ib62x0.c
index cfc47f8..db08e37 100644
--- a/arch/arm/mach-kirkwood/board-ib62x0.c
+++ b/arch/arm/mach-kirkwood/board-ib62x0.c
@@ -61,7 +61,6 @@ void __init ib62x0_init(void)
 */
kirkwood_mpp_conf(ib62x0_mpp_config);
 
-   kirkwood_ehci_init();
kirkwood_ge00_init(ib62x0_ge00_data);
if (gpio_request(IB62X0_GPIO_POWER_OFF, ib62x0:power:off) == 0 
gpio_direction_output(IB62X0_GPIO_POWER_OFF, 0) == 0)
diff --git a/arch/arm/mach-kirkwood/board-iconnect.c 
b/arch/arm/mach-kirkwood/board-iconnect.c
index d084b1e..8275fb0 100644
--- a/arch/arm/mach-kirkwood/board-iconnect.c
+++ b/arch/arm/mach-kirkwood/board-iconnect.c
@@ -45,7 +45,6 @@ void

Re: [PATCH 1/2] ARM: Kirkwood: ehci-orion: Add device tree binding

[Andrew Lunn]

  +Required properties:
  +- compatible: must be marvell,orion-ehci
  +- reg: physical base address of the controller and length of memory mapped
  +  region.
  +- interrupts: The EHCI interrupt
  +- phy-version: Can be one of:
  +  NA - Don't touch the phy, something else has already configured it.
  +  orion5x - PHY setup as specified by the Orion5x Errata
  +
  +Example:
  +
  +   ehci@5 {
  +   compatible = marvell,orion-ehci;
  +   reg = 0x5 0x1000;
  +   interrupts = 19;
  +   phy-version = NA;
  +   };
 
 This isn't an appropriate binding for phy. I know, it maps straight
 over from the platform data, but it doesn't focus on what the actual
 hardware is.
 
 A couple of options. What probably makes most sense depending on how
 other phy bindings are moving ahead is to add a phy node under the
 ehci controller for the orion5x case, and have an appropriate
 compatible value there. No node means the same as NA in the above
 binding. Alternatively, have a phy phandle that points to the phy
 device if it sits on an i2c bus, etc.

I Olaf

Could i suggest a third option:

I just drop USB phy configuration all together.  Only mach-orion5x
needs this and nobody has shown any interest in moving mach-orion5x to
DT. So i would just hard code it to NA.

If anybody does show interest in DT for orion5x, we can add a phy node
under ehci as a pure extension which does not affect backward
compatibility.

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

[PATCH] USB: PLAT_ORION fulfils USB_ARCH_HAS_EHCI

[Andrew Lunn]

The various Orion SoCs, i.e. orion5x, kirkwood, dove, mv78xx0
and 370/XP have EHCI. Make sure USB_ARCH_HAS_EHCI reflects this.

Reported-by: Sebastian Hesselbarth sebastian.hesselba...@gmail.com
Signed-off-by: Andrew Lunn and...@lunn.ch
---
 drivers/usb/Kconfig |1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
index a7773a3..723efcc 100644
--- a/drivers/usb/Kconfig
+++ b/drivers/usb/Kconfig
@@ -48,6 +48,7 @@ config USB_ARCH_HAS_EHCI
default y if SPARC_LEON
default y if ARCH_MMP
default y if MACH_LOONGSON1
+   default y if PLAT_ORION
default PCI
 
 # some non-PCI HCDs implement xHCI
-- 
1.7.10.4

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

[PATCH 1/2] ARM: Kirkwood: ehci-orion: Add device tree binding

[Andrew Lunn]

Based on previous work by Michael Walle and Jason Cooper.

Made their work actually work, which required added interrupt from DT
and auxdata, along with setting the dma_mask, which DT does not
currently do.

Signed-off-by: Andrew Lunn and...@lunn.ch
---
 .../devicetree/bindings/usb/ehci-orion.txt |   19 +++
 drivers/usb/host/ehci-orion.c  |   59 +++-
 2 files changed, 75 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/ehci-orion.txt

diff --git a/Documentation/devicetree/bindings/usb/ehci-orion.txt 
b/Documentation/devicetree/bindings/usb/ehci-orion.txt
new file mode 100644
index 000..1bd704e
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/ehci-orion.txt
@@ -0,0 +1,19 @@
+* EHCI controller, Orion Marvell variants
+
+Required properties:
+- compatible: must be marvell,orion-ehci
+- reg: physical base address of the controller and length of memory mapped
+  region.
+- interrupts: The EHCI interrupt
+- phy-version: Can be one of:
+  NA - Don't touch the phy, something else has already configured it.
+  orion5x - PHY setup as specified by the Orion5x Errata
+
+Example:
+
+   ehci@5 {
+   compatible = marvell,orion-ehci;
+   reg = 0x5 0x1000;
+   interrupts = 19;
+   phy-version = NA;
+   };
diff --git a/drivers/usb/host/ehci-orion.c b/drivers/usb/host/ehci-orion.c
index b34b928..605feb09 100644
--- a/drivers/usb/host/ehci-orion.c
+++ b/drivers/usb/host/ehci-orion.c
@@ -14,6 +14,9 @@
 #include linux/mbus.h
 #include linux/clk.h
 #include linux/platform_data/ehci-orion.h
+#include linux/of.h
+#include linux/of_device.h
+#include linux/of_irq.h
 
 #define rdl(off)   __raw_readl(hcd-regs + (off))
 #define wrl(off, val)  __raw_writel((val), hcd-regs + (off))
@@ -181,6 +184,27 @@ ehci_orion_conf_mbus_windows(struct usb_hcd *hcd,
}
 }
 
+static const int get_phy_version(struct device_node *np)
+{
+   const char *pm;
+   int err;
+
+   err = of_property_read_string(np, phy-version, pm);
+
+   if (err  0)
+   return err;
+
+
+   if (!strcasecmp(pm, NA))
+   return EHCI_PHY_NA;
+   if (!strcasecmp(pm, orion5x))
+   return EHCI_PHY_ORION;
+
+   return -ENODEV;
+}
+
+static u64 ehci_orion_dma_mask = DMA_BIT_MASK(32);
+
 static int __devinit ehci_orion_drv_probe(struct platform_device *pdev)
 {
struct orion_ehci_data *pd = pdev-dev.platform_data;
@@ -191,13 +215,17 @@ static int __devinit ehci_orion_drv_probe(struct 
platform_device *pdev)
struct clk *clk;
void __iomem *regs;
int irq, err;
+   enum orion_ehci_phy_ver phy_version;
 
if (usb_disabled())
return -ENODEV;
 
pr_debug(Initializing Orion-SoC USB Host Controller\n);
 
-   irq = platform_get_irq(pdev, 0);
+   if (pdev-dev.of_node)
+   irq = irq_of_parse_and_map(pdev-dev.of_node, 0);
+   else
+   irq = platform_get_irq(pdev, 0);
if (irq = 0) {
dev_err(pdev-dev,
Found HC with no IRQ. Check %s setup!\n,
@@ -215,6 +243,14 @@ static int __devinit ehci_orion_drv_probe(struct 
platform_device *pdev)
goto err1;
}
 
+   /*
+* Right now device-tree probed devices don't get dma_mask
+* set. Since shared usb code relies on it, set it here for
+* now. Once we have dma capability bindings this can go away.
+*/
+   if (!pdev-dev.dma_mask)
+   pdev-dev.dma_mask = ehci_orion_dma_mask;
+
if (!request_mem_region(res-start, resource_size(res),
ehci_orion_hc_driver.description)) {
dev_dbg(pdev-dev, controller already in use\n);
@@ -262,7 +298,14 @@ static int __devinit ehci_orion_drv_probe(struct 
platform_device *pdev)
/*
 * setup Orion USB controller.
 */
-   switch (pd-phy_version) {
+   if (pdev-dev.of_node) {
+   phy_version = get_phy_version(pdev-dev.of_node);
+   if (phy_version  0)
+   goto err3;
+   } else
+   phy_version = pd-phy_version;
+
+   switch (phy_version) {
case EHCI_PHY_NA:   /* dont change USB phy settings */
break;
case EHCI_PHY_ORION:
@@ -317,9 +360,19 @@ static int __exit ehci_orion_drv_remove(struct 
platform_device *pdev)
 
 MODULE_ALIAS(platform:orion-ehci);
 
+static const struct of_device_id ehci_orion_dt_ids[] __devinitdata = {
+   { .compatible = marvell,orion-ehci, },
+   {},
+};
+MODULE_DEVICE_TABLE(of, ehci_orion_dt_ids);
+
 static struct platform_driver ehci_orion_driver = {
.probe  = ehci_orion_drv_probe,
.remove = __exit_p(ehci_orion_drv_remove),
.shutdown   = usb_hcd_platform_shutdown,
-   .driver.name= orion-ehci,
+   .driver

[PATCH 2/2] ARM: Kirkwood: Convert all DT boards to EHCI via DT.

[Andrew Lunn]

Now that the EHCI driver has DT support, drop old style configuration
of it and add DT in its place. Since all the boards enable the EHCI,
enable it by default in kirkwood.dtsi. Any new boards which don't have
USB can specifically disable it.

Signed-off-by: Andrew Lunn and...@lunn.ch
---
 arch/arm/boot/dts/kirkwood.dtsi   |8 
 arch/arm/mach-kirkwood/board-dnskw.c  |1 -
 arch/arm/mach-kirkwood/board-dockstar.c   |1 -
 arch/arm/mach-kirkwood/board-dreamplug.c  |1 -
 arch/arm/mach-kirkwood/board-dt.c |1 +
 arch/arm/mach-kirkwood/board-goflexnet.c  |1 -
 arch/arm/mach-kirkwood/board-ib62x0.c |1 -
 arch/arm/mach-kirkwood/board-iconnect.c   |1 -
 arch/arm/mach-kirkwood/board-iomega_ix2_200.c |2 --
 arch/arm/mach-kirkwood/board-lsxl.c   |1 -
 arch/arm/mach-kirkwood/board-ts219.c  |1 -
 11 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/arch/arm/boot/dts/kirkwood.dtsi b/arch/arm/boot/dts/kirkwood.dtsi
index cef9616..b36bb85 100644
--- a/arch/arm/boot/dts/kirkwood.dtsi
+++ b/arch/arm/boot/dts/kirkwood.dtsi
@@ -76,6 +76,14 @@
status = okay;
};
 
+   ehci@5 {
+   compatible = marvell,orion-ehci;
+   reg = 0x5 0x1000;
+   interrupts = 19;
+   phy-version = NA;
+   status = okay;
+   };
+
sata@8 {
compatible = marvell,orion-sata;
reg = 0x8 0x5000;
diff --git a/arch/arm/mach-kirkwood/board-dnskw.c 
b/arch/arm/mach-kirkwood/board-dnskw.c
index 8103961..0fc78a6 100644
--- a/arch/arm/mach-kirkwood/board-dnskw.c
+++ b/arch/arm/mach-kirkwood/board-dnskw.c
@@ -111,7 +111,6 @@ void __init dnskw_init(void)
 {
kirkwood_mpp_conf(dnskw_mpp_config);
 
-   kirkwood_ehci_init();
kirkwood_ge00_init(dnskw_ge00_data);
 
platform_device_register(dnskw_fan_device);
diff --git a/arch/arm/mach-kirkwood/board-dockstar.c 
b/arch/arm/mach-kirkwood/board-dockstar.c
index ed35c12..08835fa 100644
--- a/arch/arm/mach-kirkwood/board-dockstar.c
+++ b/arch/arm/mach-kirkwood/board-dockstar.c
@@ -55,7 +55,6 @@ void __init dockstar_dt_init(void)
if (gpio_request(29, USB Power Enable) != 0 ||
gpio_direction_output(29, 1) != 0)
pr_err(can't setup GPIO 29 (USB Power Enable)\n);
-   kirkwood_ehci_init();
 
kirkwood_ge00_init(dockstar_ge00_data);
 }
diff --git a/arch/arm/mach-kirkwood/board-dreamplug.c 
b/arch/arm/mach-kirkwood/board-dreamplug.c
index b5a4922..66c5ac8 100644
--- a/arch/arm/mach-kirkwood/board-dreamplug.c
+++ b/arch/arm/mach-kirkwood/board-dreamplug.c
@@ -64,7 +64,6 @@ void __init dreamplug_init(void)
 */
kirkwood_mpp_conf(dreamplug_mpp_config);
 
-   kirkwood_ehci_init();
kirkwood_ge00_init(dreamplug_ge00_data);
kirkwood_ge01_init(dreamplug_ge01_data);
kirkwood_sdio_init(dreamplug_mvsdio_data);
diff --git a/arch/arm/mach-kirkwood/board-dt.c 
b/arch/arm/mach-kirkwood/board-dt.c
index 30a4caa..16b767e 100644
--- a/arch/arm/mach-kirkwood/board-dt.c
+++ b/arch/arm/mach-kirkwood/board-dt.c
@@ -33,6 +33,7 @@ struct of_dev_auxdata kirkwood_auxdata_lookup[] __initdata = {
OF_DEV_AUXDATA(marvell,orion-wdt, 0xf1020300, orion_wdt, NULL),
OF_DEV_AUXDATA(marvell,orion-sata, 0xf108, sata_mv.0, NULL),
OF_DEV_AUXDATA(marvell,orion-nand, 0xf400, orion_nand, NULL),
+   OF_DEV_AUXDATA(marvell,orion-ehci, 0xf105, orion-ehci.0, NULL),
{},
 };
 
diff --git a/arch/arm/mach-kirkwood/board-goflexnet.c 
b/arch/arm/mach-kirkwood/board-goflexnet.c
index 52cde7c..6c1e353 100644
--- a/arch/arm/mach-kirkwood/board-goflexnet.c
+++ b/arch/arm/mach-kirkwood/board-goflexnet.c
@@ -65,7 +65,6 @@ void __init goflexnet_init(void)
if (gpio_request(29, USB Power Enable) != 0 ||
gpio_direction_output(29, 1) != 0)
pr_err(can't setup GPIO 29 (USB Power Enable)\n);
-   kirkwood_ehci_init();
 
kirkwood_ge00_init(goflexnet_ge00_data);
 }
diff --git a/arch/arm/mach-kirkwood/board-ib62x0.c 
b/arch/arm/mach-kirkwood/board-ib62x0.c
index 4304a44..4baedae 100644
--- a/arch/arm/mach-kirkwood/board-ib62x0.c
+++ b/arch/arm/mach-kirkwood/board-ib62x0.c
@@ -61,7 +61,6 @@ void __init ib62x0_init(void)
 */
kirkwood_mpp_conf(ib62x0_mpp_config);
 
-   kirkwood_ehci_init();
kirkwood_ge00_init(ib62x0_ge00_data);
if (gpio_request(IB62X0_GPIO_POWER_OFF, ib62x0:power:off) == 0 
gpio_direction_output(IB62X0_GPIO_POWER_OFF, 0) == 0)
diff --git a/arch/arm/mach-kirkwood/board-iconnect.c 
b/arch/arm/mach-kirkwood/board-iconnect.c
index 71a6cc0..b6b89ea 100644
--- a/arch/arm/mach-kirkwood/board-iconnect.c
+++ b/arch/arm/mach-kirkwood/board-iconnect.c
@@ -46,7 +46,6 @@ void

EHCI: Centralize controller initialization

[Andrew Lunn]

Hi Alan

kisskb is showing up a warning in drivers/usb/host/ehci-orion.c
which i think is from a change you made:

drivers/usb/host/ehci-orion.c:109:2: warning: passing argument 1 of 
'ehci_setup' from incompatible pointer type [enabled by default]

diff --git a/drivers/usb/host/ehci-orion.c b/drivers/usb/host/ehci-orion.c
index 82de107..3e41123 100644 (file)
--- a/drivers/usb/host/ehci-orion.c
+++ b/drivers/usb/host/ehci-orion.c
@@ -106,21 +106,10 @@ static int ehci_orion_setup(struct usb_hcd *hcd)
struct ehci_hcd *ehci = hcd_to_ehci(hcd);
int retval;
 
-   hcd-has_tt = 1;
-
-   retval = ehci_halt(ehci);
-   if (retval)
-   return retval;
-
-   /*
-* data structure init
-*/
-   retval = ehci_init(hcd);
+   retval = ehci_setup(ehci);
if (retval)
return retval;
 
ehci_setup() is being passed a struct ehci_hcd *, but:

 811 static int ehci_setup(struct usb_hcd *hcd)
 812 {
 813 struct ehci_hcd *ehci = hcd_to_ehci(hcd);
 814 int retval;

it seems to expect a struct usb_hcd *

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