Re: [PATCH] USB: ftdi_sio: Added custom PID for CustomWare products

[Matthijs Kooijman]

Hi Lars,

 What I meant is that ...FTDI_C. should come before ...FTDI_S
 and not after, but I've had a look at the whole ftdi_sio.c source
 now and it looks horrible.
 I think it doesn't really matter where you put your additions, do as
 others has done and put them at a random position..
Yeah, I looked at that, the commits I looked at just added new stuff at
the end, and so did I :-)

Gr.

Matthijs


signature.asc
Description: Digital signature

[PATCH] USB: ftdi_sio: Added custom PID for CustomWare products

[Matthijs Kooijman]

CustomWare uses the FTDI VID with custom PIDs for their ShipModul MiniPlex
products.

Signed-off-by: Matthijs Kooijman matth...@stdin.nl
---
 drivers/usb/serial/ftdi_sio.c | 4 
 drivers/usb/serial/ftdi_sio_ids.h | 8 
 2 files changed, 12 insertions(+)

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index 4c8b3b8..a5a0376 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -605,6 +605,10 @@ static const struct usb_device_id id_table_combined[] = {
{ USB_DEVICE(FTDI_VID, FTDI_NT_ORIONLXM_PID),
.driver_info = (kernel_ulong_t)ftdi_jtag_quirk },
{ USB_DEVICE(FTDI_VID, FTDI_SYNAPSE_SS200_PID) },
+   { USB_DEVICE(FTDI_VID, FTDI_CUSTOMWARE_MINIPLEX_PID) },
+   { USB_DEVICE(FTDI_VID, FTDI_CUSTOMWARE_MINIPLEX2_PID) },
+   { USB_DEVICE(FTDI_VID, FTDI_CUSTOMWARE_MINIPLEX2WI_PID) },
+   { USB_DEVICE(FTDI_VID, FTDI_CUSTOMWARE_MINIPLEX3_PID) },
/*
 * ELV devices:
 */
diff --git a/drivers/usb/serial/ftdi_sio_ids.h 
b/drivers/usb/serial/ftdi_sio_ids.h
index 792e054..f5775b8 100644
--- a/drivers/usb/serial/ftdi_sio_ids.h
+++ b/drivers/usb/serial/ftdi_sio_ids.h
@@ -568,6 +568,14 @@
  */
 #define FTDI_SYNAPSE_SS200_PID 0x9090 /* SS200 - SNAP Stick 200 */
 
+/*
+ * CustomWare / ShipModul NMEA multiplexers product ids (FTDI_VID)
+ */
+#define FTDI_CUSTOMWARE_MINIPLEX_PID 0xfd48 /* MiniPlex first generation NMEA 
Multiplexer */
+#define FTDI_CUSTOMWARE_MINIPLEX2_PID 0xfd49 /* MiniPlex-USB and MiniPlex-2 
series */
+#define FTDI_CUSTOMWARE_MINIPLEX2WI_PID 0xfd4a /* MiniPlex-2Wi */
+#define FTDI_CUSTOMWARE_MINIPLEX3_PID 0xfd4b /* MiniPlex-3 series */
+
 
 //
 /** third-party VID/PID combos **/
-- 
2.1.4

--
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] USB: ftdi_sio: Added custom PID for CustomWare products

[Matthijs Kooijman]

Hi Lars,

thanks for your feedback.

 +{ USB_DEVICE(FTDI_VID, FTDI_CUSTOMWARE_MINIPLEX_PID) },
 +{ USB_DEVICE(FTDI_VID, FTDI_CUSTOMWARE_MINIPLEX2_PID) },
 +{ USB_DEVICE(FTDI_VID, FTDI_CUSTOMWARE_MINIPLEX2WI_PID) },
 +{ USB_DEVICE(FTDI_VID, FTDI_CUSTOMWARE_MINIPLEX3_PID) },
 
 Please sort in alphabetic order.
Do you mean to sort these four entries relative to each other? They
already look sorted to me (though if you insist on using ASCII values
for sorting, _ would sort after 2, 3 and W and the order would somewhat
change). Or do you mean to move all four entries to a different place in
the file?

Gr.

Matthijs


signature.asc
Description: Digital signature

Re: [RFC] Does PHY UTMI data width belong to DWC2 or PHY binding?

[Matthijs Kooijman]

Hi Kishon,

On Mon, Oct 21, 2013 at 02:57:26PM +0530, Kishon Vijay Abraham I wrote:
 I think it makes sense to keep the data width property in the dwc2 node 
 itself.
 I mean it describes how the dwc2 IP is configured in that particular SoC 
 (given
 that it can be either 8 or 16).
If I'm reading the RT3052 datasheet correctly (GHWCFG4 register), the IP
can be configured for 8, 16 or 8 _and_ 16. In the latter case, the 8
and 16 supported would make sense as a property of dwc2 (though this
value should be autodetectable through GHWCFG4), while the actual 8 or
16 supported by the PHY would make sense as property of a phy.

Note sure if this is really useful in practice as well, or if just
setting the actual width to use on dwc2 makes more sense...

Gr.

Matthijs


signature.asc
Description: Digital signature

Re: [PATCH] staging: dwc2: Make dwc2_hw_params.host_channels large enough

[Matthijs Kooijman]

Hi Paul,

 By the way, it looks like 'num_dev_ep' would have the same problem,
I don't think so, since the hardware doesn't do the off-by-one trick
there (presumably because having 0 endpoints make sense, but 0 host
channels doesn't):

hw-num_dev_ep = (hwcfg2  GHWCFG2_NUM_DEV_EP_MASK) 
 GHWCFG2_NUM_DEV_EP_SHIFT;

 except it is not used because we don't support device mode yet.
 That one and also 'num_dev_perio_in_ep' and 'dev_token_q_depth' should
 be removed, I think.
Doesn't hurt to keep them around, I think? When device mode is
implemented, I guess this code will need to present exactly like it is
now (unlike other device mode code, which likely needs to be adapted to
whatever s3-hsotg is doing now)?

Gr.

Matthijs


signature.asc
Description: Digital signature

[PATCH] staging: dwc2: Make dwc2_hw_params.host_channels large enough

[Matthijs Kooijman]

The hardware offers a 4-bit register containing the number of host
channels. However, the values of these register mean 1-16 host channels,
not 0-15. Since the dwc2_hw_params struct stores the actual number of
host channels supported instead of the raw register value, it should be
5 bits wide instead of 4.

Before this commit, hardware with 16 host channels would overflow the
field, making it appear as 0 channels.

This bug was introduced in commit 9badec2 (staging: dwc2: interpret all
hwcfg and related register at init time).

Reported-by: Dinh Nguyen dingu...@altera.com
Signed-off-by: Matthijs Kooijman matth...@stdin.nl
Acked-by: Paul Zimmerman pa...@synopsys.com
---

This is exactly the same patch as before, but with updated tags in the
commit message.

 drivers/staging/dwc2/core.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/dwc2/core.h b/drivers/staging/dwc2/core.h
index f7ba34b..fab718d 100644
--- a/drivers/staging/dwc2/core.h
+++ b/drivers/staging/dwc2/core.h
@@ -294,7 +294,7 @@ struct dwc2_hw_params {
unsigned dev_token_q_depth:5;
unsigned max_transfer_size:26;
unsigned max_packet_count:11;
-   unsigned host_channels:4;
+   unsigned host_channels:5;
unsigned hs_phy_type:2;
unsigned fs_phy_type:2;
unsigned i2c_enable:1;
-- 
1.8.4.rc1

--
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] staging: dwc2: Make dwc2_hw_params.host_channels large enough

[Matthijs Kooijman]

Hey Dinh,

 Reported-by: Dinh Nguyen dinh.li...@gmail.com
 Can you please use:
 Dinh Nguyen dingu...@altera.com
Sorry, I just used your sender address, should have checked your patch
instead.

Paul, if you can ack this patch, I'll resend it with the proper tag and
include your acked-by as well.

Gr.

Matthijs


signature.asc
Description: Digital signature

Re: [PATCHv2] staging: dwc2: Fix code that gets the nummber of host channels

[Matthijs Kooijman]

On Tue, Oct 01, 2013 at 10:05:17AM +0300, Dan Carpenter wrote:
 On Tue, Oct 01, 2013 at 01:21:28AM +, Paul Zimmerman wrote:
   From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
   Sent: Monday, September 30, 2013 6:09 PM
   
   Yeah.  I guess it's fine...  I was going to suggest adding the + 1 in a
   different place but actually it doesn't matter.
   
   The key to understanding dwc2_set_param_host_channels() is that the
   val parameter is always -1.  That means it always returns -EINVAL and
   the caller jumbles the error code in with some other error codes and
   then ignores any errors.  :/
  
  The intent of this was that the value can be overridden by the platform
  code if required, in which case val would not be -1. However, to my
  knowledge, no in-tree platforms do that, so I guess it would be fine to
  redo this as you suggest below. We can always add it back if needed.
If we redo the dwc2_set_param_host_channels function, we should probably
redo _all_ of the dwc2_set_param_* functions, since they all do this.

 Why are we even adding one to the number of channels that the hardware
 reports?
Because that's how the hardware register is defined. I presume it never
makes sense to have 0 host channels, this allows a range of 1-16 instead
of 0-15.

In any case, for this particular problem, I would think that simply
making the host_channels field in dcw2_hw_params bigger is the better solution
here. E.g., something like:

 struct dwc2_hw_params {
 ...
-unsigned host_channels:4;
+unsigned host_channels:5;
 }

Moving the +1 calculation reverts the code back to what it was before one of my
patches. I moved the +1 to this place, so that the off-by-one detail of
the hardware register is only known at the place where hardware
registers are read. All other code can simply assume that the
host_channels variable contains just that: the number of host channels
present.

However, in that patch I apparently chose the wrong size for the
host_channels field, which I think should be problem to fix here.

Gr.

Matthijs


signature.asc
Description: Digital signature

Re: [PATCHv2] staging: dwc2: Fix code that gets the nummber of host channels

[Matthijs Kooijman]

Hi Dinh,

 Somehow I assumed that was fixed by the hardware, but I see now that you
 are right.  Yes, making the definition larger is better than moving the
 + 1.
 This was my original fix to the problem, but I thought that it would
 be confusing when reading the code. I also thought about the +1
 for host_channels was strange too. For debug outputs, it would be
 more accurate to display 16 channels, in code-wise, I see that
 host_channels is used in 2 for loops. Does it make sense to just fix
 the for loops to include channels 0-15?
I think that fixing this in the places where the value is used is moving
the complexity the wrong way. Not sure if I'm understanding you
correctly, thoguh.

 Dinh, do you want to do that?  The other option is that Matthijs could
 fix it and give you the Reported-by credit.
 I'm fine with that, if Matthijs wants to submit the fix. I can test
 it on my hardware too.
I'll prepare a patch in a few hours. Would be great if you could test,
since my hardware only has a meagre 4 host channels ;-)

Gr.

Matthijs


signature.asc
Description: Digital signature

Re: [PATCHv2] staging: dwc2: Fix code that gets the nummber of host channels

[Matthijs Kooijman]

Hey Dan, Dinh,

  [resend]: previous reply didn't include Matthijs
  
 
 He sets his Mail-Followup-To: so that we don't CC him on replies.  I
 assume it's deliberate because he only wants the copy from the mailing
 list?
Exactly, I just set that for whatever mailing list I subscribe to.
However, since it's customary to explicitely CC involved people on the
kernel mailing lists, I've just disabled this behaviour for the kernel
lists, to prevent further confusion :-)

Gr.

Matthijs


signature.asc
Description: Digital signature

[PATCH] staging: dwc2: Make dwc2_hw_params.host_channels large enough

[Matthijs Kooijman]

The hardware offers a 4-bit register containing the number of host
channels. However, the values of these register mean 1-16 host channels,
not 0-15. Since the dwc2_hw_params struct stores the actual number of
host channels supported instead of the raw register value, it should be
5 bits wide instead of 4.

Before this commit, hardware with 16 host channels would overflow the
field, making it appear as 0 channels.

This bug was introduced in commit 9badec2 (staging: dwc2: interpret all
hwcfg and related register at init time).

Reported-by: Dinh Nguyen dinh.li...@gmail.com
Signed-off-by: Matthijs Kooijman matth...@stdin.nl
---
 drivers/staging/dwc2/core.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/dwc2/core.h b/drivers/staging/dwc2/core.h
index f7ba34b..fab718d 100644
--- a/drivers/staging/dwc2/core.h
+++ b/drivers/staging/dwc2/core.h
@@ -294,7 +294,7 @@ struct dwc2_hw_params {
unsigned dev_token_q_depth:5;
unsigned max_transfer_size:26;
unsigned max_packet_count:11;
-   unsigned host_channels:4;
+   unsigned host_channels:5;
unsigned hs_phy_type:2;
unsigned fs_phy_type:2;
unsigned i2c_enable:1;
-- 
1.8.4.rc1

--
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 v3 1/2] staging: dwc2: validate urb-actual_length for OUT endpoints

[Matthijs Kooijman]

Hi folks,

On Tue, Sep 24, 2013 at 11:08:53AM -0500, Felipe Balbi wrote:
   On Mon, Sep 23, 2013 at 02:23:33PM -0700, Paul Zimmerman wrote:
+   if ((urb-actual_length  0 || urb-actual_length  
urb-length) 
+   !dwc2_hcd_is_pipe_in(urb-pipe_info))
+   urb-actual_length = urb-length;
 
  I assume it was to fix some issue seen by our developers. We don't have
  detailed commit logs for that driver, so I am unable to say for certain.
  But since it is part of the downstream RaspberryPi driver, which has
  seen a lot of testing, I thought it best to include it.
 
 Maybe RPi folks can give some details ? Because I really wonder if we'd
 not be masking a bug somewhere else and this would make it a lot harder
 to pin-point that bug.

Agreed. Perhaps there should be a message printed or a WARN() there so
we can pinpoint in which cases this case actually occurs?

Paul, did you see if this case actually triggers with your hardware?

Gr.

Matthijs


signature.asc
Description: Digital signature

[PATCH V2 13/13] staging: dwc2: make dwc2_core_params documentation more complete

[Matthijs Kooijman]

Some of the defaults were missing or unclear. In particular, I suspect
the defaults were documented assuming there were still module parameters
and taking the default module parameters into account. Now, the defaults
are the values that will get chosen when the params passed to
dwc2_hcd_init are all -1.

Signed-off-by: Matthijs Kooijman matth...@stdin.nl
Acked-by: Paul Zimmerman pa...@synopsys.com

---
v2: Rebase on top of Paul's ahbcfg patch and documentation re-ordering patch
Fix the documentation for the ahbcfg parameter
---
 drivers/staging/dwc2/core.h | 85 +++--
 1 file changed, 59 insertions(+), 26 deletions(-)

diff --git a/drivers/staging/dwc2/core.h b/drivers/staging/dwc2/core.h
index ecfcad8..9102f66 100644
--- a/drivers/staging/dwc2/core.h
+++ b/drivers/staging/dwc2/core.h
@@ -68,19 +68,18 @@ enum dwc2_lx_state {
 /**
  * struct dwc2_core_params - Parameters for configuring the core
  *
- * @otg_cap:Specifies the OTG capabilities. The driver will
- *  automatically detect the value for this parameter if
- *  none is specified.
- *   0 - HNP and SRP capable (default)
+ * @otg_cap:Specifies the OTG capabilities.
+ *   0 - HNP and SRP capable
  *   1 - SRP Only capable
- *   2 - No HNP/SRP capable
+ *   2 - No HNP/SRP capable (always available)
+ *  Defaults to best available option (0, 1, then 2)
  * @otg_ver:OTG version supported
- *   0 - 1.3
+ *   0 - 1.3 (default)
  *   1 - 2.0
  * @dma_enable: Specifies whether to use slave or DMA mode for 
accessing
  *  the data FIFOs. The driver will automatically detect 
the
  *  value for this parameter if none is specified.
- *   0 - Slave
+ *   0 - Slave (always available)
  *   1 - DMA (default, if available)
  * @dma_desc_enable:When DMA mode is enabled, specifies whether to use
  *  address DMA mode or descriptor DMA mode for accessing
@@ -91,29 +90,47 @@ enum dwc2_lx_state {
  * @speed:  Specifies the maximum speed of operation in host and
  *  device mode. The actual speed depends on the speed of
  *  the attached device and the value of phy_type.
- *   0 - High Speed (default)
+ *   0 - High Speed
+ *   (default when phy_type is UTMI+ or ULPI)
  *   1 - Full Speed
+ *   (default when phy_type is Full Speed)
  * @enable_dynamic_fifo: 0 - Use coreConsultant-specified FIFO size parameters
- *   1 - Allow dynamic FIFO sizing (default)
+ *   1 - Allow dynamic FIFO sizing (default, if available)
  * @en_multiple_tx_fifo: Specifies whether dedicated per-endpoint transmit 
FIFOs
  *  are enabled
  * @host_rx_fifo_size:  Number of 4-byte words in the Rx FIFO in host mode when
  *  dynamic FIFO sizing is enabled
- *   16 to 32768 (default 1024)
+ *   16 to 32768
+ *  Actual maximum value is autodetected and also
+ *  the default.
  * @host_nperio_tx_fifo_size: Number of 4-byte words in the non-periodic Tx 
FIFO
  *  in host mode when dynamic FIFO sizing is enabled
- *   16 to 32768 (default 1024)
+ *   16 to 32768
+ *  Actual maximum value is autodetected and also
+ *  the default.
  * @host_perio_tx_fifo_size: Number of 4-byte words in the periodic Tx FIFO in
  *  host mode when dynamic FIFO sizing is enabled
- *   16 to 32768 (default 1024)
+ *   16 to 32768
+ *  Actual maximum value is autodetected and also
+ *  the default.
  * @max_transfer_size:  The maximum transfer size supported, in bytes
- *   2047 to 65,535 (default 65,535)
+ *   2047 to 65,535
+ *  Actual maximum value is autodetected and also
+ *  the default.
  * @max_packet_count:   The maximum number of packets in a transfer
- *   15 to 511 (default 511)
+ *   15 to 511
+ *  Actual maximum value is autodetected and also
+ *  the default.
  * @host_channels:  The number of host channel registers to use
- *   1 to 16 (default 12)
+ *   1 to 16
+ *  Actual maximum value is autodetected and also
+ *  the default

[PATCH V2 05/13] staging: dwc2: simplify register shift expressions

[Matthijs Kooijman]

This commit changes expressions from (val  shift)  (mask  shift) to
(val  mask)  shift.

Signed-off-by: Matthijs Kooijman matth...@stdin.nl
Acked-by: Paul Zimmerman pa...@synopsys.com
---
 drivers/staging/dwc2/core.c | 20 ++---
 drivers/staging/dwc2/hcd.c  | 62 +++--
 drivers/staging/dwc2/hcd_ddma.c |  8 +++---
 drivers/staging/dwc2/hcd_intr.c | 22 ++-
 4 files changed, 50 insertions(+), 62 deletions(-)

diff --git a/drivers/staging/dwc2/core.c b/drivers/staging/dwc2/core.c
index 9eabebb..04c251c 100644
--- a/drivers/staging/dwc2/core.c
+++ b/drivers/staging/dwc2/core.c
@@ -1386,14 +1386,14 @@ void dwc2_hc_start_transfer(struct dwc2_hsotg *hsotg,
dev_vdbg(hsotg-dev, %s: Channel %d\n, __func__,
 chan-hc_num);
dev_vdbg(hsotg-dev,Xfer Size: %d\n,
-hctsiz  TSIZ_XFERSIZE_SHIFT 
-TSIZ_XFERSIZE_MASK  TSIZ_XFERSIZE_SHIFT);
+(hctsiz  TSIZ_XFERSIZE_MASK) 
+TSIZ_XFERSIZE_SHIFT);
dev_vdbg(hsotg-dev,Num Pkts: %d\n,
-hctsiz  TSIZ_PKTCNT_SHIFT 
-TSIZ_PKTCNT_MASK  TSIZ_PKTCNT_SHIFT);
+(hctsiz  TSIZ_PKTCNT_MASK) 
+TSIZ_PKTCNT_SHIFT);
dev_vdbg(hsotg-dev,Start PID: %d\n,
-hctsiz  TSIZ_SC_MC_PID_SHIFT 
-TSIZ_SC_MC_PID_MASK  TSIZ_SC_MC_PID_SHIFT);
+(hctsiz  TSIZ_SC_MC_PID_MASK) 
+TSIZ_SC_MC_PID_SHIFT);
}
 
if (hsotg-core_params-dma_enable  0) {
@@ -1437,8 +1437,8 @@ void dwc2_hc_start_transfer(struct dwc2_hsotg *hsotg,
 
if (dbg_hc(chan))
dev_vdbg(hsotg-dev,Multi Cnt: %d\n,
-hcchar  HCCHAR_MULTICNT_SHIFT 
-HCCHAR_MULTICNT_MASK  HCCHAR_MULTICNT_SHIFT);
+(hcchar  HCCHAR_MULTICNT_MASK) 
+HCCHAR_MULTICNT_SHIFT);
 
writel(hcchar, hsotg-regs + HCCHAR(chan-hc_num));
if (dbg_hc(chan))
@@ -1526,8 +1526,8 @@ void dwc2_hc_start_transfer_ddma(struct dwc2_hsotg *hsotg,
 
if (dbg_hc(chan))
dev_vdbg(hsotg-dev,Multi Cnt: %d\n,
-hcchar  HCCHAR_MULTICNT_SHIFT 
-HCCHAR_MULTICNT_MASK  HCCHAR_MULTICNT_SHIFT);
+(hcchar  HCCHAR_MULTICNT_MASK) 
+HCCHAR_MULTICNT_SHIFT);
 
writel(hcchar, hsotg-regs + HCCHAR(chan-hc_num));
if (dbg_hc(chan))
diff --git a/drivers/staging/dwc2/hcd.c b/drivers/staging/dwc2/hcd.c
index d488304..f11b4f0 100644
--- a/drivers/staging/dwc2/hcd.c
+++ b/drivers/staging/dwc2/hcd.c
@@ -1005,10 +1005,10 @@ static void dwc2_process_periodic_channels(struct 
dwc2_hsotg *hsotg)
dev_vdbg(hsotg-dev, Queue periodic transactions\n);
 
tx_status = readl(hsotg-regs + HPTXSTS);
-   qspcavail = tx_status  TXSTS_QSPCAVAIL_SHIFT 
-   TXSTS_QSPCAVAIL_MASK  TXSTS_QSPCAVAIL_SHIFT;
-   fspcavail = tx_status  TXSTS_FSPCAVAIL_SHIFT 
-   TXSTS_FSPCAVAIL_MASK  TXSTS_FSPCAVAIL_SHIFT;
+   qspcavail = (tx_status  TXSTS_QSPCAVAIL_MASK) 
+   TXSTS_QSPCAVAIL_SHIFT;
+   fspcavail = (tx_status  TXSTS_FSPCAVAIL_MASK) 
+   TXSTS_FSPCAVAIL_SHIFT;
 
if (dbg_perio()) {
dev_vdbg(hsotg-dev,   P Tx Req Queue Space Avail (before 
queue): %d\n,
@@ -1046,8 +1046,8 @@ static void dwc2_process_periodic_channels(struct 
dwc2_hsotg *hsotg)
qh-channel-multi_count  1)
hsotg-queuing_high_bandwidth = 1;
 
-   fspcavail = tx_status  TXSTS_FSPCAVAIL_SHIFT 
-   TXSTS_FSPCAVAIL_MASK  TXSTS_FSPCAVAIL_SHIFT;
+   fspcavail = (tx_status  TXSTS_FSPCAVAIL_MASK) 
+   TXSTS_FSPCAVAIL_SHIFT;
status = dwc2_queue_transaction(hsotg, qh-channel, fspcavail);
if (status  0) {
no_fifo_space = 1;
@@ -1078,10 +1078,10 @@ static void dwc2_process_periodic_channels(struct 
dwc2_hsotg *hsotg)
 
if (hsotg-core_params-dma_enable = 0) {
tx_status = readl(hsotg-regs + HPTXSTS);
-   qspcavail = tx_status  TXSTS_QSPCAVAIL_SHIFT 
-   TXSTS_QSPCAVAIL_MASK  TXSTS_QSPCAVAIL_SHIFT;
-   fspcavail = tx_status  TXSTS_FSPCAVAIL_SHIFT 
-   TXSTS_FSPCAVAIL_MASK  TXSTS_FSPCAVAIL_SHIFT;
+   qspcavail = (tx_status  TXSTS_QSPCAVAIL_MASK) 
+   TXSTS_QSPCAVAIL_SHIFT;
+   fspcavail = (tx_status  TXSTS_FSPCAVAIL_MASK) 
+   TXSTS_FSPCAVAIL_SHIFT;
if (dbg_perio()) {
dev_vdbg(hsotg-dev

[PATCH V2 06/13] staging: dwc2: add missing shift

[Matthijs Kooijman]

This line extracted the available queue space without properly shifting
it. Since the code only cared wether it was zero or not, it worked as
expected without the shift, but adding shift makes the code cleaner.

While we're here, store the result in a helper variable that was already
declared to increase readability a bit more.

Signed-off-by: Matthijs Kooijman matth...@stdin.nl
Acked-by: Paul Zimmerman pa...@synopsys.com
---
 drivers/staging/dwc2/hcd.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/dwc2/hcd.c b/drivers/staging/dwc2/hcd.c
index f11b4f0..9a1e062 100644
--- a/drivers/staging/dwc2/hcd.c
+++ b/drivers/staging/dwc2/hcd.c
@@ -1020,7 +1020,9 @@ static void dwc2_process_periodic_channels(struct 
dwc2_hsotg *hsotg)
qh_ptr = hsotg-periodic_sched_assigned.next;
while (qh_ptr != hsotg-periodic_sched_assigned) {
tx_status = readl(hsotg-regs + HPTXSTS);
-   if ((tx_status  TXSTS_QSPCAVAIL_MASK) == 0) {
+   qspcavail = (tx_status  TXSTS_QSPCAVAIL_MASK) 
+   TXSTS_QSPCAVAIL_SHIFT;
+   if (qspcavail == 0) {
no_queue_space = 1;
break;
}
-- 
1.8.4.rc1

--
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 V2 02/13] staging: dwc2: fix off-by-one in check for max_packet_count parameter

[Matthijs Kooijman]

Previously, the max_packet_count could be set to 1  x, where x is the
number of bits available (width + 4 in the code). Since 1  x requires
x + 1 bits to represent, this will not work. The real maximum value is
(1  x) - 1. This value is already used the default when the set value
is invalid, but the upper limit for the set value was off-by-one.

This change makes the check the same as the one for max_transfer_size,
which was already correct.

Signed-off-by: Matthijs Kooijman matth...@stdin.nl
Acked-by: Paul Zimmerman pa...@synopsys.com
---
 drivers/staging/dwc2/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/dwc2/core.c b/drivers/staging/dwc2/core.c
index a090f79..5f09f47 100644
--- a/drivers/staging/dwc2/core.c
+++ b/drivers/staging/dwc2/core.c
@@ -2209,7 +2209,7 @@ int dwc2_set_param_max_packet_count(struct dwc2_hsotg 
*hsotg, int val)
GHWCFG3_PACKET_SIZE_CNTR_WIDTH_MASK 
GHWCFG3_PACKET_SIZE_CNTR_WIDTH_SHIFT;
 
-   if (val  15 || val  (1  (width + 4)))
+   if (val  15 || val = (1  (width + 4)))
valid = 0;
 
if (!valid) {
-- 
1.8.4.rc1

--
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 V2 10/13] staging: dwc2: properly mask the GRXFSIZ register

[Matthijs Kooijman]

Bits 16-31 are reserved, so the old code just reads the whole register to
get bits 0-15, assuming the reserved bits would be 0 (which seems true
on current hardware, but who knows...).

This commit properly masks out the reserved bits when reading and
doesn't touch the reserved bits while writing.

Signed-off-by: Matthijs Kooijman matth...@stdin.nl
Acked-by: Paul Zimmerman pa...@synopsys.com
---
 drivers/staging/dwc2/core.c | 23 ---
 drivers/staging/dwc2/hw.h   |  2 ++
 2 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/dwc2/core.c b/drivers/staging/dwc2/core.c
index f6494df..2f84c24 100644
--- a/drivers/staging/dwc2/core.c
+++ b/drivers/staging/dwc2/core.c
@@ -402,7 +402,9 @@ int dwc2_core_init(struct dwc2_hsotg *hsotg, bool 
select_phy, int irq)
 
hsotg-total_fifo_size = hsotg-hwcfg3  GHWCFG3_DFIFO_DEPTH_SHIFT 
GHWCFG3_DFIFO_DEPTH_MASK  GHWCFG3_DFIFO_DEPTH_SHIFT;
-   hsotg-rx_fifo_size = readl(hsotg-regs + GRXFSIZ);
+   hsotg-rx_fifo_size = (readl(hsotg-regs + GRXFSIZ) 
+  GRXFSIZ_DEPTH_MASK) 
+ GRXFSIZ_DEPTH_SHIFT;
hsotg-nperio_tx_fifo_size =
readl(hsotg-regs + GNPTXFSIZ)  16  0x;
 
@@ -507,7 +509,7 @@ void dwc2_disable_host_interrupts(struct dwc2_hsotg *hsotg)
 static void dwc2_config_fifos(struct dwc2_hsotg *hsotg)
 {
struct dwc2_core_params *params = hsotg-core_params;
-   u32 nptxfsiz, hptxfsiz, dfifocfg;
+   u32 nptxfsiz, hptxfsiz, dfifocfg, grxfsiz;
 
if (!params-enable_dynamic_fifo)
return;
@@ -520,9 +522,12 @@ static void dwc2_config_fifos(struct dwc2_hsotg *hsotg)
params-host_perio_tx_fifo_size);
 
/* Rx FIFO */
-   dev_dbg(hsotg-dev, initial grxfsiz=%08x\n,
-   readl(hsotg-regs + GRXFSIZ));
-   writel(params-host_rx_fifo_size, hsotg-regs + GRXFSIZ);
+   grxfsiz = readl(hsotg-regs + GRXFSIZ);
+   dev_dbg(hsotg-dev, initial grxfsiz=%08x\n, grxfsiz);
+   grxfsiz = ~GRXFSIZ_DEPTH_MASK;
+   grxfsiz |= params-host_rx_fifo_size 
+  GRXFSIZ_DEPTH_SHIFT  GRXFSIZ_DEPTH_MASK;
+   writel(grxfsiz, hsotg-regs + GRXFSIZ);
dev_dbg(hsotg-dev, new grxfsiz=%08x\n, readl(hsotg-regs + GRXFSIZ));
 
/* Non-periodic Tx FIFO */
@@ -2115,7 +2120,9 @@ int dwc2_set_param_host_rx_fifo_size(struct dwc2_hsotg 
*hsotg, int val)
int valid = 1;
int retval = 0;
 
-   if (val  16 || val  readl(hsotg-regs + GRXFSIZ))
+   if (val  16 || val  (readl(hsotg-regs + GRXFSIZ) 
+  GRXFSIZ_DEPTH_MASK) 
+ GRXFSIZ_DEPTH_SHIFT)
valid = 0;
 
if (!valid) {
@@ -2123,7 +2130,9 @@ int dwc2_set_param_host_rx_fifo_size(struct dwc2_hsotg 
*hsotg, int val)
dev_err(hsotg-dev,
%d invalid for host_rx_fifo_size. Check HW 
configuration.\n,
val);
-   val = readl(hsotg-regs + GRXFSIZ);
+   val = (readl(hsotg-regs + GRXFSIZ) 
+  GRXFSIZ_DEPTH_MASK) 
+ GRXFSIZ_DEPTH_SHIFT;
dev_dbg(hsotg-dev, Setting host_rx_fifo_size to %d\n, val);
retval = -EINVAL;
}
diff --git a/drivers/staging/dwc2/hw.h b/drivers/staging/dwc2/hw.h
index 321d071b..fafdc19 100644
--- a/drivers/staging/dwc2/hw.h
+++ b/drivers/staging/dwc2/hw.h
@@ -188,6 +188,8 @@
 #define GRXSTS_EPNUM_SHIFT 0
 
 #define GRXFSIZHSOTG_REG(0x024)
+#define GRXFSIZ_DEPTH_MASK (0x  0)
+#define GRXFSIZ_DEPTH_SHIFT0
 
 #define GNPTXFSIZ  HSOTG_REG(0x028)
 /* Use FIFOSIZE_* constants to access this register */
-- 
1.8.4.rc1

--
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 V2 11/13] staging: dwc2: interpret all hwcfg and related register at init time

