| Message ID | 1384067901-9377-1-git-send-email-shc_work@mail.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Alexander, On Sunday, November 10, 2013 03:18 PM, Alexander Shiyan wrote: > This adds i.MX27 and i.MX31 as the next user of the usbmisc driver. > > Signed-off-by: Alexander Shiyan<shc_work@mail.ru> > --- > drivers/usb/chipidea/usbmisc_imx.c | 42 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 42 insertions(+) > > diff --git a/drivers/usb/chipidea/usbmisc_imx.c b/drivers/usb/chipidea/usbmisc_imx.c > index 8a1094b..4381c5a6 100644 > --- a/drivers/usb/chipidea/usbmisc_imx.c > +++ b/drivers/usb/chipidea/usbmisc_imx.c > @@ -21,6 +21,10 @@ > #define MX25_USB_PHY_CTRL_OFFSET 0x08 > #define MX25_BM_EXTERNAL_VBUS_DIVIDER BIT(23) > > +#define MX27_H1_PM_BIT BIT(8) > +#define MX27_H2_PM_BIT BIT(16) > +#define MX27_OTG_PM_BIT BIT(24) > + > #define MX53_USB_OTG_PHY_CTRL_0_OFFSET 0x08 > #define MX53_USB_UH2_CTRL_OFFSET 0x14 > #define MX53_USB_UH3_CTRL_OFFSET 0x18 > @@ -68,6 +72,36 @@ static int usbmisc_imx25_post(struct imx_usbmisc_data *data) > return 0; > } > > +static int usbmisc_imx27_init(struct imx_usbmisc_data *data) > +{ > + unsigned long flags; > + u32 val; > + > + switch (data->index) { > + case 0: > + val = MX27_OTG_PM_BIT; > + break; > + case 1: > + val = MX27_H1_PM_BIT; > + break; > + case 2: > + val = MX27_H2_PM_BIT; > + break; > + default: > + return -EINVAL; > + }; > + From my understanding this can not work, the usbmisc->base not point into the usb control register (USB_CTRL). Reference manual 30.5.1.1 says BASE + 0x600 you must add the offset to the readl instruction. > + spin_lock_irqsave(&usbmisc->lock, flags); > + if (data->disable_oc) > + val = readl(usbmisc->base) | val; else part not needed, the registers bits are set to 0 (reset) the function is called on start-up once only, right?! > + else > + val = readl(usbmisc->base)& ~val; > + writel(val, usbmisc->base); > + spin_unlock_irqrestore(&usbmisc->lock, flags); > + > + return 0; > +} > + > static int usbmisc_imx53_init(struct imx_usbmisc_data *data) > { > void __iomem *reg = NULL; > @@ -128,6 +162,10 @@ static const struct usbmisc_ops imx25_usbmisc_ops = { > .post = usbmisc_imx25_post, > }; > > +static const struct usbmisc_ops imx27_usbmisc_ops = { > + .init = usbmisc_imx27_init, > +}; > + > static const struct usbmisc_ops imx53_usbmisc_ops = { > .init = usbmisc_imx53_init, > }; > @@ -162,6 +200,10 @@ static const struct of_device_id usbmisc_imx_dt_ids[] = { > .data =&imx25_usbmisc_ops, > }, > { > + .compatible = "fsl,imx27-usbmisc", > + .data =&imx27_usbmisc_ops, > + }, > + { > .compatible = "fsl,imx53-usbmisc", > .data =&imx53_usbmisc_ops, > }, Regards Chris
> Hi Alexander, > > On Sunday, November 10, 2013 03:18 PM, Alexander Shiyan wrote: > > This adds i.MX27 and i.MX31 as the next user of the usbmisc driver. > > > > Signed-off-by: Alexander Shiyan<shc_work@mail.ru> > > --- > > drivers/usb/chipidea/usbmisc_imx.c | 42 ++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 42 insertions(+) > > > > diff --git a/drivers/usb/chipidea/usbmisc_imx.c b/drivers/usb/chipidea/usbmisc_imx.c ... > > +static int usbmisc_imx27_init(struct imx_usbmisc_data *data) > > +{ > > + unsigned long flags; > > + u32 val; > > + > > + switch (data->index) { > > + case 0: > > + val = MX27_OTG_PM_BIT; > > + break; > > + case 1: > > + val = MX27_H1_PM_BIT; > > + break; > > + case 2: > > + val = MX27_H2_PM_BIT; > > + break; > > + default: > > + return -EINVAL; > > + }; > > + > > From my understanding this can not work, the usbmisc->base not point into the > usb control register (USB_CTRL). Reference manual 30.5.1.1 says > BASE + 0x600 > you must add the offset to the readl instruction. Why not work? usbotg: usb@10024000 usbh1: usb@10024200 usbh2: usb@10024400 usbmisc: usbmisc@10024600 So, offset to USB_CTRL should already be defined by DTS. > > + spin_lock_irqsave(&usbmisc->lock, flags); > > + if (data->disable_oc) > > + val = readl(usbmisc->base) | val; > > else part not needed, the registers bits are set to 0 (reset) > the function is called on start-up once only, right?! > > > + else > > + val = readl(usbmisc->base)& ~val; > > + writel(val, usbmisc->base); > > + spin_unlock_irqrestore(&usbmisc->lock, flags); > > + > > + return 0; > > +} Bit can be set/cleared wrongly by the bootloader, it is not a big overhead to set it in proper state. ... ---
On Monday, November 11, 2013 12:45 PM, Alexander Shiyan wrote: >> Hi Alexander, >> >> On Sunday, November 10, 2013 03:18 PM, Alexander Shiyan wrote: >>> This adds i.MX27 and i.MX31 as the next user of the usbmisc driver. >>> >>> Signed-off-by: Alexander Shiyan<shc_work@mail.ru> >>> --- >>> drivers/usb/chipidea/usbmisc_imx.c | 42 ++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 42 insertions(+) >>> >>> diff --git a/drivers/usb/chipidea/usbmisc_imx.c b/drivers/usb/chipidea/usbmisc_imx.c > ... >>> +static int usbmisc_imx27_init(struct imx_usbmisc_data *data) >>> +{ >>> + unsigned long flags; >>> + u32 val; >>> + >>> + switch (data->index) { >>> + case 0: >>> + val = MX27_OTG_PM_BIT; >>> + break; >>> + case 1: >>> + val = MX27_H1_PM_BIT; >>> + break; >>> + case 2: >>> + val = MX27_H2_PM_BIT; >>> + break; >>> + default: >>> + return -EINVAL; >>> + }; >>> + >> >> From my understanding this can not work, the usbmisc->base not point into the >> usb control register (USB_CTRL). Reference manual 30.5.1.1 says >> BASE + 0x600 >> you must add the offset to the readl instruction. > > Why not work? > usbotg: usb@10024000 > usbh1: usb@10024200 > usbh2: usb@10024400 > usbmisc: usbmisc@10024600 > So, offset to USB_CTRL should already be defined by DTS. in the usbmisc_imx_probe() the base pointer is loaded from res = platform_get_resource(pdev, IORESOURCE_MEM, 0); data->base = devm_ioremap_resource(&pdev->dev, res); (and I did not see any of_ operations) usbmisc = data; base is set to 0x10024000 when I look around all other functions init functions did a offset calculation. > > >>> + spin_lock_irqsave(&usbmisc->lock, flags); >>> + if (data->disable_oc) >>> + val = readl(usbmisc->base) | val; >> >> else part not needed, the registers bits are set to 0 (reset) >> the function is called on start-up once only, right?! >> >>> + else >>> + val = readl(usbmisc->base)& ~val; >>> + writel(val, usbmisc->base); >>> + spin_unlock_irqrestore(&usbmisc->lock, flags); >>> + >>> + return 0; >>> +} > > Bit can be set/cleared wrongly by the bootloader, it is not a big > overhead to set it in proper state. > ohh, yes that true! > ... >
> On Monday, November 11, 2013 12:45 PM, Alexander Shiyan wrote: > >> Hi Alexander, > >> > >> On Sunday, November 10, 2013 03:18 PM, Alexander Shiyan wrote: > >>> This adds i.MX27 and i.MX31 as the next user of the usbmisc driver. > >>> > >>> Signed-off-by: Alexander Shiyan<shc_work@mail.ru> > >>> --- > >>> drivers/usb/chipidea/usbmisc_imx.c | 42 ++++++++++++++++++++++++++++++++++++++ > >>> 1 file changed, 42 insertions(+) > >>> > >>> diff --git a/drivers/usb/chipidea/usbmisc_imx.c b/drivers/usb/chipidea/usbmisc_imx.c > > ... > >>> +static int usbmisc_imx27_init(struct imx_usbmisc_data *data) > >>> +{ > >>> + unsigned long flags; > >>> + u32 val; > >>> + > >>> + switch (data->index) { > >>> + case 0: > >>> + val = MX27_OTG_PM_BIT; > >>> + break; > >>> + case 1: > >>> + val = MX27_H1_PM_BIT; > >>> + break; > >>> + case 2: > >>> + val = MX27_H2_PM_BIT; > >>> + break; > >>> + default: > >>> + return -EINVAL; > >>> + }; > >>> + > >> > >> From my understanding this can not work, the usbmisc->base not point into the > >> usb control register (USB_CTRL). Reference manual 30.5.1.1 says > >> BASE + 0x600 > >> you must add the offset to the readl instruction. > > > > Why not work? > > usbotg: usb@10024000 > > usbh1: usb@10024200 > > usbh2: usb@10024400 > > usbmisc: usbmisc@10024600 > > So, offset to USB_CTRL should already be defined by DTS. > > in the usbmisc_imx_probe() the base pointer is loaded from > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > data->base = devm_ioremap_resource(&pdev->dev, res); > > (and I did not see any of_ operations) Yes, and this is an address of usbmisc node, not otg. > usbmisc = data; > > base is set to 0x10024000 > > when I look around all other functions init functions did a offset calculation. Can you point me on this? For example, for i.MX5 CPUs we calculate only offset to PHY_CTRL_X register relative to basic offset 0x800, which is already defined in DTS. ... ---
On Mon, Nov 11, 2013 at 12:45 PM, Alexander Shiyan <shc_work@mail.ru> wrote: >> Hi Alexander, >> >> On Sunday, November 10, 2013 03:18 PM, Alexander Shiyan wrote: >> > This adds i.MX27 and i.MX31 as the next user of the usbmisc driver. >> > >> > Signed-off-by: Alexander Shiyan<shc_work@mail.ru> >> > --- >> > drivers/usb/chipidea/usbmisc_imx.c | 42 ++++++++++++++++++++++++++++++++++++++ >> > 1 file changed, 42 insertions(+) >> > >> > diff --git a/drivers/usb/chipidea/usbmisc_imx.c b/drivers/usb/chipidea/usbmisc_imx.c > ... >> > +static int usbmisc_imx27_init(struct imx_usbmisc_data *data) >> > +{ >> > + unsigned long flags; >> > + u32 val; >> > + >> > + switch (data->index) { >> > + case 0: >> > + val = MX27_OTG_PM_BIT; >> > + break; >> > + case 1: >> > + val = MX27_H1_PM_BIT; >> > + break; >> > + case 2: >> > + val = MX27_H2_PM_BIT; >> > + break; >> > + default: >> > + return -EINVAL; >> > + }; >> > + >> >> From my understanding this can not work, the usbmisc->base not point into the >> usb control register (USB_CTRL). Reference manual 30.5.1.1 says >> BASE + 0x600 >> you must add the offset to the readl instruction. > > Why not work? > usbotg: usb@10024000 > usbh1: usb@10024200 > usbh2: usb@10024400 > usbmisc: usbmisc@10024600 > So, offset to USB_CTRL should already be defined by DTS. > > >> > + spin_lock_irqsave(&usbmisc->lock, flags); >> > + if (data->disable_oc) >> > + val = readl(usbmisc->base) | val; >> >> else part not needed, the registers bits are set to 0 (reset) >> the function is called on start-up once only, right?! >> >> > + else >> > + val = readl(usbmisc->base)& ~val; >> > + writel(val, usbmisc->base); >> > + spin_unlock_irqrestore(&usbmisc->lock, flags); >> > + >> > + return 0; >> > +} > > Bit can be set/cleared wrongly by the bootloader, it is not a big > overhead to set it in proper state. > I think the "else" is needed, unless the over-current is not connected or the user wants to disable it, we need to enable over-current by default. Acked-by: Peter Chen <peter.chen@freescale.com> > ... > > --- > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff --git a/drivers/usb/chipidea/usbmisc_imx.c b/drivers/usb/chipidea/usbmisc_imx.c index 8a1094b..4381c5a6 100644 --- a/drivers/usb/chipidea/usbmisc_imx.c +++ b/drivers/usb/chipidea/usbmisc_imx.c @@ -21,6 +21,10 @@ #define MX25_USB_PHY_CTRL_OFFSET 0x08 #define MX25_BM_EXTERNAL_VBUS_DIVIDER BIT(23) +#define MX27_H1_PM_BIT BIT(8) +#define MX27_H2_PM_BIT BIT(16) +#define MX27_OTG_PM_BIT BIT(24) + #define MX53_USB_OTG_PHY_CTRL_0_OFFSET 0x08 #define MX53_USB_UH2_CTRL_OFFSET 0x14 #define MX53_USB_UH3_CTRL_OFFSET 0x18 @@ -68,6 +72,36 @@ static int usbmisc_imx25_post(struct imx_usbmisc_data *data) return 0; } +static int usbmisc_imx27_init(struct imx_usbmisc_data *data) +{ + unsigned long flags; + u32 val; + + switch (data->index) { + case 0: + val = MX27_OTG_PM_BIT; + break; + case 1: + val = MX27_H1_PM_BIT; + break; + case 2: + val = MX27_H2_PM_BIT; + break; + default: + return -EINVAL; + }; + + spin_lock_irqsave(&usbmisc->lock, flags); + if (data->disable_oc) + val = readl(usbmisc->base) | val; + else + val = readl(usbmisc->base) & ~val; + writel(val, usbmisc->base); + spin_unlock_irqrestore(&usbmisc->lock, flags); + + return 0; +} + static int usbmisc_imx53_init(struct imx_usbmisc_data *data) { void __iomem *reg = NULL; @@ -128,6 +162,10 @@ static const struct usbmisc_ops imx25_usbmisc_ops = { .post = usbmisc_imx25_post, }; +static const struct usbmisc_ops imx27_usbmisc_ops = { + .init = usbmisc_imx27_init, +}; + static const struct usbmisc_ops imx53_usbmisc_ops = { .init = usbmisc_imx53_init, }; @@ -162,6 +200,10 @@ static const struct of_device_id usbmisc_imx_dt_ids[] = { .data = &imx25_usbmisc_ops, }, { + .compatible = "fsl,imx27-usbmisc", + .data = &imx27_usbmisc_ops, + }, + { .compatible = "fsl,imx53-usbmisc", .data = &imx53_usbmisc_ops, },
This adds i.MX27 and i.MX31 as the next user of the usbmisc driver. Signed-off-by: Alexander Shiyan <shc_work@mail.ru> --- drivers/usb/chipidea/usbmisc_imx.c | 42 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+)