[v3] PCI: dwc: dra7xx: Fix compilation warning.

drivers/pci/dwc/pci-dra7xx.c: In function ‘dra7xx_pcie_enable_msi_interrupts’:
drivers/pci/dwc/pci-dra7xx.c:177:7: warning: large integer implicitly truncated to unsigned type [-Woverflow]
       ~LEG_EP_INTERRUPTS & ~MSI);
       ^
drivers/pci/dwc/pci-dra7xx.c: In function ‘dra7xx_pcie_enable_wrapper_interrupts’:
drivers/pci/dwc/pci-dra7xx.c:187:7: warning: large integer implicitly truncated to unsigned type [-Woverflow]
       ~INTERRUPTS);

Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
---
Changes in v2:
             Add casts in the definitions.
Changes in v3:
             Change logic insted of casting.

 drivers/pci/dwc/pci-dra7xx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

On Thu, Jun 15, 2017 at 02:19:20PM +0530, Arvind Yadav wrote:
> drivers/pci/dwc/pci-dra7xx.c: In function ‘dra7xx_pcie_enable_msi_interrupts’:
> drivers/pci/dwc/pci-dra7xx.c:177:7: warning: large integer implicitly truncated to unsigned type [-Woverflow]
>        ~LEG_EP_INTERRUPTS & ~MSI);
>        ^
> drivers/pci/dwc/pci-dra7xx.c: In function ‘dra7xx_pcie_enable_wrapper_interrupts’:
> drivers/pci/dwc/pci-dra7xx.c:187:7: warning: large integer implicitly truncated to unsigned type [-Woverflow]
>        ~INTERRUPTS);

I don't object to the patch itself (hopefully Kishon will verify that
it's correct), but the subject and changelog are now completely wrong.

If the patch is correct, it is now a bug fix and is not at all about
fixing a compilation warning.  We used to write

  ~LEG_EP_INTERRUPTS & ~MSI
  ~(INTA | INTB | INTC | INTD) & ~MSI
  ~(BIT(0) | BIT(1) | BIT(2) | BIT(3)) & ~(BIT(4))
  ~(0x1 | 0x2 | 0x4 | 0x8) & ~(0x10)
  ~(0xf) & ~(0x10)
  0xfffffff0 & 0xffffffef
  0xffffffe0

and now we write

  LEG_EP_INTERRUPTS | MSI
  ...
  0x1f

It is about using these two registers correctly, and the fact that the
compilation warning goes away is a happy coincidence but not even
worth mentioning in the changelog.

So, if Kishon acks the content of the patch, please post this again
with an updated subject and changelog.

> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
> ---
> Changes in v2:
>              Add casts in the definitions.
> Changes in v3:
>              Change logic insted of casting.
> 
>  drivers/pci/dwc/pci-dra7xx.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c
> index 8decf46..668dc15 100644
> --- a/drivers/pci/dwc/pci-dra7xx.c
> +++ b/drivers/pci/dwc/pci-dra7xx.c
> @@ -174,7 +174,7 @@ static int dra7xx_pcie_establish_link(struct dw_pcie *pci)
>  static void dra7xx_pcie_enable_msi_interrupts(struct dra7xx_pcie *dra7xx)
>  {
>  	dra7xx_pcie_writel(dra7xx, PCIECTRL_DRA7XX_CONF_IRQSTATUS_MSI,
> -			   ~LEG_EP_INTERRUPTS & ~MSI);
> +			   LEG_EP_INTERRUPTS | MSI);
>  
>  	dra7xx_pcie_writel(dra7xx,
>  			   PCIECTRL_DRA7XX_CONF_IRQENABLE_SET_MSI,
> @@ -184,7 +184,7 @@ static void dra7xx_pcie_enable_msi_interrupts(struct dra7xx_pcie *dra7xx)
>  static void dra7xx_pcie_enable_wrapper_interrupts(struct dra7xx_pcie *dra7xx)
>  {
>  	dra7xx_pcie_writel(dra7xx, PCIECTRL_DRA7XX_CONF_IRQSTATUS_MAIN,
> -			   ~INTERRUPTS);
> +			   INTERRUPTS);
>  	dra7xx_pcie_writel(dra7xx, PCIECTRL_DRA7XX_CONF_IRQENABLE_SET_MAIN,
>  			   INTERRUPTS);
>  }
> -- 
> 1.9.1
>

Hi Kishon/Bjorn,