[Matthijs Kooijman]

Before, the hwcfg registers were read at device init time, but
interpreted at various parts in the code. This commit unpacks the hwcfg
register values into a struct with properly labeled variables at init
time, which makes all the other code using these values more consise and
easier to read. Some values that were previously stored in the hsotg
struct are now moved into this new struct as well.

In addition to the hwcfg registers, the contents of some fifo size
registers are also unpacked. The hwcfg registers are read-only, so they
can be safely stored. The fifo size registers are read-write registers,
but their power-on values are significant: they give the maximum depth
of the fifo they describe.

This commit mostly moves code, but also attempts to simplify some
expressions from (val  shift)  (mask  shift) to
(val  mask)  shift.

Finally, all of the parameters read from the hardware are debug printed
after unpacking them, so a bunch of debug prints can be removed from
other places.

Signed-off-by: Matthijs Kooijman matth...@stdin.nl
Acked-by: Paul Zimmerman pa...@synopsys.com

---
v2: Update to use FIFOSIZE_* constants
Use u32 instead of unsigned for temporary variables
Move declaration to fix warning

---
 drivers/staging/dwc2/core.c  | 307 +--
 drivers/staging/dwc2/core.h  |  96 +---
 drivers/staging/dwc2/core_intr.c |   4 +-
 drivers/staging/dwc2/hcd.c   |  80 ++
 drivers/staging/dwc2/hcd.h   |   1 +
 drivers/staging/dwc2/hcd_intr.c  |   2 +-
 6 files changed, 290 insertions(+), 200 deletions(-)

