| Message ID | 1384067901-9377-2-git-send-email-shc_work@mail.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Nov 10, 2013 at 3:18 PM, Alexander Shiyan <shc_work@mail.ru> wrote: > This adds i.MX51 as the next user of the usbmisc driver. > Since the functional is similar i.MX53, we just rename the > definitions and add an alias for the new CPU. > > Signed-off-by: Alexander Shiyan <shc_work@mail.ru> > --- > drivers/usb/chipidea/usbmisc_imx.c | 40 +++++++++++++++++++++----------------- > 1 file changed, 22 insertions(+), 18 deletions(-) > > diff --git a/drivers/usb/chipidea/usbmisc_imx.c b/drivers/usb/chipidea/usbmisc_imx.c > index 4381c5a6..f348ebb 100644 > --- a/drivers/usb/chipidea/usbmisc_imx.c > +++ b/drivers/usb/chipidea/usbmisc_imx.c > @@ -25,12 +25,12 @@ > #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 > -#define MX53_BM_OVER_CUR_DIS_H1 BIT(5) > -#define MX53_BM_OVER_CUR_DIS_OTG BIT(8) > -#define MX53_BM_OVER_CUR_DIS_UHx BIT(30) > +#define MX5_USB_OTG_PHY_CTRL_0_OFFSET 0x08 > +#define MX5_USB_UH2_CTRL_OFFSET 0x14 > +#define MX5_USB_UH3_CTRL_OFFSET 0x18 > +#define MX5_BM_OVER_CUR_DIS_H1 BIT(5) > +#define MX5_BM_OVER_CUR_DIS_OTG BIT(8) > +#define MX5_BM_OVER_CUR_DIS_UHx BIT(30) > > #define MX6_BM_OVER_CUR_DIS BIT(7) > > @@ -102,7 +102,7 @@ static int usbmisc_imx27_init(struct imx_usbmisc_data *data) > return 0; > } > > -static int usbmisc_imx53_init(struct imx_usbmisc_data *data) > +static int usbmisc_imx5_init(struct imx_usbmisc_data *data) > { > void __iomem *reg = NULL; > unsigned long flags; > @@ -115,20 +115,20 @@ static int usbmisc_imx53_init(struct imx_usbmisc_data *data) > spin_lock_irqsave(&usbmisc->lock, flags); > switch (data->index) { > case 0: > - reg = usbmisc->base + MX53_USB_OTG_PHY_CTRL_0_OFFSET; > - val = readl(reg) | MX53_BM_OVER_CUR_DIS_OTG; > + reg = usbmisc->base + MX5_USB_OTG_PHY_CTRL_0_OFFSET; > + val = readl(reg) | MX5_BM_OVER_CUR_DIS_OTG; > break; > case 1: > - reg = usbmisc->base + MX53_USB_OTG_PHY_CTRL_0_OFFSET; > - val = readl(reg) | MX53_BM_OVER_CUR_DIS_H1; > + reg = usbmisc->base + MX5_USB_OTG_PHY_CTRL_0_OFFSET; > + val = readl(reg) | MX5_BM_OVER_CUR_DIS_H1; > break; > case 2: > - reg = usbmisc->base + MX53_USB_UH2_CTRL_OFFSET; > - val = readl(reg) | MX53_BM_OVER_CUR_DIS_UHx; > + reg = usbmisc->base + MX5_USB_UH2_CTRL_OFFSET; > + val = readl(reg) | MX5_BM_OVER_CUR_DIS_UHx; > break; > case 3: > - reg = usbmisc->base + MX53_USB_UH3_CTRL_OFFSET; > - val = readl(reg) | MX53_BM_OVER_CUR_DIS_UHx; > + reg = usbmisc->base + MX5_USB_UH3_CTRL_OFFSET; > + val = readl(reg) | MX5_BM_OVER_CUR_DIS_UHx; > break; > } > if (reg && val) > @@ -166,8 +166,8 @@ static const struct usbmisc_ops imx27_usbmisc_ops = { > .init = usbmisc_imx27_init, > }; > > -static const struct usbmisc_ops imx53_usbmisc_ops = { > - .init = usbmisc_imx53_init, > +static const struct usbmisc_ops imx5_usbmisc_ops = { > + .init = usbmisc_imx5_init, > }; > > static const struct usbmisc_ops imx6q_usbmisc_ops = { > @@ -204,8 +204,12 @@ static const struct of_device_id usbmisc_imx_dt_ids[] = { > .data = &imx27_usbmisc_ops, > }, > { > + .compatible = "fsl,imx51-usbmisc", > + .data = &imx5_usbmisc_ops, > + }, > + { > .compatible = "fsl,imx53-usbmisc", > - .data = &imx53_usbmisc_ops, > + .data = &imx5_usbmisc_ops, > }, > { > .compatible = "fsl,imx6q-usbmisc", > -- Acked-by: Peter Chen <peter.chen@freescale.com>
I'd prefer this was kept separated out a little bit, as follows: On Sun, Nov 10, 2013 at 1:18 AM, Alexander Shiyan <shc_work@mail.ru> wrote: > This adds i.MX51 as the next user of the usbmisc driver. > Since the functional is similar i.MX53, we just rename the > definitions and add an alias for the new CPU. > > Signed-off-by: Alexander Shiyan <shc_work@mail.ru> > --- > drivers/usb/chipidea/usbmisc_imx.c | 40 +++++++++++++++++++++----------------- > 1 file changed, 22 insertions(+), 18 deletions(-) > > > -static int usbmisc_imx53_init(struct imx_usbmisc_data *data) > +static int usbmisc_imx5_init(struct imx_usbmisc_data *data) Can we keep usbmisc_imx53_init named this? There is more to do here on i.MX5 for cases where the bootloader did not already try to set up USB (or did it badly) such as the transceiver reference clock rate and also setting the USB sysclk clock source (internal DPLL or from external transceiver in the external ULPI case) and so on, which is related and needs doing on both, but is different on i.MX51 than i.MX53. This is information sort of best passed in the PHY node that goes along with this, but it's set within the usbmisc block of the chips so the usbmisc driver will have a responsibility to go see if it's an external PHY that is feeding it's clock back into the USB block in this way. I am not sure we (Peter etc.) discussed how best to do this, the code to pull the correct information out always seems kind of misplaced no matter where it goes, but the responsibility for tweaking those registers is most certainly this driver. Essentially the layout of usbmisc->base + 0x10 register (USB_CTRL_1) is different when doing the above, and dependent on a board-specific option for the input clock to the transceiver. We could reduce a little churn, later, when usbmisc_imx could be given related usbphy information and actually do the right thing. I have a patch kinda sitting in the wings to do this.. and two *real* pieces of consumer hardware that need it, and some other kicking, to make USB work in the never-touched-before-Linux case. > -static const struct usbmisc_ops imx53_usbmisc_ops = { > - .init = usbmisc_imx53_init, > +static const struct usbmisc_ops imx5_usbmisc_ops = { > + .init = usbmisc_imx5_init, > }; And keep imx53_usbmisc_ops named this? > static const struct usbmisc_ops imx6q_usbmisc_ops = { > @@ -204,8 +204,12 @@ static const struct of_device_id usbmisc_imx_dt_ids[] = { > .data = &imx27_usbmisc_ops, > }, > { > + .compatible = "fsl,imx51-usbmisc", > + .data = &imx5_usbmisc_ops, And then just use &imx53_usbmisc_ops? This gives us some breathing room later to actually do the right thing without additionally performing renames all over the place to make imx5 -> imx53 (again)/imx51 (new). Thanks, Matt Sealey <neko@bakuhatsu.net>
> > > > Signed-off-by: Alexander Shiyan <shc_work@mail.ru> > > --- > > drivers/usb/chipidea/usbmisc_imx.c | 40 +++++++++++++++++++++--------- > -------- > > 1 file changed, 22 insertions(+), 18 deletions(-) > > > > > > -static int usbmisc_imx53_init(struct imx_usbmisc_data *data) > > +static int usbmisc_imx5_init(struct imx_usbmisc_data *data) > > Can we keep usbmisc_imx53_init named this? > > There is more to do here on i.MX5 for cases where the bootloader did > not already try to set up USB (or did it badly) such as the > transceiver reference clock rate and also setting the USB sysclk clock > source (internal DPLL or from external transceiver in the external > ULPI case) and so on, which is related and needs doing on both, but is > different on i.MX51 than i.MX53. This is information sort of best > passed in the PHY node that goes along with this, but it's set within > the usbmisc block of the chips so the usbmisc driver will have a > responsibility to go see if it's an external PHY that is feeding it's > clock back into the USB block in this way. > > I am not sure we (Peter etc.) discussed how best to do this, the code > to pull the correct information out always seems kind of misplaced no > matter where it goes, but the responsibility for tweaking those > registers is most certainly this driver. > > Essentially the layout of usbmisc->base + 0x10 register (USB_CTRL_1) > is different when doing the above, and dependent on a board-specific > option for the input clock to the transceiver. We could reduce a > little churn, later, when usbmisc_imx could be given related usbphy > information and actually do the right thing. I have a patch kinda > sitting in the wings to do this.. and two *real* pieces of consumer > hardware that need it, and some other kicking, to make USB work in the > never-touched-before-Linux case. > > > -static const struct usbmisc_ops imx53_usbmisc_ops = { > > - .init = usbmisc_imx53_init, > > +static const struct usbmisc_ops imx5_usbmisc_ops = { > > + .init = usbmisc_imx5_init, > > }; > > And keep imx53_usbmisc_ops named this? > > > static const struct usbmisc_ops imx6q_usbmisc_ops = { > > @@ -204,8 +204,12 @@ static const struct of_device_id > usbmisc_imx_dt_ids[] = { > > .data = &imx27_usbmisc_ops, > > }, > > { > > + .compatible = "fsl,imx51-usbmisc", > > + .data = &imx5_usbmisc_ops, > > And then just use &imx53_usbmisc_ops? > > This gives us some breathing room later to actually do the right thing > without additionally performing renames all over the place to make > imx5 -> imx53 (again)/imx51 (new). Hi Alexander, You may take matt's suggestion, it can reduce the code change now and in future. We can only add device_id for imx51 in this patch, split imx53 and imx51's ops when their differences are added in future. Peter
Hello. > > > Signed-off-by: Alexander Shiyan <shc_work@mail.ru> > > > --- > > > drivers/usb/chipidea/usbmisc_imx.c | 40 +++++++++++++++++++++--------- > > -------- > > > 1 file changed, 22 insertions(+), 18 deletions(-) > > > > > > > > > -static int usbmisc_imx53_init(struct imx_usbmisc_data *data) > > > +static int usbmisc_imx5_init(struct imx_usbmisc_data *data) > > > > Can we keep usbmisc_imx53_init named this? > > > > There is more to do here on i.MX5 for cases where the bootloader did > > not already try to set up USB (or did it badly) such as the > > transceiver reference clock rate and also setting the USB sysclk clock > > source (internal DPLL or from external transceiver in the external > > ULPI case) and so on, which is related and needs doing on both, but is > > different on i.MX51 than i.MX53. This is information sort of best > > passed in the PHY node that goes along with this, but it's set within > > the usbmisc block of the chips so the usbmisc driver will have a > > responsibility to go see if it's an external PHY that is feeding it's > > clock back into the USB block in this way. > > > > I am not sure we (Peter etc.) discussed how best to do this, the code > > to pull the correct information out always seems kind of misplaced no > > matter where it goes, but the responsibility for tweaking those > > registers is most certainly this driver. > > > > Essentially the layout of usbmisc->base + 0x10 register (USB_CTRL_1) > > is different when doing the above, and dependent on a board-specific > > option for the input clock to the transceiver. We could reduce a > > little churn, later, when usbmisc_imx could be given related usbphy > > information and actually do the right thing. I have a patch kinda > > sitting in the wings to do this.. and two *real* pieces of consumer > > hardware that need it, and some other kicking, to make USB work in the > > never-touched-before-Linux case. > > > > > -static const struct usbmisc_ops imx53_usbmisc_ops = { > > > - .init = usbmisc_imx53_init, > > > +static const struct usbmisc_ops imx5_usbmisc_ops = { > > > + .init = usbmisc_imx5_init, > > > }; > > > > And keep imx53_usbmisc_ops named this? > > > > > static const struct usbmisc_ops imx6q_usbmisc_ops = { > > > @@ -204,8 +204,12 @@ static const struct of_device_id > > usbmisc_imx_dt_ids[] = { > > > .data = &imx27_usbmisc_ops, > > > }, > > > { > > > + .compatible = "fsl,imx51-usbmisc", > > > + .data = &imx5_usbmisc_ops, > > > > And then just use &imx53_usbmisc_ops? > > > > This gives us some breathing room later to actually do the right thing > > without additionally performing renames all over the place to make > > imx5 -> imx53 (again)/imx51 (new). > > You may take matt's suggestion, it can reduce the code change now and in future. > We can only add device_id for imx51 in this patch, split imx53 and imx51's ops > when their differences are added in future. OK. Names of registers and bits also keep as is? ---
> > > This gives us some breathing room later to actually do the right > thing > > > without additionally performing renames all over the place to make > > > imx5 -> imx53 (again)/imx51 (new). > > > > You may take matt's suggestion, it can reduce the code change now and > in future. > > We can only add device_id for imx51 in this patch, split imx53 and > imx51's ops > > when their differences are added in future. > > OK. Names of registers and bits also keep as is? > Yes. Peter
diff --git a/drivers/usb/chipidea/usbmisc_imx.c b/drivers/usb/chipidea/usbmisc_imx.c index 4381c5a6..f348ebb 100644 --- a/drivers/usb/chipidea/usbmisc_imx.c +++ b/drivers/usb/chipidea/usbmisc_imx.c @@ -25,12 +25,12 @@ #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 -#define MX53_BM_OVER_CUR_DIS_H1 BIT(5) -#define MX53_BM_OVER_CUR_DIS_OTG BIT(8) -#define MX53_BM_OVER_CUR_DIS_UHx BIT(30) +#define MX5_USB_OTG_PHY_CTRL_0_OFFSET 0x08 +#define MX5_USB_UH2_CTRL_OFFSET 0x14 +#define MX5_USB_UH3_CTRL_OFFSET 0x18 +#define MX5_BM_OVER_CUR_DIS_H1 BIT(5) +#define MX5_BM_OVER_CUR_DIS_OTG BIT(8) +#define MX5_BM_OVER_CUR_DIS_UHx BIT(30) #define MX6_BM_OVER_CUR_DIS BIT(7) @@ -102,7 +102,7 @@ static int usbmisc_imx27_init(struct imx_usbmisc_data *data) return 0; } -static int usbmisc_imx53_init(struct imx_usbmisc_data *data) +static int usbmisc_imx5_init(struct imx_usbmisc_data *data) { void __iomem *reg = NULL; unsigned long flags; @@ -115,20 +115,20 @@ static int usbmisc_imx53_init(struct imx_usbmisc_data *data) spin_lock_irqsave(&usbmisc->lock, flags); switch (data->index) { case 0: - reg = usbmisc->base + MX53_USB_OTG_PHY_CTRL_0_OFFSET; - val = readl(reg) | MX53_BM_OVER_CUR_DIS_OTG; + reg = usbmisc->base + MX5_USB_OTG_PHY_CTRL_0_OFFSET; + val = readl(reg) | MX5_BM_OVER_CUR_DIS_OTG; break; case 1: - reg = usbmisc->base + MX53_USB_OTG_PHY_CTRL_0_OFFSET; - val = readl(reg) | MX53_BM_OVER_CUR_DIS_H1; + reg = usbmisc->base + MX5_USB_OTG_PHY_CTRL_0_OFFSET; + val = readl(reg) | MX5_BM_OVER_CUR_DIS_H1; break; case 2: - reg = usbmisc->base + MX53_USB_UH2_CTRL_OFFSET; - val = readl(reg) | MX53_BM_OVER_CUR_DIS_UHx; + reg = usbmisc->base + MX5_USB_UH2_CTRL_OFFSET; + val = readl(reg) | MX5_BM_OVER_CUR_DIS_UHx; break; case 3: - reg = usbmisc->base + MX53_USB_UH3_CTRL_OFFSET; - val = readl(reg) | MX53_BM_OVER_CUR_DIS_UHx; + reg = usbmisc->base + MX5_USB_UH3_CTRL_OFFSET; + val = readl(reg) | MX5_BM_OVER_CUR_DIS_UHx; break; } if (reg && val) @@ -166,8 +166,8 @@ static const struct usbmisc_ops imx27_usbmisc_ops = { .init = usbmisc_imx27_init, }; -static const struct usbmisc_ops imx53_usbmisc_ops = { - .init = usbmisc_imx53_init, +static const struct usbmisc_ops imx5_usbmisc_ops = { + .init = usbmisc_imx5_init, }; static const struct usbmisc_ops imx6q_usbmisc_ops = { @@ -204,8 +204,12 @@ static const struct of_device_id usbmisc_imx_dt_ids[] = { .data = &imx27_usbmisc_ops, }, { + .compatible = "fsl,imx51-usbmisc", + .data = &imx5_usbmisc_ops, + }, + { .compatible = "fsl,imx53-usbmisc", - .data = &imx53_usbmisc_ops, + .data = &imx5_usbmisc_ops, }, { .compatible = "fsl,imx6q-usbmisc",
This adds i.MX51 as the next user of the usbmisc driver. Since the functional is similar i.MX53, we just rename the definitions and add an alias for the new CPU. Signed-off-by: Alexander Shiyan <shc_work@mail.ru> --- drivers/usb/chipidea/usbmisc_imx.c | 40 +++++++++++++++++++++----------------- 1 file changed, 22 insertions(+), 18 deletions(-)