Re: [PATCH 2/7] usb: chipidea: add freescale imx28 special write register method

2014-01-10 Thread gre...@linuxfoundation.org 
On Fri, Jan 10, 2014 at 10:25:54AM +, David Laight wrote:
 From: Peter Chen
 ...
  +#ifdef 
  +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 (ci-imx28_write_fix)
  +   imx28_ci_writel(val, addr);
  +   else
  +   iowrite32(val, addr);
  +}
  +
 
 This code ought to be defensive against 'imx28_write_fix' being
 set when the kernel isn't compiled with CONFIG_SOC_IMX28.
 This can easily be done by changing the #else block to do an iowrite().

You missed the long email thread we just had about this earlier in the
week...

 It is worth looking to see if the compiler manages to optimise away
 the 'if' test in this case. It might do better if a #define is used
 instead of the inline function.

It shouldn't make a difference, gcc should optimize it away.

thanks,

greg k-h
--
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 2/7] usb: chipidea: add freescale imx28 special write register method

2014-01-10 Thread David Laight 
From: gre...@linuxfoundation.org 
  ...
   +#ifdef
   +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 (ci-imx28_write_fix)
   + imx28_ci_writel(val, addr);
   + else
   + iowrite32(val, addr);
   +}
   +
 
  This code ought to be defensive against 'imx28_write_fix' being
  set when the kernel isn't compiled with CONFIG_SOC_IMX28.
  This can easily be done by changing the #else block to do an iowrite().
 
 You missed the long email thread we just had about this earlier in the
 week...

No - I read most of it, I even commented earlier than the alternate
definition of imx28_ci_writel() should be iowrite32().
 
  It is worth looking to see if the compiler manages to optimise away
  the 'if' test in this case. It might do better if a #define is used
  instead of the inline function.
 
 It shouldn't make a difference, gcc should optimize it away.

I've seen differences between the optimisations performed on
#defines and inline functions.

#defines tend to optimise slightly better - probably because they
get some optimisations before code generation.

inline functions do have better type checking - especially useful
if they contain casts or local variables.

I've also had to use #defines so I can add an asm statement
containing an asm comment that includes __LINE__ in order to stop
gcc tail-merging code (I didn't want the clock cycles taken by
the extra branch instruction).

Getting static branch prediction right can make a significant difference.
Although each branch is only a few clocks they can soon add up.
Protocol stack code can easily be dominated by conditionals,
removing pipeline stalls can easily speed things up by a measurable 10%.

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 2/7] usb: chipidea: add freescale imx28 special write register method

2014-01-10 Thread 'gre...@linuxfoundation.org' 
On Fri, Jan 10, 2014 at 03:09:03PM +, David Laight wrote:
   This code ought to be defensive against 'imx28_write_fix' being
   set when the kernel isn't compiled with CONFIG_SOC_IMX28.
   This can easily be done by changing the #else block to do an iowrite().
  
  You missed the long email thread we just had about this earlier in the
  week...
 
 No - I read most of it, I even commented earlier than the alternate
 definition of imx28_ci_writel() should be iowrite32().

That would work too.

 Getting static branch prediction right can make a significant difference.
 Although each branch is only a few clocks they can soon add up.
 Protocol stack code can easily be dominated by conditionals,
 removing pipeline stalls can easily speed things up by a measurable 10%.

If you can measure a problem with the code as-is, great, I'll be glad to
take a patch, otherwise we are just bike-shedding, and I'm sure the gcc
developers would love a bug report as to how they aren't getting the
static inline empty function logic correct :)

thanks,

greg k-h
--
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

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

2014-01-09 Thread 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

This patch is needed for stable tree 3.11+.

Cc: sta...@vger.kernel.org
Cc: robert.hoda...@digi.com
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 a71dc1c..88b80f7 100644
--- a/drivers/usb/chipidea/ci.h
+++ b/drivers/usb/chipidea/ci.h
@@ -164,6 +164,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;
@@ -202,6 +203,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)
@@ -250,6 +252,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 (ci-imx28_write_fix)
+   imx28_ci_writel(val, addr);
+   else
+   iowrite32(val, addr);
+}
+
 /**
  * hw_write: writes to a hw register
  * @reg:  register index
@@ -263,7 +285,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]);
 }
 
 /**
@@ -278,7 +300,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 458d8e4..0ead0b4 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -548,6 +548,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