* Re: [PATCH] ide: pmac: Convert to using %pOF instead of full_name
From: David Miller @ 2017-07-18 21:58 UTC (permalink / raw)
To: robh; +Cc: benh, paulus, mpe, linux-kernel, devicetree, linux-ide,
linuxppc-dev
In-Reply-To: <20170718214339.7774-27-robh@kernel.org>
From: Rob Herring <robh@kernel.org>
Date: Tue, 18 Jul 2017 16:43:07 -0500
> Now that we have a custom printf format specifier, convert users of
> full_name to use %pOF instead. This is preparation to remove storing
> of the full path string for each node.
>
> Signed-off-by: Rob Herring <robh@kernel.org>
Acked-by: David S. Miller <davem@davemloft.net>
^ permalink raw reply
* [PATCH] ide: pmac: Convert to using %pOF instead of full_name
From: Rob Herring @ 2017-07-18 21:43 UTC (permalink / raw)
To: David S. Miller, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman
Cc: devicetree, linuxppc-dev, linux-kernel, linux-ide
Now that we have a custom printf format specifier, convert users of
full_name to use %pOF instead. This is preparation to remove storing
of the full path string for each node.
Signed-off-by: Rob Herring <robh@kernel.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: linux-ide@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
---
drivers/ide/pmac.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/ide/pmac.c b/drivers/ide/pmac.c
index 0c5d3a99468e..c5b902b86b44 100644
--- a/drivers/ide/pmac.c
+++ b/drivers/ide/pmac.c
@@ -1145,8 +1145,8 @@ static int pmac_ide_macio_attach(struct macio_dev *mdev,
return -ENOMEM;
if (macio_resource_count(mdev) == 0) {
- printk(KERN_WARNING "ide-pmac: no address for %s\n",
- mdev->ofdev.dev.of_node->full_name);
+ printk(KERN_WARNING "ide-pmac: no address for %pOF\n",
+ mdev->ofdev.dev.of_node);
rc = -ENXIO;
goto out_free_pmif;
}
@@ -1154,7 +1154,7 @@ static int pmac_ide_macio_attach(struct macio_dev *mdev,
/* Request memory resource for IO ports */
if (macio_request_resource(mdev, 0, "ide-pmac (ports)")) {
printk(KERN_ERR "ide-pmac: can't request MMIO resource for "
- "%s!\n", mdev->ofdev.dev.of_node->full_name);
+ "%pOF!\n", mdev->ofdev.dev.of_node);
rc = -EBUSY;
goto out_free_pmif;
}
@@ -1165,8 +1165,8 @@ static int pmac_ide_macio_attach(struct macio_dev *mdev,
* where that happens though...
*/
if (macio_irq_count(mdev) == 0) {
- printk(KERN_WARNING "ide-pmac: no intrs for device %s, using "
- "13\n", mdev->ofdev.dev.of_node->full_name);
+ printk(KERN_WARNING "ide-pmac: no intrs for device %pOF, using "
+ "13\n", mdev->ofdev.dev.of_node);
irq = irq_create_mapping(NULL, 13);
} else
irq = macio_irq(mdev, 0);
@@ -1183,8 +1183,8 @@ static int pmac_ide_macio_attach(struct macio_dev *mdev,
if (macio_resource_count(mdev) >= 2) {
if (macio_request_resource(mdev, 1, "ide-pmac (dma)"))
printk(KERN_WARNING "ide-pmac: can't request DMA "
- "resource for %s!\n",
- mdev->ofdev.dev.of_node->full_name);
+ "resource for %pOF!\n",
+ mdev->ofdev.dev.of_node);
else
pmif->dma_regs = ioremap(macio_resource_start(mdev, 1), 0x1000);
} else
@@ -1274,7 +1274,7 @@ static int pmac_ide_pci_attach(struct pci_dev *pdev,
if (pci_enable_device(pdev)) {
printk(KERN_WARNING "ide-pmac: Can't enable PCI device for "
- "%s\n", np->full_name);
+ "%pOF\n", np);
rc = -ENXIO;
goto out_free_pmif;
}
@@ -1282,7 +1282,7 @@ static int pmac_ide_pci_attach(struct pci_dev *pdev,
if (pci_request_regions(pdev, "Kauai ATA")) {
printk(KERN_ERR "ide-pmac: Cannot obtain PCI resources for "
- "%s\n", np->full_name);
+ "%pOF\n", np);
rc = -ENXIO;
goto out_free_pmif;
}
--
2.11.0
^ permalink raw reply related
* [PATCH] ata: Convert to using %pOF instead of full_name
From: Rob Herring @ 2017-07-18 21:42 UTC (permalink / raw)
To: Tejun Heo
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-ide-u79uwXL29TY76Z2rM5mHXA
Now that we have a custom printf format specifier, convert users of
full_name to use %pOF instead. This is preparation to remove storing
of the full path string for each node.
Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: linux-ide-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
drivers/ata/sata_svw.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/ata/sata_svw.c b/drivers/ata/sata_svw.c
index 0fd6ac7e57ba..a9d692c6c182 100644
--- a/drivers/ata/sata_svw.c
+++ b/drivers/ata/sata_svw.c
@@ -339,7 +339,7 @@ static int k2_sata_show_info(struct seq_file *m, struct Scsi_Host *shost)
if (!reg)
continue;
if (index == *reg) {
- seq_printf(m, "devspec: %s\n", np->full_name);
+ seq_printf(m, "devspec: %pOF\n", np);
break;
}
}
--
2.11.0
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [PATCH 07/14] proc/kcore: hide a harmless warning
From: Arnd Bergmann @ 2017-07-18 20:21 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: linux-kernel@vger.kernel.org, Jiri Olsa, Greg Kroah-Hartman,
Linus Torvalds, Tejun Heo, Guenter Roeck, IDE-ML,
Linux Media Mailing List, Andrew Morton, dri-devel, Kees Cook,
Ingo Molnar, Laura Abbott, Pratyush Anand
In-Reply-To: <CAKv+Gu8XZa6twiqheZ1JCCwKEmqvqeECAq7MyG_4WwO_WQMMQg@mail.gmail.com>
On Tue, Jul 18, 2017 at 10:07 PM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> On 18 July 2017 at 21:01, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Tue, Jul 18, 2017 at 9:55 PM, Ard Biesheuvel
>
> Ah, now it makes sense. I was a bit surprised that
> -Wtautological-compare complains about symbolic constants that resolve
> to the same expression, but apparently it doesn't.
>
> I see how ccache needs to preprocess first: that is how it notices
> changes, by hashing the preprocessed input and comparing it to the
> stored hash. I'd still expect it to go back to letting the compiler
> preprocess for the actual build, but apparently it doesn't.
When I tried to figure this out, I saw that ccache has two modes, "direct"
and "preprocessed". It usually tries to use direct mode unless something
prevents that.
In "direct" mode, it hashes the source file and the included headers
instead of the preprocessed source file, however it still calls the compiler
for the preprocessed source file, I guess since it has to preprocess the
file the first time it is seen so it can record which headers are included.
> A quick google search didn't produce anything useful, but I'd expect
> other ccache users to run into the same issue.
I suspect gcc-7 is still too new for most people to have noticed this.
The kernel is a very large codebase, and we only got a handful
of -Wtautological-compare warnings at all, most of them happen
wtihout ccache, too.
Among the four patches, three are for -Wtautological-compare, and one
is for -Wint-in-bool-context:
if (v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, &fmt->fmt.sliced))
v4l2_subdev_call() in this case is a function-like macro that may return
-ENODEV if its first argument is NULL. The other -Wint-in-bool-context
I found all happen with or without ccache, most commonly there is
an constant integer expression passed into a macro and then checked
like
#define macro(arg) \
do { \
if (arg) \
do_something(arg); \
} while (0)
Arnd
^ permalink raw reply
* Re: [PATCH 07/14] proc/kcore: hide a harmless warning
From: Ard Biesheuvel @ 2017-07-18 20:07 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-kernel@vger.kernel.org, Jiri Olsa, Greg Kroah-Hartman,
Linus Torvalds, Tejun Heo, Guenter Roeck, IDE-ML,
Linux Media Mailing List, Andrew Morton, dri-devel, Kees Cook,
Ingo Molnar, Laura Abbott, Pratyush Anand
In-Reply-To: <CAK8P3a0549U8zt7MPRJ5a6+pSZ-faqYiDTG3tNQGmvAbrZZfqw@mail.gmail.com>
On 18 July 2017 at 21:01, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tue, Jul 18, 2017 at 9:55 PM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>> On 18 July 2017 at 20:53, Arnd Bergmann <arnd@arndb.de> wrote:
>>> On Fri, Jul 14, 2017 at 2:28 PM, Ard Biesheuvel
>>> <ard.biesheuvel@linaro.org> wrote:
>>>> On 14 July 2017 at 10:25, Arnd Bergmann <arnd@arndb.de> wrote:
>>>>> gcc warns when MODULES_VADDR/END is defined to the same value as
>>>>> VMALLOC_START/VMALLOC_END, e.g. on x86-32:
>>>>>
>>>>> fs/proc/kcore.c: In function ‘add_modules_range’:
>>>>> fs/proc/kcore.c:622:161: error: self-comparison always evaluates to false [-Werror=tautological-compare]
>>>>> if (/*MODULES_VADDR != VMALLOC_START && */MODULES_END != VMALLOC_END) {
>>>>>
>>>>
>>>> Does it occur for subtraction as well? Or only for comparison?
>>>
>>> This replacement patch would also address the warning:
>>>
>>> diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
>>> index 45629f4b5402..35824e986c2c 100644
>>> --- a/fs/proc/kcore.c
>>> +++ b/fs/proc/kcore.c
>>> @@ -623,7 +623,7 @@ static void __init proc_kcore_text_init(void)
>>> struct kcore_list kcore_modules;
>>> static void __init add_modules_range(void)
>>> {
>>> - if (MODULES_VADDR != VMALLOC_START && MODULES_END != VMALLOC_END) {
>>> + if (MODULES_VADDR - VMALLOC_START && MODULES_END - VMALLOC_END) {
>>> kclist_add(&kcore_modules, (void *)MODULES_VADDR,
>>> MODULES_END - MODULES_VADDR, KCORE_VMALLOC);
>>> }
>>>
>>> I have also verified that four of the 14 patches are not needed when building
>>> without ccache, this is one of them:
>>>
>>> acpi: thermal: fix gcc-6/ccache warning
>>> proc/kcore: hide a harmless warning
>>> SFI: fix tautological-compare warning
>>> [media] fix warning on v4l2_subdev_call() result interpreted as bool
>>>
>>> Not sure what to do with those, we could either ignore them all and
>>> not care about ccache, or we try to address them all in some way.
>>>
>>
>> Any idea why ccache makes a difference here? It is not obvious (not to
>> me at least)
>
> When ccache is used, the compilation stage is apparently always done on
> the preprocessed source file. So instead of parsing (with the integrated
> preprocessor)
>
> if (MODULES_VADDR != VMALLOC_START ...)
>
> the compiler sees
>
> if (((unsigned long)high_memory + (8 * 1024 * 1024)) !=
> ((unsigned long)high_memory + (8 * 1024 * 1024)) ...)
>
> and it correctly considers the first expression something that one
> would write in source code, while -Wtautological-compare
> is intended to warn about the second version being always true,
> which makes the 'if()' pointless.
>
Ah, now it makes sense. I was a bit surprised that
-Wtautological-compare complains about symbolic constants that resolve
to the same expression, but apparently it doesn't.
I see how ccache needs to preprocess first: that is how it notices
changes, by hashing the preprocessed input and comparing it to the
stored hash. I'd still expect it to go back to letting the compiler
preprocess for the actual build, but apparently it doesn't.
A quick google search didn't produce anything useful, but I'd expect
other ccache users to run into the same issue.
^ permalink raw reply
* Re: [PATCH 07/14] proc/kcore: hide a harmless warning
From: Arnd Bergmann @ 2017-07-18 20:01 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: linux-kernel@vger.kernel.org, Jiri Olsa, Greg Kroah-Hartman,
Linus Torvalds, Tejun Heo, Guenter Roeck, IDE-ML,
Linux Media Mailing List, Andrew Morton, dri-devel, Kees Cook,
Ingo Molnar, Laura Abbott, Pratyush Anand
In-Reply-To: <CAKv+Gu_OYCNK2qvyDb0+7MqyG4rTEhsf57i6m9SJU80CE7Yt+g@mail.gmail.com>
On Tue, Jul 18, 2017 at 9:55 PM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> On 18 July 2017 at 20:53, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Fri, Jul 14, 2017 at 2:28 PM, Ard Biesheuvel
>> <ard.biesheuvel@linaro.org> wrote:
>>> On 14 July 2017 at 10:25, Arnd Bergmann <arnd@arndb.de> wrote:
>>>> gcc warns when MODULES_VADDR/END is defined to the same value as
>>>> VMALLOC_START/VMALLOC_END, e.g. on x86-32:
>>>>
>>>> fs/proc/kcore.c: In function ‘add_modules_range’:
>>>> fs/proc/kcore.c:622:161: error: self-comparison always evaluates to false [-Werror=tautological-compare]
>>>> if (/*MODULES_VADDR != VMALLOC_START && */MODULES_END != VMALLOC_END) {
>>>>
>>>
>>> Does it occur for subtraction as well? Or only for comparison?
>>
>> This replacement patch would also address the warning:
>>
>> diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
>> index 45629f4b5402..35824e986c2c 100644
>> --- a/fs/proc/kcore.c
>> +++ b/fs/proc/kcore.c
>> @@ -623,7 +623,7 @@ static void __init proc_kcore_text_init(void)
>> struct kcore_list kcore_modules;
>> static void __init add_modules_range(void)
>> {
>> - if (MODULES_VADDR != VMALLOC_START && MODULES_END != VMALLOC_END) {
>> + if (MODULES_VADDR - VMALLOC_START && MODULES_END - VMALLOC_END) {
>> kclist_add(&kcore_modules, (void *)MODULES_VADDR,
>> MODULES_END - MODULES_VADDR, KCORE_VMALLOC);
>> }
>>
>> I have also verified that four of the 14 patches are not needed when building
>> without ccache, this is one of them:
>>
>> acpi: thermal: fix gcc-6/ccache warning
>> proc/kcore: hide a harmless warning
>> SFI: fix tautological-compare warning
>> [media] fix warning on v4l2_subdev_call() result interpreted as bool
>>
>> Not sure what to do with those, we could either ignore them all and
>> not care about ccache, or we try to address them all in some way.
>>
>
> Any idea why ccache makes a difference here? It is not obvious (not to
> me at least)
When ccache is used, the compilation stage is apparently always done on
the preprocessed source file. So instead of parsing (with the integrated
preprocessor)
if (MODULES_VADDR != VMALLOC_START ...)
the compiler sees
if (((unsigned long)high_memory + (8 * 1024 * 1024)) !=
((unsigned long)high_memory + (8 * 1024 * 1024)) ...)
and it correctly considers the first expression something that one
would write in source code, while -Wtautological-compare
is intended to warn about the second version being always true,
which makes the 'if()' pointless.
Arnd
^ permalink raw reply
* Re: [PATCH 07/14] proc/kcore: hide a harmless warning
From: Ard Biesheuvel @ 2017-07-18 19:55 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-kernel@vger.kernel.org, Jiri Olsa, Greg Kroah-Hartman,
Linus Torvalds, Tejun Heo, Guenter Roeck, IDE-ML,
Linux Media Mailing List, Andrew Morton, dri-devel, Kees Cook,
Ingo Molnar, Laura Abbott, Pratyush Anand
In-Reply-To: <CAK8P3a3qrkZLkL=PuUHkaq9p021-Y+odTj5UrdM=dZw8L=oM8g@mail.gmail.com>
On 18 July 2017 at 20:53, Arnd Bergmann <arnd@arndb.de> wrote:
> On Fri, Jul 14, 2017 at 2:28 PM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>> On 14 July 2017 at 10:25, Arnd Bergmann <arnd@arndb.de> wrote:
>>> gcc warns when MODULES_VADDR/END is defined to the same value as
>>> VMALLOC_START/VMALLOC_END, e.g. on x86-32:
>>>
>>> fs/proc/kcore.c: In function ‘add_modules_range’:
>>> fs/proc/kcore.c:622:161: error: self-comparison always evaluates to false [-Werror=tautological-compare]
>>> if (/*MODULES_VADDR != VMALLOC_START && */MODULES_END != VMALLOC_END) {
>>>
>>
>> Does it occur for subtraction as well? Or only for comparison?
>
> This replacement patch would also address the warning:
>
> diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
> index 45629f4b5402..35824e986c2c 100644
> --- a/fs/proc/kcore.c
> +++ b/fs/proc/kcore.c
> @@ -623,7 +623,7 @@ static void __init proc_kcore_text_init(void)
> struct kcore_list kcore_modules;
> static void __init add_modules_range(void)
> {
> - if (MODULES_VADDR != VMALLOC_START && MODULES_END != VMALLOC_END) {
> + if (MODULES_VADDR - VMALLOC_START && MODULES_END - VMALLOC_END) {
> kclist_add(&kcore_modules, (void *)MODULES_VADDR,
> MODULES_END - MODULES_VADDR, KCORE_VMALLOC);
> }
>
> I have also verified that four of the 14 patches are not needed when building
> without ccache, this is one of them:
>
> acpi: thermal: fix gcc-6/ccache warning
> proc/kcore: hide a harmless warning
> SFI: fix tautological-compare warning
> [media] fix warning on v4l2_subdev_call() result interpreted as bool
>
> Not sure what to do with those, we could either ignore them all and
> not care about ccache, or we try to address them all in some way.
>
Any idea why ccache makes a difference here? It is not obvious (not to
me at least)
^ permalink raw reply
* Re: [PATCH 07/14] proc/kcore: hide a harmless warning
From: Arnd Bergmann @ 2017-07-18 19:53 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Pratyush Anand, Kees Cook, Greg Kroah-Hartman,
linux-kernel@vger.kernel.org, dri-devel, IDE-ML, Jiri Olsa,
Tejun Heo, Andrew Morton, Linus Torvalds, Ingo Molnar,
Guenter Roeck, Linux Media Mailing List
In-Reply-To: <CAKv+Gu9+0H9w8z8_Zedd-RxXt89Y7sJ=57_ZTThfDpFe4H0uXg@mail.gmail.com>
On Fri, Jul 14, 2017 at 2:28 PM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> On 14 July 2017 at 10:25, Arnd Bergmann <arnd@arndb.de> wrote:
>> gcc warns when MODULES_VADDR/END is defined to the same value as
>> VMALLOC_START/VMALLOC_END, e.g. on x86-32:
>>
>> fs/proc/kcore.c: In function ‘add_modules_range’:
>> fs/proc/kcore.c:622:161: error: self-comparison always evaluates to false [-Werror=tautological-compare]
>> if (/*MODULES_VADDR != VMALLOC_START && */MODULES_END != VMALLOC_END) {
>>
>
> Does it occur for subtraction as well? Or only for comparison?
This replacement patch would also address the warning:
diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
index 45629f4b5402..35824e986c2c 100644
--- a/fs/proc/kcore.c
+++ b/fs/proc/kcore.c
@@ -623,7 +623,7 @@ static void __init proc_kcore_text_init(void)
struct kcore_list kcore_modules;
static void __init add_modules_range(void)
{
- if (MODULES_VADDR != VMALLOC_START && MODULES_END != VMALLOC_END) {
+ if (MODULES_VADDR - VMALLOC_START && MODULES_END - VMALLOC_END) {
kclist_add(&kcore_modules, (void *)MODULES_VADDR,
MODULES_END - MODULES_VADDR, KCORE_VMALLOC);
}
I have also verified that four of the 14 patches are not needed when building
without ccache, this is one of them:
acpi: thermal: fix gcc-6/ccache warning
proc/kcore: hide a harmless warning
SFI: fix tautological-compare warning
[media] fix warning on v4l2_subdev_call() result interpreted as bool
Not sure what to do with those, we could either ignore them all and
not care about ccache, or we try to address them all in some way.
Arnd
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related
* Re: [PATCH V5 1/3] ata: ahci_tegra: Add AHCI support for tegra210
From: Bartlomiej Zolnierkiewicz @ 2017-07-18 16:16 UTC (permalink / raw)
To: Preetham Chandru Ramchandra
Cc: thierry.reding, tj, cyndis, preetham260, linux-tegra, linux-ide,
vbyravarasu, pkunapuli
In-Reply-To: <1498830993-24666-2-git-send-email-pchandru@nvidia.com>
Hi,
On Friday, June 30, 2017 07:26:31 PM Preetham Chandru Ramchandra wrote:
> From: Preetham Chandru R <pchandru@nvidia.com>
>
> 1. Move tegra124 specifics to tegra124_ahci_init.
> 2. Separate the regulators needed for tegra124 and tegra210.
> 3. Disable DIPM for t210 and t124 as there are known issues.
> 4. Disable Devslp for t210 since devslp pin is shared with
> pcie clkreq.
>
> Signed-off-by: Preetham Chandru R <pchandru@nvidia.com>
Please consider splitting this combined patch on smaller logical
parts (adding new initialization code used on both chipsets to
tegra_ahci_controller_init() should be done in preparatory
patch before adding T210 support, same for addition of NO_DIPM
flag) to ease the review, potential handling of regressions and
backporting of fixes to older kernels. The code changes itself
look good to me.
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply
* (unknown),
From: mitch_128 @ 2017-07-18 12:45 UTC (permalink / raw)
To: linux-ide
[-- Attachment #1: "EMAIL_2297760905018_linux-ide.zip --]
[-- Type: application/zip, Size: 3312 bytes --]
^ permalink raw reply
* Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool
From: Arnd Bergmann @ 2017-07-17 21:23 UTC (permalink / raw)
To: Hans Verkuil
Cc: Linux Kernel Mailing List, Mauro Carvalho Chehab,
Greg Kroah-Hartman, Linus Torvalds, Tejun Heo, Guenter Roeck,
IDE-ML, Linux Media Mailing List, Andrew Morton, dri-devel,
Niklas Söderlund, Robert Jarzmik, Daeseok Youn, Alan Cox,
adi-buildroot-devel, Linux-Renesas, Linux ARM
In-Reply-To: <3a927e60-332a-f01d-f1af-98649e9f51b5@xs4all.nl>
On Mon, Jul 17, 2017 at 4:35 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On 17/07/17 16:26, Arnd Bergmann wrote:
>> Let me try again without ccache for now and see what warnings remain.
>> We can find a solution for those first, and then decide how to deal with
>> ccache.
>
> Sounds good.
>
> I'm OK with applying this if there is no other way to prevent these warnings.
Small update: I noticed that having ccache being the default compiler
even with CCACHE_DISABLE=1 causes a lot of these warnings. Completely
taking ccache out of the picture however seems to have eliminated the
warnings about v4l2_subdev_call() and other silly warnings, but not
the interesting ones in the -Wint-in-bool-context category.
Arnd
^ permalink raw reply
* Re: [PATCH V5 3/3] dt-bindings: tegra: add binding documentation
From: Mikko Perttunen @ 2017-07-17 15:08 UTC (permalink / raw)
To: Preetham Chandru Ramchandra,
thierry.reding-Re5JQEeQqe8AvxtiuMwx3w, tj-DgEjT+Ai2ygdnm+yROfE0A
Cc: preetham260-Re5JQEeQqe8AvxtiuMwx3w,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-ide-u79uwXL29TY76Z2rM5mHXA,
vbyravarasu-DDmLM1+adcrQT0dZR+AlfA,
pkunapuli-DDmLM1+adcrQT0dZR+AlfA
In-Reply-To: <1498830993-24666-4-git-send-email-pchandru-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
On 30.06.2017 16:56, Preetham Chandru Ramchandra wrote:
> From: Preetham Chandru R <pchandru-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>
> This adds bindings documentation for the
> AHCI controller on Tegra210
>
> Signed-off-by: Preetham Chandru R <pchandru-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> v4:
> * changed the commit message
> * changed 'sata-cold' reset to mandatory for t210 and t124
> * Removed the regulators for T210 since these regulators
> will be enabled in phy driver.
> v3:
> * Add AUX register.
> v2:
> * change cml1, pll_e and phy regulators as optional
> for T210.
> ---
> .../bindings/ata/nvidia,tegra124-ahci.txt | 38 ++++++++++++++--------
> 1 file changed, 25 insertions(+), 13 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt b/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
> index 66c83c3..df4dc2c 100644
> --- a/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
> +++ b/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
> @@ -1,20 +1,19 @@
> -Tegra124 SoC SATA AHCI controller
> +Tegra SoC SATA AHCI controller
>
> Required properties :
> -- compatible : For Tegra124, must contain "nvidia,tegra124-ahci". Otherwise,
> - must contain '"nvidia,<chip>-ahci", "nvidia,tegra124-ahci"', where <chip>
> - is tegra132.
> -- reg : Should contain 2 entries:
> +- compatible : Must be one of:
> + - Tegra124 : "nvidia,tegra124-ahci"
> + - Tegra210 : "nvidia,tegra210-ahci"
> +- reg : Should contain 3 entries:
> - AHCI register set (SATA BAR5)
> - SATA register set
> + - Tegra210 : AUX register set
Perhaps just drop the Tegra210 since this register set also works on
Tegra124, even if it is optional.
> - interrupts : Defines the interrupt used by SATA
> - clocks : Must contain an entry for each entry in clock-names.
> See ../clocks/clock-bindings.txt for details.
> - clock-names : Must include the following entries:
> - sata
> - sata-oob
> - - cml1
> - - pll_e
> - resets : Must contain an entry for each entry in reset-names.
> See ../reset/reset.txt for details.
> - reset-names : Must include the following entries:
> @@ -24,9 +23,22 @@ Required properties :
> - phys : Must contain an entry for each entry in phy-names.
> See ../phy/phy-bindings.txt for details.
> - phy-names : Must include the following entries:
> - - sata-phy : XUSB PADCTL SATA PHY
> -- hvdd-supply : Defines the SATA HVDD regulator
> -- vddio-supply : Defines the SATA VDDIO regulator
> -- avdd-supply : Defines the SATA AVDD regulator
> -- target-5v-supply : Defines the SATA 5V power regulator
> -- target-12v-supply : Defines the SATA 12V power regulator
> + - For T124:
> + - sata-phy : XUSB PADCTL SATA PHY
> + - For T210:
> + - sata-0
> +- For T124:
> + - hvdd-supply : Defines the SATA HVDD regulator
> + - vddio-supply : Defines the SATA VDDIO regulator
> + - avdd-supply : Defines the SATA AVDD regulator
> + - target-5v-supply : Defines the SATA 5V power regulator
> + - target-12v-supply : Defines the SATA 12V power regulator
> +
> +Optional properties:
> +- clock-names :
> + - cml1 :
> + cml1 clock is required by phy so it is optional to define
> + here as phy driver will be enabling this clock.
> + - pll_e :
> + pll_e is the parent of cml1 clock so it is optional to define
> + here as phy driver will be enabling this clock.
Perhaps something like "These clocks should be defined here if the PHY
driver doesn't manage them. If it does, they should not be."
Also for next round, please also add robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org and
mark.rutland-5wv7dgnIgG8@public.gmane.org to To field and devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org to Cc
field. They should review the binding documentation. (Sorry for not
mentioning this earlier but I have been a bit fuzzy on the proper
procedure myself.)
Cheers,
Mikko
^ permalink raw reply
* Re: [PATCH V5 2/3] arm64: tegra: Enable AHCI on Tegra210
From: Mikko Perttunen @ 2017-07-17 14:56 UTC (permalink / raw)
To: Preetham Chandru Ramchandra, thierry.reding, tj
Cc: preetham260, linux-tegra, linux-ide, vbyravarasu, pkunapuli
In-Reply-To: <1498830993-24666-3-git-send-email-pchandru@nvidia.com>
On 30.06.2017 16:56, Preetham Chandru Ramchandra wrote:
> From: Preetham Chandru R <pchandru@nvidia.com>
>
> Enable AHCI on Tegra210 systems.
>
> Signed-off-by: Preetham Chandru R <pchandru@nvidia.com>
> ---
> v4:
> * Fixed missing space after 'AUX'
> ---
> arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi | 6 ++++++
> arch/arm64/boot/dts/nvidia/tegra210.dtsi | 16 ++++++++++++++++
> 2 files changed, 22 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi b/arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi
> index e5fc67b..58c28b9 100644
> --- a/arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi
> +++ b/arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi
> @@ -1324,6 +1324,12 @@
> status = "okay";
> };
>
> + sata@70020000 {
> + status = "okay";
> + phys = <&{/padctl@7009f000/pads/sata/lanes/sata-0}>;
> + phy-names = "sata-0";
> + };
> +
> padctl@7009f000 {
> status = "okay";
>
> diff --git a/arch/arm64/boot/dts/nvidia/tegra210.dtsi b/arch/arm64/boot/dts/nvidia/tegra210.dtsi
> index 29f471e..d767972 100644
> --- a/arch/arm64/boot/dts/nvidia/tegra210.dtsi
> +++ b/arch/arm64/boot/dts/nvidia/tegra210.dtsi
> @@ -797,6 +797,22 @@
> #iommu-cells = <1>;
> };
>
> + sata@70020000 {
> + compatible = "nvidia,tegra210-ahci";
> + reg = <0x0 0x70027000 0x0 0x2000>, /* AHCI */
> + <0x0 0x70020000 0x0 0x7000>, /* SATA */
> + <0x0 0x70001100 0x0 0x1000>; /* SATA AUX */
> + interrupts = <GIC_SPI 23 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&tegra_car TEGRA210_CLK_SATA>,
> + <&tegra_car TEGRA210_CLK_SATA_OOB>;
> + clock-names = "sata", "sata-oob";
> + resets = <&tegra_car 124>,
> + <&tegra_car 123>,
> + <&tegra_car 129>;
> + reset-names = "sata", "sata-oob", "sata-cold";
> + status = "disabled";
> + };
> +
> hda@70030000 {
> compatible = "nvidia,tegra210-hda", "nvidia,tegra30-hda";
> reg = <0x0 0x70030000 0x0 0x10000>;
>
Reviewed-by: Mikko Perttunen <mperttunen@nvidia.com>
Tested-by: Mikko Perttunen <mperttunen@nvidia.com> (on Jetson TX1)
^ permalink raw reply
* Re: [PATCH V5 1/3] ata: ahci_tegra: Add AHCI support for tegra210
From: Mikko Perttunen @ 2017-07-17 14:56 UTC (permalink / raw)
To: Preetham Chandru Ramchandra,
thierry.reding-Re5JQEeQqe8AvxtiuMwx3w, tj-DgEjT+Ai2ygdnm+yROfE0A
Cc: preetham260-Re5JQEeQqe8AvxtiuMwx3w,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-ide-u79uwXL29TY76Z2rM5mHXA,
vbyravarasu-DDmLM1+adcrQT0dZR+AlfA,
pkunapuli-DDmLM1+adcrQT0dZR+AlfA
In-Reply-To: <1498830993-24666-2-git-send-email-pchandru-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
On 30.06.2017 16:56, Preetham Chandru Ramchandra wrote:
> From: Preetham Chandru R <pchandru-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>
> 1. Move tegra124 specifics to tegra124_ahci_init.
> 2. Separate the regulators needed for tegra124 and tegra210.
> 3. Disable DIPM for t210 and t124 as there are known issues.
> 4. Disable Devslp for t210 since devslp pin is shared with
> pcie clkreq.
>
> Signed-off-by: Preetham Chandru R <pchandru-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> v5:
> * Remove unused NO_DIPM enum.
> * Move tegra_ahci_handle_quirks function definition
> instead of having a declaration.
> * Make ahci_tegra_port_info as const.
> v4:
> * Instead of disabling DIPM through quirk function,
> hardcode by adding ATA_FLAG_NO_DIPM to
> ahci_tegra_port_info's static definition
> * Changed access to aux register to optional because for T124,
> we need to continue to support older device trees without
> the aux_regs property
> v3:
> * Remove inline functions for read/write and modify to
> SATA, SATA Config and SATA Aux registers.
> * Add code to disable DIPM and DevSlp for t210 and t124
> v2:
> * Fix indentation issues
> * Move the change to disable DIPM, HIPM, DevSlp, partial,
> slumber and NCQ into a separate patch
> ---
> drivers/ata/ahci_tegra.c | 379 ++++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 296 insertions(+), 83 deletions(-)
>
> diff --git a/drivers/ata/ahci_tegra.c b/drivers/ata/ahci_tegra.c
> index 3a62eb2..d4c3fcd 100644
> --- a/drivers/ata/ahci_tegra.c
> +++ b/drivers/ata/ahci_tegra.c
> @@ -34,7 +34,8 @@
> #define DRV_NAME "tegra-ahci"
>
> #define SATA_CONFIGURATION_0 0x180
> -#define SATA_CONFIGURATION_EN_FPCI BIT(0)
> +#define SATA_CONFIGURATION_0_EN_FPCI BIT(0)
> +#define SATA_CONFIGURATION_0_CLK_OVERRIDE BIT(31)
>
> #define SCFG_OFFSET 0x1000
>
> @@ -45,17 +46,55 @@
> #define T_SATA0_CFG_1_SERR BIT(8)
>
> #define T_SATA0_CFG_9 0x24
> -#define T_SATA0_CFG_9_BASE_ADDRESS_SHIFT 13
> +#define T_SATA0_CFG_9_BASE_ADDRESS 0x40020000
>
> #define SATA_FPCI_BAR5 0x94
> -#define SATA_FPCI_BAR5_START_SHIFT 4
> +#define SATA_FPCI_BAR5_START_MASK (0xfffffff << 4)
> +#define SATA_FPCI_BAR5_START (0x0040020 << 4)
> +#define SATA_FPCI_BAR5_ACCESS_TYPE (0x1)
>
> #define SATA_INTR_MASK 0x188
> #define SATA_INTR_MASK_IP_INT_MASK BIT(16)
>
> +#define T_SATA0_CFG_35 0x94
> +#define T_SATA0_CFG_35_IDP_INDEX_MASK (0x7ff << 2)
> +#define T_SATA0_CFG_35_IDP_INDEX (0x2a << 2)
> +
> +#define T_SATA0_AHCI_IDP1 0x98
> +#define T_SATA0_AHCI_IDP1_DATA (0x400040)
> +
> +#define T_SATA0_CFG_PHY_1 0x12c
> +#define T_SATA0_CFG_PHY_1_PADS_IDDQ_EN BIT(23)
> +#define T_SATA0_CFG_PHY_1_PAD_PLL_IDDQ_EN BIT(22)
> +
> +#define T_SATA0_NVOOB 0x114
> +#define T_SATA0_NVOOB_COMMA_CNT_MASK (0xff << 16)
> +#define T_SATA0_NVOOB_COMMA_CNT (0x07 << 16)
> +#define T_SATA0_NVOOB_SQUELCH_FILTER_MODE_MASK (0x3 << 24)
> +#define T_SATA0_NVOOB_SQUELCH_FILTER_MODE (0x1 << 24)
> +#define T_SATA0_NVOOB_SQUELCH_FILTER_LENGTH_MASK (0x3 << 26)
> +#define T_SATA0_NVOOB_SQUELCH_FILTER_LENGTH (0x3 << 26)
> +
> +#define T_SATA_CFG_PHY_0 0x120
> +#define T_SATA_CFG_PHY_0_USE_7BIT_ALIGN_DET_FOR_SPD BIT(11)
> +#define T_SATA_CFG_PHY_0_MASK_SQUELCH BIT(24)
> +
> +#define T_SATA0_CFG2NVOOB_2 0x134
> +#define T_SATA0_CFG2NVOOB_2_COMWAKE_IDLE_CNT_LOW_MASK (0x1ff << 18)
> +#define T_SATA0_CFG2NVOOB_2_COMWAKE_IDLE_CNT_LOW (0xc << 18)
> +
> #define T_SATA0_AHCI_HBA_CAP_BKDR 0x300
> +#define T_SATA0_AHCI_HBA_CAP_BKDR_PARTIAL_ST_CAP BIT(13)
> +#define T_SATA0_AHCI_HBA_CAP_BKDR_SLUMBER_ST_CAP BIT(14)
> +#define T_SATA0_AHCI_HBA_CAP_BKDR_SALP BIT(26)
> +#define T_SATA0_AHCI_HBA_CAP_BKDR_SUPP_PM BIT(17)
> +#define T_SATA0_AHCI_HBA_CAP_BKDR_SNCQ BIT(30)
>
> #define T_SATA0_BKDOOR_CC 0x4a4
> +#define T_SATA0_BKDOOR_CC_CLASS_CODE_MASK (0xffff << 16)
> +#define T_SATA0_BKDOOR_CC_CLASS_CODE (0x0106 << 16)
> +#define T_SATA0_BKDOOR_CC_PROG_IF_MASK (0xff << 8)
> +#define T_SATA0_BKDOOR_CC_PROG_IF (0x01 << 8)
>
> #define T_SATA0_CFG_SATA 0x54c
> #define T_SATA0_CFG_SATA_BACKDOOR_PROG_IF_EN BIT(12)
> @@ -82,9 +121,34 @@
> #define T_SATA0_CHX_PHY_CTRL11 0x6d0
> #define T_SATA0_CHX_PHY_CTRL11_GEN2_RX_EQ (0x2800 << 16)
>
> +#define T_SATA0_CHX_PHY_CTRL17_0 0x6e8
> +#define T_SATA0_CHX_PHY_CTRL17_0_RX_EQ_CTRL_L_GEN1 0x55010000
> +#define T_SATA0_CHX_PHY_CTRL18_0 0x6ec
> +#define T_SATA0_CHX_PHY_CTRL18_0_RX_EQ_CTRL_L_GEN2 0x55010000
> +#define T_SATA0_CHX_PHY_CTRL20_0 0x6f4
> +#define T_SATA0_CHX_PHY_CTRL20_0_RX_EQ_CTRL_H_GEN1 0x1
> +#define T_SATA0_CHX_PHY_CTRL21_0 0x6f8
> +#define T_SATA0_CHX_PHY_CTRL21_0_RX_EQ_CTRL_H_GEN2 0x1
> +
> +/* AUX Registers */
> +#define SATA_AUX_MISC_CNTL_1_0 0x8
> +#define SATA_AUX_MISC_CNTL_1_0_DEVSLP_OVERRIDE BIT(17)
> +#define SATA_AUX_MISC_CNTL_1_0_SDS_SUPPORT BIT(13)
> +#define SATA_AUX_MISC_CNTL_1_0_DESO_SUPPORT BIT(15)
> +
> +#define SATA_AUX_RX_STAT_INT_0 0xc
> +#define SATA_AUX_RX_STAT_INT_0_SATA_DEVSLP BIT(7)
> +
> +#define SATA_AUX_SPARE_CFG0_0 0x18
> +#define SATA_AUX_SPARE_CFG0_0_MDAT_TIMER_AFTER_PG_VALID BIT(14)
> +
> #define FUSE_SATA_CALIB 0x124
> #define FUSE_SATA_CALIB_MASK 0x3
>
> +enum {
> + NO_DEVSLP = (1 << 0),
> +};
> +
> struct sata_pad_calibration {
> u8 gen1_tx_amp;
> u8 gen1_tx_peak;
> @@ -99,15 +163,101 @@ static const struct sata_pad_calibration tegra124_pad_calibration[] = {
> {0x14, 0x0e, 0x1a, 0x0e},
> };
>
> +struct tegra_ahci_ops {
> + int (*init)(struct ahci_host_priv *);
> +};
> +
> +struct tegra_ahci_soc {
> + const char *const *supply_names;
> + u32 num_supplies;
> + u32 quirks;
> + struct tegra_ahci_ops ops;
> +};
> +
> struct tegra_ahci_priv {
> struct platform_device *pdev;
> void __iomem *sata_regs;
> + void __iomem *sata_aux_regs;
> struct reset_control *sata_rst;
> struct reset_control *sata_oob_rst;
> struct reset_control *sata_cold_rst;
> /* Needs special handling, cannot use ahci_platform */
> struct clk *sata_clk;
> - struct regulator_bulk_data supplies[5];
> + struct regulator_bulk_data *supplies;
> + struct tegra_ahci_soc *soc_data;
> +};
> +
> +static const char *const tegra124_supply_names[] = {
> + "avdd", "hvdd", "vddio", "target-5v", "target-12v"
> +};
> +
> +static void tegra_ahci_handle_quirks(struct ahci_host_priv *hpriv)
> +{
> + struct tegra_ahci_priv *tegra = hpriv->plat_data;
> + u32 val;
> +
> + if (tegra->sata_aux_regs && (tegra->soc_data->quirks & NO_DEVSLP)) {
> + val = readl(tegra->sata_aux_regs + SATA_AUX_MISC_CNTL_1_0);
> + val &= ~SATA_AUX_MISC_CNTL_1_0_SDS_SUPPORT;
> + writel(val, tegra->sata_aux_regs + SATA_AUX_MISC_CNTL_1_0);
> + }
> +}
> +
> +static int tegra124_ahci_init(struct ahci_host_priv *hpriv)
> +{
> + struct tegra_ahci_priv *tegra = hpriv->plat_data;
> + struct sata_pad_calibration calib;
> + int ret;
> + u32 val;
> +
> + /* Pad calibration */
> + ret = tegra_fuse_readl(FUSE_SATA_CALIB, &val);
> + if (ret)
> + return ret;
> +
> + calib = tegra124_pad_calibration[val & FUSE_SATA_CALIB_MASK];
> +
> + writel(BIT(0), tegra->sata_regs + SCFG_OFFSET + T_SATA0_INDEX);
> +
> + val = readl(tegra->sata_regs +
> + SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL1_GEN1);
> + val &= ~T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_MASK;
> + val &= ~T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_MASK;
> + val |= calib.gen1_tx_amp << T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT;
> + val |= calib.gen1_tx_peak << T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_SHIFT;
> + writel(val, tegra->sata_regs + SCFG_OFFSET +
> + T_SATA0_CHX_PHY_CTRL1_GEN1);
> +
> + val = readl(tegra->sata_regs +
> + SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL1_GEN2);
> + val &= ~T_SATA0_CHX_PHY_CTRL1_GEN2_TX_AMP_MASK;
> + val &= ~T_SATA0_CHX_PHY_CTRL1_GEN2_TX_PEAK_MASK;
> + val |= calib.gen2_tx_amp << T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT;
> + val |= calib.gen2_tx_peak << T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_SHIFT;
> + writel(val, tegra->sata_regs + SCFG_OFFSET +
> + T_SATA0_CHX_PHY_CTRL1_GEN2);
> +
> + writel(T_SATA0_CHX_PHY_CTRL11_GEN2_RX_EQ,
> + tegra->sata_regs + SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL11);
> + writel(T_SATA0_CHX_PHY_CTRL2_CDR_CNTL_GEN1,
> + tegra->sata_regs + SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL2);
> +
> + writel(0, tegra->sata_regs + SCFG_OFFSET + T_SATA0_INDEX);
> +
> + return 0;
> +}
> +
> +static const struct tegra_ahci_soc tegra124_ahci_soc_data = {
> + .supply_names = tegra124_supply_names,
> + .num_supplies = ARRAY_SIZE(tegra124_supply_names),
> + .quirks = NO_DEVSLP,
> + .ops = {
> + .init = tegra124_ahci_init,
> + },
> +};
> +
> +static const struct tegra_ahci_soc tegra210_ahci_soc_data = {
> + .quirks = NO_DEVSLP,
> };
>
> static int tegra_ahci_power_on(struct ahci_host_priv *hpriv)
> @@ -115,7 +265,7 @@ static int tegra_ahci_power_on(struct ahci_host_priv *hpriv)
> struct tegra_ahci_priv *tegra = hpriv->plat_data;
> int ret;
>
> - ret = regulator_bulk_enable(ARRAY_SIZE(tegra->supplies),
> + ret = regulator_bulk_enable(tegra->soc_data->num_supplies,
> tegra->supplies);
> if (ret)
> return ret;
> @@ -144,8 +294,7 @@ static int tegra_ahci_power_on(struct ahci_host_priv *hpriv)
> tegra_powergate_power_off(TEGRA_POWERGATE_SATA);
>
> disable_regulators:
> - regulator_bulk_disable(ARRAY_SIZE(tegra->supplies), tegra->supplies);
> -
> + regulator_bulk_disable(tegra->soc_data->num_supplies, tegra->supplies);
> return ret;
> }
>
> @@ -162,97 +311,139 @@ static void tegra_ahci_power_off(struct ahci_host_priv *hpriv)
> clk_disable_unprepare(tegra->sata_clk);
> tegra_powergate_power_off(TEGRA_POWERGATE_SATA);
>
> - regulator_bulk_disable(ARRAY_SIZE(tegra->supplies), tegra->supplies);
> + regulator_bulk_disable(tegra->soc_data->num_supplies, tegra->supplies);
> }
>
> static int tegra_ahci_controller_init(struct ahci_host_priv *hpriv)
> {
> struct tegra_ahci_priv *tegra = hpriv->plat_data;
> int ret;
> - unsigned int val;
> - struct sata_pad_calibration calib;
> + u32 val;
>
> ret = tegra_ahci_power_on(hpriv);
> - if (ret) {
> - dev_err(&tegra->pdev->dev,
> - "failed to power on AHCI controller: %d\n", ret);
> + if (ret)
> return ret;
> - }
>
> + /*
> + * Program the following SATA IPFS registers
> + * to allow SW accesses to SATA's MMIO register range.
> + */
> + val = readl(tegra->sata_regs + SATA_FPCI_BAR5);
> + val &= ~(SATA_FPCI_BAR5_START_MASK | SATA_FPCI_BAR5_ACCESS_TYPE);
> + val |= SATA_FPCI_BAR5_START | SATA_FPCI_BAR5_ACCESS_TYPE;
> + writel(val, tegra->sata_regs + SATA_FPCI_BAR5);
> +
> + /* Program the following SATA IPFS register to enable the SATA */
> val = readl(tegra->sata_regs + SATA_CONFIGURATION_0);
> - val |= SATA_CONFIGURATION_EN_FPCI;
> + val |= SATA_CONFIGURATION_0_EN_FPCI;
> writel(val, tegra->sata_regs + SATA_CONFIGURATION_0);
>
> - /* Pad calibration */
> -
> - ret = tegra_fuse_readl(FUSE_SATA_CALIB, &val);
> - if (ret) {
> - dev_err(&tegra->pdev->dev,
> - "failed to read calibration fuse: %d\n", ret);
> - return ret;
> - }
> -
> - calib = tegra124_pad_calibration[val & FUSE_SATA_CALIB_MASK];
> -
> - writel(BIT(0), tegra->sata_regs + SCFG_OFFSET + T_SATA0_INDEX);
> -
> - val = readl(tegra->sata_regs +
> - SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL1_GEN1);
> - val &= ~T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_MASK;
> - val &= ~T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_MASK;
> - val |= calib.gen1_tx_amp <<
> - T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT;
> - val |= calib.gen1_tx_peak <<
> - T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_SHIFT;
> - writel(val, tegra->sata_regs + SCFG_OFFSET +
> - T_SATA0_CHX_PHY_CTRL1_GEN1);
> -
> - val = readl(tegra->sata_regs +
> - SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL1_GEN2);
> - val &= ~T_SATA0_CHX_PHY_CTRL1_GEN2_TX_AMP_MASK;
> - val &= ~T_SATA0_CHX_PHY_CTRL1_GEN2_TX_PEAK_MASK;
> - val |= calib.gen2_tx_amp <<
> - T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT;
> - val |= calib.gen2_tx_peak <<
> - T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_SHIFT;
> - writel(val, tegra->sata_regs + SCFG_OFFSET +
> - T_SATA0_CHX_PHY_CTRL1_GEN2);
> -
> - writel(T_SATA0_CHX_PHY_CTRL11_GEN2_RX_EQ,
> - tegra->sata_regs + SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL11);
> - writel(T_SATA0_CHX_PHY_CTRL2_CDR_CNTL_GEN1,
> - tegra->sata_regs + SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL2);
> -
> - writel(0, tegra->sata_regs + SCFG_OFFSET + T_SATA0_INDEX);
> -
> - /* Program controller device ID */
> + /* Electrical settings for better link stability */
> + val = T_SATA0_CHX_PHY_CTRL17_0_RX_EQ_CTRL_L_GEN1;
> + writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL17_0);
> + val = T_SATA0_CHX_PHY_CTRL18_0_RX_EQ_CTRL_L_GEN2;
> + writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL18_0);
> + val = T_SATA0_CHX_PHY_CTRL20_0_RX_EQ_CTRL_H_GEN1;
> + writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL20_0);
> + val = T_SATA0_CHX_PHY_CTRL21_0_RX_EQ_CTRL_H_GEN2;
> + writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL21_0);
> +
> + /* For SQUELCH Filter & Gen3 drive getting detected as Gen1 drive */
> +
> + val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA_CFG_PHY_0);
> + val |= T_SATA_CFG_PHY_0_MASK_SQUELCH;
> + val &= ~T_SATA_CFG_PHY_0_USE_7BIT_ALIGN_DET_FOR_SPD;
> + writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA_CFG_PHY_0);
> +
> + val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA0_NVOOB);
> + val &= ~(T_SATA0_NVOOB_COMMA_CNT_MASK |
> + T_SATA0_NVOOB_SQUELCH_FILTER_LENGTH_MASK |
> + T_SATA0_NVOOB_SQUELCH_FILTER_MODE_MASK);
> + val |= (T_SATA0_NVOOB_COMMA_CNT |
> + T_SATA0_NVOOB_SQUELCH_FILTER_LENGTH |
> + T_SATA0_NVOOB_SQUELCH_FILTER_MODE);
> + writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_NVOOB);
> +
> + /*
> + * Change CFG2NVOOB_2_COMWAKE_IDLE_CNT_LOW from 83.3 ns to 58.8ns
> + */
> + val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG2NVOOB_2);
> + val &= ~T_SATA0_CFG2NVOOB_2_COMWAKE_IDLE_CNT_LOW_MASK;
> + val |= T_SATA0_CFG2NVOOB_2_COMWAKE_IDLE_CNT_LOW;
> + writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG2NVOOB_2);
> +
> + if (tegra->soc_data->ops.init)
> + tegra->soc_data->ops.init(hpriv);
> +
> + /*
> + * Program the following SATA configuration registers
> + * to initialize SATA
> + */
> + val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_1);
> + val |= (T_SATA0_CFG_1_IO_SPACE | T_SATA0_CFG_1_MEMORY_SPACE |
> + T_SATA0_CFG_1_BUS_MASTER | T_SATA0_CFG_1_SERR);
> + writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_1);
> + val = T_SATA0_CFG_9_BASE_ADDRESS;
> + writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_9);
>
> + /* Program Class Code and Programming interface for SATA */
> val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_SATA);
> val |= T_SATA0_CFG_SATA_BACKDOOR_PROG_IF_EN;
> writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_SATA);
>
> - writel(0x01060100, tegra->sata_regs + SCFG_OFFSET + T_SATA0_BKDOOR_CC);
> + val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA0_BKDOOR_CC);
> + val &=
> + ~(T_SATA0_BKDOOR_CC_CLASS_CODE_MASK |
> + T_SATA0_BKDOOR_CC_PROG_IF_MASK);
> + val |= T_SATA0_BKDOOR_CC_CLASS_CODE | T_SATA0_BKDOOR_CC_PROG_IF;
> + writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_BKDOOR_CC);
>
> val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_SATA);
> val &= ~T_SATA0_CFG_SATA_BACKDOOR_PROG_IF_EN;
> writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_SATA);
>
> - /* Enable IO & memory access, bus master mode */
> -
> - val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_1);
> - val |= T_SATA0_CFG_1_IO_SPACE | T_SATA0_CFG_1_MEMORY_SPACE |
> - T_SATA0_CFG_1_BUS_MASTER | T_SATA0_CFG_1_SERR;
> - writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_1);
> -
> - /* Program SATA MMIO */
> -
> - writel(0x10000 << SATA_FPCI_BAR5_START_SHIFT,
> - tegra->sata_regs + SATA_FPCI_BAR5);
> + /* Enabling LPM capabilities through Backdoor Programming */
> + val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA0_AHCI_HBA_CAP_BKDR);
> + val |= (T_SATA0_AHCI_HBA_CAP_BKDR_PARTIAL_ST_CAP |
> + T_SATA0_AHCI_HBA_CAP_BKDR_SLUMBER_ST_CAP |
> + T_SATA0_AHCI_HBA_CAP_BKDR_SALP |
> + T_SATA0_AHCI_HBA_CAP_BKDR_SUPP_PM);
> + writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_AHCI_HBA_CAP_BKDR);
> +
> + /* SATA Second Level Clock Gating configuration
> + * Enabling Gating of Tx/Rx clocks and driving Pad IDDQ and Lane
> + * IDDQ Signals
> + */
> + val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_35);
> + val &= ~T_SATA0_CFG_35_IDP_INDEX_MASK;
> + val |= T_SATA0_CFG_35_IDP_INDEX;
> + writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_35);
> +
> + val = T_SATA0_AHCI_IDP1_DATA;
> + writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_AHCI_IDP1);
> +
> + val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_PHY_1);
> + val |= (T_SATA0_CFG_PHY_1_PADS_IDDQ_EN |
> + T_SATA0_CFG_PHY_1_PAD_PLL_IDDQ_EN);
> + writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_PHY_1);
> +
> + /*
> + * Indicate Sata only has the capability to enter DevSleep
> + * from slumber link.
> + */
> +
> + if (tegra->sata_aux_regs) {
> + val = readl(tegra->sata_aux_regs + SATA_AUX_MISC_CNTL_1_0);
> + val |= SATA_AUX_MISC_CNTL_1_0_DESO_SUPPORT;
> + writel(val, tegra->sata_aux_regs + SATA_AUX_MISC_CNTL_1_0);
> + }
>
> - writel(0x08000 << T_SATA0_CFG_9_BASE_ADDRESS_SHIFT,
> - tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_9);
> + /* Enabling IPFS Clock Gating */
> + val = readl(tegra->sata_regs + SATA_CONFIGURATION_0);
> + val &= ~SATA_CONFIGURATION_0_CLK_OVERRIDE;
> + writel(val, tegra->sata_regs + SATA_CONFIGURATION_0);
>
> - /* Unmask SATA interrupts */
> + tegra_ahci_handle_quirks(hpriv);
>
> val = readl(tegra->sata_regs + SATA_INTR_MASK);
> val |= SATA_INTR_MASK_IP_INT_MASK;
> @@ -279,14 +470,21 @@ static struct ata_port_operations ahci_tegra_port_ops = {
> };
>
> static const struct ata_port_info ahci_tegra_port_info = {
> - .flags = AHCI_FLAG_COMMON,
> + .flags = AHCI_FLAG_COMMON | ATA_FLAG_NO_DIPM,
> .pio_mask = ATA_PIO4,
> .udma_mask = ATA_UDMA6,
> .port_ops = &ahci_tegra_port_ops,
> };
>
> static const struct of_device_id tegra_ahci_of_match[] = {
> - { .compatible = "nvidia,tegra124-ahci" },
> + {
> + .compatible = "nvidia,tegra124-ahci",
> + .data = &tegra124_ahci_soc_data
> + },
> + {
> + .compatible = "nvidia,tegra210-ahci",
> + .data = &tegra210_ahci_soc_data
> + },
> {}
> };
> MODULE_DEVICE_TABLE(of, tegra_ahci_of_match);
> @@ -301,6 +499,7 @@ static int tegra_ahci_probe(struct platform_device *pdev)
> struct tegra_ahci_priv *tegra;
> struct resource *res;
> int ret;
> + unsigned int i;
>
> hpriv = ahci_platform_get_resources(pdev);
> if (IS_ERR(hpriv))
> @@ -311,13 +510,23 @@ static int tegra_ahci_probe(struct platform_device *pdev)
> return -ENOMEM;
>
> hpriv->plat_data = tegra;
> -
> tegra->pdev = pdev;
> + tegra->soc_data =
> + (struct tegra_ahci_soc *)of_device_get_match_data(&pdev->dev);
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> tegra->sata_regs = devm_ioremap_resource(&pdev->dev, res);
> if (IS_ERR(tegra->sata_regs))
> return PTR_ERR(tegra->sata_regs);
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> + /*
> + * Aux register is optional.
> + */
> + if (res) {
> + tegra->sata_aux_regs = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(tegra->sata_aux_regs))
> + return PTR_ERR(tegra->sata_aux_regs);
> + }
>
> tegra->sata_rst = devm_reset_control_get(&pdev->dev, "sata");
> if (IS_ERR(tegra->sata_rst)) {
> @@ -343,13 +552,17 @@ static int tegra_ahci_probe(struct platform_device *pdev)
> return PTR_ERR(tegra->sata_clk);
> }
>
> - tegra->supplies[0].supply = "avdd";
> - tegra->supplies[1].supply = "hvdd";
> - tegra->supplies[2].supply = "vddio";
> - tegra->supplies[3].supply = "target-5v";
> - tegra->supplies[4].supply = "target-12v";
> + tegra->supplies = devm_kcalloc(&pdev->dev,
> + tegra->soc_data->num_supplies,
> + sizeof(*tegra->supplies), GFP_KERNEL);
> + if (!tegra->supplies)
> + return -ENOMEM;
> +
> + for (i = 0; i < tegra->soc_data->num_supplies; i++)
> + tegra->supplies[i].supply = tegra->soc_data->supply_names[i];
>
> - ret = devm_regulator_bulk_get(&pdev->dev, ARRAY_SIZE(tegra->supplies),
> + ret = devm_regulator_bulk_get(&pdev->dev,
> + tegra->soc_data->num_supplies,
> tegra->supplies);
> if (ret) {
> dev_err(&pdev->dev, "Failed to get regulators\n");
> @@ -385,5 +598,5 @@ static struct platform_driver tegra_ahci_driver = {
> module_platform_driver(tegra_ahci_driver);
>
> MODULE_AUTHOR("Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>");
> -MODULE_DESCRIPTION("Tegra124 AHCI SATA driver");
> +MODULE_DESCRIPTION("Tegra AHCI SATA driver");
> MODULE_LICENSE("GPL v2");
>
Reviewed-by: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Tested-by: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> (on Jetson TX1)
For whatever reason this seems to work even though the PHY regulators
aren't currently handled by anyone. In any case, AIUI they should be
handled by the PHY driver so that isn't a problem for this driver.
Potential cosmetic questions are around the quirks and regulator lists.
In my opinion we are good enough.
Thierry, Tejun, please take a look.
Cheers,
Mikko
^ permalink raw reply
* Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool
From: Hans Verkuil @ 2017-07-17 14:35 UTC (permalink / raw)
To: Arnd Bergmann
Cc: devel, Niklas Söderlund, Alan Cox, Greg Kroah-Hartman,
Robert Jarzmik, Linux Kernel Mailing List, dri-devel,
adi-buildroot-devel, Linux-Renesas, IDE-ML, Linux ARM, Tejun Heo,
Andrew Morton, Mauro Carvalho Chehab, Linus Torvalds,
Daeseok Youn, Guenter Roeck, Linux Media Mailing List
In-Reply-To: <CAK8P3a1zBW_QuPtRFNwuVyE_ziySoV9_ebz4sD7Bya3eRoo8SA@mail.gmail.com>
On 17/07/17 16:26, Arnd Bergmann wrote:
> On Mon, Jul 17, 2017 at 3:45 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> On 14/07/17 11:36, Arnd Bergmann wrote:
>>> @@ -201,8 +202,9 @@ static int cx18_g_fmt_sliced_vbi_cap(struct file *file, void *fh,
>>> * digitizer/slicer. Note, cx18_av_vbi() wipes the passed in
>>> * fmt->fmt.sliced under valid calling conditions
>>> */
>>> - if (v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, &fmt->fmt.sliced))
>>> - return -EINVAL;
>>> + ret = v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, &fmt->fmt.sliced);
>>> + if (ret)
>>> + return ret;
>>
>> Please keep the -EINVAL here. I can't be 100% certain that returning 'ret' wouldn't
>> break something.
>
> I think Dan was recommending the opposite here, if I understood you
> both correctly:
> he said we should propagate the error code unless we know it's wrong, while you
> want to keep the current behavior to avoid introducing changes ;-)
>
> I guess in either case, looking at the callers more carefully would be
> a good idea.
The subtle problem here is that v4l2_subdev_call will return -ENOIOCTLCMD if
ops->vbi->g_sliced_fmt == NULL, which typically is not returned to userspace
but either ignored or replaced by another error. It indicates that the
sub device doesn't implement this operation, and it depends on the context
and the operation whether or not that is to be considered an error.
I have no clue what is expected here, without digging deep in the code.
Better to keep it as-is. It really isn't important to waste time on this.
>
>>> - return 0;
>>> + return ret;
>>> }
>>>
>>> int atomisp_flash_enable(struct atomisp_sub_device *asd, int num_frames)
>>>
>>
>> This is all very hackish, though. I'm not terribly keen on this patch. It's not
>> clear to me *why* these warnings appear in your setup.
>
> it's possible that this only happened with 'ccache', which first preprocesses
> the source and the passes it with v4l2_subdev_call expanded into the
> compiler. This means the line looks like
>
> if ((!(cx->sd_av) ? -ENODEV :
> (((cx->sd_av)->ops->vbi && (cx->sd_av)->ops->vbi->g_sliced_fmt) ?
> (cx->sd_av)->ops->vbi->g_sliced_fmt(cx->sd_av)),
> &fmt->fmt.sliced) :
> -ENOIOCTLCMD))
>
> The compiler now complains about the sub-expression that it sees for
> cx->sd_av==NULL:
>
> if (-ENODEV)
>
> which it considers nonsense because it is always true and the value gets
> ignored.
>
> Let me try again without ccache for now and see what warnings remain.
> We can find a solution for those first, and then decide how to deal with
> ccache.
Sounds good.
I'm OK with applying this if there is no other way to prevent these warnings.
Regards,
Hans
>
> Arnd
>
^ permalink raw reply
* Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool
From: Hans Verkuil @ 2017-07-17 14:32 UTC (permalink / raw)
To: Arnd Bergmann
Cc: devel, Niklas Söderlund, Alan Cox, Greg Kroah-Hartman,
Robert Jarzmik, Linux Kernel Mailing List, dri-devel,
adi-buildroot-devel, Linux-Renesas, IDE-ML, Linux ARM, Tejun Heo,
Andrew Morton, Mauro Carvalho Chehab, Linus Torvalds,
Daeseok Youn, Guenter Roeck, Linux Media Mailing List
In-Reply-To: <CAK8P3a1zBW_QuPtRFNwuVyE_ziySoV9_ebz4sD7Bya3eRoo8SA@mail.gmail.com>
On 17/07/17 16:26, Arnd Bergmann wrote:
> On Mon, Jul 17, 2017 at 3:45 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> On 14/07/17 11:36, Arnd Bergmann wrote:
>>> @@ -201,8 +202,9 @@ static int cx18_g_fmt_sliced_vbi_cap(struct file *file, void *fh,
>>> * digitizer/slicer. Note, cx18_av_vbi() wipes the passed in
>>> * fmt->fmt.sliced under valid calling conditions
>>> */
>>> - if (v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, &fmt->fmt.sliced))
>>> - return -EINVAL;
>>> + ret = v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, &fmt->fmt.sliced);
>>> + if (ret)
>>> + return ret;
>>
>> Please keep the -EINVAL here. I can't be 100% certain that returning 'ret' wouldn't
>> break something.
>
> I think Dan was recommending the opposite here, if I understood you
> both correctly:
> he said we should propagate the error code unless we know it's wrong, while you
> want to keep the current behavior to avoid introducing changes ;-)
>
> I guess in either case, looking at the callers more carefully would be
> a good idea.
The subtle problem here is that v4l2_subdev_call will return -ENOIOCTLCMD if
ops->vbi->g_sliced_fmt == NULL, which typically is not returned to userspace
but either ignored or replaced by another error. It indicates that the
sub device doesn't implement this operation, and it depends on the context
and the operation whether or not that is to be considered an error.
I have no clue what is expected here, without digging deep in the code.
Better to keep it as-is. It really isn't important to waste time on this.
>
>>> - return 0;
>>> + return ret;
>>> }
>>>
>>> int atomisp_flash_enable(struct atomisp_sub_device *asd, int num_frames)
>>>
>>
>> This is all very hackish, though. I'm not terribly keen on this patch. It's not
>> clear to me *why* these warnings appear in your setup.
>
> it's possible that this only happened with 'ccache', which first preprocesses
> the source and the passes it with v4l2_subdev_call expanded into the
> compiler. This means the line looks like
>
> if ((!(cx->sd_av) ? -ENODEV :
> (((cx->sd_av)->ops->vbi && (cx->sd_av)->ops->vbi->g_sliced_fmt) ?
> (cx->sd_av)->ops->vbi->g_sliced_fmt(cx->sd_av)),
> &fmt->fmt.sliced) :
> -ENOIOCTLCMD))
>
> The compiler now complains about the sub-expression that it sees for
> cx->sd_av==NULL:
>
> if (-ENODEV)
>
> which it considers nonsense because it is always true and the value gets
> ignored.
>
> Let me try again without ccache for now and see what warnings remain.
> We can find a solution for those first, and then decide how to deal with
> ccache.
Sounds good.
I'm OK with applying this if there is no other way to prevent these warnings.
Regards,
Hans
>
> Arnd
>
^ permalink raw reply
* Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool
From: Dan Carpenter @ 2017-07-17 14:28 UTC (permalink / raw)
To: Arnd Bergmann
Cc: devel, Linux-Renesas, Linux Media Mailing List,
Greg Kroah-Hartman, Daeseok Youn, Linus Torvalds,
Linux Kernel Mailing List, dri-devel, adi-buildroot-devel,
Hans Verkuil, IDE-ML, Guenter Roeck, Niklas Söderlund,
Tejun Heo, Andrew Morton, Mauro Carvalho Chehab, Robert Jarzmik,
Linux ARM, Alan Cox
In-Reply-To: <CAK8P3a1zBW_QuPtRFNwuVyE_ziySoV9_ebz4sD7Bya3eRoo8SA@mail.gmail.com>
On Mon, Jul 17, 2017 at 04:26:23PM +0200, Arnd Bergmann wrote:
> On Mon, Jul 17, 2017 at 3:45 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > On 14/07/17 11:36, Arnd Bergmann wrote:
> >> @@ -201,8 +202,9 @@ static int cx18_g_fmt_sliced_vbi_cap(struct file *file, void *fh,
> >> * digitizer/slicer. Note, cx18_av_vbi() wipes the passed in
> >> * fmt->fmt.sliced under valid calling conditions
> >> */
> >> - if (v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, &fmt->fmt.sliced))
> >> - return -EINVAL;
> >> + ret = v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, &fmt->fmt.sliced);
> >> + if (ret)
> >> + return ret;
> >
> > Please keep the -EINVAL here. I can't be 100% certain that returning 'ret' wouldn't
> > break something.
>
> I think Dan was recommending the opposite here, if I understood you
> both correctly:
> he said we should propagate the error code unless we know it's wrong, while you
> want to keep the current behavior to avoid introducing changes ;-)
>
I don't know the subsystem rules at all, so don't listen to me.
regards,
dan carpenter
^ permalink raw reply
* Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool
From: Arnd Bergmann @ 2017-07-17 14:26 UTC (permalink / raw)
To: Hans Verkuil
Cc: Linux Kernel Mailing List, Mauro Carvalho Chehab,
Greg Kroah-Hartman, Linus Torvalds, Tejun Heo, Guenter Roeck,
IDE-ML, Linux Media Mailing List, Andrew Morton, dri-devel,
Niklas Söderlund, Robert Jarzmik, Daeseok Youn, Alan Cox,
adi-buildroot-devel, Linux-Renesas, Linux ARM
In-Reply-To: <f57e08d9-0984-b67c-c64b-c7e0542d0361@xs4all.nl>
On Mon, Jul 17, 2017 at 3:45 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On 14/07/17 11:36, Arnd Bergmann wrote:
>> @@ -201,8 +202,9 @@ static int cx18_g_fmt_sliced_vbi_cap(struct file *file, void *fh,
>> * digitizer/slicer. Note, cx18_av_vbi() wipes the passed in
>> * fmt->fmt.sliced under valid calling conditions
>> */
>> - if (v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, &fmt->fmt.sliced))
>> - return -EINVAL;
>> + ret = v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, &fmt->fmt.sliced);
>> + if (ret)
>> + return ret;
>
> Please keep the -EINVAL here. I can't be 100% certain that returning 'ret' wouldn't
> break something.
I think Dan was recommending the opposite here, if I understood you
both correctly:
he said we should propagate the error code unless we know it's wrong, while you
want to keep the current behavior to avoid introducing changes ;-)
I guess in either case, looking at the callers more carefully would be
a good idea.
>> - return 0;
>> + return ret;
>> }
>>
>> int atomisp_flash_enable(struct atomisp_sub_device *asd, int num_frames)
>>
>
> This is all very hackish, though. I'm not terribly keen on this patch. It's not
> clear to me *why* these warnings appear in your setup.
it's possible that this only happened with 'ccache', which first preprocesses
the source and the passes it with v4l2_subdev_call expanded into the
compiler. This means the line looks like
if ((!(cx->sd_av) ? -ENODEV :
(((cx->sd_av)->ops->vbi && (cx->sd_av)->ops->vbi->g_sliced_fmt) ?
(cx->sd_av)->ops->vbi->g_sliced_fmt(cx->sd_av)),
&fmt->fmt.sliced) :
-ENOIOCTLCMD))
The compiler now complains about the sub-expression that it sees for
cx->sd_av==NULL:
if (-ENODEV)
which it considers nonsense because it is always true and the value gets
ignored.
Let me try again without ccache for now and see what warnings remain.
We can find a solution for those first, and then decide how to deal with
ccache.
Arnd
^ permalink raw reply
* Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool
From: Hans Verkuil @ 2017-07-17 13:45 UTC (permalink / raw)
To: Arnd Bergmann, linux-kernel, Mauro Carvalho Chehab
Cc: Greg Kroah-Hartman, Linus Torvalds, Tejun Heo, Guenter Roeck,
linux-ide, linux-media, akpm, dri-devel, Niklas Söderlund,
Robert Jarzmik, Daeseok Youn, Alan Cox, adi-buildroot-devel,
linux-renesas-soc, linux-arm-kernel, devel
In-Reply-To: <20170714093938.1469319-1-arnd@arndb.de>
On 14/07/17 11:36, Arnd Bergmann wrote:
> v4l2_subdev_call is a macro returning whatever the callback return
> type is, usually 'int'. With gcc-7 and ccache, this can lead to
> many wanings like:
>
> media/platform/pxa_camera.c: In function 'pxa_mbus_build_fmts_xlate':
> media/platform/pxa_camera.c:766:27: error: ?: using integer constants in boolean context [-Werror=int-in-bool-context]
> while (!v4l2_subdev_call(subdev, pad, enum_mbus_code, NULL, &code)) {
> media/atomisp/pci/atomisp2/atomisp_cmd.c: In function 'atomisp_s_ae_window':
> media/atomisp/pci/atomisp2/atomisp_cmd.c:6414:52: error: ?: using integer constants in boolean context [-Werror=int-in-bool-context]
> if (v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
>
> The best workaround I could come up with is to change all the
> callers that use the return code from v4l2_subdev_call() in an
> 'if' or 'while' condition.
>
> In case of simple 'if' checks, adding a temporary variable is
> usually ok, and sometimes this can be used to propagate or
> print an error code, so I do that.
>
> For the 'while' loops, I ended up adding an otherwise useless
> comparison with zero, which unfortunately makes the code a little
> uglied.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> drivers/media/pci/cx18/cx18-ioctl.c | 6 ++++--
> drivers/media/pci/saa7146/mxb.c | 5 +++--
> drivers/media/platform/atmel/atmel-isc.c | 4 ++--
> drivers/media/platform/atmel/atmel-isi.c | 4 ++--
> drivers/media/platform/blackfin/bfin_capture.c | 4 ++--
> drivers/media/platform/omap3isp/ispccdc.c | 5 +++--
> drivers/media/platform/pxa_camera.c | 3 ++-
> drivers/media/platform/rcar-vin/rcar-core.c | 2 +-
> drivers/media/platform/rcar-vin/rcar-dma.c | 4 +++-
> drivers/media/platform/soc_camera/soc_camera.c | 4 ++--
> drivers/media/platform/stm32/stm32-dcmi.c | 4 ++--
> drivers/media/platform/ti-vpe/cal.c | 6 ++++--
> drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c | 13 +++++++------
> 13 files changed, 37 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/media/pci/cx18/cx18-ioctl.c b/drivers/media/pci/cx18/cx18-ioctl.c
> index 80b902b12a78..1803f28fc501 100644
> --- a/drivers/media/pci/cx18/cx18-ioctl.c
> +++ b/drivers/media/pci/cx18/cx18-ioctl.c
> @@ -188,6 +188,7 @@ static int cx18_g_fmt_sliced_vbi_cap(struct file *file, void *fh,
> {
> struct cx18 *cx = fh2id(fh)->cx;
> struct v4l2_sliced_vbi_format *vbifmt = &fmt->fmt.sliced;
> + int ret;
>
> /* sane, V4L2 spec compliant, defaults */
> vbifmt->reserved[0] = 0;
> @@ -201,8 +202,9 @@ static int cx18_g_fmt_sliced_vbi_cap(struct file *file, void *fh,
> * digitizer/slicer. Note, cx18_av_vbi() wipes the passed in
> * fmt->fmt.sliced under valid calling conditions
> */
> - if (v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, &fmt->fmt.sliced))
> - return -EINVAL;
> + ret = v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, &fmt->fmt.sliced);
> + if (ret)
> + return ret;
Please keep the -EINVAL here. I can't be 100% certain that returning 'ret' wouldn't
break something.
>
> vbifmt->service_set = cx18_get_service_set(vbifmt);
> return 0;
<snip>
> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> index 97093baf28ac..fe56a037f065 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> @@ -6405,19 +6405,20 @@ int atomisp_s_ae_window(struct atomisp_sub_device *asd,
> struct atomisp_device *isp = asd->isp;
> /* Coverity CID 298071 - initialzize struct */
> struct v4l2_subdev_selection sel = { 0 };
> + int ret;
>
> sel.r.left = arg->x_left;
> sel.r.top = arg->y_top;
> sel.r.width = arg->x_right - arg->x_left + 1;
> sel.r.height = arg->y_bottom - arg->y_top + 1;
>
> - if (v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
> - pad, set_selection, NULL, &sel)) {
> - dev_err(isp->dev, "failed to call sensor set_selection.\n");
> - return -EINVAL;
> - }
> + ret = v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
> + pad, set_selection, NULL, &sel);
> + if (ret)
> + dev_err(isp->dev, "failed to call sensor set_selection: %d\n",
> + ret);
Same here: just keep the 'return -EINVAL'.
>
> - return 0;
> + return ret;
> }
>
> int atomisp_flash_enable(struct atomisp_sub_device *asd, int num_frames)
>
This is all very hackish, though. I'm not terribly keen on this patch. It's not
clear to me *why* these warnings appear in your setup.
Regards,
Hans
^ permalink raw reply
* Re: [PATCH, RESEND 03/14] drm/vmwgfx: avoid gcc-7 parentheses warning
From: Sinclair Yeh @ 2017-07-17 13:15 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Thomas Hellstrom, Greg Kroah-Hartman, Linux Kernel Mailing List,
DRI, IDE-ML, VMware Graphics, Tejun Heo, Brian Paul,
Andrew Morton, Linus Torvalds, Guenter Roeck,
Linux Media Mailing List
In-Reply-To: <CAK8P3a2QEOuQXy51q-EqzTh3STKTDHy2V-twi5nFPbuzOSEDkQ@mail.gmail.com>
On Fri, Jul 14, 2017 at 10:28:29PM +0200, Arnd Bergmann wrote:
> On Fri, Jul 14, 2017 at 9:23 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> > On Fri, Jul 14, 2017 at 12:21 PM, Linus Torvalds
> > <torvalds@linux-foundation.org> wrote:
> >>
> >> NAK. This takes unintentionally insane code and turns it intentionally
> >> insane. Any non-zero return is considered an error.
> >>
> >> The right fix is almost certainly to just return -EINVAL unconditionally.
Correct. I'll fix this.
> >
> > Btw, this is why I hate compiler warning fix patch series. Even when
> > they don't actually break the code (and sometimes they do that too),
> > they can actually end up making the code worse.
>
> I generally agree, and this is also why I held up sending patches for the
> -Wformat warnings until you brought those up. I also frequently send
> patches for recently introduced warnings, which tend to have a better
> chance of getting reviewed by the person that just introduced the code,
> to catch this kind of mistake in my patches.
>
> I also regularly run into cases where I send a correct patch and find
> that another broken patch has been applied the following day ;-)
>
> > The *intent* of that code was to return zero for the CAP_SYS_ADMIN.
> > But the code has never done that in its lifetime and nobody ever
> > noticed, so clearly the code shouldn't even have tried.
>
> Makes sense, yes. In this case, the review process has failed as
> well, as one of the maintainers even gave an Ack on the wrong patch,
> and then the patch got dropped without any feedback.
I've done some digging and noticed that my -fixes pull request
didn't get picked up last December. It's most likely because I
initially made an address typo in the original request, and then
followed it up with a direct email with the correct address.
Sinclair
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH 00/11] Drop unnecessary static
From: Julia Lawall @ 2017-07-15 21:00 UTC (permalink / raw)
To: Joe Perches
Cc: Julia Lawall, linux-arm-kernel, keescook, kernel-janitors,
linux-ide, linux-mtd, linux-kernel, linux-clk, linux-fbdev,
linux-omap, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, linux-input, linux-iio
In-Reply-To: <1500151079.4457.80.camel@perches.com>
On Sat, 15 Jul 2017, Joe Perches wrote:
> On Sat, 2017-07-15 at 22:07 +0200, Julia Lawall wrote:
> > These patches fix cases where there is a static on a local variable, but
> > the variable is initialized before any possible use. Thus, the static has
> > no benefit.
>
> Nice one.
Thanks. It was suggested by Kees Cook a while ago. There seem to be many
opportunities.
julia
^ permalink raw reply
* Re: [PATCH 00/11] Drop unnecessary static
From: Joe Perches @ 2017-07-15 20:37 UTC (permalink / raw)
To: Julia Lawall, linux-arm-kernel
Cc: keescook, kernel-janitors, linux-ide, linux-mtd, linux-kernel,
linux-clk, linux-fbdev, linux-omap, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, linux-input,
linux-iio
In-Reply-To: <1500149266-32357-1-git-send-email-Julia.Lawall@lip6.fr>
On Sat, 2017-07-15 at 22:07 +0200, Julia Lawall wrote:
> These patches fix cases where there is a static on a local variable, but
> the variable is initialized before any possible use. Thus, the static has
> no benefit.
Nice one.
^ permalink raw reply
* [PATCH 08/11] ata: Drop unnecessary static
From: Julia Lawall @ 2017-07-15 20:07 UTC (permalink / raw)
To: Linus Walleij
Cc: keescook, kernel-janitors, Tejun Heo, linux-ide, linux-kernel
In-Reply-To: <1500149266-32357-1-git-send-email-Julia.Lawall@lip6.fr>
Drop static on a local variable, when the variable is initialized before
any possible use. Thus, the static has no benefit.
The semantic patch that fixes this problem is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@bad exists@
position p;
identifier x;
type T;
@@
static T x@p;
...
x = <+...x...+>
@@
identifier x;
expression e;
type T;
position p != bad.p;
@@
-static
T x@p;
... when != x
when strict
?x = e;
// </smpl>
Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
---
These patches are all independent of each other.
drivers/ata/sata_gemini.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff -u -p a/drivers/ata/sata_gemini.c b/drivers/ata/sata_gemini.c
--- a/drivers/ata/sata_gemini.c
+++ b/drivers/ata/sata_gemini.c
@@ -305,7 +305,7 @@ static int gemini_sata_probe(struct plat
struct device *dev = &pdev->dev;
struct device_node *np = dev->of_node;
struct sata_gemini *sg;
- static struct regmap *map;
+ struct regmap *map;
struct resource *res;
enum gemini_muxmode muxmode;
u32 gmode;
^ permalink raw reply
* [PATCH 00/11] Drop unnecessary static
From: Julia Lawall @ 2017-07-15 20:07 UTC (permalink / raw)
To: linux-arm-kernel
Cc: linux-fbdev, Lars-Peter Clausen, keescook, linux-iio, linux-input,
kernel-janitors, linux-kernel, linux-ide, linux-mtd,
Peter Meerwald-Stadler, Hartmut Knaack, linux-omap, linux-clk
These patches fix cases where there is a static on a local variable, but
the variable is initialized before any possible use. Thus, the static has
no benefit.
---
drivers/ata/sata_gemini.c | 2 +-
drivers/clk/clk-moxart.c | 4 ++--
drivers/clk/mmp/clk.c | 2 +-
drivers/iio/orientation/hid-sensor-rotation.c | 2 +-
drivers/irqchip/irq-digicolor.c | 2 +-
drivers/irqchip/irq-gic-realview.c | 2 +-
drivers/irqchip/irq-mips-cpu.c | 2 +-
drivers/mfd/max8925-i2c.c | 2 +-
drivers/mfd/twl4030-irq.c | 2 +-
drivers/mtd/maps/physmap_of_versatile.c | 2 +-
drivers/video/fbdev/da8xx-fb.c | 2 +-
11 files changed, 12 insertions(+), 12 deletions(-)
^ permalink raw reply
* Re: [PATCH 10/14] staging:iio:resolver:ad2s1210 fix negative IIO_ANGL_VEL read
From: Jonathan Cameron @ 2017-07-15 11:42 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-kernel, Lars-Peter Clausen, Michael Hennerich,
Greg Kroah-Hartman, Linus Torvalds, Tejun Heo, Guenter Roeck,
linux-ide, linux-media, akpm, dri-devel, stable, Hartmut Knaack,
Peter Meerwald-Stadler, linux-iio, devel
In-Reply-To: <20170714093129.1366900-1-arnd@arndb.de>
On Fri, 14 Jul 2017 11:31:03 +0200
Arnd Bergmann <arnd@arndb.de> wrote:
> gcc-7 points out an older regression:
>
> drivers/staging/iio/resolver/ad2s1210.c: In function 'ad2s1210_read_raw':
> drivers/staging/iio/resolver/ad2s1210.c:515:42: error: '<<' in boolean context, did you mean '<' ? [-Werror=int-in-bool-context]
>
> The original code had 'unsigned short' here, but incorrectly got
> converted to 'bool'. This reverts the regression and uses a normal
> type instead.
>
> Fixes: 29148543c521 ("staging:iio:resolver:ad2s1210 minimal chan spec conversion.")
> Cc: stable@vger.kernel.org
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Thanks Arnd,
Applied to the fixes-togreg branch of iio.git.
Jonathan
> ---
> drivers/staging/iio/resolver/ad2s1210.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
> index a6a8393d6664..3e00df74b18c 100644
> --- a/drivers/staging/iio/resolver/ad2s1210.c
> +++ b/drivers/staging/iio/resolver/ad2s1210.c
> @@ -472,7 +472,7 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev,
> long m)
> {
> struct ad2s1210_state *st = iio_priv(indio_dev);
> - bool negative;
> + u16 negative;
> int ret = 0;
> u16 pos;
> s16 vel;
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox