This patchset adds support for the TC7USB40MU usb mux found on
db410c 96boards platforms via the new multiplexer framework and
hooks that into the chipidea driver. This allows us to properly
control host or device mode on this board via the sysfs knob.
So far I've only tested this on db410c, so I need to test it on
db8974 and ifc6410 still. But role switching and cable connect/disconnect
on the micro-usb port and gadget looks to be working on db410c.
Changes from v1:
* Dropped mux driver and used gpio-mux instead
* Fixed role switch by moving mux selecting in udc.c to right place
TODO:
1. The userspace side of things is murky. What is expected to go and toggle
the host/gadget side of things in userspace at this very specific location
for chipidea devices?
Stephen Boyd (3):
mux: Add mux_control_get_optional() API
usb: chipidea: Hook into mux framework to toggle usb switch
arm64: dts: qcom: Collapse usb support into one node
.../devicetree/bindings/usb/ci-hdrc-usb2.txt | 6 ++
Documentation/driver-model/devres.txt | 1 +
arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi | 38 +++++----
arch/arm64/boot/dts/qcom/msm8916.dtsi | 62 +++++++-------
drivers/mux/mux-core.c | 98 +++++++++++++++++-----
drivers/usb/chipidea/Kconfig | 1 +
drivers/usb/chipidea/core.c | 5 ++
drivers/usb/chipidea/host.c | 7 ++
drivers/usb/chipidea/udc.c | 13 ++-
include/linux/mux/consumer.h | 4 +
include/linux/usb/chipidea.h | 2 +
11 files changed, 166 insertions(+), 71 deletions(-)
--
2.10.0.297.gf6727b0
We currently have three device nodes for the same USB hardware
block, as evident by the reuse of the same reg address multiple
times. Now that the chipidea driver fully supports OTG with the
MSM wrapper we can collapse all these nodes into one USB device
node, reflecting the true nature of the hardware.
Signed-off-by: Stephen Boyd <[email protected]>
---
arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi | 38 ++++++++++---------
arch/arm64/boot/dts/qcom/msm8916.dtsi | 62 +++++++++++++++----------------
2 files changed, 50 insertions(+), 50 deletions(-)
diff --git a/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi b/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
index f326f4fb4d72..494560a1a6ef 100644
--- a/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
+++ b/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
@@ -213,24 +213,20 @@
};
usb@78d9000 {
- extcon = <&usb_id>, <&usb_id>;
+ extcon = <&usb_id>;
status = "okay";
- };
-
- ehci@78d9000 {
- status = "okay";
- };
-
- phy@78d9000 {
- v1p8-supply = <&pm8916_l7>;
- v3p3-supply = <&pm8916_l13>;
- vddcx-supply = <&pm8916_s1>;
- extcon = <&usb_id>, <&usb_id>;
- dr_mode = "otg";
- status = "okay";
- switch-gpio = <&pm8916_gpios 4 GPIO_ACTIVE_HIGH>;
- pinctrl-names = "default";
- pinctrl-0 = <&usb_sw_sel_pm>;
+ adp-disable;
+ hnp-disable;
+ srp-disable;
+ mux-controls = <&usb_switch>;
+ mux-control-names = "usb_switch";
+
+ ulpi {
+ phy {
+ v1p8-supply = <&pm8916_l7>;
+ v3p3-supply = <&pm8916_l13>;
+ };
+ };
};
lpass@07708000 {
@@ -348,6 +344,14 @@
pinctrl-0 = <&usb_id_default>;
};
+ usb_switch: usb-switch {
+ compatible = "gpio-mux";
+ mux-gpios = <&pm8916_gpios 4 GPIO_ACTIVE_HIGH>;
+ #mux-control-cells = <0>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&usb_sw_sel_pm>;
+ };
+
hdmi-out {
compatible = "hdmi-connector";
type = "a";
diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
index 17691abea608..039991f80831 100644
--- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
@@ -546,44 +546,40 @@
status = "disabled";
};
- usb_dev: usb@78d9000 {
+ otg: usb@78d9000 {
compatible = "qcom,ci-hdrc";
- reg = <0x78d9000 0x400>;
- dr_mode = "peripheral";
- interrupts = <GIC_SPI 134 IRQ_TYPE_LEVEL_HIGH>;
- usb-phy = <&usb_otg>;
- status = "disabled";
- };
-
- usb_host: ehci@78d9000 {
- compatible = "qcom,ehci-host";
- reg = <0x78d9000 0x400>;
- interrupts = <GIC_SPI 134 IRQ_TYPE_LEVEL_HIGH>;
- usb-phy = <&usb_otg>;
- status = "disabled";
- };
-
- usb_otg: phy@78d9000 {
- compatible = "qcom,usb-otg-snps";
- reg = <0x78d9000 0x400>;
+ reg = <0x78d9000 0x200>,
+ <0x78d9200 0x200>;
interrupts = <GIC_SPI 134 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 140 IRQ_TYPE_LEVEL_HIGH>;
-
- qcom,vdd-levels = <500000 1000000 1320000>;
- qcom,phy-init-sequence = <0x44 0x6B 0x24 0x13>;
- dr_mode = "peripheral";
- qcom,otg-control = <2>; // PMIC
- qcom,manual-pullup;
-
clocks = <&gcc GCC_USB_HS_AHB_CLK>,
- <&gcc GCC_USB_HS_SYSTEM_CLK>,
- <&gcc GCC_USB2A_PHY_SLEEP_CLK>;
- clock-names = "iface", "core", "sleep";
-
- resets = <&gcc GCC_USB2A_PHY_BCR>,
- <&gcc GCC_USB_HS_BCR>;
- reset-names = "phy", "link";
+ <&gcc GCC_USB_HS_SYSTEM_CLK>;
+ clock-names = "iface", "core";
+ assigned-clocks = <&gcc GCC_USB_HS_SYSTEM_CLK>;
+ assigned-clock-rates = <80000000>;
+ resets = <&gcc GCC_USB_HS_BCR>;
+ reset-names = "core";
+ phy_type = "ulpi";
+ dr_mode = "otg";
+ ahb-burst-config = <0>;
+ phy-names = "usb-phy";
+ phys = <&usb_hs_phy>;
status = "disabled";
+ #reset-cells = <1>;
+
+ ulpi {
+ usb_hs_phy: phy {
+ compatible = "qcom,usb-hs-phy-msm8916",
+ "qcom,usb-hs-phy";
+ #phy-cells = <0>;
+ clocks = <&xo_board>, <&gcc GCC_USB2A_PHY_SLEEP_CLK>;
+ clock-names = "ref", "sleep";
+ resets = <&gcc GCC_USB2A_PHY_BCR>, <&otg 0>;
+ reset-names = "phy", "por";
+ qcom,init-seq = /bits/ 8 <0x0 0x44
+ 0x1 0x6b 0x2 0x24 0x3 0x13>;
+ };
+ };
};
intc: interrupt-controller@b000000 {
--
2.10.0.297.gf6727b0
On the db410c 96boards platform we have a TC7USB40MU on the board
to mux the D+/D- lines coming from the controller between a micro
usb "device" port and a USB hub for "host" roles[1]. During a
role switch, we need to toggle this mux to forward the D+/D-
lines to either the port or the hub. Add the necessary code to do
the role switch in chipidea core via the generic mux framework.
Board configurations like on db410c are expected to change roles
via the sysfs API described in
Documentation/ABI/testing/sysfs-platform-chipidea-usb2.
[1] https://github.com/96boards/documentation/raw/master/ConsumerEdition/DragonBoard-410c/HardwareDocs/Schematics_DragonBoard.pdf
Cc: Peter Rosin <[email protected]>
Cc: Peter Chen <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt | 6 ++++++
drivers/usb/chipidea/Kconfig | 1 +
drivers/usb/chipidea/core.c | 5 +++++
drivers/usb/chipidea/host.c | 7 +++++++
drivers/usb/chipidea/udc.c | 13 ++++++++++++-
include/linux/usb/chipidea.h | 2 ++
6 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
index 0e03344e2e8b..2e9318151df7 100644
--- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
+++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
@@ -76,6 +76,10 @@ Optional properties:
needs to make sure it does not send more than 90%
maximum_periodic_data_per_frame. The use case is multiple transactions, but
less frame rate.
+- mux-controls: The mux control for toggling host/device output of this
+ controller. It's expected that a mux state of 0 indicates device mode and a
+ mux state of 1 indicates host mode.
+- mux-control-names: Shall be "usb_switch" if mux-controls is specified.
i.mx specific properties
- fsl,usbmisc: phandler of non-core register device, with one
@@ -102,4 +106,6 @@ Example:
rx-burst-size-dword = <0x10>;
extcon = <0>, <&usb_id>;
phy-clkgate-delay-us = <400>;
+ mux-controls = <&usb_switch>;
+ mux-control-names = "usb_switch";
};
diff --git a/drivers/usb/chipidea/Kconfig b/drivers/usb/chipidea/Kconfig
index 51f4157bbecf..3798e0e69d57 100644
--- a/drivers/usb/chipidea/Kconfig
+++ b/drivers/usb/chipidea/Kconfig
@@ -3,6 +3,7 @@ config USB_CHIPIDEA
depends on ((USB_EHCI_HCD && USB_GADGET) || (USB_EHCI_HCD && !USB_GADGET) || (!USB_EHCI_HCD && USB_GADGET)) && HAS_DMA
select EXTCON
select RESET_CONTROLLER
+ select MULTIPLEXER
help
Say Y here if your system has a dual role high speed USB
controller based on ChipIdea silicon IP. It supports:
diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index b17ed3a9a304..d088c262ebb8 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -64,6 +64,7 @@
#include <linux/of.h>
#include <linux/regulator/consumer.h>
#include <linux/usb/ehci_def.h>
+#include <linux/mux/consumer.h>
#include "ci.h"
#include "udc.h"
@@ -690,6 +691,10 @@ static int ci_get_platdata(struct device *dev,
if (of_find_property(dev->of_node, "non-zero-ttctrl-ttha", NULL))
platdata->flags |= CI_HDRC_SET_NON_ZERO_TTHA;
+ platdata->usb_switch = devm_mux_control_get_optional(dev, "usb_switch");
+ if (IS_ERR(platdata->usb_switch))
+ return PTR_ERR(platdata->usb_switch);
+
ext_id = ERR_PTR(-ENODEV);
ext_vbus = ERR_PTR(-ENODEV);
if (of_property_read_bool(dev->of_node, "extcon")) {
diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
index 18cb8e46262d..9ef3ecf27ad3 100644
--- a/drivers/usb/chipidea/host.c
+++ b/drivers/usb/chipidea/host.c
@@ -25,6 +25,7 @@
#include <linux/usb/hcd.h>
#include <linux/usb/chipidea.h>
#include <linux/regulator/consumer.h>
+#include <linux/mux/consumer.h>
#include "../host/ehci.h"
@@ -175,6 +176,10 @@ static int host_start(struct ci_hdrc *ci)
if (ci_otg_is_fsm_mode(ci)) {
otg->host = &hcd->self;
hcd->self.otg_port = 1;
+ } else {
+ ret = mux_control_select(ci->platdata->usb_switch, 1);
+ if (ret)
+ goto disable_reg;
}
}
@@ -195,6 +200,8 @@ static void host_stop(struct ci_hdrc *ci)
struct usb_hcd *hcd = ci->hcd;
if (hcd) {
+ if (!ci_otg_is_fsm_mode(ci))
+ mux_control_deselect(ci->platdata->usb_switch);
if (ci->platdata->notify_event)
ci->platdata->notify_event(ci,
CI_HDRC_CONTROLLER_STOPPED_EVENT);
diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index d68b125796f9..deb18099e168 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -22,6 +22,7 @@
#include <linux/usb/gadget.h>
#include <linux/usb/otg-fsm.h>
#include <linux/usb/chipidea.h>
+#include <linux/mux/consumer.h>
#include "ci.h"
#include "udc.h"
@@ -1964,16 +1965,26 @@ void ci_hdrc_gadget_destroy(struct ci_hdrc *ci)
static int udc_id_switch_for_device(struct ci_hdrc *ci)
{
+ int ret = 0;
+
if (ci->is_otg)
/* Clear and enable BSV irq */
hw_write_otgsc(ci, OTGSC_BSVIS | OTGSC_BSVIE,
OTGSC_BSVIS | OTGSC_BSVIE);
- return 0;
+ if (!ci_otg_is_fsm_mode(ci))
+ ret = mux_control_select(ci->platdata->usb_switch, 0);
+
+ if (ci->is_otg && ret)
+ hw_write_otgsc(ci, OTGSC_BSVIE | OTGSC_BSVIS, OTGSC_BSVIS);
+
+ return ret;
}
static void udc_id_switch_for_host(struct ci_hdrc *ci)
{
+ mux_control_deselect(ci->platdata->usb_switch);
+
/*
* host doesn't care B_SESSION_VALID event
* so clear and disbale BSV irq
diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h
index c5fdfcf99828..3b27e333de1d 100644
--- a/include/linux/usb/chipidea.h
+++ b/include/linux/usb/chipidea.h
@@ -9,6 +9,7 @@
#include <linux/usb/otg.h>
struct ci_hdrc;
+struct mux_control;
/**
* struct ci_hdrc_cable - structure for external connector cable state tracking
@@ -74,6 +75,7 @@ struct ci_hdrc_platform_data {
/* VBUS and ID signal state tracking, using extcon framework */
struct ci_hdrc_cable vbus_extcon;
struct ci_hdrc_cable id_extcon;
+ struct mux_control *usb_switch;
u32 phy_clkgate_delay_us;
};
--
2.10.0.297.gf6727b0
Sometimes drivers only use muxes under certain scenarios. For
example, the chipidea usb controller may be connected to a usb
switch on some platforms, and connected directly to a usb port on
others. The driver won't know one way or the other though, so add
a mux_control_get_optional() API that allows the driver to
differentiate errors getting the mux from there not being a mux
for the driver to use at all.
Cc: Jonathan Cameron <[email protected]>
Cc: Philipp Zabel <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
Documentation/driver-model/devres.txt | 1 +
drivers/mux/mux-core.c | 98 ++++++++++++++++++++++++++++-------
include/linux/mux/consumer.h | 4 ++
3 files changed, 83 insertions(+), 20 deletions(-)
diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt
index 30e04f7a690d..4fdd3e63ff8b 100644
--- a/Documentation/driver-model/devres.txt
+++ b/Documentation/driver-model/devres.txt
@@ -342,6 +342,7 @@ MUX
devm_mux_chip_alloc()
devm_mux_chip_register()
devm_mux_control_get()
+ devm_mux_control_get_optional()
PER-CPU MEM
devm_alloc_percpu()
diff --git a/drivers/mux/mux-core.c b/drivers/mux/mux-core.c
index 90b8995f07cb..a0e5bf16f02f 100644
--- a/drivers/mux/mux-core.c
+++ b/drivers/mux/mux-core.c
@@ -289,6 +289,9 @@ EXPORT_SYMBOL_GPL(devm_mux_chip_register);
*/
unsigned int mux_control_states(struct mux_control *mux)
{
+ if (!mux)
+ return 0;
+
return mux->states;
}
EXPORT_SYMBOL_GPL(mux_control_states);
@@ -338,6 +341,9 @@ int mux_control_select(struct mux_control *mux, unsigned int state)
{
int ret;
+ if (!mux)
+ return 0;
+
ret = down_killable(&mux->lock);
if (ret < 0)
return ret;
@@ -370,6 +376,9 @@ int mux_control_try_select(struct mux_control *mux, unsigned int state)
{
int ret;
+ if (!mux)
+ return 0;
+
if (down_trylock(&mux->lock))
return -EBUSY;
@@ -398,6 +407,9 @@ int mux_control_deselect(struct mux_control *mux)
{
int ret = 0;
+ if (!mux)
+ return 0;
+
if (mux->idle_state != MUX_IDLE_AS_IS &&
mux->idle_state != mux->cached_state)
ret = mux_control_set(mux, mux->idle_state);
@@ -422,14 +434,8 @@ static struct mux_chip *of_find_mux_chip_by_node(struct device_node *np)
return dev ? to_mux_chip(dev) : NULL;
}
-/**
- * mux_control_get() - Get the mux-control for a device.
- * @dev: The device that needs a mux-control.
- * @mux_name: The name identifying the mux-control.
- *
- * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno.
- */
-struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
+struct mux_control *
+__mux_control_get(struct device *dev, const char *mux_name, bool optional)
{
struct device_node *np = dev->of_node;
struct of_phandle_args args;
@@ -441,6 +447,8 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
if (mux_name) {
index = of_property_match_string(np, "mux-control-names",
mux_name);
+ if (index == -EINVAL && optional)
+ return NULL;
if (index < 0) {
dev_err(dev, "mux controller '%s' not found\n",
mux_name);
@@ -451,6 +459,8 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
ret = of_parse_phandle_with_args(np,
"mux-controls", "#mux-control-cells",
index, &args);
+ if (ret == -ENOENT && optional)
+ return NULL;
if (ret) {
dev_err(dev, "%s: failed to get mux-control %s(%i)\n",
np->full_name, mux_name ?: "", index);
@@ -482,9 +492,35 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
get_device(&mux_chip->dev);
return &mux_chip->mux[controller];
}
+
+/**
+ * mux_control_get() - Get the mux-control for a device.
+ * @dev: The device that needs a mux-control.
+ * @mux_name: The name identifying the mux-control.
+ *
+ * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno.
+ */
+struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
+{
+ return __mux_control_get(dev, mux_name, false);
+}
EXPORT_SYMBOL_GPL(mux_control_get);
/**
+ * mux_control_get_optional() - Get the optional mux-control for a device.
+ * @dev: The device that needs a mux-control.
+ * @mux_name: The name identifying the mux-control.
+ *
+ * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno.
+ */
+struct mux_control *
+mux_control_get_optional(struct device *dev, const char *mux_name)
+{
+ return __mux_control_get(dev, mux_name, true);
+}
+EXPORT_SYMBOL_GPL(mux_control_get_optional);
+
+/**
* mux_control_put() - Put away the mux-control for good.
* @mux: The mux-control to put away.
*
@@ -492,7 +528,8 @@ EXPORT_SYMBOL_GPL(mux_control_get);
*/
void mux_control_put(struct mux_control *mux)
{
- put_device(&mux->chip->dev);
+ if (mux)
+ put_device(&mux->chip->dev);
}
EXPORT_SYMBOL_GPL(mux_control_put);
@@ -503,16 +540,8 @@ static void devm_mux_control_release(struct device *dev, void *res)
mux_control_put(mux);
}
-/**
- * devm_mux_control_get() - Get the mux-control for a device, with resource
- * management.
- * @dev: The device that needs a mux-control.
- * @mux_name: The name identifying the mux-control.
- *
- * Return: Pointer to the mux-control, or an ERR_PTR with a negative errno.
- */
-struct mux_control *devm_mux_control_get(struct device *dev,
- const char *mux_name)
+static struct mux_control *
+__devm_mux_control_get(struct device *dev, const char *mux_name, bool optional)
{
struct mux_control **ptr, *mux;
@@ -520,7 +549,7 @@ struct mux_control *devm_mux_control_get(struct device *dev,
if (!ptr)
return ERR_PTR(-ENOMEM);
- mux = mux_control_get(dev, mux_name);
+ mux = __mux_control_get(dev, mux_name, optional);
if (IS_ERR(mux)) {
devres_free(ptr);
return mux;
@@ -531,8 +560,37 @@ struct mux_control *devm_mux_control_get(struct device *dev,
return mux;
}
+
+/**
+ * devm_mux_control_get() - Get the mux-control for a device, with resource
+ * management.
+ * @dev: The device that needs a mux-control.
+ * @mux_name: The name identifying the mux-control.
+ *
+ * Return: Pointer to the mux-control, or an ERR_PTR with a negative errno.
+ */
+struct mux_control *
+devm_mux_control_get(struct device *dev, const char *mux_name)
+{
+ return __devm_mux_control_get(dev, mux_name, false);
+}
EXPORT_SYMBOL_GPL(devm_mux_control_get);
+/**
+ * devm_mux_control_get_optional() - Get the optional mux-control for a device,
+ * with resource management.
+ * @dev: The device that needs a mux-control.
+ * @mux_name: The name identifying the mux-control.
+ *
+ * Return: Pointer to the mux-control, or an ERR_PTR with a negative errno.
+ */
+struct mux_control *
+devm_mux_control_get_optional(struct device *dev, const char *mux_name)
+{
+ return __devm_mux_control_get(dev, mux_name, true);
+}
+EXPORT_SYMBOL_GPL(devm_mux_control_get_optional);
+
/*
* Using subsys_initcall instead of module_init here to try to ensure - for
* the non-modular case - that the subsystem is initialized when mux consumers
diff --git a/include/linux/mux/consumer.h b/include/linux/mux/consumer.h
index 5577e1b773c4..5e2aa046f032 100644
--- a/include/linux/mux/consumer.h
+++ b/include/linux/mux/consumer.h
@@ -24,9 +24,13 @@ int __must_check mux_control_try_select(struct mux_control *mux,
int mux_control_deselect(struct mux_control *mux);
struct mux_control *mux_control_get(struct device *dev, const char *mux_name);
+struct mux_control *mux_control_get_optional(struct device *dev,
+ const char *mux_name);
void mux_control_put(struct mux_control *mux);
struct mux_control *devm_mux_control_get(struct device *dev,
const char *mux_name);
+struct mux_control *devm_mux_control_get_optional(struct device *dev,
+ const char *mux_name);
#endif /* _LINUX_MUX_CONSUMER_H */
--
2.10.0.297.gf6727b0
Generally looks like I imagined, but there are a few nits and some
things that I'd like to do differently. Comments inline. Thanks!
On 2017-07-14 23:40, Stephen Boyd wrote:
> Sometimes drivers only use muxes under certain scenarios. For
> example, the chipidea usb controller may be connected to a usb
> switch on some platforms, and connected directly to a usb port on
> others. The driver won't know one way or the other though, so add
> a mux_control_get_optional() API that allows the driver to
> differentiate errors getting the mux from there not being a mux
> for the driver to use at all.
>
> Cc: Jonathan Cameron <[email protected]>
> Cc: Philipp Zabel <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
> Documentation/driver-model/devres.txt | 1 +
> drivers/mux/mux-core.c | 98 ++++++++++++++++++++++++++++-------
> include/linux/mux/consumer.h | 4 ++
> 3 files changed, 83 insertions(+), 20 deletions(-)
>
> diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt
> index 30e04f7a690d..4fdd3e63ff8b 100644
> --- a/Documentation/driver-model/devres.txt
> +++ b/Documentation/driver-model/devres.txt
> @@ -342,6 +342,7 @@ MUX
> devm_mux_chip_alloc()
> devm_mux_chip_register()
> devm_mux_control_get()
> + devm_mux_control_get_optional()
>
> PER-CPU MEM
> devm_alloc_percpu()
> diff --git a/drivers/mux/mux-core.c b/drivers/mux/mux-core.c
> index 90b8995f07cb..a0e5bf16f02f 100644
> --- a/drivers/mux/mux-core.c
> +++ b/drivers/mux/mux-core.c
> @@ -289,6 +289,9 @@ EXPORT_SYMBOL_GPL(devm_mux_chip_register);
> */
> unsigned int mux_control_states(struct mux_control *mux)
> {
> + if (!mux)
> + return 0;
> +
I don't think this is appropriate. For this function, it might be ok,
but...
> return mux->states;
> }
> EXPORT_SYMBOL_GPL(mux_control_states);
> @@ -338,6 +341,9 @@ int mux_control_select(struct mux_control *mux, unsigned int state)
> {
> int ret;
>
> + if (!mux)
> + return 0;
> +
...here and for other cases below it's very odd to return "ok", when
-EINVAL or something seems much more appropriate. And if -EINVAL is
returned here, the benefit of returning fake values for anything
pretty much falls apart.
I simply don't like it, and prefer if the consumer code is arranged
to not call the mux functions when the optional get() does not find
the mux.
> ret = down_killable(&mux->lock);
> if (ret < 0)
> return ret;
> @@ -370,6 +376,9 @@ int mux_control_try_select(struct mux_control *mux, unsigned int state)
> {
> int ret;
>
> + if (!mux)
> + return 0;
> +
> if (down_trylock(&mux->lock))
> return -EBUSY;
>
> @@ -398,6 +407,9 @@ int mux_control_deselect(struct mux_control *mux)
> {
> int ret = 0;
>
> + if (!mux)
> + return 0;
> +
> if (mux->idle_state != MUX_IDLE_AS_IS &&
> mux->idle_state != mux->cached_state)
> ret = mux_control_set(mux, mux->idle_state);
> @@ -422,14 +434,8 @@ static struct mux_chip *of_find_mux_chip_by_node(struct device_node *np)
> return dev ? to_mux_chip(dev) : NULL;
> }
>
> -/**
> - * mux_control_get() - Get the mux-control for a device.
> - * @dev: The device that needs a mux-control.
> - * @mux_name: The name identifying the mux-control.
> - *
> - * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno.
> - */
> -struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
> +struct mux_control *
> +__mux_control_get(struct device *dev, const char *mux_name, bool optional)
> {
> struct device_node *np = dev->of_node;
> struct of_phandle_args args;
> @@ -441,6 +447,8 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
> if (mux_name) {
> index = of_property_match_string(np, "mux-control-names",
> mux_name);
> + if (index == -EINVAL && optional)
> + return NULL;
What about -ENODATA? And if an optional mux is found here, but lookup
fails later in e.g. the of_parse_phandle_with_args call, then I think
an error should be returned. Because that seems like an indication that
DT specifies that there *should* be a mux, but then there isn't one.
> if (index < 0) {
> dev_err(dev, "mux controller '%s' not found\n",
> mux_name);
> @@ -451,6 +459,8 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
> ret = of_parse_phandle_with_args(np,
> "mux-controls", "#mux-control-cells",
> index, &args);
> + if (ret == -ENOENT && optional)
> + return NULL;
> if (ret) {
> dev_err(dev, "%s: failed to get mux-control %s(%i)\n",
> np->full_name, mux_name ?: "", index);
> @@ -482,9 +492,35 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
> get_device(&mux_chip->dev);
> return &mux_chip->mux[controller];
> }
> +
> +/**
> + * mux_control_get() - Get the mux-control for a device.
> + * @dev: The device that needs a mux-control.
> + * @mux_name: The name identifying the mux-control.
> + *
> + * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno.
> + */
> +struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
> +{
> + return __mux_control_get(dev, mux_name, false);
> +}
> EXPORT_SYMBOL_GPL(mux_control_get);
>
> /**
> + * mux_control_get_optional() - Get the optional mux-control for a device.
> + * @dev: The device that needs a mux-control.
> + * @mux_name: The name identifying the mux-control.
> + *
> + * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno.
You don't mention that NULL may be returned, or when.
> + */
> +struct mux_control *
> +mux_control_get_optional(struct device *dev, const char *mux_name)
> +{
> + return __mux_control_get(dev, mux_name, true);
> +}
> +EXPORT_SYMBOL_GPL(mux_control_get_optional);
> +
> +/**
> * mux_control_put() - Put away the mux-control for good.
> * @mux: The mux-control to put away.
> *
> @@ -492,7 +528,8 @@ EXPORT_SYMBOL_GPL(mux_control_get);
> */
> void mux_control_put(struct mux_control *mux)
> {
> - put_device(&mux->chip->dev);
> + if (mux)
> + put_device(&mux->chip->dev);
Don't put it if you don't have it.
> }
> EXPORT_SYMBOL_GPL(mux_control_put);
>
> @@ -503,16 +540,8 @@ static void devm_mux_control_release(struct device *dev, void *res)
> mux_control_put(mux);
> }
>
> -/**
> - * devm_mux_control_get() - Get the mux-control for a device, with resource
> - * management.
> - * @dev: The device that needs a mux-control.
> - * @mux_name: The name identifying the mux-control.
> - *
> - * Return: Pointer to the mux-control, or an ERR_PTR with a negative errno.
> - */
> -struct mux_control *devm_mux_control_get(struct device *dev,
> - const char *mux_name)
> +static struct mux_control *
> +__devm_mux_control_get(struct device *dev, const char *mux_name, bool optional)
> {
> struct mux_control **ptr, *mux;
>
> @@ -520,7 +549,7 @@ struct mux_control *devm_mux_control_get(struct device *dev,
> if (!ptr)
> return ERR_PTR(-ENOMEM);
>
> - mux = mux_control_get(dev, mux_name);
> + mux = __mux_control_get(dev, mux_name, optional);
> if (IS_ERR(mux)) {
> devres_free(ptr);
> return mux;
> @@ -531,8 +560,37 @@ struct mux_control *devm_mux_control_get(struct device *dev,
>
> return mux;
> }
> +
> +/**
> + * devm_mux_control_get() - Get the mux-control for a device, with resource
> + * management.
> + * @dev: The device that needs a mux-control.
> + * @mux_name: The name identifying the mux-control.
> + *
> + * Return: Pointer to the mux-control, or an ERR_PTR with a negative errno.
> + */
> +struct mux_control *
> +devm_mux_control_get(struct device *dev, const char *mux_name)
> +{
> + return __devm_mux_control_get(dev, mux_name, false);
> +}
> EXPORT_SYMBOL_GPL(devm_mux_control_get);
>
> +/**
> + * devm_mux_control_get_optional() - Get the optional mux-control for a device,
> + * with resource management.
> + * @dev: The device that needs a mux-control.
> + * @mux_name: The name identifying the mux-control.
> + *
> + * Return: Pointer to the mux-control, or an ERR_PTR with a negative errno.
You don't mention that NULL may be returned, or when.
Cheers,
Peter
> + */
> +struct mux_control *
> +devm_mux_control_get_optional(struct device *dev, const char *mux_name)
> +{
> + return __devm_mux_control_get(dev, mux_name, true);
> +}
> +EXPORT_SYMBOL_GPL(devm_mux_control_get_optional);
> +
> /*
> * Using subsys_initcall instead of module_init here to try to ensure - for
> * the non-modular case - that the subsystem is initialized when mux consumers
> diff --git a/include/linux/mux/consumer.h b/include/linux/mux/consumer.h
> index 5577e1b773c4..5e2aa046f032 100644
> --- a/include/linux/mux/consumer.h
> +++ b/include/linux/mux/consumer.h
> @@ -24,9 +24,13 @@ int __must_check mux_control_try_select(struct mux_control *mux,
> int mux_control_deselect(struct mux_control *mux);
>
> struct mux_control *mux_control_get(struct device *dev, const char *mux_name);
> +struct mux_control *mux_control_get_optional(struct device *dev,
> + const char *mux_name);
> void mux_control_put(struct mux_control *mux);
>
> struct mux_control *devm_mux_control_get(struct device *dev,
> const char *mux_name);
> +struct mux_control *devm_mux_control_get_optional(struct device *dev,
> + const char *mux_name);
>
> #endif /* _LINUX_MUX_CONSUMER_H */
>
On Fri, Jul 14, 2017 at 02:40:04PM -0700, Stephen Boyd wrote:
>
> @@ -175,6 +176,10 @@ static int host_start(struct ci_hdrc *ci)
> if (ci_otg_is_fsm_mode(ci)) {
> otg->host = &hcd->self;
> hcd->self.otg_port = 1;
> + } else {
> + ret = mux_control_select(ci->platdata->usb_switch, 1);
It is better to use MACRO for 1 and 0.
> + if (ret)
> + goto disable_reg;
> }
> }
>
> @@ -195,6 +200,8 @@ static void host_stop(struct ci_hdrc *ci)
> struct usb_hcd *hcd = ci->hcd;
>
> if (hcd) {
> + if (!ci_otg_is_fsm_mode(ci))
> + mux_control_deselect(ci->platdata->usb_switch);
> if (ci->platdata->notify_event)
> ci->platdata->notify_event(ci,
> CI_HDRC_CONTROLLER_STOPPED_EVENT);
> diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> index d68b125796f9..deb18099e168 100644
> --- a/drivers/usb/chipidea/udc.c
> +++ b/drivers/usb/chipidea/udc.c
> @@ -22,6 +22,7 @@
> #include <linux/usb/gadget.h>
> #include <linux/usb/otg-fsm.h>
> #include <linux/usb/chipidea.h>
> +#include <linux/mux/consumer.h>
>
> #include "ci.h"
> #include "udc.h"
> @@ -1964,16 +1965,26 @@ void ci_hdrc_gadget_destroy(struct ci_hdrc *ci)
>
> static int udc_id_switch_for_device(struct ci_hdrc *ci)
> {
> + int ret = 0;
> +
> if (ci->is_otg)
> /* Clear and enable BSV irq */
> hw_write_otgsc(ci, OTGSC_BSVIS | OTGSC_BSVIE,
> OTGSC_BSVIS | OTGSC_BSVIE);
>
> - return 0;
> + if (!ci_otg_is_fsm_mode(ci))
> + ret = mux_control_select(ci->platdata->usb_switch, 0);
> +
> + if (ci->is_otg && ret)
> + hw_write_otgsc(ci, OTGSC_BSVIE | OTGSC_BSVIS, OTGSC_BSVIS);
Should use !ret?
Peter
> +
> + return ret;
> }
>
> static void udc_id_switch_for_host(struct ci_hdrc *ci)
> {
> + mux_control_deselect(ci->platdata->usb_switch);
> +
> /*
> * host doesn't care B_SESSION_VALID event
> * so clear and disbale BSV irq
> diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h
> index c5fdfcf99828..3b27e333de1d 100644
> --- a/include/linux/usb/chipidea.h
> +++ b/include/linux/usb/chipidea.h
> @@ -9,6 +9,7 @@
> #include <linux/usb/otg.h>
>
> struct ci_hdrc;
> +struct mux_control;
>
> /**
> * struct ci_hdrc_cable - structure for external connector cable state tracking
> @@ -74,6 +75,7 @@ struct ci_hdrc_platform_data {
> /* VBUS and ID signal state tracking, using extcon framework */
> struct ci_hdrc_cable vbus_extcon;
> struct ci_hdrc_cable id_extcon;
> + struct mux_control *usb_switch;
> u32 phy_clkgate_delay_us;
> };
>
> --
> 2.10.0.297.gf6727b0
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
Best Regards,
Peter Chen
Quoting Peter Chen (2017-07-17 21:41:11)
> On Fri, Jul 14, 2017 at 02:40:04PM -0700, Stephen Boyd wrote:
> >
> > @@ -175,6 +176,10 @@ static int host_start(struct ci_hdrc *ci)
> > if (ci_otg_is_fsm_mode(ci)) {
> > otg->host = &hcd->self;
> > hcd->self.otg_port = 1;
> > + } else {
> > + ret = mux_control_select(ci->platdata->usb_switch, 1);
>
> It is better to use MACRO for 1 and 0.
>
Ok.
> > + if (ret)
> > + goto disable_reg;
> > }
> > }
> >
> > @@ -195,6 +200,8 @@ static void host_stop(struct ci_hdrc *ci)
> > struct usb_hcd *hcd = ci->hcd;
> >
> > if (hcd) {
> > + if (!ci_otg_is_fsm_mode(ci))
> > + mux_control_deselect(ci->platdata->usb_switch);
> > if (ci->platdata->notify_event)
> > ci->platdata->notify_event(ci,
> > CI_HDRC_CONTROLLER_STOPPED_EVENT);
> > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> > index d68b125796f9..deb18099e168 100644
> > --- a/drivers/usb/chipidea/udc.c
> > +++ b/drivers/usb/chipidea/udc.c
> > @@ -22,6 +22,7 @@
> > #include <linux/usb/gadget.h>
> > #include <linux/usb/otg-fsm.h>
> > #include <linux/usb/chipidea.h>
> > +#include <linux/mux/consumer.h>
> >
> > #include "ci.h"
> > #include "udc.h"
> > @@ -1964,16 +1965,26 @@ void ci_hdrc_gadget_destroy(struct ci_hdrc *ci)
> >
> > static int udc_id_switch_for_device(struct ci_hdrc *ci)
> > {
> > + int ret = 0;
> > +
> > if (ci->is_otg)
> > /* Clear and enable BSV irq */
> > hw_write_otgsc(ci, OTGSC_BSVIS | OTGSC_BSVIE,
> > OTGSC_BSVIS | OTGSC_BSVIE);
> >
> > - return 0;
> > + if (!ci_otg_is_fsm_mode(ci))
> > + ret = mux_control_select(ci->platdata->usb_switch, 0);
> > +
> > + if (ci->is_otg && ret)
> > + hw_write_otgsc(ci, OTGSC_BSVIE | OTGSC_BSVIS, OTGSC_BSVIS);
>
> Should use !ret?
>
No? This is intended to unwind the clearing and enabling of the BSV irq
on failure (ret is non-zero) and so we clear and disable the BSV irq.
On Tue, Jul 18, 2017 at 06:47:02PM -0700, Stephen Boyd wrote:
> Quoting Peter Chen (2017-07-17 21:41:11)
> > On Fri, Jul 14, 2017 at 02:40:04PM -0700, Stephen Boyd wrote:
> > >
> > > @@ -175,6 +176,10 @@ static int host_start(struct ci_hdrc *ci)
> > > if (ci_otg_is_fsm_mode(ci)) {
> > > otg->host = &hcd->self;
> > > hcd->self.otg_port = 1;
> > > + } else {
> > > + ret = mux_control_select(ci->platdata->usb_switch, 1);
> >
> > It is better to use MACRO for 1 and 0.
> >
>
> Ok.
>
> > > + if (ret)
> > > + goto disable_reg;
> > > }
> > > }
> > >
> > > @@ -195,6 +200,8 @@ static void host_stop(struct ci_hdrc *ci)
> > > struct usb_hcd *hcd = ci->hcd;
> > >
> > > if (hcd) {
> > > + if (!ci_otg_is_fsm_mode(ci))
> > > + mux_control_deselect(ci->platdata->usb_switch);
> > > if (ci->platdata->notify_event)
> > > ci->platdata->notify_event(ci,
> > > CI_HDRC_CONTROLLER_STOPPED_EVENT);
> > > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> > > index d68b125796f9..deb18099e168 100644
> > > --- a/drivers/usb/chipidea/udc.c
> > > +++ b/drivers/usb/chipidea/udc.c
> > > @@ -22,6 +22,7 @@
> > > #include <linux/usb/gadget.h>
> > > #include <linux/usb/otg-fsm.h>
> > > #include <linux/usb/chipidea.h>
> > > +#include <linux/mux/consumer.h>
> > >
> > > #include "ci.h"
> > > #include "udc.h"
> > > @@ -1964,16 +1965,26 @@ void ci_hdrc_gadget_destroy(struct ci_hdrc *ci)
> > >
> > > static int udc_id_switch_for_device(struct ci_hdrc *ci)
> > > {
> > > + int ret = 0;
> > > +
> > > if (ci->is_otg)
> > > /* Clear and enable BSV irq */
> > > hw_write_otgsc(ci, OTGSC_BSVIS | OTGSC_BSVIE,
> > > OTGSC_BSVIS | OTGSC_BSVIE);
> > >
> > > - return 0;
> > > + if (!ci_otg_is_fsm_mode(ci))
> > > + ret = mux_control_select(ci->platdata->usb_switch, 0);
> > > +
> > > + if (ci->is_otg && ret)
> > > + hw_write_otgsc(ci, OTGSC_BSVIE | OTGSC_BSVIS, OTGSC_BSVIS);
> >
> > Should use !ret?
> >
>
> No? This is intended to unwind the clearing and enabling of the BSV irq
> on failure (ret is non-zero) and so we clear and disable the BSV irq.
I see now, I did not notice we have already enabled BSV above.
--
Best Regards,
Peter Chen
Quoting Peter Rosin (2017-07-17 01:20:14)
> On 2017-07-14 23:40, Stephen Boyd wrote:
> > diff --git a/drivers/mux/mux-core.c b/drivers/mux/mux-core.c
> > index 90b8995f07cb..a0e5bf16f02f 100644
> > --- a/drivers/mux/mux-core.c
> > +++ b/drivers/mux/mux-core.c
> > @@ -289,6 +289,9 @@ EXPORT_SYMBOL_GPL(devm_mux_chip_register);
> > */
> > unsigned int mux_control_states(struct mux_control *mux)
> > {
> > + if (!mux)
> > + return 0;
> > +
>
> I don't think this is appropriate. For this function, it might be ok,
> but...
>
> > return mux->states;
> > }
> > EXPORT_SYMBOL_GPL(mux_control_states);
> > @@ -338,6 +341,9 @@ int mux_control_select(struct mux_control *mux, unsigned int state)
> > {
> > int ret;
> >
> > + if (!mux)
> > + return 0;
> > +
>
> ...here and for other cases below it's very odd to return "ok", when
> -EINVAL or something seems much more appropriate. And if -EINVAL is
> returned here, the benefit of returning fake values for anything
> pretty much falls apart.
>
> I simply don't like it, and prefer if the consumer code is arranged
> to not call the mux functions when the optional get() does not find
> the mux.
Do you want the callers of the mux APIs to know that an optional mux
isn't there, and then have checks at all callsites on optional muxes to
make sure consumers don't call the mux functions? Won't that duplicate
lots of checks in drivers for something the core could treat as a don't
care case? Sorry, I don't understand why the consumer cares that it was
there or not when it is optional.
>
> > ret = down_killable(&mux->lock);
> > if (ret < 0)
> > return ret;
> > @@ -370,6 +376,9 @@ int mux_control_try_select(struct mux_control *mux, unsigned int state)
> > {
> > int ret;
> >
> > + if (!mux)
> > + return 0;
> > +
> > if (down_trylock(&mux->lock))
> > return -EBUSY;
> >
> > @@ -398,6 +407,9 @@ int mux_control_deselect(struct mux_control *mux)
> > {
> > int ret = 0;
> >
> > + if (!mux)
> > + return 0;
> > +
> > if (mux->idle_state != MUX_IDLE_AS_IS &&
> > mux->idle_state != mux->cached_state)
> > ret = mux_control_set(mux, mux->idle_state);
> > @@ -422,14 +434,8 @@ static struct mux_chip *of_find_mux_chip_by_node(struct device_node *np)
> > return dev ? to_mux_chip(dev) : NULL;
> > }
> >
> > -/**
> > - * mux_control_get() - Get the mux-control for a device.
> > - * @dev: The device that needs a mux-control.
> > - * @mux_name: The name identifying the mux-control.
> > - *
> > - * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno.
> > - */
> > -struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
> > +struct mux_control *
> > +__mux_control_get(struct device *dev, const char *mux_name, bool optional)
> > {
> > struct device_node *np = dev->of_node;
> > struct of_phandle_args args;
> > @@ -441,6 +447,8 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
> > if (mux_name) {
> > index = of_property_match_string(np, "mux-control-names",
> > mux_name);
> > + if (index == -EINVAL && optional)
> > + return NULL;
>
> What about -ENODATA?
At this point in the code we're finding the index of a mux-control-names
property so I was trying to handle the case where there isn't a
mux-control-names property at all but we're looking for something
optional anyway. If there is a property, then we would see some other
error if something went wrong and then pass the error up. There is a
hole where there isn't a mux-control-names property and we're looking
for something that's optional, but there is a mux-control property. Do
we care though? The DT seems broken then.
> And if an optional mux is found here, but lookup
> fails later in e.g. the of_parse_phandle_with_args call, then I think
> an error should be returned. Because that seems like an indication that
> DT specifies that there *should* be a mux, but then there isn't one.
of_parse_phandle_with_args() would return ENOENT when there isn't a
mux-control property in DT. So I've trapped that case and returned an
"optional mux" pointer of NULL. I think we handle the case you mention,
where some index is found but it returns an error, because that would
return some error besides -ENOENT.
Sorry, I'm not really following what you're suggesting. Maybe it got
mixed up with the NULL vs. non-NULL return value from mux_control_get().
On 2017-07-19 04:08, Stephen Boyd wrote:
> Quoting Peter Rosin (2017-07-17 01:20:14)
>> On 2017-07-14 23:40, Stephen Boyd wrote:
>>> diff --git a/drivers/mux/mux-core.c b/drivers/mux/mux-core.c
>>> index 90b8995f07cb..a0e5bf16f02f 100644
>>> --- a/drivers/mux/mux-core.c
>>> +++ b/drivers/mux/mux-core.c
>>> @@ -289,6 +289,9 @@ EXPORT_SYMBOL_GPL(devm_mux_chip_register);
>>> */
>>> unsigned int mux_control_states(struct mux_control *mux)
>>> {
>>> + if (!mux)
>>> + return 0;
>>> +
>>
>> I don't think this is appropriate. For this function, it might be ok,
>> but...
>>
>>> return mux->states;
>>> }
>>> EXPORT_SYMBOL_GPL(mux_control_states);
>>> @@ -338,6 +341,9 @@ int mux_control_select(struct mux_control *mux, unsigned int state)
>>> {
>>> int ret;
>>>
>>> + if (!mux)
>>> + return 0;
>>> +
>>
>> ...here and for other cases below it's very odd to return "ok", when
>> -EINVAL or something seems much more appropriate. And if -EINVAL is
>> returned here, the benefit of returning fake values for anything
>> pretty much falls apart.
>>
>> I simply don't like it, and prefer if the consumer code is arranged
>> to not call the mux functions when the optional get() does not find
>> the mux.
>
> Do you want the callers of the mux APIs to know that an optional mux
> isn't there, and then have checks at all callsites on optional muxes to
> make sure consumers don't call the mux functions? Won't that duplicate
> lots of checks in drivers for something the core could treat as a don't
> care case? Sorry, I don't understand why the consumer cares that it was
> there or not when it is optional.
Ok, I had a look around to figure out how others handle this, and e.g.
gpio has (ugly) macros (VALIDATE_DESC) to handle this. I guess you are
right and I'm wrong. So, please keep all the if (!mux) checks.
Thanks for insisting!
>>
>>> ret = down_killable(&mux->lock);
>>> if (ret < 0)
>>> return ret;
>>> @@ -370,6 +376,9 @@ int mux_control_try_select(struct mux_control *mux, unsigned int state)
>>> {
>>> int ret;
>>>
>>> + if (!mux)
>>> + return 0;
>>> +
>>> if (down_trylock(&mux->lock))
>>> return -EBUSY;
>>>
>>> @@ -398,6 +407,9 @@ int mux_control_deselect(struct mux_control *mux)
>>> {
>>> int ret = 0;
>>>
>>> + if (!mux)
>>> + return 0;
>>> +
>>> if (mux->idle_state != MUX_IDLE_AS_IS &&
>>> mux->idle_state != mux->cached_state)
>>> ret = mux_control_set(mux, mux->idle_state);
>>> @@ -422,14 +434,8 @@ static struct mux_chip *of_find_mux_chip_by_node(struct device_node *np)
>>> return dev ? to_mux_chip(dev) : NULL;
>>> }
>>>
>>> -/**
>>> - * mux_control_get() - Get the mux-control for a device.
>>> - * @dev: The device that needs a mux-control.
>>> - * @mux_name: The name identifying the mux-control.
>>> - *
>>> - * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno.
>>> - */
>>> -struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
>>> +struct mux_control *
>>> +__mux_control_get(struct device *dev, const char *mux_name, bool optional)
>>> {
>>> struct device_node *np = dev->of_node;
>>> struct of_phandle_args args;
>>> @@ -441,6 +447,8 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
>>> if (mux_name) {
>>> index = of_property_match_string(np, "mux-control-names",
>>> mux_name);
>>> + if (index == -EINVAL && optional)
>>> + return NULL;
>>
>> What about -ENODATA?
>
> At this point in the code we're finding the index of a mux-control-names
> property so I was trying to handle the case where there isn't a
> mux-control-names property at all
Yes, you indeed need to check for -EINVAL to catch that. No argument
about that.
> but we're looking for something
> optional anyway. If there is a property, then we would see some other
> error if something went wrong and then pass the error up. There is a
> hole where there isn't a mux-control-names property and we're looking
> for something that's optional, but there is a mux-control property. Do
> we care though? The DT seems broken then.
I was thinking about the case where mux-control-names names *other* muxes
but not the one asked for in this call. That's not broken and should be
handled. The way I read it, you get -ENODATA in that case?
>> And if an optional mux is found here, but lookup
>> fails later in e.g. the of_parse_phandle_with_args call, then I think
>> an error should be returned. Because that seems like an indication that
>> DT specifies that there *should* be a mux, but then there isn't one.
>
> of_parse_phandle_with_args() would return ENOENT when there isn't a
> mux-control property in DT. So I've trapped that case and returned an
> "optional mux" pointer of NULL. I think we handle the case you mention,
> where some index is found but it returns an error, because that would
> return some error besides -ENOENT.
>
> Sorry, I'm not really following what you're suggesting. Maybe it got
> mixed up with the NULL vs. non-NULL return value from mux_control_get().
What I mean is that if you have passed a mux_name and the index of that
name was indeed found in the of_property_match_string call, then any
failure from of_parse_phandle_with_args indicates a bad DT and should
IMO result in an error. I.e., when evaluating the result from
of_parse_phandle_with_args, you should account for the optional param
if and only if mux_name is NULL.
You can do that by e.g. setting optional to false after looking up the
mux_name index (because at that point the mux is no longer considered
optional). E.g. as the last statement in the if (!mux_name) block.
Cheers,
peda
Quoting Peter Rosin (2017-07-19 00:15:38)
> On 2017-07-19 04:08, Stephen Boyd wrote:
> > Quoting Peter Rosin (2017-07-17 01:20:14)
> >> On 2017-07-14 23:40, Stephen Boyd wrote:
> >>> @@ -441,6 +447,8 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
> >>> if (mux_name) {
> >>> index = of_property_match_string(np, "mux-control-names",
> >>> mux_name);
> >>> + if (index == -EINVAL && optional)
> >>> + return NULL;
> >>
> >> What about -ENODATA?
> >
> > At this point in the code we're finding the index of a mux-control-names
> > property so I was trying to handle the case where there isn't a
> > mux-control-names property at all
>
> Yes, you indeed need to check for -EINVAL to catch that. No argument
> about that.
Ok.
>
> > but we're looking for something
> > optional anyway. If there is a property, then we would see some other
> > error if something went wrong and then pass the error up. There is a
> > hole where there isn't a mux-control-names property and we're looking
> > for something that's optional, but there is a mux-control property. Do
> > we care though? The DT seems broken then.
>
> I was thinking about the case where mux-control-names names *other* muxes
> but not the one asked for in this call. That's not broken and should be
> handled. The way I read it, you get -ENODATA in that case?
Yes that would return -ENODATA. Similarly, it would be returned if we
had a boolean mux-control-names property (which is completely broken).
>
> >> And if an optional mux is found here, but lookup
> >> fails later in e.g. the of_parse_phandle_with_args call, then I think
> >> an error should be returned. Because that seems like an indication that
> >> DT specifies that there *should* be a mux, but then there isn't one.
> >
> > of_parse_phandle_with_args() would return ENOENT when there isn't a
> > mux-control property in DT. So I've trapped that case and returned an
> > "optional mux" pointer of NULL. I think we handle the case you mention,
> > where some index is found but it returns an error, because that would
> > return some error besides -ENOENT.
> >
> > Sorry, I'm not really following what you're suggesting. Maybe it got
> > mixed up with the NULL vs. non-NULL return value from mux_control_get().
>
> What I mean is that if you have passed a mux_name and the index of that
> name was indeed found in the of_property_match_string call, then any
> failure from of_parse_phandle_with_args indicates a bad DT and should
> IMO result in an error. I.e., when evaluating the result from
> of_parse_phandle_with_args, you should account for the optional param
> if and only if mux_name is NULL.
>
> You can do that by e.g. setting optional to false after looking up the
> mux_name index (because at that point the mux is no longer considered
> optional). E.g. as the last statement in the if (!mux_name) block.
>
Ok got it. I'll rework the logic.
On 2017-07-14 23:40, Stephen Boyd wrote:
> On the db410c 96boards platform we have a TC7USB40MU on the board
> to mux the D+/D- lines coming from the controller between a micro
> usb "device" port and a USB hub for "host" roles[1]. During a
> role switch, we need to toggle this mux to forward the D+/D-
> lines to either the port or the hub. Add the necessary code to do
> the role switch in chipidea core via the generic mux framework.
> Board configurations like on db410c are expected to change roles
> via the sysfs API described in
> Documentation/ABI/testing/sysfs-platform-chipidea-usb2.
>
> [1] https://github.com/96boards/documentation/raw/master/ConsumerEdition/DragonBoard-410c/HardwareDocs/Schematics_DragonBoard.pdf
>
> Cc: Peter Rosin <[email protected]>
> Cc: Peter Chen <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
> Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt | 6 ++++++
> drivers/usb/chipidea/Kconfig | 1 +
> drivers/usb/chipidea/core.c | 5 +++++
> drivers/usb/chipidea/host.c | 7 +++++++
> drivers/usb/chipidea/udc.c | 13 ++++++++++++-
> include/linux/usb/chipidea.h | 2 ++
> 6 files changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> index 0e03344e2e8b..2e9318151df7 100644
> --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> @@ -76,6 +76,10 @@ Optional properties:
> needs to make sure it does not send more than 90%
> maximum_periodic_data_per_frame. The use case is multiple transactions, but
> less frame rate.
> +- mux-controls: The mux control for toggling host/device output of this
> + controller. It's expected that a mux state of 0 indicates device mode and a
> + mux state of 1 indicates host mode.
> +- mux-control-names: Shall be "usb_switch" if mux-controls is specified.
>
> i.mx specific properties
> - fsl,usbmisc: phandler of non-core register device, with one
> @@ -102,4 +106,6 @@ Example:
> rx-burst-size-dword = <0x10>;
> extcon = <0>, <&usb_id>;
> phy-clkgate-delay-us = <400>;
> + mux-controls = <&usb_switch>;
> + mux-control-names = "usb_switch";
> };
> diff --git a/drivers/usb/chipidea/Kconfig b/drivers/usb/chipidea/Kconfig
> index 51f4157bbecf..3798e0e69d57 100644
> --- a/drivers/usb/chipidea/Kconfig
> +++ b/drivers/usb/chipidea/Kconfig
> @@ -3,6 +3,7 @@ config USB_CHIPIDEA
> depends on ((USB_EHCI_HCD && USB_GADGET) || (USB_EHCI_HCD && !USB_GADGET) || (!USB_EHCI_HCD && USB_GADGET)) && HAS_DMA
> select EXTCON
> select RESET_CONTROLLER
> + select MULTIPLEXER
> help
> Say Y here if your system has a dual role high speed USB
> controller based on ChipIdea silicon IP. It supports:
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index b17ed3a9a304..d088c262ebb8 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -64,6 +64,7 @@
> #include <linux/of.h>
> #include <linux/regulator/consumer.h>
> #include <linux/usb/ehci_def.h>
> +#include <linux/mux/consumer.h>
>
> #include "ci.h"
> #include "udc.h"
> @@ -690,6 +691,10 @@ static int ci_get_platdata(struct device *dev,
> if (of_find_property(dev->of_node, "non-zero-ttctrl-ttha", NULL))
> platdata->flags |= CI_HDRC_SET_NON_ZERO_TTHA;
>
> + platdata->usb_switch = devm_mux_control_get_optional(dev, "usb_switch");
> + if (IS_ERR(platdata->usb_switch))
> + return PTR_ERR(platdata->usb_switch);
> +
> ext_id = ERR_PTR(-ENODEV);
> ext_vbus = ERR_PTR(-ENODEV);
> if (of_property_read_bool(dev->of_node, "extcon")) {
> diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
> index 18cb8e46262d..9ef3ecf27ad3 100644
> --- a/drivers/usb/chipidea/host.c
> +++ b/drivers/usb/chipidea/host.c
> @@ -25,6 +25,7 @@
> #include <linux/usb/hcd.h>
> #include <linux/usb/chipidea.h>
> #include <linux/regulator/consumer.h>
> +#include <linux/mux/consumer.h>
>
> #include "../host/ehci.h"
>
> @@ -175,6 +176,10 @@ static int host_start(struct ci_hdrc *ci)
> if (ci_otg_is_fsm_mode(ci)) {
> otg->host = &hcd->self;
> hcd->self.otg_port = 1;
> + } else {
> + ret = mux_control_select(ci->platdata->usb_switch, 1);
> + if (ret)
> + goto disable_reg;
> }
> }
>
> @@ -195,6 +200,8 @@ static void host_stop(struct ci_hdrc *ci)
> struct usb_hcd *hcd = ci->hcd;
>
> if (hcd) {
> + if (!ci_otg_is_fsm_mode(ci))
> + mux_control_deselect(ci->platdata->usb_switch);
> if (ci->platdata->notify_event)
> ci->platdata->notify_event(ci,
> CI_HDRC_CONTROLLER_STOPPED_EVENT);
> diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> index d68b125796f9..deb18099e168 100644
> --- a/drivers/usb/chipidea/udc.c
> +++ b/drivers/usb/chipidea/udc.c
> @@ -22,6 +22,7 @@
> #include <linux/usb/gadget.h>
> #include <linux/usb/otg-fsm.h>
> #include <linux/usb/chipidea.h>
> +#include <linux/mux/consumer.h>
>
> #include "ci.h"
> #include "udc.h"
> @@ -1964,16 +1965,26 @@ void ci_hdrc_gadget_destroy(struct ci_hdrc *ci)
>
> static int udc_id_switch_for_device(struct ci_hdrc *ci)
> {
> + int ret = 0;
> +
> if (ci->is_otg)
> /* Clear and enable BSV irq */
> hw_write_otgsc(ci, OTGSC_BSVIS | OTGSC_BSVIE,
> OTGSC_BSVIS | OTGSC_BSVIE);
>
> - return 0;
> + if (!ci_otg_is_fsm_mode(ci))
> + ret = mux_control_select(ci->platdata->usb_switch, 0);
> +
> + if (ci->is_otg && ret)
> + hw_write_otgsc(ci, OTGSC_BSVIE | OTGSC_BSVIS, OTGSC_BSVIS);
> +
> + return ret;
> }
>
> static void udc_id_switch_for_host(struct ci_hdrc *ci)
> {
> + mux_control_deselect(ci->platdata->usb_switch);
> +
This looks broken. You conditionally lock the mux and you unconditionally
unlock it. Quoting the mux_control_deselect doc:
* It is required that a single call is made to mux_control_deselect() for
* each and every successful call made to either of mux_control_select() or
* mux_control_try_select().
Think of the mux as a semaphore with a max count of one. If you lock it,
you have to unlock it when you're done. If you didn't lock it, you have
zero business unlocking it. If you try to lock it but fail, you also have
no business unlocking it. Just like a semaphore.
Another point: I do not know if udc_id_switch_for_host is *only* called
when there has been a prior call to udc_id_switch_for_device that
succeeded or how this works exactly. But this all looks fragile. Using
mux_control_select and mux_control_deselect *must* be done in pairs.
If you want a mux to be locked for "a while", such as in this case, you
have to make sure you stay within the rules. There is no room for half
measures, or you will likely cause a deadlock when something unexpected
happens.
Cheers,
Peter
> /*
> * host doesn't care B_SESSION_VALID event
> * so clear and disbale BSV irq
> diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h
> index c5fdfcf99828..3b27e333de1d 100644
> --- a/include/linux/usb/chipidea.h
> +++ b/include/linux/usb/chipidea.h
> @@ -9,6 +9,7 @@
> #include <linux/usb/otg.h>
>
> struct ci_hdrc;
> +struct mux_control;
>
> /**
> * struct ci_hdrc_cable - structure for external connector cable state tracking
> @@ -74,6 +75,7 @@ struct ci_hdrc_platform_data {
> /* VBUS and ID signal state tracking, using extcon framework */
> struct ci_hdrc_cable vbus_extcon;
> struct ci_hdrc_cable id_extcon;
> + struct mux_control *usb_switch;
> u32 phy_clkgate_delay_us;
> };
>
>
Quoting Peter Rosin (2017-07-31 03:33:22)
> On 2017-07-14 23:40, Stephen Boyd wrote:
> > @@ -1964,16 +1965,26 @@ void ci_hdrc_gadget_destroy(struct ci_hdrc *ci)
> >
> > static int udc_id_switch_for_device(struct ci_hdrc *ci)
> > {
> > + int ret = 0;
> > +
> > if (ci->is_otg)
> > /* Clear and enable BSV irq */
> > hw_write_otgsc(ci, OTGSC_BSVIS | OTGSC_BSVIE,
> > OTGSC_BSVIS | OTGSC_BSVIE);
> >
> > - return 0;
> > + if (!ci_otg_is_fsm_mode(ci))
> > + ret = mux_control_select(ci->platdata->usb_switch, 0);
> > +
> > + if (ci->is_otg && ret)
> > + hw_write_otgsc(ci, OTGSC_BSVIE | OTGSC_BSVIS, OTGSC_BSVIS);
> > +
> > + return ret;
> > }
> >
> > static void udc_id_switch_for_host(struct ci_hdrc *ci)
> > {
> > + mux_control_deselect(ci->platdata->usb_switch);
> > +
>
> This looks broken. You conditionally lock the mux and you unconditionally
> unlock it. Quoting the mux_control_deselect doc:
>
> * It is required that a single call is made to mux_control_deselect() for
> * each and every successful call made to either of mux_control_select() or
> * mux_control_try_select().
>
> Think of the mux as a semaphore with a max count of one. If you lock it,
> you have to unlock it when you're done. If you didn't lock it, you have
> zero business unlocking it. If you try to lock it but fail, you also have
> no business unlocking it. Just like a semaphore.
Good catch. I've added a if (!ci_otg_is_fsm_mode()) check here.
>
> Another point: I do not know if udc_id_switch_for_host is *only* called
> when there has been a prior call to udc_id_switch_for_device that
> succeeded or how this works exactly. But this all looks fragile. Using
> mux_control_select and mux_control_deselect *must* be done in pairs.
> If you want a mux to be locked for "a while", such as in this case, you
> have to make sure you stay within the rules. There is no room for half
> measures, or you will likely cause a deadlock when something unexpected
> happens.
>
Can you elaborate? Is it bad that we keep it "locked" while we're in
host or device mode? It looked like we paired the start/stop ops with
each other so that the mux is properly managed across these ops. My
testing hasn't shown a problem, but maybe there's some corner case
you're thinking of? I'll double check the code.
On 2017-08-08 03:51, Stephen Boyd wrote:
> Quoting Peter Rosin (2017-07-31 03:33:22)
>> On 2017-07-14 23:40, Stephen Boyd wrote:
>>> @@ -1964,16 +1965,26 @@ void ci_hdrc_gadget_destroy(struct ci_hdrc *ci)
>>>
>>> static int udc_id_switch_for_device(struct ci_hdrc *ci)
>>> {
>>> + int ret = 0;
>>> +
>>> if (ci->is_otg)
>>> /* Clear and enable BSV irq */
>>> hw_write_otgsc(ci, OTGSC_BSVIS | OTGSC_BSVIE,
>>> OTGSC_BSVIS | OTGSC_BSVIE);
>>>
>>> - return 0;
>>> + if (!ci_otg_is_fsm_mode(ci))
>>> + ret = mux_control_select(ci->platdata->usb_switch, 0);
>>> +
>>> + if (ci->is_otg && ret)
>>> + hw_write_otgsc(ci, OTGSC_BSVIE | OTGSC_BSVIS, OTGSC_BSVIS);
>>> +
>>> + return ret;
>>> }
>>>
>>> static void udc_id_switch_for_host(struct ci_hdrc *ci)
>>> {
>>> + mux_control_deselect(ci->platdata->usb_switch);
>>> +
>>
>> This looks broken. You conditionally lock the mux and you unconditionally
>> unlock it. Quoting the mux_control_deselect doc:
>>
>> * It is required that a single call is made to mux_control_deselect() for
>> * each and every successful call made to either of mux_control_select() or
>> * mux_control_try_select().
>>
>> Think of the mux as a semaphore with a max count of one. If you lock it,
>> you have to unlock it when you're done. If you didn't lock it, you have
>> zero business unlocking it. If you try to lock it but fail, you also have
>> no business unlocking it. Just like a semaphore.
>
> Good catch. I've added a if (!ci_otg_is_fsm_mode()) check here.
>
>>
>> Another point: I do not know if udc_id_switch_for_host is *only* called
>> when there has been a prior call to udc_id_switch_for_device that
>> succeeded or how this works exactly. But this all looks fragile. Using
>> mux_control_select and mux_control_deselect *must* be done in pairs.
>> If you want a mux to be locked for "a while", such as in this case, you
>> have to make sure you stay within the rules. There is no room for half
>> measures, or you will likely cause a deadlock when something unexpected
>> happens.
>>
>
> Can you elaborate? Is it bad that we keep it "locked" while we're in
> host or device mode?
No no, that's good. You do not want some other consumer of the same mux
controller to clobber your state (in case it's shared), so you absolutely
want to have the mux locked when you want it to remain in a specific
position.
> It looked like we paired the start/stop ops with
> each other so that the mux is properly managed across these ops.
Yes, it *looks* ok...
> My
> testing hasn't shown a problem, but maybe there's some corner case
> you're thinking of? I'll double check the code.
...but since I do not know the usb code, I can't tell. What I worry about
is the usb core calling udc_id_switch_for_host or udc_id_switch_for_device
more than once without any call to the other in between. Maybe that is a
guarantee that the usb core makes? Or maybe it isn't? If e.g. there is a
third mode (or if one is added in the future), then the calls to
mux_control_select and mux_control_deselect would not be paired correctly.
Ok, sure, a third mode probably doesn't exist and will probably not be
added, but but but...
Also, what happens if udc_id_switch_for_device fails? Is it certain that
it will be called again before udc_id_switch_for_host is called, or is
the failure simply logged? If the latter, you might have a call to
mux_control_deselect without a preceding (and successful) call to
mux_control_select. That's fatal.
I have similar worries for host_start/host_stop, but for that case
host_stop is not allowed to fail, and it seems like a safe bet that
host_stop will only be called if host_start succeeds. So, I'm not as
worried there.
In other words, the question is if the usb core is designed to allow
this kind of "raw" resource administration in udc_id_switch_for_host and
udc_id_switch_for_device, or if you need to keep a local record of the
state so that you do not do double resource acquisition or attempt to
free resources you don't have?
I think I would feel better if the muxing for the device mode could
be done in a start/stop pair of function just like the host mode is
doing. Again, I don't know the usb code and don't know if such hooks
exist or not?
Cheers,
Peter
Quoting Peter Rosin (2017-08-08 05:46:30)
> On 2017-08-08 03:51, Stephen Boyd wrote:
>
> > It looked like we paired the start/stop ops with
> > each other so that the mux is properly managed across these ops.
>
> Yes, it *looks* ok...
>
> > My
> > testing hasn't shown a problem, but maybe there's some corner case
> > you're thinking of? I'll double check the code.
>
> ...but since I do not know the usb code, I can't tell. What I worry about
> is the usb core calling udc_id_switch_for_host or udc_id_switch_for_device
> more than once without any call to the other in between. Maybe that is a
> guarantee that the usb core makes? Or maybe it isn't? If e.g. there is a
> third mode (or if one is added in the future), then the calls to
> mux_control_select and mux_control_deselect would not be paired correctly.
> Ok, sure, a third mode probably doesn't exist and will probably not be
> added, but but but...
>
> Also, what happens if udc_id_switch_for_device fails? Is it certain that
> it will be called again before udc_id_switch_for_host is called, or is
> the failure simply logged? If the latter, you might have a call to
> mux_control_deselect without a preceding (and successful) call to
> mux_control_select. That's fatal.
The only thing that could fail right now is the mux selection, so we
wouldn't get into some sort of situation where that's locked in and
unchangeable. We do rollback the role if it fails to switch, so we also
wouldn't go into a half-way state of being in one role but not actually
switching all the way over to it.
>
> I have similar worries for host_start/host_stop, but for that case
> host_stop is not allowed to fail, and it seems like a safe bet that
> host_stop will only be called if host_start succeeds. So, I'm not as
> worried there.
>
> In other words, the question is if the usb core is designed to allow
> this kind of "raw" resource administration in udc_id_switch_for_host and
> udc_id_switch_for_device, or if you need to keep a local record of the
> state so that you do not do double resource acquisition or attempt to
> free resources you don't have?
>
> I think I would feel better if the muxing for the device mode could
> be done in a start/stop pair of function just like the host mode is
> doing. Again, I don't know the usb code and don't know if such hooks
> exist or not?
>
The host_start/host_stop functions are assigned to the same struct
ci_role_driver ops that udc_idc_switch_for_{device,host} are for the
gadget role. Really, these things are called from the same place by the
chipidea driver so not much is different between the two files I modify
to make the mux calls. Furthermore, we don't want to do this if we have
HNP or "true" OTG support so I've put it behind the ci_otg_is_fsm_mode()
check to make sure we don't do any muxing stuff based on fsm state
changes. It doesn't really make any sense here anyway because this
device I have doesn't support OTG, just role switching.
On 2017-08-12 00:26, Stephen Boyd wrote:
> Quoting Peter Rosin (2017-08-08 05:46:30)
>> On 2017-08-08 03:51, Stephen Boyd wrote:
>>
>>> It looked like we paired the start/stop ops with
>>> each other so that the mux is properly managed across these ops.
>>
>> Yes, it *looks* ok...
>>
>>> My
>>> testing hasn't shown a problem, but maybe there's some corner case
>>> you're thinking of? I'll double check the code.
>>
>> ...but since I do not know the usb code, I can't tell. What I worry about
>> is the usb core calling udc_id_switch_for_host or udc_id_switch_for_device
>> more than once without any call to the other in between. Maybe that is a
>> guarantee that the usb core makes? Or maybe it isn't? If e.g. there is a
>> third mode (or if one is added in the future), then the calls to
>> mux_control_select and mux_control_deselect would not be paired correctly.
>> Ok, sure, a third mode probably doesn't exist and will probably not be
>> added, but but but...
>>
>> Also, what happens if udc_id_switch_for_device fails? Is it certain that
>> it will be called again before udc_id_switch_for_host is called, or is
>> the failure simply logged? If the latter, you might have a call to
>> mux_control_deselect without a preceding (and successful) call to
>> mux_control_select. That's fatal.
>
> The only thing that could fail right now is the mux selection, so we
> wouldn't get into some sort of situation where that's locked in and
> unchangeable. We do rollback the role if it fails to switch, so we also
> wouldn't go into a half-way state of being in one role but not actually
> switching all the way over to it.
What do you roll back to if the initial setting of the role fails?
Hopefully that causes a probe failure?
Cheers,
Peter
>> I have similar worries for host_start/host_stop, but for that case
>> host_stop is not allowed to fail, and it seems like a safe bet that
>> host_stop will only be called if host_start succeeds. So, I'm not as
>> worried there.
>>
>> In other words, the question is if the usb core is designed to allow
>> this kind of "raw" resource administration in udc_id_switch_for_host and
>> udc_id_switch_for_device, or if you need to keep a local record of the
>> state so that you do not do double resource acquisition or attempt to
>> free resources you don't have?
>>
>> I think I would feel better if the muxing for the device mode could
>> be done in a start/stop pair of function just like the host mode is
>> doing. Again, I don't know the usb code and don't know if such hooks
>> exist or not?
>>
>
> The host_start/host_stop functions are assigned to the same struct
> ci_role_driver ops that udc_idc_switch_for_{device,host} are for the
> gadget role. Really, these things are called from the same place by the
> chipidea driver so not much is different between the two files I modify
> to make the mux calls. Furthermore, we don't want to do this if we have
> HNP or "true" OTG support so I've put it behind the ci_otg_is_fsm_mode()
> check to make sure we don't do any muxing stuff based on fsm state
> changes. It doesn't really make any sense here anyway because this
> device I have doesn't support OTG, just role switching.
Hi Stephen,
On Fri, Jul 14, 2017 at 02:40:05PM -0700, Stephen Boyd wrote:
> We currently have three device nodes for the same USB hardware
> block, as evident by the reuse of the same reg address multiple
> times. Now that the chipidea driver fully supports OTG with the
> MSM wrapper we can collapse all these nodes into one USB device
> node, reflecting the true nature of the hardware.
>
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
> arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi | 38 ++++++++++---------
> arch/arm64/boot/dts/qcom/msm8916.dtsi | 62 +++++++++++++++----------------
> 2 files changed, 50 insertions(+), 50 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi b/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
> index f326f4fb4d72..494560a1a6ef 100644
> --- a/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
> +++ b/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
> @@ -213,24 +213,20 @@
> };
>
> usb@78d9000 {
> - extcon = <&usb_id>, <&usb_id>;
> + extcon = <&usb_id>;
I'm trying to play the series on db410c, and it doesn't seem to work as
expected. Here is basically what I did:
- Revert ed75d6a96905, and apply the series.
- Turn on the following options:
CONFIG_USB_CHIPIDEA_ULPI
CONFIG_USB_ULPI_BUS
CONFIG_PHY_QCOM_USB_HS
CONFIG_MUX_GPIO
The role switching doesn't happen when I connect/disconnect cable
to/from micro-usb port. But if I revert above extcon change (keep two
usb_id phandle for extcon), the role switching happens. However, host
driver still fails to enumerate the usb mouse attached to Type-A port.
[ 15.400074] ci_hdrc ci_hdrc.0: EHCI Host Controller
[ 15.400119] ci_hdrc ci_hdrc.0: new USB bus registered, assigned bus number 1
[ 15.419847] ci_hdrc ci_hdrc.0: USB 2.0 started, EHCI 1.00
[ 15.420700] hub 1-0:1.0: USB hub found
[ 15.425820] hub 1-0:1.0: 1 port detected
[ 15.759862] usb 1-1: new high-speed USB device number 2 using ci_hdrc
[ 31.943942] usb 1-1: device not accepting address 2, error -110
[ 32.063938] usb 1-1: new high-speed USB device number 3 using ci_hdrc
[ 48.071943] usb 1-1: device not accepting address 3, error -110
[ 48.191935] usb 1-1: new high-speed USB device number 4 using ci_hdrc
[ 58.823934] usb 1-1: device not accepting address 4, error -110
[ 58.943936] usb 1-1: new high-speed USB device number 5 using ci_hdrc
[ 69.575935] usb 1-1: device not accepting address 5, error -110
[ 69.576001] usb usb1-port1: unable to enumerate USB device
I must have missed something. Can you please advice? Thanks.
Shawn
Quoting Shawn Guo (2017-08-16 23:43:49)
> Hi Stephen,
>
> On Fri, Jul 14, 2017 at 02:40:05PM -0700, Stephen Boyd wrote:
> > We currently have three device nodes for the same USB hardware
> > block, as evident by the reuse of the same reg address multiple
> > times. Now that the chipidea driver fully supports OTG with the
> > MSM wrapper we can collapse all these nodes into one USB device
> > node, reflecting the true nature of the hardware.
> >
> > Signed-off-by: Stephen Boyd <[email protected]>
> > ---
> > arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi | 38 ++++++++++---------
> > arch/arm64/boot/dts/qcom/msm8916.dtsi | 62 +++++++++++++++----------------
> > 2 files changed, 50 insertions(+), 50 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi b/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
> > index f326f4fb4d72..494560a1a6ef 100644
> > --- a/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
> > @@ -213,24 +213,20 @@
> > };
> >
> > usb@78d9000 {
> > - extcon = <&usb_id>, <&usb_id>;
> > + extcon = <&usb_id>;
>
> I'm trying to play the series on db410c, and it doesn't seem to work as
> expected. Here is basically what I did:
>
> - Revert ed75d6a96905, and apply the series.
> - Turn on the following options:
> CONFIG_USB_CHIPIDEA_ULPI
> CONFIG_USB_ULPI_BUS
> CONFIG_PHY_QCOM_USB_HS
> CONFIG_MUX_GPIO
>
> The role switching doesn't happen when I connect/disconnect cable
> to/from micro-usb port.
Good. That's expected.
> But if I revert above extcon change (keep two
> usb_id phandle for extcon), the role switching happens. However, host
> driver still fails to enumerate the usb mouse attached to Type-A port.
>
> [ 15.400074] ci_hdrc ci_hdrc.0: EHCI Host Controller
> [ 15.400119] ci_hdrc ci_hdrc.0: new USB bus registered, assigned bus number 1
> [ 15.419847] ci_hdrc ci_hdrc.0: USB 2.0 started, EHCI 1.00
> [ 15.420700] hub 1-0:1.0: USB hub found
> [ 15.425820] hub 1-0:1.0: 1 port detected
> [ 15.759862] usb 1-1: new high-speed USB device number 2 using ci_hdrc
> [ 31.943942] usb 1-1: device not accepting address 2, error -110
> [ 32.063938] usb 1-1: new high-speed USB device number 3 using ci_hdrc
> [ 48.071943] usb 1-1: device not accepting address 3, error -110
> [ 48.191935] usb 1-1: new high-speed USB device number 4 using ci_hdrc
> [ 58.823934] usb 1-1: device not accepting address 4, error -110
> [ 58.943936] usb 1-1: new high-speed USB device number 5 using ci_hdrc
> [ 69.575935] usb 1-1: device not accepting address 5, error -110
> [ 69.576001] usb usb1-port1: unable to enumerate USB device
>
> I must have missed something. Can you please advice? Thanks.
Yes. The role switch happens now by userspace writing different values
to the chipidea node sysfs file. For db410c it's located at
/sys/bus/platform/devices/ci_hdrc.0/role and by echoing 'host' or
'gadget' into that file you can change the role. Unplugging the cable
won't do anything anymore, because the micro-usb port is only indicating
vbus presence, and not the ID pin. At least that is my reading of the
schematic.
Obviously, this changes behavior from previous designs where
disconnecting the cable changed the role, but I don't know if we want to
support this method via the kernel alone. Instead, it seems simpler to
have userspace decide to change the role whenever it wants with some
policy. Do you have any suggestion on how this can be integrated into
userspace so we write this file at boot? That way, we can switch to host
mode by default on db410c and then users can decide to make a gadget if
they want to lose the host ports while the gadget is active. We could
probably have an extcon event handler in userspace as well to change the
role when the micro-usb cable is disconnected. That way, old behavior
could be maintained but it would be a pure policy decision in userspace.