[PATCH v3 1/5] usb: ehci: add freescale imx28 special write register method

[PATCH v3 1/5] usb: ehci: add freescale imx28 special write register method

[Peter Chen]

According to Freescale imx28 Errata, "ENGR119653 USB: ARM to USB
register error issue", All USB register write operations must
use the ARM SWP instruction. So, we implement a special ehci_write
for imx28.

Discussion for it at below:
http://marc.info/?l=linux-usb&m=137996395529294&w=2

Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
 drivers/usb/host/ehci.h |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
index e8f41c5..535aa8b 100644
--- a/drivers/usb/host/ehci.h
+++ b/drivers/usb/host/ehci.h
@@ -225,6 +225,7 @@ struct ehci_hcd {			/* one per controller */
 	unsigned		has_synopsys_hc_bug:1; /* Synopsys HC */
 	unsigned		frame_index_bug:1; /* MosChip (AKA NetMos) */
 	unsigned		need_oc_pp_cycle:1; /* MPC834X port power */
+	unsigned		imx28_write_fix:1; /* For Freescale i.MX28 */
 
 	/* required for usb32 quirk */
 	#define OHCI_CTRL_HCFS          (3 << 6)
@@ -728,6 +729,13 @@ static inline unsigned int ehci_readl(const struct ehci_hcd *ehci,
 #endif
 }
 
+#ifdef CONFIG_SOC_IMX28
+static inline void imx28_ehci_writel(u32 val32, volatile u32 *addr)
+{
+	__asm__ ("swp %0, %0, [%1]" : : "r"(val32), "r"(addr));
+}
+#endif
+
 static inline void ehci_writel(const struct ehci_hcd *ehci,
 		const unsigned int val, __u32 __iomem *regs)
 {
@@ -735,6 +743,11 @@ static inline void ehci_writel(const struct ehci_hcd *ehci,
 	ehci_big_endian_mmio(ehci) ?
 		writel_be(val, regs) :
 		writel(val, regs);
+#elif defined(CONFIG_SOC_IMX28)
+	if (ehci->imx28_write_fix)
+		imx28_ehci_writel(val, regs);
+	else
+		writel(val, regs);
 #else
 	writel(val, regs);
 #endif
-- 
1.7.1

[PATCH v3 2/5] usb: chipidea: add freescale imx28 special write register method

[Peter Chen]

According to Freescale imx28 Errata, "ENGR119653 USB: ARM to USB
register error issue", All USB register write operations must
use the ARM SWP instruction. So, we implement special hw_write
and hw_test_and_clear for imx28.

Discussion for it at below:
http://marc.info/?l=linux-usb&m=137996395529294&w=2

Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
Changes for v2:
- Rebase to latest usb-next tree

 drivers/usb/chipidea/ci.h    |   23 +++++++++++++++++++++++
 drivers/usb/chipidea/core.c  |    2 ++
 drivers/usb/chipidea/host.c  |    1 +
 include/linux/usb/chipidea.h |    1 +
 4 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
index 1c94fc5..4eb61d0 100644
--- a/drivers/usb/chipidea/ci.h
+++ b/drivers/usb/chipidea/ci.h
@@ -173,6 +173,8 @@ struct ci_hdrc {
 	struct dentry			*debugfs;
 	bool				id_event;
 	bool				b_sess_valid_event;
+	/* imx28 needs swp instruction for writing */
+	bool				imx28_write_fix;
 };
 
 static inline struct ci_role_driver *ci_role(struct ci_hdrc *ci)
@@ -253,6 +255,13 @@ static inline u32 hw_read(struct ci_hdrc *ci, enum ci_hw_regs reg, u32 mask)
 	return ioread32(ci->hw_bank.regmap[reg]) & mask;
 }
 
+#ifdef CONFIG_SOC_IMX28
+static inline void imx28_ci_writel(u32 val32, volatile u32 *addr)
+{
+	__asm__ ("swp %0, %0, [%1]" : : "r"(val32), "r"(addr));
+}
+#endif
+
 /**
  * hw_write: writes to a hw register
  * @reg:  register index
@@ -266,7 +275,14 @@ static inline void hw_write(struct ci_hdrc *ci, enum ci_hw_regs reg,
 		data = (ioread32(ci->hw_bank.regmap[reg]) & ~mask)
 			| (data & mask);
 
+#ifdef CONFIG_SOC_IMX28
+	if (ci->imx28_write_fix)
+		imx28_ci_writel(data, ci->hw_bank.regmap[reg]);
+	else
+		iowrite32(data, ci->hw_bank.regmap[reg]);
+#else
 	iowrite32(data, ci->hw_bank.regmap[reg]);
+#endif
 }
 
 /**
@@ -281,7 +297,14 @@ static inline u32 hw_test_and_clear(struct ci_hdrc *ci, enum ci_hw_regs reg,
 {
 	u32 val = ioread32(ci->hw_bank.regmap[reg]) & mask;
 
+#ifdef CONFIG_SOC_IMX28
+	if (ci->imx28_write_fix)
+		imx28_ci_writel(val, ci->hw_bank.regmap[reg]);
+	else
+		iowrite32(val, ci->hw_bank.regmap[reg]);
+#else
 	iowrite32(val, ci->hw_bank.regmap[reg]);
+#endif
 	return val;
 }
 
diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index 06204b7..357a059 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -551,6 +551,8 @@ static int ci_hdrc_probe(struct platform_device *pdev)
 
 	ci->dev = dev;
 	ci->platdata = dev->platform_data;
+	ci->imx28_write_fix = !!(ci->platdata->flags &
+		CI_HDRC_IMX28_WRITE_FIX);
 
 	ret = hw_device_init(ci, base);
 	if (ret < 0) {
diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
index 59e6020..06fd042 100644
--- a/drivers/usb/chipidea/host.c
+++ b/drivers/usb/chipidea/host.c
@@ -65,6 +65,7 @@ static int host_start(struct ci_hdrc *ci)
 	ehci->caps = ci->hw_bank.cap;
 	ehci->has_hostpc = ci->hw_bank.lpm;
 	ehci->has_tdi_phy_lpm = ci->hw_bank.lpm;
+	ehci->imx28_write_fix = ci->imx28_write_fix;
 
 	if (ci->platdata->reg_vbus) {
 		ret = regulator_enable(ci->platdata->reg_vbus);
diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h
index 7d39967..708bd11 100644
--- a/include/linux/usb/chipidea.h
+++ b/include/linux/usb/chipidea.h
@@ -24,6 +24,7 @@ struct ci_hdrc_platform_data {
 	 * but otg is not supported (no register otgsc).
 	 */
 #define CI_HDRC_DUAL_ROLE_NOT_OTG	BIT(4)
+#define CI_HDRC_IMX28_WRITE_FIX		BIT(5)
 	enum usb_dr_mode	dr_mode;
 #define CI_HDRC_CONTROLLER_RESET_EVENT		0
 #define CI_HDRC_CONTROLLER_STOPPED_EVENT	1
-- 
1.7.1

[PATCH v3 2/5] usb: chipidea: add freescale imx28 special write register method

[Hector Palacios]

Dear Peter,

On 10/25/2013 08:02 AM, Peter Chen wrote:
> According to Freescale imx28 Errata, "ENGR119653 USB: ARM to USB
> register error issue", All USB register write operations must
> use the ARM SWP instruction. So, we implement special hw_write
> and hw_test_and_clear for imx28.
>
> Discussion for it at below:
> http://marc.info/?l=linux-usb&m=137996395529294&w=2
>
> Signed-off-by: Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> ---
> Changes for v2:
> - Rebase to latest usb-next tree
>
>   drivers/usb/chipidea/ci.h    |   23 +++++++++++++++++++++++
>   drivers/usb/chipidea/core.c  |    2 ++
>   drivers/usb/chipidea/host.c  |    1 +
>   include/linux/usb/chipidea.h |    1 +
>   4 files changed, 27 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
> index 1c94fc5..4eb61d0 100644
> --- a/drivers/usb/chipidea/ci.h
> +++ b/drivers/usb/chipidea/ci.h
> @@ -173,6 +173,8 @@ struct ci_hdrc {
>   	struct dentry			*debugfs;
>   	bool				id_event;
>   	bool				b_sess_valid_event;
> +	/* imx28 needs swp instruction for writing */
> +	bool				imx28_write_fix;
>   };
>
>   static inline struct ci_role_driver *ci_role(struct ci_hdrc *ci)
> @@ -253,6 +255,13 @@ static inline u32 hw_read(struct ci_hdrc *ci, enum ci_hw_regs reg, u32 mask)
>   	return ioread32(ci->hw_bank.regmap[reg]) & mask;
>   }
>
> +#ifdef CONFIG_SOC_IMX28
> +static inline void imx28_ci_writel(u32 val32, volatile u32 *addr)
> +{
> +	__asm__ ("swp %0, %0, [%1]" : : "r"(val32), "r"(addr));
> +}
> +#endif
> +
>   /**
>    * hw_write: writes to a hw register
>    * @reg:  register index
> @@ -266,7 +275,14 @@ static inline void hw_write(struct ci_hdrc *ci, enum ci_hw_regs reg,
>   		data = (ioread32(ci->hw_bank.regmap[reg]) & ~mask)
>   			| (data & mask);
>
> +#ifdef CONFIG_SOC_IMX28
> +	if (ci->imx28_write_fix)
> +		imx28_ci_writel(data, ci->hw_bank.regmap[reg]);
> +	else
> +		iowrite32(data, ci->hw_bank.regmap[reg]);
> +#else
>   	iowrite32(data, ci->hw_bank.regmap[reg]);
> +#endif
>   }
>
>   /**
> @@ -281,7 +297,14 @@ static inline u32 hw_test_and_clear(struct ci_hdrc *ci, enum ci_hw_regs reg,
>   {
>   	u32 val = ioread32(ci->hw_bank.regmap[reg]) & mask;
>
> +#ifdef CONFIG_SOC_IMX28
> +	if (ci->imx28_write_fix)
> +		imx28_ci_writel(val, ci->hw_bank.regmap[reg]);
> +	else
> +		iowrite32(val, ci->hw_bank.regmap[reg]);
> +#else
>   	iowrite32(val, ci->hw_bank.regmap[reg]);
> +#endif
>   	return val;
>   }

Can't we remove the #ifdefs CONFIG_SOC_IMX28 all around?
The check is done on the new flag ci->imx28_write_fix which exists for all platforms, 
isn't it?.

Best regards,
-- 
H?ctor Palacios

[PATCH v3 2/5] usb: chipidea: add freescale imx28 special write register method

[Marek Vasut]

Dear Hector Palacios,

> Dear Peter,
> 
> On 10/25/2013 08:02 AM, Peter Chen wrote:
> > According to Freescale imx28 Errata, "ENGR119653 USB: ARM to USB
> > register error issue", All USB register write operations must
> > use the ARM SWP instruction. So, we implement special hw_write
> > and hw_test_and_clear for imx28.
> > 
> > Discussion for it at below:
> > http://marc.info/?l=linux-usb&m=137996395529294&w=2
> > 
> > Signed-off-by: Peter Chen
> > <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org> ---
> > Changes for v2:
> > - Rebase to latest usb-next tree
> > 
> >   drivers/usb/chipidea/ci.h    |   23 +++++++++++++++++++++++
> >   drivers/usb/chipidea/core.c  |    2 ++
> >   drivers/usb/chipidea/host.c  |    1 +
> >   include/linux/usb/chipidea.h |    1 +
> >   4 files changed, 27 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
> > index 1c94fc5..4eb61d0 100644
> > --- a/drivers/usb/chipidea/ci.h
> > +++ b/drivers/usb/chipidea/ci.h
> > @@ -173,6 +173,8 @@ struct ci_hdrc {
> > 
> >   	struct dentry			*debugfs;
> >   	bool				id_event;
> >   	bool				b_sess_valid_event;
> > 
> > +	/* imx28 needs swp instruction for writing */
> > +	bool				imx28_write_fix;
> > 
> >   };
> >   
> >   static inline struct ci_role_driver *ci_role(struct ci_hdrc *ci)
> > 
> > @@ -253,6 +255,13 @@ static inline u32 hw_read(struct ci_hdrc *ci, enum
> > ci_hw_regs reg, u32 mask)
> > 
> >   	return ioread32(ci->hw_bank.regmap[reg]) & mask;
> >   
> >   }
> > 
> > +#ifdef CONFIG_SOC_IMX28
> > +static inline void imx28_ci_writel(u32 val32, volatile u32 *addr)
> > +{
> > +	__asm__ ("swp %0, %0, [%1]" : : "r"(val32), "r"(addr));
> > +}
> > +#endif
> > +
> > 
> >   /**
> >   
> >    * hw_write: writes to a hw register
> >    * @reg:  register index
> > 
> > @@ -266,7 +275,14 @@ static inline void hw_write(struct ci_hdrc *ci, enum
> > ci_hw_regs reg,
> > 
> >   		data = (ioread32(ci->hw_bank.regmap[reg]) & ~mask)
> >   		
> >   			| (data & mask);
> > 
> > +#ifdef CONFIG_SOC_IMX28
> > +	if (ci->imx28_write_fix)
> > +		imx28_ci_writel(data, ci->hw_bank.regmap[reg]);
> > +	else
> > +		iowrite32(data, ci->hw_bank.regmap[reg]);
> > +#else
> > 
> >   	iowrite32(data, ci->hw_bank.regmap[reg]);
> > 
> > +#endif
> > 
> >   }
> >   
> >   /**
> > 
> > @@ -281,7 +297,14 @@ static inline u32 hw_test_and_clear(struct ci_hdrc
> > *ci, enum ci_hw_regs reg,
> > 
> >   {
> >   
> >   	u32 val = ioread32(ci->hw_bank.regmap[reg]) & mask;
> > 
> > +#ifdef CONFIG_SOC_IMX28
> > +	if (ci->imx28_write_fix)
> > +		imx28_ci_writel(val, ci->hw_bank.regmap[reg]);
> > +	else
> > +		iowrite32(val, ci->hw_bank.regmap[reg]);
> > +#else
> > 
> >   	iowrite32(val, ci->hw_bank.regmap[reg]);
> > 
> > +#endif
> > 
> >   	return val;
> >   
> >   }
> 
> Can't we remove the #ifdefs CONFIG_SOC_IMX28 all around?
> The check is done on the new flag ci->imx28_write_fix which exists for all
> platforms, isn't it?.

The SWP instruction is specific to ARM, so you'd need to stub-out the 
imx28_ci_writel() with ifdef then.

[PATCH v3 2/5] usb: chipidea: add freescale imx28 special write register method

[gregkh at linuxfoundation.org]

On Mon, Oct 28, 2013 at 11:47:53AM +0100, Marek Vasut wrote:
> Dear Hector Palacios,
> 
> > Dear Peter,
> > 
> > On 10/25/2013 08:02 AM, Peter Chen wrote:
> > > According to Freescale imx28 Errata, "ENGR119653 USB: ARM to USB
> > > register error issue", All USB register write operations must
> > > use the ARM SWP instruction. So, we implement special hw_write
> > > and hw_test_and_clear for imx28.
> > > 
> > > Discussion for it at below:
> > > http://marc.info/?l=linux-usb&m=137996395529294&w=2
> > > 
> > > Signed-off-by: Peter Chen
> > > <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org> ---
> > > Changes for v2:
> > > - Rebase to latest usb-next tree
> > > 
> > >   drivers/usb/chipidea/ci.h    |   23 +++++++++++++++++++++++
> > >   drivers/usb/chipidea/core.c  |    2 ++
> > >   drivers/usb/chipidea/host.c  |    1 +
> > >   include/linux/usb/chipidea.h |    1 +
> > >   4 files changed, 27 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
> > > index 1c94fc5..4eb61d0 100644
> > > --- a/drivers/usb/chipidea/ci.h
> > > +++ b/drivers/usb/chipidea/ci.h
> > > @@ -173,6 +173,8 @@ struct ci_hdrc {
> > > 
> > >   	struct dentry			*debugfs;
> > >   	bool				id_event;
> > >   	bool				b_sess_valid_event;
> > > 
> > > +	/* imx28 needs swp instruction for writing */
> > > +	bool				imx28_write_fix;
> > > 
> > >   };
> > >   
> > >   static inline struct ci_role_driver *ci_role(struct ci_hdrc *ci)
> > > 
> > > @@ -253,6 +255,13 @@ static inline u32 hw_read(struct ci_hdrc *ci, enum
> > > ci_hw_regs reg, u32 mask)
> > > 
> > >   	return ioread32(ci->hw_bank.regmap[reg]) & mask;
> > >   
> > >   }
> > > 
> > > +#ifdef CONFIG_SOC_IMX28
> > > +static inline void imx28_ci_writel(u32 val32, volatile u32 *addr)
> > > +{
> > > +	__asm__ ("swp %0, %0, [%1]" : : "r"(val32), "r"(addr));
> > > +}
> > > +#endif
> > > +
> > > 
> > >   /**
> > >   
> > >    * hw_write: writes to a hw register
> > >    * @reg:  register index
> > > 
> > > @@ -266,7 +275,14 @@ static inline void hw_write(struct ci_hdrc *ci, enum
> > > ci_hw_regs reg,
> > > 
> > >   		data = (ioread32(ci->hw_bank.regmap[reg]) & ~mask)
> > >   		
> > >   			| (data & mask);
> > > 
> > > +#ifdef CONFIG_SOC_IMX28
> > > +	if (ci->imx28_write_fix)
> > > +		imx28_ci_writel(data, ci->hw_bank.regmap[reg]);
> > > +	else
> > > +		iowrite32(data, ci->hw_bank.regmap[reg]);
> > > +#else
> > > 
> > >   	iowrite32(data, ci->hw_bank.regmap[reg]);
> > > 
> > > +#endif
> > > 
> > >   }
> > >   
> > >   /**
> > > 
> > > @@ -281,7 +297,14 @@ static inline u32 hw_test_and_clear(struct ci_hdrc
> > > *ci, enum ci_hw_regs reg,
> > > 
> > >   {
> > >   
> > >   	u32 val = ioread32(ci->hw_bank.regmap[reg]) & mask;
> > > 
> > > +#ifdef CONFIG_SOC_IMX28
> > > +	if (ci->imx28_write_fix)
> > > +		imx28_ci_writel(val, ci->hw_bank.regmap[reg]);
> > > +	else
> > > +		iowrite32(val, ci->hw_bank.regmap[reg]);
> > > +#else
> > > 
> > >   	iowrite32(val, ci->hw_bank.regmap[reg]);
> > > 
> > > +#endif
> > > 
> > >   	return val;
> > >   
> > >   }
> > 
> > Can't we remove the #ifdefs CONFIG_SOC_IMX28 all around?
> > The check is done on the new flag ci->imx28_write_fix which exists for all
> > platforms, isn't it?.
> 
> The SWP instruction is specific to ARM, so you'd need to stub-out the 
> imx28_ci_writel() with ifdef then.

That's better than the mess of #ifdefs this patch adds, which isn't ok
at all :(

[PATCH v3 2/5] usb: chipidea: add freescale imx28 special write register method

[Peter Chen]

On Tue, Oct 29, 2013 at 04:54:47PM -0700, gregkh at linuxfoundation.org wrote:
> On Mon, Oct 28, 2013 at 11:47:53AM +0100, Marek Vasut wrote:
> > Dear Hector Palacios,
> > 
> > > Dear Peter,
> > > 
> > > On 10/25/2013 08:02 AM, Peter Chen wrote:
> > > > According to Freescale imx28 Errata, "ENGR119653 USB: ARM to USB
> > > > register error issue", All USB register write operations must
> > > > use the ARM SWP instruction. So, we implement special hw_write
> > > > and hw_test_and_clear for imx28.
> > > > 
> > > > Discussion for it at below:
> > > > http://marc.info/?l=linux-usb&m=137996395529294&w=2
> > > > 
> > > > Signed-off-by: Peter Chen
> > > > <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org> ---
> > > > Changes for v2:
> > > > - Rebase to latest usb-next tree
> > > > 
> > > >   drivers/usb/chipidea/ci.h    |   23 +++++++++++++++++++++++
> > > >   drivers/usb/chipidea/core.c  |    2 ++
> > > >   drivers/usb/chipidea/host.c  |    1 +
> > > >   include/linux/usb/chipidea.h |    1 +
> > > >   4 files changed, 27 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
> > > > index 1c94fc5..4eb61d0 100644
> > > > --- a/drivers/usb/chipidea/ci.h
> > > > +++ b/drivers/usb/chipidea/ci.h
> > > > @@ -173,6 +173,8 @@ struct ci_hdrc {
> > > > 
> > > >   	struct dentry			*debugfs;
> > > >   	bool				id_event;
> > > >   	bool				b_sess_valid_event;
> > > > 
> > > > +	/* imx28 needs swp instruction for writing */
> > > > +	bool				imx28_write_fix;
> > > > 
> > > >   };
> > > >   
> > > >   static inline struct ci_role_driver *ci_role(struct ci_hdrc *ci)
> > > > 
> > > > @@ -253,6 +255,13 @@ static inline u32 hw_read(struct ci_hdrc *ci, enum
> > > > ci_hw_regs reg, u32 mask)
> > > > 
> > > >   	return ioread32(ci->hw_bank.regmap[reg]) & mask;
> > > >   
> > > >   }
> > > > 
> > > > +#ifdef CONFIG_SOC_IMX28
> > > > +static inline void imx28_ci_writel(u32 val32, volatile u32 *addr)
> > > > +{
> > > > +	__asm__ ("swp %0, %0, [%1]" : : "r"(val32), "r"(addr));
> > > > +}
> > > > +#endif
> > > > +
> > > > 
> > > >   /**
> > > >   
> > > >    * hw_write: writes to a hw register
> > > >    * @reg:  register index
> > > > 
> > > > @@ -266,7 +275,14 @@ static inline void hw_write(struct ci_hdrc *ci, enum
> > > > ci_hw_regs reg,
> > > > 
> > > >   		data = (ioread32(ci->hw_bank.regmap[reg]) & ~mask)
> > > >   		
> > > >   			| (data & mask);
> > > > 
> > > > +#ifdef CONFIG_SOC_IMX28
> > > > +	if (ci->imx28_write_fix)
> > > > +		imx28_ci_writel(data, ci->hw_bank.regmap[reg]);
> > > > +	else
> > > > +		iowrite32(data, ci->hw_bank.regmap[reg]);
> > > > +#else
> > > > 
> > > >   	iowrite32(data, ci->hw_bank.regmap[reg]);
> > > > 
> > > > +#endif
> > > > 
> > > >   }
> > > >   
> > > >   /**
> > > > 
> > > > @@ -281,7 +297,14 @@ static inline u32 hw_test_and_clear(struct ci_hdrc
> > > > *ci, enum ci_hw_regs reg,
> > > > 
> > > >   {
> > > >   
> > > >   	u32 val = ioread32(ci->hw_bank.regmap[reg]) & mask;
> > > > 
> > > > +#ifdef CONFIG_SOC_IMX28
> > > > +	if (ci->imx28_write_fix)
> > > > +		imx28_ci_writel(val, ci->hw_bank.regmap[reg]);
> > > > +	else
> > > > +		iowrite32(val, ci->hw_bank.regmap[reg]);
> > > > +#else
> > > > 
> > > >   	iowrite32(val, ci->hw_bank.regmap[reg]);
> > > > 
> > > > +#endif
> > > > 
> > > >   	return val;
> > > >   
> > > >   }
> > > 
> > > Can't we remove the #ifdefs CONFIG_SOC_IMX28 all around?
> > > The check is done on the new flag ci->imx28_write_fix which exists for all
> > > platforms, isn't it?.
> > 
> > The SWP instruction is specific to ARM, so you'd need to stub-out the 
> > imx28_ci_writel() with ifdef then.
> 
> That's better than the mess of #ifdefs this patch adds, which isn't ok
> at all :(
> 

You mean try to reduce the number of #ifdef?

-- 

Best Regards,
Peter Chen

[PATCH v3 2/5] usb: chipidea: add freescale imx28 special write register method

[gregkh at linuxfoundation.org]

On Wed, Oct 30, 2013 at 08:53:42AM +0800, Peter Chen wrote:
> On Tue, Oct 29, 2013 at 04:54:47PM -0700, gregkh at linuxfoundation.org wrote:
> > On Mon, Oct 28, 2013 at 11:47:53AM +0100, Marek Vasut wrote:
> > > Dear Hector Palacios,
> > > 
> > > > Dear Peter,
> > > > 
> > > > On 10/25/2013 08:02 AM, Peter Chen wrote:
> > > > > According to Freescale imx28 Errata, "ENGR119653 USB: ARM to USB
> > > > > register error issue", All USB register write operations must
> > > > > use the ARM SWP instruction. So, we implement special hw_write
> > > > > and hw_test_and_clear for imx28.
> > > > > 
> > > > > Discussion for it at below:
> > > > > http://marc.info/?l=linux-usb&m=137996395529294&w=2
> > > > > 
> > > > > Signed-off-by: Peter Chen
> > > > > <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org> ---
> > > > > Changes for v2:
> > > > > - Rebase to latest usb-next tree
> > > > > 
> > > > >   drivers/usb/chipidea/ci.h    |   23 +++++++++++++++++++++++
> > > > >   drivers/usb/chipidea/core.c  |    2 ++
> > > > >   drivers/usb/chipidea/host.c  |    1 +
> > > > >   include/linux/usb/chipidea.h |    1 +
> > > > >   4 files changed, 27 insertions(+), 0 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
> > > > > index 1c94fc5..4eb61d0 100644
> > > > > --- a/drivers/usb/chipidea/ci.h
> > > > > +++ b/drivers/usb/chipidea/ci.h
> > > > > @@ -173,6 +173,8 @@ struct ci_hdrc {
> > > > > 
> > > > >   	struct dentry			*debugfs;
> > > > >   	bool				id_event;
> > > > >   	bool				b_sess_valid_event;
> > > > > 
> > > > > +	/* imx28 needs swp instruction for writing */
> > > > > +	bool				imx28_write_fix;
> > > > > 
> > > > >   };
> > > > >   
> > > > >   static inline struct ci_role_driver *ci_role(struct ci_hdrc *ci)
> > > > > 
> > > > > @@ -253,6 +255,13 @@ static inline u32 hw_read(struct ci_hdrc *ci, enum
> > > > > ci_hw_regs reg, u32 mask)
> > > > > 
> > > > >   	return ioread32(ci->hw_bank.regmap[reg]) & mask;
> > > > >   
> > > > >   }
> > > > > 
> > > > > +#ifdef CONFIG_SOC_IMX28
> > > > > +static inline void imx28_ci_writel(u32 val32, volatile u32 *addr)
> > > > > +{
> > > > > +	__asm__ ("swp %0, %0, [%1]" : : "r"(val32), "r"(addr));
> > > > > +}
> > > > > +#endif
> > > > > +
> > > > > 
> > > > >   /**
> > > > >   
> > > > >    * hw_write: writes to a hw register
> > > > >    * @reg:  register index
> > > > > 
> > > > > @@ -266,7 +275,14 @@ static inline void hw_write(struct ci_hdrc *ci, enum
> > > > > ci_hw_regs reg,
> > > > > 
> > > > >   		data = (ioread32(ci->hw_bank.regmap[reg]) & ~mask)
> > > > >   		
> > > > >   			| (data & mask);
> > > > > 
> > > > > +#ifdef CONFIG_SOC_IMX28
> > > > > +	if (ci->imx28_write_fix)
> > > > > +		imx28_ci_writel(data, ci->hw_bank.regmap[reg]);
> > > > > +	else
> > > > > +		iowrite32(data, ci->hw_bank.regmap[reg]);
> > > > > +#else
> > > > > 
> > > > >   	iowrite32(data, ci->hw_bank.regmap[reg]);
> > > > > 
> > > > > +#endif
> > > > > 
> > > > >   }
> > > > >   
> > > > >   /**
> > > > > 
> > > > > @@ -281,7 +297,14 @@ static inline u32 hw_test_and_clear(struct ci_hdrc
> > > > > *ci, enum ci_hw_regs reg,
> > > > > 
> > > > >   {
> > > > >   
> > > > >   	u32 val = ioread32(ci->hw_bank.regmap[reg]) & mask;
> > > > > 
> > > > > +#ifdef CONFIG_SOC_IMX28
> > > > > +	if (ci->imx28_write_fix)
> > > > > +		imx28_ci_writel(val, ci->hw_bank.regmap[reg]);
> > > > > +	else
> > > > > +		iowrite32(val, ci->hw_bank.regmap[reg]);
> > > > > +#else
> > > > > 
> > > > >   	iowrite32(val, ci->hw_bank.regmap[reg]);
> > > > > 
> > > > > +#endif
> > > > > 
> > > > >   	return val;
> > > > >   
> > > > >   }
> > > > 
> > > > Can't we remove the #ifdefs CONFIG_SOC_IMX28 all around?
> > > > The check is done on the new flag ci->imx28_write_fix which exists for all
> > > > platforms, isn't it?.
> > > 
> > > The SWP instruction is specific to ARM, so you'd need to stub-out the 
> > > imx28_ci_writel() with ifdef then.
> > 
> > That's better than the mess of #ifdefs this patch adds, which isn't ok
> > at all :(
> > 
> 
> You mean try to reduce the number of #ifdef?

Of course you should.

[PATCH v3 3/5] usb: chipidea: imx: set CI_HDRC_IMX28_WRITE_FIX for imx28

[Peter Chen]

Due to imx28 needs ARM swp instruction for writing, we set
CI_HDRC_IMX28_WRITE_FIX for imx28.

Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
 drivers/usb/chipidea/ci_hdrc_imx.c |   32 ++++++++++++++++++++++++++------
 1 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c
index 023d3cb..68f7f5e 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.c
+++ b/drivers/usb/chipidea/ci_hdrc_imx.c
@@ -23,6 +23,26 @@
 #include "ci.h"
 #include "ci_hdrc_imx.h"
 
+#define CI_HDRC_IMX_IMX28_WRITE_FIX BIT(0)
+
+struct ci_hdrc_imx_platform_flag {
+	unsigned int flags;
+};
+
+static const struct ci_hdrc_imx_platform_flag imx27_usb_data = {
+};
+
+static const struct ci_hdrc_imx_platform_flag imx28_usb_data = {
+	.flags = CI_HDRC_IMX_IMX28_WRITE_FIX,
+};
+
+static const struct of_device_id ci_hdrc_imx_dt_ids[] = {
+	{ .compatible = "fsl,imx28-usb", .data = &imx28_usb_data},
+	{ .compatible = "fsl,imx27-usb", .data = &imx27_usb_data},
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, ci_hdrc_imx_dt_ids);
+
 struct ci_hdrc_imx_data {
 	struct usb_phy *phy;
 	struct platform_device *ci_pdev;
@@ -82,6 +102,9 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
 				  CI_HDRC_DISABLE_STREAMING,
 	};
 	int ret;
+	const struct of_device_id *of_id =
+			of_match_device(ci_hdrc_imx_dt_ids, &pdev->dev);
+	const struct ci_hdrc_imx_platform_flag *imx_platform_flag = of_id->data;
 
 	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
 	if (!data) {
@@ -115,6 +138,9 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
 
 	pdata.phy = data->phy;
 
+	if (imx_platform_flag->flags & CI_HDRC_IMX_IMX28_WRITE_FIX)
+		pdata.flags |= CI_HDRC_IMX28_WRITE_FIX;
+
 	if (!pdev->dev.dma_mask)
 		pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
 	if (!pdev->dev.coherent_dma_mask)
@@ -174,12 +200,6 @@ static int ci_hdrc_imx_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static const struct of_device_id ci_hdrc_imx_dt_ids[] = {
-	{ .compatible = "fsl,imx27-usb", },
-	{ /* sentinel */ }
-};
-MODULE_DEVICE_TABLE(of, ci_hdrc_imx_dt_ids);
-
 static struct platform_driver ci_hdrc_imx_driver = {
 	.probe = ci_hdrc_imx_probe,
 	.remove = ci_hdrc_imx_remove,
-- 
1.7.1

[PATCH v3 3/5] usb: chipidea: imx: set CI_HDRC_IMX28_WRITE_FIX for imx28

[Marek Vasut]

Dear Peter Chen,

> Due to imx28 needs ARM swp instruction for writing, we set
> CI_HDRC_IMX28_WRITE_FIX for imx28.
> 
> Signed-off-by: Peter Chen <peter.chen@freescale.com>
> ---
>  drivers/usb/chipidea/ci_hdrc_imx.c |   32 ++++++++++++++++++++++++++------
>  1 files changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c
> b/drivers/usb/chipidea/ci_hdrc_imx.c index 023d3cb..68f7f5e 100644
> --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> @@ -23,6 +23,26 @@
>  #include "ci.h"
>  #include "ci_hdrc_imx.h"
> 
> +#define CI_HDRC_IMX_IMX28_WRITE_FIX BIT(0)
> +
> +struct ci_hdrc_imx_platform_flag {
> +	unsigned int flags;
> +};
> +
> +static const struct ci_hdrc_imx_platform_flag imx27_usb_data = {
> +};
> +
> +static const struct ci_hdrc_imx_platform_flag imx28_usb_data = {
> +	.flags = CI_HDRC_IMX_IMX28_WRITE_FIX,
> +};
> +
> +static const struct of_device_id ci_hdrc_imx_dt_ids[] = {
> +	{ .compatible = "fsl,imx28-usb", .data = &imx28_usb_data},
> +	{ .compatible = "fsl,imx27-usb", .data = &imx27_usb_data},

Just a nit-pick, but the order here is wrong ;-)
[...]
Best regards,
Marek Vasut

[PATCH v3 3/5] usb: chipidea: imx: set CI_HDRC_IMX28_WRITE_FIX for imx28

[Shawn Guo]

On Sun, Oct 27, 2013 at 05:25:36PM +0100, Marek Vasut wrote:
> > +static const struct of_device_id ci_hdrc_imx_dt_ids[] = {
> > +	{ .compatible = "fsl,imx28-usb", .data = &imx28_usb_data},
> > +	{ .compatible = "fsl,imx27-usb", .data = &imx27_usb_data},
> 
> Just a nit-pick, but the order here is wrong ;-)

Oh, no.  Before of_match_device() gets improved to find the best match,
we have to sort the table from the most specific entry to the most
generic one.

Shawn

[PATCH v3 3/5] usb: chipidea: imx: set CI_HDRC_IMX28_WRITE_FIX for imx28

[Marek Vasut]

Hi Shawn,

> On Sun, Oct 27, 2013 at 05:25:36PM +0100, Marek Vasut wrote:
> > > +static const struct of_device_id ci_hdrc_imx_dt_ids[] = {
> > > +	{ .compatible = "fsl,imx28-usb", .data = &imx28_usb_data},
> > > +	{ .compatible = "fsl,imx27-usb", .data = &imx27_usb_data},
> > 
> > Just a nit-pick, but the order here is wrong ;-)
> 
> Oh, no.  Before of_match_device() gets improved to find the best match,
> we have to sort the table from the most specific entry to the most
> generic one.

Oh, thanks for explaining!

Best regards,
Marek Vasut

[PATCH v3 4/5] usb: doc: chipidea: imx: add compatiable string for imx28 SoC

[Peter Chen]

Due to imx28 usb has special write requirement compared to other imx SoCs.

Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
Changes for v3:
- 4/5, 5/5 are added

 .../devicetree/bindings/usb/ci13xxx-imx.txt        |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
index b4b5b79..a502d41 100644
--- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
+++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
@@ -1,7 +1,8 @@
 * Freescale i.MX ci13xxx usb controllers
 
 Required properties:
-- compatible: Should be "fsl,imx27-usb"
+- compatible: "fsl,imx28-usb" for imx28 SoC, "fsl,imx27-usb" for
+non-imx28 SoC.
 - reg: Should contain registers location and length
 - interrupts: Should contain controller interrupt
 
-- 
1.7.1

[PATCH v3 4/5] usb: doc: chipidea: imx: add compatiable string for imx28 SoC

[Marek Vasut]

Dear Peter Chen,

> Due to imx28 usb has special write requirement compared to other imx SoCs.
> 
> Signed-off-by: Peter Chen <peter.chen@freescale.com>
> ---
> Changes for v3:
> - 4/5, 5/5 are added
> 
>  .../devicetree/bindings/usb/ci13xxx-imx.txt        |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt index
> b4b5b79..a502d41 100644
> --- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> @@ -1,7 +1,8 @@
>  * Freescale i.MX ci13xxx usb controllers
> 
>  Required properties:
> -- compatible: Should be "fsl,imx27-usb"
> +- compatible: "fsl,imx28-usb" for imx28 SoC, "fsl,imx27-usb" for
> +non-imx28 SoC.
>  - reg: Should contain registers location and length
>  - interrupts: Should contain controller interrupt

Does the ERRATA apply to MX23 as well btw?

Best regards,
Marek Vasut

[PATCH v3 4/5] usb: doc: chipidea: imx: add compatiable string for imx28 SoC

[Peter Chen]

On Sun, Oct 27, 2013 at 05:26:16PM +0100, Marek Vasut wrote:
> Dear Peter Chen,
> 
> > Due to imx28 usb has special write requirement compared to other imx SoCs.
> > 
> > Signed-off-by: Peter Chen <peter.chen@freescale.com>
> > ---
> > Changes for v3:
> > - 4/5, 5/5 are added
> > 
> >  .../devicetree/bindings/usb/ci13xxx-imx.txt        |    3 ++-
> >  1 files changed, 2 insertions(+), 1 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt index
> > b4b5b79..a502d41 100644
> > --- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > @@ -1,7 +1,8 @@
> >  * Freescale i.MX ci13xxx usb controllers
> > 
> >  Required properties:
> > -- compatible: Should be "fsl,imx27-usb"
> > +- compatible: "fsl,imx28-usb" for imx28 SoC, "fsl,imx27-usb" for
> > +non-imx28 SoC.
> >  - reg: Should contain registers location and length
> >  - interrupts: Should contain controller interrupt
> 
> Does the ERRATA apply to MX23 as well btw?
> 

It is for i.mx28 only.

-- 

Best Regards,
Peter Chen

[PATCH v3 5/5] ARM: dts: imx28: changing usb compatible string as only "fsl, imx28-usb"

[Peter Chen]

Due to imx28 usb has special write request, it is not compatible
with other imx27 sytle usb controllers.

Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
 arch/arm/boot/dts/imx28.dtsi |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/imx28.dtsi b/arch/arm/boot/dts/imx28.dtsi
index 7363fde..6cd2607 100644
--- a/arch/arm/boot/dts/imx28.dtsi
+++ b/arch/arm/boot/dts/imx28.dtsi
@@ -1041,7 +1041,7 @@
 		ranges;
 
 		usb0: usb at 80080000 {
-			compatible = "fsl,imx28-usb", "fsl,imx27-usb";
+			compatible = "fsl,imx28-usb";
 			reg = <0x80080000 0x10000>;
 			interrupts = <93>;
 			clocks = <&clks 60>;
@@ -1050,7 +1050,7 @@
 		};
 
 		usb1: usb at 80090000 {
-			compatible = "fsl,imx28-usb", "fsl,imx27-usb";
+			compatible = "fsl,imx28-usb";
 			reg = <0x80090000 0x10000>;
 			interrupts = <92>;
 			clocks = <&clks 61>;
-- 
1.7.1

[PATCH v3 5/5] ARM: dts: imx28: changing usb compatible string as only "fsl,imx28-usb"

[Shawn Guo]

On Fri, Oct 25, 2013 at 02:02:23PM +0800, Peter Chen wrote:
> Due to imx28 usb has special write request, it is not compatible
> with other imx27 sytle usb controllers.
> 
> Signed-off-by: Peter Chen <peter.chen@freescale.com>
> ---
>  arch/arm/boot/dts/imx28.dtsi |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/imx28.dtsi b/arch/arm/boot/dts/imx28.dtsi
> index 7363fde..6cd2607 100644
> --- a/arch/arm/boot/dts/imx28.dtsi
> +++ b/arch/arm/boot/dts/imx28.dtsi
> @@ -1041,7 +1041,7 @@
>  		ranges;
>  
>  		usb0: usb at 80080000 {
> -			compatible = "fsl,imx28-usb", "fsl,imx27-usb";
> +			compatible = "fsl,imx28-usb";

You shouldn't need the change as long as you put "fsl,imx28-usb" before
"fsl,imx27-usb" in driver ci_hdrc_imx_dt_ids[] table.

Shawn

>  			reg = <0x80080000 0x10000>;
>  			interrupts = <93>;
>  			clocks = <&clks 60>;
> @@ -1050,7 +1050,7 @@
>  		};
>  
>  		usb1: usb at 80090000 {
> -			compatible = "fsl,imx28-usb", "fsl,imx27-usb";
> +			compatible = "fsl,imx28-usb";
>  			reg = <0x80090000 0x10000>;
>  			interrupts = <92>;
>  			clocks = <&clks 61>;
> -- 
> 1.7.1
> 
> 

[PATCH v3 5/5] ARM: dts: imx28: changing usb compatible string as only "fsl,imx28-usb"

[Peter Chen]

On Fri, Oct 25, 2013 at 04:23:32PM +0800, Shawn Guo wrote:
> On Fri, Oct 25, 2013 at 02:02:23PM +0800, Peter Chen wrote:
> > Due to imx28 usb has special write request, it is not compatible
> > with other imx27 sytle usb controllers.
> > 
> > Signed-off-by: Peter Chen <peter.chen@freescale.com>
> > ---
> >  arch/arm/boot/dts/imx28.dtsi |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm/boot/dts/imx28.dtsi b/arch/arm/boot/dts/imx28.dtsi
> > index 7363fde..6cd2607 100644
> > --- a/arch/arm/boot/dts/imx28.dtsi
> > +++ b/arch/arm/boot/dts/imx28.dtsi
> > @@ -1041,7 +1041,7 @@
> >  		ranges;
> >  
> >  		usb0: usb at 80080000 {
> > -			compatible = "fsl,imx28-usb", "fsl,imx27-usb";
> > +			compatible = "fsl,imx28-usb";
> 
> You shouldn't need the change as long as you put "fsl,imx28-usb" before
> "fsl,imx27-usb" in driver ci_hdrc_imx_dt_ids[] table.
> 

I know it can work without this dts change, but that driver change like
a workaround. Since the imx28-usb is not compatible with imx27-usb,
we'd better only keep "fsl, imx28-usb" compatible string at imx28 dtsi.

-- 

Best Regards,
Peter Chen

[PATCH v3 5/5] ARM: dts: imx28: changing usb compatible string as only "fsl,imx28-usb"

[Shawn Guo]

On Fri, Oct 25, 2013 at 04:14:30PM +0800, Peter Chen wrote:
> > > @@ -1041,7 +1041,7 @@
> > >  		ranges;
> > >  
> > >  		usb0: usb at 80080000 {
> > > -			compatible = "fsl,imx28-usb", "fsl,imx27-usb";
> > > +			compatible = "fsl,imx28-usb";
> > 
> > You shouldn't need the change as long as you put "fsl,imx28-usb" before
> > "fsl,imx27-usb" in driver ci_hdrc_imx_dt_ids[] table.
> > 
> 
> I know it can work without this dts change, but that driver change like
> a workaround.

No, it's not a workaround.  Before of_match_device() gets improved to
match the best one, it should just work that way.

> Since the imx28-usb is not compatible with imx27-usb,
> we'd better only keep "fsl, imx28-usb" compatible string at imx28 dtsi.

>From hardware point of view, the USB block on imx28 *is* inherited from
imx27, and compatible to imx27 USB block, even though we have a little
difference to handle in software.  The compatible string is written in
this way at the first place exactly for the reason we can save such
DTS change when we have some little incompatibility to handle.

Shawn

[PATCH v3 5/5] ARM: dts: imx28: changing usb compatible string as only "fsl,imx28-usb"

[Peter Chen]

On Fri, Oct 25, 2013 at 04:46:09PM +0800, Shawn Guo wrote:
> On Fri, Oct 25, 2013 at 04:14:30PM +0800, Peter Chen wrote:
> > > > @@ -1041,7 +1041,7 @@
> > > >  		ranges;
> > > >  
> > > >  		usb0: usb at 80080000 {
> > > > -			compatible = "fsl,imx28-usb", "fsl,imx27-usb";
> > > > +			compatible = "fsl,imx28-usb";
> > > 
> > > You shouldn't need the change as long as you put "fsl,imx28-usb" before
> > > "fsl,imx27-usb" in driver ci_hdrc_imx_dt_ids[] table.
> > > 
> > 
> > I know it can work without this dts change, but that driver change like
> > a workaround.
> 
> No, it's not a workaround.  Before of_match_device() gets improved to
> match the best one, it should just work that way.
> 

hmm, before of_match_device gets improved or it is well documented,
how user knows to organize device_id table.

> > Since the imx28-usb is not compatible with imx27-usb,
> > we'd better only keep "fsl, imx28-usb" compatible string at imx28 dtsi.
> 
> From hardware point of view, the USB block on imx28 *is* inherited from
> imx27, and compatible to imx27 USB block, even though we have a little
> difference to handle in software.  The compatible string is written in
> this way at the first place exactly for the reason we can save such
> DTS change when we have some little incompatibility to handle.
> 

OK, I agree, then the of_match_device really should be improved to get
above purpose. BTW, any reasons why esdhc has different compatible string
for every SoCs?

-- 

Best Regards,
Peter Chen

[PATCH v3 5/5] ARM: dts: imx28: changing usb compatible string as only "fsl,imx28-usb"

[Shawn Guo]

On Mon, Oct 28, 2013 at 09:59:27AM +0800, Peter Chen wrote:
> hmm, before of_match_device gets improved or it is well documented,
> how user knows to organize device_id table.

Just like you, you found it after you saw that of_match_device() does
not return you the expected device :)

> 
> > > Since the imx28-usb is not compatible with imx27-usb,
> > > we'd better only keep "fsl, imx28-usb" compatible string at imx28 dtsi.
> > 
> > From hardware point of view, the USB block on imx28 *is* inherited from
> > imx27, and compatible to imx27 USB block, even though we have a little
> > difference to handle in software.  The compatible string is written in
> > this way at the first place exactly for the reason we can save such
> > DTS change when we have some little incompatibility to handle.
> > 
> 
> OK, I agree, then the of_match_device really should be improved to get
> above purpose.

Yes, there is already patch for that but it just hasn't found its way
to mainline because it causes some issue on SPARC.

> BTW, any reasons why esdhc has different compatible string
> for every SoCs?

Mostly because the hardware design sucks.  There so many register
differences and bugs to deal with between difference SoCs.

Shawn

[PATCH v3 1/5] usb: ehci: add freescale imx28 special write register method

[Peter Chen]

On Fri, Oct 25, 2013 at 02:02:19PM +0800, Peter Chen wrote:
> According to Freescale imx28 Errata, "ENGR119653 USB: ARM to USB
> register error issue", All USB register write operations must
> use the ARM SWP instruction. So, we implement a special ehci_write
> for imx28.
> 
> Discussion for it at below:
> http://marc.info/?l=linux-usb&m=137996395529294&w=2
> 

Hi Greg, the last two DT patches are not needed at current time.
The first three are OK. Thanks.

Peter

> Signed-off-by: Peter Chen <peter.chen@freescale.com>
> ---
>  drivers/usb/host/ehci.h |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
> index e8f41c5..535aa8b 100644
> --- a/drivers/usb/host/ehci.h
> +++ b/drivers/usb/host/ehci.h
> @@ -225,6 +225,7 @@ struct ehci_hcd {			/* one per controller */
>  	unsigned		has_synopsys_hc_bug:1; /* Synopsys HC */
>  	unsigned		frame_index_bug:1; /* MosChip (AKA NetMos) */
>  	unsigned		need_oc_pp_cycle:1; /* MPC834X port power */
> +	unsigned		imx28_write_fix:1; /* For Freescale i.MX28 */
>  
>  	/* required for usb32 quirk */
>  	#define OHCI_CTRL_HCFS          (3 << 6)
> @@ -728,6 +729,13 @@ static inline unsigned int ehci_readl(const struct ehci_hcd *ehci,
>  #endif
>  }
>  
> +#ifdef CONFIG_SOC_IMX28
> +static inline void imx28_ehci_writel(u32 val32, volatile u32 *addr)
> +{
> +	__asm__ ("swp %0, %0, [%1]" : : "r"(val32), "r"(addr));
> +}
> +#endif
> +
>  static inline void ehci_writel(const struct ehci_hcd *ehci,
>  		const unsigned int val, __u32 __iomem *regs)
>  {
> @@ -735,6 +743,11 @@ static inline void ehci_writel(const struct ehci_hcd *ehci,
>  	ehci_big_endian_mmio(ehci) ?
>  		writel_be(val, regs) :
>  		writel(val, regs);
> +#elif defined(CONFIG_SOC_IMX28)
> +	if (ehci->imx28_write_fix)
> +		imx28_ehci_writel(val, regs);
> +	else
> +		writel(val, regs);
>  #else
>  	writel(val, regs);
>  #endif
> -- 
> 1.7.1
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 

Best Regards,
Peter Chen