diff --git a/drivers/staging/dwc2/core.c b/drivers/staging/dwc2/core.c
index 2f84c24..825c5e5 100644
--- a/drivers/staging/dwc2/core.c
+++ b/drivers/staging/dwc2/core.c
@@ -90,14 +90,10 @@ static void dwc2_enable_common_interrupts(struct dwc2_hsotg 
*hsotg)
  */
 static void dwc2_init_fs_ls_pclk_sel(struct dwc2_hsotg *hsotg)
 {
-   u32 hs_phy_type = (hsotg-hwcfg2  GHWCFG2_HS_PHY_TYPE_MASK) 
- GHWCFG2_HS_PHY_TYPE_SHIFT;
-   u32 fs_phy_type = (hsotg-hwcfg2  GHWCFG2_FS_PHY_TYPE_MASK) 
- GHWCFG2_FS_PHY_TYPE_SHIFT;
u32 hcfg, val;
 
-   if ((hs_phy_type == GHWCFG2_HS_PHY_TYPE_ULPI 
-fs_phy_type == GHWCFG2_FS_PHY_TYPE_DEDICATED 
+   if ((hsotg-hw_params.hs_phy_type == GHWCFG2_HS_PHY_TYPE_ULPI 
+hsotg-hw_params.fs_phy_type == GHWCFG2_FS_PHY_TYPE_DEDICATED 
 hsotg-core_params-ulpi_fs_ls  0) ||
hsotg-core_params-phy_type == DWC2_PHY_TYPE_PARAM_FS) {
/* Full speed PHY */
@@ -247,7 +243,7 @@ static void dwc2_hs_phy_init(struct dwc2_hsotg *hsotg, bool 
select_phy)
 
 static void dwc2_phy_init(struct dwc2_hsotg *hsotg, bool select_phy)
 {
-   u32 usbcfg, hs_phy_type, fs_phy_type;
+   u32 usbcfg;
 
if (hsotg-core_params-speed == DWC2_SPEED_PARAM_FULL 
hsotg-core_params-phy_type == DWC2_PHY_TYPE_PARAM_FS) {
@@ -258,13 +254,8 @@ static void dwc2_phy_init(struct dwc2_hsotg *hsotg, bool 
select_phy)
dwc2_hs_phy_init(hsotg, select_phy);
}
 
-   hs_phy_type = (hsotg-hwcfg2  GHWCFG2_HS_PHY_TYPE_MASK) 
- GHWCFG2_HS_PHY_TYPE_SHIFT;
-   fs_phy_type = (hsotg-hwcfg2  GHWCFG2_FS_PHY_TYPE_MASK) 
- GHWCFG2_FS_PHY_TYPE_SHIFT;
-
-   if (hs_phy_type == GHWCFG2_HS_PHY_TYPE_ULPI 
-   fs_phy_type == GHWCFG2_FS_PHY_TYPE_DEDICATED 
+   if (hsotg-hw_params.hs_phy_type == GHWCFG2_HS_PHY_TYPE_ULPI 
+   hsotg-hw_params.fs_phy_type == GHWCFG2_FS_PHY_TYPE_DEDICATED 
hsotg-core_params-ulpi_fs_ls  0) {
dev_dbg(hsotg-dev, Setting ULPI FSLS\n);
usbcfg = readl(hsotg-regs + GUSBCFG);
@@ -283,8 +274,7 @@ static int dwc2_gahbcfg_init(struct dwc2_hsotg *hsotg)
 {
u32 ahbcfg = readl(hsotg-regs + GAHBCFG);
 
-   switch ((hsotg-hwcfg2  GHWCFG2_ARCHITECTURE_MASK) 
-   GHWCFG2_ARCHITECTURE_SHIFT) {
+   switch (hsotg-hw_params.arch) {
case GHWCFG2_EXT_DMA_ARCH:
dev_err(hsotg-dev, External DMA Mode not supported\n);
return -EINVAL;
@@ -333,8 +323,7 @@ static void dwc2_gusbcfg_init(struct dwc2_hsotg *hsotg)
usbcfg = readl(hsotg-regs + GUSBCFG);
usbcfg = ~(GUSBCFG_HNPCAP | GUSBCFG_SRPCAP);
 
-   switch ((hsotg-hwcfg2  GHWCFG2_OP_MODE_MASK) 
-   GHWCFG2_OP_MODE_SHIFT) {
+   switch (hsotg-hw_params.op_mode) {
case GHWCFG2_OP_MODE_HNP_SRP_CAPABLE:
if (hsotg-core_params-otg_cap ==
DWC2_CAP_PARAM_HNP_SRP_CAPABLE)
@@ -395,23 +384,6 @@ int dwc2_core_init(struct dwc2_hsotg *hsotg, bool 
select_phy, int irq)
/* Reset the Controller */
dwc2_core_reset(hsotg);
 
-   dev_dbg(hsotg-dev, num_dev_perio_in_ep=%d\n,
-   hsotg-hwcfg4  GHWCFG4_NUM_DEV_PERIO_IN_EP_SHIFT 
-   GHWCFG4_NUM_DEV_PERIO_IN_EP_MASK

[PATCH V2 09/13] staging: dwc2: remove redundant register reads

[Matthijs Kooijman]

For calculating FIFO offsets, the sizes of preceding fifos need to be
known. For filling the GDFIFOCFG register, these fifo sizes were read
from hardware registers. However, these values were written to these
registers just a few lines before, so we can just use the values written
instead.

Signed-off-by: Matthijs Kooijman matth...@stdin.nl
Acked-by: Paul Zimmerman pa...@synopsys.com
---
 drivers/staging/dwc2/core.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/dwc2/core.c b/drivers/staging/dwc2/core.c
index 5799f47..f6494df 100644
--- a/drivers/staging/dwc2/core.c
+++ b/drivers/staging/dwc2/core.c
@@ -507,7 +507,7 @@ void dwc2_disable_host_interrupts(struct dwc2_hsotg *hsotg)
 static void dwc2_config_fifos(struct dwc2_hsotg *hsotg)
 {
struct dwc2_core_params *params = hsotg-core_params;
-   u32 rxfsiz, nptxfsiz, hptxfsiz, dfifocfg;
+   u32 nptxfsiz, hptxfsiz, dfifocfg;
 
if (!params-enable_dynamic_fifo)
return;
@@ -555,11 +555,10 @@ static void dwc2_config_fifos(struct dwc2_hsotg *hsotg)
 * include RxFIFO, NPTXFIFO and HPTXFIFO
 */
dfifocfg = readl(hsotg-regs + GDFIFOCFG);
-   rxfsiz = readl(hsotg-regs + GRXFSIZ)  0x;
-   nptxfsiz = readl(hsotg-regs + GNPTXFSIZ)  16  0x;
-   hptxfsiz = readl(hsotg-regs + HPTXFSIZ)  16  0x;
dfifocfg = ~GDFIFOCFG_EPINFOBASE_MASK;
-   dfifocfg |= (rxfsiz + nptxfsiz + hptxfsiz) 
+   dfifocfg |= (params-host_rx_fifo_size +
+params-host_nperio_tx_fifo_size +
+params-host_perio_tx_fifo_size) 
GDFIFOCFG_EPINFOBASE_SHIFT 
GDFIFOCFG_EPINFOBASE_MASK;
writel(dfifocfg, hsotg-regs + GDFIFOCFG);
-- 
1.8.4.rc1

--
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 V2 01/13] staging: dwc2: remove specific fifo size constants

[Matthijs Kooijman]

A generic set of FIFOSIZE_* constants is defined which applies to all
fifo size and offset registers. It is already used for both the
GNPTXFSIZ and HPTXFSIZ registers, but it applies to DPTXFSIZN as well.
Some of these also had specific constants defined. This patch removes
the specific constants and documents to use the generic constants.

Note that the removed constants weren't actually used. Instead, most of
the related code uses hardcoded masks and shifts. But given that
subsequent patches will be moving that code around and introducing the
constants in the process, this patch leaves those untouched.

Also note that the GRXFSIZ register also contains a fifo size, but
there is no corresponding start address register (it is always the first
fifo in memory), the layout of the GRXFSIZ register is different and
cannot use the same constants.

Signed-off-by: Matthijs Kooijman matth...@stdin.nl

---
v2: Reverse this patch: Instead of removing the generic constants and
introducing specific constants, drop the specicic ones in favor of the
generic ones.
---
 drivers/staging/dwc2/hw.h | 19 ---
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/dwc2/hw.h b/drivers/staging/dwc2/hw.h
index cf0dc97..0777d8a 100644
--- a/drivers/staging/dwc2/hw.h
+++ b/drivers/staging/dwc2/hw.h
@@ -190,14 +190,7 @@
 #define GRXFSIZHSOTG_REG(0x024)
 
 #define GNPTXFSIZ  HSOTG_REG(0x028)
-#define GNPTXFSIZ_NP_TXF_DEP_MASK  (0x  16)
-#define GNPTXFSIZ_NP_TXF_DEP_SHIFT 16
-#define GNPTXFSIZ_NP_TXF_DEP_LIMIT 0x
-#define GNPTXFSIZ_NP_TXF_DEP(_x)   ((_x)  16)
-#define GNPTXFSIZ_NP_TXF_ST_ADDR_MASK  (0x  0)
-#define GNPTXFSIZ_NP_TXF_ST_ADDR_SHIFT 0
-#define GNPTXFSIZ_NP_TXF_ST_ADDR_LIMIT 0x
-#define GNPTXFSIZ_NP_TXF_ST_ADDR(_x)   ((_x)  0)
+/* Use FIFOSIZE_* constants to access this register */
 
 #define GNPTXSTS   HSOTG_REG(0x02C)
 #define GNPTXSTS_NP_TXQ_TOP_MASK   (0x7f  24)
@@ -395,16 +388,12 @@
 #define ADPCTL_PRB_DSCHRG_SHIFT0
 
 #define HPTXFSIZ   HSOTG_REG(0x100)
+/* Use FIFOSIZE_* constants to access this register */
 
 #define DPTXFSIZN(_a)  HSOTG_REG(0x104 + (((_a) - 1) * 4))
-#define DPTXFSIZN_DP_TXF_SIZE_MASK (0x  16)
-#define DPTXFSIZN_DP_TXF_SIZE_SHIFT16
-#define DPTXFSIZN_DP_TXF_SIZE_GET(_v)  (((_v)  16)  0x)
-#define DPTXFSIZN_DP_TXF_SIZE_LIMIT0x
-#define DPTXFSIZN_DP_TXF_SIZE(_x)  ((_x)  16)
-#define DPTXFSIZN_DP_TXF_ST_ADDR_MASK  (0x  0)
-#define DPTXFSIZN_DP_TXF_ST_ADDR_SHIFT 0
+/* Use FIFOSIZE_* constants to access this register */
 
+/* These apply to the GNPTXFSIZ, HPTXFSIZ and DPTXFSIZN registers */
 #define FIFOSIZE_DEPTH_MASK(0x  16)
 #define FIFOSIZE_DEPTH_SHIFT   16
 #define FIFOSIZE_STARTADDR_MASK(0x  0)
-- 
1.8.4.rc1

--
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: staging:DWC2 USB driver issues

[Matthijs Kooijman]

Hi Dinh,

 Any chance anyone has a similar experience with this DWC2 driver, any
 help will greatly appreciated. Of course, I will go back and verify the
 initialization between the DWC2 and the old driver to see if I can spot
 anything.
At first glance, the symptoms (getting transaction errors on every
transaction attempt) looks similar to what I was seeing a while back:

http://comments.gmane.org/gmane.linux.usb.general/86117

However, in my case it seems there was some hardware issue (possibly
even a hardware fault in a single unit) that caused it, so I doubt this
will really help you...


What hardware are you working with?

Gr.

Matthijs
--
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/3] staging: dwc2: add NAK holdoff patch from downstream Pi kernel

[Matthijs Kooijman]

Hi Paul,

   @@ -365,6 +366,7 @@ struct dwc2_hsotg {
 u8 otg_port;
 u32 *frame_list;
 dma_addr_t frame_list_dma;
   + int next_sched_frame;
  
  This variable is still not really used, I think. Most of the mentions in
  the patch are assignments, except for these two:
  
   + if (list_empty(hsotg-periodic_sched_inactive) ||
   + dwc2_frame_num_le(qh-sched_frame, hsotg-next_sched_frame))
   + hsotg-next_sched_frame = qh-sched_frame;
   +
  ...
   + if (!dwc2_frame_num_le(hsotg-next_sched_frame,
   +qh-sched_frame))
   + hsotg-next_sched_frame =
   + qh-sched_frame;
  
  However, these two uses of the variable have only the single purpose
  of updating the variable itself, no other behaviour is influenced by it.
  In effect, the variable does not influence the code at all and can be
  dropped, significantly simplifying this patch.
 
 Yep, that's kind of embarrassing. I will have to go back and read the
 downstream driver code again to figure out how this code is supposed to
 work. Thanks for the review.
Not sure if you figured this out already, but I seem to remember that
this next_sched_frame variable code was used by the driver to disable
the SOF interrupt when it wasn't needed and/or for this priority
interrupt thing on ARM (FIQ?). Not entirely sure, though.

Gr.

Matthijs
--
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/3] staging: dwc2: add NAK holdoff patch from downstream Pi kernel

[Matthijs Kooijman]

Hi Paul,

 Add the NAK holdoff patch from the downstream Raspberry Pi kernel.
 This allows the transfer scheduler to better handle cheeky devices
 that just hold off using NAKs.

 @@ -365,6 +366,7 @@ struct dwc2_hsotg {
   u8 otg_port;
   u32 *frame_list;
   dma_addr_t frame_list_dma;
 + int next_sched_frame;

This variable is still not really used, I think. Most of the mentions in
the patch are assignments, except for these two:

 + if (list_empty(hsotg-periodic_sched_inactive) ||
 + dwc2_frame_num_le(qh-sched_frame, hsotg-next_sched_frame))
 + hsotg-next_sched_frame = qh-sched_frame;
 +
...
 + if (!dwc2_frame_num_le(hsotg-next_sched_frame,
 +qh-sched_frame))
 + hsotg-next_sched_frame =
 + qh-sched_frame;

However, these two uses of the variable have only the single purpose
of updating the variable itself, no other behaviour is influenced by it.
In effect, the variable does not influence the code at all and can be
dropped, significantly simplifying this patch.

Gr.

Matthijs
--
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 3/3] staging: dwc2: add microframe scheduler from downstream Pi kernel

[Matthijs Kooijman]

Hi Paul  Dom,

I haven't got time to look closely right now, but at first glance most
of my comments have been resolved. One thing that is still in there, is
this piece of code for which I'm not sure if it is really related to the
topic of the patch:


 @@ -780,6 +784,10 @@ static void dwc2_assign_and_init_hc(struct dwc2_hsotg 
 *hsotg,
   chan-data_pid_start = qh-data_toggle;
   chan-multi_count = 1;
  
 + if ((urb-actual_length  0 || urb-actual_length  urb-length) 
 + !dwc2_hcd_is_pipe_in(urb-pipe_info))
 + urb-actual_length = urb-length;
 +
   if (hsotg-core_params-dma_enable  0) {
   chan-xfer_dma = urb-dma + urb-actual_length;
  

Paul, did you try the patch without this hunk?
Dom, can you tell use why this hunk is needed?

Gr.

Matthijs
--
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 0/3] staging: dwc2: 2nd try at uframe scheduler patch

[Matthijs Kooijman]

Hi Paul,

 Matthijs' concern about periodic endpoints with bInterval=1 seems
 to be unfounded. I tried a webcam, which uses a bInterval=1 isoc
 endpoint, on my PCI-based dev board, and it still works fine with
 this patch applied.
For the record, I still think this concern actually exists, but you will
need to completely fill up the available bandwidth to notice the
problem, since the scheduler is now too optimistic about how much
bandwidth is availble when devices with bInterval  8 are present (but
pessimistic for devices with bInterval  8).

However, this is something I think we can easily fix later, so getting
this series in is a step forward anyway.

I haven't got time to properly review and test these patches now, since
I'm about to leave for a two-week vacation though. I did a quick scan
just now and replied with some comments that occured to me.

Gr.

Matthijs
--
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 03/13] staging: dwc2: unshift non-bool register value constants

[Matthijs Kooijman]

Hey Paul,

 OK, I'm kind of on the fence about this one, so if you prefer it this way
 I guess it's OK with me.
 
 Acked-by: Paul Zimmerman pa...@synopsys.com
Thanks.

 What platforms have you tested this on, BTW? You said you have a Raspberry
 Pi, so I'd like you to test these patches there before resubmitting them.
 And I'll test them on my PCI-based platform.
I mostly tested the patches on the Ralink RT3052 SoC. I think I did some
basic testing on the Pi at some point, but I'll have to look again to do
some proper testing on there as well.

I had hoped to get around to this last week, but unfortunately that
didn't work out. Since I'm leaving on a vacation today, I won't be able
to resubmit this series until over two weeks at the earliest.

Gr.

Matthijs
--
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 00/13] staging: dwc2: Register-related cleanups

[Matthijs Kooijman]

Hi Paul,

 With the exception of 1 and 13 which I already commented on, you can add
 my acked-by to the rest of this series.
I assume you meant 3 instead of 13 here, given that you commented
on patch 3 (about unshifting stuff) but not 13?

Gr.

Matthijs
--
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 03/13] staging: dwc2: unshift non-bool register value constants

[Matthijs Kooijman]

Hi Paul,

On Wed, Jul 17, 2013 at 06:52:56PM +, Paul Zimmerman wrote:
  From: Matthijs Kooijman [mailto:matth...@stdin.nl]
  Sent: Wednesday, July 17, 2013 8:10 AM
  
  Various register fields wider than one bit have constants defined for
  their value. Previously, these registers would define the values as they
  appear in the register, so shifted to the right to the position the
  value appears in the register.
  
  This commit changes those constants to their natural values (e.g, 0, 1,
  2, etc.), as they are after shifting the register value to the right.
  This also changes all relevant code to shift the values before comparing
  them with constants.
  
  This has the advantage that the values can be stored in smaller
  variables (now they always require a u32) and makes the handling of
  these values more consistent with other register fields that represent
  natural numbers instead of enumerations (e.g., number of host channels).
 
 Hi Matthijs,
 
 Are you sure the advantages outweigh the drawbacks and the amount
 of code churn here? There are a lot of changes similar to this:
 
  -   u32 hs_phy_type = hsotg-hwcfg2  GHWCFG2_HS_PHY_TYPE_MASK;
  -   u32 fs_phy_type = hsotg-hwcfg2  GHWCFG2_FS_PHY_TYPE_MASK;
  +   u32 hs_phy_type = (hsotg-hwcfg2  GHWCFG2_HS_PHY_TYPE_MASK) 
  + GHWCFG2_HS_PHY_TYPE_SHIFT;
  +   u32 fs_phy_type = (hsotg-hwcfg2  GHWCFG2_FS_PHY_TYPE_MASK) 
  + GHWCFG2_FS_PHY_TYPE_SHIFT;
 
 I.e we went from two lines of code to four. Other than saving a tiny
 amount of RAM by being able to store the config values in a smaller
 variable, are there any other advantages?

I'd consider the RAM gain just a small bonus. For me, the real gain is
that the code makes more sense. My view is that a register is a
collection of fields, placed at different offsets into the register.
With this patch, you're properly extracting the field value from the
register value right at the spot where you read the register and after
that can just work with a (zero-based) field value as documented,
without caring where in the register it was stored.

For natural number type fields (e.g., number of host channels), this
handling makes the most sense, since otherwise you'd have to do the
shifting later when you actually need to interpret the value.

For boolean fields, the shifting is done implicitely by using the !!
operator.

For enumeration type fields (e.g., the hs_phy_type above), the gain
isn't so clear, since the comparisons will almost always be done against
constants.


Having all types of fields be handled in the same way, seems like a gain
in itself to me.

For me, the gains outweigh the extra lines of code (especially since
a lot of them are moved into a single dwc2_get_hw_params function in a
later patch).

If I haven't convinced you yet, I'll drop this patch from the series and
resubmit. This would require quite some (textual, not functional)
changes to the later patches. Can I still add your acked-by to them, or
do you want to see the changed patches first?

Gr.

Matthijs
--
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] staging: dwc2: add microframe scheduler from downstream Pi kernel

[Matthijs Kooijman]

Hi Paul,

  Furthermore, I wonder about how this scheduler works exactly. What I see
  is:
   - the number usecs needed for a single transfer in a periodic qh is
 calculated
   - When the qh is scheduled, the first of the 8 microframes with enough
 usecs available is picked for this qh (disregarding FS transfers that
 don't fit in 1 microframe for now)
  
  However, this seems correct only for endpoints with an interval of 8
  microframes? If an isoc endpoint has an interval of 1, it should be
  scheduled in _every_ microframe, right?
  
  The old code was pessimistic (the dwc2_check_periodic_bandwidth
  essentially assumes an interval of 1 for everything) but the new code is
  actually too optimistic (as it essentially assumes an interval of 8 for
  everything).
  
  This leads me to believe that this code works because it makes the
  scheduler way to optimistic and because it removes the one channel per
  periodic endpoint policy (which is way too optimistic), but it would
  break when adding too much (periodic) devices (in nasty ways, no nice
  not enough bandwidth messages).

It occurred to me that there is in fact more merit to this scheduler
than I originally thought. Before, I thought its only actual purpose was
to detect if there was room or not in the schedule for a new periodic
transfer. (and if I'm not mistaken, it does that badly when intervals of
 8 uframes are involved).

However, what this scheduler also does is (obviously...) scheduling :-)
It makes sure that the various periodic transfers actually get sent to
the hardware spaced over different uframes instead of all at the first
or last uframe in a frame as before. This allows the driver to actually
accept more than 1 uframe worth of periodic transfers.


  Overall, I don't think this patch is not even nearly ready to be
  merged...

 Well, I disagree :)
I would argue that at least any questions about what this patch does and
why need to be resolved. Even if you did not write the code, you're the
one submitting it. Also, regardless of who wrote the code, the quality
of it should be high enough to accept in mainline. And frankly, I do not
think But it works is sufficient indication of quality by itself.

What I'm afraid of, is that if we introduce some code now that isn't
really needed or solves an unknown problem in the wrong way and we'll
end up carrying that code around without properly understanding what
it's for forever.

Of course this is just how I _think_ things should work, I'm not really
in a position to decide anything. So I'll just stick to commenting on
the code now, as far as I understand it, and try to understand better so
I can offer suggestions for improvements.

   - chan = list_first_entry(hsotg-free_hc_list, struct dwc2_host_chan,
   - hc_list_entry);
   + if (!chan) {
   + dev_dbg(hsotg-dev, No free channel to assign\n);
   + return -ENOMEM;
   + }
  As above, I don't think this can ever happen.
 
 Sure it can. If hsotg-free_hc_list is empty on entry to this
 function, for example.
Ah, you're right.

  Only the addition of nak_frame and frame_usecs seem relevant to this
  patch, the rest seems noise. And those variables could use some actual
  documentation. I propose:
  
   * @nak_frame:  (Micro)frame number in which this qh was
  re-queued because a NAK was received. Is 0x
  when no NAK was received.
   * @frame_usecs:Internal variable used by the microframe scheduler
 
 No, its not noise. I took the opportunity while adding the new
 members to reorder the existing ones from smallest to largest, to
 reduce the amount of wasted space due to structure padding. I think
 that's a legitimate thing to do in this patch.
I'm not saying it's a useless change, I meant it is noise for _this_
patch and should go into a separate one.

Gr.

Matthijs
--
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] staging: dwc2: add driver parameter to set AHB config register value

[Matthijs Kooijman]

Hey Paul,

   diff --git a/drivers/staging/dwc2/core.h b/drivers/staging/dwc2/core.h
   index fc075a7..e771e40 100644
   --- a/drivers/staging/dwc2/core.h
   +++ b/drivers/staging/dwc2/core.h
   @@ -150,10 +150,11 @@ enum dwc2_lx_state {
 *  are enabled
 * @reload_ctl: True to allow dynamic reloading of HFIR register 
   during
 *  runtime
   - * @ahb_single: This bit enables SINGLE transfers for remainder 
   data in
   - *  a transfer for DMA mode of operation.
   - *   0 - remainder data will be sent using INCR 
   burst size
   - *   1 - remainder data will be sent using SINGLE 
   burst size
   + * @ahbcfg: This field allows the default value of the 
   GAHBCFG
   + *  register to be overridden
   + *   -1 - GAHBCFG value will not be 
   overridden
   + *   all others - GAHBCFG value will be overridden 
   with
   + *this value
 * @otg_ver:OTG version supported
 *   0 - 1.3
 *   1 - 2.0
  Shouldn't this mention that a few of the bits in the register cannot be
  set like this?
 
 I could have added a comment along the lines of see the HSOTG databook
 for valid values for this register. But e.g. the value used by the
 Broadcom SOC is not documented in the Synopsys databook. I'm guessing
 they made some enhancement to our core. So I don't even know what bits
 cannot be set in this register.

With cannot be set, I meant the bits that are masked out by the driver
code (e.g., GAHBCFG_CTRL_MASK). I'll add a remark along those lines to
my improve comments patch I have queued, since your patch is already
picked up by Greg.

Gr.

Matthijs
--
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] staging: dwc2: add driver parameter to set AHB config register value

[Matthijs Kooijman]

Hey Paul,

one more thing:

+ *   -1 - GAHBCFG value will not be 
overridden

This seems incorrect: If it is set to -1, GAHBCFG will be set to 0x06
(INCR4), it is not left unchanged. I'll also include this in my
documentation patch.

Gr.

Matthijs
--
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 v2] staging: dwc2: Don't touch the dma_mask when dma is disabled

[Matthijs Kooijman]

There was some code that cleared the dma_mask when dma was disabled in
the driver. Given that clearing the mask doesn't actually tell the usb
core we're not using dma, and a previous commit explicitely sets the
hcd-self.uses_dma value, it seems these values are unneeded and can
only potentially cause problems (when reloading a module, for example).

Signed-off-by: Matthijs Kooijman matth...@stdin.nl
Acked-by: Paul Zimmerman paul.zimmer...@synopsys.com

---
v2: Rebased on top of the latest staging-next

 drivers/staging/dwc2/hcd.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/staging/dwc2/hcd.c b/drivers/staging/dwc2/hcd.c
index fbacf6a..32b52ad 100644
--- a/drivers/staging/dwc2/hcd.c
+++ b/drivers/staging/dwc2/hcd.c
@@ -2845,9 +2845,6 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
dev_warn(hsotg-dev, can't set DMA mask\n);
if (dma_set_coherent_mask(hsotg-dev, DMA_BIT_MASK(32))  0)
dev_warn(hsotg-dev, can't set coherent DMA mask\n);
-   } else {
-   dma_set_mask(hsotg-dev, 0);
-   dma_set_coherent_mask(hsotg-dev, 0);
}
 
hcd = usb_create_hcd(dwc2_hc_driver, hsotg-dev, dev_name(hsotg-dev));
-- 
1.8.3.rc1

--
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 V3] usb: Add Device Tree support to XHCI Platform driver

[Matthijs Kooijman]

Hi Alan,

 This compiles without CONFIG_OF because of_match_ptr() assigns NULL if
 CONFIG_OF is not defined.
Ah, I guess that makes sense :-)

Apologies for the noise...

Gr.

Matthijs
--
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: dwc2: Transaction errors with device connected at boot

[Matthijs Kooijman]

Hi Paul,

I noticed that this bug occurs with the old dwc_otg driver (from the
Ralink SDK) as well, so it is not dwc2-specific.

However, I also ound out this bug does not occur on another (identical)
unit I have (using the old driver), so it seems likely that this is
somehow a one-off (hardware) fault in my debug board.

There is still a chance that running the dwc2 driver on a board makes it
faulty forever afterwards (I have only ran the dwc2 driver on the
faulty board so far, since that's the only one with a serial port
soldered on). However, this doesn't seem so likely to me.

I'll probably do a few more tests in the future, but for now I don't
think this is a bug that should receive much more attention.

Gr.

Matthijs
--
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: dwc2: Transaction errors with device connected at boot

[Matthijs Kooijman]

Hi Paul,

 There's no firmware in the HSOTG core, so I don't see how running the
 driver could permanently damage the core. However, if it's your only
 good working unit then I guess some paranoia is understandable ;)
Well, it was just that I had only one unit where running a clean 3.10
kernel was easy because I could TFTP boot it. However, I compiled an
OpenWRT image with 3.10 for the other unit this afternoon and that
worked without errors, so I guess the first one is really broken
somehow.

In any case, no point in investing more time in this and I should
probably solder on a serial port onto one of my other units :-)

Gr.

Matthijs
--
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 0/3] staging: dwc2: Add some dma-related defensive programming

[Matthijs Kooijman]

Hi Greg,

here's three patches for dwc2, acked by Paul. They're not critical, so
no need to push them to 3.11.

Gr.

Matthijs

Matthijs Kooijman (3):
  staging: dwc2: disable dma when no dma_mask was setup
  staging: dwc2: when dma is disabled, clear hcd-self.uses_dma
  staging: dwc2: Don't touch the dma_mask when dma is disabled

 drivers/staging/dwc2/hcd.c | 15 ---
 1 file changed, 12 insertions(+), 3 deletions(-)

-- 
1.8.3.rc1

--
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 3/3] staging: dwc2: Don't touch the dma_mask when dma is disabled

[Matthijs Kooijman]

There was some code that cleared the dma_mask when dma was disabled in
the driver. Given that clearing the mask doesn't actually tell the usb
core we're not using dma, and a previous commit explicitely sets the
hcd-self.uses_dma value, it seems these values are unneeded and can
only potentially cause problems (when reloading a module, for example).

Signed-off-by: Matthijs Kooijman matth...@stdin.nl
Acked-by: Paul Zimmerman paul.zimmer...@synopsys.com
---
 drivers/staging/dwc2/hcd.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/staging/dwc2/hcd.c b/drivers/staging/dwc2/hcd.c
index 50a379d..075e53b 100644
--- a/drivers/staging/dwc2/hcd.c
+++ b/drivers/staging/dwc2/hcd.c
@@ -2817,9 +2817,6 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
if (dma_set_coherent_mask(hsotg-dev, DMA_BIT_MASK(31))  0)
dev_warn(hsotg-dev,
 can't enable workaround for 2GB RAM\n);
-   } else {
-   dma_set_mask(hsotg-dev, 0);
-   dma_set_coherent_mask(hsotg-dev, 0);
}
 
hcd = usb_create_hcd(dwc2_hc_driver, hsotg-dev, dev_name(hsotg-dev));
-- 
1.8.3.rc1

--
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 1/3] staging: dwc2: disable dma when no dma_mask was setup

[Matthijs Kooijman]

If the platform or bus driver failed to setup a dma_mask, but the
hardware advertises support for DMA, before DMA would be enabled in
dwc2, but disabled in the usb core, making all connectivity break.

With this commit, the dwc2 driver will emit a warning and fall back to
slave mode in this case.

Note that since commit 642f2ec (staging: dwc2: Fix dma-enabled platform
devices using a default dma_mask) the platform bindings make sure a DMA
mask is always present, but having this check here anyway is probably a
good from a defensive programming standpoint (in case of changes to
platform.c or addition of new glue layers).

Signed-off-by: Matthijs Kooijman matth...@stdin.nl
Acked-by: Paul Zimmerman paul.zimmer...@synopsys.com
---
 drivers/staging/dwc2/hcd.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/drivers/staging/dwc2/hcd.c b/drivers/staging/dwc2/hcd.c
index 2ed54b1..b5b229a 100644
--- a/drivers/staging/dwc2/hcd.c
+++ b/drivers/staging/dwc2/hcd.c
@@ -2801,6 +2801,15 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
/* Validate parameter values */
dwc2_set_parameters(hsotg, params);
 
+   /* Check if the bus driver or platform code has setup a dma_mask */
+   if (hsotg-core_params-dma_enable  0 
+   hsotg-dev-dma_mask == NULL) {
+   dev_warn(hsotg-dev,
+dma_mask not set, disabling DMA\n);
+   hsotg-core_params-dma_enable = 0;
+   hsotg-core_params-dma_desc_enable = 0;
+   }
+
/* Set device flags indicating whether the HCD supports DMA */
if (hsotg-core_params-dma_enable  0) {
if (dma_set_mask(hsotg-dev, DMA_BIT_MASK(32))  0)
-- 
1.8.3.rc1

--
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/3] staging: dwc2: when dma is disabled, clear hcd-self.uses_dma

[Matthijs Kooijman]

When dma is disabled inside dwc2 (because the hardware does not support
it, or the code was changed to disable it for testing), let the usb core
know about this by clearing hcd-self.uses_dma.

By default, the usb core assumes that dma is used when a dma_mask is
set, but this might not always match the dma_enable value in dwc2. To
prevent problems resulting from a mismatch, better to explicitely
disable dma in this case (though everything seemed to work with the
wrong value of uses_dma as well, probably only resulted in some unneeded
work).

Signed-off-by: Matthijs Kooijman matth...@stdin.nl
Acked-by: Paul Zimmerman paul.zimmer...@synopsys.com
---
 drivers/staging/dwc2/hcd.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/staging/dwc2/hcd.c b/drivers/staging/dwc2/hcd.c
index b5b229a..50a379d 100644
--- a/drivers/staging/dwc2/hcd.c
+++ b/drivers/staging/dwc2/hcd.c
@@ -2826,6 +2826,9 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
if (!hcd)
goto error1;
 
+   if (hsotg-core_params-dma_enable = 0)
+   hcd-self.uses_dma = 0;
+
hcd-has_tt = 1;
 
spin_lock_init(hsotg-lock);
-- 
1.8.3.rc1

--
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] staging: dwc2: add microframe scheduler from downstream Pi kernel

[Matthijs Kooijman]

Hi Paul,

 The transfer scheduler in the dwc2 driver is pretty basic, not to
 mention buggy. It works fairly well with just a couple of devices
 plugged in, but if you add, say, multiple devices with periodic
 endpoints, the scheduler breaks down and can't even enumerate all
 the devices.

This seems related to a patch I made last year for the dwc_otg driver.
On RT3052, we only have 4 host channels, so it was easy to run out of
them using a 3G stick and a hub. The 3G sticks hog up 2 host channels
because they keep NAKing their bulk in transfers and 2 for intr
endpoints, leaving none for any other transfers. I created a patch to
allow canceling a NAKing host channel, but I suspect that this
microframe scheduler might help in this case as well, because it allows
sharing a single host channel for all periodic transfers, I think,
leaving the other three for bulk transfers. It might still be useful to
forward port my patch at some point, since that would in theory allow
multiple blocking endpoints to be used at the same time.

 To fix this, import the microframe scheduler patch from the
 driver in the downstream Raspberry Pi kernel, which is based on
 the Synopsys vendor driver. That patch was done by the guys from
 raspberrypi.org, including at least Gordon Hollingsworth and
 popcornmix.
 
 I have added a driver parameter for this, enabled by default, in
 case anyone has problems with it and needs to disable it. I don't
 think we should add a DT binding for that, though, since I plan to
 remove the option once any bugs are fixed.

Some general remarks:

It seems that this patch doesn't include just the microframe scheduling,
but also the NAK holdoff patch (which was a separate patch in the
downstream kernel IIRC and seems like a separate feature in any case)
and other unrelated and unused bits.

Furthermore, I wonder about how this scheduler works exactly. What I see
is:
 - the number usecs needed for a single transfer in a periodic qh is
   calculated
 - When the qh is scheduled, the first of the 8 microframes with enough
   usecs available is picked for this qh (disregarding FS transfers that
   don't fit in 1 microframe for now)

However, this seems correct only for endpoints with an interval of 8
microframes? If an isoc endpoint has an interval of 1, it should be
scheduled in _every_ microframe, right?

The old code was pessimistic (the dwc2_check_periodic_bandwidth
essentially assumes an interval of 1 for everything) but the new code is
actually too optimistic (as it essentially assumes an interval of 8 for
everything).

This leads me to believe that this code works because it makes the
scheduler way to optimistic and because it removes the one channel per
periodic endpoint policy (which is way too optimistic), but it would
break when adding too much (periodic) devices (in nasty ways, no nice
not enough bandwidth messages).

Overall, I don't think this patch is not even nearly ready to be
merged...

There's a lot more detailed comments inline in the code below.

 Signed-off-by: Paul Zimmerman pa...@synopsys.com
 ---
 Gordon, I would like to add a copyright notice for the work you
 guys did on this (thanks!). Can you send me the names and dates I
 should add?
 
 This patch should be applied on top of staging: dwc2: add driver
 parameter to set AHB config register value or else it won't apply
 cleanly.
 
  drivers/staging/dwc2/core.c  |  21 
  drivers/staging/dwc2/core.h  |  47 +---
  drivers/staging/dwc2/hcd.c   |  94 +---
  drivers/staging/dwc2/hcd.h   |  27 +++--
  drivers/staging/dwc2/hcd_ddma.c  |  13 ++-
  drivers/staging/dwc2/hcd_intr.c  |  59 +++---
  drivers/staging/dwc2/hcd_queue.c | 236 
 ---
  drivers/staging/dwc2/pci.c   |   1 +
  8 files changed, 417 insertions(+), 81 deletions(-)
 
 diff --git a/drivers/staging/dwc2/core.c b/drivers/staging/dwc2/core.c
 index a090f79..16afc06 100644
 --- a/drivers/staging/dwc2/core.c
 +++ b/drivers/staging/dwc2/core.c
 @@ -2612,6 +2612,26 @@ int dwc2_set_param_otg_ver(struct dwc2_hsotg *hsotg, 
 int val)
   return retval;
  }
  
 +int dwc2_set_param_uframe_sched(struct dwc2_hsotg *hsotg, int val)
 +{
 + int retval = 0;
 +
 + if (DWC2_PARAM_TEST(val, 0, 1)) {
 + if (val = 0) {
 + dev_err(hsotg-dev,
 + '%d' invalid for parameter uframe_sched\n,
 + val);
 + dev_err(hsotg-dev, uframe_sched must be 0 or 1\n);
 + }
 + val = 1;
 + dev_dbg(hsotg-dev, Setting uframe_sched to %d\n, val);
 + retval = -EINVAL;
 + }
 +
 + hsotg-core_params-uframe_sched = val;
 + return retval;
 +}
 +
  /*
   * This function is called during module intialization to pass module 
 parameters
   * for the DWC_otg core. It returns non-0 if any parameters are invalid.
 @@ -2658,6 +2678,7 @@ int dwc2_set_parameters(struct dwc2_hsotg 

Re: [PATCH] staging: dwc2: add driver parameter to set AHB config register value

[Matthijs Kooijman]

Hi Paul,

On Tue, Jul 16, 2013 at 12:22:12PM -0700, Paul Zimmerman wrote:
 The dwc2 driver sets the value of the DWC2 GAHBCFG register to 0x6,
 which is GAHBCFG_HBSTLEN_INCR4. But different platforms may require
 different values. In particular, the Broadcom 2835 SOC used in the
 Raspberry Pi needs a value of 0x10, otherwise the DWC2 controller
 stops working after a short period of heavy USB traffic.
 
 So this patch adds another driver parameter named 'ahbcfg'. The
 default value is 0x6. Any platform needing a different value should
 add a DT attribute to set it.
 
 This patch also removes the 'ahb_single' driver parameter, since
 that bit can now be set using 'ahbcfg'.
 
 This patch does not add DT support to platform.c, I will leave that
 to whoever owns the first platform that needs a non-default value.
 (Stephen?)
I previously submitted a patch to load everything from DT, which might
serve as inspiration: http://article.gmane.org/gmane.linux.usb.general/85086
(but note this reply: http://article.gmane.org/gmane.linux.usb.general/85104)

 Signed-off-by: Paul Zimmerman pa...@synopsys.com
Reviewed-by: Matthijs Kooijman matth...@stdin.nl

However, I do wonder if it makes sense to let (almost) the entire
GAHBCFG be set from one variable. IMHO it would be more elegant to split
out the separate fields that can be set and have separate config
variables for them (I especially have the feeling that a literal
register value does not belong in a DT attribute, though I'm not quite
sure what's the policy there). Not sure if it's worth the extra effort,
though.


 -int dwc2_set_param_ahb_single(struct dwc2_hsotg *hsotg, int val)
 +int dwc2_set_param_ahbcfg(struct dwc2_hsotg *hsotg, int val)
  {
 - int valid = 1;
 - int retval = 0;
 -
 - if (DWC2_PARAM_TEST(val, 0, 1)) {
 - if (val = 0) {
 - dev_err(hsotg-dev,
 - '%d' invalid for parameter ahb_single\n, val);
 - dev_err(hsotg-dev, ahb_single must be 0 or 1\n);
 - }
 - valid = 0;
 - }
 -
 - if (val  0  hsotg-snpsid  DWC2_CORE_REV_2_94a)
 - valid = 0;
 -
 - if (!valid) {
 - if (val = 0)
 - dev_err(hsotg-dev,
 - %d invalid for parameter ahb_single. Check HW 
 configuration.\n,
 - val);
 - val = 0;
 - dev_dbg(hsotg-dev, Setting ahb_single to %d\n, val);
 - retval = -EINVAL;
 - }
 -
 - hsotg-core_params-ahb_single = val;
 - return retval;
 + if (val != -1)
 + hsotg-core_params-ahbcfg = val;
 + else
 + hsotg-core_params-ahbcfg = GAHBCFG_HBSTLEN_INCR4;
 + return 0;
  }

Does it make sense to remove all the validity checking here? I could
imagine adding a check like:

if (val  GAHBCFG_CTRL_MASK) {
// Bits in GAHBCFG_CTRL_MASK cannot be set this way
...
valid = 0
}

to prevent silently dropping bits from an invalid value?

Also, it seems that there used to be a requirement of having at least
core revision 2.94a for types other than single. Is this no longer
relevant?

 diff --git a/drivers/staging/dwc2/core.h b/drivers/staging/dwc2/core.h
 index fc075a7..e771e40 100644
 --- a/drivers/staging/dwc2/core.h
 +++ b/drivers/staging/dwc2/core.h
 @@ -150,10 +150,11 @@ enum dwc2_lx_state {
   *  are enabled
   * @reload_ctl: True to allow dynamic reloading of HFIR register 
 during
   *  runtime
 - * @ahb_single: This bit enables SINGLE transfers for remainder data 
 in
 - *  a transfer for DMA mode of operation.
 - *   0 - remainder data will be sent using INCR burst 
 size
 - *   1 - remainder data will be sent using SINGLE burst 
 size
 + * @ahbcfg: This field allows the default value of the GAHBCFG
 + *  register to be overridden
 + *   -1 - GAHBCFG value will not be overridden
 + *   all others - GAHBCFG value will be overridden with
 + *this value
   * @otg_ver:OTG version supported
   *   0 - 1.3
   *   1 - 2.0
Shouldn't this mention that a few of the bits in the register cannot be
set like this?

Gr.

Matthijs
--
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: Incompleteness of HW capability registers (Was: [PATCH] staging: dwc2: fix thinko in dwc2_hc_set_even_odd_frame())

[Matthijs Kooijman]

Hey Stephen,

 Ah, that makes sense. It wasn't clear to me from the config structure
 that some of the variables didn't correspond directly to config
 fields/registers in the HW.
I have a patch pending that separates configured values from hardware
values, which would help here. I'll try to send that one out today or
tomorrow, together with some other cleanups.

 It'd be nice if the DT bindings (when we create them) and/or platform
 device wrapper only explicitly set values in the config structure that
 fall into this category; anything that can be queried from HW probably
 should be.
That makes sense to me.

 Perhaps the config structure could even be split up into two
 sub-structures: One for must specify values which can't be probed
 from HW, and another to override values where the HW value is wrong
 for some reason, but usually shouldn't need to be specified.
Hmm, my patch doesn't do this (it separates detected values from set
values, not optional set values from required set values), though it
will probably make it easier to make this separation later on.

Gr.

Matthijs
--
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 04/13] staging: dwc2: only read the snpsid register once

[Matthijs Kooijman]

This (read-only) register was read twice, storing it for later use the
second time. Now it is only read once, storing it right away.

Signed-off-by: Matthijs Kooijman matth...@stdin.nl
---
 drivers/staging/dwc2/hcd.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/dwc2/hcd.c b/drivers/staging/dwc2/hcd.c
index b5713aa..5e8b2a8 100644
--- a/drivers/staging/dwc2/hcd.c
+++ b/drivers/staging/dwc2/hcd.c
@@ -2705,7 +2705,7 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
 {
struct usb_hcd *hcd;
struct dwc2_host_chan *channel;
-   u32 snpsid, gusbcfg, hcfg;
+   u32 gusbcfg, hcfg;
int i, num_channels;
int retval = -ENOMEM;
 
@@ -2717,10 +2717,11 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
 * 0x45f42xxx or 0x45f43xxx, which corresponds to either OT2 or OT3,
 * as in OTG version 2.xx or OTG version 3.xx.
 */
-   snpsid = readl(hsotg-regs + GSNPSID);
-   if ((snpsid  0xf000) != 0x4f542000 
-   (snpsid  0xf000) != 0x4f543000) {
-   dev_err(hsotg-dev, Bad value for GSNPSID: 0x%08x\n, snpsid);
+   hsotg-snpsid = readl(hsotg-regs + GSNPSID);
+   if ((hsotg-snpsid  0xf000) != 0x4f542000 
+   (hsotg-snpsid  0xf000) != 0x4f543000) {
+   dev_err(hsotg-dev, Bad value for GSNPSID: 0x%08x\n,
+   hsotg-snpsid);
retval = -ENODEV;
goto error1;
}
@@ -2843,7 +2844,6 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
}
INIT_WORK(hsotg-wf_otg, dwc2_conn_id_status_change);
 
-   hsotg-snpsid = readl(hsotg-regs + GSNPSID);
dev_dbg(hsotg-dev, Core Release: %1x.%1x%1x%1x\n,
hsotg-snpsid  12  0xf, hsotg-snpsid  8  0xf,
hsotg-snpsid  4  0xf, hsotg-snpsid  0xf);
-- 
1.8.3.rc1

--
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 05/13] staging: dwc2: simplify register shift expressions

[Matthijs Kooijman]

This commit changes expressions from (val  shift)  (mask  shift) to
(val  mask)  shift.

Signed-off-by: Matthijs Kooijman matth...@stdin.nl
---
 drivers/staging/dwc2/core.c | 20 ++---
 drivers/staging/dwc2/hcd.c  | 62 +++--
 drivers/staging/dwc2/hcd_ddma.c |  8 +++---
 drivers/staging/dwc2/hcd_intr.c | 22 ++-
 4 files changed, 50 insertions(+), 62 deletions(-)

diff --git a/drivers/staging/dwc2/core.c b/drivers/staging/dwc2/core.c
index bf28b55..23677b3 100644
--- a/drivers/staging/dwc2/core.c
+++ b/drivers/staging/dwc2/core.c
@@ -1391,14 +1391,14 @@ void dwc2_hc_start_transfer(struct dwc2_hsotg *hsotg,
dev_vdbg(hsotg-dev, %s: Channel %d\n, __func__,
 chan-hc_num);
dev_vdbg(hsotg-dev,Xfer Size: %d\n,
-hctsiz  TSIZ_XFERSIZE_SHIFT 
-TSIZ_XFERSIZE_MASK  TSIZ_XFERSIZE_SHIFT);
+(hctsiz  TSIZ_XFERSIZE_MASK) 
+TSIZ_XFERSIZE_SHIFT);
dev_vdbg(hsotg-dev,Num Pkts: %d\n,
-hctsiz  TSIZ_PKTCNT_SHIFT 
-TSIZ_PKTCNT_MASK  TSIZ_PKTCNT_SHIFT);
+(hctsiz  TSIZ_PKTCNT_MASK) 
+TSIZ_PKTCNT_SHIFT);
dev_vdbg(hsotg-dev,Start PID: %d\n,
-hctsiz  TSIZ_SC_MC_PID_SHIFT 
-TSIZ_SC_MC_PID_MASK  TSIZ_SC_MC_PID_SHIFT);
+(hctsiz  TSIZ_SC_MC_PID_MASK) 
+TSIZ_SC_MC_PID_SHIFT);
}
 