What is correct Setting for these two PCIECTRL_DRA7XX_CONF_IRQSTATUS_MSI 
and
PCIECTRL_DRA7XX_CONF_IRQSTATUS_MAIN register.

Value of register After change:
  register[PCIECTRL_DRA7XX_CONF_IRQSTATUS_MSI] = LEG_EP_INTERRUPTS |  MSI
                                           = 0x1f
register[PCIECTRL_DRA7XX_CONF_IRQSTATUS_MAIN] =  INTERRUPTS
                   =  0x1fff
Is this correct?

Thanks
~arvind

On Friday 16 June 2017 02:19 AM, Bjorn Helgaas wrote:
> On Thu, Jun 15, 2017 at 02:19:20PM +0530, Arvind Yadav wrote:
>> drivers/pci/dwc/pci-dra7xx.c: In function ‘dra7xx_pcie_enable_msi_interrupts’:
>> drivers/pci/dwc/pci-dra7xx.c:177:7: warning: large integer implicitly truncated to unsigned type [-Woverflow]
>>         ~LEG_EP_INTERRUPTS & ~MSI);
>>         ^
>> drivers/pci/dwc/pci-dra7xx.c: In function ‘dra7xx_pcie_enable_wrapper_interrupts’:
>> drivers/pci/dwc/pci-dra7xx.c:187:7: warning: large integer implicitly truncated to unsigned type [-Woverflow]
>>         ~INTERRUPTS);
> I don't object to the patch itself (hopefully Kishon will verify that
> it's correct), but the subject and changelog are now completely wrong.
>
> If the patch is correct, it is now a bug fix and is not at all about
> fixing a compilation warning.  We used to write
>
>    ~LEG_EP_INTERRUPTS & ~MSI
>    ~(INTA | INTB | INTC | INTD) & ~MSI
>    ~(BIT(0) | BIT(1) | BIT(2) | BIT(3)) & ~(BIT(4))
>    ~(0x1 | 0x2 | 0x4 | 0x8) & ~(0x10)
>    ~(0xf) & ~(0x10)
>    0xfffffff0 & 0xffffffef
>    0xffffffe0
>
> and now we write
>
>    LEG_EP_INTERRUPTS | MSI
>    ...
>    0x1f
>
> It is about using these two registers correctly, and the fact that the
> compilation warning goes away is a happy coincidence but not even
> worth mentioning in the changelog.
>
> So, if Kishon acks the content of the patch, please post this again
> with an updated subject and changelog.
>
>> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
>> ---
>> Changes in v2:
>>               Add casts in the definitions.
>> Changes in v3:
>>               Change logic insted of casting.
>>
>>   drivers/pci/dwc/pci-dra7xx.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c
>> index 8decf46..668dc15 100644
>> --- a/drivers/pci/dwc/pci-dra7xx.c
>> +++ b/drivers/pci/dwc/pci-dra7xx.c
>> @@ -174,7 +174,7 @@ static int dra7xx_pcie_establish_link(struct dw_pcie *pci)
>>   static void dra7xx_pcie_enable_msi_interrupts(struct dra7xx_pcie *dra7xx)
>>   {
>>   	dra7xx_pcie_writel(dra7xx, PCIECTRL_DRA7XX_CONF_IRQSTATUS_MSI,
>> -			   ~LEG_EP_INTERRUPTS & ~MSI);
>> +			   LEG_EP_INTERRUPTS | MSI);
>>   
>>   	dra7xx_pcie_writel(dra7xx,
>>   			   PCIECTRL_DRA7XX_CONF_IRQENABLE_SET_MSI,
>> @@ -184,7 +184,7 @@ static void dra7xx_pcie_enable_msi_interrupts(struct dra7xx_pcie *dra7xx)
>>   static void dra7xx_pcie_enable_wrapper_interrupts(struct dra7xx_pcie *dra7xx)
>>   {
>>   	dra7xx_pcie_writel(dra7xx, PCIECTRL_DRA7XX_CONF_IRQSTATUS_MAIN,
>> -			   ~INTERRUPTS);
>> +			   INTERRUPTS);
>>   	dra7xx_pcie_writel(dra7xx, PCIECTRL_DRA7XX_CONF_IRQENABLE_SET_MAIN,
>>   			   INTERRUPTS);
>>   }
>> -- 
>> 1.9.1
>>

Hi,

