Re: [PATCH 1/2] extcon: add driver for Intel USB mux
Hi Heikki,
On Wed, Dec 02, 2015 at 12:27:10PM +0200, Heikki Krogerus wrote:
> Hi David,
>
>
>
> > > +void intel_usb_mux_unregister(struct intel_usb_mux *mux)
> > > +{
> >
> > There are still 2 pending comments for this unregister function:
> >
> > 1) How about a protection against unbalanced unregistering? In case an
> > user mistakenly unregisters twice or unregisters without a previous
> > registering.
>
> True. You already pointed that out in our off-list thread, but I
> forgot. Sorry about that.
>
> > 2) When unregistering, you need to clear the bit CFG0_SW_IDPIN_EN to
> > set mux to automatic mode again. Or maybe you could save CFG0's initial
> > value and set it again here. Anyway, when unregistering the mux driver
> > you need to make sure the mux goes back to its original configuration.
>
> This one is already been taken care of..
>
> > > + extcon_unregister_notifier(&mux->edev, EXTCON_USB_HOST, &mux->nb);
> > > + extcon_dev_unregister(&mux->edev);
> > > + writel(mux->cfg0_ctx, mux->regs + INTEL_MUX_CFG0);
>
>
Of course :)
Sorry I missed this part.
Br, David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] extcon: add driver for Intel USB mux
Hi David,
> > +void intel_usb_mux_unregister(struct intel_usb_mux *mux)
> > +{
>
> There are still 2 pending comments for this unregister function:
>
> 1) How about a protection against unbalanced unregistering? In case an
> user mistakenly unregisters twice or unregisters without a previous
> registering.
True. You already pointed that out in our off-list thread, but I
forgot. Sorry about that.
> 2) When unregistering, you need to clear the bit CFG0_SW_IDPIN_EN to
> set mux to automatic mode again. Or maybe you could save CFG0's initial
> value and set it again here. Anyway, when unregistering the mux driver
> you need to make sure the mux goes back to its original configuration.
This one is already been taken care of..
> > + extcon_unregister_notifier(&mux->edev, EXTCON_USB_HOST, &mux->nb);
> > + extcon_dev_unregister(&mux->edev);
> > + writel(mux->cfg0_ctx, mux->regs + INTEL_MUX_CFG0);
Thanks,
--
heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] extcon: add driver for Intel USB mux
Hi,
David Cohen writes:
> Hi Felipe,
>
> On Tue, Dec 01, 2015 at 02:34:34PM -0600, Felipe Balbi wrote:
>
> [snip]
>
>> > +EXPORT_SYMBOL_GPL(intel_usb_mux_register);
>> > +
>> > +void intel_usb_mux_unregister(struct intel_usb_mux *mux)
>> > +{
>> > + extcon_unregister_notifier(&mux->edev, EXTCON_USB_HOST, &mux->nb);
>> > + extcon_dev_unregister(&mux->edev);
>> > + writel(mux->cfg0_ctx, mux->regs + INTEL_MUX_CFG0);
>> > + iounmap(mux->regs);
>> > + kfree(mux);
>> > +}
>> > +EXPORT_SYMBOL_GPL(intel_usb_mux_unregister);
>>
>> so who's gonna call these two functions ? IMO, this looks like a recipe
>> for randbuild breakage.
>
> There are function stubs on header file when the functions aren't
> available.
> But also notice CONFIG_EXTCON_INTEL_USB is not user-selectable. It's
> automatically selected when a driver that requires it is selected too.
>
> With the 2 cases above, IMHO it should not bring issues with randbuild
> tests.
right now it won't break, that's correct :-) But things change over time
and this is, at least, fragile. I mean, why couldn't this mux be an
actual driver ?
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH 1/2] extcon: add driver for Intel USB mux
Hi Felipe,
On Tue, Dec 01, 2015 at 02:34:34PM -0600, Felipe Balbi wrote:
[snip]
> > +EXPORT_SYMBOL_GPL(intel_usb_mux_register);
> > +
> > +void intel_usb_mux_unregister(struct intel_usb_mux *mux)
> > +{
> > + extcon_unregister_notifier(&mux->edev, EXTCON_USB_HOST, &mux->nb);
> > + extcon_dev_unregister(&mux->edev);
> > + writel(mux->cfg0_ctx, mux->regs + INTEL_MUX_CFG0);
> > + iounmap(mux->regs);
> > + kfree(mux);
> > +}
> > +EXPORT_SYMBOL_GPL(intel_usb_mux_unregister);
>
> so who's gonna call these two functions ? IMO, this looks like a recipe
> for randbuild breakage.
There are function stubs on header file when the functions aren't
available.
But also notice CONFIG_EXTCON_INTEL_USB is not user-selectable. It's
automatically selected when a driver that requires it is selected too.
With the 2 cases above, IMHO it should not bring issues with randbuild
tests.
Br, David
>
> --
> balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] extcon: add driver for Intel USB mux
Hi,
Heikki Krogerus writes:
> Several Intel PCHs and SOCs have an internal mux that is
> used to share one USB port between USB Device Controller and
> xHCI. The mux is normally handled by System FW/BIOS, but not
> always. For those platforms where the FW does not take care
> of the mux, this driver is needed.
except that this is not exactly a driver. Note that it lacks a
module_init() of its own.
> diff --git a/drivers/extcon/extcon-intel-usb.c
> b/drivers/extcon/extcon-intel-usb.c
> new file mode 100644
> index 000..5534781
> --- /dev/null
> +++ b/drivers/extcon/extcon-intel-usb.c
> @@ -0,0 +1,112 @@
> +/**
> + * extcon-intel-usb.c - Driver for Intel USB mux
> + *
> + * Copyright (C) 2015 Intel Corporation
> + * Author: Heikki Krogerus
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include
> +#include
> +
> +#include
> +
> +#define INTEL_MUX_CFG0 0x00
> +#define INTEL_MUX_CFG1 0x04
> +
> +#define CFG0_SW_DRD_MODE_MASK0x3
> +#define CFG0_SW_DRD_DYN 0
> +#define CFG0_SW_DRD_STATIC_HOST 1
> +#define CFG0_SW_DRD_STATIC_DEV 2
> +#define CFG0_SW_SYNC_SS_AND_HS BIT(2)
> +#define CFG0_SW_SWITCH_ENBIT(16)
> +#define CFG0_SW_IDPINBIT(20)
> +#define CFG0_SW_IDPIN_EN BIT(21)
> +#define CFG0_SW_VBUS_VALID BIT(24)
> +
> +#define CFG1_MODEBIT(29)
> +
> +struct intel_usb_mux {
> + struct notifier_block nb;
> + struct extcon_dev edev;
> + void __iomem *regs;
> + u32 cfg0_ctx;
> +};
> +
> +static const int intel_mux_cable[] = {
> + EXTCON_USB_HOST,
> + EXTCON_NONE,
> +};
> +
> +static int intel_usb_mux_notifier(struct notifier_block *nb,
> + unsigned long old, void *ptr)
> +{
> + struct intel_usb_mux *mux = container_of(nb, struct intel_usb_mux, nb);
> + u32 val;
> +
> + if (mux->edev.state)
> + val = CFG0_SW_IDPIN_EN | CFG0_SW_DRD_STATIC_HOST;
> + else
> + val = CFG0_SW_IDPIN_EN | CFG0_SW_IDPIN | CFG0_SW_VBUS_VALID |
> + CFG0_SW_DRD_STATIC_DEV;
> +
> + writel(val, mux->regs);
> + return NOTIFY_OK;
> +}
> +
> +struct intel_usb_mux *intel_usb_mux_register(struct device *dev,
> + struct resource *r)
> +{
> + struct intel_usb_mux *mux;
> + int ret;
> +
> + mux = kzalloc(sizeof(*mux), GFP_KERNEL);
> + if (!mux)
> + return ERR_PTR(-ENOMEM);
> +
> + mux->regs = ioremap_nocache(r->start, resource_size(r));
> + if (!mux->regs) {
> + kfree(mux);
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + mux->cfg0_ctx = readl(mux->regs + INTEL_MUX_CFG0);
> +
> + mux->edev.dev.parent = dev;
> + mux->edev.supported_cable = intel_mux_cable;
> +
> + ret = extcon_dev_register(&mux->edev);
> + if (ret)
> + goto err;
> +
> + mux->edev.name = "intel_usb_mux";
> + mux->edev.state = !!(readl(mux->regs + INTEL_MUX_CFG1) & CFG1_MODE);
> +
> + /* An external source needs to tell us what to do */
> + mux->nb.notifier_call = intel_usb_mux_notifier;
> + ret = extcon_register_notifier(&mux->edev, EXTCON_USB_HOST, &mux->nb);
> + if (ret) {
> + dev_err(&mux->edev.dev, "failed to register notifier\n");
> + extcon_dev_unregister(&mux->edev);
> + goto err;
> + }
> + return mux;
> +err:
> + iounmap(mux->regs);
> + kfree(mux);
> + return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(intel_usb_mux_register);
> +
> +void intel_usb_mux_unregister(struct intel_usb_mux *mux)
> +{
> + extcon_unregister_notifier(&mux->edev, EXTCON_USB_HOST, &mux->nb);
> + extcon_dev_unregister(&mux->edev);
> + writel(mux->cfg0_ctx, mux->regs + INTEL_MUX_CFG0);
> + iounmap(mux->regs);
> + kfree(mux);
> +}
> +EXPORT_SYMBOL_GPL(intel_usb_mux_unregister);
so who's gonna call these two functions ? IMO, this looks like a recipe
for randbuild breakage.
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH 1/2] extcon: add driver for Intel USB mux
Hi Heikki,
Follow my comments below.
On Tue, Dec 01, 2015 at 03:32:37PM +0200, Heikki Krogerus wrote:
> Several Intel PCHs and SOCs have an internal mux that is
> used to share one USB port between USB Device Controller and
> xHCI. The mux is normally handled by System FW/BIOS, but not
> always. For those platforms where the FW does not take care
> of the mux, this driver is needed.
>
> Signed-off-by: Heikki Krogerus
> ---
> drivers/extcon/Kconfig | 5 ++
> drivers/extcon/Makefile | 1 +
> drivers/extcon/extcon-intel-usb.c| 112
> +++
> include/linux/extcon/intel_usb_mux.h | 31 ++
> 4 files changed, 149 insertions(+)
> create mode 100644 drivers/extcon/extcon-intel-usb.c
> create mode 100644 include/linux/extcon/intel_usb_mux.h
>
> diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
> index 0cebbf6..0a7ccb1 100644
> --- a/drivers/extcon/Kconfig
> +++ b/drivers/extcon/Kconfig
> @@ -118,3 +118,8 @@ config EXTCON_USB_GPIO
> Used typically if GPIO is used for USB ID pin detection.
>
> endif
> +
> +config EXTCON_INTEL_USB
> + bool
> + depends on X86 && USB
> + select EXTCON
> diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
> index ba787d0..e6e031a 100644
> --- a/drivers/extcon/Makefile
> +++ b/drivers/extcon/Makefile
> @@ -15,3 +15,4 @@ obj-$(CONFIG_EXTCON_PALMAS) += extcon-palmas.o
> obj-$(CONFIG_EXTCON_RT8973A) += extcon-rt8973a.o
> obj-$(CONFIG_EXTCON_SM5502) += extcon-sm5502.o
> obj-$(CONFIG_EXTCON_USB_GPIO)+= extcon-usb-gpio.o
> +obj-$(CONFIG_EXTCON_INTEL_USB) += extcon-intel-usb.o
> diff --git a/drivers/extcon/extcon-intel-usb.c
> b/drivers/extcon/extcon-intel-usb.c
> new file mode 100644
> index 000..5534781
> --- /dev/null
> +++ b/drivers/extcon/extcon-intel-usb.c
> @@ -0,0 +1,112 @@
> +/**
> + * extcon-intel-usb.c - Driver for Intel USB mux
> + *
> + * Copyright (C) 2015 Intel Corporation
> + * Author: Heikki Krogerus
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include
> +#include
> +
> +#include
> +
> +#define INTEL_MUX_CFG0 0x00
> +#define INTEL_MUX_CFG1 0x04
> +
> +#define CFG0_SW_DRD_MODE_MASK0x3
> +#define CFG0_SW_DRD_DYN 0
> +#define CFG0_SW_DRD_STATIC_HOST 1
> +#define CFG0_SW_DRD_STATIC_DEV 2
> +#define CFG0_SW_SYNC_SS_AND_HS BIT(2)
> +#define CFG0_SW_SWITCH_ENBIT(16)
> +#define CFG0_SW_IDPINBIT(20)
> +#define CFG0_SW_IDPIN_EN BIT(21)
> +#define CFG0_SW_VBUS_VALID BIT(24)
> +
> +#define CFG1_MODEBIT(29)
> +
> +struct intel_usb_mux {
> + struct notifier_block nb;
> + struct extcon_dev edev;
> + void __iomem *regs;
> + u32 cfg0_ctx;
> +};
> +
> +static const int intel_mux_cable[] = {
> + EXTCON_USB_HOST,
> + EXTCON_NONE,
> +};
> +
> +static int intel_usb_mux_notifier(struct notifier_block *nb,
> + unsigned long old, void *ptr)
> +{
> + struct intel_usb_mux *mux = container_of(nb, struct intel_usb_mux, nb);
> + u32 val;
> +
> + if (mux->edev.state)
> + val = CFG0_SW_IDPIN_EN | CFG0_SW_DRD_STATIC_HOST;
> + else
> + val = CFG0_SW_IDPIN_EN | CFG0_SW_IDPIN | CFG0_SW_VBUS_VALID |
> + CFG0_SW_DRD_STATIC_DEV;
> +
> + writel(val, mux->regs);
> + return NOTIFY_OK;
> +}
> +
> +struct intel_usb_mux *intel_usb_mux_register(struct device *dev,
> + struct resource *r)
> +{
> + struct intel_usb_mux *mux;
> + int ret;
> +
> + mux = kzalloc(sizeof(*mux), GFP_KERNEL);
> + if (!mux)
> + return ERR_PTR(-ENOMEM);
> +
> + mux->regs = ioremap_nocache(r->start, resource_size(r));
> + if (!mux->regs) {
> + kfree(mux);
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + mux->cfg0_ctx = readl(mux->regs + INTEL_MUX_CFG0);
> +
> + mux->edev.dev.parent = dev;
> + mux->edev.supported_cable = intel_mux_cable;
> +
> + ret = extcon_dev_register(&mux->edev);
> + if (ret)
> + goto err;
> +
> + mux->edev.name = "intel_usb_mux";
> + mux->edev.state = !!(readl(mux->regs + INTEL_MUX_CFG1) & CFG1_MODE);
> +
> + /* An external source needs to tell us what to do */
> + mux->nb.notifier_call = intel_usb_mux_notifier;
> + ret = extcon_register_notifier(&mux->edev, EXTCON_USB_HOST, &mux->nb);
> + if (ret) {
> + dev_err(&mux->edev.dev, "failed to register notifier\n");
> + extcon_dev_unregister(&mux->edev);
> + goto err;
> + }
> + return mux;
> +err:
> + iounmap(mux->regs);
> + kfree(mux);
> + return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(intel_usb_mux_register);
> +
> +void intel_usb_mux_unr
[PATCH 1/2] extcon: add driver for Intel USB mux
Several Intel PCHs and SOCs have an internal mux that is
used to share one USB port between USB Device Controller and
xHCI. The mux is normally handled by System FW/BIOS, but not
always. For those platforms where the FW does not take care
of the mux, this driver is needed.
Signed-off-by: Heikki Krogerus
---
drivers/extcon/Kconfig | 5 ++
drivers/extcon/Makefile | 1 +
drivers/extcon/extcon-intel-usb.c| 112 +++
include/linux/extcon/intel_usb_mux.h | 31 ++
4 files changed, 149 insertions(+)
create mode 100644 drivers/extcon/extcon-intel-usb.c
create mode 100644 include/linux/extcon/intel_usb_mux.h
diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
index 0cebbf6..0a7ccb1 100644
--- a/drivers/extcon/Kconfig
+++ b/drivers/extcon/Kconfig
@@ -118,3 +118,8 @@ config EXTCON_USB_GPIO
Used typically if GPIO is used for USB ID pin detection.
endif
+
+config EXTCON_INTEL_USB
+ bool
+ depends on X86 && USB
+ select EXTCON
diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
index ba787d0..e6e031a 100644
--- a/drivers/extcon/Makefile
+++ b/drivers/extcon/Makefile
@@ -15,3 +15,4 @@ obj-$(CONFIG_EXTCON_PALMAS) += extcon-palmas.o
obj-$(CONFIG_EXTCON_RT8973A) += extcon-rt8973a.o
obj-$(CONFIG_EXTCON_SM5502)+= extcon-sm5502.o
obj-$(CONFIG_EXTCON_USB_GPIO) += extcon-usb-gpio.o
+obj-$(CONFIG_EXTCON_INTEL_USB) += extcon-intel-usb.o
diff --git a/drivers/extcon/extcon-intel-usb.c
b/drivers/extcon/extcon-intel-usb.c
new file mode 100644
index 000..5534781
--- /dev/null
+++ b/drivers/extcon/extcon-intel-usb.c
@@ -0,0 +1,112 @@
+/**
+ * extcon-intel-usb.c - Driver for Intel USB mux
+ *
+ * Copyright (C) 2015 Intel Corporation
+ * Author: Heikki Krogerus
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include
+#include
+
+#include
+
+#define INTEL_MUX_CFG0 0x00
+#define INTEL_MUX_CFG1 0x04
+
+#define CFG0_SW_DRD_MODE_MASK 0x3
+#define CFG0_SW_DRD_DYN0
+#define CFG0_SW_DRD_STATIC_HOST1
+#define CFG0_SW_DRD_STATIC_DEV 2
+#define CFG0_SW_SYNC_SS_AND_HS BIT(2)
+#define CFG0_SW_SWITCH_EN BIT(16)
+#define CFG0_SW_IDPIN BIT(20)
+#define CFG0_SW_IDPIN_EN BIT(21)
+#define CFG0_SW_VBUS_VALID BIT(24)
+
+#define CFG1_MODE BIT(29)
+
+struct intel_usb_mux {
+ struct notifier_block nb;
+ struct extcon_dev edev;
+ void __iomem *regs;
+ u32 cfg0_ctx;
+};
+
+static const int intel_mux_cable[] = {
+ EXTCON_USB_HOST,
+ EXTCON_NONE,
+};
+
+static int intel_usb_mux_notifier(struct notifier_block *nb,
+ unsigned long old, void *ptr)
+{
+ struct intel_usb_mux *mux = container_of(nb, struct intel_usb_mux, nb);
+ u32 val;
+
+ if (mux->edev.state)
+ val = CFG0_SW_IDPIN_EN | CFG0_SW_DRD_STATIC_HOST;
+ else
+ val = CFG0_SW_IDPIN_EN | CFG0_SW_IDPIN | CFG0_SW_VBUS_VALID |
+ CFG0_SW_DRD_STATIC_DEV;
+
+ writel(val, mux->regs);
+ return NOTIFY_OK;
+}
+
+struct intel_usb_mux *intel_usb_mux_register(struct device *dev,
+struct resource *r)
+{
+ struct intel_usb_mux *mux;
+ int ret;
+
+ mux = kzalloc(sizeof(*mux), GFP_KERNEL);
+ if (!mux)
+ return ERR_PTR(-ENOMEM);
+
+ mux->regs = ioremap_nocache(r->start, resource_size(r));
+ if (!mux->regs) {
+ kfree(mux);
+ return ERR_PTR(-ENOMEM);
+ }
+
+ mux->cfg0_ctx = readl(mux->regs + INTEL_MUX_CFG0);
+
+ mux->edev.dev.parent = dev;
+ mux->edev.supported_cable = intel_mux_cable;
+
+ ret = extcon_dev_register(&mux->edev);
+ if (ret)
+ goto err;
+
+ mux->edev.name = "intel_usb_mux";
+ mux->edev.state = !!(readl(mux->regs + INTEL_MUX_CFG1) & CFG1_MODE);
+
+ /* An external source needs to tell us what to do */
+ mux->nb.notifier_call = intel_usb_mux_notifier;
+ ret = extcon_register_notifier(&mux->edev, EXTCON_USB_HOST, &mux->nb);
+ if (ret) {
+ dev_err(&mux->edev.dev, "failed to register notifier\n");
+ extcon_dev_unregister(&mux->edev);
+ goto err;
+ }
+ return mux;
+err:
+ iounmap(mux->regs);
+ kfree(mux);
+ return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(intel_usb_mux_register);
+
+void intel_usb_mux_unregister(struct intel_usb_mux *mux)
+{
+ extcon_unregister_notifier(&mux->edev, EXTCON_USB_HOST, &mux->nb);
+ extcon_dev_unregister(&mux->edev);
+ writel(mux->cfg0_ctx, mux->regs + INTEL_MUX_CFG0);
+ iounmap(mux->regs);
+ kfree(mux);
+}
+EXPORT_SYMBOL_GPL(intel_usb_mux_unregister);
diff --git a/include/