if (hsotg-core_params-dma_enable  0) {
@@ -1442,8 +1442,8 @@ void dwc2_hc_start_transfer(struct dwc2_hsotg *hsotg,
 
if (dbg_hc(chan))
dev_vdbg(hsotg-dev,Multi Cnt: %d\n,
-hcchar  HCCHAR_MULTICNT_SHIFT 
-HCCHAR_MULTICNT_MASK  HCCHAR_MULTICNT_SHIFT);
+(hcchar  HCCHAR_MULTICNT_MASK) 
+HCCHAR_MULTICNT_SHIFT);
 
writel(hcchar, hsotg-regs + HCCHAR(chan-hc_num));
if (dbg_hc(chan))
@@ -1531,8 +1531,8 @@ void dwc2_hc_start_transfer_ddma(struct dwc2_hsotg *hsotg,
 
if (dbg_hc(chan))
dev_vdbg(hsotg-dev,Multi Cnt: %d\n,
-hcchar  HCCHAR_MULTICNT_SHIFT 
-HCCHAR_MULTICNT_MASK  HCCHAR_MULTICNT_SHIFT);
+(hcchar  HCCHAR_MULTICNT_MASK) 
+HCCHAR_MULTICNT_SHIFT);
 
writel(hcchar, hsotg-regs + HCCHAR(chan-hc_num));
if (dbg_hc(chan))
diff --git a/drivers/staging/dwc2/hcd.c b/drivers/staging/dwc2/hcd.c
index 5e8b2a8..f3c38a9 100644
--- a/drivers/staging/dwc2/hcd.c
+++ b/drivers/staging/dwc2/hcd.c
@@ -1006,10 +1006,10 @@ static void dwc2_process_periodic_channels(struct 
dwc2_hsotg *hsotg)
dev_vdbg(hsotg-dev, Queue periodic transactions\n);
 
tx_status = readl(hsotg-regs + HPTXSTS);
-   qspcavail = tx_status  TXSTS_QSPCAVAIL_SHIFT 
-   TXSTS_QSPCAVAIL_MASK  TXSTS_QSPCAVAIL_SHIFT;
-   fspcavail = tx_status  TXSTS_FSPCAVAIL_SHIFT 
-   TXSTS_FSPCAVAIL_MASK  TXSTS_FSPCAVAIL_SHIFT;
+   qspcavail = (tx_status  TXSTS_QSPCAVAIL_MASK) 
+   TXSTS_QSPCAVAIL_SHIFT;
+   fspcavail = (tx_status  TXSTS_FSPCAVAIL_MASK) 
+   TXSTS_FSPCAVAIL_SHIFT;
 
if (dbg_perio()) {
dev_vdbg(hsotg-dev,   P Tx Req Queue Space Avail (before 
queue): %d\n,
@@ -1047,8 +1047,8 @@ static void dwc2_process_periodic_channels(struct 
dwc2_hsotg *hsotg)
qh-channel-multi_count  1)
hsotg-queuing_high_bandwidth = 1;
 
-   fspcavail = tx_status  TXSTS_FSPCAVAIL_SHIFT 
-   TXSTS_FSPCAVAIL_MASK  TXSTS_FSPCAVAIL_SHIFT;
+   fspcavail = (tx_status  TXSTS_FSPCAVAIL_MASK) 
+   TXSTS_FSPCAVAIL_SHIFT;
status = dwc2_queue_transaction(hsotg, qh-channel, fspcavail);
if (status  0) {
no_fifo_space = 1;
@@ -1079,10 +1079,10 @@ static void dwc2_process_periodic_channels(struct 
dwc2_hsotg *hsotg)
 
if (hsotg-core_params-dma_enable = 0) {
tx_status = readl(hsotg-regs + HPTXSTS);
-   qspcavail = tx_status  TXSTS_QSPCAVAIL_SHIFT 
-   TXSTS_QSPCAVAIL_MASK  TXSTS_QSPCAVAIL_SHIFT;
-   fspcavail = tx_status  TXSTS_FSPCAVAIL_SHIFT 
-   TXSTS_FSPCAVAIL_MASK  TXSTS_FSPCAVAIL_SHIFT;
+   qspcavail = (tx_status  TXSTS_QSPCAVAIL_MASK) 
+   TXSTS_QSPCAVAIL_SHIFT;
+   fspcavail = (tx_status  TXSTS_FSPCAVAIL_MASK) 
+   TXSTS_FSPCAVAIL_SHIFT;
if (dbg_perio()) {
dev_vdbg(hsotg-dev,
   P Tx Req

[PATCH 02/13] staging: dwc2: fix off-by-one in check for max_packet_count parameter

[Matthijs Kooijman]

Previously, the max_packet_count could be set to 1  x, where x is the
number of bits available (width + 4 in the code). Since 1  x requires
x + 1 bits to represent, this will not work. The real maximum value is
(1  x) - 1. This value is already used the default when the set value
is invalid, but the upper limit for the set value was off-by-one.

This change makes the check the same as the one for max_transfer_size,
which was already correct.

Signed-off-by: Matthijs Kooijman matth...@stdin.nl
---
 drivers/staging/dwc2/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/dwc2/core.c b/drivers/staging/dwc2/core.c
index ebd0f12..0302e15 100644
--- a/drivers/staging/dwc2/core.c
+++ b/drivers/staging/dwc2/core.c
@@ -2214,7 +2214,7 @@ int dwc2_set_param_max_packet_count(struct dwc2_hsotg 
*hsotg, int val)
GHWCFG3_PACKET_SIZE_CNTR_WIDTH_MASK 
GHWCFG3_PACKET_SIZE_CNTR_WIDTH_SHIFT;
 
-   if (val  15 || val  (1  (width + 4)))
+   if (val  15 || val = (1  (width + 4)))
valid = 0;
 
if (!valid) {
-- 
1.8.3.rc1

--
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 11/13] staging: dwc2: interpret all hwcfg and related register at init time

[Matthijs Kooijman]

Before, the hwcfg registers were read at device init time, but
interpreted at various parts in the code. This commit unpacks the hwcfg
register values into a struct with properly labeled variables at init
time, which makes all the other code using these values more consise and
easier to read. Some values that were previously stored in the hsotg
struct are now moved into this new struct as well.

In addition to the hwcfg registers, the contents of some fifo size
registers are also unpacked. The hwcfg registers are read-only, so they
can be safely stored. The fifo size registers are read-write registers,
but their power-on values are significant: they give the maximum depth
of the fifo they describe.

This commit mostly moves code, but also attempts to simplify some
expressions from (val  shift)  (mask  shift) to
(val  mask)  shift.

Finally, all of the parameters read from the hardware are debug printed
after unpacking them, so a bunch of debug prints can be removed from
other places.

Signed-off-by: Matthijs Kooijman matth...@stdin.nl
---
 drivers/staging/dwc2/core.c  | 308 +--
 drivers/staging/dwc2/core.h  |  96 +---
 drivers/staging/dwc2/core_intr.c |   4 +-
 drivers/staging/dwc2/hcd.c   |  80 ++
 drivers/staging/dwc2/hcd.h   |   1 +
 drivers/staging/dwc2/hcd_intr.c  |   2 +-
 6 files changed, 290 insertions(+), 201 deletions(-)

diff --git a/drivers/staging/dwc2/core.c b/drivers/staging/dwc2/core.c
index 025760e..65e9bc6 100644
--- a/drivers/staging/dwc2/core.c
+++ b/drivers/staging/dwc2/core.c
@@ -90,14 +90,10 @@ static void dwc2_enable_common_interrupts(struct dwc2_hsotg 
*hsotg)
  */
 static void dwc2_init_fs_ls_pclk_sel(struct dwc2_hsotg *hsotg)
 {
-   u32 hs_phy_type = (hsotg-hwcfg2  GHWCFG2_HS_PHY_TYPE_MASK) 
- GHWCFG2_HS_PHY_TYPE_SHIFT;
-   u32 fs_phy_type = (hsotg-hwcfg2  GHWCFG2_FS_PHY_TYPE_MASK) 
- GHWCFG2_FS_PHY_TYPE_SHIFT;
u32 hcfg, val;
 
-   if ((hs_phy_type == GHWCFG2_HS_PHY_TYPE_ULPI 
-fs_phy_type == GHWCFG2_FS_PHY_TYPE_DEDICATED 
+   if ((hsotg-hw_params.hs_phy_type == GHWCFG2_HS_PHY_TYPE_ULPI 
+hsotg-hw_params.fs_phy_type == GHWCFG2_FS_PHY_TYPE_DEDICATED 
 hsotg-core_params-ulpi_fs_ls  0) ||
hsotg-core_params-phy_type == DWC2_PHY_TYPE_PARAM_FS) {
/* Full speed PHY */
@@ -247,7 +243,7 @@ static void dwc2_hs_phy_init(struct dwc2_hsotg *hsotg, bool 
select_phy)
 
 static void dwc2_phy_init(struct dwc2_hsotg *hsotg, bool select_phy)
 {
-   u32 usbcfg, hs_phy_type, fs_phy_type;
+   u32 usbcfg;
 
if (hsotg-core_params-speed == DWC2_SPEED_PARAM_FULL 
hsotg-core_params-phy_type == DWC2_PHY_TYPE_PARAM_FS) {
@@ -258,13 +254,8 @@ static void dwc2_phy_init(struct dwc2_hsotg *hsotg, bool 
select_phy)
dwc2_hs_phy_init(hsotg, select_phy);
}
 
-   hs_phy_type = (hsotg-hwcfg2  GHWCFG2_HS_PHY_TYPE_MASK) 
- GHWCFG2_HS_PHY_TYPE_SHIFT
-   fs_phy_type = (hsotg-hwcfg2  GHWCFG2_FS_PHY_TYPE_MASK) 
- GHWCFG2_FS_PHY_TYPE_SHIFT
-
-   if (hs_phy_type == GHWCFG2_HS_PHY_TYPE_ULPI 
-   fs_phy_type == GHWCFG2_FS_PHY_TYPE_DEDICATED 
+   if (hsotg-hw_params.hs_phy_type == GHWCFG2_HS_PHY_TYPE_ULPI 
+   hsotg-hw_params.fs_phy_type == GHWCFG2_FS_PHY_TYPE_DEDICATED 
hsotg-core_params-ulpi_fs_ls  0) {
dev_dbg(hsotg-dev, Setting ULPI FSLS\n);
usbcfg = readl(hsotg-regs + GUSBCFG);
@@ -283,8 +274,7 @@ static int dwc2_gahbcfg_init(struct dwc2_hsotg *hsotg)
 {
u32 ahbcfg = 0;
 
-   switch ((hsotg-hwcfg2  GHWCFG2_ARCHITECTURE_MASK) 
-   GHWCFG2_ARCHITECTURE_SHIFT) {
+   switch (hsotg-hw_params.arch) {
case GHWCFG2_EXT_DMA_ARCH:
dev_err(hsotg-dev, External DMA Mode not supported\n);
return -EINVAL;
@@ -336,8 +326,7 @@ static void dwc2_gusbcfg_init(struct dwc2_hsotg *hsotg)
usbcfg = readl(hsotg-regs + GUSBCFG);
usbcfg = ~(GUSBCFG_HNPCAP | GUSBCFG_SRPCAP);
 
-   switch ((hsotg-hwcfg2  GHWCFG2_OP_MODE_MASK) 
-   GHWCFG_OP_MODE_SHIFT) {
+   switch (hsotg-hw_params.op_mode) {
case GHWCFG2_OP_MODE_HNP_SRP_CAPABLE:
if (hsotg-core_params-otg_cap ==
DWC2_CAP_PARAM_HNP_SRP_CAPABLE)
@@ -398,23 +387,6 @@ int dwc2_core_init(struct dwc2_hsotg *hsotg, bool 
select_phy, int irq)
/* Reset the Controller */
dwc2_core_reset(hsotg);
 
-   dev_dbg(hsotg-dev, num_dev_perio_in_ep=%d\n,
-   hsotg-hwcfg4  GHWCFG4_NUM_DEV_PERIO_IN_EP_SHIFT 
-   GHWCFG4_NUM_DEV_PERIO_IN_EP_MASK 
-   GHWCFG4_NUM_DEV_PERIO_IN_EP_SHIFT);
-
-   hsotg-total_fifo_size = hsotg-hwcfg3  GHWCFG3_DFIFO_DEPTH_SHIFT 
-   GHWCFG3_DFIFO_DEPTH_MASK

[PATCH 03/13] staging: dwc2: unshift non-bool register value constants

[Matthijs Kooijman]

Various register fields wider than one bit have constants defined for
their value. Previously, these registers would define the values as they
appear in the register, so shifted to the right to the position the
value appears in the register.

This commit changes those constants to their natural values (e.g, 0, 1,
2, etc.), as they are after shifting the register value to the right.
This also changes all relevant code to shift the values before comparing
them with constants.

This has the advantage that the values can be stored in smaller
variables (now they always require a u32) and makes the handling of
these values more consistent with other register fields that represent
natural numbers instead of enumerations (e.g., number of host channels).

Signed-off-by: Matthijs Kooijman matth...@stdin.nl
---
 drivers/staging/dwc2/core.c  |  52 ++--
 drivers/staging/dwc2/hcd.c   |   2 +-
 drivers/staging/dwc2/hcd.h   |  18 +++---
 drivers/staging/dwc2/hcd_intr.c  |  19 +++---
 drivers/staging/dwc2/hcd_queue.c |   2 +-
 drivers/staging/dwc2/hw.h| 128 +++
 6 files changed, 117 insertions(+), 104 deletions(-)

diff --git a/drivers/staging/dwc2/core.c b/drivers/staging/dwc2/core.c
index 0302e15..bf28b55 100644
--- a/drivers/staging/dwc2/core.c
+++ b/drivers/staging/dwc2/core.c
@@ -90,8 +90,10 @@ static void dwc2_enable_common_interrupts(struct dwc2_hsotg 
*hsotg)
  */
 static void dwc2_init_fs_ls_pclk_sel(struct dwc2_hsotg *hsotg)
 {
-   u32 hs_phy_type = hsotg-hwcfg2  GHWCFG2_HS_PHY_TYPE_MASK;
-   u32 fs_phy_type = hsotg-hwcfg2  GHWCFG2_FS_PHY_TYPE_MASK;
+   u32 hs_phy_type = (hsotg-hwcfg2  GHWCFG2_HS_PHY_TYPE_MASK) 
+ GHWCFG2_HS_PHY_TYPE_SHIFT;
+   u32 fs_phy_type = (hsotg-hwcfg2  GHWCFG2_FS_PHY_TYPE_MASK) 
+ GHWCFG2_FS_PHY_TYPE_SHIFT;
u32 hcfg, val;
 
if ((hs_phy_type == GHWCFG2_HS_PHY_TYPE_ULPI 
@@ -108,7 +110,7 @@ static void dwc2_init_fs_ls_pclk_sel(struct dwc2_hsotg 
*hsotg)
dev_dbg(hsotg-dev, Initializing HCFG.FSLSPClkSel to %08x\n, val);
hcfg = readl(hsotg-regs + HCFG);
hcfg = ~HCFG_FSLSPCLKSEL_MASK;
-   hcfg |= val;
+   hcfg |= val  HCFG_FSLSPCLKSEL_SHIFT;
writel(hcfg, hsotg-regs + HCFG);
 }
 
@@ -256,8 +258,10 @@ static void dwc2_phy_init(struct dwc2_hsotg *hsotg, bool 
select_phy)
dwc2_hs_phy_init(hsotg, select_phy);
}
 
-   hs_phy_type = hsotg-hwcfg2  GHWCFG2_HS_PHY_TYPE_MASK;
-   fs_phy_type = hsotg-hwcfg2  GHWCFG2_FS_PHY_TYPE_MASK;
+   hs_phy_type = (hsotg-hwcfg2  GHWCFG2_HS_PHY_TYPE_MASK) 
+ GHWCFG2_HS_PHY_TYPE_SHIFT
+   fs_phy_type = (hsotg-hwcfg2  GHWCFG2_FS_PHY_TYPE_MASK) 
+ GHWCFG2_FS_PHY_TYPE_SHIFT
 
if (hs_phy_type == GHWCFG2_HS_PHY_TYPE_ULPI 
fs_phy_type == GHWCFG2_FS_PHY_TYPE_DEDICATED 
@@ -279,7 +283,8 @@ static int dwc2_gahbcfg_init(struct dwc2_hsotg *hsotg)
 {
u32 ahbcfg = 0;
 
-   switch (hsotg-hwcfg2  GHWCFG2_ARCHITECTURE_MASK) {
+   switch ((hsotg-hwcfg2  GHWCFG2_ARCHITECTURE_MASK) 
+   GHWCFG2_ARCHITECTURE_SHIFT) {
case GHWCFG2_EXT_DMA_ARCH:
dev_err(hsotg-dev, External DMA Mode not supported\n);
return -EINVAL;
@@ -290,7 +295,7 @@ static int dwc2_gahbcfg_init(struct dwc2_hsotg *hsotg)
 * Old value was GAHBCFG_HBSTLEN_INCR - done for
 * Host mode ISOC in issue fix - vahrama
 */
-   ahbcfg |= GAHBCFG_HBSTLEN_INCR4;
+   ahbcfg |= GAHBCFG_HBSTLEN_INCR4  GAHBCFG_HBSTLEN_SHIFT;
break;
 
case GHWCFG2_SLAVE_ONLY_ARCH:
@@ -331,7 +336,8 @@ static void dwc2_gusbcfg_init(struct dwc2_hsotg *hsotg)
usbcfg = readl(hsotg-regs + GUSBCFG);
usbcfg = ~(GUSBCFG_HNPCAP | GUSBCFG_SRPCAP);
 
-   switch (hsotg-hwcfg2  GHWCFG2_OP_MODE_MASK) {
+   switch ((hsotg-hwcfg2  GHWCFG2_OP_MODE_MASK) 
+   GHWCFG_OP_MODE_SHIFT) {
case GHWCFG2_OP_MODE_HNP_SRP_CAPABLE:
if (hsotg-core_params-otg_cap ==
DWC2_CAP_PARAM_HNP_SRP_CAPABLE)
@@ -604,7 +610,8 @@ void dwc2_core_host_init(struct dwc2_hsotg *hsotg)
}
 
if (hsotg-core_params-dma_desc_enable  0) {
-   u32 op_mode = hsotg-hwcfg2  GHWCFG2_OP_MODE_MASK;
+   u32 op_mode = (hsotg-hwcfg2  GHWCFG2_OP_MODE_MASK) 
+ GHCFG2_OP_MODE_SHIFT;
 
if (hsotg-snpsid  DWC2_CORE_REV_2_90a ||
!(hsotg-hwcfg4  GHWCFG4_DESC_DMA) ||
@@ -1671,7 +1678,8 @@ u32 dwc2_calc_frame_interval(struct dwc2_hsotg *hsotg)
if (!(usbcfg  GUSBCFG_PHYSEL)  (usbcfg  GUSBCFG_ULPI_UTMI_SEL) 
!(usbcfg  GUSBCFG_PHYIF16))
clock = 60;
-   if ((usbcfg  GUSBCFG_PHYSEL)  (hwcfg2  GHWCFG2_FS_PHY_TYPE_MASK) ==
+   if ((usbcfg  GUSBCFG_PHYSEL

[PATCH 13/13] staging: dwc2: make dwc2_core_params documentation more complete

[Matthijs Kooijman]

Some of the defaults were missing or unclear. In particular, I suspect
the defaults were documented assuming there were still module parameters
and taking the default module parameters into account. Now, the defaults
are the values that will get chosen when the params passed to
dwc2_hcd_init are all -1.

Signed-off-by: Matthijs Kooijman matth...@stdin.nl
---
 drivers/staging/dwc2/core.h | 84 ++---
 1 file changed, 57 insertions(+), 27 deletions(-)

diff --git a/drivers/staging/dwc2/core.h b/drivers/staging/dwc2/core.h
index 67d6dc7..c106db8 100644
--- a/drivers/staging/dwc2/core.h
+++ b/drivers/staging/dwc2/core.h
@@ -68,16 +68,15 @@ enum dwc2_lx_state {
 /**
  * struct dwc2_core_params - Parameters for configuring the core
  *
- * @otg_cap:Specifies the OTG capabilities. The driver will
- *  automatically detect the value for this parameter if
- *  none is specified.
- *   0 - HNP and SRP capable (default)
+ * @otg_cap:Specifies the OTG capabilities.
+ *   0 - HNP and SRP capable
  *   1 - SRP Only capable
- *   2 - No HNP/SRP capable
+ *   2 - No HNP/SRP capable (always available)
+ *  Defaults to best available option (0, 1, then 2)
  * @dma_enable: Specifies whether to use slave or DMA mode for 
accessing
  *  the data FIFOs. The driver will automatically detect 
the
  *  value for this parameter if none is specified.
- *   0 - Slave
+ *   0 - Slave (always available)
  *   1 - DMA (default, if available)
  * @dma_desc_enable:When DMA mode is enabled, specifies whether to use
  *  address DMA mode or descriptor DMA mode for accessing
@@ -88,39 +87,58 @@ enum dwc2_lx_state {
  * @speed:  Specifies the maximum speed of operation in host and
  *  device mode. The actual speed depends on the speed of
  *  the attached device and the value of phy_type.
- *   0 - High Speed (default)
+ *   0 - High Speed
+ *   (default when phy_type is UTMI+ or ULPI)
  *   1 - Full Speed
+ *   (default when phy_type is Full Speed)
  * @host_support_fs_ls_low_power: Specifies whether low power mode is supported
  *  when attached to a Full Speed or Low Speed device in
  *  host mode.
  *   0 - Don't support low power mode (default)
  *   1 - Support low power mode
  * @host_ls_low_power_phy_clk: Specifies the PHY clock rate in low power mode
- *  when connected to a Low Speed device in host mode. This
- *  parameter is applicable only if
- *  host_support_fs_ls_low_power is enabled. If phy_type is
- *  set to FS then defaults to 6 MHZ otherwise 48 MHZ.
+ *  when connected to a Low Speed device in host
+ *  mode. This parameter is applicable only if
+ *  host_support_fs_ls_low_power is enabled.
  *   0 - 48 MHz
+ *   (default when phy_type is UTMI+ or ULPI)
  *   1 - 6 MHz
+ *   (default when phy_type is Full Speed)
  * @enable_dynamic_fifo: 0 - Use coreConsultant-specified FIFO size parameters
- *   1 - Allow dynamic FIFO sizing (default)
+ *   1 - Allow dynamic FIFO sizing (default, if available)
  * @host_rx_fifo_size:  Number of 4-byte words in the Rx FIFO in host mode when
  *  dynamic FIFO sizing is enabled
- *   16 to 32768 (default 1024)
+ *   16 to 32768
+ *  Actual maximum value is autodetected and also
+ *  the default.
  * @host_nperio_tx_fifo_size: Number of 4-byte words in the non-periodic Tx 
FIFO
  *  in host mode when dynamic FIFO sizing is enabled
- *   16 to 32768 (default 1024)
+ *   16 to 32768
+ *  Actual maximum value is autodetected and also
+ *  the default.
  * @host_perio_tx_fifo_size: Number of 4-byte words in the periodic Tx FIFO in
  *  host mode when dynamic FIFO sizing is enabled
- *   16 to 32768 (default 1024)
+ *   16 to 32768
+ *  Actual maximum value is autodetected and also
+ *  the default.
  * @max_transfer_size:  The maximum transfer size supported, in bytes
- *   2047 to 65,535 (default 65,535

[PATCH 10/13] staging: dwc2: properly mask the GRXFSIZ register

[Matthijs Kooijman]

Bits 16-31 are reserved, so the old code just reads the whole register to
get bits 0-15, assuming the reserved bits would be 0 (which seems true
on current hardware, but who knows...).

This commit properly masks out the reserved bits when reading and
doesn't touch the reserved bits while writing.

Signed-off-by: Matthijs Kooijman matth...@stdin.nl
---
 drivers/staging/dwc2/core.c | 23 ---
 drivers/staging/dwc2/hw.h   |  2 ++
 2 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/dwc2/core.c b/drivers/staging/dwc2/core.c
index 5c80b21..025760e 100644
--- a/drivers/staging/dwc2/core.c
+++ b/drivers/staging/dwc2/core.c
@@ -405,7 +405,9 @@ int dwc2_core_init(struct dwc2_hsotg *hsotg, bool 
select_phy, int irq)
 
hsotg-total_fifo_size = hsotg-hwcfg3  GHWCFG3_DFIFO_DEPTH_SHIFT 
GHWCFG3_DFIFO_DEPTH_MASK  GHWCFG3_DFIFO_DEPTH_SHIFT;
-   hsotg-rx_fifo_size = readl(hsotg-regs + GRXFSIZ);
+   hsotg-rx_fifo_size = (readl(hsotg-regs + GRXFSIZ) 
+  GRXFSIZ_RXF_DEP_MASK) 
+ GRXFSIZ_RXF_DEP_SHIFT;
hsotg-nperio_tx_fifo_size =
readl(hsotg-regs + GNPTXFSIZ)  16  0x;
 
@@ -510,7 +512,7 @@ void dwc2_disable_host_interrupts(struct dwc2_hsotg *hsotg)
 static void dwc2_config_fifos(struct dwc2_hsotg *hsotg)
 {
struct dwc2_core_params *params = hsotg-core_params;
-   u32 nptxfsiz, hptxfsiz, dfifocfg;
+   u32 nptxfsiz, hptxfsiz, dfifocfg, grxfsiz;
 
if (!params-enable_dynamic_fifo)
return;
@@ -523,9 +525,12 @@ static void dwc2_config_fifos(struct dwc2_hsotg *hsotg)
params-host_perio_tx_fifo_size);
 
/* Rx FIFO */
-   dev_dbg(hsotg-dev, initial grxfsiz=%08x\n,
-   readl(hsotg-regs + GRXFSIZ));
-   writel(params-host_rx_fifo_size, hsotg-regs + GRXFSIZ);
+   grxfsiz = readl(hsotg-regs + GRXFSIZ);
+   dev_dbg(hsotg-dev, initial grxfsiz=%08x\n, grxfsiz);
+   grxfsiz = ~GRXFSIZ_RXF_DEP_MASK;
+   grxfsiz |= params-host_rx_fifo_size 
+  GRXFSIZ_RXF_DEP_SHIFT  GRXFSIZ_RXF_DEP_MASK;
+   writel(grxfsiz, hsotg-regs + GRXFSIZ);
dev_dbg(hsotg-dev, new grxfsiz=%08x\n, readl(hsotg-regs + GRXFSIZ));
 
/* Non-periodic Tx FIFO */
@@ -2120,7 +2125,9 @@ int dwc2_set_param_host_rx_fifo_size(struct dwc2_hsotg 
*hsotg, int val)
int valid = 1;
int retval = 0;
 
-   if (val  16 || val  readl(hsotg-regs + GRXFSIZ))
+   if (val  16 || val  (readl(hsotg-regs + GRXFSIZ) 
+  GRXFSIZ_RXF_DEP_MASK) 
+ GRXFSIZ_RXF_DEP_SHIFT)
valid = 0;
 
if (!valid) {
@@ -2128,7 +2135,9 @@ int dwc2_set_param_host_rx_fifo_size(struct dwc2_hsotg 
*hsotg, int val)
dev_err(hsotg-dev,
%d invalid for host_rx_fifo_size. Check HW 
configuration.\n,
val);
-   val = readl(hsotg-regs + GRXFSIZ);
+   val = (readl(hsotg-regs + GRXFSIZ) 
+  GRXFSIZ_RXF_DEP_MASK) 
+ GRXFSIZ_RXF_DEP_SHIFT;
dev_dbg(hsotg-dev, Setting host_rx_fifo_size to %d\n, val);
retval = -EINVAL;
}
diff --git a/drivers/staging/dwc2/hw.h b/drivers/staging/dwc2/hw.h
index b5268882..4f813b8 100644
--- a/drivers/staging/dwc2/hw.h
+++ b/drivers/staging/dwc2/hw.h
@@ -184,6 +184,8 @@
 #define GRXSTS_EPNUM_SHIFT 0
 
 #define GRXFSIZHSOTG_REG(0x024)
+#define GRXFSIZ_RXF_DEP_MASK   (0x  0)
+#define GRXFSIZ_RXF_DEP_SHIFT  0
 
 #define GNPTXFSIZ  HSOTG_REG(0x028)
 #define GNPTXFSIZ_NP_TXF_DEP_MASK  (0x  16)
-- 
1.8.3.rc1

--
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 01/13] staging: dwc2: fix naming of register constants

[Matthijs Kooijman]

A generic set of FIFOSIZE_* constants was used for both the GNPTXFSIZ
and HPTXFSIZ registers, even where a specific set of constants was a
available for the GNPTXFSIZ register. This change adds specific
constants for the HPTXFSIZ register as well, changes the code to use
those specific constants and removes the generic constants.

Signed-off-by: Matthijs Kooijman matth...@stdin.nl
---
 drivers/staging/dwc2/core.c | 10 ++
 drivers/staging/dwc2/hw.h   | 14 +++---
 2 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/dwc2/core.c b/drivers/staging/dwc2/core.c
index e3a0e77..ebd0f12 100644
--- a/drivers/staging/dwc2/core.c
+++ b/drivers/staging/dwc2/core.c
@@ -526,9 +526,11 @@ static void dwc2_config_fifos(struct dwc2_hsotg *hsotg)
dev_dbg(hsotg-dev, initial gnptxfsiz=%08x\n,
readl(hsotg-regs + GNPTXFSIZ));
nptxfsiz = params-host_nperio_tx_fifo_size 
-  FIFOSIZE_DEPTH_SHIFT  FIFOSIZE_DEPTH_MASK;
+  GNPTXFSIZ_NP_TXF_DEP_SHIFT 
+  GNPTXFSIZ_NP_TXF_DEP_MASK;
nptxfsiz |= params-host_rx_fifo_size 
-   FIFOSIZE_STARTADDR_SHIFT  FIFOSIZE_STARTADDR_MASK;
+   GNPTXFSIZ_NP_TXF_ST_ADDR_SHIFT 
+   GNPTXFSIZ_NP_TXF_ST_ADDR_MASK;
writel(nptxfsiz, hsotg-regs + GNPTXFSIZ);
dev_dbg(hsotg-dev, new gnptxfsiz=%08x\n,
readl(hsotg-regs + GNPTXFSIZ));
@@ -537,10 +539,10 @@ static void dwc2_config_fifos(struct dwc2_hsotg *hsotg)
dev_dbg(hsotg-dev, initial hptxfsiz=%08x\n,
readl(hsotg-regs + HPTXFSIZ));
ptxfsiz = params-host_perio_tx_fifo_size 
- FIFOSIZE_DEPTH_SHIFT  FIFOSIZE_DEPTH_MASK;
+ HPTXFSIZ_P_TXF_DEP_SHIFT  HPTXFSIZ_P_TXF_DEP_MASK;
ptxfsiz |= (params-host_rx_fifo_size +
params-host_nperio_tx_fifo_size) 
-  FIFOSIZE_STARTADDR_SHIFT  FIFOSIZE_STARTADDR_MASK;
+  HPTXFSIZ_P_TXF_ST_ADDR_SHIFT  HPTXFSIZ_P_TXF_ST_ADDR_MASK;
writel(ptxfsiz, hsotg-regs + HPTXFSIZ);
dev_dbg(hsotg-dev, new hptxfsiz=%08x\n,
readl(hsotg-regs + HPTXFSIZ));
diff --git a/drivers/staging/dwc2/hw.h b/drivers/staging/dwc2/hw.h
index 382a1d7..fd56963 100644
--- a/drivers/staging/dwc2/hw.h
+++ b/drivers/staging/dwc2/hw.h
@@ -392,6 +392,13 @@
 
 #define HPTXFSIZ   HSOTG_REG(0x100)
 
+#define HPTXFSIZ_P_TXF_DEP_MASK(0x  16)
+#define HPTXFSIZ_P_TXF_DEP_SHIFT   16
+#define HPTXFSIZ_P_TXF_ST_ADDR_MASK(0x  0)
+#define HPTXFSIZ_P_TXF_ST_ADDR_SHIFT   0
+
+/* Device mode registers */
+
 #define DPTXFSIZN(_a)  HSOTG_REG(0x104 + (((_a) - 1) * 4))
 #define DPTXFSIZN_DP_TXF_SIZE_MASK (0x  16)
 #define DPTXFSIZN_DP_TXF_SIZE_SHIFT16
@@ -401,13 +408,6 @@
 #define DPTXFSIZN_DP_TXF_ST_ADDR_MASK  (0x  0)
 #define DPTXFSIZN_DP_TXF_ST_ADDR_SHIFT 0
 
-#define FIFOSIZE_DEPTH_MASK(0x  16)
-#define FIFOSIZE_DEPTH_SHIFT   16
-#define FIFOSIZE_STARTADDR_MASK(0x  0)
-#define FIFOSIZE_STARTADDR_SHIFT   0
-
-/* Device mode registers */
-
 #define DCFG   HSOTG_REG(0x800)
 #define DCFG_EPMISCNT_MASK (0x1f  18)
 #define DCFG_EPMISCNT_SHIFT18
-- 
1.8.3.rc1

--
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 08/13] staging: dwc2: re-use hptxfsiz variable

[Matthijs Kooijman]

For some reason, the value of the HPTXFSIZ register was built in the
ptxfsiz variable, while there was also a hptxfsiz variable availble.
Better just use that and remove the (now unused) ptxfsiz variable.

Signed-off-by: Matthijs Kooijman matth...@stdin.nl
---
 drivers/staging/dwc2/core.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/dwc2/core.c b/drivers/staging/dwc2/core.c
index cc23337..b199dbd 100644
--- a/drivers/staging/dwc2/core.c
+++ b/drivers/staging/dwc2/core.c
@@ -510,7 +510,7 @@ void dwc2_disable_host_interrupts(struct dwc2_hsotg *hsotg)
 static void dwc2_config_fifos(struct dwc2_hsotg *hsotg)
 {
struct dwc2_core_params *params = hsotg-core_params;
-   u32 rxfsiz, nptxfsiz, ptxfsiz, hptxfsiz, dfifocfg;
+   u32 rxfsiz, nptxfsiz, hptxfsiz, dfifocfg;
 
if (!params-enable_dynamic_fifo)
return;
@@ -544,12 +544,12 @@ static void dwc2_config_fifos(struct dwc2_hsotg *hsotg)
/* Periodic Tx FIFO */
dev_dbg(hsotg-dev, initial hptxfsiz=%08x\n,
readl(hsotg-regs + HPTXFSIZ));
-   ptxfsiz = params-host_perio_tx_fifo_size 
- HPTXFSIZ_P_TXF_DEP_SHIFT  HPTXFSIZ_P_TXF_DEP_MASK;
-   ptxfsiz |= (params-host_rx_fifo_size +
+   hptxfsiz = params-host_perio_tx_fifo_size 
+  HPTXFSIZ_P_TXF_DEP_SHIFT  HPTXFSIZ_P_TXF_DEP_MASK;
+   hptxfsiz |= (params-host_rx_fifo_size +
params-host_nperio_tx_fifo_size) 
-  HPTXFSIZ_P_TXF_ST_ADDR_SHIFT  HPTXFSIZ_P_TXF_ST_ADDR_MASK;
-   writel(ptxfsiz, hsotg-regs + HPTXFSIZ);
+   HPTXFSIZ_P_TXF_ST_ADDR_SHIFT  HPTXFSIZ_P_TXF_ST_ADDR_MASK;
+   writel(hptxfsiz, hsotg-regs + HPTXFSIZ);
dev_dbg(hsotg-dev, new hptxfsiz=%08x\n,
readl(hsotg-regs + HPTXFSIZ));
 
-- 
1.8.3.rc1

--
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 07/13] staging: dwc2: simplify debug output in dwc_hc_init

[Matthijs Kooijman]

The value of the hcchar register is built from individual values by
shifting and masking. Before, the debug output extracted the individual
values out of the complete hcchar register again by doing the reverse.
This commit makes the debug output use the original values instead.

One debug message got removed, since it would always print a fixed value
of zero.

Signed-off-by: Matthijs Kooijman matth...@stdin.nl
---
 drivers/staging/dwc2/core.c | 25 +
 1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/dwc2/core.c b/drivers/staging/dwc2/core.c
index 23677b3..cc23337 100644
--- a/drivers/staging/dwc2/core.c
+++ b/drivers/staging/dwc2/core.c
@@ -892,26 +892,20 @@ void dwc2_hc_init(struct dwc2_hsotg *hsotg, struct 
dwc2_host_chan *chan)
dev_vdbg(hsotg-dev, set HCCHAR(%d) to %08x\n,
 hc_num, hcchar);
 
-   dev_vdbg(hsotg-dev, %s: Channel %d\n, __func__, hc_num);
+   dev_vdbg(hsotg-dev, %s: Channel %d\n,
+__func__, hc_num);
dev_vdbg(hsotg-dev,Dev Addr: %d\n,
-hcchar  HCCHAR_DEVADDR_SHIFT 
-HCCHAR_DEVADDR_MASK  HCCHAR_DEVADDR_SHIFT);
+chan-dev_addr);
dev_vdbg(hsotg-dev,Ep Num: %d\n,
-hcchar  HCCHAR_EPNUM_SHIFT 
-HCCHAR_EPNUM_MASK  HCCHAR_EPNUM_SHIFT);
+chan-ep_num);
dev_vdbg(hsotg-dev,Is In: %d\n,
-!!(hcchar  HCCHAR_EPDIR));
+chan-ep_is_in);
dev_vdbg(hsotg-dev,Is Low Speed: %d\n,
-!!(hcchar  HCCHAR_LSPDDEV));
+chan-speed == USB_SPEED_LOW);
dev_vdbg(hsotg-dev,Ep Type: %d\n,
-hcchar  HCCHAR_EPTYPE_SHIFT 
-HCCHAR_EPTYPE_MASK  HCCHAR_EPTYPE_SHIFT);
+chan-ep_type);
dev_vdbg(hsotg-dev,Max Pkt: %d\n,
-hcchar  HCCHAR_MPS_SHIFT 
-HCCHAR_MPS_MASK  HCCHAR_MPS_SHIFT);
-   dev_vdbg(hsotg-dev,Multi Cnt: %d\n,
-hcchar  HCCHAR_MULTICNT_SHIFT 
-HCCHAR_MULTICNT_MASK  HCCHAR_MULTICNT_SHIFT);
+chan-max_packet);
}
 
/* Program the HCSPLT register for SPLITs */
@@ -941,8 +935,7 @@ void dwc2_hc_init(struct dwc2_hsotg *hsotg, struct 
dwc2_host_chan *chan)
dev_vdbg(hsotg-dev, is_in %d\n,
 chan-ep_is_in);
dev_vdbg(hsotg-dev, Max Pkt %d\n,
-hcchar  HCCHAR_MPS_SHIFT 
-HCCHAR_MPS_MASK  HCCHAR_MPS_SHIFT);
+chan-max_packet);
dev_vdbg(hsotg-dev, xferlen %d\n,
 chan-xfer_len);
}
-- 
1.8.3.rc1

--
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 12/13] staging: dwc2: validate the value for phy_utmi_width

[Matthijs Kooijman]

The HWCFG4 register stores the supported utmi width values (8, 16 or
both). This commit reads that value and validates the configured value
against that.

If no (valid) value is given, the parameter defaulted to 8 bits
previously.  However, the documentation for dwc2_core_params_struct
suggests that the default should have been 16. Also, the pci bindings
explicitely set the value to 16, so this commit changes the default to
16 bits (if supported, 8 bits otherwise).

With the default changed, the value set in pci.c is changed to -1 to
make it autodetected as well.

Signed-off-by: Matthijs Kooijman matth...@stdin.nl
---
 drivers/staging/dwc2/core.c | 27 +++
 drivers/staging/dwc2/core.h |  5 +
 drivers/staging/dwc2/hw.h   |  3 +++
 drivers/staging/dwc2/pci.c  |  2 +-
 4 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/dwc2/core.c b/drivers/staging/dwc2/core.c
index 65e9bc6..4bdc74f 100644
--- a/drivers/staging/dwc2/core.c
+++ b/drivers/staging/dwc2/core.c
@@ -2380,14 +2380,29 @@ int dwc2_set_param_phy_ulpi_ext_vbus(struct dwc2_hsotg 
*hsotg, int val)
 
 int dwc2_set_param_phy_utmi_width(struct dwc2_hsotg *hsotg, int val)
 {
+   int valid = 0;
int retval = 0;
 
-   if (DWC2_PARAM_TEST(val, 8, 8)  DWC2_PARAM_TEST(val, 16, 16)) {
+   switch (hsotg-hw_params.utmi_phy_data_width) {
+   case GHWCFG4_UTMI_PHY_DATA_WIDTH_8:
+   valid = (val == 8);
+   break;
+   case GHWCFG4_UTMI_PHY_DATA_WIDTH_16:
+   valid = (val == 16);
+   break;
+   case GHWCFG4_UTMI_PHY_DATA_WIDTH_8_OR_16:
+   valid = (val == 8 || val == 16);
+   break;
+   }
+
+   if (!valid) {
if (val = 0) {
-   dev_err(hsotg-dev, Wrong value for phy_utmi_width\n);
-   dev_err(hsotg-dev, phy_utmi_width must be 8 or 16\n);
+   dev_err(hsotg-dev,
+   %d invalid for phy_utmi_width. Check HW 
configuration.\n,
+   val);
}
-   val = 8;
+   val = (hsotg-hw_params.utmi_phy_data_width ==
+  GHWCFG4_UTMI_PHY_DATA_WIDTH_8) ? 8 : 16;
dev_dbg(hsotg-dev, Setting phy_utmi_width to %d\n, val);
retval = -EINVAL;
}
@@ -2685,6 +2700,8 @@ int dwc2_get_hwparams(struct dwc2_hsotg *hsotg)
  GHWCFG4_NUM_DEV_PERIO_IN_EP_SHIFT;
hw-dma_desc_enable = !!(hwcfg4  GHWCFG4_DESC_DMA);
hw-power_optimized = !!(hwcfg4  GHWCFG4_POWER_OPTIMIZ);
+   hw-utmi_phy_data_width = (hwcfg4  GHWCFG4_UTMI_PHY_DATA_WIDTH_MASK) 
+ GHWCFG4_UTMI_PHY_DATA_WIDTH_SHIFT;
 
/* fifo sizes */
hw-host_rx_fifo_size = (grxfsiz  GRXFSIZ_RXF_DEP_MASK) 
@@ -2709,6 +2726,8 @@ int dwc2_get_hwparams(struct dwc2_hsotg *hsotg)
hw-hs_phy_type);
dev_dbg(hsotg-dev,   fs_phy_type=%d\n,
hw-fs_phy_type);
+   dev_dbg(hsotg-dev,   utmi_phy_data_wdith=%d\n,
+   hw-utmi_phy_data_width);
dev_dbg(hsotg-dev,   num_dev_ep=%d\n,
hw-num_dev_ep);
dev_dbg(hsotg-dev,   num_dev_perio_in_ep=%d\n,
diff --git a/drivers/staging/dwc2/core.h b/drivers/staging/dwc2/core.h
index 7dbe2fe..67d6dc7 100644
--- a/drivers/staging/dwc2/core.h
+++ b/drivers/staging/dwc2/core.h
@@ -238,6 +238,10 @@ struct dwc2_core_params {
  *   2 - FS pins shared with UTMI+ pins
  *   3 - FS pins shared with ULPI pins
  * @total_fifo_size:Total internal RAM for FIFOs (bytes)
+ * @utmi_phy_data_width UTMI+ PHY data width
+ *   0 - 8 bits
+ *   1 - 16 bits
+ *   2 - 8 or 16 bits
  * @snpsid: Value from SNPSID register
  */
 struct dwc2_hw_params {
@@ -262,6 +266,7 @@ struct dwc2_hw_params {
unsigned num_dev_perio_in_ep:4;
unsigned total_fifo_size:16;
unsigned power_optimized:1;
+   unsigned utmi_phy_data_width:2;
u32 snpsid;
 };
 
diff --git a/drivers/staging/dwc2/hw.h b/drivers/staging/dwc2/hw.h
index 4f813b8..278d887 100644
--- a/drivers/staging/dwc2/hw.h
+++ b/drivers/staging/dwc2/hw.h
@@ -305,6 +305,9 @@
 #define GHWCFG4_NUM_DEV_MODE_CTRL_EP_SHIFT 16
 #define GHWCFG4_UTMI_PHY_DATA_WIDTH_MASK   (0x3  14)
 #define GHWCFG4_UTMI_PHY_DATA_WIDTH_SHIFT  14
+#define GHWCFG4_UTMI_PHY_DATA_WIDTH_8  0
+#define GHWCFG4_UTMI_PHY_DATA_WIDTH_16 1
+#define GHWCFG4_UTMI_PHY_DATA_WIDTH_8_OR_162
 #define GHWCFG4_XHIBER (1  7)
 #define GHWCFG4_HIBER  (1  6)
 #define GHWCFG4_MIN_AHB_FREQ   (1  5)
diff --git a/drivers/staging/dwc2/pci.c b/drivers/staging/dwc2/pci.c
index 3ca54d6..308d7c6 100644
--- a/drivers/staging/dwc2/pci.c
+++ b/drivers/staging/dwc2/pci.c
@@ -74,7 +74,7

[PATCH 09/13] staging: dwc2: remove redundant register reads

[Matthijs Kooijman]

For calculating FIFO offsets, the sizes of preceding fifos need to be
known. For filling the GDFIFOCFG register, these fifo sizes were read
from hardware registers. However, these values were written to these
registers just a few lines before, so we can just use the values written
instead.

Signed-off-by: Matthijs Kooijman matth...@stdin.nl
---
 drivers/staging/dwc2/core.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/dwc2/core.c b/drivers/staging/dwc2/core.c
index b199dbd..5c80b21 100644
--- a/drivers/staging/dwc2/core.c
+++ b/drivers/staging/dwc2/core.c
@@ -510,7 +510,7 @@ void dwc2_disable_host_interrupts(struct dwc2_hsotg *hsotg)
 static void dwc2_config_fifos(struct dwc2_hsotg *hsotg)
 {
struct dwc2_core_params *params = hsotg-core_params;
-   u32 rxfsiz, nptxfsiz, hptxfsiz, dfifocfg;
+   u32 nptxfsiz, hptxfsiz, dfifocfg;
 
if (!params-enable_dynamic_fifo)
return;
@@ -560,11 +560,10 @@ static void dwc2_config_fifos(struct dwc2_hsotg *hsotg)
 * include RxFIFO, NPTXFIFO and HPTXFIFO
 */
dfifocfg = readl(hsotg-regs + GDFIFOCFG);
-   rxfsiz = readl(hsotg-regs + GRXFSIZ)  0x;
-   nptxfsiz = readl(hsotg-regs + GNPTXFSIZ)  16  0x;
-   hptxfsiz = readl(hsotg-regs + HPTXFSIZ)  16  0x;
dfifocfg = ~GDFIFOCFG_EPINFOBASE_MASK;
-   dfifocfg |= (rxfsiz + nptxfsiz + hptxfsiz) 
+   dfifocfg |= (params-host_rx_fifo_size +
+params-host_nperio_tx_fifo_size +
+params-host_perio_tx_fifo_size) 
GDFIFOCFG_EPINFOBASE_SHIFT 
GDFIFOCFG_EPINFOBASE_MASK;
writel(dfifocfg, hsotg-regs + GDFIFOCFG);
-- 
1.8.3.rc1

--
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 3/3] staging: dwc2: Don't touch the dma_mask when dma is disabled

[Matthijs Kooijman]

There was some code that cleared the dma_mask when dma was disabled in
the driver. Given that clearing the mask doesn't actually tell the usb
core we're not using dma, and a previous commit explicitely sets the
hcd-self.uses_dma value, it seems these values are unneeded and can
only potentially cause problems (when reloading a module, for example).

Signed-off-by: Matthijs Kooijman matth...@stdin.nl
---
 drivers/staging/dwc2/hcd.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/staging/dwc2/hcd.c b/drivers/staging/dwc2/hcd.c
index 22ac24c..74498ef 100644
--- a/drivers/staging/dwc2/hcd.c
+++ b/drivers/staging/dwc2/hcd.c
@@ -2756,9 +2756,6 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
if (dma_set_coherent_mask(hsotg-dev, DMA_BIT_MASK(31))  0)
dev_warn(hsotg-dev,
 can't enable workaround for 2GB RAM\n);
-   } else {
-   dma_set_mask(hsotg-dev, 0);
-   dma_set_coherent_mask(hsotg-dev, 0);
}
 
hcd = usb_create_hcd(dwc2_hc_driver, hsotg-dev, dev_name(hsotg-dev));
-- 
1.8.3.rc1

--
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 1/3] staging: dwc2: disable dma when no dma_mask was setup

[Matthijs Kooijman]

If the platform or bus driver failed to setup a dma_mask, but the
hardware advertises support for DMA, before DMA would be enabled in
dwc2, but disabled in the usb core, making all connectivity break.

With this commit, the dwc2 driver will emit a warning and fall back to
slave mode in this case.

Note that since commit 642f2ec (staging: dwc2: Fix dma-enabled platform
devices using a default dma_mask) the platform bindings make sure a DMA
mask is always present, but having this check here anyway is probably a
good from a defensive programming standpoint (in case of changes to
platform.c or addition of new glue layers).

Signed-off-by: Matthijs Kooijman matth...@stdin.nl
---
 drivers/staging/dwc2/hcd.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/drivers/staging/dwc2/hcd.c b/drivers/staging/dwc2/hcd.c
index 8cbdbd9..4791d81 100644
--- a/drivers/staging/dwc2/hcd.c
+++ b/drivers/staging/dwc2/hcd.c
@@ -2740,6 +2740,15 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
/* Validate parameter values */
dwc2_set_parameters(hsotg, params);
 
+   /* Check if the bus driver or platform code has setup a dma_mask */
+   if (hsotg-core_params-dma_enable  0 
+   hsotg-dev-dma_mask == NULL) {
+   dev_warn(hsotg-dev,
+dma_mask not set, disabling DMA\n);
+   hsotg-core_params-dma_enable = 0;
+   hsotg-core_params-dma_desc_enable = 0;
+   }
+
/* Set device flags indicating whether the HCD supports DMA */
if (hsotg-core_params-dma_enable  0) {
if (dma_set_mask(hsotg-dev, DMA_BIT_MASK(32))  0)
-- 
1.8.3.rc1

--
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 0/3] staging: dwc2: Add some dma-related defensive programming

[Matthijs Kooijman]

Here's a resend of three patches I sent a while ago as part of a bigger
series. These were the unimportant patches to be included later, so here
they are again.

Gr.

Matthijs

Matthijs Kooijman (3):
  staging: dwc2: disable dma when no dma_mask was setup
  staging: dwc2: when dma is disabled, clear hcd-self.uses_dma
  staging: dwc2: Don't touch the dma_mask when dma is disabled

 drivers/staging/dwc2/hcd.c | 15 ---
 1 file changed, 12 insertions(+), 3 deletions(-)

-- 
1.8.3.rc1

--
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/3] staging: dwc2: when dma is disabled, clear hcd-self.uses_dma

[Matthijs Kooijman]

When dma is disabled inside dwc2 (because the hardware does not support
it, or the code was changed to disable it for testing), let the usb core
know about this by clearing hcd-self.uses_dma.

By default, the usb core assumes that dma is used when a dma_mask is
set, but this might not always match the dma_enable value in dwc2. To
prevent problems resulting from a mismatch, better to explicitely
disable dma in this case (though everything seemed to work with the
wrong value of uses_dma as well, probably only resulted in some unneeded
work).

Signed-off-by: Matthijs Kooijman matth...@stdin.nl
---
 drivers/staging/dwc2/hcd.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/staging/dwc2/hcd.c b/drivers/staging/dwc2/hcd.c
index 4791d81..22ac24c 100644
--- a/drivers/staging/dwc2/hcd.c
+++ b/drivers/staging/dwc2/hcd.c
@@ -2765,6 +2765,9 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
if (!hcd)
goto error1;
 
+   if (hsotg-core_params-dma_enable = 0)
+   hcd-self.uses_dma = 0;
+
hcd-has_tt = 1;
 
spin_lock_init(hsotg-lock);
-- 
1.8.3.rc1

--
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 00/13] staging: dwc2: Register-related cleanups

[Matthijs Kooijman]

Hi Paul,

 I was about to send out another patch which enhances the scheduling
 code in the driver. It finally makes the driver work well on the
 Raspberry Pi, at least in my testing. So I would rather submit that
 code first rather than risk breaking it with this big set of changes.
 
 So can you rebase this series on top of that patch once I submit it?
Sure. Would be good if you would review and ack the patches as they are
now, so they can be merged directly after I rebase?

Gr.

Matthijs
--
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 01/13] staging: dwc2: fix naming of register constants

[Matthijs Kooijman]

Hi Paul,

On Wed, Jul 17, 2013 at 06:44:04PM +, Paul Zimmerman wrote:
  From: Matthijs Kooijman [mailto:matth...@stdin.nl]
  Sent: Wednesday, July 17, 2013 8:10 AM
  
  A generic set of FIFOSIZE_* constants was used for both the GNPTXFSIZ
  and HPTXFSIZ registers, even where a specific set of constants was a
  available for the GNPTXFSIZ register. This change adds specific
  constants for the HPTXFSIZ register as well, changes the code to use
  those specific constants and removes the generic constants.
 
 Hi Matthijs,
 
 I'd rather not do this, if you don't mind. The peripheral-mode register
 set also has several FIFO size registers, all of which use the exact
 same format. So I think it makes more sense to keep the generic
 versions.
Ok, I'll reverse the patch and drop the specific constants in favor of
the generic ones, then.

Gr.

Matthijs
--
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

dwc2: Invalid (?) initialization of PCGCTL register

[Matthijs Kooijman]

Hi Paul,

could you perhaps look up some documentation on the PCGCTL register for me?

The issue John and I have been facing is that on one particular board,
the dwc2 driver fails to initialize saying:

dwc2 101c.usb: Bad value for GSNPSID: 0x

We've traced this back to the initalization done in the uboot bootloader. On
the broken board, uboot does (among other things):

RT2882_REG(0xB01C0E00) = 0xF;//disable USB module, optimize for 
power-saving

E.g, enable bits 0-3 in the PCGCTL register. I can reproduce this
problem on the good boards by doing the same thing somewhere early in
the kernel initalization. Further experimentation shows that writing 0x4
to the PCGTL register is enough to break things.

In order to better understand what's going on, I'm not wondering what
the meaning of the PCGCTL register is, and in particular bit 2 (0x4).
Could you clarify?


As said, on most of our boards this problem does not occur. Reviewing
a the bootloader assembler code shows that those boards do not write the
PCGCTL register. However, the reference uboot code from Ralink _does_
write the register, so it seems good to make the driver work even when
this particular bit was enabled.

Simply resetting the PCGCTL register in hcd_init or somewhere around
there fixes the problem. However, John suggested an even more generic
problem: using the kernel-wide reset controller driver to completely
reset the dwc controller before initializing it (by calling
device_reset()). On the RT3052 SoC there is a global reset register
which can be used to reset the dwc controller. Support for this register
is being added by John currently. Does this seem sane?



Gr.

Matthijs
--
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

Incompleteness of HW capability registers (Was: [PATCH] staging: dwc2: fix thinko in dwc2_hc_set_even_odd_frame())

[Matthijs Kooijman]

Hi Stephen,

  I'm not sure how many of the below driver parameters are actually
  required. I think as least the three fifo size parameters, plus
  max_transfer_size and max_packet_count, are needed.

 I'm curious why any of them are required; why aren't the values embedded
 in the HW registers correct?

As far as I've understood, there are some values (mostly PHY related)
which cannot be detected from the HW registers. My interpretation is
that the HW registers only store parameters used to synthesize the core,
(i.e., I think they store if the core supports UTMI+ and/or ULPI PHYs,
but not what kind of PHY is actually attached).

Also IIUC, the FIFO size registers store the maximum values per FIFO and
a global maximum, but it could be that the sum of the individual maximum
FIFO sizes is more than the global maximum. This is something that could
be fixed in the driver though, by somehow fairly (or otherwise
intelligently) distributing the available FIFO memory over the various
FIFOs.

These are just the suspicions I have encountered while going through the
code, perhaps Paul can confirm them.

Gr.

Matthijs
--
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] staging: dwc2: fix value used in dwc2_set_all_params

[Matthijs Kooijman]

On Thu, Jul 11, 2013 at 02:24:10PM +0200, Julien Delacou wrote:
 From: Julien Delacou julien.dela...@stericsson.com
 
 This fix uses 'value' parameter as it should be instead
 of hardcoded -1.
Woops!

 Signed-off-by: Julien Delacou julien.dela...@stericsson.com
 Acked-by: Paul Zimmerman pa...@synopys.com
Reviewed-by: Matthijs Kooijman matth...@stdin.nl

 ---
  drivers/staging/dwc2/hcd.c |2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/drivers/staging/dwc2/hcd.c b/drivers/staging/dwc2/hcd.c
 index 2ed54b1..ca38aaa 100644
 --- a/drivers/staging/dwc2/hcd.c
 +++ b/drivers/staging/dwc2/hcd.c
 @@ -2690,7 +2690,7 @@ void dwc2_set_all_params(struct dwc2_core_params 
 *params, int value)
   int i;
  
   for (i = 0; i  size; i++)
 - p[i] = -1;
 + p[i] = value;
  }
  EXPORT_SYMBOL_GPL(dwc2_set_all_params);
  
 -- 
 1.7.9.5
 
 --
 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
--
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] staging: dwc2: fix thinko in dwc2_hc_set_even_odd_frame()

[Matthijs Kooijman]

Hi Paul,

 As I mentioned in private email, I was not able to get your
 mainline-based kernel from git://github.com/swarren/linux-rpi.git
 to boot.
Something to try if it's still not working: Try a less fancy SD card. I
found that u-boot didn't want to boot a 16GB SDHC card (IIRC), but using
an older 1GB SD card worked.

Gr.

Matthijs
--
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] staging: dwc2: fix value of dma_mask

[Matthijs Kooijman]

Hi Paul,

On Fri, May 24, 2013 at 04:27:56PM -0700, Paul Zimmerman wrote:
 Passing the value DMA_BIT_MASK(31) to dma_set_mask() causes the
 dwc2-pci driver to sometimes fail (cannot enumerate the connected
 device). Change it to DMA_BIT_MASK(32) instead, which is a more
 sensible value anyway.
Isn't 32 the default value usually? Not sure a about PCI devices,
though. It also made sense to set it to 31, since the comment talks
about a workaround for  2GB ram (and 2GB ~~ 31 bits).

Or, is the workaround really only needed in the coherent mask and is
that the reason why you didn't change the coherent dma mask to 32?

If this is so, wouldn't it make sense to only set the coherent dma mask
and leave the dma mask at whatever default (just guessing here...).

Gr.

Matthijs
--
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: dwc2: Transaction errors with device connected at boot

[Matthijs Kooijman]

Hi Paul,

 I did see a different issue when booting/rebooting the system.
 I discovered that the DMA_BIT_MASK(31)'s in dwc2_hcd_init() were
 randomly preventing the controller from working after boot. When
 I changed both of them to DMA_BIT_MASK(32) the problem went away.
 I am going to submit a patch with that change shortly, maybe you
 could try that and see if it makes a difference for you?
I tried it quickly with that patch and got one succesful boot and one
failed boot, so it seems the patch doesn't help for my problem.

 After making that change, I am unable to reproduce any boot problem
 on my platform (PC with the dwc2 controller on a PCIe bus). I wonder
 if this could be some issue with the Phy on your platform? Maybe you
 could play around with the Phy initialization/resets in the driver
 and see if that makes a difference.
 
 I remember when bringing up the driver, that the sleep at the end of
 dwc2_core_reset() was very critical to making host mode work. Maybe
 you could experiment with that also.
I'll have a look next week, thanks.

Gr.

Matthijs
--
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 0/7] staging: dwc2: miscellaneous cleanups

[Matthijs Kooijman]

Hi Greg,

On Thu, May 16, 2013 at 03:26:25PM -0700, Greg KH wrote:
 On Mon, May 13, 2013 at 07:08:05PM -0700, Paul Zimmerman wrote:
  Here are some dwc2 patches which Matthijs submitted just before the merge
  window opened, looks like they did not get picked up. Please apply.
 
 I think I just got all of these, right?
Yup, you did.

Thanks!
--
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 1/4] staging: dwc2: Set a default dma_mask for platform devices

[Matthijs Kooijman]

Hi Paul,

On Wed, May 08, 2013 at 10:46:29PM +0200, Matthijs Kooijman wrote:
 Paul wrote:
  Hmm. Is it kosher to override these in a driver and force DMA to be
  enabled?
 
 Apparently, since a lot of drivers do it like this. This particular code
 was taken from the ehci-platform driver. See also:
 
   http://thread.gmane.org/gmane.linux.usb.general/86066
 
 The discussion in that thread suggests that even though this is not
 quite the optimal way to do this, it is the accepted way for now and
 changing it needs a bit more complicated changes and more discussion,
 apparently.
 
  What if it has been disabled earlier on purpose, say because the
  platform does not have DMA support? You say This still allows any
  platform code to set any more specific mask if needed, but how would
  that be done exactly?
 Platform code could go over the list of platform devices and (based on the
 device-tree compatible value, for example) change the mask of a device.
 Admittedly, this could set a more specific mask, but not exactly disable
 dma entirely. I guess this is not a usecase for the other drivers?
 
 For the dwc2 driver, I guess the dwc2 hardware would not have dma
 enabled if the system does not support it anyway?
 
 Note that this code only runs for platform devices, so not for pci
 devices, which can disable dma by not setting a dma_mask when combined
 with the next patch.
Does this satisfy your doubts, or do you think further changes are
needed to this series of patches?

Gr.

Matthijs
--
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 1/4] staging: dwc2: Set a default dma_mask for platform devices

[Matthijs Kooijman]

Hi Paul,

 No, seems like other folks think it's OK, so I'm fine with it.
Great.

 I'll ack the patches and send them on once Greg starts taking staging
 patches again.
Forgive my ignorance here, but does that mean it'll wait until 3.11, or
can it still be included in 3.10? I'd like to get at least the first
patch in the series to be in 3.10, since it is needed to make dwc2 work
on ramips (without it, it is broken, not just running without DMA).

Gr.

Matthijs
--
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] USB: set device dma_mask without reference to global data

[Matthijs Kooijman]

Hi folks,

I also bumped into the question of how to set the dma_mask when enabling
the dwc2 driver on the ramips target and found there didn't seem to be
any clear way to get a dma_mask.

It seems to me that in the pre-DT era, a platform_device would get a
dma_mask when it was defined in the board / soc code, which makes sense
since that code knows if a dma_mask is required and what its value
should be (it seems to me that a driver can only know it needs a
dma_mask, but not what value it should have?).

  This probably could be initialized from some DT property. However,
  there's no such property defined right now, and considering that DT is
  supposed to be an ABI, we'd always need the code in this patch as a
  fallback for DTs that were created before any such property was defined.
It seems there has already been a patch to implement this. For
reference, this seems to be the most recent version:

https://lkml.org/lkml/2012/12/4/54

And here's the previous attempt, to which Rob Herring refers in a reply.

https://lists.ozlabs.org/pipermail/devicetree-discuss/2012-March/013180.html

  Equally, since the data is SoC-specific rather than board-specific, and
  is even fairly unlikely to vary between SoC versions since these values
  are all 0x anyway, I don't really see much point in putting it
  into DT, rather than just putting the static data into the driver.

 I mean there is already dev-dev.coherent_dma_mask = DMA_BIT_MASK(32);
 at function of_platform_device_create, why can't add
 dev-dev.dma_mask = dev-dev.coherent_dma_mask after that?
Perhaps it would sense to set the 32-bit mask as a default, but allow to
override this mask from the devicetree for boards that need another
value? Or perhaps override it from the soc code instead?

For the ramips target, the MIPS folks suggested another approach: The
soc code finds the platform_device generated by DT and adds the
dma_mask:

http://www.linux-mips.org/archives/linux-mips/2013-04/msg00162.html

 If DT core can do above things, can we delete dma_mask assignment
 at every driver?
That would seem like a likeably goal to me :-)


Gr.

Matthijs
--
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] USB: set device dma_mask without reference to global data

[Matthijs Kooijman]

Hi,

 For the ramips target, the MIPS folks suggested another approach: The
 soc code finds the platform_device generated by DT and adds the
 dma_mask:
 
 http://www.linux-mips.org/archives/linux-mips/2013-04/msg00162.html

For completeness: It seems this approach is not going to be used after
all. The approach (not this particular patch) was conceived to get ehci
support on ramips, but now that echi also sets a default mask and it
turns out that the other setup needed is best done through a phy device,
this approach is probably not needed anymore for ehci. For dwc2, I guess
it should also set up a dma_mask like ehci does (at least until we sort
out this thread) :-)

Gr.

Matthijs
--
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 1/2] staging: dwc2: when dma is disabled, clear dev-dma_mask

[Matthijs Kooijman]

Hi Greg,

 Greg, is it ok for a HCD to modify hcd-self.uses_dma like this?
perhaps you missed this question in my previous mail? :-)

Gr.

Matthijs

 diff --git a/drivers/staging/dwc2/hcd.c b/drivers/staging/dwc2/hcd.c
 index d9a2055..18a91de 100644
 --- a/drivers/staging/dwc2/hcd.c
 +++ b/drivers/staging/dwc2/hcd.c
 @@ -2766,6 +2766,9 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
 if (!hcd)
 goto error1;
  
 +   if (hsotg-core_params-dma_enable = 0)
 +   hcd-self.uses_dma = 0;
 +
 hcd-has_tt = 1;
  
 spin_lock_init(hsotg-lock);
 
 --
 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
--
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 1/2] staging: dwc2: when dma is disabled, clear dev-dma_mask

[Matthijs Kooijman]

   +   if (hsotg-core_params-dma_enable = 0)
   +   hcd-self.uses_dma = 0;
   +
   hcd-has_tt = 1;

   spin_lock_init(hsotg-lock);
 
 This is okay providing you do it before calling usb_add_hcd().

Great, thanks!

Matthijs
--
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 1/4] staging: dwc2: Set a default dma_mask for platform devices

[Matthijs Kooijman]

Platform devices added through OF usually do not have any dma_mask
pointer set.  In this case, point it at the coherent_dma_mask and set
their value to a 32 bit mask. This still allows any platform code to set
any more specific mask if needed, but makes the driver work for most
dma-enabled hardware.

Signed-off-by: Matthijs Kooijman matth...@stdin.nl
---
 drivers/staging/dwc2/platform.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/staging/dwc2/platform.c b/drivers/staging/dwc2/platform.c
index 74f1b7d..fdf81c2 100644
--- a/drivers/staging/dwc2/platform.c
+++ b/drivers/staging/dwc2/platform.c
@@ -187,6 +187,14 @@ static int dwc2_driver_probe(struct platform_device *dev)
 
hsotg-dev = dev-dev;
 
+   /*
+* Use reasonable defaults so platforms don't have to provide these.
+*/
+   if (!dev-dev.dma_mask)
+   dev-dev.dma_mask = dev-dev.coherent_dma_mask;
+   if (!dev-dev.coherent_dma_mask)
+   dev-dev.coherent_dma_mask = DMA_BIT_MASK(32);
+
irq = platform_get_irq(dev, 0);
if (irq  0) {
dev_err(dev-dev, missing IRQ resource\n);
-- 
1.8.0

--
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 4/4] staging: dwc2: Don't touch the dma_mask when dma is disabled

[Matthijs Kooijman]

There was some code that cleared the dma_mask when dma was disabled in
the driver. Given that clearing the mask doesn't actually tell the usb
core we're not using dma, and a previous commit explicitely sets the
hcd-self.uses_dma value, it seems these values are unneeded and can
only potentially cause problems (when reloading a module, for example).

Signed-off-by: Matthijs Kooijman matth...@stdin.nl
---
 drivers/staging/dwc2/hcd.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/staging/dwc2/hcd.c b/drivers/staging/dwc2/hcd.c
index 18a91de..57c6ee7 100644
--- a/drivers/staging/dwc2/hcd.c
+++ b/drivers/staging/dwc2/hcd.c
@@ -2757,9 +2757,6 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
if (dma_set_coherent_mask(hsotg-dev, DMA_BIT_MASK(31))  0)
dev_warn(hsotg-dev,
 can't enable workaround for 2GB RAM\n);
-   } else {
-   dma_set_mask(hsotg-dev, 0);
-   dma_set_coherent_mask(hsotg-dev, 0);
}
 
hcd = usb_create_hcd(dwc2_hc_driver, hsotg-dev, dev_name(hsotg-dev));
-- 
1.8.0

--
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 3/4] staging: dwc2: when dma is disabled, clear hcd-self.uses_dma

[Matthijs Kooijman]

When dma is disabled inside dwc2 (because the hardware does not support
it, or the code was changed to disable it for testing), let the usb core
know about this by clearing hcd-self.uses_dma.

By default, the usb core assumes that dma is used when a dma_mask is
set, but this might not always match the dma_enable value in dwc2. To
prevent problems resulting from a mismatch, better to explicitely
disable dma in this case (though everything seemed to work with the
wrong value of uses_dma as well, probably only resulted in some unneeded
work).

Signed-off-by: Matthijs Kooijman matth...@stdin.nl
---
 drivers/staging/dwc2/hcd.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/staging/dwc2/hcd.c b/drivers/staging/dwc2/hcd.c
index d9a2055..18a91de 100644
--- a/drivers/staging/dwc2/hcd.c
+++ b/drivers/staging/dwc2/hcd.c
@@ -2766,6 +2766,9 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
if (!hcd)
goto error1;
 
+   if (hsotg-core_params-dma_enable = 0)
+   hcd-self.uses_dma = 0;
+
hcd-has_tt = 1;
 
spin_lock_init(hsotg-lock);
-- 
1.8.0

--
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 0/4] staging: dwc2: DMA improvements

[Matthijs Kooijman]

Hi folks,

here's some more dma fixes for the dwc2 driver. The second patch of this
series is the same as in my previous series, the others are new.

 staging: dwc2: Set a default dma_mask for platform devices

It would be great if at least this first patch in this series could be
included in 3.10, since it is needed to make the dwc2 driver work on the
ralink rt3052 target.
Before, the plan was to set up the dma mask in MIPS platform code, but
because of a similar change in ehci and the uglyness of the code for
that, the plan for that infrastructure was dropped. This patch makes the
setting of the dma_mask happen in the same way as the patch Stephen
Warren (set device dma_mask without reference to global data), so
perhaps it can be pushed to 3.10 together with that one?

 staging: dwc2: disable dma when no dma_mask was setup
This patch might not be strictly required if the previous one is
accepted, but it seems reasonable to just check this to be on the safe
side (in case a bus driver messes up, for example).

 staging: dwc2: when dma is disabled, clear hcd-self.uses_dma
This is a different approach to another patch in my previous series.

 staging: dwc2: Don't touch the dma_mask when dma is disabled
This removes some messing with the dma_mask that I think is unneeded,
but it's not very important in any way.

Thanks,

Matthijs

 drivers/staging/dwc2/hcd.c  | 15 ---
 drivers/staging/dwc2/platform.c |  8 
 2 files changed, 20 insertions(+), 3 deletions(-)

-- 
1.8.0

--
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/4] staging: dwc2: disable dma when no dma_mask was setup

[Matthijs Kooijman]

If the platform driver failed to setup a dma_mask, but the hardware
advertises support for DMA, before DMA would be enabled in dwc2, but
disabled in the usb core, making all connectivity break.

With this commit, the dwc2 driver will emit a warning and fall back to
slave mode in this case.

Signed-off-by: Matthijs Kooijman matth...@stdin.nl
---
 drivers/staging/dwc2/hcd.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/drivers/staging/dwc2/hcd.c b/drivers/staging/dwc2/hcd.c
index eff8e59..d9a2055 100644
--- a/drivers/staging/dwc2/hcd.c
+++ b/drivers/staging/dwc2/hcd.c
@@ -2740,6 +2740,15 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
/* Validate parameter values */
dwc2_set_parameters(hsotg, params);
 
+   /* Check if the bus driver or platform code has setup a dma_mask */
+   if (hsotg-core_params-dma_enable  0 
+   hsotg-dev-dma_mask == NULL) {
+   dev_warn(hsotg-dev,
+dma_mask not set, disabling DMA\n);
+   hsotg-core_params-dma_enable = 0;
+   hsotg-core_params-dma_desc_enable = 0;
+   }
+
/* Set device flags indicating whether the HCD supports DMA */
if (hsotg-core_params-dma_enable  0) {
if (dma_set_mask(hsotg-dev, DMA_BIT_MASK(31))  0)
-- 
1.8.0

--
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 1/4] staging: dwc2: Set a default dma_mask for platform devices

[Matthijs Kooijman]

Hi Paul,

  +   /*
  +* Use reasonable defaults so platforms don't have to provide these.
  +*/
  +   if (!dev-dev.dma_mask)
  +   dev-dev.dma_mask = dev-dev.coherent_dma_mask;
  +   if (!dev-dev.coherent_dma_mask)
  +   dev-dev.coherent_dma_mask = DMA_BIT_MASK(32);
 
 Hmm. Is it kosher to override these in a driver and force DMA to be
 enabled?

Apparently, since a lot of drivers do it like this. This particular code
was taken from the ehci-platform driver. See also:

http://thread.gmane.org/gmane.linux.usb.general/86066

The discussion in that thread suggests that even though this is not
quite the optimal way to do this, it is the accepted way for now and
changing it needs a bit more complicated changes and more discussion,
apparently.

 What if it has been disabled earlier on purpose, say because the
 platform does not have DMA support? You say This still allows any
 platform code to set any more specific mask if needed, but how would
 that be done exactly?
Platform code could go over the list of platform devices and (based on the
device-tree compatible value, for example) change the mask of a device.
Admittedly, this could set a more specific mask, but not exactly disable
dma entirely. I guess this is not a usecase for the other drivers?

For the dwc2 driver, I guess the dwc2 hardware would not have dma
enabled if the system does not support it anyway?

Note that this code only runs for platform devices, so not for pci
devices, which can disable dma by not setting a dma_mask when combined
with the next patch.

Gr.

Matthijs
--
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

dwc2: Transaction errors with device connected at boot

[Matthijs Kooijman]

Hi Paul,

I'm running into an issue with the dwc2 driver. When I power on my board
with an usb device (mass storage in this case) connected, the hardware
is returning transaction errors (e.g., triggering channel halted +
xacterr interrupts) for every transfer scheduled, starting with the
first ones.

This condition stays even through a reboot, though a power cycle cures
the condition (provided I remove the USB device for the boot). If I
insert the USB device after the board has booted and the driver has
loaded, everything works as expected, also after a reboot (with the
device still plugged in).

This leads me to suspect there is some issue when the time between the
first hardware initialization and the first host channel start is too
small or something like that? Any ideas?

Also, is there any fixed list of what could cause a transaction error,
or is this just a catchall meaning Something went wrong?

Thanks,

Matthijs
--
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] staging: dwc2: use devm_ioremap_resource()

