[06/11] usb: dwc2/gadget: ensure that all fifos have correct memory buffers

Message ID 1403509890-14103-7-git-send-email-r.baldyga@samsung.com
State New
Headers show

Commit Message

Robert Baldyga June 23, 2014, 7:51 a.m.
From: Marek Szyprowski <m.szyprowski@samsung.com>

Print warning if FIFOs are configured in such a way that they don't fit
into the SPRAM available on the s3c hsotg module.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
---
 drivers/usb/dwc2/core.h   |  1 +
 drivers/usb/dwc2/gadget.c | 15 ++++++++++-----
 2 files changed, 11 insertions(+), 5 deletions(-)

Comments

Paul Zimmerman June 23, 2014, 6:40 p.m. | #1
> From: Robert Baldyga [mailto:r.baldyga@samsung.com]
> Sent: Monday, June 23, 2014 12:51 AM
> 
> From: Marek Szyprowski <m.szyprowski@samsung.com>
> 
> Print warning if FIFOs are configured in such a way that they don't fit
> into the SPRAM available on the s3c hsotg module.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
> ---
>  drivers/usb/dwc2/core.h   |  1 +
>  drivers/usb/dwc2/gadget.c | 15 ++++++++++-----
>  2 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index 1efd10c..067390e 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -194,6 +194,7 @@ struct s3c_hsotg {
>  	struct regulator_bulk_data supplies[ARRAY_SIZE(s3c_hsotg_supply_names)];
> 
>  	u32                     phyif;
> +	int			fifo_mem;
>  	unsigned int            dedicated_fifos:1;
>  	unsigned char           num_of_eps;
> 
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 95b6dcb..21d21de 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -194,6 +194,8 @@ static void s3c_hsotg_init_fifo(struct s3c_hsotg *hsotg)
>  	for (ep = 1; ep <= 15; ep++) {
>  		val = addr;
>  		val |= size << FIFOSIZE_DEPTH_SHIFT;
> +		WARN_ONCE(addr + size > hsotg->fifo_mem,
> +			  "insufficient fifo memory");
>  		addr += size;
> 
>  		writel(val, hsotg->regs + DPTXFSIZN(ep));
> @@ -3030,19 +3032,22 @@ static void s3c_hsotg_initep(struct s3c_hsotg *hsotg,
>   */
>  static void s3c_hsotg_hw_cfg(struct s3c_hsotg *hsotg)
>  {
> -	u32 cfg2, cfg4;
> +	u32 cfg2, cfg3, cfg4;
>  	/* check hardware configuration */
> 
>  	cfg2 = readl(hsotg->regs + 0x48);
>  	hsotg->num_of_eps = (cfg2 >> 10) & 0xF;
> 
> -	dev_info(hsotg->dev, "EPs:%d\n", hsotg->num_of_eps);
> +	cfg3 = readl(hsotg->regs + 0x4C);
> +	hsotg->fifo_mem = (cfg3 >> 16);
> 
>  	cfg4 = readl(hsotg->regs + 0x50);
>  	hsotg->dedicated_fifos = (cfg4 >> 25) & 1;
> 
> -	dev_info(hsotg->dev, "%s fifos\n",
> -		 hsotg->dedicated_fifos ? "dedicated" : "shared");
> +	dev_info(hsotg->dev, "EPs: %d, %s fifos, %d entries in SPRAM\n",
> +		 hsotg->num_of_eps,
> +		 hsotg->dedicated_fifos ? "dedicated" : "shared",
> +		 hsotg->fifo_mem);
>  }
> 
>  /**
> @@ -3495,8 +3500,8 @@ static int s3c_hsotg_probe(struct platform_device *pdev)
>  	s3c_hsotg_phy_enable(hsotg);
> 
>  	s3c_hsotg_corereset(hsotg);
> -	s3c_hsotg_init(hsotg);
>  	s3c_hsotg_hw_cfg(hsotg);
> +	s3c_hsotg_init(hsotg);

This last hunk looks like it is not related to the rest of the patch? If
so, I think it should be a separate patch.
Marek Szyprowski June 24, 2014, 5:50 a.m. | #2
Hello,

On 2014-06-23 20:40, Paul Zimmerman wrote:
>> From: Robert Baldyga [mailto:r.baldyga@samsung.com]
>> Sent: Monday, June 23, 2014 12:51 AM
>>
>> From: Marek Szyprowski <m.szyprowski@samsung.com>
>>
>> Print warning if FIFOs are configured in such a way that they don't fit
>> into the SPRAM available on the s3c hsotg module.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
>> ---
>>   drivers/usb/dwc2/core.h   |  1 +
>>   drivers/usb/dwc2/gadget.c | 15 ++++++++++-----
>>   2 files changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
>> index 1efd10c..067390e 100644
>> --- a/drivers/usb/dwc2/core.h
>> +++ b/drivers/usb/dwc2/core.h
>> @@ -194,6 +194,7 @@ struct s3c_hsotg {
>>   	struct regulator_bulk_data supplies[ARRAY_SIZE(s3c_hsotg_supply_names)];
>>
>>   	u32                     phyif;
>> +	int			fifo_mem;
>>   	unsigned int            dedicated_fifos:1;
>>   	unsigned char           num_of_eps;
>>
>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>> index 95b6dcb..21d21de 100644
>> --- a/drivers/usb/dwc2/gadget.c
>> +++ b/drivers/usb/dwc2/gadget.c
>> @@ -194,6 +194,8 @@ static void s3c_hsotg_init_fifo(struct s3c_hsotg *hsotg)
>>   	for (ep = 1; ep <= 15; ep++) {
>>   		val = addr;
>>   		val |= size << FIFOSIZE_DEPTH_SHIFT;
>> +		WARN_ONCE(addr + size > hsotg->fifo_mem,
>> +			  "insufficient fifo memory");
>>   		addr += size;
>>
>>   		writel(val, hsotg->regs + DPTXFSIZN(ep));
>> @@ -3030,19 +3032,22 @@ static void s3c_hsotg_initep(struct s3c_hsotg *hsotg,
>>    */
>>   static void s3c_hsotg_hw_cfg(struct s3c_hsotg *hsotg)
>>   {
>> -	u32 cfg2, cfg4;
>> +	u32 cfg2, cfg3, cfg4;
>>   	/* check hardware configuration */
>>
>>   	cfg2 = readl(hsotg->regs + 0x48);
>>   	hsotg->num_of_eps = (cfg2 >> 10) & 0xF;
>>
>> -	dev_info(hsotg->dev, "EPs:%d\n", hsotg->num_of_eps);
>> +	cfg3 = readl(hsotg->regs + 0x4C);
>> +	hsotg->fifo_mem = (cfg3 >> 16);
>>
>>   	cfg4 = readl(hsotg->regs + 0x50);
>>   	hsotg->dedicated_fifos = (cfg4 >> 25) & 1;
>>
>> -	dev_info(hsotg->dev, "%s fifos\n",
>> -		 hsotg->dedicated_fifos ? "dedicated" : "shared");
>> +	dev_info(hsotg->dev, "EPs: %d, %s fifos, %d entries in SPRAM\n",
>> +		 hsotg->num_of_eps,
>> +		 hsotg->dedicated_fifos ? "dedicated" : "shared",
>> +		 hsotg->fifo_mem);
>>   }
>>
>>   /**
>> @@ -3495,8 +3500,8 @@ static int s3c_hsotg_probe(struct platform_device *pdev)
>>   	s3c_hsotg_phy_enable(hsotg);
>>
>>   	s3c_hsotg_corereset(hsotg);
>> -	s3c_hsotg_init(hsotg);
>>   	s3c_hsotg_hw_cfg(hsotg);
>> +	s3c_hsotg_init(hsotg);
> This last hunk looks like it is not related to the rest of the patch? If
> so, I think it should be a separate patch.

s3c_hsotg_hw_cfg() only reads hw configuration and some values read there (fifo_mem)
are used in s3c_hsotg_init_fifo(), which is called from s3c_hsotg_init(). This chunk
really belongs to this patch, although it might not be easy to notice it at the first
glance.

Best regards

Patch

diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 1efd10c..067390e 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -194,6 +194,7 @@  struct s3c_hsotg {
 	struct regulator_bulk_data supplies[ARRAY_SIZE(s3c_hsotg_supply_names)];
 
 	u32                     phyif;
+	int			fifo_mem;
 	unsigned int            dedicated_fifos:1;
 	unsigned char           num_of_eps;
 
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 95b6dcb..21d21de 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -194,6 +194,8 @@  static void s3c_hsotg_init_fifo(struct s3c_hsotg *hsotg)
 	for (ep = 1; ep <= 15; ep++) {
 		val = addr;
 		val |= size << FIFOSIZE_DEPTH_SHIFT;
+		WARN_ONCE(addr + size > hsotg->fifo_mem,
+			  "insufficient fifo memory");
 		addr += size;
 
 		writel(val, hsotg->regs + DPTXFSIZN(ep));
@@ -3030,19 +3032,22 @@  static void s3c_hsotg_initep(struct s3c_hsotg *hsotg,
  */
 static void s3c_hsotg_hw_cfg(struct s3c_hsotg *hsotg)
 {
-	u32 cfg2, cfg4;
+	u32 cfg2, cfg3, cfg4;
 	/* check hardware configuration */
 
 	cfg2 = readl(hsotg->regs + 0x48);
 	hsotg->num_of_eps = (cfg2 >> 10) & 0xF;
 
-	dev_info(hsotg->dev, "EPs:%d\n", hsotg->num_of_eps);
+	cfg3 = readl(hsotg->regs + 0x4C);
+	hsotg->fifo_mem = (cfg3 >> 16);
 
 	cfg4 = readl(hsotg->regs + 0x50);
 	hsotg->dedicated_fifos = (cfg4 >> 25) & 1;
 
-	dev_info(hsotg->dev, "%s fifos\n",
-		 hsotg->dedicated_fifos ? "dedicated" : "shared");
+	dev_info(hsotg->dev, "EPs: %d, %s fifos, %d entries in SPRAM\n",
+		 hsotg->num_of_eps,
+		 hsotg->dedicated_fifos ? "dedicated" : "shared",
+		 hsotg->fifo_mem);
 }
 
 /**
@@ -3495,8 +3500,8 @@  static int s3c_hsotg_probe(struct platform_device *pdev)
 	s3c_hsotg_phy_enable(hsotg);
 
 	s3c_hsotg_corereset(hsotg);
-	s3c_hsotg_init(hsotg);
 	s3c_hsotg_hw_cfg(hsotg);
+	s3c_hsotg_init(hsotg);
 
 	/* hsotg->num_of_eps holds number of EPs other than ep0 */