Hi,
On Mon, Mar 03, 2014 at 05:09:27PM -0600, dinguyen@xxxxxxxxxx wrote:
> From: Dinh Nguyen <dinguyen@xxxxxxxxxx>
>
> Remove reading the fifo sizes from dts in platform.c
>
> Add dwc2_calculate_dynamic_fifo
>
> Conflicts:
>
> arch/arm/boot/dts/socfpga.dtsi
> drivers/staging/dwc2/core.c
if you rebased the patch and got conflicts, when sending the patch
upstream, you should remove this section.
Here's a few things for you to fix before getting this patch merged:
ERROR: trailing whitespace
#91: FILE: drivers/usb/dwc2/core.c:511:
+^I$
WARNING: printk() should include KERN_ facility level
#104: FILE: drivers/usb/dwc2/core.c:534:
+ printk("initial grxfsiz=%08x\n",
CHECK: Alignment should match open parenthesis
#105: FILE: drivers/usb/dwc2/core.c:535:
+ printk("initial grxfsiz=%08x\n",
+ readl(hsotg->regs + GRXFSIZ));
WARNING: printk() should include KERN_ facility level
#107: FILE: drivers/usb/dwc2/core.c:537:
+ printk("new grxfsiz=%08x\n", readl(hsotg->regs + GRXFSIZ));
WARNING: printk() should include KERN_ facility level
#111: FILE: drivers/usb/dwc2/core.c:540:
+ printk("initial gnptxfsiz=%08x\n",
CHECK: Alignment should match open parenthesis
#112: FILE: drivers/usb/dwc2/core.c:541:
+ printk("initial gnptxfsiz=%08x\n",
readl(hsotg->regs + GNPTXFSIZ));
WARNING: printk() should include KERN_ facility level
#119: FILE: drivers/usb/dwc2/core.c:547:
+ printk("new gnptxfsiz=%08x\n",
CHECK: Alignment should match open parenthesis
#120: FILE: drivers/usb/dwc2/core.c:548:
+ printk("new gnptxfsiz=%08x\n",
readl(hsotg->regs + GNPTXFSIZ));
WARNING: printk() should include KERN_ facility level
#124: FILE: drivers/usb/dwc2/core.c:551:
+ printk("initial hptxfsiz=%08x\n",
CHECK: Alignment should match open parenthesis
#125: FILE: drivers/usb/dwc2/core.c:552:
+ printk("initial hptxfsiz=%08x\n",
readl(hsotg->regs + HPTXFSIZ));
WARNING: printk() should include KERN_ facility level
#133: FILE: drivers/usb/dwc2/core.c:559:
+ printk("new hptxfsiz=%08x\n",
CHECK: Alignment should match open parenthesis
#134: FILE: drivers/usb/dwc2/core.c:560:
+ printk("new hptxfsiz=%08x\n",
readl(hsotg->regs + HPTXFSIZ));
ERROR: Missing Signed-off-by: line(s)
total: 2 errors, 6 warnings, 5 checks, 123 lines checked
NOTE: whitespace errors detected, you may wish to use scripts/cleanpatch or
scripts/cleanfile
/home/balbi/apply.diff/cur/1393900412.3689_5.saruman:2,S has style problems, please review.
If any of these errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
> ---
> arch/arm/boot/dts/socfpga.dtsi | 8 -------
this should be moved to another patch. First you add
dwc3_calculate_dynamic_fifo() - which in turn deprecates any of the
host-*-fifo-size DT properties - and in a following patch you update DT
data.
> drivers/usb/dwc2/core.c | 49 ++++++++++++++++++++++++++++++++++++----
> drivers/usb/dwc2/platform.c | 20 ----------------
> 3 files changed, 45 insertions(+), 32 deletions(-)
>
> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> index 03e7a6d..e4c0f72 100644
> --- a/arch/arm/boot/dts/socfpga.dtsi
> +++ b/arch/arm/boot/dts/socfpga.dtsi
> @@ -860,10 +860,6 @@
> clock-names = "otg";
> phys = <&usbphy0>;
> phy-names = "usb2-phy";
> - enable-dynamic-fifo = <1>;
> - host-rx-fifo-size = <0xa00>;
> - host-perio-tx-fifo-size = <0xa00>;
> - host-nperio-tx-fifo-size = <0xa00>;
> dma-desc-enable = <0>;
> status = "disabled";
> };
> @@ -876,10 +872,6 @@
> clock-names = "otg";
> phys = <&usbphy0>;
> phy-names = "usb2-phy";
> - enable-dynamic-fifo = <1>;
> - host-rx-fifo-size = <0xa00>;
> - host-perio-tx-fifo-size = <0xa00>;
> - host-nperio-tx-fifo-size = <0xa00>;
> dma-desc-enable = <0>;
> status = "disabled";
> };
> diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
> index 6d001b5..3acb7ee 100644
> --- a/drivers/usb/dwc2/core.c
> +++ b/drivers/usb/dwc2/core.c
> @@ -478,6 +478,38 @@ void dwc2_disable_host_interrupts(struct dwc2_hsotg *hsotg)
> writel(intmsk, hsotg->regs + GINTMSK);
> }
>
> +static void dwc2_calculate_dynamic_fifo(struct dwc2_hsotg *hsotg)
> +{
> + struct dwc2_core_params *params = hsotg->core_params;
> + struct dwc2_hw_params *hw = &hsotg->hw_params;
> + u32 rxfsiz, nptxfsiz, ptxfsiz, total_fifo_size;
> +
> + total_fifo_size = hw->total_fifo_size;
> + rxfsiz = params->host_rx_fifo_size;
> + nptxfsiz = params->host_nperio_tx_fifo_size;
> + ptxfsiz = params->host_perio_tx_fifo_size;
> +
> + if (total_fifo_size >= (rxfsiz + nptxfsiz + ptxfsiz))
> + /* Params are valid, nothing to do */
> + return;
You really don't need this branch but in any case, if one branch has
curly braces, both branches should have it too.
> + else {
> + /* min rx fifo size = ((largest packet/4)*2)+2 */
> + rxfsiz = (((1024/4) + 1 + 1) * 3) + 1;
> + /* min non-periodic tx fifo depth */
> + nptxfsiz = 3 * (1024/4);
> + /* min periodic tx fifo depth */
> + ptxfsiz = ((1024*3)/4) * 3;
> + }
> +
> + if (total_fifo_size < (rxfsiz + nptxfsiz + ptxfsiz))
hmm, so in the if condition above you do:
if (total_fifo_size >= (rxfsiz + nptxfsiz + ptxfsiz))
then you have an else branch following it and the only way for that else
branch to be reached is if this other condition is true. I think this
code could be rewritten as:
if (total_fifo_size < (rxfsiz + nptxfsiz + ptxfsiz)) {
/* min rx fifo size = ((largest packet / 4) * 2) + 2 */
rxfsiz = (((1024 / 4) + 1 + 1) * 3) + 1;
/* min non-periodic tx fifo depth */
nptxfsiz = 3 * (1024 / 4);
/* min periodic tx fifo depth */
ptxfsiz = ((1024 * 3) / 4) * 3;
}
Also, you could be a bit more verbose on your comments above. One thing
I see is that your fifo allocation, apparently, won't work for
high-bandwidth ISOC endpoints. Is that correct ? Another thing is that
pre-allocating FIFO sizes might not be the best choice. I mean, if you
only have two bulk endpoints enabled, you would be better off allocating
bigger FIFOs for those endpoints in order to allow for double- or
triple-buffering as that would give SW more time to program DMA to read
out the data.
> + dev_err(hsotg->dev, "invalid fifo sizes\n");
> +
> + params->host_rx_fifo_size = rxfsiz;
> + params->host_nperio_tx_fifo_size = nptxfsiz;
> + params->host_perio_tx_fifo_size = ptxfsiz;
> +}
> +
> +
> static void dwc2_config_fifos(struct dwc2_hsotg *hsotg)
> {
> struct dwc2_core_params *params = hsotg->core_params;
> @@ -495,19 +527,28 @@ static void dwc2_config_fifos(struct dwc2_hsotg *hsotg)
> writel(grxfsiz, hsotg->regs + GRXFSIZ);
> dev_dbg(hsotg->dev, "new grxfsiz=%08x\n", readl(hsotg->regs + GRXFSIZ));
>
> + /* Calculate the correct FIFO sizes */
> + dwc2_calculate_dynamic_fifo(hsotg);
> +
> + /* Rx FIFO */
> + printk("initial grxfsiz=%08x\n",
> + readl(hsotg->regs + GRXFSIZ));
looks like a case for dev_vdbg(). You *really* don't want that in every
case.
> + writel(params->host_rx_fifo_size, hsotg->regs + GRXFSIZ);
> + printk("new grxfsiz=%08x\n", readl(hsotg->regs + GRXFSIZ));
likewise.
> +
> /* Non-periodic Tx FIFO */
> - dev_dbg(hsotg->dev, "initial gnptxfsiz=%08x\n",
> + printk("initial gnptxfsiz=%08x\n",
> readl(hsotg->regs + GNPTXFSIZ));
dev_vdbg()
> nptxfsiz = params->host_nperio_tx_fifo_size <<
> FIFOSIZE_DEPTH_SHIFT & FIFOSIZE_DEPTH_MASK;
> nptxfsiz |= params->host_rx_fifo_size <<
> FIFOSIZE_STARTADDR_SHIFT & FIFOSIZE_STARTADDR_MASK;
> writel(nptxfsiz, hsotg->regs + GNPTXFSIZ);
> - dev_dbg(hsotg->dev, "new gnptxfsiz=%08x\n",
> + printk("new gnptxfsiz=%08x\n",
> readl(hsotg->regs + GNPTXFSIZ));
>
dev_vdbg()
> /* Periodic Tx FIFO */
> - dev_dbg(hsotg->dev, "initial hptxfsiz=%08x\n",
> + printk("initial hptxfsiz=%08x\n",
> readl(hsotg->regs + HPTXFSIZ));
dev_vdbg()
> hptxfsiz = params->host_perio_tx_fifo_size <<
> FIFOSIZE_DEPTH_SHIFT & FIFOSIZE_DEPTH_MASK;
> @@ -515,7 +556,7 @@ static void dwc2_config_fifos(struct dwc2_hsotg *hsotg)
> params->host_nperio_tx_fifo_size) <<
> FIFOSIZE_STARTADDR_SHIFT & FIFOSIZE_STARTADDR_MASK;
> writel(hptxfsiz, hsotg->regs + HPTXFSIZ);
> - dev_dbg(hsotg->dev, "new hptxfsiz=%08x\n",
> + printk("new hptxfsiz=%08x\n",
> readl(hsotg->regs + HPTXFSIZ));
dev_vdbg()
>
> if (hsotg->core_params->en_multiple_tx_fifo > 0 &&
> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index b8d4193..afbe1a4 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
> @@ -133,26 +133,6 @@ static int dwc2_driver_probe(struct platform_device *dev)
> (unsigned long)res->start, hsotg->regs);
>
> if (!of_property_read_u32(dev->dev.of_node,
> - "enable-dynamic-fifo", &prop)) {
> - params.enable_dynamic_fifo = prop;
> -
> - if (!of_property_read_u32(dev->dev.of_node,
> - "host-rx-fifo-size", &prop)) {
> - params.host_rx_fifo_size = prop;
> - }
> -
> - if (!of_property_read_u32(dev->dev.of_node,
> - "host-perio-tx-fifo-size", &prop)) {
> - params.host_perio_tx_fifo_size = prop;
> - }
> -
> - if (!of_property_read_u32(dev->dev.of_node,
> - "host-nperio-tx-fifo-size", &prop)) {
> - params.host_nperio_tx_fifo_size = prop;
> - }
> - }
> -
> - if (!of_property_read_u32(dev->dev.of_node,
> "dma-desc-enable", &prop)) {
> params.dma_desc_enable = prop;
> }
the changes to platform.c also should be in a separate patch. $subject
alone should be split into 3 patches.
--
balbi
Attachment:
signature.asc
Description: Digital signature
