* [PATCH v3 1/5] usb: ehci: add freescale imx28 special write register method
@ 2013-10-25 6:02 Peter Chen
2013-10-25 6:02 ` [PATCH v3 2/5] usb: chipidea: " Peter Chen
` (4 more replies)
0 siblings, 5 replies; 21+ messages in thread
From: Peter Chen @ 2013-10-25 6:02 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v3 2/5] usb: chipidea: add freescale imx28 special write register method
2013-10-25 6:02 [PATCH v3 1/5] usb: ehci: add freescale imx28 special write register method Peter Chen
@ 2013-10-25 6:02 ` Peter Chen
2013-10-28 10:36 ` Hector Palacios
2013-10-25 6:02 ` [PATCH v3 3/5] usb: chipidea: imx: set CI_HDRC_IMX28_WRITE_FIX for imx28 Peter Chen
` (3 subsequent siblings)
4 siblings, 1 reply; 21+ messages in thread
From: Peter Chen @ 2013-10-25 6:02 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v3 3/5] usb: chipidea: imx: set CI_HDRC_IMX28_WRITE_FIX for imx28
2013-10-25 6:02 [PATCH v3 1/5] usb: ehci: add freescale imx28 special write register method Peter Chen
2013-10-25 6:02 ` [PATCH v3 2/5] usb: chipidea: " Peter Chen
@ 2013-10-25 6:02 ` Peter Chen
2013-10-27 16:25 ` Marek Vasut
2013-10-25 6:02 ` [PATCH v3 4/5] usb: doc: chipidea: imx: add compatiable string for imx28 SoC Peter Chen
` (2 subsequent siblings)
4 siblings, 1 reply; 21+ messages in thread
From: Peter Chen @ 2013-10-25 6:02 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v3 4/5] usb: doc: chipidea: imx: add compatiable string for imx28 SoC
2013-10-25 6:02 [PATCH v3 1/5] usb: ehci: add freescale imx28 special write register method Peter Chen
2013-10-25 6:02 ` [PATCH v3 2/5] usb: chipidea: " Peter Chen
2013-10-25 6:02 ` [PATCH v3 3/5] usb: chipidea: imx: set CI_HDRC_IMX28_WRITE_FIX for imx28 Peter Chen
@ 2013-10-25 6:02 ` Peter Chen
2013-10-27 16:26 ` Marek Vasut
2013-10-25 6:02 ` [PATCH v3 5/5] ARM: dts: imx28: changing usb compatible string as only "fsl, imx28-usb" Peter Chen
2013-10-28 8:24 ` [PATCH v3 1/5] usb: ehci: add freescale imx28 special write register method Peter Chen
4 siblings, 1 reply; 21+ messages in thread
From: Peter Chen @ 2013-10-25 6:02 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v3 5/5] ARM: dts: imx28: changing usb compatible string as only "fsl, imx28-usb"
2013-10-25 6:02 [PATCH v3 1/5] usb: ehci: add freescale imx28 special write register method Peter Chen
` (2 preceding siblings ...)
2013-10-25 6:02 ` [PATCH v3 4/5] usb: doc: chipidea: imx: add compatiable string for imx28 SoC Peter Chen
@ 2013-10-25 6:02 ` Peter Chen
2013-10-25 8:23 ` [PATCH v3 5/5] ARM: dts: imx28: changing usb compatible string as only "fsl,imx28-usb" Shawn Guo
2013-10-28 8:24 ` [PATCH v3 1/5] usb: ehci: add freescale imx28 special write register method Peter Chen
4 siblings, 1 reply; 21+ messages in thread
From: Peter Chen @ 2013-10-25 6:02 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v3 5/5] ARM: dts: imx28: changing usb compatible string as only "fsl,imx28-usb"
2013-10-25 8:23 ` [PATCH v3 5/5] ARM: dts: imx28: changing usb compatible string as only "fsl,imx28-usb" Shawn Guo
@ 2013-10-25 8:14 ` Peter Chen
2013-10-25 8:46 ` Shawn Guo
0 siblings, 1 reply; 21+ messages in thread
From: Peter Chen @ 2013-10-25 8:14 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 5/5] ARM: dts: imx28: changing usb compatible string as only "fsl,imx28-usb"
2013-10-25 6:02 ` [PATCH v3 5/5] ARM: dts: imx28: changing usb compatible string as only "fsl, imx28-usb" Peter Chen
@ 2013-10-25 8:23 ` Shawn Guo
2013-10-25 8:14 ` Peter Chen
0 siblings, 1 reply; 21+ messages in thread
From: Shawn Guo @ 2013-10-25 8:23 UTC (permalink / raw)
To: linux-arm-kernel
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
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 5/5] ARM: dts: imx28: changing usb compatible string as only "fsl,imx28-usb"
2013-10-25 8:14 ` Peter Chen
@ 2013-10-25 8:46 ` Shawn Guo
2013-10-28 1:59 ` Peter Chen
0 siblings, 1 reply; 21+ messages in thread
From: Shawn Guo @ 2013-10-25 8:46 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 3/5] usb: chipidea: imx: set CI_HDRC_IMX28_WRITE_FIX for imx28
2013-10-25 6:02 ` [PATCH v3 3/5] usb: chipidea: imx: set CI_HDRC_IMX28_WRITE_FIX for imx28 Peter Chen
@ 2013-10-27 16:25 ` Marek Vasut
2013-10-28 2:03 ` Shawn Guo
0 siblings, 1 reply; 21+ messages in thread
From: Marek Vasut @ 2013-10-27 16:25 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 4/5] usb: doc: chipidea: imx: add compatiable string for imx28 SoC
2013-10-25 6:02 ` [PATCH v3 4/5] usb: doc: chipidea: imx: add compatiable string for imx28 SoC Peter Chen
@ 2013-10-27 16:26 ` Marek Vasut
2013-10-28 2:00 ` Peter Chen
0 siblings, 1 reply; 21+ messages in thread
From: Marek Vasut @ 2013-10-27 16:26 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 5/5] ARM: dts: imx28: changing usb compatible string as only "fsl,imx28-usb"
2013-10-25 8:46 ` Shawn Guo
@ 2013-10-28 1:59 ` Peter Chen
2013-10-28 2:50 ` Shawn Guo
0 siblings, 1 reply; 21+ messages in thread
From: Peter Chen @ 2013-10-28 1:59 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 4/5] usb: doc: chipidea: imx: add compatiable string for imx28 SoC
2013-10-27 16:26 ` Marek Vasut
@ 2013-10-28 2:00 ` Peter Chen
0 siblings, 0 replies; 21+ messages in thread
From: Peter Chen @ 2013-10-28 2:00 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 3/5] usb: chipidea: imx: set CI_HDRC_IMX28_WRITE_FIX for imx28
2013-10-27 16:25 ` Marek Vasut
@ 2013-10-28 2:03 ` Shawn Guo
2013-10-28 7:52 ` Marek Vasut
0 siblings, 1 reply; 21+ messages in thread
From: Shawn Guo @ 2013-10-28 2:03 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 5/5] ARM: dts: imx28: changing usb compatible string as only "fsl,imx28-usb"
2013-10-28 1:59 ` Peter Chen
@ 2013-10-28 2:50 ` Shawn Guo
0 siblings, 0 replies; 21+ messages in thread
From: Shawn Guo @ 2013-10-28 2:50 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 3/5] usb: chipidea: imx: set CI_HDRC_IMX28_WRITE_FIX for imx28
2013-10-28 2:03 ` Shawn Guo
@ 2013-10-28 7:52 ` Marek Vasut
0 siblings, 0 replies; 21+ messages in thread
From: Marek Vasut @ 2013-10-28 7:52 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 1/5] usb: ehci: add freescale imx28 special write register method
2013-10-25 6:02 [PATCH v3 1/5] usb: ehci: add freescale imx28 special write register method Peter Chen
` (3 preceding siblings ...)
2013-10-25 6:02 ` [PATCH v3 5/5] ARM: dts: imx28: changing usb compatible string as only "fsl, imx28-usb" Peter Chen
@ 2013-10-28 8:24 ` Peter Chen
4 siblings, 0 replies; 21+ messages in thread
From: Peter Chen @ 2013-10-28 8:24 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 2/5] usb: chipidea: add freescale imx28 special write register method
2013-10-25 6:02 ` [PATCH v3 2/5] usb: chipidea: " Peter Chen
@ 2013-10-28 10:36 ` Hector Palacios
2013-10-28 10:47 ` Marek Vasut
0 siblings, 1 reply; 21+ messages in thread
From: Hector Palacios @ 2013-10-28 10:36 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 2/5] usb: chipidea: add freescale imx28 special write register method
2013-10-28 10:36 ` Hector Palacios
@ 2013-10-28 10:47 ` Marek Vasut
2013-10-29 23:54 ` gregkh at linuxfoundation.org
0 siblings, 1 reply; 21+ messages in thread
From: Marek Vasut @ 2013-10-28 10:47 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 2/5] usb: chipidea: add freescale imx28 special write register method
2013-10-28 10:47 ` Marek Vasut
@ 2013-10-29 23:54 ` gregkh at linuxfoundation.org
2013-10-30 0:53 ` Peter Chen
0 siblings, 1 reply; 21+ messages in thread
From: gregkh at linuxfoundation.org @ 2013-10-29 23:54 UTC (permalink / raw)
To: linux-arm-kernel
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 :(
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 2/5] usb: chipidea: add freescale imx28 special write register method
2013-10-29 23:54 ` gregkh at linuxfoundation.org
@ 2013-10-30 0:53 ` Peter Chen
2013-10-30 3:09 ` gregkh at linuxfoundation.org
0 siblings, 1 reply; 21+ messages in thread
From: Peter Chen @ 2013-10-30 0:53 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 2/5] usb: chipidea: add freescale imx28 special write register method
2013-10-30 0:53 ` Peter Chen
@ 2013-10-30 3:09 ` gregkh at linuxfoundation.org
0 siblings, 0 replies; 21+ messages in thread
From: gregkh at linuxfoundation.org @ 2013-10-30 3:09 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2013-10-30 3:09 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-10-25 6:02 [PATCH v3 1/5] usb: ehci: add freescale imx28 special write register method Peter Chen
2013-10-25 6:02 ` [PATCH v3 2/5] usb: chipidea: " Peter Chen
2013-10-28 10:36 ` Hector Palacios
2013-10-28 10:47 ` Marek Vasut
2013-10-29 23:54 ` gregkh at linuxfoundation.org
2013-10-30 0:53 ` Peter Chen
2013-10-30 3:09 ` gregkh at linuxfoundation.org
2013-10-25 6:02 ` [PATCH v3 3/5] usb: chipidea: imx: set CI_HDRC_IMX28_WRITE_FIX for imx28 Peter Chen
2013-10-27 16:25 ` Marek Vasut
2013-10-28 2:03 ` Shawn Guo
2013-10-28 7:52 ` Marek Vasut
2013-10-25 6:02 ` [PATCH v3 4/5] usb: doc: chipidea: imx: add compatiable string for imx28 SoC Peter Chen
2013-10-27 16:26 ` Marek Vasut
2013-10-28 2:00 ` Peter Chen
2013-10-25 6:02 ` [PATCH v3 5/5] ARM: dts: imx28: changing usb compatible string as only "fsl, imx28-usb" Peter Chen
2013-10-25 8:23 ` [PATCH v3 5/5] ARM: dts: imx28: changing usb compatible string as only "fsl,imx28-usb" Shawn Guo
2013-10-25 8:14 ` Peter Chen
2013-10-25 8:46 ` Shawn Guo
2013-10-28 1:59 ` Peter Chen
2013-10-28 2:50 ` Shawn Guo
2013-10-28 8:24 ` [PATCH v3 1/5] usb: ehci: add freescale imx28 special write register method Peter Chen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.