On Friday 16 June 2017 01:48 PM, Arvind Yadav wrote:
> Hi Kishon/Bjorn,
> 
> What is correct Setting for these two PCIECTRL_DRA7XX_CONF_IRQSTATUS_MSI and
> PCIECTRL_DRA7XX_CONF_IRQSTATUS_MAIN register.
> 
> Value of register After change:
>  register[PCIECTRL_DRA7XX_CONF_IRQSTATUS_MSI] = LEG_EP_INTERRUPTS |  MSI
>                                           = 0x1f
> register[PCIECTRL_DRA7XX_CONF_IRQSTATUS_MAIN] =  INTERRUPTS
>                   =  0x1fff
> Is this correct?

That's correct. That patch as such is correct but the changelog and subject has
to be fixed. You can use something like below for subject and changelog.

"PCI: dwc: dra7xx: Fix incorrect usage of IRQSTATUS_* registers

commit 47ff3de911 ("PCI: dra7xx: Add TI DRA7xx PCIe driver") in order to clear
MSI and MAIN interrupts requests wrote '0' to PCIECTRL_TI_CONF_IRQSTATUS_MSI
and PCIECTRL_TI_CONF_IRQSTATUS_MAIN registers. However the TRM has mentioned to
write '1' to clear pending event in these two registers. Fix it here.

This also helps to get rid of the following compilation warning:
drivers/pci/dwc/pci-dra7xx.c: In function ‘dra7xx_pcie_enable_msi_interrupts’:
drivers/pci/dwc/pci-dra7xx.c:177:7: warning: large integer implicitly truncated
to unsigned type [-Woverflow]
       ~LEG_EP_INTERRUPTS & ~MSI);
       ^
drivers/pci/dwc/pci-dra7xx.c: In function ‘dra7xx_pcie_enable_wrapper_interrupts’:
drivers/pci/dwc/pci-dra7xx.c:187:7: warning: large integer implicitly truncated
to unsigned type [-Woverflow]
       ~INTERRUPTS);"

For the patch itself
Acked-by: Kishon Vijay Abraham I <kishon@ti.com>

Thanks
Kishon

Hi,

Thanks for you suggestion. I will update change log and re-submit.

Regards
~arvind

On Monday 19 June 2017 02:49 PM, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Friday 16 June 2017 01:48 PM, Arvind Yadav wrote:
>> Hi Kishon/Bjorn,
>>
>> What is correct Setting for these two PCIECTRL_DRA7XX_CONF_IRQSTATUS_MSI and
>> PCIECTRL_DRA7XX_CONF_IRQSTATUS_MAIN register.
>>
>> Value of register After change:
>>   register[PCIECTRL_DRA7XX_CONF_IRQSTATUS_MSI] = LEG_EP_INTERRUPTS |  MSI
>>                                            = 0x1f
>> register[PCIECTRL_DRA7XX_CONF_IRQSTATUS_MAIN] =  INTERRUPTS
>>                    =  0x1fff
>> Is this correct?
> That's correct. That patch as such is correct but the changelog and subject has
> to be fixed. You can use something like below for subject and changelog.
>
> "PCI: dwc: dra7xx: Fix incorrect usage of IRQSTATUS_* registers
>
> commit 47ff3de911 ("PCI: dra7xx: Add TI DRA7xx PCIe driver") in order to clear
> MSI and MAIN interrupts requests wrote '0' to PCIECTRL_TI_CONF_IRQSTATUS_MSI
> and PCIECTRL_TI_CONF_IRQSTATUS_MAIN registers. However the TRM has mentioned to
> write '1' to clear pending event in these two registers. Fix it here.
>
> This also helps to get rid of the following compilation warning:
> drivers/pci/dwc/pci-dra7xx.c: In function ‘dra7xx_pcie_enable_msi_interrupts’:
> drivers/pci/dwc/pci-dra7xx.c:177:7: warning: large integer implicitly truncated
> to unsigned type [-Woverflow]
>         ~LEG_EP_INTERRUPTS & ~MSI);
>         ^
> drivers/pci/dwc/pci-dra7xx.c: In function ‘dra7xx_pcie_enable_wrapper_interrupts’:
> drivers/pci/dwc/pci-dra7xx.c:187:7: warning: large integer implicitly truncated
> to unsigned type [-Woverflow]
>         ~INTERRUPTS);"
>
> For the patch itself
> Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
>
> Thanks
> Kishon