[Matthijs Kooijman]

Hi Laurent,

On Thu, May 02, 2013 at 01:23:17PM +0200, Laurent Navet wrote:
 Replace a call to deprecated devm_request_and_ioremap by 
 devm_ioremap_resource.
 
 Found with coccicheck and this semantic patch:
  scripts/coccinelle/api/devm_request_and_ioremap.cocci.
 
 Signed-off-by: Laurent Navet laurent.na...@gmail.com
 ---
  drivers/staging/dwc2/pci.c |6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)
 
Patch looks good, I had a bit-for-bit identical patch in my queue here
as well :-)

Reviewed-by: Matthijs Kooijman matth...@stdin.nl

Gr.

Matthijs

 diff --git a/drivers/staging/dwc2/pci.c b/drivers/staging/dwc2/pci.c
 index 69c65eb..407e021 100644
 --- a/drivers/staging/dwc2/pci.c
 +++ b/drivers/staging/dwc2/pci.c
 @@ -134,9 +134,9 @@ static int dwc2_driver_probe(struct pci_dev *dev,
   pci_set_power_state(dev, PCI_D0);
  
   hsotg-dev = dev-dev;
 - hsotg-regs = devm_request_and_ioremap(dev-dev, dev-resource[0]);
 - if (!hsotg-regs)
 - return -ENOMEM;
 + hsotg-regs = devm_ioremap_resource(dev-dev, dev-resource[0]);
 + if (IS_ERR(hsotg-regs))
 + return PTR_ERR(hsotg-regs);
  
   dev_dbg(dev-dev, mapped PA %08lx to VA %p\n,
   (unsigned long)pci_resource_start(dev, 0), hsotg-regs);
 -- 
 1.7.10.4
 
 --
 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
--
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 1/2] staging: dwc2: when dma is disabled, clear dev-dma_mask

