Re: [PATCH 4/7] usb: chipidea: add freescale imx28 special write register method
On Thu, Dec 05, 2013 at 03:20:52PM +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 special hw_write and hw_test_and_clear for imx28. Discussion for it at below: http://marc.info/?l=linux-usbm=137996395529294w=2 Signed-off-by: Peter Chen peter.c...@freescale.com Signed-off-by: Marc Kleine-Budde m...@pengutronix.de Tested-by: Marc Kleine-Budde m...@pengutronix.de --- drivers/usb/chipidea/ci.h| 26 -- drivers/usb/chipidea/core.c |2 ++ drivers/usb/chipidea/host.c |1 + include/linux/usb/chipidea.h |1 + 4 files changed, 28 insertions(+), 2 deletions(-) Same here, is this really 3.13-final material? -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 4/7] usb: chipidea: add freescale imx28 special write register method
From: 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-usbm=137996395529294w=2 Personally I think you should include the text of the errata in one of the files. I'm not suggesting you change the guts of the patch, but... Having read the errata it isn't 100% clear to me that using SWP is necessary for all register accesses. Since SWP is a locked bus cycle it will have performance penalties, depending on the number of write operations this might be measurable. AFAICT the reason that SWP works is that does a read cycle prior to the write that ensures that logic is all in the correct state (I think the read returns garbage). There is no requirement for a locked bus cycle, or for the cycles to be adjacent (from the cpu's point of view). Just that there can be no other cycles on what I assume is a peripheral bus. So if the code can guarantee no other cycles on the same bus (the ones that are documented in the errata to cause problems) then there is no need to use the SWP instruction. Typically this would be single cpu systems doing a series of accesses (the first could be a read) with interrupts hard disabled. David -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 4/7] usb: chipidea: add freescale imx28 special write register method
Personally I think you should include the text of the errata in one of the files. Yes, some may be interested in detail, not only the solution, will do it next time. I'm not suggesting you change the guts of the patch, but... Having read the errata it isn't 100% clear to me that using SWP is necessary for all register accesses. Only write USB controller register is needed. Since SWP is a locked bus cycle it will have performance penalties, depending on the number of write operations this might be measurable. AFAICT the reason that SWP works is that does a read cycle prior to the write that ensures that logic is all in the correct state (I think the read returns garbage). There is no requirement for a locked bus cycle, or for the cycles to be adjacent (from the cpu's point of view). Just that there can be no other cycles on what I assume is a peripheral bus. So if the code can guarantee no other cycles on the same bus (the ones that are documented in the errata to cause problems) then there is no need to use the SWP instruction. Typically this would be single cpu systems doing a series of accesses (the first could be a read) with interrupts hard disabled. Thanks for your analysis. As far as I know, the workaround for this problem is it needs a read usb controller register operation before every write, the swp instruction may be the easiest way to implement it, so the design team suggests it, this patch just implements that workaround. We can't guarantee there is no other access on the AHB without disable interrupt, would you think below operation will be more efficient than swp, or any better solutions? local_irq_save(flags); readl(addr); writel(val, addr); local_irq_restore(flags); Peter -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/7] usb: chipidea: add freescale imx28 special write register method
On 12/05/2013 01:16 PM, Peter Chen wrote: Personally I think you should include the text of the errata in one of the files. Yes, some may be interested in detail, not only the solution, will do it next time. I'm not suggesting you change the guts of the patch, but... Having read the errata it isn't 100% clear to me that using SWP is necessary for all register accesses. Only write USB controller register is needed. Since SWP is a locked bus cycle it will have performance penalties, depending on the number of write operations this might be measurable. AFAICT the reason that SWP works is that does a read cycle prior to the write that ensures that logic is all in the correct state (I think the read returns garbage). There is no requirement for a locked bus cycle, or for the cycles to be adjacent (from the cpu's point of view). Just that there can be no other cycles on what I assume is a peripheral bus. So if the code can guarantee no other cycles on the same bus (the ones that are documented in the errata to cause problems) then there is no need to use the SWP instruction. Typically this would be single cpu systems doing a series of accesses (the first could be a read) with interrupts hard disabled. Thanks for your analysis. As far as I know, the workaround for this problem is it needs a read usb controller register operation before every write, the swp instruction may be the easiest way to implement it, so the design team suggests it, this patch just implements that workaround. We can't guarantee there is no other access on the AHB without disable interrupt, would you think below operation will be more efficient than swp, or any better solutions? local_irq_save(flags); readl(addr); writel(val, addr); local_irq_restore(flags); Does this work without modifications on PREEMPT_RT? Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature
RE: [PATCH 4/7] usb: chipidea: add freescale imx28 special write register method
On 12/05/2013 01:16 PM, Peter Chen wrote: Personally I think you should include the text of the errata in one of the files. Yes, some may be interested in detail, not only the solution, will do it next time. I'm not suggesting you change the guts of the patch, but... Having read the errata it isn't 100% clear to me that using SWP is necessary for all register accesses. Only write USB controller register is needed. Since SWP is a locked bus cycle it will have performance penalties, depending on the number of write operations this might be measurable. AFAICT the reason that SWP works is that does a read cycle prior to the write that ensures that logic is all in the correct state (I think the read returns garbage). There is no requirement for a locked bus cycle, or for the cycles to be adjacent (from the cpu's point of view). Just that there can be no other cycles on what I assume is a peripheral bus. So if the code can guarantee no other cycles on the same bus (the ones that are documented in the errata to cause problems) then there is no need to use the SWP instruction. Typically this would be single cpu systems doing a series of accesses (the first could be a read) with interrupts hard disabled. Thanks for your analysis. As far as I know, the workaround for this problem is it needs a read usb controller register operation before every write, the swp instruction may be the easiest way to implement it, so the design team suggests it, this patch just implements that workaround. We can't guarantee there is no other access on the AHB without disable interrupt, would you think below operation will be more efficient than swp, or any better solutions? local_irq_save(flags); readl(addr); writel(val, addr); local_irq_restore(flags); Does this work without modifications on PREEMPT_RT? What's the relationship between PREEMPT_RT and local_irq_save? local_irq_save will save irq registers and disable arm hardware interrupt (I bit is set), in that case, no other threads will be scheduled at single core system. Peter
RE: [PATCH 4/7] usb: chipidea: add freescale imx28 special write register method
We can't guarantee there is no other access on the AHB without disable
interrupt, would you think below operation will be more efficient than swp,
or any better solutions?
I don't know whether it would be more or less efficient, it rather
depends on the finer points of the architecture.
It might be that some of the accesses are already inside an interrupt
disabled block.
local_irq_save(flags);
local_irq_save() might only stop driver ISR being dispatched, in which
case you'd need to know whether the main ISR code does any AHB cycles.
Actually disabling interrupts might take more clocks than the RMW cycle.
readl(addr);
I think the read above need not be exactly the same address as the
write below (in which case a barrier is needed).
You could also add a small amount of code here, using the clock
cycles that would almost certainly be stalls in SWP were used.
Especially between back to back transfers.
writel(val, addr);
At this point you could write to other USB registers.
This code might be a win if there are multiple accesses.
local_irq_restore(flags);
Does this work without modifications on PREEMPT_RT?
Disabling interrupts usually also disables pre-emption
(if only because you need an interrupt to get into a code
path that might do pre-emption).
Just commenting that SWP is used to ensure that the previous AHB cycle
is to the USB block (then refer to the errata) would allow other people
to consider other options without having to chase down the errata itself.
David
N�r��yb�X��ǧv�^�){.n�+{��^n�r���z���h��� �G���h�(�階�ݢj���m��z�ޖ���f���h���~�m�
[PATCH 4/7] usb: chipidea: add freescale imx28 special write register method
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-usbm=137996395529294w=2
Signed-off-by: Peter Chen peter.c...@freescale.com
Signed-off-by: Marc Kleine-Budde m...@pengutronix.de
Tested-by: Marc Kleine-Budde m...@pengutronix.de
---
drivers/usb/chipidea/ci.h| 26 --
drivers/usb/chipidea/core.c |2 ++
drivers/usb/chipidea/host.c |1 +
include/linux/usb/chipidea.h |1 +
4 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
index 1c94fc5..f66bc24 100644
--- a/drivers/usb/chipidea/ci.h
+++ b/drivers/usb/chipidea/ci.h
@@ -135,6 +135,7 @@ struct hw_bank {
* @id_event: indicates there is an id event, and handled at ci_otg_work
* @b_sess_valid_event: indicates there is a vbus event, and handled
* at ci_otg_work
+ * @imx28_write_fix: Freescale imx28 needs swp instruction for writing
*/
struct ci_hdrc {
struct device *dev;
@@ -173,6 +174,7 @@ struct ci_hdrc {
struct dentry *debugfs;
boolid_event;
boolb_sess_valid_event;
+ boolimx28_write_fix;
};
static inline struct ci_role_driver *ci_role(struct ci_hdrc *ci)
@@ -253,6 +255,26 @@ 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 val, volatile void __iomem *addr)
+{
+ __asm__ (swp %0, %0, [%1] : : r(val), r(addr));
+}
+#else
+static inline void imx28_ci_writel(u32 val, volatile void __iomem *addr)
+{
+}
+#endif
+
+static inline void __hw_write(struct ci_hdrc *ci, u32 val,
+ void __iomem *addr)
+{
+ if (IS_ENABLED(CONFIG_SOC_IMX28) ci-imx28_write_fix)
+ imx28_ci_writel(val, addr);
+ else
+ iowrite32(val, addr);
+}
+
/**
* hw_write: writes to a hw register
* @reg: register index
@@ -266,7 +288,7 @@ static inline void hw_write(struct ci_hdrc *ci, enum
ci_hw_regs reg,
data = (ioread32(ci-hw_bank.regmap[reg]) ~mask)
| (data mask);
- iowrite32(data, ci-hw_bank.regmap[reg]);
+ __hw_write(ci, data, ci-hw_bank.regmap[reg]);
}
/**
@@ -281,7 +303,7 @@ 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;
- iowrite32(val, ci-hw_bank.regmap[reg]);
+ __hw_write(ci, val, ci-hw_bank.regmap[reg]);
return val;
}
diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index 6e73f8c..5efdc9b 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -554,6 +554,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 526cd77..a8ac6c1 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_FIXBIT(5)
enum usb_dr_modedr_mode;
#define CI_HDRC_CONTROLLER_RESET_EVENT 0
#define CI_HDRC_CONTROLLER_STOPPED_EVENT 1
--
1.7.8
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
