Re: [PATCH 1/3] usb: chipidea: add CSR SiRFSoC ci13xxx usb driver
Barry Song 21cn...@gmail.com writes: +if USB_CHIPIDEA_UDC USB_CHIPIDEA_HOST + +config USB_CHIPIDEA_SIRF + depends on ARCH_SIRF + bool SiRF USB controller ChipIdea driver binding + default y + help + Say Y here to enable sirf usb ChipIdea driver binding. + +config USB_CHIPIDEA_MSM + depends on ARCH_MSM + bool MSM USB controller ChipIdea driver binding + default y + help + Say Y here to enable msm usb ChipIdea driver binding. + +config USB_CHIPIDEA_IMX + depends on ARCH_MXC || ARCH_MXS + bool i.MX USB controller ChipIdea driver binding + default y + help + Say Y here to enable imx usb ChipIdea driver binding. + +endif No. This is wrong on at least 3 different levels. + endif diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile index 4ab83e9..7004fde 100644 --- a/drivers/usb/chipidea/Makefile +++ b/drivers/usb/chipidea/Makefile @@ -9,7 +9,7 @@ ci_hdrc-$(CONFIG_USB_CHIPIDEA_DEBUG) += debug.o # Glue/Bridge layers go here -obj-$(CONFIG_USB_CHIPIDEA) += ci13xxx_msm.o +obj-$(CONFIG_USB_CHIPIDEA_MSM) += ci13xxx_msm.o No. # PCI doesn't provide stubs, need to check ifneq ($(CONFIG_PCI),) @@ -17,5 +17,6 @@ ifneq ($(CONFIG_PCI),) endif ifneq ($(CONFIG_OF_DEVICE),) - obj-$(CONFIG_USB_CHIPIDEA) += ci13xxx_imx.o usbmisc_imx.o + obj-$(CONFIG_USB_CHIPIDEA_IMX) += ci13xxx_imx.o usbmisc_imx.o + obj-$(CONFIG_USB_CHIPIDEA_SIRF) += ci13xxx_sirf.o endif No. Regards, -- Alex -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] usb: chipidea: add CSR SiRFSoC ci13xxx usb driver
2013/6/14 Alexander Shishkin alexander.shish...@linux.intel.com: Barry Song 21cn...@gmail.com writes: +if USB_CHIPIDEA_UDC USB_CHIPIDEA_HOST + +config USB_CHIPIDEA_SIRF + depends on ARCH_SIRF + bool SiRF USB controller ChipIdea driver binding + default y + help + Say Y here to enable sirf usb ChipIdea driver binding. + +config USB_CHIPIDEA_MSM + depends on ARCH_MSM + bool MSM USB controller ChipIdea driver binding + default y + help + Say Y here to enable msm usb ChipIdea driver binding. + +config USB_CHIPIDEA_IMX + depends on ARCH_MXC || ARCH_MXS + bool i.MX USB controller ChipIdea driver binding + default y + help + Say Y here to enable imx usb ChipIdea driver binding. + +endif No. This is wrong on at least 3 different levels. the point is we don't directly include MSM, IMX, SiRF and all other drivers once we enable chipidea. that makes these drivers optional, not always be built-in. -barry -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] usb: chipidea: add CSR SiRFSoC ci13xxx usb driver
Barry Song 21cn...@gmail.com writes: 2013/6/14 Alexander Shishkin alexander.shish...@linux.intel.com: Barry Song 21cn...@gmail.com writes: +if USB_CHIPIDEA_UDC USB_CHIPIDEA_HOST + +config USB_CHIPIDEA_SIRF + depends on ARCH_SIRF + bool SiRF USB controller ChipIdea driver binding + default y + help + Say Y here to enable sirf usb ChipIdea driver binding. + +config USB_CHIPIDEA_MSM + depends on ARCH_MSM + bool MSM USB controller ChipIdea driver binding + default y + help + Say Y here to enable msm usb ChipIdea driver binding. + +config USB_CHIPIDEA_IMX + depends on ARCH_MXC || ARCH_MXS + bool i.MX USB controller ChipIdea driver binding + default y + help + Say Y here to enable imx usb ChipIdea driver binding. + +endif No. This is wrong on at least 3 different levels. the point is we don't directly include MSM, IMX, SiRF and all other drivers once we enable chipidea. that makes these drivers optional, not always be built-in. This has been discussed several times already. Just set the USB_CHIPIDEA=m and then everything will be built as modules and nothing will be built in. What exactly is the problem you're trying to solve with this? The only way something like this will go in is with Jiri's COMPILE_TEST patchset. This snippet above is just buggy. Regards, -- Alex -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] usb: chipidea: add CSR SiRFSoC ci13xxx usb driver
2013/6/14 Alexander Shishkin alexander.shish...@linux.intel.com: Barry Song 21cn...@gmail.com writes: 2013/6/14 Alexander Shishkin alexander.shish...@linux.intel.com: Barry Song 21cn...@gmail.com writes: +if USB_CHIPIDEA_UDC USB_CHIPIDEA_HOST + +config USB_CHIPIDEA_SIRF + depends on ARCH_SIRF + bool SiRF USB controller ChipIdea driver binding + default y + help + Say Y here to enable sirf usb ChipIdea driver binding. + +config USB_CHIPIDEA_MSM + depends on ARCH_MSM + bool MSM USB controller ChipIdea driver binding + default y + help + Say Y here to enable msm usb ChipIdea driver binding. + +config USB_CHIPIDEA_IMX + depends on ARCH_MXC || ARCH_MXS + bool i.MX USB controller ChipIdea driver binding + default y + help + Say Y here to enable imx usb ChipIdea driver binding. + +endif No. This is wrong on at least 3 different levels. the point is we don't directly include MSM, IMX, SiRF and all other drivers once we enable chipidea. that makes these drivers optional, not always be built-in. This has been discussed several times already. Just set the USB_CHIPIDEA=m and then everything will be built as modules and nothing will be built in. What exactly is the problem you're trying to solve with this? Alex, it seems it is not much a generic way to use building-module to avoid all involved components built-in. but since you have discussed several times and got agreement, i'll follow it and don't re-open it. The only way something like this will go in is with Jiri's COMPILE_TEST patchset. This snippet above is just buggy. Regards, -- Alex -barry -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] usb: chipidea: add CSR SiRFSoC ci13xxx usb driver
On Sun, Jun 9, 2013 at 6:25 AM, Barry Song 21cn...@gmail.com wrote:
CSR SiRF SoCs licensed chipidea ci13xxx USB IP, this patch
makes the chipidea drivers support CSR SiRF SoCS.
It also changes the Kconfig, only compile MSM and IMX if related
drivers are enabled. Otherwise, we always need to enable all
clients of chipidea drivers.
+ /* 5. set device dma mask */
+ if (!pdev-dev.dma_mask) {
+ pdev-dev.dma_mask = devm_kzalloc(pdev-dev,
+ sizeof(*pdev-dev.dma_mask),
GFP_KERNEL);
+ if (!pdev-dev.dma_mask) {
+ dev_err(pdev-dev, Failed to alloc dma_mask!\n);
+ ret = -ENOMEM;
+ goto err;
+ }
+ *pdev-dev.dma_mask = DMA_BIT_MASK(32);
+ dma_set_coherent_mask(pdev-dev, *pdev-dev.dma_mask);
Perhaps you could avoid this allocation if you apply coherent mask
first and its address will be used as dma_mask value.
+ }
+err:
+ clk_disable_unprepare(data-clk);
+ return ret;
Please, check, but if I remember correctly devm_clk* is doing this for
you on release stage.
+static int ci13xxx_sirf_remove(struct platform_device *pdev)
+{
+ struct ci13xxx_sirf_data *data = platform_get_drvdata(pdev);
+
+ pm_runtime_disable(pdev-dev);
+ ci13xxx_remove_device(data-ci_pdev);
+
+ clk_disable_unprepare(data-clk);
Ditto.
--
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] usb: chipidea: add CSR SiRFSoC ci13xxx usb driver
On Sun, Jun 09, 2013 at 11:25:36AM +0800, Barry Song wrote:
From: Rong Wang rong.w...@csr.com
CSR SiRF SoCs licensed chipidea ci13xxx USB IP, this patch
makes the chipidea drivers support CSR SiRF SoCS.
It also changes the Kconfig, only compile MSM and IMX if related
drivers are enabled. Otherwise, we always need to enable all
clients of chipidea drivers.
Cc: Marek Vasut ma...@denx.de
Cc: Richard Zhao richard.z...@freescale.com
Signed-off-by: Rong Wang rong.w...@csr.com
Signed-off-by: Barry Song baohua.s...@csr.com
---
drivers/usb/Kconfig |1 +
drivers/usb/chipidea/Kconfig| 25
drivers/usb/chipidea/Makefile |5 +-
drivers/usb/chipidea/ci13xxx_sirf.c | 223
+++
4 files changed, 252 insertions(+), 2 deletions(-)
create mode 100644 drivers/usb/chipidea/ci13xxx_sirf.c
diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
index 92e1dc9..9cbe1e0 100644
--- a/drivers/usb/Kconfig
+++ b/drivers/usb/Kconfig
@@ -49,6 +49,7 @@ config USB_ARCH_HAS_EHCI
default y if ARCH_MMP
default y if MACH_LOONGSON1
default y if PLAT_ORION
+ default y if ARCH_SIRF
default PCI
# some non-PCI HCDs implement xHCI
diff --git a/drivers/usb/chipidea/Kconfig b/drivers/usb/chipidea/Kconfig
index b2df442..847b9f7 100644
--- a/drivers/usb/chipidea/Kconfig
+++ b/drivers/usb/chipidea/Kconfig
@@ -31,4 +31,29 @@ config USB_CHIPIDEA_DEBUG
help
Say Y here to enable debugging output of the ChipIdea driver.
+if USB_CHIPIDEA_UDC USB_CHIPIDEA_HOST
+
+config USB_CHIPIDEA_SIRF
+ depends on ARCH_SIRF
+ bool SiRF USB controller ChipIdea driver binding
+ default y
+ help
+ Say Y here to enable sirf usb ChipIdea driver binding.
+
+config USB_CHIPIDEA_MSM
+ depends on ARCH_MSM
+ bool MSM USB controller ChipIdea driver binding
+ default y
+ help
+ Say Y here to enable msm usb ChipIdea driver binding.
+
+config USB_CHIPIDEA_IMX
+ depends on ARCH_MXC || ARCH_MXS
+ bool i.MX USB controller ChipIdea driver binding
+ default y
+ help
+ Say Y here to enable imx usb ChipIdea driver binding.
+
+endif
+
endif
diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile
index 4ab83e9..7004fde 100644
--- a/drivers/usb/chipidea/Makefile
+++ b/drivers/usb/chipidea/Makefile
@@ -9,7 +9,7 @@ ci_hdrc-$(CONFIG_USB_CHIPIDEA_DEBUG) += debug.o
# Glue/Bridge layers go here
-obj-$(CONFIG_USB_CHIPIDEA) += ci13xxx_msm.o
+obj-$(CONFIG_USB_CHIPIDEA_MSM) += ci13xxx_msm.o
# PCI doesn't provide stubs, need to check
ifneq ($(CONFIG_PCI),)
@@ -17,5 +17,6 @@ ifneq ($(CONFIG_PCI),)
endif
ifneq ($(CONFIG_OF_DEVICE),)
- obj-$(CONFIG_USB_CHIPIDEA) += ci13xxx_imx.o usbmisc_imx.o
+ obj-$(CONFIG_USB_CHIPIDEA_IMX) += ci13xxx_imx.o usbmisc_imx.o
+ obj-$(CONFIG_USB_CHIPIDEA_SIRF) += ci13xxx_sirf.o
endif
diff --git a/drivers/usb/chipidea/ci13xxx_sirf.c
b/drivers/usb/chipidea/ci13xxx_sirf.c
new file mode 100644
index 000..1d84a2f
--- /dev/null
+++ b/drivers/usb/chipidea/ci13xxx_sirf.c
@@ -0,0 +1,223 @@
+/*
+ * USB Controller Driver for CSR SiRF SoC
+ *
+ * Copyright (c) 2011 Cambridge Silicon Radio Limited, a CSR plc group
company.
+ * Rong Wangrong.w...@csr.com
+ *
+ * Licensed under GPLv2 or later.
+ */
+#include linux/module.h
+#include linux/io.h
+#include linux/bitops.h
+#include linux/platform_device.h
+#include linux/pm_runtime.h
+#include linux/of.h
+#include linux/of_gpio.h
+#include linux/of_address.h
+#include linux/of_platform.h
+#include linux/clk.h
+#include linux/dma-mapping.h
+#include linux/delay.h
+#include linux/reset.h
+
+#include linux/usb/chipidea.h
+#include ci.h
+
+#define RSC_USB_UART_SHARE 0x0
+#define USB1_MODE_SELBIT(2)
+#define pdev_to_phy(pdev)((struct usb_phy *)platform_get_drvdata(pdev))
sorry, no go. This is not the right way to handle USB PHYs.
+static int sirfsoc_vbus_gpio;
this should be removed too, add it as a member of ci13xx_sirf_data.
+struct ci13xxx_sirf_data {
+ struct platform_device *ci_pdev;
most likely you don't need the platform_device, you need the struct
device only.
+ struct clk *clk;
+};
+
+static inline int ci13xxx_sirf_drive_vbus(int value)
NACK, you should pass your ci13xx_sirf_data as argument here.
+{
+ return gpio_direction_output(sirfsoc_vbus_gpio, value ? 0 : 1);
+}
+
+static void ci13xxx_sirf_notify_event(struct ci13xxx *ci, unsigned event)
+{
+ switch (event) {
+ case CI13XXX_CONTROLLER_RESET_EVENT:
+ ci13xxx_sirf_drive_vbus(1);
+ break;
+ case CI13XXX_CONTROLLER_STOPPED_EVENT:
+ ci13xxx_sirf_drive_vbus(0);
+ break;
+ default:
+ dev_info(ci-dev, Unknown Event\n);
+ break;
+ }
+}
+
+static
Re: [PATCH 1/3] usb: chipidea: add CSR SiRFSoC ci13xxx usb driver
Hi Arnd,
2013/6/9 Arnd Bergmann a...@arndb.de:
On Sunday 09 June 2013 11:25:36 Barry Song wrote:
From: Rong Wang rong.w...@csr.com
CSR SiRF SoCs licensed chipidea ci13xxx USB IP, this patch
makes the chipidea drivers support CSR SiRF SoCS.
It also changes the Kconfig, only compile MSM and IMX if related
drivers are enabled. Otherwise, we always need to enable all
clients of chipidea drivers.
Can't you use the same driver as for imx and make it more generic?
I don't actually see anything in here that is really specific to SiRF
and most of the code is the same as for imx.
it seems you means a common driver like drivers/mmc/host/sdhci-pltfm.c
for mmc host.
it is a good idea.
If there are bits that are truly SiRF specific, at least that can
be a much smaller part I think.
+#define RSC_USB_UART_SHARE 0x0
+#define USB1_MODE_SELBIT(2)
+#define pdev_to_phy(pdev)((struct usb_phy *)platform_get_drvdata(pdev))
+
+static int sirfsoc_vbus_gpio;
What do you need static data for? This seems like a bad idea because it
makes it impossible to support multiple such devices.
this should be attached to the data struct. sorry for my carelessness.
rong did a very good job at first glance, then i took easy and missed
to read one line by one line before i sent and missed some issues.
+struct ci13xxx_sirf_data {
+ struct platform_device *ci_pdev;
+ struct clk *clk;
+};
+
+static inline int ci13xxx_sirf_drive_vbus(int value)
+{
+ return gpio_direction_output(sirfsoc_vbus_gpio, value ? 0 : 1);
+}
+
+static void ci13xxx_sirf_notify_event(struct ci13xxx *ci, unsigned event)
+{
+ switch (event) {
+ case CI13XXX_CONTROLLER_RESET_EVENT:
+ ci13xxx_sirf_drive_vbus(1);
+ break;
+ case CI13XXX_CONTROLLER_STOPPED_EVENT:
+ ci13xxx_sirf_drive_vbus(0);
+ break;
+ default:
+ dev_info(ci-dev, Unknown Event\n);
+ break;
+ }
+}
+
+static struct ci13xxx_platform_data ci13xxx_sirf_platdata = {
+ .name = ci13xxx_sirf,
+ .flags = CI13XXX_DISABLE_STREAMING,
+ .capoffset = DEF_CAPOFFSET,
+ .notify_event = ci13xxx_sirf_notify_event,
+};
+
+static struct of_device_id rsc_ids[] = {
+ { .compatible = sirf,prima2-rsc, },
+ { /* sentinel */ }
+};
This is the reset controller, right? You already use the reset API
below, why do you need to open-code the gpio
it's not reset controller. this is a Resource Sharing Control Module
involved with pinmux, we have sirf pinctrl driver, so we should move
to that.
here the problem is the driver is still hardcoding the pinmux between
uart and usb.
+static int ci13xxx_sirf_probe(struct platform_device *pdev)
+{
+ struct platform_device *plat_ci, *phy_pdev;
+ struct device_node *rsc_np, *phy_np;
+ struct ci13xxx_sirf_data *data;
+ struct usb_phy *phy;
+ void __iomem *rsc_vbase;
+ int ret;
+
+ data = devm_kzalloc(pdev-dev, sizeof(*data), GFP_KERNEL);
+ if (!data) {
+ dev_err(pdev-dev, Failed to allocate ci13xxx_sirf_data!\n);
+ return -ENOMEM;
+ }
+
+ /* 1. set usb controller clock */
+ data-clk = devm_clk_get(pdev-dev, NULL);
+ if (IS_ERR(data-clk)) {
+ dev_err(pdev-dev,
+ Failed to get clock, err=%ld\n, PTR_ERR(data-clk));
+ return PTR_ERR(data-clk);
+ }
+ ret = clk_prepare_enable(data-clk);
+ if (ret) {
+ dev_err(pdev-dev,
+ Failed to prepare or enable clock, err=%d\n, ret);
+ return ret;
+ }
+
+ /* 2. software reset */
+ ret = device_reset(pdev-dev);
+ if (ret)
+ dev_info(pdev-dev,
+ Failed to reset device, err=%d\n, ret);
+
+ /* 3. vbus configuration */
+ sirfsoc_vbus_gpio = of_get_named_gpio(pdev-dev.of_node,
+ vbus-gpios, 0);
+ if (sirfsoc_vbus_gpio 0) {
+ dev_err(pdev-dev, Can't get vbus gpio from DT\n);
+ ret = -ENODEV;
+ goto err;
+ }
+ ret = gpio_request(sirfsoc_vbus_gpio, ci13xxx_sirf);
+ if (ret) {
+ dev_err(pdev-dev, Failed to get gpio control\n);
+ goto err;
+ }
+
This seems totally generic so far, better put it into a common file.
not so generic, it seems. i think we might need some comments here to
explain why.
+ /* 4. rsc control */
+ rsc_np = of_find_matching_node(NULL, rsc_ids);
+ if (!rsc_np) {
+ dev_err(pdev-dev, Failed to get rsc device node\n);
+ ret = -ENODEV;
+ goto err;
+ }
+ rsc_vbase = of_iomap(rsc_np, 0);
+ if (!rsc_vbase) {
+ dev_err(pdev-dev, Failed to iomap rsc memory\n);
+ ret = -ENOMEM;
+
