Re: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support
Hi,
Bin Gao writes:
>> > +int pd_sink_queue_msg(struct pd_sink_msg *msg)
>> > +{
>> > + unsigned long flags;
>> > + struct pd_sink_port *port;
>> > +
>> > + if (msg->port < 0 || msg->port >= MAX_NR_SINK_PORTS) {
>> > + pr_err("Invalid port number\n");
>> > + return -EINVAL;
>> > + }
>> > +
>> > + port = sink_ports[msg->port];
>> > +
>> > + spin_lock_irqsave(&port->rx_lock, flags);
>> > + list_add_tail(&msg->list, &port->rx_list);
>> > + spin_unlock_irqrestore(&port->rx_lock, flags);
>> > +
>> > + queue_work(port->rx_wq, &port->rx_work);
>>
>> can we really queue several messages at a time? It seems unfeasible to
>> me. It's not like we can queue several power request in a role. Why do
>> you need this workqueue? Why don't you process message here, in place?
> Some Type-C chargers send two messages in a short duration(less than 1 ms),
> e.g. a SOURCE_CAPABILITY follows the previous SOURCE_CAPABILITY, or a
> GET_SINK_CAPABILITY follows a previous SOURCE_CAPABILITY, etc. Queuing
> message to PD stack by Type-C phy driver typically happens in a interrupt
> context. So in this case a nested interrupt may happen. Our whole PD
> stack while processing one message is not re-entrant so the nested
> interrupt would cause a problem.
keep interrupts masked for as long as necessary until your message is
processed.
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH 2/2] usb: chipidea: usbmisc: set over current polarity for imx6 and imx7
On Mon, Jul 18, 2016 at 07:04:57AM +, Jun Li wrote: > Hi Peter, > > > -Original Message- > > From: Peter Chen [mailto:hzpeterc...@gmail.com] > > Sent: Friday, July 15, 2016 5:21 PM > > To: Jun Li > > Cc: Peter Chen ; linux-usb@vger.kernel.org > > Subject: Re: [PATCH 2/2] usb: chipidea: usbmisc: set over current polarity > > for imx6 and imx7 > > > > On Fri, Jul 15, 2016 at 07:38:23AM +, Jun Li wrote: > > > Hi, > > > > -Original Message- > > > > From: Peter Chen [mailto:hzpeterc...@gmail.com] > > > > Sent: Friday, July 15, 2016 3:02 PM > > > > To: Jun Li > > > > Cc: Peter Chen ; linux-usb@vger.kernel.org > > > > Subject: Re: [PATCH 2/2] usb: chipidea: usbmisc: set over current > > > > polarity for imx6 and imx7 > > > > > > > > On Tue, Jul 12, 2016 at 03:24:49PM +0800, Li Jun wrote: > > > > > As all usb power supply use low active for over current flag on > > > > > imx6 > > > > > imx7 boards, and the default register setting(0) is for high > > > > > active, this patch is to correct it. > > > > > > > > > > > > > We may can't ensure all USB power switch chips work like that, I > > > > suggest you making this as default. > > > > > > > > I will change the commit log like below if you are ok. > > > > > > > > As most of all usb power switch chips use active-low for over > > > > current flag, but the default register setting(0) is for active-high > > > > at imx6/imx7, this patch changes default value as active-low. > > > > > > Looks better, I am okay with it except a tiny comment :%s/As most of > > > all usb power/As most of usb power > > > > > > Li Jun > > > > > > Since we can't break current default behaviour, but with your patch, the > > imx6sx sdb board creates over current event. > > > > I think you may need to introduce a flag for OC polarity, and use it for > > exist platforms if necessary. It can narrow down affect only on single > > platform. > > > > For new platforms, you can change SoC values by default. > > To keep those platforms without specify either disable-over-current > or over-current-polarity out of problem, we need set the disable_oc > to be "1", or oc_polarity to be "1"(then register value to be "0" > as before) for them. > > if (!of_property_read_u32(np, "over-current-polarity", &ret)) > data->oc_polarity = ret ? 1 : 0; > else > data->oc_polarity = 1; > If there is no property "over-current-polarity", you should keep register value unchanging. You know we should not break the platforms which may still use old dts. So, we could set the value according to DT property, but can't change default value unless for new SoC platform. Peter > or > > if (!of_property_read_u32(np, "over-current-polarity", &ret)) > data->oc_polarity = ret ? 1 : 0; > else > data->disable_oc = 1; > > which way do you prefer? > > Li Jun > > > > -- > > > > Best Regards, > > Peter Chen -- Best Regards, Peter Chen -- 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] phy: rockchip-inno-usb2: add a new driver for Rockchip usb2phy
Hi Heiko, On 2016/6/25 3:58, Heiko Stuebner wrote: Am Dienstag, 21. Juni 2016, 18:30:05 schrieb Frank Wang: The newer SoCs (rk3366, rk3399) take a different usb-phy IP block than rk3288 and before, and most of phy-related registers are also different from the past, so a new phy driver is required necessarily. Signed-off-by: Frank Wang Suggested-by: Heiko Stuebner Suggested-by: Guenter Roeck Suggested-by: Doug Anderson still looks nice, so still Reviewed-by: Heiko Stuebner Currently, we have already prepared compatible for rk3399, however, I found these series of patch had not merged in, so shall I reopen it or commit a new one? BR. Frank -- 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: chipidea: usbmisc: set over current polarity for imx6 and imx7
Hi Peter, > -Original Message- > From: Peter Chen [mailto:hzpeterc...@gmail.com] > Sent: Friday, July 15, 2016 5:21 PM > To: Jun Li > Cc: Peter Chen ; linux-usb@vger.kernel.org > Subject: Re: [PATCH 2/2] usb: chipidea: usbmisc: set over current polarity > for imx6 and imx7 > > On Fri, Jul 15, 2016 at 07:38:23AM +, Jun Li wrote: > > Hi, > > > -Original Message- > > > From: Peter Chen [mailto:hzpeterc...@gmail.com] > > > Sent: Friday, July 15, 2016 3:02 PM > > > To: Jun Li > > > Cc: Peter Chen ; linux-usb@vger.kernel.org > > > Subject: Re: [PATCH 2/2] usb: chipidea: usbmisc: set over current > > > polarity for imx6 and imx7 > > > > > > On Tue, Jul 12, 2016 at 03:24:49PM +0800, Li Jun wrote: > > > > As all usb power supply use low active for over current flag on > > > > imx6 > > > > imx7 boards, and the default register setting(0) is for high > > > > active, this patch is to correct it. > > > > > > > > > > We may can't ensure all USB power switch chips work like that, I > > > suggest you making this as default. > > > > > > I will change the commit log like below if you are ok. > > > > > > As most of all usb power switch chips use active-low for over > > > current flag, but the default register setting(0) is for active-high > > > at imx6/imx7, this patch changes default value as active-low. > > > > Looks better, I am okay with it except a tiny comment :%s/As most of > > all usb power/As most of usb power > > > > Li Jun > > > > Since we can't break current default behaviour, but with your patch, the > imx6sx sdb board creates over current event. > > I think you may need to introduce a flag for OC polarity, and use it for > exist platforms if necessary. It can narrow down affect only on single > platform. > > For new platforms, you can change SoC values by default. To keep those platforms without specify either disable-over-current or over-current-polarity out of problem, we need set the disable_oc to be "1", or oc_polarity to be "1"(then register value to be "0" as before) for them. if (!of_property_read_u32(np, "over-current-polarity", &ret)) data->oc_polarity = ret ? 1 : 0; else data->oc_polarity = 1; or if (!of_property_read_u32(np, "over-current-polarity", &ret)) data->oc_polarity = ret ? 1 : 0; else data->disable_oc = 1; which way do you prefer? Li Jun > > -- > > Best Regards, > Peter Chen -- 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/4] doc: usb: usbmisc-imx: add imx7d compatible string
Add compatible string for imx7d-usbmisc. Signed-off-by: Li Jun --- Documentation/devicetree/bindings/usb/usbmisc-imx.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/devicetree/bindings/usb/usbmisc-imx.txt b/Documentation/devicetree/bindings/usb/usbmisc-imx.txt index 3539d4e..f1e27fa 100644 --- a/Documentation/devicetree/bindings/usb/usbmisc-imx.txt +++ b/Documentation/devicetree/bindings/usb/usbmisc-imx.txt @@ -6,6 +6,7 @@ Required properties: "fsl,imx6q-usbmisc" for imx6q "fsl,vf610-usbmisc" for Vybrid vf610 "fsl,imx6sx-usbmisc" for imx6sx + "fsl,imx7d-usbmisc" for imx7d - reg: Should contain registers location and length Examples: -- 1.9.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] phy: rockchip-inno-usb2: add a new driver for Rockchip usb2phy
Hi Frank, Am Montag, 18. Juli 2016, 18:02:28 schrieb Frank Wang: > On 2016/6/25 3:58, Heiko Stuebner wrote: > > Am Dienstag, 21. Juni 2016, 18:30:05 schrieb Frank Wang: > >> The newer SoCs (rk3366, rk3399) take a different usb-phy IP block > >> than rk3288 and before, and most of phy-related registers are also > >> different from the past, so a new phy driver is required necessarily. > >> > >> Signed-off-by: Frank Wang > >> Suggested-by: Heiko Stuebner > >> Suggested-by: Guenter Roeck > >> Suggested-by: Doug Anderson > > > > still looks nice, so still > > Reviewed-by: Heiko Stuebner > > Currently, we have already prepared compatible for rk3399, however, I > found these series of patch had not merged in, so shall I reopen it or > commit a new one? the phy tree from Kishon feeds into the usb tree and the merge for 4.8 was done on friday it seems. So it looks like the usb2 phy will need to wait until after the merge window anyway, so you could include the rk3399 support already and resend the series, so everything is ready then. Heiko -- 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 4/4] usb: chipidea: imx: set over current polarity per dts setting
With over-current-polarity property added, imx usb over current
polarity can be configed to be low or high active, since the default
setting value(0) is for active high, so keep this setting for those
legacy platforms without this property specified.
Signed-off-by: Li Jun
---
drivers/usb/chipidea/ci_hdrc_imx.c | 9 +
drivers/usb/chipidea/ci_hdrc_imx.h | 1 +
drivers/usb/chipidea/usbmisc_imx.c | 30 +-
3 files changed, 35 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c
b/drivers/usb/chipidea/ci_hdrc_imx.c
index dedc33e..61e712b 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.c
+++ b/drivers/usb/chipidea/ci_hdrc_imx.c
@@ -140,6 +140,15 @@ static struct imx_usbmisc_data
*usbmisc_get_init_data(struct device *dev)
if (of_find_property(np, "disable-over-current", NULL))
data->disable_oc = 1;
+ if (!of_property_read_u32(np, "over-current-polarity", &ret))
+ data->oc_polarity = ret ? 1 : 0;
+ else
+ /*
+* Keep the oc polarity setting of legacy
+* platforms unchanged.
+*/
+ data->oc_polarity = 1;
+
if (of_find_property(np, "external-vbus-divider", NULL))
data->evdo = 1;
diff --git a/drivers/usb/chipidea/ci_hdrc_imx.h
b/drivers/usb/chipidea/ci_hdrc_imx.h
index 635717e..409aa5ca8 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.h
+++ b/drivers/usb/chipidea/ci_hdrc_imx.h
@@ -17,6 +17,7 @@ struct imx_usbmisc_data {
int index;
unsigned int disable_oc:1; /* over current detect disabled */
+ unsigned int oc_polarity:1; /* over current polarity if oc enabled */
unsigned int evdo:1; /* set external vbus divider option */
};
diff --git a/drivers/usb/chipidea/usbmisc_imx.c
b/drivers/usb/chipidea/usbmisc_imx.c
index ab8b027..193dbe4 100644
--- a/drivers/usb/chipidea/usbmisc_imx.c
+++ b/drivers/usb/chipidea/usbmisc_imx.c
@@ -56,6 +56,7 @@
#define MX6_BM_NON_BURST_SETTING BIT(1)
#define MX6_BM_OVER_CUR_DISBIT(7)
+#define MX6_BM_OVER_CUR_POLARITY BIT(8)
#define MX6_BM_WAKEUP_ENABLE BIT(10)
#define MX6_BM_ID_WAKEUP BIT(16)
#define MX6_BM_VBUS_WAKEUP BIT(17)
@@ -266,11 +267,18 @@ static int usbmisc_imx6q_init(struct imx_usbmisc_data
*data)
spin_lock_irqsave(&usbmisc->lock, flags);
+ reg = readl(usbmisc->base + data->index * 4);
if (data->disable_oc) {
- reg = readl(usbmisc->base + data->index * 4);
- writel(reg | MX6_BM_OVER_CUR_DIS,
- usbmisc->base + data->index * 4);
+ reg |= MX6_BM_OVER_CUR_DIS;
+ } else if (data->oc_polarity == 1) {
+ /* High active */
+ reg &= ~(MX6_BM_OVER_CUR_DIS | MX6_BM_OVER_CUR_POLARITY);
+ } else {
+ /* Low active */
+ reg &= ~MX6_BM_OVER_CUR_DIS;
+ reg |= MX6_BM_OVER_CUR_POLARITY;
}
+ writel(reg, usbmisc->base + data->index * 4);
/* SoC non-burst setting */
reg = readl(usbmisc->base + data->index * 4);
@@ -365,10 +373,18 @@ static int usbmisc_imx7d_init(struct imx_usbmisc_data
*data)
return -EINVAL;
spin_lock_irqsave(&usbmisc->lock, flags);
+ reg = readl(usbmisc->base);
if (data->disable_oc) {
- reg = readl(usbmisc->base);
- writel(reg | MX6_BM_OVER_CUR_DIS, usbmisc->base);
+ reg |= MX6_BM_OVER_CUR_DIS;
+ } else if (data->oc_polarity == 1) {
+ /* High active */
+ reg &= ~(MX6_BM_OVER_CUR_DIS | MX6_BM_OVER_CUR_POLARITY);
+ } else {
+ /* Low active */
+ reg &= ~MX6_BM_OVER_CUR_DIS;
+ reg |= MX6_BM_OVER_CUR_POLARITY;
}
+ writel(reg, usbmisc->base);
reg = readl(usbmisc->base + MX7D_USBNC_USB_CTRL2);
reg &= ~MX7D_USB_VBUS_WAKEUP_SOURCE_MASK;
@@ -492,6 +508,10 @@ static const struct of_device_id usbmisc_imx_dt_ids[] = {
.compatible = "fsl,imx6ul-usbmisc",
.data = &imx6sx_usbmisc_ops,
},
+ {
+ .compatible = "fsl,imx7d-usbmisc",
+ .data = &imx7d_usbmisc_ops,
+ },
{ /* sentinel */ }
};
MODULE_DEVICE_TABLE(of, usbmisc_imx_dt_ids);
--
1.9.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 net-next] cdc_ether: Improve ZTE MF823/831/910 handling
The firmware in the ZTE MF823/831/910 modems/mifis use OS fingerprinting to
determine which type of device to export. In addition, these devices export
a REST API which can also be used to control the type of device. So far, on
Linux, the devices have been seen as RNDIS or CDC Ether.
When CDC Ether is used, devices of the same type are, as with RNDIS,
exported with the same, bogus random MAC address. In addition, the devices
(at least on all firmware revisions I have found) use this MAC when sending
traffic routed from external networks. And as a final feature, the devices
sometimes export the link state incorrectly.
This patch tries to improve the handling of these devices by doing the
following:
* If a random MAC is read from device, then generate a new random MAC
address. This fix will apply to all devices using cdc_ether, but that
should be ok as we will also fix similar mistakes from other
manufacturers.
* The MF823/MF832/MF910 sometimes export cdc carrier on twice on connect
(the correct behavior is off then on). Work around this by manually setting
carrier to off if an on-notification is received and the NOCARRIER-bit is
not set.
This change will also affect all devices, but as with the MAC-fix it should
take care of similar mistakes. I tried to think of/look/test for
problems/regressions that could be introduced by this behavior, but could
not find any. However, my familiarity with this code path is not that
great, so there could be something I have overlooked.
* Add an rx_fixup-function (and a new driver info-struct) which is used by
these three devices (identified either as PID 0x1405 or 0x1408). The
rx_fixup-function replaces the destination MAC address in the skb with that
of the device. I have not seen a revision of these three device that
behaves correctly (i.e., sets the right destination MAC), so I chose not to
do any comparison with for example the known, bogus addresses.
I have tested this patch with multiple revisions of all three devices, and
they behave as expected. In other words, they all got a valid, random MAC,
the correct operational state and I can receive/sent traffic without
problems. I also tested with some other cdc_ether devices I have and did
not find any problems/regressions caused by the two general changes.
Signed-off-by: Kristian Evensen
---
drivers/net/usb/cdc_ether.c | 53 +
1 file changed, 53 insertions(+)
diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c
index 7cba2c3..2325097 100644
--- a/drivers/net/usb/cdc_ether.c
+++ b/drivers/net/usb/cdc_ether.c
@@ -388,6 +388,12 @@ void usbnet_cdc_status(struct usbnet *dev, struct urb *urb)
case USB_CDC_NOTIFY_NETWORK_CONNECTION:
netif_dbg(dev, timer, dev->net, "CDC: carrier %s\n",
event->wValue ? "on" : "off");
+
+ /* Work-around for devices with broken off-notifications */
+ if (event->wValue &&
+ !test_bit(__LINK_STATE_NOCARRIER, &dev->net->state))
+ usbnet_link_change(dev, 0, 0);
+
usbnet_link_change(dev, !!event->wValue, 0);
break;
case USB_CDC_NOTIFY_SPEED_CHANGE: /* tx/rx rates */
@@ -428,10 +434,34 @@ int usbnet_cdc_bind(struct usbnet *dev, struct
usb_interface *intf)
return status;
}
+ if (dev->net->dev_addr[0] & 0x02)
+ eth_hw_addr_random(dev->net);
+
return 0;
}
EXPORT_SYMBOL_GPL(usbnet_cdc_bind);
+/* Make sure packets have correct destination MAC address
+ *
+ * A firmware bug observed on some devices (ZTE MF823/831/910) is that the
+ * device sends packets with a static, bogus, random MAC address (event if
+ * device MAC address has been updated). Always set MAC address to that of the
+ * device.
+ */
+static int usbnet_cdc_zte_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
+{
+ u8 num_buggy_hwaddrs;
+ u8 buggy_hwaddrs_idx = 0;
+
+ if (skb->len < ETH_HLEN || !(skb->data[0] & 0x02))
+ return 1;
+
+ skb_reset_mac_header(skb);
+ ether_addr_copy(eth_hdr(skb)->h_dest, dev->net->dev_addr);
+
+ return 1;
+}
+
static const struct driver_infocdc_info = {
.description = "CDC Ethernet Device",
.flags =FLAG_ETHER | FLAG_POINTTOPOINT,
@@ -442,6 +472,17 @@ static const struct driver_infocdc_info = {
.manage_power = usbnet_manage_power,
};
+static const struct driver_infozte_cdc_info = {
+ .description = "ZTE MF823/831/910 CDC Ethernet Device",
+ .flags =FLAG_ETHER | FLAG_POINTTOPOINT,
+ .bind = usbnet_cdc_bind,
+ .unbind = usbnet_cdc_unbind,
+ .status = usbnet_cdc_status,
+ .set_rx_mode = usbnet_cdc_update_filter,
+ .manage_power = usbnet_manage_power,
+ .rx_fixup = usbnet_cdc_zte_rx_fixup,
+};
+
static const struct driver_info wwan_info = {
drivers/usb/usbip/vudc_rx.c:145: possible bad bitmask ?
Hello there, drivers/usb/usbip/vudc_rx.c:145:27: warning: result of ‘11 << 30’ requires 35 bits to represent, but ‘int’ only has 32 bits [-Wshift-overflow=] Source code is urb_p->urb->pipe &= ~(11 << 30); Maybe better code urb_p->urb->pipe &= ~(11UL << 30); Regards David Binderman -- 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-next] cdc_ether: Improve ZTE MF823/831/910 handling
On Mon, 2016-07-18 at 14:24 +0200, Kristian Evensen wrote: > The firmware in the ZTE MF823/831/910 modems/mifis use OS fingerprinting to > determine which type of device to export. In addition, these devices export > a REST API which can also be used to control the type of device. So far, on > Linux, the devices have been seen as RNDIS or CDC Ether. > > When CDC Ether is used, devices of the same type are, as with RNDIS, > exported with the same, bogus random MAC address. In addition, the devices Please explain. If the MAC is random, I fail to see why the host would be any better at making up a MAC. > (at least on all firmware revisions I have found) use this MAC when sending > traffic routed from external networks. And as a final feature, the devices > sometimes export the link state incorrectly. > > This patch tries to improve the handling of these devices by doing the > following: > > * If a random MAC is read from device, then generate a new random MAC > address. This fix will apply to all devices using cdc_ether, but that > should be ok as we will also fix similar mistakes from other > manufacturers. I am not really happy with this. If this is specific to the device a quirk should be used. If it is a truly generic misdesign, it does not belong into cdc-ether. It should go into the generic layer. > * The MF823/MF832/MF910 sometimes export cdc carrier on twice on connect > (the correct behavior is off then on). Work around this by manually setting > carrier to off if an on-notification is received and the NOCARRIER-bit is > not set. > > This change will also affect all devices, but as with the MAC-fix it should > take care of similar mistakes. I tried to think of/look/test for > problems/regressions that could be introduced by this behavior, but could > not find any. However, my familiarity with this code path is not that > great, so there could be something I have overlooked. Looks OK > * Add an rx_fixup-function (and a new driver info-struct) which is used by > these three devices (identified either as PID 0x1405 or 0x1408). The > rx_fixup-function replaces the destination MAC address in the skb with that > of the device. I have not seen a revision of these three device that > behaves correctly (i.e., sets the right destination MAC), so I chose not to > do any comparison with for example the known, bogus addresses. Looks OK Regards Oliver -- 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-next] cdc_ether: Improve ZTE MF823/831/910 handling
Hi, On Mon, Jul 18, 2016 at 3:01 PM, Oliver Neukum wrote: > On Mon, 2016-07-18 at 14:24 +0200, Kristian Evensen wrote: >> The firmware in the ZTE MF823/831/910 modems/mifis use OS fingerprinting to >> determine which type of device to export. In addition, these devices export >> a REST API which can also be used to control the type of device. So far, on >> Linux, the devices have been seen as RNDIS or CDC Ether. >> >> When CDC Ether is used, devices of the same type are, as with RNDIS, >> exported with the same, bogus random MAC address. In addition, the devices > > Please explain. If the MAC is random, I fail to see why the host would > be any better at making up a MAC. Sorry for not explaining this properly. The problem is that all devices of the same type store the same value in iMACAddress. On all MF831/MF910s (that I have seen) 36:4b:50:b7:ef:da is stored in this value, while 36:4b:50:b7:ef:38 is stored on MF823. In order to ensure that all devices on a host has a unique MAC-address, I think it is better to generate a new random MAC on the host than to trust the value returned from the device. >> * If a random MAC is read from device, then generate a new random MAC >> address. This fix will apply to all devices using cdc_ether, but that >> should be ok as we will also fix similar mistakes from other >> manufacturers. > > I am not really happy with this. > > If this is specific to the device a quirk should be used. If it is a > truly generic misdesign, it does not belong into cdc-ether. It should go > into the generic layer. We saw the same behaviour when these devices are exposed as RNDIS and decided to apply a generic fix instead of limiting ourself to for example the two, known bogus MAC address. See commit a5a18bdf7453d505783e40e47ebb84bfdd35f93b and the thread "[PATCH] rndis_host: Set random MAC for ZTE MF910" (http://www.spinics.net/lists/linux-usb/msg143727.html) for the discussion. However, I have no strong objections towards for example checking against the two bad MAC addresses before generating a random MAC. -Kristian -- 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 2/4] doc: usb: ci-hdrc-usb2: add property over-current-polarity
Adding over-current-polarity to indicate the over current flag is low active or high active. Signed-off-by: Li Jun --- Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt index 341dc67..c5d35f4 100644 --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt @@ -81,6 +81,8 @@ i.mx specific properties - fsl,usbmisc: phandler of non-core register device, with one argument that indicate usb controller index - disable-over-current: disable over current detect +- over-current-polarity: 0 if the over current signal polarity is low active, + 1 if the over current signal polarity is high active. - external-vbus-divider: enables off-chip resistor divider for Vbus Example: -- 1.9.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 net-next] cdc_ether: Improve ZTE MF823/831/910 handling
On Mon, 2016-07-18 at 15:23 +0200, Kristian Evensen wrote: > Hi, > > On Mon, Jul 18, 2016 at 3:01 PM, Oliver Neukum wrote: > > On Mon, 2016-07-18 at 14:24 +0200, Kristian Evensen wrote: > >> The firmware in the ZTE MF823/831/910 modems/mifis use OS fingerprinting to > >> determine which type of device to export. In addition, these devices export > >> a REST API which can also be used to control the type of device. So far, on > >> Linux, the devices have been seen as RNDIS or CDC Ether. > >> > >> When CDC Ether is used, devices of the same type are, as with RNDIS, > >> exported with the same, bogus random MAC address. In addition, the devices > > > > Please explain. If the MAC is random, I fail to see why the host would > > be any better at making up a MAC. > > Sorry for not explaining this properly. The problem is that all > devices of the same type store the same value in iMACAddress. On all > MF831/MF910s (that I have seen) 36:4b:50:b7:ef:da is stored in this > value, while 36:4b:50:b7:ef:38 is stored on MF823. In order to ensure > that all devices on a host has a unique MAC-address, I think it is > better to generate a new random MAC on the host than to trust the > value returned from the device. OK, thanks for explaining. > >> * If a random MAC is read from device, then generate a new random MAC > >> address. This fix will apply to all devices using cdc_ether, but that > >> should be ok as we will also fix similar mistakes from other > >> manufacturers. > > > > I am not really happy with this. > > > > If this is specific to the device a quirk should be used. If it is a > > truly generic misdesign, it does not belong into cdc-ether. It should go > > into the generic layer. > > We saw the same behaviour when these devices are exposed as RNDIS and > decided to apply a generic fix instead of limiting ourself to for > example the two, known bogus MAC address. See commit > a5a18bdf7453d505783e40e47ebb84bfdd35f93b and the thread "[PATCH] > rndis_host: Set random MAC for ZTE MF910" > (http://www.spinics.net/lists/linux-usb/msg143727.html) for the > discussion. > > However, I have no strong objections towards for example checking > against the two bad MAC addresses before generating a random MAC. No, you misunderstand me. I don't want quirks if we can avoid it. But if you need to do this for rndis_host and cdc_ether and maybe other drivers you should not be touching drivers. This belongs into the core ethernet code. Your code is good, but you are putting it in the wrong places. Regards Oliver -- 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-next] cdc_ether: Improve ZTE MF823/831/910 handling
On Mon, Jul 18, 2016 at 3:50 PM, Oliver Neukum wrote: > No, you misunderstand me. I don't want quirks if we can avoid it. > But if you need to do this for rndis_host and cdc_ether and maybe other > drivers you should not be touching drivers. This belongs into the core > ethernet code. Your code is good, but you are putting it in the wrong > places. Ok, sounds good. So far, I have only seen the random MAC issue with the three previously mentioned devices, but who knows how many else is out there with the same error ... I don't think it should be in the core ethernet code, at least not yet, but I agree it would make sense to move it to for example usbnet_core(). If you agree, I can prepare a patch for it. -- 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-next] cdc_ether: Improve ZTE MF823/831/910 handling
On Mon, 2016-07-18 at 16:10 +0200, Kristian Evensen wrote: > On Mon, Jul 18, 2016 at 3:50 PM, Oliver Neukum wrote: > > No, you misunderstand me. I don't want quirks if we can avoid it. > > But if you need to do this for rndis_host and cdc_ether and maybe other > > drivers you should not be touching drivers. This belongs into the core > > ethernet code. Your code is good, but you are putting it in the wrong > > places. > > Ok, sounds good. So far, I have only seen the random MAC issue with > the three previously mentioned devices, but who knows how many else is > out there with the same error ... I don't think it should be in the > core ethernet code, at least not yet, but I agree it would make sense > to move it to for example usbnet_core(). If you agree, I can prepare a > patch for it. I don't see how it would be specific for a subsystem. If the patch is correct, it belongs into the networking core. Regards Oliver -- 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-next] cdc_ether: Improve ZTE MF823/831/910 handling
On Mon, Jul 18, 2016 at 4:14 PM, Oliver Neukum wrote: >> Ok, sounds good. So far, I have only seen the random MAC issue with >> the three previously mentioned devices, but who knows how many else is >> out there with the same error ... I don't think it should be in the >> core ethernet code, at least not yet, but I agree it would make sense >> to move it to for example usbnet_core(). If you agree, I can prepare a >> patch for it. > > I don't see how it would be specific for a subsystem. If the patch > is correct, it belongs into the networking core. To me it sounds a bit intrusive to change the networking core for something that has so far only been seen with devices belonging to one subsystem, but I will do it if required. I guess David M. will have an opinion on if we should add this fix on a per-driver basis, in usbnet or in the networking core? If we go for the latter, what would be correct place for this piece of code, register_netdevice()? -Kristian -- 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-next] cdc_ether: Improve ZTE MF823/831/910 handling
On Mon, Jul 18, 2016 at 4:14 PM, Oliver Neukum wrote: >> Ok, sounds good. So far, I have only seen the random MAC issue with >> the three previously mentioned devices, but who knows how many else is >> out there with the same error ... I don't think it should be in the >> core ethernet code, at least not yet, but I agree it would make sense >> to move it to for example usbnet_core(). If you agree, I can prepare a >> patch for it. > > I don't see how it would be specific for a subsystem. If the patch > is correct, it belongs into the networking core. I had a look at some other drivers, and I think we need to be very careful about making setting a random MAC too generic. For example, we might be unlucky and break the possibly_iphdr()-code/assumption in qmi_wwan.c. And there is probably more code/assumptions like that in the network stack. -Kristian -- 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] doc: usb: ci-hdrc-usb2: add property over-current-polarity
Greetings, On 07/18/2016 04:15 AM, Li Jun wrote: > Adding over-current-polarity to indicate the over current flag > is low active or high active. > > Signed-off-by: Li Jun > --- > Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt > b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt > index 341dc67..c5d35f4 100644 > --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt > +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt > @@ -81,6 +81,8 @@ i.mx specific properties > - fsl,usbmisc: phandler of non-core register device, with one >argument that indicate usb controller index > - disable-over-current: disable over current detect > +- over-current-polarity: 0 if the over current signal polarity is low active, > + 1 if the over current signal polarity is high active. > - external-vbus-divider: enables off-chip resistor divider for Vbus > > Example: The gpio device bindings already have active low/ active high. Could that be used here? In the imx SPI subsystem, for instance, the developers used gpio rather than MUXED spi chipselect lines for exactly this kind of flexibility. If there is no magic happening in silicon (which would seem to be the case if we can handle inverting polarity in the driver), I would suggest going the gpiod route. Thanks, Joshua -- 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: Fwd: USB HID problem
On Sun, Jul 17, 2016 at 12:43 PM, Alan Stern wrote: > A USB-3.1 device should work okay with a USB-3 driver. However, it > would help if you upgrade to the latest available kernel version. It is the latest available openSUSE release. I've downloaded the latest Slackware release. I'll try it later today. A different USB 3.0/1 hub has the same problem. Genesys Logic again, but a 05e3:0617 instead of :0610. usb 2-2: New USB device found, idVendor=05e3, idProduct=0617 usb 2-2: New USB device strings: Mfr=1, Product=2, SerialNumber=0 usb 2-2: Product: USB3.0 Hub usb 2-2: Manufacturer: GenesysLogic The 0610 seems to be 3 years old, so I guess there is a long adoption delay. -- 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 3/4] ARM: dts: imx7d-sdb: Set OTG2 oc polarity to be active low
Set OTG2 over current flag to be active low, OTG1 OC is not used
by default.
Signed-off-by: Li Jun
---
arch/arm/boot/dts/imx7d-sdb.dts | 8
1 file changed, 8 insertions(+)
diff --git a/arch/arm/boot/dts/imx7d-sdb.dts b/arch/arm/boot/dts/imx7d-sdb.dts
index b267f79..44ed399 100644
--- a/arch/arm/boot/dts/imx7d-sdb.dts
+++ b/arch/arm/boot/dts/imx7d-sdb.dts
@@ -287,7 +287,9 @@
&usbotg2 {
vbus-supply = <®_usb_otg2_vbus>;
+ pinctrl-0 = <&pinctrl_usbotg2>;
dr_mode = "host";
+ over-current-polarity = <0>;
status = "okay";
};
@@ -415,6 +417,12 @@
>;
};
+ pinctrl_usbotg2: usbotg2grp {
+ fsl,pins = <
+ MX7D_PAD_GPIO1_IO06__USB_OTG2_OC0x59
+ >;
+ };
+
pinctrl_usdhc1: usdhc1grp {
fsl,pins = <
MX7D_PAD_SD1_CMD__SD1_CMD 0x59
--
1.9.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: Fwd: USB HID problem
On Mon, 18 Jul 2016, Bruce Korb wrote: > On Sun, Jul 17, 2016 at 12:43 PM, Alan Stern > wrote: > > A USB-3.1 device should work okay with a USB-3 driver. However, it > > would help if you upgrade to the latest available kernel version. > > It is the latest available openSUSE release. The release doesn't matter, only the kernel version. You can use either release with a more recent kernel. > I've downloaded the latest Slackware release. I'll try it later today. > A different USB 3.0/1 hub has the same problem. Genesys Logic > again, but a 05e3:0617 instead of :0610. This is not surprising, because the problem probably stems from the xHCI host controller in your computer, not from the hub. Alan Stern -- 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: Fwd: USB HID problem
On Mon, Jul 18, 2016 at 2:24 PM, Alan Stern wrote: > This is not surprising, because the problem probably stems from the > xHCI host controller in your computer, not from the hub. Gigabyte is supposed to make reliable motherboards. Haven't been trying to use USB3 until now, but still Given the repeatability, maybe I can dig into where stuff is going awry. Maybe I can trace down how the bogus speed information is derived. I do not believe "Link Power Management" is enabled, but don't know exactly how to tell, either. It would not be terribly hard to make xhci_hcd more noisy. dmidecode output: # dmidecode 2.12 # SMBIOS entry point at 0x000f04c0 SMBIOS 2.7 present. 58 structures occupying 2435 bytes. Table at 0xBF5C4018. Handle 0x, DMI type 0, 24 bytes BIOS Information Vendor: American Megatrends Inc. Version: FA Release Date: 10/23/2012 Address: 0xF Runtime Size: 64 kB ROM Size: 4096 kB Characteristics: PCI is supported BIOS is upgradeable BIOS shadowing is allowed Boot from CD is supported Selectable boot is supported BIOS ROM is socketed EDD is supported 5.25"/1.2 MB floppy services are supported (int 13h) 3.5"/720 kB floppy services are supported (int 13h) 3.5"/2.88 MB floppy services are supported (int 13h) Print screen service is supported (int 5h) 8042 keyboard services are supported (int 9h) Serial services are supported (int 14h) Printer services are supported (int 17h) ACPI is supported USB legacy is supported BIOS boot specification is supported Targeted content distribution is supported UEFI is supported BIOS Revision: 4.6 Handle 0x0001, DMI type 1, 27 bytes System Information Manufacturer: Gigabyte Technology Co., Ltd. Product Name: To be filled by O.E.M. Version: To be filled by O.E.M. Serial Number: To be filled by O.E.M. UUID: 03DE0294-0480-050D-9C06-440700080009 Wake-up Type: Power Switch SKU Number: To be filled by O.E.M. Family: To be filled by O.E.M. Handle 0x0002, DMI type 2, 15 bytes Base Board Information Manufacturer: AMD Corporation Product Name: 990FXA-UD3 Version: x.x Serial Number: To be filled by O.E.M. Asset Tag: To be filled by O.E.M. Features: Board is a hosting board Board is replaceable Location In Chassis: To be filled by O.E.M. Chassis Handle: 0x0003 Type: Motherboard Contained Object Handles: 0 Handle 0x0003, DMI type 3, 22 bytes Chassis Information Manufacturer: Gigabyte Technology Co., Ltd. Type: Desktop Lock: Not Present Version: To Be Filled By O.E.M. Serial Number: To Be Filled By O.E.M. Asset Tag: To Be Filled By O.E.M. Boot-up State: Safe Power Supply State: Safe Thermal State: Safe Security Status: None OEM Information: 0x Height: Unspecified Number Of Power Cords: 1 Contained Elements: 0 SKU Number: To be filled by O.E.M. [...] Handle 0x0012, DMI type 8, 9 bytes Port Connector Information Internal Reference Designator: J3A1 Internal Connector Type: None External Reference Designator: USB1 External Connector Type: Access Bus (USB) Port Type: USB Handle 0x0013, DMI type 8, 9 bytes Port Connector Information Internal Reference Designator: J3A1 Internal Connector Type: None External Reference Designator: USB2 External Connector Type: Access Bus (USB) Port Type: USB Handle 0x0014, DMI type 8, 9 bytes Port Connector Information Internal Reference Designator: J3A1 Internal Connector Type: None External Reference Designator: USB3 External Connector Type: Access Bus (USB) Port Type: USB -- 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: Fwd: USB HID problem
On Mon, 18 Jul 2016, Bruce Korb wrote: > On Mon, Jul 18, 2016 at 2:24 PM, Alan Stern wrote: > > This is not surprising, because the problem probably stems from the > > xHCI host controller in your computer, not from the hub. > > Gigabyte is supposed to make reliable motherboards. > Haven't been trying to use USB3 until now, but still > > Given the repeatability, maybe I can dig into where stuff is going awry. > Maybe I can trace down how the bogus speed information is derived. If my guess was correct, there is no bogus speed information. > I do not believe "Link Power Management" is enabled, but don't know > exactly how to tell, either. It would not be terribly hard to make xhci_hcd > more noisy. LPM is enabled by default. You can turn it off in later kernels by adding a quirk entry, but I believe 4.1 doesn't include the LPM quirk. On the other hand, you can turn off all LPM in 4.1 very easily by editing the usb_device_supports_lpm() routine in drivers/usb/core/hub.c. Make it return 0 immediately. That will be an easy way to tell whether my guess was right. Alan Stern -- 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] doc: usb: ci-hdrc-usb2: add property over-current-polarity
On Mon, Jul 18, 2016 at 08:24:50AM -0700, Joshua Clayton wrote: > Greetings, > On 07/18/2016 04:15 AM, Li Jun wrote: > > Adding over-current-polarity to indicate the over current flag > > is low active or high active. > > > > Signed-off-by: Li Jun > > --- > > Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt > > b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt > > index 341dc67..c5d35f4 100644 > > --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt > > +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt > > @@ -81,6 +81,8 @@ i.mx specific properties > > - fsl,usbmisc: phandler of non-core register device, with one > >argument that indicate usb controller index > > - disable-over-current: disable over current detect > > +- over-current-polarity: 0 if the over current signal polarity is low > > active, > > + 1 if the over current signal polarity is high active. > > - external-vbus-divider: enables off-chip resistor divider for Vbus > > > > Example: > The gpio device bindings already have active low/ active high. > Could that be used here? > No, the over current pin is a dedicated pin for USB, you need to configure it through pinmux. If we are using a gpio (even the same pin as OC pin), the USB subsystem will not know OC event. The polarity is for OC pin and configured through USB register. > In the imx SPI subsystem, for instance, the developers used gpio rather than > MUXED spi chipselect lines for exactly this kind of flexibility. > > If there is no magic happening in silicon (which would seem to be the case if > we can > handle inverting polarity in the driver), I would suggest going the gpiod > route. > -- Best Regards, Peter Chen -- 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] usb: chipidea: imx: set over current polarity per dts setting
On Mon, Jul 18, 2016 at 07:15:47PM +0800, Li Jun wrote:
> With over-current-polarity property added, imx usb over current
> polarity can be configed to be low or high active, since the default
> setting value(0) is for active high, so keep this setting for those
> legacy platforms without this property specified.
>
> Signed-off-by: Li Jun
> ---
> drivers/usb/chipidea/ci_hdrc_imx.c | 9 +
> drivers/usb/chipidea/ci_hdrc_imx.h | 1 +
> drivers/usb/chipidea/usbmisc_imx.c | 30 +-
> 3 files changed, 35 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c
> b/drivers/usb/chipidea/ci_hdrc_imx.c
> index dedc33e..61e712b 100644
> --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> @@ -140,6 +140,15 @@ static struct imx_usbmisc_data
> *usbmisc_get_init_data(struct device *dev)
> if (of_find_property(np, "disable-over-current", NULL))
> data->disable_oc = 1;
>
> + if (!of_property_read_u32(np, "over-current-polarity", &ret))
> + data->oc_polarity = ret ? 1 : 0;
> + else
> + /*
> + * Keep the oc polarity setting of legacy
> + * platforms unchanged.
> + */
> + data->oc_polarity = 1;
We may can't ensure that, since there are lots of i.mx platforms, eg
from imx27 to imx7.
If the user does not assign oc polarity at DT, we need to read it from
register.
Peter
> +
> if (of_find_property(np, "external-vbus-divider", NULL))
> data->evdo = 1;
>
> diff --git a/drivers/usb/chipidea/ci_hdrc_imx.h
> b/drivers/usb/chipidea/ci_hdrc_imx.h
> index 635717e..409aa5ca8 100644
> --- a/drivers/usb/chipidea/ci_hdrc_imx.h
> +++ b/drivers/usb/chipidea/ci_hdrc_imx.h
> @@ -17,6 +17,7 @@ struct imx_usbmisc_data {
> int index;
>
> unsigned int disable_oc:1; /* over current detect disabled */
> + unsigned int oc_polarity:1; /* over current polarity if oc enabled */
> unsigned int evdo:1; /* set external vbus divider option */
> };
>
> diff --git a/drivers/usb/chipidea/usbmisc_imx.c
> b/drivers/usb/chipidea/usbmisc_imx.c
> index ab8b027..193dbe4 100644
> --- a/drivers/usb/chipidea/usbmisc_imx.c
> +++ b/drivers/usb/chipidea/usbmisc_imx.c
> @@ -56,6 +56,7 @@
>
> #define MX6_BM_NON_BURST_SETTING BIT(1)
> #define MX6_BM_OVER_CUR_DIS BIT(7)
> +#define MX6_BM_OVER_CUR_POLARITY BIT(8)
> #define MX6_BM_WAKEUP_ENABLE BIT(10)
> #define MX6_BM_ID_WAKEUP BIT(16)
> #define MX6_BM_VBUS_WAKEUP BIT(17)
> @@ -266,11 +267,18 @@ static int usbmisc_imx6q_init(struct imx_usbmisc_data
> *data)
>
> spin_lock_irqsave(&usbmisc->lock, flags);
>
> + reg = readl(usbmisc->base + data->index * 4);
> if (data->disable_oc) {
> - reg = readl(usbmisc->base + data->index * 4);
> - writel(reg | MX6_BM_OVER_CUR_DIS,
> - usbmisc->base + data->index * 4);
> + reg |= MX6_BM_OVER_CUR_DIS;
> + } else if (data->oc_polarity == 1) {
> + /* High active */
> + reg &= ~(MX6_BM_OVER_CUR_DIS | MX6_BM_OVER_CUR_POLARITY);
> + } else {
> + /* Low active */
> + reg &= ~MX6_BM_OVER_CUR_DIS;
> + reg |= MX6_BM_OVER_CUR_POLARITY;
> }
> + writel(reg, usbmisc->base + data->index * 4);
>
> /* SoC non-burst setting */
> reg = readl(usbmisc->base + data->index * 4);
> @@ -365,10 +373,18 @@ static int usbmisc_imx7d_init(struct imx_usbmisc_data
> *data)
> return -EINVAL;
>
> spin_lock_irqsave(&usbmisc->lock, flags);
> + reg = readl(usbmisc->base);
> if (data->disable_oc) {
> - reg = readl(usbmisc->base);
> - writel(reg | MX6_BM_OVER_CUR_DIS, usbmisc->base);
> + reg |= MX6_BM_OVER_CUR_DIS;
> + } else if (data->oc_polarity == 1) {
> + /* High active */
> + reg &= ~(MX6_BM_OVER_CUR_DIS | MX6_BM_OVER_CUR_POLARITY);
> + } else {
> + /* Low active */
> + reg &= ~MX6_BM_OVER_CUR_DIS;
> + reg |= MX6_BM_OVER_CUR_POLARITY;
> }
> + writel(reg, usbmisc->base);
>
> reg = readl(usbmisc->base + MX7D_USBNC_USB_CTRL2);
> reg &= ~MX7D_USB_VBUS_WAKEUP_SOURCE_MASK;
> @@ -492,6 +508,10 @@ static const struct of_device_id usbmisc_imx_dt_ids[] = {
> .compatible = "fsl,imx6ul-usbmisc",
> .data = &imx6sx_usbmisc_ops,
> },
> + {
> + .compatible = "fsl,imx7d-usbmisc",
> + .data = &imx7d_usbmisc_ops,
> + },
> { /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(of, usbmisc_imx_dt_ids);
> --
> 1.9.1
>
>
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
Best Regards,
Peter Chen
--
To unsubscribe
Re: [PATCH 4/4] usb: chipidea: imx: set over current polarity per dts setting
On Tue, Jul 19, 2016 at 02:31:41AM +, Jun Li wrote:
> Hi
>
> > -Original Message-
> > From: Peter Chen [mailto:hzpeterc...@gmail.com]
> > Sent: Tuesday, July 19, 2016 9:57 AM
> > To: Jun Li
> > Cc: Peter Chen ; robh...@kernel.org;
> > shawn...@kernel.org; devicet...@vger.kernel.org; linux-usb@vger.kernel.org;
> > linux-arm-ker...@lists.infradead.org
> > Subject: Re: [PATCH 4/4] usb: chipidea: imx: set over current polarity per
> > dts setting
> >
> > On Mon, Jul 18, 2016 at 07:15:47PM +0800, Li Jun wrote:
> > > With over-current-polarity property added, imx usb over current
> > > polarity can be configed to be low or high active, since the default
> > > setting value(0) is for active high, so keep this setting for those
> > > legacy platforms without this property specified.
> > >
> > > Signed-off-by: Li Jun
> > > ---
> > > drivers/usb/chipidea/ci_hdrc_imx.c | 9 +
> > > drivers/usb/chipidea/ci_hdrc_imx.h | 1 +
> > > drivers/usb/chipidea/usbmisc_imx.c | 30 +-
> > > 3 files changed, 35 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c
> > > b/drivers/usb/chipidea/ci_hdrc_imx.c
> > > index dedc33e..61e712b 100644
> > > --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> > > +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> > > @@ -140,6 +140,15 @@ static struct imx_usbmisc_data
> > *usbmisc_get_init_data(struct device *dev)
> > > if (of_find_property(np, "disable-over-current", NULL))
> > > data->disable_oc = 1;
> > >
> > > + if (!of_property_read_u32(np, "over-current-polarity", &ret))
> > > + data->oc_polarity = ret ? 1 : 0;
> > > + else
> > > + /*
> > > + * Keep the oc polarity setting of legacy
> > > + * platforms unchanged.
> > > + */
> > > + data->oc_polarity = 1;
> >
> > We may can't ensure that, since there are lots of i.mx platforms, eg from
> > imx27 to imx7.
> >
> > If the user does not assign oc polarity at DT, we need to read it from
> > register.
>
> I suppose old platforms (other than i.mx6&7) should set either
> "over-current-polarity" or "disable-over-current" if want to use
> "data->oc_polarity".
>
The user may use old dtb and the latest kernel, so the introduced
DT property should not affect old dtb platforms.
> Do you mean read the register value before set "data->oc_polarity" here?
yes, if there is a dts property, read it from DT; else, read it from the
register.
if (!of_property_read_u32(np, "over-current-polarity", &ret))
data->oc_polarity = ret ? 1 : 0;
else
/*
* Keep the oc polarity setting of legacy
* platforms unchanged.
*/
data->oc_polarity = readl(oc_polarity);
Then, when you set register for oc polarity, you can depend on the
flag.
reg = readl(usbmisc->base + data->index * 4);
value = data->oc_polarity ? MX6_BM_OVER_CUR_POLARITY : 0;
writel(reg | value, usbmisc->base + data->index * 4);
> Even with that, as I still can't ensure all i.mx platforms have the same
> mapping:
> 0 <--> active high.
> 1 <--> active low.
> How should I set "data->oc_polarity" accordingly?
Just keep current register value unchanging if there is no dts property.
Peter
> Li Jun
> >
> > Peter
> > > +
> > > if (of_find_property(np, "external-vbus-divider", NULL))
> > > data->evdo = 1;
> > >
> > > diff --git a/drivers/usb/chipidea/ci_hdrc_imx.h
> > > b/drivers/usb/chipidea/ci_hdrc_imx.h
> > > index 635717e..409aa5ca8 100644
> > > --- a/drivers/usb/chipidea/ci_hdrc_imx.h
> > > +++ b/drivers/usb/chipidea/ci_hdrc_imx.h
> > > @@ -17,6 +17,7 @@ struct imx_usbmisc_data {
> > > int index;
> > >
> > > unsigned int disable_oc:1; /* over current detect disabled */
> > > + unsigned int oc_polarity:1; /* over current polarity if oc enabled
> > > +*/
> > > unsigned int evdo:1; /* set external vbus divider option */ };
> > >
> > > diff --git a/drivers/usb/chipidea/usbmisc_imx.c
> > > b/drivers/usb/chipidea/usbmisc_imx.c
> > > index ab8b027..193dbe4 100644
> > > --- a/drivers/usb/chipidea/usbmisc_imx.c
> > > +++ b/drivers/usb/chipidea/usbmisc_imx.c
> > > @@ -56,6 +56,7 @@
> > >
> > > #define MX6_BM_NON_BURST_SETTING BIT(1)
> > > #define MX6_BM_OVER_CUR_DIS BIT(7)
> > > +#define MX6_BM_OVER_CUR_POLARITY BIT(8)
> > > #define MX6_BM_WAKEUP_ENABLE BIT(10)
> > > #define MX6_BM_ID_WAKEUP BIT(16)
> > > #define MX6_BM_VBUS_WAKEUP BIT(17)
> > > @@ -266,11 +267,18 @@ static int usbmisc_imx6q_init(struct
> > > imx_usbmisc_data *data)
> > >
> > > spin_lock_irqsave(&usbmisc->lock, flags);
> > >
> > > + reg = readl(usbmisc->base + data->index * 4);
> > > if (data->disable_oc) {
> > > - reg = readl(usbmisc->base + data->index * 4);
> > > - writel(reg | MX6_BM_OVER_CUR_DIS,
> > > - usbmisc->base + data->index * 4);
> > > + reg |= MX6_BM_OVER_CUR_DIS;
> > > + } else if (data->oc_polarity ==
RE: [PATCH 4/4] usb: chipidea: imx: set over current polarity per dts setting
Hi
> -Original Message-
> From: Peter Chen [mailto:hzpeterc...@gmail.com]
> Sent: Tuesday, July 19, 2016 9:57 AM
> To: Jun Li
> Cc: Peter Chen ; robh...@kernel.org;
> shawn...@kernel.org; devicet...@vger.kernel.org; linux-usb@vger.kernel.org;
> linux-arm-ker...@lists.infradead.org
> Subject: Re: [PATCH 4/4] usb: chipidea: imx: set over current polarity per
> dts setting
>
> On Mon, Jul 18, 2016 at 07:15:47PM +0800, Li Jun wrote:
> > With over-current-polarity property added, imx usb over current
> > polarity can be configed to be low or high active, since the default
> > setting value(0) is for active high, so keep this setting for those
> > legacy platforms without this property specified.
> >
> > Signed-off-by: Li Jun
> > ---
> > drivers/usb/chipidea/ci_hdrc_imx.c | 9 +
> > drivers/usb/chipidea/ci_hdrc_imx.h | 1 +
> > drivers/usb/chipidea/usbmisc_imx.c | 30 +-
> > 3 files changed, 35 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c
> > b/drivers/usb/chipidea/ci_hdrc_imx.c
> > index dedc33e..61e712b 100644
> > --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> > +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> > @@ -140,6 +140,15 @@ static struct imx_usbmisc_data
> *usbmisc_get_init_data(struct device *dev)
> > if (of_find_property(np, "disable-over-current", NULL))
> > data->disable_oc = 1;
> >
> > + if (!of_property_read_u32(np, "over-current-polarity", &ret))
> > + data->oc_polarity = ret ? 1 : 0;
> > + else
> > + /*
> > +* Keep the oc polarity setting of legacy
> > +* platforms unchanged.
> > +*/
> > + data->oc_polarity = 1;
>
> We may can't ensure that, since there are lots of i.mx platforms, eg from
> imx27 to imx7.
>
> If the user does not assign oc polarity at DT, we need to read it from
> register.
I suppose old platforms (other than i.mx6&7) should set either
"over-current-polarity" or "disable-over-current" if want to use
"data->oc_polarity".
Do you mean read the register value before set "data->oc_polarity" here?
Even with that, as I still can't ensure all i.mx platforms have the same
mapping:
0 <--> active high.
1 <--> active low.
How should I set "data->oc_polarity" accordingly?
Li Jun
>
> Peter
> > +
> > if (of_find_property(np, "external-vbus-divider", NULL))
> > data->evdo = 1;
> >
> > diff --git a/drivers/usb/chipidea/ci_hdrc_imx.h
> > b/drivers/usb/chipidea/ci_hdrc_imx.h
> > index 635717e..409aa5ca8 100644
> > --- a/drivers/usb/chipidea/ci_hdrc_imx.h
> > +++ b/drivers/usb/chipidea/ci_hdrc_imx.h
> > @@ -17,6 +17,7 @@ struct imx_usbmisc_data {
> > int index;
> >
> > unsigned int disable_oc:1; /* over current detect disabled */
> > + unsigned int oc_polarity:1; /* over current polarity if oc enabled
> > +*/
> > unsigned int evdo:1; /* set external vbus divider option */ };
> >
> > diff --git a/drivers/usb/chipidea/usbmisc_imx.c
> > b/drivers/usb/chipidea/usbmisc_imx.c
> > index ab8b027..193dbe4 100644
> > --- a/drivers/usb/chipidea/usbmisc_imx.c
> > +++ b/drivers/usb/chipidea/usbmisc_imx.c
> > @@ -56,6 +56,7 @@
> >
> > #define MX6_BM_NON_BURST_SETTING BIT(1)
> > #define MX6_BM_OVER_CUR_DISBIT(7)
> > +#define MX6_BM_OVER_CUR_POLARITY BIT(8)
> > #define MX6_BM_WAKEUP_ENABLE BIT(10)
> > #define MX6_BM_ID_WAKEUP BIT(16)
> > #define MX6_BM_VBUS_WAKEUP BIT(17)
> > @@ -266,11 +267,18 @@ static int usbmisc_imx6q_init(struct
> > imx_usbmisc_data *data)
> >
> > spin_lock_irqsave(&usbmisc->lock, flags);
> >
> > + reg = readl(usbmisc->base + data->index * 4);
> > if (data->disable_oc) {
> > - reg = readl(usbmisc->base + data->index * 4);
> > - writel(reg | MX6_BM_OVER_CUR_DIS,
> > - usbmisc->base + data->index * 4);
> > + reg |= MX6_BM_OVER_CUR_DIS;
> > + } else if (data->oc_polarity == 1) {
> > + /* High active */
> > + reg &= ~(MX6_BM_OVER_CUR_DIS | MX6_BM_OVER_CUR_POLARITY);
> > + } else {
> > + /* Low active */
> > + reg &= ~MX6_BM_OVER_CUR_DIS;
> > + reg |= MX6_BM_OVER_CUR_POLARITY;
> > }
> > + writel(reg, usbmisc->base + data->index * 4);
> >
> > /* SoC non-burst setting */
> > reg = readl(usbmisc->base + data->index * 4); @@ -365,10 +373,18 @@
> > static int usbmisc_imx7d_init(struct imx_usbmisc_data *data)
> > return -EINVAL;
> >
> > spin_lock_irqsave(&usbmisc->lock, flags);
> > + reg = readl(usbmisc->base);
> > if (data->disable_oc) {
> > - reg = readl(usbmisc->base);
> > - writel(reg | MX6_BM_OVER_CUR_DIS, usbmisc->base);
> > + reg |= MX6_BM_OVER_CUR_DIS;
> > + } else if (data->oc_polarity == 1) {
> > + /* High active */
> > + reg &= ~(MX6_BM_OVER_CUR_DIS | MX6_BM_OVER_CUR_POLARITY);
> > + } else {
> > + /* Low active */
> > +
Re: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support
On Sat, Jul 16, 2016 at 08:49:53AM +0900, Greg Kroah-Hartman wrote:
> On Fri, Jul 15, 2016 at 03:41:10PM -0700, Bin Gao wrote:
> > On Fri, Jul 15, 2016 at 02:21:48PM +0300, Felipe Balbi wrote:
> > > Greg Kroah-Hartman writes:
> > > > On Fri, Jul 15, 2016 at 01:38:12PM +0300, Felipe Balbi wrote:
> > > >>
> > > >> Hi,
> > > >>
> > > >> Bin Gao writes:
> > > >> > +static void print_message(int port, bool is_cmsg, u8 msg, bool recv)
> > > >> > +{
> > > >> > +pr_info("sink port %d: %s message %s %s\n", port,
> > > >> > +is_cmsg ? "Control" : "Data",
> > > >> > +msg_to_string(is_cmsg, msg),
> > > >> > + recv ? "received" : "sent(wait GOODCRC)");
> > > >> > +}
> > > >>
> > > >> this is problematic. By default, we're all using 115200 8N1 baud
> > > >> rate. This message alone prints anywhere from 50 to 100 characters (I
> > > >> didn't really count properly, these are rough numbers), and that takes:
> > > >>
> > > >> n50chars_time = 50 / (115200 / 10) = 4.3ms
> > > >> n100chars_time = 100 / (115200 / 10) = 8.6ms
> > > >>
> > > >> Considering you have 30ms to reply with Power Request after GoodCRC,
> > > >> and
> > > >> considering you're printing several of these messages, they become
> > > >> really expensive and eat up valuable time from tSenderReply.
> > > >
> > > > printk() should be async, so it shouldn't be that big of a deal.
> > >
> > > I can actually see this causing problems ;-) With this pr_info(),
> > > sometimes tSenderReply times out and Source gives a HardReset. Without
> > > pr_info(), type-c analyzer tells me we reply in less than 1ms.
> > >
> > > > What is wrong is that this isn't using dev_info().
> > >
> > > right, that too.
> > >
> > > --
> > > balbi
> >
> > When we don't have a struct device pointer for this driver,
>
> Then you should fix that, as this is a driver for hardware :)
This is actualy a software stack to implement the USB PD spec.
Only the USB Type-C phy driver has a device pointer.
The PD stack vs. USB Type-C phy driver is similar to TCP/IP stack
vs. ethernet driver in the kernel. We don't have a device pointer
for TCP/IP stack code either.
Thanks,
Bin
>
> thanks,
>
> greg k-h
--
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/2] usb: typec: Add USB Power Delivery sink port support
On Mon, Jul 18, 2016 at 10:07:24AM +0300, Felipe Balbi wrote:
>
> Hi,
>
> Bin Gao writes:
> >> > +int pd_sink_queue_msg(struct pd_sink_msg *msg)
> >> > +{
> >> > +unsigned long flags;
> >> > +struct pd_sink_port *port;
> >> > +
> >> > +if (msg->port < 0 || msg->port >= MAX_NR_SINK_PORTS) {
> >> > +pr_err("Invalid port number\n");
> >> > +return -EINVAL;
> >> > +}
> >> > +
> >> > +port = sink_ports[msg->port];
> >> > +
> >> > +spin_lock_irqsave(&port->rx_lock, flags);
> >> > +list_add_tail(&msg->list, &port->rx_list);
> >> > +spin_unlock_irqrestore(&port->rx_lock, flags);
> >> > +
> >> > +queue_work(port->rx_wq, &port->rx_work);
> >>
> >> can we really queue several messages at a time? It seems unfeasible to
> >> me. It's not like we can queue several power request in a role. Why do
> >> you need this workqueue? Why don't you process message here, in place?
> > Some Type-C chargers send two messages in a short duration(less than 1 ms),
> > e.g. a SOURCE_CAPABILITY follows the previous SOURCE_CAPABILITY, or a
> > GET_SINK_CAPABILITY follows a previous SOURCE_CAPABILITY, etc. Queuing
> > message to PD stack by Type-C phy driver typically happens in a interrupt
> > context. So in this case a nested interrupt may happen. Our whole PD
> > stack while processing one message is not re-entrant so the nested
> > interrupt would cause a problem.
>
> keep interrupts masked for as long as necessary until your message is
> processed.
Yes, that's a right way to go.
We'll have to document this because there might be other Type-C
PHY drivers(other than Intel Whiskey Cove PMIC) to use the PD stack.
>
> --
> balbi
--
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-next] cdc_ether: Improve ZTE MF823/831/910 handling
On Mon, 2016-07-18 at 17:04 +0200, Kristian Evensen wrote: > On Mon, Jul 18, 2016 at 4:14 PM, Oliver Neukum wrote: > >> Ok, sounds good. So far, I have only seen the random MAC issue with > >> the three previously mentioned devices, but who knows how many else is > >> out there with the same error ... I don't think it should be in the > >> core ethernet code, at least not yet, but I agree it would make sense > >> to move it to for example usbnet_core(). If you agree, I can prepare a > >> patch for it. > > > > I don't see how it would be specific for a subsystem. If the patch > > is correct, it belongs into the networking core. > > I had a look at some other drivers, and I think we need to be very > careful about making setting a random MAC too generic. For example, we > might be unlucky and break the possibly_iphdr()-code/assumption in > qmi_wwan.c. And there is probably more code/assumptions like that in > the network stack. In this case please use special cases for usbnet, too. We need a quirk. Regards Oliver -- 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-next] cdc_ether: Improve ZTE MF823/831/910 handling
On Tue, Jul 19, 2016 at 8:20 AM, Oliver Neukum wrote: >> I had a look at some other drivers, and I think we need to be very >> careful about making setting a random MAC too generic. For example, we >> might be unlucky and break the possibly_iphdr()-code/assumption in >> qmi_wwan.c. And there is probably more code/assumptions like that in >> the network stack. > > In this case please use special cases for usbnet, too. > We need a quirk. I guess I can match on the VID/PID in usbnet, but won't it be cleaner to add a new bind() function (in cdc_ether) which matches the two PIDs and leave usbnet as is? Or am I misunderstanding how to add this functionality to usbnet? -Kristian -- 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
