* [PATCH V6 05/10] acpi: apei: handle SEA notification type for ARMv8
From: Baicar, Tyler @ 2017-01-10 17:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <586F74DB.3020107@arm.com>
Hello James,
On 1/6/2017 3:43 AM, James Morse wrote:
> On 05/01/17 22:31, Baicar, Tyler wrote:
>> On 12/20/2016 8:29 AM, James Morse wrote:
>>> On 07/12/16 21:48, Tyler Baicar wrote:
>>>> ARM APEI extension proposal added SEA (Synchrounous External
>>>> Abort) notification type for ARMv8.
>>>> Add a new GHES error source handling function for SEA. If an error
>>>> source's notification type is SEA, then this function can be registered
>>>> into the SEA exception handler. That way GHES will parse and report
>>>> SEA exceptions when they occur.
>>>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>>>> index 2acbc60..66ab3fd 100644
>>>> --- a/drivers/acpi/apei/ghes.c
>>>> +++ b/drivers/acpi/apei/ghes.c
>>>> @@ -767,6 +771,62 @@ static struct notifier_block ghes_notifier_sci = {
>>>> .notifier_call = ghes_notify_sci,
>>>> };
>>>> +#ifdef CONFIG_HAVE_ACPI_APEI_SEA
>>>> +static LIST_HEAD(ghes_sea);
>>>> +
>>>> +static int ghes_notify_sea(struct notifier_block *this,
>>>> + unsigned long event, void *data)
>>>> +{
>>>> + struct ghes *ghes;
>>>> + int ret = NOTIFY_DONE;
>>>> +
>>>> + rcu_read_lock();
>>>> + list_for_each_entry_rcu(ghes, &ghes_sea, list) {
>>>> + if (!ghes_proc(ghes))
>>>> + ret = NOTIFY_OK;
>>>> + }
>>>> + rcu_read_unlock();
>>>> +
>>>> + return ret;
>>>> +}
>>> What stops this from being re-entrant?
>>>
>>> ghes_copy_tofrom_phs() takes the ghes_ioremap_lock_irq spinlock, but there is
>>> nothing to stop a subsequent instruction fetch or memory access causing another
>>> (maybe different) Synchronous External Abort which deadlocks trying to take the
>>> same lock.
>>>
>>> ghes_notify_sea() looks to be based on ghes_notify_sci(), which (if I've found
>>> the right part of the ACPI spec) is a level-low interrupt. spin_lock_irqsave()
>>> would mask interrupts so there is no risk of a different notification firing on
>>> the same CPU, (it looks like they are almost all ultimately an irq).
>>>
>>> NMI is the odd one out because its not maskable like this, but ghes_notify_nmi()
>>> has:
>>>> if (!atomic_add_unless(&ghes_in_nmi, 1, 1))
>>>> return ret;
>>> To ensure there is only ever one thread poking around in this code.
>>>
>>> What happens if a system describes two GHES sources, one using an irq the other
>>> SEA? The SEA error can interrupt the irq error while its holding the above lock.
>>> I guess this is also why all the NMI code in that file is separate.
>
>> Let me see if I'm following you right :)
>> I should use spin_lock_irqsave() in ghes_notify_sea() to avoid ghes_notify_sci()
>> from
>> interrupting this process and potentially causing the deadlock?
> This way round you are already safe: The CPU masks interrupts when it takes the
> exception, they should still be masked by the time we get in here...
>
> The other way round is a lot more fun!
>
> What happens if APEI is processing some error record that was notified via an
> interrupt, and then takes the Synchronous External Abort, and ends up back in
> this code? Masking interrupts doesn't stop the external-abort, and trying to
> take the ghes_ioremap_lock_irq will deadlock.
>
> What happens if we interrupt printk() holding all its locks is another thing I
> haven't worked out yet.
>
>
>> This race condition does seem valid. We are using the same acknowledgment for
>> all our
>> HEST table entries, so our firmware will not populate more than one entry at a
>> time. That
>> gets us around this race condition.
> Ah, so your firmware will wait for the interrupt-signalled error to be finished
> before it triggers the Synchronous External Abort. I think this would still be a
> linux bug if the firmware didn't do this.
>
> x86 could have done the same with NMI notifications, but we have all this 'if
> (in_nmi)' to allow interrupts-masked GHES handling to be interrupted.
>
> What do you think to re-using the 'if (in_nmi)' code for SEA? We can argue that
> SEA is NMI-like in that it can't be masked, and it interrupts code that had
> interrupts masked. It 'should' be as simple as putting 'HAVE_NMI' in arm64's
> Kconfig, and wrapping the atomic notifier call with nmi_enter()/nmi_exit() from
> linux/hardirq.h. (...famous last words...)
>
> This probably answers my printk() questions too, but I need to look into it some
> more.
Thanks for the detailed description! I looked through this and it seems
like re-using the NMI code should work. I'll add the use of the in_nmi
code in the next patchset.
Thanks,
Tyler
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
^ permalink raw reply
* [PATCH v2 1/2] virtio_mmio: Set DMA masks appropriately
From: Robin Murphy @ 2017-01-10 17:51 UTC (permalink / raw)
To: linux-arm-kernel
Once DMA API usage is enabled, it becomes apparent that virtio-mmio is
inadvertently relying on the default 32-bit DMA mask, which leads to
problems like rapidly exhausting SWIOTLB bounce buffers.
Ensure that we set the appropriate 64-bit DMA mask whenever possible,
with the coherent mask suitably limited for the legacy vring as per
a0be1db4304f ("virtio_pci: Limit DMA mask to 44 bits for legacy virtio
devices").
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Michael S. Tsirkin <mst@redhat.com>
Reported-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
Fixes: b42111382f0e ("virtio_mmio: Use the DMA API if enabled")
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
drivers/virtio/virtio_mmio.c | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index d47a2fcef818..c71fde5fe835 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -59,6 +59,7 @@
#define pr_fmt(fmt) "virtio-mmio: " fmt
#include <linux/acpi.h>
+#include <linux/dma-mapping.h>
#include <linux/highmem.h>
#include <linux/interrupt.h>
#include <linux/io.h>
@@ -498,6 +499,7 @@ static int virtio_mmio_probe(struct platform_device *pdev)
struct virtio_mmio_device *vm_dev;
struct resource *mem;
unsigned long magic;
+ int rc;
mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (!mem)
@@ -547,9 +549,25 @@ static int virtio_mmio_probe(struct platform_device *pdev)
}
vm_dev->vdev.id.vendor = readl(vm_dev->base + VIRTIO_MMIO_VENDOR_ID);
- if (vm_dev->version == 1)
+ if (vm_dev->version == 1) {
writel(PAGE_SIZE, vm_dev->base + VIRTIO_MMIO_GUEST_PAGE_SIZE);
+ rc = dma_set_mask(&pdev->dev, DMA_BIT_MASK(64));
+ /*
+ * In the legacy case, ensure our coherently-allocated virtio
+ * ring will be at an address expressable as a 32-bit PFN.
+ */
+ if (!rc)
+ dma_set_coherent_mask(&pdev->dev,
+ DMA_BIT_MASK(32 + PAGE_SHIFT));
+ } else {
+ rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
+ }
+ if (rc)
+ rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
+ if (rc)
+ dev_warn(&pdev->dev, "Failed to enable 64-bit or 32-bit DMA. Trying to continue, but this might not work.\n");
+
platform_set_drvdata(pdev, vm_dev);
return register_virtio_device(&vm_dev->vdev);
--
2.10.2.dirty
^ permalink raw reply related
* [PATCH v2 2/2] vring: Force use of DMA API for ARM-based systems
From: Robin Murphy @ 2017-01-10 17:51 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <85015f1653eb7e36f992708362b75d1f4391b783.1484070340.git.robin.murphy@arm.com>
From: Will Deacon <will.deacon@arm.com>
Booting Linux on an ARM fastmodel containing an SMMU emulation results
in an unexpected I/O page fault from the legacy virtio-blk PCI device:
[ 1.211721] arm-smmu-v3 2b400000.smmu: event 0x10 received:
[ 1.211800] arm-smmu-v3 2b400000.smmu: 0x00000000fffff010
[ 1.211880] arm-smmu-v3 2b400000.smmu: 0x0000020800000000
[ 1.211959] arm-smmu-v3 2b400000.smmu: 0x00000008fa081002
[ 1.212075] arm-smmu-v3 2b400000.smmu: 0x0000000000000000
[ 1.212155] arm-smmu-v3 2b400000.smmu: event 0x10 received:
[ 1.212234] arm-smmu-v3 2b400000.smmu: 0x00000000fffff010
[ 1.212314] arm-smmu-v3 2b400000.smmu: 0x0000020800000000
[ 1.212394] arm-smmu-v3 2b400000.smmu: 0x00000008fa081000
[ 1.212471] arm-smmu-v3 2b400000.smmu: 0x0000000000000000
<system hangs failing to read partition table>
This is because the virtio-blk is behind an SMMU, so we have consequently
swizzled its DMA ops and configured the SMMU to translate accesses. This
then requires the vring code to use the DMA API to establish translations,
otherwise all transactions will result in fatal faults and termination.
Given that ARM-based systems only see an SMMU if one is really present
(the topology is all described by firmware tables such as device-tree or
IORT), then we can safely use the DMA API for all virtio devices.
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
drivers/virtio/virtio_ring.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 409aeaa49246..447245f2c813 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -159,6 +159,10 @@ static bool vring_use_dma_api(struct virtio_device *vdev)
if (xen_domain())
return true;
+ /* On ARM-based machines, the DMA ops will do the right thing */
+ if (IS_ENABLED(CONFIG_ARM) || IS_ENABLED(CONFIG_ARM64))
+ return true;
+
return false;
}
--
2.10.2.dirty
^ permalink raw reply related
* [GIT PULL] Allwinner fixes for 4.10
From: Maxime Ripard @ 2017-01-10 17:51 UTC (permalink / raw)
To: linux-arm-kernel
Hi Arnd, Olof,
Please pull the following changes for 4.10.
Thanks!
Maxime
The following changes since commit 7ce7d89f48834cefece7804d38fc5d85382edf77:
Linux 4.10-rc1 (2016-12-25 16:13:08 -0800)
are available in the git repository at:
https://git.kernel.org/pub/scm/linux/kernel/git/mripard/linux.git tags/sunxi-fixes-for-4.10
for you to fetch changes up to 3116d37651d77125bf50f81f859b1278e02ccce6:
ARM: dts: sunxi: Change node name for pwrseq pin on Olinuxino-lime2-emmc (2017-01-10 18:33:16 +0100)
----------------------------------------------------------------
Allwinner fixes for 4.10
A few fixes here and there to enable the build of some DT leftover, prevent
display issues or setup a proper muxing.
----------------------------------------------------------------
Chen-Yu Tsai (2):
ARM: dts: sun6i: Disable display pipeline by default
ARM: dts: sun6i: hummingbird: Enable display engine again
Emmanuel Vadot (1):
ARM: dts: sunxi: Change node name for pwrseq pin on Olinuxino-lime2-emmc
Milo Kim (1):
ARM: dts: sun8i: Support DTB build for NanoPi M1
arch/arm/boot/dts/Makefile | 1 +
arch/arm/boot/dts/sun6i-a31-hummingbird.dts | 4 ++++
arch/arm/boot/dts/sun6i-a31.dtsi | 1 +
arch/arm/boot/dts/sun7i-a20-olinuxino-lime2-emmc.dts | 2 +-
4 files changed, 7 insertions(+), 1 deletion(-)
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170110/5ec32ec7/attachment.sig>
^ permalink raw reply
* [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling
From: Anand Moon @ 2017-01-10 17:53 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <28815306-e220-4847-99ce-b058baae0d07@osg.samsung.com>
Hi Shuah,
On 10 January 2017 at 21:58, Shuah Khan <shuahkh@osg.samsung.com> wrote:
> On 01/10/2017 09:05 AM, Bartlomiej Zolnierkiewicz wrote:
>>
>> Hi,
>>
>> On Tuesday, January 10, 2017 07:36:35 AM Shuah Khan wrote:
>>> On 01/10/2017 07:16 AM, Shuah Khan wrote:
>>>> On 01/10/2017 05:05 AM, Bartlomiej Zolnierkiewicz wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> On Monday, January 09, 2017 07:21:31 PM Shuah Khan wrote:
>>>>>> Fix dwc3_exynos_probe() to call clk_prepare_enable() only when suspend
>>>>>> clock is specified. Call clk_disable_unprepare() from remove and probe
>>>>>> error path only when susp_clk has been set from remove and probe error
>>>>>> paths.
>>>>>
>>>>> It is legal to call clk_prepare_enable() and clk_disable_unprepare()
>>>>> for NULL clock. Also your patch changes susp_clk handling while
>>>>> leaves axius_clk handling (which also can be NULL) untouched.
>>>>>
>>>>> Do you actually see some runtime problem with the current code?
>>>>>
>>>>> If not then the patch should probably be dropped.
>>>>>
>>>>> Best regards,
>>>>> --
>>>>> Bartlomiej Zolnierkiewicz
>>>>> Samsung R&D Institute Poland
>>>>> Samsung Electronics
>>>>
>>>> Hi Bartlomiej,
>>>>
>>>> I am seeing the "no suspend clk specified" message in dmesg.
>>>> After that it sets the exynos->susp_clk = NULL and starts
>>>> calling clk_prepare_enable(exynos->susp_clk);
>>>>
>>>> That can't be good. If you see the logic right above this
>>>> one for exynos->clk, it returns error and fails the probe.
>>>> This this case it doesn't, but tries to use null susp_clk.
>>
>> exynos->susp_clk is optional, exynos->clk is not.
>
> Right. That is clear since we don't fail the probe.
>
>>
>>>> I believe this patch is necessary.
>>>
>>> Let me clarify this a bit further. Since we already know
>>> susp_clk is null, with this patch we can avoid extra calls
>>> to clk_prepare_enable() and clk_disable_unprepare().
>>>
>>> One can say, it also adds extra checks, hence I will let you
>>> decide one way or the other. :)
>>
>> I would prefer to leave the things as they are currently.
>>
>> The code in question is not performance sensitive so extra
>> calls are not a problem. No extra checks means less code.
>>
>> Also the current code seems to be more in line with the rest
>> of the kernel.
>
> What functionality is missing without the suspend clock? Would
> it make sense to change the info. message to include what it
> means. At the moment it doesn't anything more than "no suspend
> clock" which is a very cryptic user visible message. It would be
> helpful for it to also include what functionality is impacted.
>
Both usbdrd30_susp_clk and usbdrd30_axius_clk are used by exynos5433 platform
so moving the clk under compatible string "samsung,exynos7-dwusb3" make sense.
Best Regards
-Anand
> thanks,
> -- Shuah
>
>>
>> Best regards,
>> --
>> Bartlomiej Zolnierkiewicz
>> Samsung R&D Institute Poland
>> Samsung Electronics
>>
>>> thanks,
>>> -- Shuah
>>>
>>>>
>>>> thanks,
>>>> -- Shuah
>>>>
>>>>>
>>>>>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
>>>>>> ---
>>>>>> drivers/usb/dwc3/dwc3-exynos.c | 10 ++++++----
>>>>>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
>>>>>> index e27899b..f97a3d7 100644
>>>>>> --- a/drivers/usb/dwc3/dwc3-exynos.c
>>>>>> +++ b/drivers/usb/dwc3/dwc3-exynos.c
>>>>>> @@ -131,8 +131,8 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
>>>>>> if (IS_ERR(exynos->susp_clk)) {
>>>>>> dev_info(dev, "no suspend clk specified\n");
>>>>>> exynos->susp_clk = NULL;
>>>>>> - }
>>>>>> - clk_prepare_enable(exynos->susp_clk);
>>>>>> + } else
>>>>>> + clk_prepare_enable(exynos->susp_clk);
>>>>>>
>>>>>> if (of_device_is_compatible(node, "samsung,exynos7-dwusb3")) {
>>>>>> exynos->axius_clk = devm_clk_get(dev, "usbdrd30_axius_clk");
>>>>>> @@ -196,7 +196,8 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
>>>>>> regulator_disable(exynos->vdd33);
>>>>>> err2:
>>>>>> clk_disable_unprepare(exynos->axius_clk);
>>>>>> - clk_disable_unprepare(exynos->susp_clk);
>>>>>> + if (exynos->susp_clk)
>>>>>> + clk_disable_unprepare(exynos->susp_clk);
>>>>>> clk_disable_unprepare(exynos->clk);
>>>>>> return ret;
>>>>>> }
>>>>>> @@ -210,7 +211,8 @@ static int dwc3_exynos_remove(struct platform_device *pdev)
>>>>>> platform_device_unregister(exynos->usb3_phy);
>>>>>>
>>>>>> clk_disable_unprepare(exynos->axius_clk);
>>>>>> - clk_disable_unprepare(exynos->susp_clk);
>>>>>> + if (exynos->susp_clk)
>>>>>> + clk_disable_unprepare(exynos->susp_clk);
>>>>>> clk_disable_unprepare(exynos->clk);
>>>>>>
>>>>>> regulator_disable(exynos->vdd33);
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH v3] ARM: dts: sun7i: Add wifi dt node on Banana Pro
From: Maxime Ripard @ 2017-01-10 17:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170109203638.29546-1-joerg.krause@embedded.rocks>
On Mon, Jan 09, 2017 at 09:36:38PM +0100, J?rg Krause wrote:
> The Banana Pro has an AMPAK AP6181 WiFi+Bluetooth module. The WiFi part
> is a BCM43362 IC connected to MMC3 of the A20 SoC via SDIO. The IC also
> takes a power enable signal via GPIO.
>
> This commit adds a device-tree node to power it up, so the mmc subsys
> can scan it, and enables the mmc controller which is connected to it.
>
> As the wifi enable pin of the AP6181 module is not really a regulator,
> switch the mmc3 node to the mmc-pwrseq framework for controlling it.
> This more accurately reflectes how the hardware actually works.
>
> Signed-off-by: J?rg Krause <joerg.krause@embedded.rocks>
Applied, thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170110/caa18787/attachment.sig>
^ permalink raw reply
* [PATCH v3 04/10] Documentation: perf: hisi: Documentation for HiP05/06/07 PMU event counting.
From: Mark Rutland @ 2017-01-10 17:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1483339777-23973-1-git-send-email-anurup.m@huawei.com>
On Mon, Jan 02, 2017 at 01:49:37AM -0500, Anurup M wrote:
> +The Hisilicon SoC HiP05/06/07 chips consist of various independent system
> +device PMU's such as L3 cache(L3C) and Miscellaneous Nodes(MN).
> +These PMU devices are independent and have hardware logic to gather
> +statistics and performance information.
> +
> +HiP0x chips are encapsulated by multiple CPU and IO die's. The CPU die is
> +called as Super CPU cluster (SCCL) which includes 16 cpu-cores. Every SCCL
> +is further grouped as CPU clusters (CCL) which includes 4 cpu-cores each.
> +Each SCCL has 1 L3 cache and 1 MN units.
Are there systems with multiple SCCLs? Or is there only one SCCL per
system?
> +The L3 cache is shared by all CPU cores in a CPU die. The L3C has four banks
> +(or instances). Each bank or instance of L3C has Eight 32-bit counter
> +registers and also event control registers. The HiP05/06 chip L3 cache has
> +22 statistics events. The HiP07 chip has 66 statistics events. These events
> +are very useful for debugging.
Is an L3C associated with a subset of physical memory (as with the ARM
CCN's L3C), or is it associated with a set of CPUs (e.g. only those in
a single SCCL) covering all physical memory (as with each CPU's L1 &
L2)?
Thanks,
Mark.
^ permalink raw reply
* [PATCH] ARM: defconfig: include QCOM_EBI2 in multi_v7
From: Olof Johansson @ 2017-01-10 18:02 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170110100931.15791-1-linus.walleij@linaro.org>
On Tue, Jan 10, 2017 at 2:09 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> This adds the Qualcomm External Bus Interface 2 to the multi_v7
> defconfig: it is hard for users to realize that this is a
> required driver for getting things like ethernet, and a necessary
> prerequisite to get the external bus discoverable on the
> MSM8660/APQ8060 machines.
Is it the kind of option that should really be selected through
Kconfig instead for those platforms?
I.e. is it ever really likely that we'll want it off?
-Olof
^ permalink raw reply
* [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling
From: Bartlomiej Zolnierkiewicz @ 2017-01-10 18:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CANAwSgQ4Nxyq5itu62qdz18d4xvTu-e3P3ZXK2acLWtzexaaCA@mail.gmail.com>
Hi,
On Tuesday, January 10, 2017 11:23:38 PM Anand Moon wrote:
> Hi Shuah,
>
> On 10 January 2017 at 21:58, Shuah Khan <shuahkh@osg.samsung.com> wrote:
> > On 01/10/2017 09:05 AM, Bartlomiej Zolnierkiewicz wrote:
> >>
> >> Hi,
> >>
> >> On Tuesday, January 10, 2017 07:36:35 AM Shuah Khan wrote:
> >>> On 01/10/2017 07:16 AM, Shuah Khan wrote:
> >>>> On 01/10/2017 05:05 AM, Bartlomiej Zolnierkiewicz wrote:
> >>>>>
> >>>>> Hi,
> >>>>>
> >>>>> On Monday, January 09, 2017 07:21:31 PM Shuah Khan wrote:
> >>>>>> Fix dwc3_exynos_probe() to call clk_prepare_enable() only when suspend
> >>>>>> clock is specified. Call clk_disable_unprepare() from remove and probe
> >>>>>> error path only when susp_clk has been set from remove and probe error
> >>>>>> paths.
> >>>>>
> >>>>> It is legal to call clk_prepare_enable() and clk_disable_unprepare()
> >>>>> for NULL clock. Also your patch changes susp_clk handling while
> >>>>> leaves axius_clk handling (which also can be NULL) untouched.
> >>>>>
> >>>>> Do you actually see some runtime problem with the current code?
> >>>>>
> >>>>> If not then the patch should probably be dropped.
> >>>>>
> >>>>> Best regards,
> >>>>> --
> >>>>> Bartlomiej Zolnierkiewicz
> >>>>> Samsung R&D Institute Poland
> >>>>> Samsung Electronics
> >>>>
> >>>> Hi Bartlomiej,
> >>>>
> >>>> I am seeing the "no suspend clk specified" message in dmesg.
> >>>> After that it sets the exynos->susp_clk = NULL and starts
> >>>> calling clk_prepare_enable(exynos->susp_clk);
> >>>>
> >>>> That can't be good. If you see the logic right above this
> >>>> one for exynos->clk, it returns error and fails the probe.
> >>>> This this case it doesn't, but tries to use null susp_clk.
> >>
> >> exynos->susp_clk is optional, exynos->clk is not.
> >
> > Right. That is clear since we don't fail the probe.
> >
> >>
> >>>> I believe this patch is necessary.
> >>>
> >>> Let me clarify this a bit further. Since we already know
> >>> susp_clk is null, with this patch we can avoid extra calls
> >>> to clk_prepare_enable() and clk_disable_unprepare().
> >>>
> >>> One can say, it also adds extra checks, hence I will let you
> >>> decide one way or the other. :)
> >>
> >> I would prefer to leave the things as they are currently.
> >>
> >> The code in question is not performance sensitive so extra
> >> calls are not a problem. No extra checks means less code.
> >>
> >> Also the current code seems to be more in line with the rest
> >> of the kernel.
> >
> > What functionality is missing without the suspend clock? Would
> > it make sense to change the info. message to include what it
> > means. At the moment it doesn't anything more than "no suspend
> > clock" which is a very cryptic user visible message. It would be
> > helpful for it to also include what functionality is impacted.
> >
>
> Both usbdrd30_susp_clk and usbdrd30_axius_clk are used by exynos5433 platform
Can you point me to the use of usbdrd30_axius_clk?
I cannot find in the upstream code.
> so moving the clk under compatible string "samsung,exynos7-dwusb3" make sense.
This is not so simple and we would probably need a new compatible for
Exynos5433 (it is currently using "samsung,exynos5250-dwusb3" one and
is not using axius_clk).
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
> Best Regards
> -Anand
>
> > thanks,
> > -- Shuah
> >
> >>
> >> Best regards,
> >> --
> >> Bartlomiej Zolnierkiewicz
> >> Samsung R&D Institute Poland
> >> Samsung Electronics
> >>
> >>> thanks,
> >>> -- Shuah
> >>>
> >>>>
> >>>> thanks,
> >>>> -- Shuah
> >>>>
> >>>>>
> >>>>>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> >>>>>> ---
> >>>>>> drivers/usb/dwc3/dwc3-exynos.c | 10 ++++++----
> >>>>>> 1 file changed, 6 insertions(+), 4 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
> >>>>>> index e27899b..f97a3d7 100644
> >>>>>> --- a/drivers/usb/dwc3/dwc3-exynos.c
> >>>>>> +++ b/drivers/usb/dwc3/dwc3-exynos.c
> >>>>>> @@ -131,8 +131,8 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
> >>>>>> if (IS_ERR(exynos->susp_clk)) {
> >>>>>> dev_info(dev, "no suspend clk specified\n");
> >>>>>> exynos->susp_clk = NULL;
> >>>>>> - }
> >>>>>> - clk_prepare_enable(exynos->susp_clk);
> >>>>>> + } else
> >>>>>> + clk_prepare_enable(exynos->susp_clk);
> >>>>>>
> >>>>>> if (of_device_is_compatible(node, "samsung,exynos7-dwusb3")) {
> >>>>>> exynos->axius_clk = devm_clk_get(dev, "usbdrd30_axius_clk");
> >>>>>> @@ -196,7 +196,8 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
> >>>>>> regulator_disable(exynos->vdd33);
> >>>>>> err2:
> >>>>>> clk_disable_unprepare(exynos->axius_clk);
> >>>>>> - clk_disable_unprepare(exynos->susp_clk);
> >>>>>> + if (exynos->susp_clk)
> >>>>>> + clk_disable_unprepare(exynos->susp_clk);
> >>>>>> clk_disable_unprepare(exynos->clk);
> >>>>>> return ret;
> >>>>>> }
> >>>>>> @@ -210,7 +211,8 @@ static int dwc3_exynos_remove(struct platform_device *pdev)
> >>>>>> platform_device_unregister(exynos->usb3_phy);
> >>>>>>
> >>>>>> clk_disable_unprepare(exynos->axius_clk);
> >>>>>> - clk_disable_unprepare(exynos->susp_clk);
> >>>>>> + if (exynos->susp_clk)
> >>>>>> + clk_disable_unprepare(exynos->susp_clk);
> >>>>>> clk_disable_unprepare(exynos->clk);
> >>>>>>
> >>>>>> regulator_disable(exynos->vdd33);
^ permalink raw reply
* [PATCH 1/5] arm: sunxi: add support for V3s SoC
From: Maxime Ripard @ 2017-01-10 18:09 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170103151629.19447-2-icenowy@aosc.xyz>
On Tue, Jan 03, 2017 at 11:16:25PM +0800, Icenowy Zheng wrote:
> Allwinner V3s is a low-end single-core Cortex-A7 SoC, with 64MB
> integrated DRAM, and several peripherals.
>
> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
> ---
> Documentation/arm/sunxi/README | 4 ++++
> arch/arm/mach-sunxi/sunxi.c | 1 +
> 2 files changed, 5 insertions(+)
>
> diff --git a/Documentation/arm/sunxi/README b/Documentation/arm/sunxi/README
> index cd0243302bc1..91ec8f2055be 100644
> --- a/Documentation/arm/sunxi/README
> +++ b/Documentation/arm/sunxi/README
> @@ -67,6 +67,10 @@ SunXi family
> + Datasheet
> http://dl.linux-sunxi.org/H3/Allwinner_H3_Datasheet_V1.0.pdf
>
> + - Allwinner V3s (sun8i)
> + + Datasheet
> + https://www.goprawn.com/forum/allwinner-cams/783-allwinner-v3s-soc-datasheet
> +
Please don't put random links in there, but at least something that we
know will be there in a couple of weeks/monthes/years
Thanks,
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170110/74d107b3/attachment.sig>
^ permalink raw reply
* [PATCH 2/5] clk: sunxi-ng: add support for V3s CCU
From: Maxime Ripard @ 2017-01-10 18:10 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170103151629.19447-3-icenowy@aosc.xyz>
On Tue, Jan 03, 2017 at 11:16:26PM +0800, Icenowy Zheng wrote:
> V3s has a similar but cut-down CCU to H3.
>
> Add support for it.
>
> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
It looks like there's nothing different but the clocks that you
register with the H3, please just use the H3 driver.
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170110/8a227678/attachment-0001.sig>
^ permalink raw reply
* [PATCH] pwm: constify pwm_ops structures
From: Bhumika Goyal @ 2017-01-10 18:12 UTC (permalink / raw)
To: linux-arm-kernel
Declare pwm_ops structures as const as they are only stored in the ops
field of a pwm_chip structure. This field is of type const struct pwm_ops
*, so pwm_ops structures having this property can be declared as const.
Done using Coccinelle:
@r1 disable optional_qualifier@
identifier i;
position p;
@@
static struct pwm_ops i at p={...};
@ok1@
identifier r1.i;
position p;
struct pxa_pwm_chip pwm;
struct bfin_pwm_chip bwm;
struct vt8500_chip vp;
struct imx_chip icp;
@@
(
pwm.chip.ops=&i at p
|
bwm.chip.ops=&i at p
|
vp.chip.ops=&i at p
|
icp.chip.ops=&i at p
)
@bad@
position p!={r1.p,ok1.p};
identifier r1.i;
@@
i at p
@depends on !bad disable optional_qualifier@
identifier r1.i;
@@
+const
struct pwm_ops i;
File size details:
text data bss dec hex filename
1646 328 0 1974 7b6 drivers/pwm/pwm-imx.o
1742 224 0 1966 7ae drivers/pwm/pwm-imx.o
1941 296 0 2237 8bd drivers/pwm/pwm-pxa.o
2037 192 0 2229 8b5 drivers/pwm/pwm-pxa.o
1946 296 0 2242 8c2 drivers/pwm/pwm-vt8500.o
2050 192 0 2242 8c2 drivers/pwm/pwm-vt8500.o
The drivers/pwm/pwm-bfin.o file did not compile.
Signed-off-by: Bhumika Goyal <bhumirks@gmail.com>
---
File: drivers/pwm/pwm-bfin.c is not tested
drivers/pwm/pwm-bfin.c | 2 +-
drivers/pwm/pwm-imx.c | 2 +-
drivers/pwm/pwm-pxa.c | 2 +-
drivers/pwm/pwm-vt8500.c | 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/pwm/pwm-bfin.c b/drivers/pwm/pwm-bfin.c
index 7631ef1..d2ed0a2 100644
--- a/drivers/pwm/pwm-bfin.c
+++ b/drivers/pwm/pwm-bfin.c
@@ -103,7 +103,7 @@ static void bfin_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
disable_gptimer(priv->pin);
}
-static struct pwm_ops bfin_pwm_ops = {
+static const struct pwm_ops bfin_pwm_ops = {
.request = bfin_pwm_request,
.free = bfin_pwm_free,
.config = bfin_pwm_config,
diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index d600fd5..177fb81 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -240,7 +240,7 @@ static void imx_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
clk_disable_unprepare(imx->clk_per);
}
-static struct pwm_ops imx_pwm_ops = {
+static const struct pwm_ops imx_pwm_ops = {
.enable = imx_pwm_enable,
.disable = imx_pwm_disable,
.config = imx_pwm_config,
diff --git a/drivers/pwm/pwm-pxa.c b/drivers/pwm/pwm-pxa.c
index 58b709f..4143a46 100644
--- a/drivers/pwm/pwm-pxa.c
+++ b/drivers/pwm/pwm-pxa.c
@@ -118,7 +118,7 @@ static void pxa_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
clk_disable_unprepare(pc->clk);
}
-static struct pwm_ops pxa_pwm_ops = {
+static const struct pwm_ops pxa_pwm_ops = {
.config = pxa_pwm_config,
.enable = pxa_pwm_enable,
.disable = pxa_pwm_disable,
diff --git a/drivers/pwm/pwm-vt8500.c b/drivers/pwm/pwm-vt8500.c
index cdb58fd..8141a49 100644
--- a/drivers/pwm/pwm-vt8500.c
+++ b/drivers/pwm/pwm-vt8500.c
@@ -184,7 +184,7 @@ static int vt8500_pwm_set_polarity(struct pwm_chip *chip,
return 0;
}
-static struct pwm_ops vt8500_pwm_ops = {
+static const struct pwm_ops vt8500_pwm_ops = {
.enable = vt8500_pwm_enable,
.disable = vt8500_pwm_disable,
.config = vt8500_pwm_config,
--
1.9.1
^ permalink raw reply related
* next-20170110 build: 1 failures 4 warnings (next-20170110)
From: Mark Brown @ 2017-01-10 18:16 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <E1cQqkO-0005XB-2K@optimist>
On Tue, Jan 10, 2017 at 07:21:32AM +0000, Build bot for Mark Brown wrote:
Today's -next fails to build an arm allmodconfig due to:
> arm-allmodconfig
> ../drivers/net/ethernet/ti/netcp_core.c:1951:28: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types]
caused by 6a8162e99ef344 (net: netcp: store network statistics in 64
bits). It's assigning the function
static struct rtnl_link_stats64 *
netcp_get_stats(struct net_device *ndev, struct rtnl_link_stats64 *stats)
to ndo_get_stats64 which expects a function returning void.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170110/7194228e/attachment.sig>
^ permalink raw reply
* [PATCH v2 4/6] ARM: davinci: add skeleton for pdata-quirks
From: Kevin Hilman @ 2017-01-10 18:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5300e77b-0e76-245d-8f80-689865d471b8@ti.com>
Sekhar Nori <nsekhar@ti.com> writes:
> On Tuesday 10 January 2017 03:56 PM, Sekhar Nori wrote:
>> On Tuesday 10 January 2017 02:25 AM, Kevin Hilman wrote:
>>> Add skeleton pdata-quirks for for davinci.
>>
>> s/for for/for
>>
>>>
>>> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
>>
>> Applied to v4.11/soc
>
> After reviewing 5/6, I think some parts of that patch should actually
> be part of basic pdata-quirks support addition. I have moved them here.
> Here is the updated patch. Let me know if it looks fine.
It looks fine. I hesitated with that part, but I'm OK either way.
Kevin
^ permalink raw reply
* [PATCH 3/5] pinctrl: sunxi: add driver for V3s SoC
From: Maxime Ripard @ 2017-01-10 18:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170103151629.19447-4-icenowy@aosc.xyz>
On Tue, Jan 03, 2017 at 11:16:27PM +0800, Icenowy Zheng wrote:
> V3s SoC features only a pin controller (for the lack of CPUs part).
>
> Add a driver for this controller.
>
> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Thanks,
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170110/fcd3dda2/attachment.sig>
^ permalink raw reply
* [PATCH 4/5] ARM: dts: sunxi: add dtsi file for V3s SoC
From: Maxime Ripard @ 2017-01-10 18:21 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170103151629.19447-5-icenowy@aosc.xyz>
On Tue, Jan 03, 2017 at 11:16:28PM +0800, Icenowy Zheng wrote:
> + uart0_pins_a: uart0 at 0 {
> + pins = "PB8", "PB9";
> + function = "uart0";
> + bias-pull-up;
Why do you need a pullup here?
Looks good otherwise.
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170110/381e01e3/attachment.sig>
^ permalink raw reply
* [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling
From: Bartlomiej Zolnierkiewicz @ 2017-01-10 18:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <2265318.ybtxfiNn3d@amdc3058>
On Tuesday, January 10, 2017 07:03:57 PM Bartlomiej Zolnierkiewicz wrote:
>
> Hi,
>
> On Tuesday, January 10, 2017 11:23:38 PM Anand Moon wrote:
> > Hi Shuah,
> >
> > On 10 January 2017 at 21:58, Shuah Khan <shuahkh@osg.samsung.com> wrote:
> > > On 01/10/2017 09:05 AM, Bartlomiej Zolnierkiewicz wrote:
> > >>
> > >> Hi,
> > >>
> > >> On Tuesday, January 10, 2017 07:36:35 AM Shuah Khan wrote:
> > >>> On 01/10/2017 07:16 AM, Shuah Khan wrote:
> > >>>> On 01/10/2017 05:05 AM, Bartlomiej Zolnierkiewicz wrote:
> > >>>>>
> > >>>>> Hi,
> > >>>>>
> > >>>>> On Monday, January 09, 2017 07:21:31 PM Shuah Khan wrote:
> > >>>>>> Fix dwc3_exynos_probe() to call clk_prepare_enable() only when suspend
> > >>>>>> clock is specified. Call clk_disable_unprepare() from remove and probe
> > >>>>>> error path only when susp_clk has been set from remove and probe error
> > >>>>>> paths.
> > >>>>>
> > >>>>> It is legal to call clk_prepare_enable() and clk_disable_unprepare()
> > >>>>> for NULL clock. Also your patch changes susp_clk handling while
> > >>>>> leaves axius_clk handling (which also can be NULL) untouched.
> > >>>>>
> > >>>>> Do you actually see some runtime problem with the current code?
> > >>>>>
> > >>>>> If not then the patch should probably be dropped.
> > >>>>>
> > >>>>> Best regards,
> > >>>>> --
> > >>>>> Bartlomiej Zolnierkiewicz
> > >>>>> Samsung R&D Institute Poland
> > >>>>> Samsung Electronics
> > >>>>
> > >>>> Hi Bartlomiej,
> > >>>>
> > >>>> I am seeing the "no suspend clk specified" message in dmesg.
> > >>>> After that it sets the exynos->susp_clk = NULL and starts
> > >>>> calling clk_prepare_enable(exynos->susp_clk);
> > >>>>
> > >>>> That can't be good. If you see the logic right above this
> > >>>> one for exynos->clk, it returns error and fails the probe.
> > >>>> This this case it doesn't, but tries to use null susp_clk.
> > >>
> > >> exynos->susp_clk is optional, exynos->clk is not.
> > >
> > > Right. That is clear since we don't fail the probe.
> > >
> > >>
> > >>>> I believe this patch is necessary.
> > >>>
> > >>> Let me clarify this a bit further. Since we already know
> > >>> susp_clk is null, with this patch we can avoid extra calls
> > >>> to clk_prepare_enable() and clk_disable_unprepare().
> > >>>
> > >>> One can say, it also adds extra checks, hence I will let you
> > >>> decide one way or the other. :)
> > >>
> > >> I would prefer to leave the things as they are currently.
> > >>
> > >> The code in question is not performance sensitive so extra
> > >> calls are not a problem. No extra checks means less code.
> > >>
> > >> Also the current code seems to be more in line with the rest
> > >> of the kernel.
> > >
> > > What functionality is missing without the suspend clock? Would
> > > it make sense to change the info. message to include what it
> > > means. At the moment it doesn't anything more than "no suspend
> > > clock" which is a very cryptic user visible message. It would be
> > > helpful for it to also include what functionality is impacted.
> > >
> >
> > Both usbdrd30_susp_clk and usbdrd30_axius_clk are used by exynos5433 platform
>
> Can you point me to the use of usbdrd30_axius_clk?
>
> I cannot find in the upstream code.
>
> > so moving the clk under compatible string "samsung,exynos7-dwusb3" make sense.
>
> This is not so simple and we would probably need a new compatible for
> Exynos5433 (it is currently using "samsung,exynos5250-dwusb3" one and
> is not using axius_clk).
I also think that regardless of what is decided on making susp_clk
non-optional for some Exynos SoCs we should probably remove the debug
message as it doesn't bring useful information and may be confusing.
Shuah, can you take care of this?
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply
* [PATCH 1/2] ARM: dts: imx6: Specify 'anatop-enable-bit' where appropriate
From: Andrey Smirnov @ 2017-01-10 18:24 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAOMZO5CnwrPcney0qzQgex_aFXCrksmEocZ28nf54Kvcm2dq-w@mail.gmail.com>
On Tue, Jan 10, 2017 at 9:28 AM, Fabio Estevam <festevam@gmail.com> wrote:
> On Tue, Jan 10, 2017 at 2:30 PM, Andrey Smirnov
> <andrew.smirnov@gmail.com> wrote:
>> ENABLE_LINREG bit is implemented by 3P0, 1P1 and 2P5 regulators on
>> i.MX6. This property is present in similar code in Fresscale BSP and
>> made its way upstream in imx6ul.dtsi, so this patch adds this property
>> to the rest of i.MX6 family for completness.
>
> Please see:
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/patch/arch/arm/boot/dts/imx6ul.dtsi?id=27958ccdf29e9971732e02494b48be54b0691269
Fabio:
I submitted a patch implementing this property to LKML as well, see
https://www.spinics.net/lists/kernel/msg2418471.html
All of these patches are a part of a broader attempt to add PCIe
support for i.MX7, and on that platform this is a part of a 1P0D
regulator which supplies PCIe PHY.
I can rebase this patch set to take your commit into account, or else
let's discuss the best way to allow setting ENABLE_LINREG.
Thanks,
Andrey Smirnov
^ permalink raw reply
* [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling
From: Krzysztof Kozlowski @ 2017-01-10 18:25 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5156028.4V9iLD3Qkj@amdc3058>
On Tue, Jan 10, 2017 at 06:09:40PM +0100, Bartlomiej Zolnierkiewicz wrote:
> BTW What is interesting is that the Exynos7 dts patch [2] has never
> made it into upstream for some reason. In the meantime however
> Exynos5433 (similar to Exynos7 to some degree) became the user of
> susp_clk.
>
> [1] https://lkml.org/lkml/2014/11/21/247
> [2] https://patchwork.kernel.org/patch/5355161/
>
+Cc Alim and Pankaj,
Anyone would like to resend [2] after rebasing and testing? Interrupt
flags would have to be fixed and status=disabled added.
Best regards,
Krzysztof
^ permalink raw reply
* [PATCH v4 2/2] dt-bindings: clk: add rockchip, grf property for RK3399
From: Doug Anderson @ 2017-01-10 18:34 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1484028930-20305-3-git-send-email-zhengxing@rock-chips.com>
Hi,
On Mon, Jan 9, 2017 at 10:15 PM, Xing Zheng <zhengxing@rock-chips.com> wrote:
> Add support for rockchip,grf property which is used for GRF muxes
> on RK3399.
>
> Signed-off-by: Xing Zheng <zhengxing@rock-chips.com>
> ---
>
> Changes in v4:
> - update the decription for rockchip,grf property
>
> Changes in v3: None
> Changes in v2: None
>
> Documentation/devicetree/bindings/clock/rockchip,rk3399-cru.txt | 6 ++++++
> 1 file changed, 6 insertions(+)
Reviewed-by: Douglas Anderson <dianders@chromium.org>
^ permalink raw reply
* [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling
From: Anand Moon @ 2017-01-10 18:36 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <2265318.ybtxfiNn3d@amdc3058>
Hi Bartlomiej,
On 10 January 2017 at 23:33, Bartlomiej Zolnierkiewicz
<b.zolnierkie@samsung.com> wrote:
>
> Hi,
>
> On Tuesday, January 10, 2017 11:23:38 PM Anand Moon wrote:
>> Hi Shuah,
>>
>> On 10 January 2017 at 21:58, Shuah Khan <shuahkh@osg.samsung.com> wrote:
>> > On 01/10/2017 09:05 AM, Bartlomiej Zolnierkiewicz wrote:
>> >>
>> >> Hi,
>> >>
>> >> On Tuesday, January 10, 2017 07:36:35 AM Shuah Khan wrote:
>> >>> On 01/10/2017 07:16 AM, Shuah Khan wrote:
>> >>>> On 01/10/2017 05:05 AM, Bartlomiej Zolnierkiewicz wrote:
>> >>>>>
>> >>>>> Hi,
>> >>>>>
>> >>>>> On Monday, January 09, 2017 07:21:31 PM Shuah Khan wrote:
>> >>>>>> Fix dwc3_exynos_probe() to call clk_prepare_enable() only when suspend
>> >>>>>> clock is specified. Call clk_disable_unprepare() from remove and probe
>> >>>>>> error path only when susp_clk has been set from remove and probe error
>> >>>>>> paths.
>> >>>>>
>> >>>>> It is legal to call clk_prepare_enable() and clk_disable_unprepare()
>> >>>>> for NULL clock. Also your patch changes susp_clk handling while
>> >>>>> leaves axius_clk handling (which also can be NULL) untouched.
>> >>>>>
>> >>>>> Do you actually see some runtime problem with the current code?
>> >>>>>
>> >>>>> If not then the patch should probably be dropped.
>> >>>>>
>> >>>>> Best regards,
>> >>>>> --
>> >>>>> Bartlomiej Zolnierkiewicz
>> >>>>> Samsung R&D Institute Poland
>> >>>>> Samsung Electronics
>> >>>>
>> >>>> Hi Bartlomiej,
>> >>>>
>> >>>> I am seeing the "no suspend clk specified" message in dmesg.
>> >>>> After that it sets the exynos->susp_clk = NULL and starts
>> >>>> calling clk_prepare_enable(exynos->susp_clk);
>> >>>>
>> >>>> That can't be good. If you see the logic right above this
>> >>>> one for exynos->clk, it returns error and fails the probe.
>> >>>> This this case it doesn't, but tries to use null susp_clk.
>> >>
>> >> exynos->susp_clk is optional, exynos->clk is not.
>> >
>> > Right. That is clear since we don't fail the probe.
>> >
>> >>
>> >>>> I believe this patch is necessary.
>> >>>
>> >>> Let me clarify this a bit further. Since we already know
>> >>> susp_clk is null, with this patch we can avoid extra calls
>> >>> to clk_prepare_enable() and clk_disable_unprepare().
>> >>>
>> >>> One can say, it also adds extra checks, hence I will let you
>> >>> decide one way or the other. :)
>> >>
>> >> I would prefer to leave the things as they are currently.
>> >>
>> >> The code in question is not performance sensitive so extra
>> >> calls are not a problem. No extra checks means less code.
>> >>
>> >> Also the current code seems to be more in line with the rest
>> >> of the kernel.
>> >
>> > What functionality is missing without the suspend clock? Would
>> > it make sense to change the info. message to include what it
>> > means. At the moment it doesn't anything more than "no suspend
>> > clock" which is a very cryptic user visible message. It would be
>> > helpful for it to also include what functionality is impacted.
>> >
>>
>> Both usbdrd30_susp_clk and usbdrd30_axius_clk are used by exynos5433 platform
>
> Can you point me to the use of usbdrd30_axius_clk?
>
> I cannot find in the upstream code.
>
>> so moving the clk under compatible string "samsung,exynos7-dwusb3" make sense.
>
> This is not so simple and we would probably need a new compatible for
> Exynos5433 (it is currently using "samsung,exynos5250-dwusb3" one and
> is not using axius_clk).
Opps: sorry for the noise, my result was based on simple grep.
Best Regards
-Anand
>
> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
>
>> Best Regards
>> -Anand
>>
>> > thanks,
>> > -- Shuah
>> >
>> >>
>> >> Best regards,
>> >> --
>> >> Bartlomiej Zolnierkiewicz
>> >> Samsung R&D Institute Poland
>> >> Samsung Electronics
>> >>
>> >>> thanks,
>> >>> -- Shuah
>> >>>
>> >>>>
>> >>>> thanks,
>> >>>> -- Shuah
>> >>>>
>> >>>>>
>> >>>>>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
>> >>>>>> ---
>> >>>>>> drivers/usb/dwc3/dwc3-exynos.c | 10 ++++++----
>> >>>>>> 1 file changed, 6 insertions(+), 4 deletions(-)
>> >>>>>>
>> >>>>>> diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
>> >>>>>> index e27899b..f97a3d7 100644
>> >>>>>> --- a/drivers/usb/dwc3/dwc3-exynos.c
>> >>>>>> +++ b/drivers/usb/dwc3/dwc3-exynos.c
>> >>>>>> @@ -131,8 +131,8 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
>> >>>>>> if (IS_ERR(exynos->susp_clk)) {
>> >>>>>> dev_info(dev, "no suspend clk specified\n");
>> >>>>>> exynos->susp_clk = NULL;
>> >>>>>> - }
>> >>>>>> - clk_prepare_enable(exynos->susp_clk);
>> >>>>>> + } else
>> >>>>>> + clk_prepare_enable(exynos->susp_clk);
>> >>>>>>
>> >>>>>> if (of_device_is_compatible(node, "samsung,exynos7-dwusb3")) {
>> >>>>>> exynos->axius_clk = devm_clk_get(dev, "usbdrd30_axius_clk");
>> >>>>>> @@ -196,7 +196,8 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
>> >>>>>> regulator_disable(exynos->vdd33);
>> >>>>>> err2:
>> >>>>>> clk_disable_unprepare(exynos->axius_clk);
>> >>>>>> - clk_disable_unprepare(exynos->susp_clk);
>> >>>>>> + if (exynos->susp_clk)
>> >>>>>> + clk_disable_unprepare(exynos->susp_clk);
>> >>>>>> clk_disable_unprepare(exynos->clk);
>> >>>>>> return ret;
>> >>>>>> }
>> >>>>>> @@ -210,7 +211,8 @@ static int dwc3_exynos_remove(struct platform_device *pdev)
>> >>>>>> platform_device_unregister(exynos->usb3_phy);
>> >>>>>>
>> >>>>>> clk_disable_unprepare(exynos->axius_clk);
>> >>>>>> - clk_disable_unprepare(exynos->susp_clk);
>> >>>>>> + if (exynos->susp_clk)
>> >>>>>> + clk_disable_unprepare(exynos->susp_clk);
>> >>>>>> clk_disable_unprepare(exynos->clk);
>> >>>>>>
>> >>>>>> regulator_disable(exynos->vdd33);
>
^ permalink raw reply
* [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling
From: Shuah Khan @ 2017-01-10 18:37 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1659345.4CcVdVckN5@amdc3058>
On 01/10/2017 11:23 AM, Bartlomiej Zolnierkiewicz wrote:
> On Tuesday, January 10, 2017 07:03:57 PM Bartlomiej Zolnierkiewicz wrote:
>>
>> Hi,
>>
>> On Tuesday, January 10, 2017 11:23:38 PM Anand Moon wrote:
>>> Hi Shuah,
>>>
>>> On 10 January 2017 at 21:58, Shuah Khan <shuahkh@osg.samsung.com> wrote:
>>>> On 01/10/2017 09:05 AM, Bartlomiej Zolnierkiewicz wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> On Tuesday, January 10, 2017 07:36:35 AM Shuah Khan wrote:
>>>>>> On 01/10/2017 07:16 AM, Shuah Khan wrote:
>>>>>>> On 01/10/2017 05:05 AM, Bartlomiej Zolnierkiewicz wrote:
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On Monday, January 09, 2017 07:21:31 PM Shuah Khan wrote:
>>>>>>>>> Fix dwc3_exynos_probe() to call clk_prepare_enable() only when suspend
>>>>>>>>> clock is specified. Call clk_disable_unprepare() from remove and probe
>>>>>>>>> error path only when susp_clk has been set from remove and probe error
>>>>>>>>> paths.
>>>>>>>>
>>>>>>>> It is legal to call clk_prepare_enable() and clk_disable_unprepare()
>>>>>>>> for NULL clock. Also your patch changes susp_clk handling while
>>>>>>>> leaves axius_clk handling (which also can be NULL) untouched.
>>>>>>>>
>>>>>>>> Do you actually see some runtime problem with the current code?
>>>>>>>>
>>>>>>>> If not then the patch should probably be dropped.
>>>>>>>>
>>>>>>>> Best regards,
>>>>>>>> --
>>>>>>>> Bartlomiej Zolnierkiewicz
>>>>>>>> Samsung R&D Institute Poland
>>>>>>>> Samsung Electronics
>>>>>>>
>>>>>>> Hi Bartlomiej,
>>>>>>>
>>>>>>> I am seeing the "no suspend clk specified" message in dmesg.
>>>>>>> After that it sets the exynos->susp_clk = NULL and starts
>>>>>>> calling clk_prepare_enable(exynos->susp_clk);
>>>>>>>
>>>>>>> That can't be good. If you see the logic right above this
>>>>>>> one for exynos->clk, it returns error and fails the probe.
>>>>>>> This this case it doesn't, but tries to use null susp_clk.
>>>>>
>>>>> exynos->susp_clk is optional, exynos->clk is not.
>>>>
>>>> Right. That is clear since we don't fail the probe.
>>>>
>>>>>
>>>>>>> I believe this patch is necessary.
>>>>>>
>>>>>> Let me clarify this a bit further. Since we already know
>>>>>> susp_clk is null, with this patch we can avoid extra calls
>>>>>> to clk_prepare_enable() and clk_disable_unprepare().
>>>>>>
>>>>>> One can say, it also adds extra checks, hence I will let you
>>>>>> decide one way or the other. :)
>>>>>
>>>>> I would prefer to leave the things as they are currently.
>>>>>
>>>>> The code in question is not performance sensitive so extra
>>>>> calls are not a problem. No extra checks means less code.
>>>>>
>>>>> Also the current code seems to be more in line with the rest
>>>>> of the kernel.
>>>>
>>>> What functionality is missing without the suspend clock? Would
>>>> it make sense to change the info. message to include what it
>>>> means. At the moment it doesn't anything more than "no suspend
>>>> clock" which is a very cryptic user visible message. It would be
>>>> helpful for it to also include what functionality is impacted.
>>>>
>>>
>>> Both usbdrd30_susp_clk and usbdrd30_axius_clk are used by exynos5433 platform
>>
>> Can you point me to the use of usbdrd30_axius_clk?
>>
>> I cannot find in the upstream code.
>>
>>> so moving the clk under compatible string "samsung,exynos7-dwusb3" make sense.
>>
>> This is not so simple and we would probably need a new compatible for
>> Exynos5433 (it is currently using "samsung,exynos5250-dwusb3" one and
>> is not using axius_clk).
>
> I also think that regardless of what is decided on making susp_clk
> non-optional for some Exynos SoCs we should probably remove the debug
> message as it doesn't bring useful information and may be confusing.
>
> Shuah, can you take care of this?
Yes. This message as it reads now is not only confusing, but also can
lead users to think something is wrong.
I can get rid of it or I could change it from info to debug and change
it to read:
"Optional Suspend clock isn't found. Diver operation isn't impacted"
thanks,
-- Shuah
>
> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
>
^ permalink raw reply
* [PATCH v2 0/3] arm64: dts: exynos: Fix DTC warnings for Exynos boards
From: Krzysztof Kozlowski @ 2017-01-10 18:42 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1484069912-6534-1-git-send-email-javier@osg.samsung.com>
On Tue, Jan 10, 2017 at 02:38:29PM -0300, Javier Martinez Canillas wrote:
> Hello Krzysztof,
>
> This trivial series contains fixes for DTC warnings caused by mismatches
> between unit names and reg properties in device tree nodes.
>
> The patches shouldn't cause a functional change but have been just build
> tested. I compared the generated DTB though to make sure that only these
> nodes changed.
>
> Best regards,
> Javier
>
> Changes since v1:
> - Fix subject line since I forgot the "exynos" prefix.
Thanks :)
Best regards,
Krzysztof
^ permalink raw reply
* [PATCH v4 1/2] arm64: dts: rockchip: add "rockchip, grf" property for RK3399 PMUCRU/CRU
From: Doug Anderson @ 2017-01-10 18:45 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1484028930-20305-2-git-send-email-zhengxing@rock-chips.com>
Hi,
On Mon, Jan 9, 2017 at 10:15 PM, Xing Zheng <zhengxing@rock-chips.com> wrote:
> The structure rockchip_clk_provider needs to refer the GRF regmap
> in somewhere, if the CRU node has not "rockchip,grf" property,
> calling syscon_regmap_lookup_by_phandle will return an invalid GRF
> regmap, and the MUXGRF type clock will be not supported.
>
> Therefore, we need to add them.
>
> Signed-off-by: Xing Zheng <zhengxing@rock-chips.com>
> ---
>
> Changes in v4:
> - separte the binding patch
>
> Changes in v3:
> - add optional roperty rockchip,grf in rockchip,rk3399-cru.txt
>
> Changes in v2:
> - referring pmugrf for PMUGRU
> - fix the typo "invaild" in COMMIT message
>
> arch/arm64/boot/dts/rockchip/rk3399.dtsi | 2 ++
> 1 file changed, 2 insertions(+)
This seems fine to me, so:
Reviewed-by: Douglas Anderson <dianders@chromium.org>
...but I will say that before you actually add any real "MUXGRF"
clocks on rk3399 you _might_ need to rework the code to make things
truly "optional". If it turns out that any existing clocks that
already exist today already go through one of these muxes in the GRF
and we've always been assuming one setting of the mux, we'll need to
make sure we keep assuming that setting of the mux even if the "grf"
isn't specified.
As I understand it, your motivation for this patch is to eventually be
able to model the EDP reference clock which can either be xin24 or
"edp osc". Presumably the eDP "reference clock" isn't related to any
of the pre-existing eDP clocks so that one should be safe.
-Doug
^ permalink raw reply
* [PATCH v2 1/3] arm64: dts: exynos: Add missing unit name to Exynos7 SoC node
From: Krzysztof Kozlowski @ 2017-01-10 18:47 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1484069912-6534-2-git-send-email-javier@osg.samsung.com>
On Tue, Jan 10, 2017 at 02:38:30PM -0300, Javier Martinez Canillas wrote:
> This patch fixes the following DTC warning about a mismatch
> between a device node reg property and its unit name:
>
> Node /soc has a reg or ranges property, but no unit name
>
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> ---
>
> arch/arm64/boot/dts/exynos/exynos7.dtsi | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/exynos/exynos7.dtsi b/arch/arm64/boot/dts/exynos/exynos7.dtsi
> index 80aa60e38237..0d2fedc6ac2f 100644
> --- a/arch/arm64/boot/dts/exynos/exynos7.dtsi
> +++ b/arch/arm64/boot/dts/exynos/exynos7.dtsi
> @@ -69,7 +69,7 @@
> method = "smc";
> };
>
> - soc: soc {
> + soc: soc at 0 {
This looks unnatural, like a fix just to silence the DTC. Mostly de do
not enumerate soc node, although there are few exceptions.
I would prefer ignore the warning... however I am happy to hear other opinions.
Best regards,
Krzysztof
> compatible = "simple-bus";
> #address-cells = <1>;
> #size-cells = <1>;
> --
> 2.7.4
>
^ 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