[Matthijs Kooijman]

Hi Paul ( Greg),

On Thu, May 02, 2013 at 01:23:42AM +, Paul Zimmerman wrote:
  From: Paul Zimmerman
  Sent: Monday, April 29, 2013 6:27 PM
  
   From: Matthijs Kooijman [mailto:matth...@stdin.nl]
   Sent: Monday, April 29, 2013 7:33 AM
  
 } else {
   - dma_set_mask(hsotg-dev, 0);
   + hsotg-dev-dma_mask = NULL;
 dma_set_coherent_mask(hsotg-dev, 0);
 }
  
  I see a problem after applying this patch. If I insert the driver
  modules with dwc2_module_params.dma_enable = 0, remove the modules,
  then insert them again with dwc2_module_params.dma_enable = 1, then
  the DMA does not get enabled. I see the message dma_mask not set,
  disabling DMA from the other patch. So setting the
  hsotg-dev-dma_mask pointer to NULL seems to be irreversible.
  
  It seems like this shouldn't happen. I think hsotg-dev should get
  deleted when the module is removed, and recreated when it is inserted
  again. But maybe I just don't understand the device model.
 
 Can you try the additional patch below on top of your original patch?
 This fixes the problem for me with the PCI driver, but I want to make
 sure it works for the platform device as well.

I tried your patch, but to I don't quite like it. It seems ugly to me to
have to save the dma_mask value and restore it on the module unload.

I'd say this points out that perhaps modifying the dma_mask variable
isn't the right way to tell the usb core that we don't want to be using
DMA. Instead, it seems the below alternative patch is more elegant by
simply setting hcd-self.uses_dma to 0 if we're not using dma (even
though a DMA mask is set up). If the USB core would be using the
uses_dma value during initialization already, this could create a
problem, but currently it is only used when submitting an urb, so this
should be ok.

Greg, is it ok for a HCD to modify hcd-self.uses_dma like this?

Gr.

Matthijs

diff --git a/drivers/staging/dwc2/hcd.c b/drivers/staging/dwc2/hcd.c
index d9a2055..18a91de 100644
--- a/drivers/staging/dwc2/hcd.c
+++ b/drivers/staging/dwc2/hcd.c
@@ -2766,6 +2766,9 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
if (!hcd)
goto error1;
 
+   if (hsotg-core_params-dma_enable = 0)
+   hcd-self.uses_dma = 0;
+
hcd-has_tt = 1;
 
spin_lock_init(hsotg-lock);

--
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 v2 0/4] dwc2: Interrupt-related cleanups

[Matthijs Kooijman]

Hi Paul,

 I already have a new version of the other patches prepared as well (with
 a bit less invasive changes), but I haven't thoroughly reviewed those
 yet (and I'm out of time until monday, so I thought to send over these
 easy patches already now).
I reconsidered this and I'll keep the rest of the patches to myself for
now. As you suggested in another thread, it might be better to keep the
code for this as-is until we look at device mode (and/or I understand
the code flow a bit better), since there are still some unsolved
questions in those patches. Better to focus on other things now.

Gr.

Matthijs
--
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 0/2] staging: dwc2: Improve dma enable consistency

[Matthijs Kooijman]

Hi folks,

with the dwc2 driver, it is currently possible to have mismatch between
dma-enable status between the usb core and the dwc2 driver because they
use different ways to check for dma enable status.

These two patches make sure that these two remain consistent.

Gr.

Matthijs


Matthijs Kooijman (2):
  staging: dwc2: when dma is disabled, clear dev-dma_mask
  staging: dwc2: disable dma when no dma_mask was setup

 drivers/staging/dwc2/hcd.c | 11 ++-
 1 file changed, 10 insertions(+), 1 deletion(-)

-- 
1.8.0

--
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 1/2] staging: dwc2: when dma is disabled, clear dev-dma_mask

[Matthijs Kooijman]

Instead of just setting the dma_mask to the empty mask, the pointer to
the mask is cleared altogether. This is needed because
usb_create_shared_hcd sets hcd-self.uses_dma based on
dev-dma_mask != NULL, not by looking at the actual mask itself.

This change thus makes hcd-self.uses_dma have the correct value when
not using dma even though a dma_mask was set up by the platform or pci
driver (which in practice should only occur when manually disabling
dma, assuming that a dma_mask is normally only setup when dma is
supported and enabled). This in turn prevents usb core from doing some
dma-specific stuff that isn't needed (though everything seemed to work
with the wrong value of uses_dma as well).

Signed-off-by: Matthijs Kooijman matth...@stdin.nl
---
 drivers/staging/dwc2/hcd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/dwc2/hcd.c b/drivers/staging/dwc2/hcd.c
index 7727112..ac34d4a 100644
--- a/drivers/staging/dwc2/hcd.c
+++ b/drivers/staging/dwc2/hcd.c
@@ -2810,7 +2810,7 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
dev_warn(hsotg-dev,
 can't enable workaround for 2GB RAM\n);
} else {
-   dma_set_mask(hsotg-dev, 0);
+   hsotg-dev-dma_mask = NULL;
dma_set_coherent_mask(hsotg-dev, 0);
}
 
-- 
1.8.0

--
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] staging: dwc2: use dwc2_hcd_get_frame_number where possible

[Matthijs Kooijman]

Before, there were two places that manually read the FRNUM registers,
while there is a function to do this.

Signed-off-by: Matthijs Kooijman matth...@stdin.nl
---
 drivers/staging/dwc2/core.c | 8 +---
 drivers/staging/dwc2/hcd_intr.c | 5 +
 2 files changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/dwc2/core.c b/drivers/staging/dwc2/core.c
index 281ca95..1a2e53a 100644
--- a/drivers/staging/dwc2/core.c
+++ b/drivers/staging/dwc2/core.c
@@ -1146,16 +1146,10 @@ void dwc2_hc_cleanup(struct dwc2_hsotg *hsotg, struct 
dwc2_host_chan *chan)
 static void dwc2_hc_set_even_odd_frame(struct dwc2_hsotg *hsotg,
   struct dwc2_host_chan *chan, u32 *hcchar)
 {
-   u32 hfnum, frnum;
-
if (chan-ep_type == USB_ENDPOINT_XFER_INT ||
chan-ep_type == USB_ENDPOINT_XFER_ISOC) {
-   hfnum = readl(hsotg-regs + HFNUM);
-   frnum = hfnum  HFNUM_FRNUM_SHIFT 
-   HFNUM_FRNUM_MASK  HFNUM_FRNUM_SHIFT;
-
/* 1 if _next_ frame is odd, 0 if it's even */
-   if (frnum  0x1)
+   if (dwc2_hcd_get_frame_number(hsotg)  0x1)
*hcchar |= HCCHAR_ODDFRM;
}
 }
diff --git a/drivers/staging/dwc2/hcd_intr.c b/drivers/staging/dwc2/hcd_intr.c
index 9fb92693..0eebba6 100644
--- a/drivers/staging/dwc2/hcd_intr.c
+++ b/drivers/staging/dwc2/hcd_intr.c
@@ -117,16 +117,13 @@ static void dwc2_sof_intr(struct dwc2_hsotg *hsotg)
 {
struct list_head *qh_entry;
struct dwc2_qh *qh;
-   u32 hfnum;
enum dwc2_transaction_type tr_type;
 
 #ifdef DEBUG_SOF
dev_vdbg(hsotg-dev, --Start of Frame Interrupt--\n);
 #endif
 
-   hfnum = readl(hsotg-regs + HFNUM);
-   hsotg-frame_number = hfnum  HFNUM_FRNUM_SHIFT 
-   HFNUM_FRNUM_MASK  HFNUM_FRNUM_SHIFT;
+   hsotg-frame_number = dwc2_hcd_get_frame_number(hsotg);
 
dwc2_track_missed_sofs(hsotg);
 
-- 
1.8.0

--
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 3/7] staging: dwc2: add helper variable to simplify code

[Matthijs Kooijman]

Now a register is masked only in once place, instead of twice. This
makes the two uses of this value shorter so they no longer need to be
linewrapped.

Signed-off-by: Matthijs Kooijman matth...@stdin.nl
---
 drivers/staging/dwc2/hcd_intr.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/dwc2/hcd_intr.c b/drivers/staging/dwc2/hcd_intr.c
