| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20120416061531.GB3037@avionic-0098.adnet.avionic-design.de>
Date: Mon, 16 Apr 2012 08:15:31 +0200
From: Thierry Reding <thierry.reding@...onic-design.de>
To: Roland Stigge <stigge@...com.de>
Cc: arm@...nel.org, linux-arm-kernel@...ts.infradead.org,
gregkh@...uxfoundation.org, linux-usb@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-input@...r.kernel.org,
dmitry.torokhov@...il.com, axel.lin@...il.com,
broonie@...nsource.wolfsonmicro.com, marek.vasut@...il.com,
devel@...verdev.osuosl.org, kevin.wells@....com,
srinivas.bakki@....com
Subject: Re: [PATCH 1/4] ohci-nxp: Device tree support
* Roland Stigge wrote:
> This patch adds device tree support to ohci-nxp.c
>
> Signed-off-by: Roland Stigge <stigge@...com.de>
>
> ---
>
> Applies to v3.4-rc2
>
> Documentation/devicetree/bindings/usb/ohci-nxp.txt | 16 ++++
> drivers/usb/host/ohci-nxp.c | 68 ++++++++++++++-------
> 2 files changed, 64 insertions(+), 20 deletions(-)
>
> --- /dev/null
> +++ linux-2.6/Documentation/devicetree/bindings/usb/ohci-nxp.txt
> @@ -0,0 +1,16 @@
> +* OHCI controller, NXP ohci-nxp variant
> +
> +Required properties:
> +- compatible: must be "nxp,ohci-nxp"
> +- reg: physical base address of the controller and length of memory mapped
> + region.
> +- interrupts: The OHCI interrupt
> +
> +Example (LPC32xx):
> +
> + ohci@...20000 {
> + compatible = "nxp,ohci-nxp";
> + reg = <0x31020000 0x300>;
> + interrupt-parent = <&mic>;
> + interrupts = <0x3b 0>;
> + };
> --- linux-2.6.orig/drivers/usb/host/ohci-nxp.c
> +++ linux-2.6/drivers/usb/host/ohci-nxp.c
> @@ -22,6 +22,7 @@
> #include <linux/clk.h>
> #include <linux/platform_device.h>
> #include <linux/i2c.h>
> +#include <linux/of.h>
>
> #include <mach/hardware.h>
> #include <asm/mach-types.h>
> @@ -29,7 +30,6 @@
>
> #include <mach/platform.h>
> #include <mach/irqs.h>
> -#include <asm/gpio.h>
>
> #define USB_CONFIG_BASE 0x31020000
> #define PWRMAN_BASE 0x40004000
> @@ -38,6 +38,7 @@
>
> /* USB_CTRL bit defines */
> #define USB_SLAVE_HCLK_EN (1 << 24)
> +#define USB_DEV_NEED_CLK_EN (1 << 22)
> #define USB_HOST_NEED_CLK_EN (1 << 21)
>
> #define USB_OTG_CLK_CTRL IO_ADDRESS(USB_CONFIG_BASE + 0xFF4)
> @@ -117,7 +118,6 @@ static struct i2c_driver isp1301_driver;
> static struct i2c_client *isp1301_i2c_client;
>
> extern int usb_disabled(void);
> -extern int ocpi_enable(void);
>
> static struct clk *usb_clk;
>
> @@ -220,7 +220,7 @@ static void isp1301_configure_lpc32xx(vo
> ISP1301_I2C_INTERRUPT_RISING | ISP1301_I2C_REG_CLEAR_ADDR, ~0);
>
> /* Enable usb_need_clk clock after transceiver is initialized */
> - __raw_writel((__raw_readl(USB_CTRL) | (1 << 22)), USB_CTRL);
> + __raw_writel((__raw_readl(USB_CTRL) | USB_HOST_NEED_CLK_EN), USB_CTRL);
>
> printk(KERN_INFO "ISP1301 Vendor ID : 0x%04x\n",
> i2c_smbus_read_word_data(isp1301_i2c_client, 0x00));
> @@ -266,7 +266,7 @@ static void nxp_stop_hc(void)
> __raw_writel(tmp, USB_OTG_STAT_CONTROL);
> }
>
The above changes aren't related to the DT conversion. Should they perhaps go
into a separate patch?
> -static int __devinit ohci_nxp_start(struct usb_hcd *hcd)
> +static int ohci_nxp_start(struct usb_hcd *hcd)
> {
> struct ohci_hcd *ohci = hcd_to_ohci(hcd);
> int ret;
> @@ -367,16 +367,20 @@ static void nxp_unset_usb_bits(void)
> }
> }
>
> -static int __devinit usb_hcd_nxp_probe(struct platform_device *pdev)
> +static int usb_hcd_nxp_probe(struct platform_device *pdev)
What's the reason for dropping __devinit?
> {
> struct usb_hcd *hcd = 0;
> struct ohci_hcd *ohci;
> const struct hc_driver *driver = &ohci_nxp_hc_driver;
> struct i2c_adapter *i2c_adap;
> struct i2c_board_info i2c_info;
> + struct resource *res;
>
> int ret = 0, irq;
>
> + pdev->dev.coherent_dma_mask = 0xFFFFFFFF;
DMA_BIT_MASK(32)?
> + pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
> +
> dev_dbg(&pdev->dev, "%s: " DRIVER_DESC " (nxp)\n", hcd_name);
> if (usb_disabled()) {
> err("USB is disabled");
> @@ -384,16 +388,10 @@ static int __devinit usb_hcd_nxp_probe(s
> goto out;
> }
>
> - if (pdev->num_resources != 2
> - || pdev->resource[0].flags != IORESOURCE_MEM
> - || pdev->resource[1].flags != IORESOURCE_IRQ) {
> - err("Invalid resource configuration");
> - ret = -ENODEV;
> - goto out;
> - }
> -
> /* Enable AHB slave USB clock, needed for further USB clock control */
> - __raw_writel(USB_SLAVE_HCLK_EN | (1 << 19), USB_CTRL);
> + __raw_writel(__raw_readl(USB_CTRL) | USB_SLAVE_HCLK_EN, USB_CTRL);
> + __raw_writel(__raw_readl(USB_CTRL) & ~(1 << 20), USB_CTRL);
> + __raw_writel(__raw_readl(USB_CTRL) | (1 << 19), USB_CTRL);
You're already adding a define for bit 22 (USB_DEV_NEED_CLK_EN) above, why
not add one for bits 19 and 20 as well?
>
> ret = i2c_add_driver(&isp1301_driver);
> if (ret < 0) {
> @@ -401,6 +399,11 @@ static int __devinit usb_hcd_nxp_probe(s
> goto out;
> }
> i2c_adap = i2c_get_adapter(2);
> + if (!i2c_adap) {
> + err("failed on i2c_get_adapter");
> + goto out_i2c_driver;
> + }
> +
> memset(&i2c_info, 0, sizeof(struct i2c_board_info));
> strlcpy(i2c_info.type, "isp1301_nxp", I2C_NAME_SIZE);
> isp1301_i2c_client = i2c_new_probed_device(i2c_adap, &i2c_info,
> @@ -435,6 +438,7 @@ static int __devinit usb_hcd_nxp_probe(s
> }
>
> __raw_writel(__raw_readl(USB_CTRL) | USB_HOST_NEED_CLK_EN, USB_CTRL);
> + __raw_writel(__raw_readl(USB_CTRL) & ~USB_DEV_NEED_CLK_EN, USB_CTRL);
>
> /* Set to enable all needed USB clocks */
> __raw_writel(USB_CLOCK_MASK, USB_OTG_CLK_CTRL);
> @@ -442,7 +446,7 @@ static int __devinit usb_hcd_nxp_probe(s
> while ((__raw_readl(USB_OTG_CLK_STAT) & USB_CLOCK_MASK) !=
> USB_CLOCK_MASK) ;
>
> - hcd = usb_create_hcd (driver, &pdev->dev, dev_name(&pdev->dev));
> + hcd = usb_create_hcd(driver, &pdev->dev, dev_name(&pdev->dev));
> if (!hcd) {
> err("Failed to allocate HC buffer");
> ret = -ENOMEM;
> @@ -452,19 +456,30 @@ static int __devinit usb_hcd_nxp_probe(s
> /* Set all USB bits in the Start Enable register */
> nxp_set_usb_bits();
>
> - hcd->rsrc_start = pdev->resource[0].start;
> - hcd->rsrc_len = pdev->resource[0].end - pdev->resource[0].start + 1;
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + dev_err(&pdev->dev, "Failed to get MEM resource");
> + ret = -ENOMEM;
> + goto out4;
> + }
> + hcd->rsrc_start = IO_ADDRESS(res->start);
This looks wrong.
> + hcd->rsrc_len = resource_size(res);
> if (!request_mem_region(hcd->rsrc_start, hcd->rsrc_len, hcd_name)) {
> - dev_dbg(&pdev->dev, "request_mem_region failed\n");
> + dev_err(&pdev->dev, "request_mem_region failed\n");
> ret = -ENOMEM;
> goto out4;
> }
> - hcd->regs = (void __iomem *)pdev->resource[0].start;
> + hcd->regs = ioremap(res->start, hcd->rsrc_len);
> + if (!hcd->regs) {
> + err("Failed to ioremap");
> + ret = -ENOMEM;
> + goto out5;
> + }
Since this is pretty much boilerplate, can you not replace it by
devm_request_and_ioremap()?
>
> irq = platform_get_irq(pdev, 0);
> if (irq < 0) {
> ret = -ENXIO;
> - goto out4;
> + goto out6;
> }
>
> nxp_start_hc();
> @@ -478,6 +493,10 @@ static int __devinit usb_hcd_nxp_probe(s
> return ret;
>
> nxp_stop_hc();
> +out6:
> + iounmap(hcd->regs);
> +out5:
> + release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
If you use devm_request_and_ioremap() this cleanup is no longer necessary.
> out4:
> nxp_unset_usb_bits();
> usb_put_hcd(hcd);
> @@ -517,10 +536,19 @@ static int usb_hcd_nxp_remove(struct pla
> /* work with hotplug and coldplug */
> MODULE_ALIAS("platform:usb-ohci");
>
> +#ifdef CONFIG_OF
> +static const struct of_device_id usb_hcd_nxp_match[] = {
> + { .compatible = "nxp,ohci-nxp" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, usb_hcd_nxp_match);
> +#endif
> +
> static struct platform_driver usb_hcd_nxp_driver = {
> .driver = {
> .name = "usb-ohci",
> .owner = THIS_MODULE,
> + .of_match_table = of_match_ptr(usb_hcd_nxp_match),
> },
> .probe = usb_hcd_nxp_probe,
> .remove = usb_hcd_nxp_remove,
This hunk and the binding documentation are the only DT related changes, the
rest should go into a separate patch because they're really only cleanups.
Thierry
Content of type "application/pgp-signature" skipped
Powered by blists - more mailing lists