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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 <[email protected]>
>> ---
>> 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 <[email protected]>
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 <[email protected]>
>
> Thanks
> Kishon