index 0eebba6..e84f244 100644
--- a/drivers/staging/dwc2/hcd_intr.c
+++ b/drivers/staging/dwc2/hcd_intr.c
@@ -243,6 +243,7 @@ static void dwc2_hprt0_enable(struct dwc2_hsotg *hsotg, u32 
hprt0,
u32 usbcfg;
u32 prtspd;
u32 hcfg;
+   u32 fslspclksel;
u32 hfir;
 
dev_vdbg(hsotg-dev, %s(%p)\n, __func__, hsotg);
@@ -274,6 +275,7 @@ static void dwc2_hprt0_enable(struct dwc2_hsotg *hsotg, u32 
hprt0,
}
 
hcfg = readl(hsotg-regs + HCFG);
+   fslspclksel = hcfg  HCFG_FSLSPCLKSEL_MASK;
 
if (prtspd == HPRT0_SPD_LOW_SPEED 
params-host_ls_low_power_phy_clk ==
@@ -281,8 +283,7 @@ static void dwc2_hprt0_enable(struct dwc2_hsotg *hsotg, u32 
hprt0,
/* 6 MHZ */
dev_vdbg(hsotg-dev,
 FS_PHY programming HCFG to 6 MHz\n);
-   if ((hcfg  HCFG_FSLSPCLKSEL_MASK) !=
-   HCFG_FSLSPCLKSEL_6_MHZ) {
+   if (fslspclksel != HCFG_FSLSPCLKSEL_6_MHZ) {
hcfg = ~HCFG_FSLSPCLKSEL_MASK;
hcfg |= HCFG_FSLSPCLKSEL_6_MHZ;
writel(hcfg, hsotg-regs + HCFG);
@@ -292,8 +293,7 @@ static void dwc2_hprt0_enable(struct dwc2_hsotg *hsotg, u32 
hprt0,
/* 48 MHZ */
dev_vdbg(hsotg-dev,
 FS_PHY programming HCFG to 48 MHz\n);
-   if ((hcfg  HCFG_FSLSPCLKSEL_MASK) !=
-   HCFG_FSLSPCLKSEL_48_MHZ) {
+   if (fslspclksel != HCFG_FSLSPCLKSEL_48_MHZ) {
hcfg = ~HCFG_FSLSPCLKSEL_MASK;
hcfg |= HCFG_FSLSPCLKSEL_48_MHZ;
writel(hcfg, hsotg-regs + HCFG);
-- 
1.8.0

--
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 0/7] dwc2: Miscelaneous cleanups

[Matthijs Kooijman]

Hi folks,

these are just some small cleanups for the dwc2 driver that I still had
lying around.

I already sent these over a few weeks ago as part of a very big series,
which I am now splitting up into parts with a similar theme. These are
the patches that don't really fit in with any other series.

Gr.

Matthijs

Matthijs Kooijman (6):
  staging: dwc2: replace some magic numbers by constants
  staging: dwc2: use dwc2_hcd_get_frame_number where possible
  staging: dwc2: add helper variable to simplify code
  staging: dwc2: remove some device-mode related debug code
  staging: dwc2: remove unneeded check
  staging: dwc2: remove some useless debug prints

Stephen Warren (1):
  staging: dwc2: add const to handling of dwc2_core_params

 drivers/staging/dwc2/core.c | 44 +
 drivers/staging/dwc2/hcd.c  |  2 +-
 drivers/staging/dwc2/hcd.h  |  4 ++--
 drivers/staging/dwc2/hcd_intr.c | 13 +---
 drivers/staging/dwc2/pci.c  |  7 +--
 5 files changed, 18 insertions(+), 52 deletions(-)

-- 
1.8.0

--
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 7/7] staging: dwc2: remove some useless debug prints

[Matthijs Kooijman]

This removes some debug prints from pci.c and makes platform.c and pci.c
a bit more similar again.

Signed-off-by: Matthijs Kooijman matth...@stdin.nl
---
 drivers/staging/dwc2/pci.c | 5 -
 1 file changed, 5 deletions(-)

diff --git a/drivers/staging/dwc2/pci.c b/drivers/staging/dwc2/pci.c
index 12ae24b..bbd3a1c 100644
--- a/drivers/staging/dwc2/pci.c
+++ b/drivers/staging/dwc2/pci.c
@@ -101,8 +101,6 @@ static void dwc2_driver_remove(struct pci_dev *dev)
 {
struct dwc2_hsotg *hsotg = pci_get_drvdata(dev);
 
-   dev_dbg(dev-dev, %s(%p)\n, __func__, dev);
-
dwc2_hcd_remove(hsotg);
pci_disable_device(dev);
 }
@@ -125,8 +123,6 @@ static int dwc2_driver_probe(struct pci_dev *dev,
struct dwc2_hsotg *hsotg;
int retval;
 
-   dev_dbg(dev-dev, %s(%p)\n, __func__, dev);
-
hsotg = devm_kzalloc(dev-dev, sizeof(*hsotg), GFP_KERNEL);
if (!hsotg)
return -ENOMEM;
@@ -153,7 +149,6 @@ static int dwc2_driver_probe(struct pci_dev *dev,
}
 
pci_set_drvdata(dev, hsotg);
-   dev_dbg(dev-dev, hsotg=%p\n, hsotg);
 
return retval;
 }
-- 
1.8.0

--
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 4/7] staging: dwc2: remove some device-mode related debug code

[Matthijs Kooijman]

This code appears to be partially incorrect. Since this is only debug
code and only applies to device mode, it seems better to remove this
code for now than to invest time fixing it.

Signed-off-by: Matthijs Kooijman matth...@stdin.nl
---
 drivers/staging/dwc2/core.c | 19 ---
 1 file changed, 19 deletions(-)

diff --git a/drivers/staging/dwc2/core.c b/drivers/staging/dwc2/core.c
index 1a2e53a..6578ed5 100644
--- a/drivers/staging/dwc2/core.c
+++ b/drivers/staging/dwc2/core.c
@@ -1809,8 +1809,6 @@ void dwc2_dump_global_registers(struct dwc2_hsotg *hsotg)
 {
 #ifdef DEBUG
u32 __iomem *addr;
-   int i, ep_num;
-   char *txfsiz;
 
dev_dbg(hsotg-dev, Core Global Registers\n);
addr = hsotg-regs + GOTGCTL;
@@ -1886,23 +1884,6 @@ void dwc2_dump_global_registers(struct dwc2_hsotg *hsotg)
dev_dbg(hsotg-dev, HPTXFSIZ@0x%08lX : 0x%08X\n,
(unsigned long)addr, readl(addr));
 
-   if (hsotg-core_params-en_multiple_tx_fifo = 0) {
-   ep_num = hsotg-hwcfg4  GHWCFG4_NUM_DEV_PERIO_IN_EP_SHIFT 
-GHWCFG4_NUM_DEV_PERIO_IN_EP_MASK 
-GHWCFG4_NUM_DEV_PERIO_IN_EP_SHIFT;
-   txfsiz = DPTXFSIZ;
-   } else {
-   ep_num = hsotg-hwcfg4  GHWCFG4_NUM_IN_EPS_SHIFT 
-GHWCFG4_NUM_IN_EPS_MASK  GHWCFG4_NUM_IN_EPS_SHIFT;
-   txfsiz = DIENPTXF;
-   }
-
-   for (i = 0; i  ep_num; i++) {
-   addr = hsotg-regs + DPTXFSIZN(i + 1);
-   dev_dbg(hsotg-dev, %s[%d] @0x%08lX : 0x%08X\n, txfsiz, i + 1,
-   (unsigned long)addr, readl(addr));
-   }
-
addr = hsotg-regs + PCGCTL;
dev_dbg(hsotg-dev, PCGCTL  @0x%08lX : 0x%08X\n,
(unsigned long)addr, readl(addr));
-- 
1.8.0

--
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 5/7] staging: dwc2: remove unneeded check

[Matthijs Kooijman]

The value in params-enable_dynamic_fifo can only be true if the
corresponding bit in hwcfg2 is set, this is already checked by
dwc2_set_param_enable_dynamic_fifo.

Signed-off-by: Matthijs Kooijman matth...@stdin.nl
---
 drivers/staging/dwc2/core.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/dwc2/core.c b/drivers/staging/dwc2/core.c
index 6578ed5..89b19d7 100644
--- a/drivers/staging/dwc2/core.c
+++ b/drivers/staging/dwc2/core.c
@@ -506,8 +506,7 @@ static void dwc2_config_fifos(struct dwc2_hsotg *hsotg)
struct dwc2_core_params *params = hsotg-core_params;
u32 rxfsiz, nptxfsiz, ptxfsiz, hptxfsiz, dfifocfg;
 
-   if (!(hsotg-hwcfg2  GHWCFG2_DYNAMIC_FIFO) ||
-   !params-enable_dynamic_fifo)
+   if (!params-enable_dynamic_fifo)
return;
 
dev_dbg(hsotg-dev, Total FIFO Size=%d\n, hsotg-total_fifo_size);
-- 
1.8.0

--
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 6/7] staging: dwc2: add const to handling of dwc2_core_params

[Matthijs Kooijman]

From: Stephen Warren swar...@wwwdotorg.org

Now the functions use proper const annotations, the global variable with
default params can be marked const, which prevents these values from
being changed for a specific device (in theory there could be multiple
controllers with different settings, for example).

Signed-off-by: Stephen Warren swar...@wwwdotorg.org
[matth...@stdin.nl: Split patch from bigger patch, marked
dwc2_module_params in pci.c as const and added
commit message]
Signed-off-by: Matthijs Kooijman matth...@stdin.nl
---
 drivers/staging/dwc2/core.c | 2 +-
 drivers/staging/dwc2/hcd.c  | 2 +-
 drivers/staging/dwc2/hcd.h  | 4 ++--
 drivers/staging/dwc2/pci.c  | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/dwc2/core.c b/drivers/staging/dwc2/core.c
index 89b19d7..ac8ed15 100644
--- a/drivers/staging/dwc2/core.c
+++ b/drivers/staging/dwc2/core.c
@@ -2642,7 +2642,7 @@ int dwc2_set_param_otg_ver(struct dwc2_hsotg *hsotg, int 
val)
  * for the DWC_otg core. It returns non-0 if any parameters are invalid.
  */
 int dwc2_set_parameters(struct dwc2_hsotg *hsotg,
-   struct dwc2_core_params *params)
+   const struct dwc2_core_params *params)
 {
int retval = 0;
 
diff --git a/drivers/staging/dwc2/hcd.c b/drivers/staging/dwc2/hcd.c
index c35583f..650f60e 100644
--- a/drivers/staging/dwc2/hcd.c
+++ b/drivers/staging/dwc2/hcd.c
@@ -2701,7 +2701,7 @@ EXPORT_SYMBOL_GPL(dwc2_set_all_params);
  * a negative error on failure.
  */
 int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
- struct dwc2_core_params *params)
+ const struct dwc2_core_params *params)
 {
struct usb_hcd *hcd;
struct dwc2_host_chan *channel;
diff --git a/drivers/staging/dwc2/hcd.h b/drivers/staging/dwc2/hcd.h
index 88b707e..cf6c055 100644
--- a/drivers/staging/dwc2/hcd.h
+++ b/drivers/staging/dwc2/hcd.h
@@ -448,10 +448,10 @@ static inline u8 dwc2_hcd_is_pipe_out(struct 
dwc2_hcd_pipe_info *pipe)
 }
 
 extern int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
-struct dwc2_core_params *params);
+const struct dwc2_core_params *params);
 extern void dwc2_hcd_remove(struct dwc2_hsotg *hsotg);
 extern int dwc2_set_parameters(struct dwc2_hsotg *hsotg,
-  struct dwc2_core_params *params);
+  const struct dwc2_core_params *params);
 extern void dwc2_set_all_params(struct dwc2_core_params *params, int value);
 
 /* Transaction Execution Functions */
diff --git a/drivers/staging/dwc2/pci.c b/drivers/staging/dwc2/pci.c
index 69c65eb..12ae24b 100644
--- a/drivers/staging/dwc2/pci.c
+++ b/drivers/staging/dwc2/pci.c
@@ -59,7 +59,7 @@
 
 static const char dwc2_driver_name[] = dwc2;
 
-static struct dwc2_core_params dwc2_module_params = {
+static const struct dwc2_core_params dwc2_module_params = {
.otg_cap= -1,
.otg_ver= -1,
.dma_enable = -1,
-- 
1.8.0

--
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 1/7] staging: dwc2: replace some magic numbers by constants

[Matthijs Kooijman]

There were already macros for these, they just weren't being used in a
few places.

Signed-off-by: Matthijs Kooijman matth...@stdin.nl
---
 drivers/staging/dwc2/core.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/dwc2/core.c b/drivers/staging/dwc2/core.c
index 3177db2..281ca95 100644
--- a/drivers/staging/dwc2/core.c
+++ b/drivers/staging/dwc2/core.c
@@ -1696,7 +1696,7 @@ u32 dwc2_calc_frame_interval(struct dwc2_hsotg *hsotg)
GHWCFG2_FS_PHY_TYPE_DEDICATED)
clock = 48;
 
-   if ((hprt0  HPRT0_SPD_MASK) == 0)
+   if ((hprt0  HPRT0_SPD_MASK) == HPRT0_SPD_HIGH_SPEED)
/* High speed case */
return 125 * clock;
else
@@ -2298,7 +2298,7 @@ int dwc2_set_param_phy_type(struct dwc2_hsotg *hsotg, int 
val)
 #ifndef NO_FS_PHY_HW_CHECKS
valid = 0;
 #else
-   val = 0;
+   val = DWC2_PHY_TYPE_PARAM_FS;
dev_dbg(hsotg-dev, Setting phy_type to %d\n, val);
retval = -EINVAL;
 #endif
@@ -2325,7 +2325,7 @@ int dwc2_set_param_phy_type(struct dwc2_hsotg *hsotg, int 
val)
dev_err(hsotg-dev,
%d invalid for phy_type. Check HW 
configuration.\n,
val);
-   val = 0;
+   val = DWC2_PHY_TYPE_PARAM_FS;
if (hs_phy_type != GHWCFG2_HS_PHY_TYPE_NOT_SUPPORTED) {
if (hs_phy_type == GHWCFG2_HS_PHY_TYPE_UTMI ||
hs_phy_type == GHWCFG2_HS_PHY_TYPE_UTMI_ULPI)
@@ -2360,8 +2360,8 @@ int dwc2_set_param_speed(struct dwc2_hsotg *hsotg, int 
val)
valid = 0;
}
 
-   if (val == 0  dwc2_get_param_phy_type(hsotg) ==
-   DWC2_PHY_TYPE_PARAM_FS)
+   if (val == DWC2_SPEED_PARAM_HIGH 
+   dwc2_get_param_phy_type(hsotg) == DWC2_PHY_TYPE_PARAM_FS)
valid = 0;
 
if (!valid) {
@@ -2370,7 +2370,7 @@ int dwc2_set_param_speed(struct dwc2_hsotg *hsotg, int 
val)
%d invalid for speed parameter. Check HW 
configuration.\n,
val);
val = dwc2_get_param_phy_type(hsotg) == DWC2_PHY_TYPE_PARAM_FS ?
-   1 : 0;
+   DWC2_SPEED_PARAM_FULL : DWC2_SPEED_PARAM_HIGH;
dev_dbg(hsotg-dev, Setting speed to %d\n, val);
retval = -EINVAL;
}
-- 
1.8.0

--
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 02/10] staging: dwc2: do not handle PRTINT in dwc2_handle_common_intr

[Matthijs Kooijman]

Hey Paul,

  In the current code / my patches I also have SOF, NPTxFEmp and RxFLvl
  marked (and handled) as a host interrupt, but I suspect that these three
  are used for device mode as well? However, since we don't have
  device handlers for them already, shouldn't they be cleared by
  dwc2_hcd_intr() as well if it detects device mode?
 
 Yes, these are relevant in both modes.
 
 I wonder if it would make sense to have a stub device-mode interrupt
 handler for now, which would be responsible for clearing these. Once
 device mode is implemented, it would be replaced with a real device-
 mode interrupt handler.
If we make sure that these interrupts are only enabled when the device
is in host-mode, then this shouldn't be needed, right? (As long as we
don't have code for device-mode, no interrupts will be enabled for
device mode either.

  Perhaps it makes sense to actually disable host (and later device)
  interrupts from GINTSTS_CONIDSTSCHNG? The interrupts for host mode are
  actually enabled in response to this interrupt already
  (dwc2_handle_conn_id_status_change_intr - dwc2_conn_id_status_change -
  dwc2_hcd_start - dwc2_hcd_start_func - dwc2_host_start -
  _dwc2_hcd_start - dwc2_hcd_reinit - dwc2_core_host_init -
  dwc2_enable_host_interrupts).
 
 It turns out that dwc2_handle_conn_id_status_change_intr() already
 disables all interrupts. It calls dwc2_core_init(), which calls
 dwc2_core_reset(), which does a soft reset of the core. According
 to the databook, that resets most of the core's state, and clears
 most of the registers, including GINTMSK.

This is true, but the step from dwc2_handle_conn_id_status_change_intr
to dwc2_conn_id_status_change (which then calls dwc2_core_init) happens
through a work queue, not as a direct call. I think this is the reason
there is the reason DISCONN needs to be handled by the common handler
right now, or outside of the is in host mode check in the hcd handler
as we discussed before. If the DISCONN interrupt was handled in the hcd
handler, and only if the device is in host mode, the following can
happen:
 - The CONIDSTSCHNG interrupt fires, the hardware starts changing to
   device mode.
 - The DISCONN interrupt fires, potentially at the same time or later as
   the CONIDSTSCHNG interrupt.
 - By the time the host irq handler is run, the hardware is in device
   mode, so the hcd irq handler doesn't do anything (in particular, the
   DISCONN interrupt is not cleared).
 - In the time between the interrupt and the scheduling of the queued
   work, the DISCONN interrupt triggers again, and keeps triggering
   until the queued work is finally scheduled and resets the core.

I would propose to clear the host (and later device) interrupts directly
in the dwc2_handle_conn_id_status_change_intr function, so the following
happens:
 - The CONIDSTSCHNG interrupt fires, the hardware starts changing to
   device mode.
 - dwc2_handle_conn_id_status_change_intr disables all host interrupts,
   so they don't get handled anymore and don't get triggered.

This should remove the race condition, while allowing the DISCONN
interrupt to be handled by the host interrupt handler where it would
make the most sense.

Furthermore, I think this will also close another race condition: If the
hardware switches to device mode after the host interupt handler checks
for host mode, but before the end of the handler, it could potentially
access non-existent registers. With this change, the host mode interrupt
handler (effectively) becomes a noop (since it only handles enabled
interrupts) as soon as the hardware starts changing to device mode,
closeing this gap.

Possibly some similar analysis needs to be applied to other places where
the hardware or driver can initiate device - host mode switches, but
perhaps this is only here as long as device mode and the otg protocols
are not implemented?

Gr.

Matthijs
--
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 03/10] staging: dwc2: move some interrupt enabling around

[Matthijs Kooijman]

Hi Paul,

  Before, the DISCONNINT interrupt was enabled in
  dwc2_enable_host_interrupts, but handled in dwc2_handle_common_intr,
  while the RXFLVL interrupt was enabled in dwc2_enable_commont_interrupts
  and handled in dwc_handle_hcd_intr.

 And, the RXFLVL interrupt is a host+device mode interrupt, so it
 _should_ be enabled in dwc2_enable_common_interrupts. The only
 handler for it is in dwc2_hcd_intr() because there is no device-
 mode driver code yet. Once there is, there will either be two
 handlers, or else a common handler with different code depending
 on the mode.

If RXFLVL is a host+device mode interrupt, doesn't it make sense to
enable it in both enable_host_interrupts and (the future)
enable_device_interrupts instead of in enable_common_interrupts?

In practice, these two are always called (close) together, so it shouldn't
really change any behaviour, but it does make the distinction between
host, device and common mode interrupts more clear (and makes sure that
enable_host_interrupts mirrors disable_host_interrupts)?

This question only occurs for this particular interrupt, since the other
host+device interrupts (SOF, NPTxFEmp) are not enabled right away, but
later when needed.

Gr.

Matthijs
--
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 03/10] staging: dwc2: move some interrupt enabling around

[Matthijs Kooijman]

Hi Paul,

 If RXFLVL is a host+device mode interrupt, doesn't it make sense to
 enable it in both enable_host_interrupts and (the future)
 enable_device_interrupts instead of in enable_common_interrupts?
Hmm, I just realized the underlying thing here is that I'm
distinguishing common interrupts from host+device interrupts, while
I think you might be lumping them together?

In my interpretation, common interrupts are those that are really
separate from either host or device mode (connection status, otg
protocols, suspend/wakeup, etc.), while device+mode interrupts are
interrupts that both the gadget and hcd driver parts use during their
operation.

I'm not entirely sure if this distinction is meaningful, though. In any
case, I do think that having a correspondence between
enable_host_interrupts and disable_host_interrupts is useful, so we
should either:
 - Make the distinction, add the host+device interrupts to GINTMSK_HOST,
   remoenable them in enable_host_interrupts (except the ones that are
   enabled on-demand, so effectively the only host+device interrupt to
   enable here is RXFLVL) and disable them in disable_host_interrupts.
 - Do not make the distinction, add the host+device interrupts to
   GINTMSK_COMMON and not GINTMSK_HOST, enable them in
   enable_common_interrupts (again, except the ones that are enabled
   on-demand) disable them in disable_common_interrupts. I suspect
   they'll also need to be disabled in disable_host_interrupts (since
   this happens now already), so they'll be explicitely listed in
   addition to GINTMSK_HOST there.

For now, I'll assume option 1 is the most sensible and I'll adapt my
patches for that.

Gr.

Matthijs
--
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 02/10] staging: dwc2: do not handle PRTINT in dwc2_handle_common_intr

[Matthijs Kooijman]

Hey Paul,

 What say we just defer this change for now? Once we go to add the
 device-mode handling, we will probably find out that further changes
 will be required anyway.
Possibly, yeah.

In any case, I'm about to send a new patch series which has the easy
fixes up front and then tries to make things a bit better separated
with less invasive changes. Just see if you think any of those are ok,
if not we'll drop them for now and I'll continue with the next batch of
cleanup patches I have :-)

Gr.

Matthijs
--
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 v2 0/4] dwc2: Interrupt-related cleanups

[Matthijs Kooijman]

Hi folks,

these are a few of the patches I sent earlier that are either already
acked by Paul, or should be simple enough.

I already have a new version of the other patches prepared as well (with
a bit less invasive changes), but I haven't thoroughly reviewed those
yet (and I'm out of time until monday, so I thought to send over these
easy patches already now).

Gr.

Matthijs


Matthijs Kooijman (4):
  staging: dwc2: do not use IRQF_DISABLED
  staging: dwc2: use irq_return_t for interrupt handlers
  staging: dwc2: rename dwc2_hcd_intr() to dwc2_handle_hcd_intr()
  staging: dwc2: remove dummy interrupt handling

 drivers/staging/dwc2/core_intr.c | 15 ---
 drivers/staging/dwc2/hcd.c   |  5 ++---
 drivers/staging/dwc2/hcd.h   |  8 
 drivers/staging/dwc2/hcd_intr.c  | 13 +
 4 files changed, 15 insertions(+), 26 deletions(-)

-- 
1.8.0

--
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 1/4] staging: dwc2: do not use IRQF_DISABLED

[Matthijs Kooijman]

This flag is a deprecated NOOP, interrupt handlers are always run
with interupts disabled.

See commit 6932bf37 (genirq: Remove IRQF_DISABLED from core code), and
include/linux/interrupt.h:

 * IRQF_DISABLED - keep irqs disabled when calling the action handler.
 * DEPRECATED. This flag is a NOOP and scheduled to be 
removed

Signed-off-by: Matthijs Kooijman matth...@stdin.nl
Acked-by: Paul Zimmerman pa...@synopsys.com
---
 drivers/staging/dwc2/hcd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/dwc2/hcd.c b/drivers/staging/dwc2/hcd.c
index 827ab78..efad02f 100644
--- a/drivers/staging/dwc2/hcd.c
+++ b/drivers/staging/dwc2/hcd.c
@@ -2920,7 +2920,7 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
 * allocates the DMA buffer pool, registers the USB bus, requests the
 * IRQ line, and calls hcd_start method.
 */
-   retval = usb_add_hcd(hcd, irq, IRQF_SHARED | IRQF_DISABLED);
+   retval = usb_add_hcd(hcd, irq, IRQF_SHARED);
if (retval  0)
goto error3;
 
-- 
1.8.0

--
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 4/4] staging: dwc2: remove dummy interrupt handling

[Matthijs Kooijman]

The handling for the IC2INT and RESTOREDONE interrupts just cleared the
interrupt flag, but did not do anything else. Since these interrupts are
not enabled anywhere, they should never trigger and there should never
be a need to clear their flags, so we can safely remove this code.

Signed-off-by: Matthijs Kooijman matth...@stdin.nl

---
v2: New patch, replacing disable I2CINT in dwc2_disable_host_interrupts
---
 drivers/staging/dwc2/core_intr.c | 9 +
 drivers/staging/dwc2/hcd_intr.c  | 3 ---
 2 files changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/staging/dwc2/core_intr.c b/drivers/staging/dwc2/core_intr.c
index e393a95..98c51bb 100644
--- a/drivers/staging/dwc2/core_intr.c
+++ b/drivers/staging/dwc2/core_intr.c
@@ -403,8 +403,7 @@ static void dwc2_handle_usb_suspend_intr(struct dwc2_hsotg 
*hsotg)
 #define GINTMSK_COMMON (GINTSTS_WKUPINT | GINTSTS_SESSREQINT | \
 GINTSTS_CONIDSTSCHNG | GINTSTS_OTGINT |\
 GINTSTS_MODEMIS | GINTSTS_DISCONNINT | \
-GINTSTS_USBSUSP | GINTSTS_RESTOREDONE |\
-GINTSTS_PRTINT)
+GINTSTS_USBSUSP | GINTSTS_PRTINT)
 
 /*
  * This function returns the Core Interrupt register
@@ -478,12 +477,6 @@ irqreturn_t dwc2_handle_common_intr(int irq, void *dev)
if (gintsts  GINTSTS_USBSUSP)
dwc2_handle_usb_suspend_intr(hsotg);
 
-   if (gintsts  GINTSTS_RESTOREDONE) {
-   gintsts = GINTSTS_RESTOREDONE;
-   writel(gintsts, hsotg-regs + GINTSTS);
-   dev_dbg(hsotg-dev,  --Restore done interrupt received--\n);
-   }
-
if (gintsts  GINTSTS_PRTINT) {
/*
 * The port interrupt occurs while in device mode with HPRT0
diff --git a/drivers/staging/dwc2/hcd_intr.c b/drivers/staging/dwc2/hcd_intr.c
index 7e28c18..9fb92693 100644
--- a/drivers/staging/dwc2/hcd_intr.c
+++ b/drivers/staging/dwc2/hcd_intr.c
@@ -2104,9 +2104,6 @@ irqreturn_t dwc2_handle_hcd_intr(struct dwc2_hsotg *hsotg)
dwc2_rx_fifo_level_intr(hsotg);
if (gintsts  GINTSTS_NPTXFEMP)
dwc2_np_tx_fifo_empty_intr(hsotg);
-   if (gintsts  GINTSTS_I2CINT)
-   /* Todo: Implement i2cintr handler */
-   writel(GINTSTS_I2CINT, hsotg-regs + GINTSTS);
if (gintsts  GINTSTS_PRTINT)
dwc2_port_intr(hsotg);
if (gintsts  GINTSTS_HCHINT)
-- 
1.8.0

--
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