* [PATCH 0/8] DMA: s3c64xx: Conversion to the new channel request API
From: Krzysztof Kozlowski @ 2016-11-10 18:42 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478791076-19528-1-git-send-email-s.nawrocki@samsung.com>
On Thu, Nov 10, 2016 at 04:17:48PM +0100, Sylwester Nawrocki wrote:
> This patch series aims to convert the s3c64xx platform to use
> the new DMA channel request API, i.e. this is only meaningful
> for non-dt systems using s3c64xx SoCs.
>
> Presumably the first 2 or 4 patches in this series could be queued
> for v4.10-rc1 and the remaining patches could be left for subsequent
> release, to avoid non-trivial conflict with patches already applied
> in the ASoC tree.
Sounds good to me. Should I put the ARM/s3c64xx commits on separate
branch in case someone would like to pull a tag with it?
Best regards,
Krzysztof
^ permalink raw reply
* [PATCH V3 1/2] PCI: add CRS support to error handling path
From: Sinan Kaya @ 2016-11-10 18:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1475473021-14251-2-git-send-email-okaya@codeaurora.org>
On 10/3/2016 1:36 AM, Sinan Kaya wrote:
> The PCIE spec allows an endpoint device to extend the initialization time
> beyond 1 second by issuing Configuration Request Retry Status (CRS) for a
> vendor ID read request.
>
> This basically means "I'm busy now, please call me back later".
>
> There are two moving parts to CRS support from the SW perspective. One part
> is to determine if CRS is supported or not. The second part is to set the
> CRS visibility register.
>
> As part of the probe, the Linux kernel sets the above two conditions in
> pci_enable_crs function. The kernel is also honoring the returned CRS in
> pci_bus_read_dev_vendor_id function if supported. The function will poll up
> to specified amount of time while endpoint is returning CRS response.
>
> The PCIe spec also allows CRS to be issued during cold, warm, hot and FLR
> resets.
>
> The hot reset is initiated by starting a secondary bus reset. A bus/device
> restore follows the reset. This patch is adding vendor ID read into dev
> restore function to validate that the device is accessible before writing
> the register contents. If the device issues CRS, the code might poll up
> to 60 seconds.
>
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> ---
> drivers/pci/pci.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index aab9d51..c8749b9 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4020,6 +4020,12 @@ static void pci_dev_save_and_disable(struct pci_dev *dev)
>
> static void pci_dev_restore(struct pci_dev *dev)
> {
> + u32 l;
> +
> + /* see if the device is accessible first */
> + if (!pci_bus_read_dev_vendor_id(dev->bus, dev->devfn, &l, 60 * 1000))
> + return;
> +
> pci_restore_state(dev);
> pci_reset_notify(dev, false);
> }
>
Any feedback on this direction?
--
Sinan Kaya
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 5/5] PCI: handle CRS returned by device after FLR
From: Sinan Kaya @ 2016-11-10 18:38 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1474056395-21843-6-git-send-email-okaya@codeaurora.org>
On 9/16/2016 4:06 PM, Sinan Kaya wrote:
> An endpoint is allowed to issue CRS following an FLR request to indicate
> that it is not ready to accept new requests. Changing the polling mechanism
> in FLR wait function to go read the vendor ID instead of the command/status
> register. A CRS indication will only be given if the address to be read is
> vendor ID.
>
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> ---
> drivers/pci/pci.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index e913467..1def11e 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3729,7 +3729,8 @@ static void pci_flr_wait(struct pci_dev *dev)
>
> do {
> msleep(100);
> - pci_read_config_dword(dev, PCI_COMMAND, &id);
> + pci_bus_read_dev_vendor_id(dev->bus, dev->devfn, &id,
> + 60 * 1000);
We found out during Mellanox CX3 testing that the card returns the vendor
ID earlier during FLR while other cards are OK with this implementaiton.
I think we need both of the reads to support the CRS for the endpoints
and also the command id register read to make sure endpoint init is complete.
> } while (i++ < 10 && id == ~0);
>
> if (id == ~0)
>
--
Sinan Kaya
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] iommu/dma: Implement dma_{map,unmap}_resource()
From: Catalin Marinas @ 2016-11-10 18:37 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1ae5dc0f-2e59-3e10-a61a-1f5452ad560f@arm.com>
On Thu, Nov 10, 2016 at 02:30:04PM +0000, Robin Murphy wrote:
> On 10/11/16 12:24, Joerg Roedel wrote:
> > On Wed, Oct 26, 2016 at 06:43:56PM +0100, Robin Murphy wrote:
> >> With the new dma_{map,unmap}_resource() functions added to the DMA API
> >> for the benefit of cases like slave DMA, add suitable implementations to
> >> the arsenal of our generic layer. Since cache maintenance should not be
> >> a concern, these can both be standalone callback implementations without
> >> the need for arch code wrappers.
> >>
> >> CC: Joerg Roedel <joro@8bytes.org>
> >> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> >> ---
> >>
> >> v2: Use iommu_dma_unmap_page for symmetry, instead of being sneaky.
> >>
> >> drivers/iommu/dma-iommu.c | 13 +++++++++++++
> >> include/linux/dma-iommu.h | 4 ++++
> >> 2 files changed, 17 insertions(+)
> >>
> >> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> >> index c5ab8667e6f2..a2fd90a6a782 100644
> >> --- a/drivers/iommu/dma-iommu.c
> >> +++ b/drivers/iommu/dma-iommu.c
> >> @@ -624,6 +624,19 @@ void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg, int nents,
> >> __iommu_dma_unmap(iommu_get_domain_for_dev(dev), sg_dma_address(sg));
> >> }
> >>
> >> +dma_addr_t iommu_dma_map_resource(struct device *dev, phys_addr_t phys,
> >> + size_t size, enum dma_data_direction dir, unsigned long attrs)
> >> +{
> >> + return iommu_dma_map_page(dev, phys_to_page(phys), offset_in_page(phys),
> >> + size, dma_direction_to_prot(dir, false) | IOMMU_MMIO);
> >> +}
> >
> > Isn't the whole point of map_resource that we can map regions which have
> > no 'struct page' associated? The use phys_to_page() will certainly not
> > do the right thing here.
>
> Perhaps it's a bit cheeky, but the struct page pointer is never
> dereferenced - the only thing iommu_dma_map_page() does with it is
> immediately convert it back again. Is there ever a case where
> page_to_phys(phys_to_page(phys)) != phys ?
Without SPARSEMEM_VMEMMAP or FLATMEM, it wouldn't work. For example,
__page_to_pfn() in the SPARSEMEM case, used by page_to_phys(), even
accesses the page structure (through page_to_section()).
If the page struct is not relevant to the iommu code in question, you
could factor it out into an __iommu_dma_map_pfn(). Or maybe move the
whole logic to iommu_dma_map_resource() and call it from
iommu_dma_map_page() with the page_to_phys(page) argument.
--
Catalin
^ permalink raw reply
* [PATCH] PCI: enable extended tags support for PCIe endpoints
From: Sinan Kaya @ 2016-11-10 18:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1474769434-5756-1-git-send-email-okaya@codeaurora.org>
On 9/24/2016 10:10 PM, Sinan Kaya wrote:
> Each PCIe device can issue up to 32 transactions at a time by default.
> Each transaction is tracked by a tag number on the bus. 32 outstanding
> transactions is not enough for some performance critical applications
> especially when a lot of small sized frames are transmitted.
>
> Extended tags support increases this number to 256. Devices not
> supporting extended tags tie-off this field to 0. According to ECN, it
> is safe to enable this feature for all PCIe endpoints.
>
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> ---
> drivers/pci/probe.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 93f280d..2424f38 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1505,12 +1505,19 @@ static void program_hpp_type2(struct pci_dev *dev, struct hpp_type2 *hpp)
> */
> }
>
> +static int pci_configure_extended_tags(struct pci_dev *dev)
> +{
I should have checked the capability here before trying to enable it.
I'll post a follow up patch on this.
Is there any other feedback?
> + return pcie_capability_set_word(dev, PCI_EXP_DEVCTL,
> + PCI_EXP_DEVCTL_EXT_TAG);
> +}
> +
> static void pci_configure_device(struct pci_dev *dev)
> {
> struct hotplug_params hpp;
> int ret;
>
> pci_configure_mps(dev);
> + pci_configure_extended_tags(dev);
>
> memset(&hpp, 0, sizeof(hpp));
> ret = pci_get_hp_params(dev, &hpp);
>
--
Sinan Kaya
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
* [RESEND PATCH v1 05/11] dt-bindings: perf: hisi: Add Devicetree bindings for Hisilicon SoC PMU
From: Mark Rutland @ 2016-11-10 18:30 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478151727-20250-6-git-send-email-anurup.m@huawei.com>
Hi,
On Thu, Nov 03, 2016 at 01:42:01AM -0400, Anurup M wrote:
> 1) Device tree bindings for Hisilicon SoC PMU.
> 2) Add example for Hisilicon L3 cache, MN and DDRC PMU.
>
> Signed-off-by: Anurup M <anurup.m@huawei.com>
> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
> ---
> .../devicetree/bindings/arm/hisilicon/pmu.txt | 127 +++++++++++++++++++++
> 1 file changed, 127 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/arm/hisilicon/pmu.txt
>
> diff --git a/Documentation/devicetree/bindings/arm/hisilicon/pmu.txt b/Documentation/devicetree/bindings/arm/hisilicon/pmu.txt
> new file mode 100644
> index 0000000..e7b35e0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/hisilicon/pmu.txt
> @@ -0,0 +1,127 @@
> +Hisilicon SoC hip05/06/07 ARMv8 PMU
> +===================================
> +
> +The Hisilicon SoC chips like hip05/06/07 etc. consist of varous independent
> +system device PMU's such as L3 cache (L3C), Miscellaneous Nodes(MN) and DDR
s/PMU's/PMUs/
> +comtroller. These PMU devices are independent and have hardware logic to
s/comtroller/controller/
> +gather statistics and performance information.
> +
> +HiSilicon SoC chip is encapsulated by multiple CPU and IO die's. The CPU die
s/die's/dies/
> +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.
> +e.g. In the case of hip05/06/07, each SCCL has 1 L3 cache and 1 MN PMU device.
> +
> +The Hisilicon SoC PMU DT node bindigs for uncore PMU devices are as below.
s/bindigs/bindings/
> +For PMU devices like L3 cache. MN etc. which are accessed using the djtag,
> +the parent node will be the djtag node of the corresponding CPU die(SCCL).
> +
> +For uncore PMU devices there are some common required properties as detailed
> +below.
> +
> +Required properties:
> + - compatible : This field contain two values. The first value is
> + always "hisilicon" and second value is the Module type as shown
> + in below examples:
Just say:
- Compatible: should contain one of:
> + (a) "hisilicon,hisi-pmu-l3c-v1" for Hisilicon SoC L3C PMU
> + device (Version 1)
> + (b) "hisilicon,hisi-pmu-mn-v1" for Hisilicon SoC MN PMU
> + device (Version 1)
> + (c) "hisilicon,hisi-pmu-ddrc-v1" for Hisilicon SoC DDRC PMU
> + device (Version 1)
> + The hip05/06/07 chips have v1 hardware for L3C, MN and DDRC.
> +
> + - scl-id : The Super Cluster ID. This can be the ID of the CPU die
> + or IO die in the chip.
What's this needed for?
> + - num-events : No of events supported by this PMU device.
> +
> + - num-counters : No of hardware counters available for counting.
This isn't probeable or well-known?
> +
> +L3 cache
> +--------
> +The L3 cache is dedicated for each SCCL and hence there are separate DT nodes
> +for L3 cache for each SCCL. For L3 cache PMU the additional required properties
> +are
> + - counter-reg : Counter register offset.
> +
> + - evtype-reg : Event select register offset.
> +
> + - evctrl-reg : Event counting control(LAUCTRL) register offset.
Surely for a given revision of the chip these offsets are known? i.e.
surely the compatible string implies specific offsets?
> + - event-en : Event enable value.
Huh?
> + - module-id : Module ID to input for djtag. This property is an array of
> + module_id for each L3 cache banks.
> +
> + - num-banks : Number of banks or instances of the device.
What's a bank? Surely they have separate instances of the PMU?
What order are these in?
> + - cfgen-map : Config enable array to select the bank.
Huh?
> +Miscellaneous Node
> +-------------------
> +The MN is dedicated for each SCCL and hence there are separate DT nodes for MN
> +for each SCCL. For MN PMU the additional required properties are
> + - counter-reg : Counter register offset.
> +
> + - evtype-reg : Event select register offset.
> +
> + - evctrl-reg : Event counting control register offset.
Likewise, surely this is well-known for a given revision of the chip?
> +
> + - module-id : Module ID to input for djtag. As MN doesnot have multiple banks
> + this property is a single value.
> +
> + - cfgen-map : Config enable to select the bank. For MN it is a single value
> +
> + - event-en : Event enable value.
Same comments as for the L3 cache nodes
[...]
> +DDR controller
> +--------------
> +Each SCCL in Hip05/06/07 chips have 2 DDR channels and hence 2 DDR controllers.
> +There are separate DT nodes for each DDR channel.
> +For DDRC PMU the additional required properties are
> +
> + - ch-id : DDRC Channel ID.
Why is this necessary?
Thanks,
Mark.
> + - reg : Register base address and range for the DDRC channel.
> +
> +Example:
> + /* DDRC for CPU die scl #2 Channel #1 for hip05 */
> + pmu_sccl0_ddrc1: pmu_ddrc1 at 80358000 {
> + compatible = "hisilicon,hisi-pmu-ddrc-v1";
> + scl-id = <0x02>;
> + ch-id = <0x1>;
> + num-events = <0x0D>;
> + num-counters = <0x04>;
> + reg = <0x80358000 0x10000>; /* TOTEMC DDRC1 */
> + };
> --
> 2.1.4
>
^ permalink raw reply
* [PATCH v6 3/9] drm/hisilicon/hibmc: Add support for frame buffer
From: Sean Paul @ 2016-11-10 18:30 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1477639682-22520-4-git-send-email-zourongrong@gmail.com>
On Fri, Oct 28, 2016 at 3:27 AM, Rongrong Zou <zourongrong@gmail.com> wrote:
> Add support for fbdev and kms fb management.
>
> Signed-off-by: Rongrong Zou <zourongrong@gmail.com>
> ---
> drivers/gpu/drm/hisilicon/hibmc/Makefile | 2 +-
> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 17 ++
> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h | 24 ++
> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c | 255 ++++++++++++++++++++++
> drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c | 66 ++++++
> 5 files changed, 363 insertions(+), 1 deletion(-)
> create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c
>
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile
> index d5c40b8..810a37e 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/Makefile
> +++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile
> @@ -1,5 +1,5 @@
> ccflags-y := -Iinclude/drm
> -hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_power.o hibmc_ttm.o
> +hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_fbdev.o hibmc_drm_power.o hibmc_ttm.o
>
> obj-$(CONFIG_DRM_HISI_HIBMC) +=hibmc-drm.o
> #obj-y += hibmc-drm.o
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> index 81f4301..5ac7a7e 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> @@ -66,11 +66,23 @@ static void hibmc_disable_vblank(struct drm_device *dev, unsigned int pipe)
>
> static int hibmc_pm_suspend(struct device *dev)
> {
> + struct pci_dev *pdev = to_pci_dev(dev);
> + struct drm_device *drm_dev = pci_get_drvdata(pdev);
> + struct hibmc_drm_device *hidev = drm_dev->dev_private;
> +
> + drm_fb_helper_set_suspend_unlocked(&hidev->fbdev->helper, 1);
> +
We have atomic helpers for suspend/resume now. Please use those.
> return 0;
> }
>
> static int hibmc_pm_resume(struct device *dev)
> {
> + struct pci_dev *pdev = to_pci_dev(dev);
> + struct drm_device *drm_dev = pci_get_drvdata(pdev);
> + struct hibmc_drm_device *hidev = drm_dev->dev_private;
> +
> + drm_fb_helper_set_suspend_unlocked(&hidev->fbdev->helper, 0);
> +
> return 0;
> }
>
> @@ -170,6 +182,7 @@ static int hibmc_unload(struct drm_device *dev)
> {
> struct hibmc_drm_device *hidev = dev->dev_private;
>
> + hibmc_fbdev_fini(hidev);
> hibmc_mm_fini(hidev);
> hibmc_hw_fini(hidev);
> dev->dev_private = NULL;
> @@ -195,6 +208,10 @@ static int hibmc_load(struct drm_device *dev, unsigned long flags)
> if (ret)
> goto err;
>
> + ret = hibmc_fbdev_init(hidev);
> + if (ret)
> + goto err;
> +
> return 0;
>
> err:
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> index db8d80e..a40e9a7 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> @@ -20,9 +20,22 @@
> #define HIBMC_DRM_DRV_H
>
> #include <drm/drmP.h>
> +#include <drm/drm_fb_helper.h>
> #include <drm/ttm/ttm_bo_driver.h>
> #include <drm/drm_gem.h>
>
> +struct hibmc_framebuffer {
> + struct drm_framebuffer fb;
> + struct drm_gem_object *obj;
> + bool is_fbdev_fb;
> +};
> +
> +struct hibmc_fbdev {
> + struct drm_fb_helper helper;
> + struct hibmc_framebuffer *fb;
> + int size;
> +};
> +
> struct hibmc_drm_device {
> /* hw */
> void __iomem *mmio;
> @@ -41,9 +54,13 @@ struct hibmc_drm_device {
> bool initialized;
> } ttm;
>
> + /* fbdev */
> + struct hibmc_fbdev *fbdev;
> bool mm_inited;
> };
>
> +#define to_hibmc_framebuffer(x) container_of(x, struct hibmc_framebuffer, fb)
> +
> struct hibmc_bo {
> struct ttm_buffer_object bo;
> struct ttm_placement placement;
> @@ -65,8 +82,15 @@ static inline struct hibmc_bo *gem_to_hibmc_bo(struct drm_gem_object *gem)
>
> #define DRM_FILE_PAGE_OFFSET (0x100000000ULL >> PAGE_SHIFT)
>
> +int hibmc_fbdev_init(struct hibmc_drm_device *hidev);
> +void hibmc_fbdev_fini(struct hibmc_drm_device *hidev);
> +
> int hibmc_gem_create(struct drm_device *dev, u32 size, bool iskernel,
> struct drm_gem_object **obj);
> +struct hibmc_framebuffer *
> +hibmc_framebuffer_init(struct drm_device *dev,
> + const struct drm_mode_fb_cmd2 *mode_cmd,
> + struct drm_gem_object *obj);
>
> int hibmc_mm_init(struct hibmc_drm_device *hibmc);
> void hibmc_mm_fini(struct hibmc_drm_device *hibmc);
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c
> new file mode 100644
> index 0000000..630124b
> --- /dev/null
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c
> @@ -0,0 +1,255 @@
> +/* Hisilicon Hibmc SoC drm driver
> + *
> + * Based on the bochs drm driver.
> + *
> + * Copyright (c) 2016 Huawei Limited.
> + *
> + * Author:
> + * Rongrong Zou <zourongrong@huawei.com>
> + * Rongrong Zou <zourongrong@gmail.com>
> + * Jianhua Li <lijianhua@huawei.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + */
> +
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_crtc_helper.h>
> +#include <drm/drm_fb_helper.h>
> +
> +#include "hibmc_drm_drv.h"
> +
> +/* ---------------------------------------------------------------------- */
Please remove
> +
> +static int hibmcfb_create_object(
> + struct hibmc_drm_device *hidev,
> + const struct drm_mode_fb_cmd2 *mode_cmd,
> + struct drm_gem_object **gobj_p)
> +{
> + struct drm_gem_object *gobj;
> + struct drm_device *dev = hidev->dev;
> + u32 size;
> + int ret = 0;
> +
> + size = mode_cmd->pitches[0] * mode_cmd->height;
> + ret = hibmc_gem_create(dev, size, true, &gobj);
> + if (ret)
> + return ret;
> +
> + *gobj_p = gobj;
> + return ret;
> +}
> +
> +static struct fb_ops hibmc_drm_fb_ops = {
> + .owner = THIS_MODULE,
> + .fb_check_var = drm_fb_helper_check_var,
> + .fb_set_par = drm_fb_helper_set_par,
> + .fb_fillrect = drm_fb_helper_sys_fillrect,
> + .fb_copyarea = drm_fb_helper_sys_copyarea,
> + .fb_imageblit = drm_fb_helper_sys_imageblit,
> + .fb_pan_display = drm_fb_helper_pan_display,
> + .fb_blank = drm_fb_helper_blank,
> + .fb_setcmap = drm_fb_helper_setcmap,
> +};
> +
> +static int hibmc_drm_fb_create(struct drm_fb_helper *helper,
> + struct drm_fb_helper_surface_size *sizes)
> +{
> + struct hibmc_fbdev *hi_fbdev =
> + container_of(helper, struct hibmc_fbdev, helper);
> + struct hibmc_drm_device *hidev =
> + (struct hibmc_drm_device *)helper->dev->dev_private;
> + struct fb_info *info;
> + struct hibmc_framebuffer *hibmc_fb;
> + struct drm_framebuffer *fb;
> + struct drm_mode_fb_cmd2 mode_cmd;
> + struct drm_gem_object *gobj = NULL;
> + int ret = 0;
> + size_t size;
> + unsigned int bytes_per_pixel;
> + struct hibmc_bo *bo = NULL;
> +
> + DRM_DEBUG_DRIVER("surface width(%d), height(%d) and bpp(%d)\n",
> + sizes->surface_width, sizes->surface_height,
> + sizes->surface_bpp);
> + sizes->surface_depth = 32;
> +
> + bytes_per_pixel = DIV_ROUND_UP(sizes->surface_bpp, 8);
> +
> + mode_cmd.width = sizes->surface_width;
> + mode_cmd.height = sizes->surface_height;
> + mode_cmd.pitches[0] = mode_cmd.width * bytes_per_pixel;
> + mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
> + sizes->surface_depth);
> +
> + size = roundup(mode_cmd.pitches[0] * mode_cmd.height, PAGE_SIZE);
It's somewhat curious that you used ALIGN in the previous patch and
roundup here. But anyways, I think PAGE_ALIGN would be the most
appropriate thing to do here.
> +
> + ret = hibmcfb_create_object(hidev, &mode_cmd, &gobj);
> + if (ret) {
> + DRM_ERROR("failed to create fbcon backing object %d\r\n",
ret);
\r, yikes!!!
> + return -ENOMEM;
> + }
> +
> + bo = gem_to_hibmc_bo(gobj);
> +
> + ret = ttm_bo_reserve(&bo->bo, true, false, NULL);
> + if (ret)
Print error here?
How about cleaning up gobj?
> + return ret;
> +
> + ret = hibmc_bo_pin(bo, TTM_PL_FLAG_VRAM, NULL);
> + if (ret) {
> + DRM_ERROR("failed to pin fbcon\n");
Print ret
ttm_bo_unreserve? It seems like you're missing clean-up in all of the
error paths in this function. Can you please make sure everything is
tidied up?
> + return ret;
> + }
> +
> + ret = ttm_bo_kmap(&bo->bo, 0, bo->bo.num_pages, &bo->kmap);
> +
nit: extra space
> + if (ret) {
> + DRM_ERROR("failed to kmap fbcon\n");
Print ret
> + ttm_bo_unreserve(&bo->bo);
> + return ret;
> + }
> +
> + ttm_bo_unreserve(&bo->bo);
Move this between ttm_bo_kmap and if (ret), then remove it from inside
the conditional.
> +
> + info = drm_fb_helper_alloc_fbi(helper);
> + if (IS_ERR(info))
Print error
> + return PTR_ERR(info);
> +
> + info->par = hi_fbdev;
> +
> + hibmc_fb = hibmc_framebuffer_init(hidev->dev, &mode_cmd, gobj);
> + if (IS_ERR(hibmc_fb)) {
> + drm_fb_helper_release_fbi(helper);
> + return PTR_ERR(hibmc_fb);
> + }
> +
> + hi_fbdev->fb = hibmc_fb;
> + hidev->fbdev->size = size;
> + fb = &hibmc_fb->fb;
The fb variable isn't necessary, and really, the hibmc_fb doesn't
really help either, it just masks what's actually happening IMO.
> + if (!fb) {
> + DRM_INFO("fb is NULL\n");
> + return -EINVAL;
> + }
> +
> + hi_fbdev->helper.fb = fb;
> +
> + strcpy(info->fix.id, "hibmcdrmfb");
> +
> + info->flags = FBINFO_DEFAULT;
> + info->fbops = &hibmc_drm_fb_ops;
> +
> + drm_fb_helper_fill_fix(info, fb->pitches[0], fb->depth);
> + drm_fb_helper_fill_var(info, &hidev->fbdev->helper, sizes->fb_width,
> + sizes->fb_height);
> +
> + info->screen_base = bo->kmap.virtual;
> + info->screen_size = size;
> +
> + info->fix.smem_start = bo->bo.mem.bus.offset + bo->bo.mem.bus.base;
> + info->fix.smem_len = size;
> +
> + return 0;
> +}
> +
> +static void hibmc_fbdev_destroy(struct hibmc_fbdev *fbdev)
> +{
> + struct hibmc_framebuffer *gfb = fbdev->fb;
> + struct drm_fb_helper *fbh = &fbdev->helper;
> +
> + DRM_DEBUG_DRIVER("hibmc_fbdev_destroy\n");
Not useful
> +
> + drm_fb_helper_unregister_fbi(fbh);
> + drm_fb_helper_release_fbi(fbh);
> +
> + drm_fb_helper_fini(fbh);
> +
> + if (gfb)
> + drm_framebuffer_unreference(&gfb->fb);
> +
> + kfree(fbdev);
> +}
> +
> +static const struct drm_fb_helper_funcs hibmc_fbdev_helper_funcs = {
> + .fb_probe = hibmc_drm_fb_create,
> +};
> +
> +int hibmc_fbdev_init(struct hibmc_drm_device *hidev)
> +{
> + int ret;
> + struct fb_var_screeninfo *var;
> + struct fb_fix_screeninfo *fix;
> + struct hibmc_fbdev *hifbdev;
> +
> + hifbdev = kzalloc(sizeof(*hifbdev), GFP_KERNEL);
devm_kzalloc?
> + if (!hifbdev)
> + return -ENOMEM;
> +
> + hidev->fbdev = hifbdev;
> + drm_fb_helper_prepare(hidev->dev, &hifbdev->helper,
> + &hibmc_fbdev_helper_funcs);
> +
> + /* Now just one crtc and one channel */
> + ret = drm_fb_helper_init(hidev->dev,
> + &hifbdev->helper, 1, 1);
> +
nit: extra line
> + if (ret)
> + return ret;
> +
> + ret = drm_fb_helper_single_add_all_connectors(&hifbdev->helper);
> + if (ret)
> + goto fini;
> +
> + ret = drm_fb_helper_initial_config(&hifbdev->helper, 16);
> + if (ret)
> + goto fini;
> +
> + var = &hifbdev->helper.fbdev->var;
> + fix = &hifbdev->helper.fbdev->fix;
> +
> + DRM_DEBUG("Member of info->var is :\n"
> + "xres=%d\n"
> + "yres=%d\n"
> + "xres_virtual=%d\n"
> + "yres_virtual=%d\n"
> + "xoffset=%d\n"
> + "yoffset=%d\n"
> + "bits_per_pixel=%d\n"
> + "...\n", var->xres, var->yres, var->xres_virtual,
> + var->yres_virtual, var->xoffset, var->yoffset,
> + var->bits_per_pixel);
> + DRM_DEBUG("Member of info->fix is :\n"
> + "smem_start=%lx\n"
> + "smem_len=%d\n"
> + "type=%d\n"
> + "type_aux=%d\n"
> + "visual=%d\n"
> + "xpanstep=%d\n"
> + "ypanstep=%d\n"
> + "ywrapstep=%d\n"
> + "line_length=%d\n"
> + "accel=%d\n"
> + "capabilities=%d\n"
> + "...\n", fix->smem_start, fix->smem_len, fix->type,
> + fix->type_aux, fix->visual, fix->xpanstep,
> + fix->ypanstep, fix->ywrapstep, fix->line_length,
> + fix->accel, fix->capabilities);
You've been using DRM_DEBUG_DRIVER elsewhere, you should probably use
it here, too.
> +
> + return 0;
> +
> +fini:
> + drm_fb_helper_fini(&hifbdev->helper);
> + return ret;
> +}
> +
> +void hibmc_fbdev_fini(struct hibmc_drm_device *hidev)
> +{
> + if (!hidev->fbdev)
> + return;
> +
> + hibmc_fbdev_destroy(hidev->fbdev);
> + hidev->fbdev = NULL;
> +}
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
> index 0802ebd..9822f62 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
> @@ -488,3 +488,69 @@ int hibmc_dumb_mmap_offset(struct drm_file *file, struct drm_device *dev,
> drm_gem_object_unreference_unlocked(obj);
> return 0;
> }
> +
> +/* ---------------------------------------------------------------------- */
> +
Please remove
> +static void hibmc_user_framebuffer_destroy(struct drm_framebuffer *fb)
> +{
> + struct hibmc_framebuffer *hibmc_fb = to_hibmc_framebuffer(fb);
> +
> + drm_gem_object_unreference_unlocked(hibmc_fb->obj);
> + drm_framebuffer_cleanup(fb);
> + kfree(hibmc_fb);
> +}
> +
> +static const struct drm_framebuffer_funcs hibmc_fb_funcs = {
> + .destroy = hibmc_user_framebuffer_destroy,
> +};
> +
> +struct hibmc_framebuffer *
> +hibmc_framebuffer_init(struct drm_device *dev,
> + const struct drm_mode_fb_cmd2 *mode_cmd,
> + struct drm_gem_object *obj)
> +{
> + struct hibmc_framebuffer *hibmc_fb;
> + int ret;
> +
> + hibmc_fb = kzalloc(sizeof(*hibmc_fb), GFP_KERNEL);
> + if (!hibmc_fb)
Print error
> + return ERR_PTR(-ENOMEM);
> +
> + drm_helper_mode_fill_fb_struct(&hibmc_fb->fb, mode_cmd);
> + hibmc_fb->obj = obj;
> + ret = drm_framebuffer_init(dev, &hibmc_fb->fb, &hibmc_fb_funcs);
> + if (ret) {
> + DRM_ERROR("drm_framebuffer_init failed: %d\n", ret);
> + kfree(hibmc_fb);
> + return ERR_PTR(ret);
> + }
> +
> + return hibmc_fb;
> +}
> +
> +static struct drm_framebuffer *
> +hibmc_user_framebuffer_create(struct drm_device *dev,
> + struct drm_file *filp,
> + const struct drm_mode_fb_cmd2 *mode_cmd)
> +{
> + struct drm_gem_object *obj;
> + struct hibmc_framebuffer *hibmc_fb;
> +
> + DRM_DEBUG_DRIVER("%dx%d, format %c%c%c%c\n",
> + mode_cmd->width, mode_cmd->height,
> + (mode_cmd->pixel_format) & 0xff,
> + (mode_cmd->pixel_format >> 8) & 0xff,
> + (mode_cmd->pixel_format >> 16) & 0xff,
> + (mode_cmd->pixel_format >> 24) & 0xff);
> +
> + obj = drm_gem_object_lookup(filp, mode_cmd->handles[0]);
> + if (!obj)
> + return ERR_PTR(-ENOENT);
> +
> + hibmc_fb = hibmc_framebuffer_init(dev, mode_cmd, obj);
> + if (IS_ERR(hibmc_fb)) {
> + drm_gem_object_unreference_unlocked(obj);
> + return ERR_PTR((long)hibmc_fb);
> + }
> + return &hibmc_fb->fb;
> +}
> --
> 1.9.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH v2 1/3] ARM: dts: imx6qdl-apalis: Do not rely on DDC I2C bus bitbang for HDMI
From: Vladimir Zapolskiy @ 2016-11-10 18:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <57268103e8fabf344723eae99b45ec7d@agner.ch>
Hi Stefan, Philipp,
On 11/09/2016 02:50 AM, Stefan Agner wrote:
> On 2016-11-08 09:33, maitysanchayan at gmail.com wrote:
>> Hello Shawn,
>>
>> On 16-10-22 15:43:04, Vladimir Zapolskiy wrote:
>>> Hi Shawn,
>>>
>>> On 10/22/2016 06:25 AM, Shawn Guo wrote:
>>>> On Mon, Sep 19, 2016 at 10:41:51AM +0530, Sanchayan Maity wrote:
>>>>> Remove the use of DDC I2C bus bitbang to support reading of EDID
>>>>> and rely on support from internal HDMI I2C master controller instead.
>>>>> As a result remove the device tree property ddc-i2c-bus.
>>>>>
>>>>> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
>>>>
>>>> I think that the dw-hdmi i2c support [1] is a prerequisite of this
>>>> patch. I do not see it lands on v4.9-rc1. Or am I missing something?
>>>>
>>>> Shawn
>>>>
>>>> [1] https://patchwork.kernel.org/patch/9296883/
>>>>
>>>
>>> I'm adding Philipp to Cc, since he is the last one who tested the change
>>> and helped me to push the change to the mainline:
>>>
>>> https://lists.freedesktop.org/archives/dri-devel/2016-September/118569.html
>>>
>>> The problem is that there is no official DW HDMI bridge maintainer, may be
>>> you can review the change, and if you find it satisfactory push it through
>>> ARM/iMX tree.
>>
>> Shawn, is it okay if that patch goes through your ARM/iMX tree?
>
> I don't think it makes sense that the DRM bridge changes go through
> Shawn's tree. Dave should merge Philipps pull request...
>
Philipp, do you mind to submit a rebased pull request one more time?
--
With best wishes,
Vladimir
^ permalink raw reply
* [PATCH v2 2/3] irqchip: mtk-cirq: Add mediatek mtk-cirq implement
From: Marc Zyngier @ 2016-11-10 18:24 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478573833.32099.11.camel@mtksdaap41>
On 08/11/16 02:57, Youlin Pei wrote:
> On Fri, 2016-11-04 at 22:21 +0000, Marc Zyngier wrote:
>> On Fri, Nov 04 2016 at 04:42:57 AM, Youlin Pei <youlin.pei@mediatek.com> wrote:
>>> On Tue, 2016-11-01 at 20:49 +0000, Marc Zyngier wrote:
>>>> On Tue, Nov 01 2016 at 11:52:01 AM, Youlin Pei <youlin.pei@mediatek.com> wrote:
>>>>> In Mediatek SOCs, the CIRQ is a low power interrupt controller
>>>>> designed to works outside MCUSYS which comprises with Cortex-Ax
>>>>> cores,CCI and GIC.
>>>>>
>>>>> The CIRQ controller is integrated in between MCUSYS( include
>>>>> Cortex-Ax, CCI and GIC ) and interrupt sources as the second
>>>>> level interrupt controller. The external interrupts which outside
>>>>> MCUSYS will feed through CIRQ then bypass to GIC. CIRQ can monitors
>>>>> all edge trigger interupts. When an edge interrupt is triggered,
>>>>> CIRQ can record the status and generate a pulse signal to GIC when
>>>>> flush command executed.
>>>>>
>>>>> When system enters sleep mode, MCUSYS will be turned off to improve
>>>>> power consumption, also GIC is power down. The edge trigger interrupts
>>>>> will be lost in this scenario without CIRQ.
>>>>>
>>>>> This commit provides the CIRQ irqchip implement.
>>>>>
>>>>> Signed-off-by: Youlin Pei <youlin.pei@mediatek.com>
>>>>> ---
>>>>> drivers/irqchip/Makefile | 2 +-
>>>>> drivers/irqchip/irq-mtk-cirq.c | 262 ++++++++++++++++++++++++++++++++++++++++
>>>>> 2 files changed, 263 insertions(+), 1 deletion(-)
>>>>> create mode 100644 drivers/irqchip/irq-mtk-cirq.c
>>>>>
>>>>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>>>>> index e4dbfc8..8f33580 100644
>>>>> --- a/drivers/irqchip/Makefile
>>>>> +++ b/drivers/irqchip/Makefile
>>>>> @@ -60,7 +60,7 @@ obj-$(CONFIG_BCM7120_L2_IRQ) += irq-bcm7120-l2.o
>>>>> obj-$(CONFIG_BRCMSTB_L2_IRQ) += irq-brcmstb-l2.o
>>>>> obj-$(CONFIG_KEYSTONE_IRQ) += irq-keystone.o
>>>>> obj-$(CONFIG_MIPS_GIC) += irq-mips-gic.o
>>>>> -obj-$(CONFIG_ARCH_MEDIATEK) += irq-mtk-sysirq.o
>>>>> +obj-$(CONFIG_ARCH_MEDIATEK) += irq-mtk-sysirq.o irq-mtk-cirq.o
>>>>> obj-$(CONFIG_ARCH_DIGICOLOR) += irq-digicolor.o
>>>>> obj-$(CONFIG_RENESAS_H8300H_INTC) += irq-renesas-h8300h.o
>>>>> obj-$(CONFIG_RENESAS_H8S_INTC) += irq-renesas-h8s.o
>>>>> diff --git a/drivers/irqchip/irq-mtk-cirq.c b/drivers/irqchip/irq-mtk-cirq.c
>>>>> new file mode 100644
>>>>> index 0000000..fc43ef3
>>>>> --- /dev/null
>>>>> +++ b/drivers/irqchip/irq-mtk-cirq.c
>>>>> @@ -0,0 +1,262 @@
>>>>> +/*
>>>>> + * Copyright (c) 2016 MediaTek Inc.
>>>>> + * Author: Youlin.Pei <youlin.pei@mediatek.com>
>>>>> + *
>>>>> + * This program is free software; you can redistribute it and/or modify
>>>>> + * it under the terms of the GNU General Public License version 2 as
>>>>> + * published by the Free Software Foundation.
>>>>> + *
>>>>> + * This program is distributed in the hope that it will be useful,
>>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>>>> + * GNU General Public License for more details.
>>>>> + */
>>>>> +
>>>>> +#include <linux/irq.h>
>>>>> +#include <linux/irqchip.h>
>>>>> +#include <linux/irqdomain.h>
>>>>> +#include <linux/of.h>
>>>>> +#include <linux/of_irq.h>
>>>>> +#include <linux/of_address.h>
>>>>> +#include <linux/io.h>
>>>>> +#include <linux/slab.h>
>>>>> +#include <linux/syscore_ops.h>
>>>>> +
>>>>> +#define CIRQ_ACK 0x40
>>>>> +#define CIRQ_MASK_SET 0xc0
>>>>> +#define CIRQ_MASK_CLR 0x100
>>>>> +#define CIRQ_SENS_SET 0x180
>>>>> +#define CIRQ_SENS_CLR 0x1c0
>>>>> +#define CIRQ_POL_SET 0x240
>>>>> +#define CIRQ_POL_CLR 0x280
>>>>> +#define CIRQ_CONTROL 0x300
>>>>> +
>>>>> +#define CIRQ_EN 0x1
>>>>> +#define CIRQ_EDGE 0x2
>>>>> +#define CIRQ_FLUSH 0x4
>>>>> +
>>>>> +#define CIRQ_IRQ_NUM 0x200
>>>>> +
>>>>> +struct mtk_cirq_chip_data {
>>>>> + void __iomem *base;
>>>>> + unsigned int ext_irq_start;
>>>>> +};
>>>>> +
>>>>> +static struct mtk_cirq_chip_data *cirq_data;
>>>>
>>>> Are you guaranteed that you'll only ever have a single CIRQ in any
>>>> system?
>>>
>>> In Mediatek's SOC, only hace a single CIRQ.
>>>
>>>>
>>>>> +
>>>>> +static void mtk_cirq_write_mask(struct irq_data *data, unsigned int offset)
>>>>> +{
>>>>> + struct mtk_cirq_chip_data *chip_data = data->chip_data;
>>>>> + unsigned int cirq_num = data->hwirq;
>>>>> + u32 mask = 1 << (cirq_num % 32);
>>>>> +
>>>>> + writel(mask, chip_data->base + offset + (cirq_num / 32) * 4);
>>>>
>>>> Why can't you use the relaxed accessors?
>>>
>>> It seems that i use wrong function, i will change the writel to
>>> writel_relaxed in next ve
>>>
>>>>
>>>>> +}
>>>>> +
>>>>> +static void mtk_cirq_mask(struct irq_data *data)
>>>>> +{
>>>>> + mtk_cirq_write_mask(data, CIRQ_MASK_SET);
>>>>> + irq_chip_mask_parent(data);
>>>>> +}
>>>>> +
>>>>> +static void mtk_cirq_unmask(struct irq_data *data)
>>>>> +{
>>>>> + mtk_cirq_write_mask(data, CIRQ_MASK_CLR);
>>>>> + irq_chip_unmask_parent(data);
>>>>> +}
>>>>> +
>>>>> +static void mtk_cirq_eoi(struct irq_data *data)
>>>>> +{
>>>>> + mtk_cirq_write_mask(data, CIRQ_ACK);
>>>>
>>>> EOI and ACK have very different semantics. What is this write actually
>>>> doing? Also, you're now doing an additional MMIO write on each interrupt
>>>> EOI, doubling its cost. Do you really need to do actually signal the HW
>>>> that we've EOIed an interrupt? I would have hoped that you'd be able to
>>>> put it in "bypass" mode as long as you're not suspending...
>>>>
>>>
>>> When external interrupt happened, CIRQ status register record the status
>>> even CIRQ is not enabled. when execute the flush command, CIRQ will
>>> resend the signals according to the status. So if don't clear the
>>> status, CIRQ will resend the wrong signals. the ACK write operation will
>>> clear the status.
>>
>> But at this time, we haven't suspended yet, and there is nothing to
>> replay. Also, you only enable the edge capture when suspending. So what
>> are you ACKing here? Can't you simply clear everything right when
>> suspending and not do it at all on the fast path?
>
> I had planned to ACK the status in cirq suspend callback, but
> encountered a problem.
> following is a piece of code from
> http://lxr.free-electrons.com/source/kernel/power/suspend.c#L361
>
> arch_suspend_disable_irqs(); ---step1
> BUG_ON(!irqs_disabled());
>
> error = syscore_suspend();
> |
> ---cirq suspend(); ---step2
>
> if ack the status in cirq suspend, the interrupts will be lost which
> happened during step1 to step2.
>
> So would you mind give me some suggestions about this?
> Thanks a lot!
Right. So maybe there is a way:
- On suspend you can iterate over all the cirq interrupts that have been
recorded
- For each of those, you inspect if it is pending at the GIC level
(using the irq_get_irqchip_state helper)
- if not pending, you Ack it, because it cannot be delivered anymore
- If it is pending, then you know it happened between step 1 and step 2.
- You then have the choice of either going into suspend and waking up
immediately, or failing the suspend.
Thoughts?
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* [PATCH 8/8] coresight: Add support for ARM Coresight STM-500
From: Mathieu Poirier @ 2016-11-10 18:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478801934-26904-1-git-send-email-mathieu.poirier@linaro.org>
From: Suzuki K Poulose <suzuki.poulose@arm.com>
Add the PIDs for STM-500 to the known STM devices list.
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Chunyan Zhang <zhang.chunyan@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
drivers/hwtracing/coresight/coresight-stm.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c
index d397849c2c6a..944c17b48d23 100644
--- a/drivers/hwtracing/coresight/coresight-stm.c
+++ b/drivers/hwtracing/coresight/coresight-stm.c
@@ -920,6 +920,11 @@ static struct amba_id stm_ids[] = {
.mask = 0x0003ffff,
.data = "STM32",
},
+ {
+ .id = 0x0003b963,
+ .mask = 0x0003ffff,
+ .data = "STM500",
+ },
{ 0, 0},
};
--
2.7.4
^ permalink raw reply related
* [PATCH 7/8] coresight: tmc: Remove duplicate memset
From: Mathieu Poirier @ 2016-11-10 18:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478801934-26904-1-git-send-email-mathieu.poirier@linaro.org>
From: "Suzuki K. Poulose" <Suzuki.Poulose@arm.com>
The tmc_etr_enable_hw() fills the buffer with 0's before enabling
the hardware. So, we don't need an explicit memset() in
tmc_enable_etr_sink_sysfs() before calling the tmc_etr_enable_hw().
This patch removes the explicit memset from tmc_enable_etr_sink_sysfs.
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
drivers/hwtracing/coresight/coresight-tmc-etr.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index 3b84d0d38c22..5d312699b3b9 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -150,8 +150,6 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev)
drvdata->buf = drvdata->vaddr;
}
- memset(drvdata->vaddr, 0, drvdata->size);
-
drvdata->mode = CS_MODE_SYSFS;
tmc_etr_enable_hw(drvdata);
out:
--
2.7.4
^ permalink raw reply related
* [PATCH 6/8] coresight: tmc: Get rid of mode parameter for helper routines
From: Mathieu Poirier @ 2016-11-10 18:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478801934-26904-1-git-send-email-mathieu.poirier@linaro.org>
From: "Suzuki K. Poulose" <Suzuki.Poulose@arm.com>
Get rid of the superfluous mode parameter and the check for
the mode in tmc_etX_enable_sink_{perf/sysfs}. While at it, also
remove the unnecessary WARN_ON() checks.
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
drivers/hwtracing/coresight/coresight-tmc-etf.c | 18 +++++-------------
drivers/hwtracing/coresight/coresight-tmc-etr.c | 15 ++++-----------
2 files changed, 9 insertions(+), 24 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
index e80a8f4cd12e..1549436e2492 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
@@ -103,7 +103,7 @@ static void tmc_etf_disable_hw(struct tmc_drvdata *drvdata)
CS_LOCK(drvdata->base);
}
-static int tmc_enable_etf_sink_sysfs(struct coresight_device *csdev, u32 mode)
+static int tmc_enable_etf_sink_sysfs(struct coresight_device *csdev)
{
int ret = 0;
bool used = false;
@@ -111,10 +111,6 @@ static int tmc_enable_etf_sink_sysfs(struct coresight_device *csdev, u32 mode)
unsigned long flags;
struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
- /* This shouldn't be happening */
- if (WARN_ON(mode != CS_MODE_SYSFS))
- return -EINVAL;
-
/*
* If we don't have a buffer release the lock and allocate memory.
* Otherwise keep the lock and move along.
@@ -176,16 +172,12 @@ static int tmc_enable_etf_sink_sysfs(struct coresight_device *csdev, u32 mode)
return ret;
}
-static int tmc_enable_etf_sink_perf(struct coresight_device *csdev, u32 mode)
+static int tmc_enable_etf_sink_perf(struct coresight_device *csdev)
{
int ret = 0;
unsigned long flags;
struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
- /* This shouldn't be happening */
- if (WARN_ON(mode != CS_MODE_PERF))
- return -EINVAL;
-
spin_lock_irqsave(&drvdata->spinlock, flags);
if (drvdata->reading) {
ret = -EINVAL;
@@ -202,7 +194,7 @@ static int tmc_enable_etf_sink_perf(struct coresight_device *csdev, u32 mode)
goto out;
}
- drvdata->mode = mode;
+ drvdata->mode = CS_MODE_PERF;
tmc_etb_enable_hw(drvdata);
out:
spin_unlock_irqrestore(&drvdata->spinlock, flags);
@@ -214,9 +206,9 @@ static int tmc_enable_etf_sink(struct coresight_device *csdev, u32 mode)
{
switch (mode) {
case CS_MODE_SYSFS:
- return tmc_enable_etf_sink_sysfs(csdev, mode);
+ return tmc_enable_etf_sink_sysfs(csdev);
case CS_MODE_PERF:
- return tmc_enable_etf_sink_perf(csdev, mode);
+ return tmc_enable_etf_sink_perf(csdev);
}
/* We shouldn't be here */
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index f23ef0c23303..3b84d0d38c22 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -93,7 +93,7 @@ static void tmc_etr_disable_hw(struct tmc_drvdata *drvdata)
CS_LOCK(drvdata->base);
}
-static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev, u32 mode)
+static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev)
{
int ret = 0;
bool used = false;
@@ -102,9 +102,6 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev, u32 mode)
dma_addr_t paddr;
struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
- /* This shouldn't be happening */
- if (WARN_ON(mode != CS_MODE_SYSFS))
- return -EINVAL;
/*
* If we don't have a buffer release the lock and allocate memory.
@@ -170,16 +167,12 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev, u32 mode)
return ret;
}
-static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, u32 mode)
+static int tmc_enable_etr_sink_perf(struct coresight_device *csdev)
{
int ret = 0;
unsigned long flags;
struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
- /* This shouldn't be happening */
- if (WARN_ON(mode != CS_MODE_PERF))
- return -EINVAL;
-
spin_lock_irqsave(&drvdata->spinlock, flags);
if (drvdata->reading) {
ret = -EINVAL;
@@ -208,9 +201,9 @@ static int tmc_enable_etr_sink(struct coresight_device *csdev, u32 mode)
{
switch (mode) {
case CS_MODE_SYSFS:
- return tmc_enable_etr_sink_sysfs(csdev, mode);
+ return tmc_enable_etr_sink_sysfs(csdev);
case CS_MODE_PERF:
- return tmc_enable_etr_sink_perf(csdev, mode);
+ return tmc_enable_etr_sink_perf(csdev);
}
/* We shouldn't be here */
--
2.7.4
^ permalink raw reply related
* [PATCH 5/8] coresight: tmc: Cleanup operation mode handling
From: Mathieu Poirier @ 2016-11-10 18:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478801934-26904-1-git-send-email-mathieu.poirier@linaro.org>
From: "Suzuki K. Poulose" <Suzuki.Poulose@arm.com>
The mode of operation of the TMC tracked in drvdata->mode is defined
as a local_t type. This is always checked and modified under the
drvdata->spinlock and hence we don't need local_t for it and the
unnecessary synchronisation instructions that comes with it. This
change makes the code a bit more cleaner.
Also fixes the order in which we update the drvdata->mode to
CS_MODE_DISABLED. i.e, in tmc_disable_etX_sink we change the
mode to CS_MODE_DISABLED before invoking tmc_disable_etX_hw()
which in turn depends on the mode to decide whether to dump the
trace to a buffer.
Applies on mathieu's coresight/next tree [1]
https://git.linaro.org/kernel/coresight.git next
Reported-by: Venkatesh Vivekanandan <venkatesh.vivekanandan@broadcom.com>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
drivers/hwtracing/coresight/coresight-tmc-etf.c | 32 +++++++++++--------------
drivers/hwtracing/coresight/coresight-tmc-etr.c | 26 +++++++++-----------
drivers/hwtracing/coresight/coresight-tmc.h | 2 +-
3 files changed, 26 insertions(+), 34 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
index d6941ea24d8d..e80a8f4cd12e 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
@@ -70,7 +70,7 @@ static void tmc_etb_disable_hw(struct tmc_drvdata *drvdata)
* When operating in sysFS mode the content of the buffer needs to be
* read before the TMC is disabled.
*/
- if (local_read(&drvdata->mode) == CS_MODE_SYSFS)
+ if (drvdata->mode == CS_MODE_SYSFS)
tmc_etb_dump_hw(drvdata);
tmc_disable_hw(drvdata);
@@ -108,7 +108,6 @@ static int tmc_enable_etf_sink_sysfs(struct coresight_device *csdev, u32 mode)
int ret = 0;
bool used = false;
char *buf = NULL;
- long val;
unsigned long flags;
struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
@@ -138,13 +137,12 @@ static int tmc_enable_etf_sink_sysfs(struct coresight_device *csdev, u32 mode)
goto out;
}
- val = local_xchg(&drvdata->mode, mode);
/*
* In sysFS mode we can have multiple writers per sink. Since this
* sink is already enabled no memory is needed and the HW need not be
* touched.
*/
- if (val == CS_MODE_SYSFS)
+ if (drvdata->mode == CS_MODE_SYSFS)
goto out;
/*
@@ -163,6 +161,7 @@ static int tmc_enable_etf_sink_sysfs(struct coresight_device *csdev, u32 mode)
drvdata->buf = buf;
}
+ drvdata->mode = CS_MODE_SYSFS;
tmc_etb_enable_hw(drvdata);
out:
spin_unlock_irqrestore(&drvdata->spinlock, flags);
@@ -180,7 +179,6 @@ static int tmc_enable_etf_sink_sysfs(struct coresight_device *csdev, u32 mode)
static int tmc_enable_etf_sink_perf(struct coresight_device *csdev, u32 mode)
{
int ret = 0;
- long val;
unsigned long flags;
struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
@@ -194,17 +192,17 @@ static int tmc_enable_etf_sink_perf(struct coresight_device *csdev, u32 mode)
goto out;
}
- val = local_xchg(&drvdata->mode, mode);
/*
* In Perf mode there can be only one writer per sink. There
* is also no need to continue if the ETB/ETR is already operated
* from sysFS.
*/
- if (val != CS_MODE_DISABLED) {
+ if (drvdata->mode != CS_MODE_DISABLED) {
ret = -EINVAL;
goto out;
}
+ drvdata->mode = mode;
tmc_etb_enable_hw(drvdata);
out:
spin_unlock_irqrestore(&drvdata->spinlock, flags);
@@ -227,7 +225,6 @@ static int tmc_enable_etf_sink(struct coresight_device *csdev, u32 mode)
static void tmc_disable_etf_sink(struct coresight_device *csdev)
{
- long val;
unsigned long flags;
struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
@@ -237,10 +234,11 @@ static void tmc_disable_etf_sink(struct coresight_device *csdev)
return;
}
- val = local_xchg(&drvdata->mode, CS_MODE_DISABLED);
/* Disable the TMC only if it needs to */
- if (val != CS_MODE_DISABLED)
+ if (drvdata->mode != CS_MODE_DISABLED) {
tmc_etb_disable_hw(drvdata);
+ drvdata->mode = CS_MODE_DISABLED;
+ }
spin_unlock_irqrestore(&drvdata->spinlock, flags);
@@ -260,7 +258,7 @@ static int tmc_enable_etf_link(struct coresight_device *csdev,
}
tmc_etf_enable_hw(drvdata);
- local_set(&drvdata->mode, CS_MODE_SYSFS);
+ drvdata->mode = CS_MODE_SYSFS;
spin_unlock_irqrestore(&drvdata->spinlock, flags);
dev_info(drvdata->dev, "TMC-ETF enabled\n");
@@ -280,7 +278,7 @@ static void tmc_disable_etf_link(struct coresight_device *csdev,
}
tmc_etf_disable_hw(drvdata);
- local_set(&drvdata->mode, CS_MODE_DISABLED);
+ drvdata->mode = CS_MODE_DISABLED;
spin_unlock_irqrestore(&drvdata->spinlock, flags);
dev_info(drvdata->dev, "TMC disabled\n");
@@ -383,7 +381,7 @@ static void tmc_update_etf_buffer(struct coresight_device *csdev,
return;
/* This shouldn't happen */
- if (WARN_ON_ONCE(local_read(&drvdata->mode) != CS_MODE_PERF))
+ if (WARN_ON_ONCE(drvdata->mode != CS_MODE_PERF))
return;
CS_UNLOCK(drvdata->base);
@@ -504,7 +502,6 @@ const struct coresight_ops tmc_etf_cs_ops = {
int tmc_read_prepare_etb(struct tmc_drvdata *drvdata)
{
- long val;
enum tmc_mode mode;
int ret = 0;
unsigned long flags;
@@ -528,9 +525,8 @@ int tmc_read_prepare_etb(struct tmc_drvdata *drvdata)
goto out;
}
- val = local_read(&drvdata->mode);
/* Don't interfere if operated from Perf */
- if (val == CS_MODE_PERF) {
+ if (drvdata->mode == CS_MODE_PERF) {
ret = -EINVAL;
goto out;
}
@@ -542,7 +538,7 @@ int tmc_read_prepare_etb(struct tmc_drvdata *drvdata)
}
/* Disable the TMC if need be */
- if (val == CS_MODE_SYSFS)
+ if (drvdata->mode == CS_MODE_SYSFS)
tmc_etb_disable_hw(drvdata);
drvdata->reading = true;
@@ -573,7 +569,7 @@ int tmc_read_unprepare_etb(struct tmc_drvdata *drvdata)
}
/* Re-enable the TMC if need be */
- if (local_read(&drvdata->mode) == CS_MODE_SYSFS) {
+ if (drvdata->mode == CS_MODE_SYSFS) {
/*
* The trace run will continue with the same allocated trace
* buffer. As such zero-out the buffer so that we don't end
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index 886ea83c68e0..f23ef0c23303 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -86,7 +86,7 @@ static void tmc_etr_disable_hw(struct tmc_drvdata *drvdata)
* When operating in sysFS mode the content of the buffer needs to be
* read before the TMC is disabled.
*/
- if (local_read(&drvdata->mode) == CS_MODE_SYSFS)
+ if (drvdata->mode == CS_MODE_SYSFS)
tmc_etr_dump_hw(drvdata);
tmc_disable_hw(drvdata);
@@ -97,7 +97,6 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev, u32 mode)
{
int ret = 0;
bool used = false;
- long val;
unsigned long flags;
void __iomem *vaddr = NULL;
dma_addr_t paddr;
@@ -134,13 +133,12 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev, u32 mode)
goto out;
}
- val = local_xchg(&drvdata->mode, mode);
/*
* In sysFS mode we can have multiple writers per sink. Since this
* sink is already enabled no memory is needed and the HW need not be
* touched.
*/
- if (val == CS_MODE_SYSFS)
+ if (drvdata->mode == CS_MODE_SYSFS)
goto out;
/*
@@ -157,6 +155,7 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev, u32 mode)
memset(drvdata->vaddr, 0, drvdata->size);
+ drvdata->mode = CS_MODE_SYSFS;
tmc_etr_enable_hw(drvdata);
out:
spin_unlock_irqrestore(&drvdata->spinlock, flags);
@@ -174,7 +173,6 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev, u32 mode)
static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, u32 mode)
{
int ret = 0;
- long val;
unsigned long flags;
struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
@@ -188,17 +186,17 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, u32 mode)
goto out;
}
- val = local_xchg(&drvdata->mode, mode);
/*
* In Perf mode there can be only one writer per sink. There
* is also no need to continue if the ETR is already operated
* from sysFS.
*/
- if (val != CS_MODE_DISABLED) {
+ if (drvdata->mode != CS_MODE_DISABLED) {
ret = -EINVAL;
goto out;
}
+ drvdata->mode = CS_MODE_PERF;
tmc_etr_enable_hw(drvdata);
out:
spin_unlock_irqrestore(&drvdata->spinlock, flags);
@@ -221,7 +219,6 @@ static int tmc_enable_etr_sink(struct coresight_device *csdev, u32 mode)
static void tmc_disable_etr_sink(struct coresight_device *csdev)
{
- long val;
unsigned long flags;
struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
@@ -231,10 +228,11 @@ static void tmc_disable_etr_sink(struct coresight_device *csdev)
return;
}
- val = local_xchg(&drvdata->mode, CS_MODE_DISABLED);
/* Disable the TMC only if it needs to */
- if (val != CS_MODE_DISABLED)
+ if (drvdata->mode != CS_MODE_DISABLED) {
tmc_etr_disable_hw(drvdata);
+ drvdata->mode = CS_MODE_DISABLED;
+ }
spin_unlock_irqrestore(&drvdata->spinlock, flags);
@@ -253,7 +251,6 @@ const struct coresight_ops tmc_etr_cs_ops = {
int tmc_read_prepare_etr(struct tmc_drvdata *drvdata)
{
int ret = 0;
- long val;
unsigned long flags;
/* config types are set a boot time and never change */
@@ -266,9 +263,8 @@ int tmc_read_prepare_etr(struct tmc_drvdata *drvdata)
goto out;
}
- val = local_read(&drvdata->mode);
/* Don't interfere if operated from Perf */
- if (val == CS_MODE_PERF) {
+ if (drvdata->mode == CS_MODE_PERF) {
ret = -EINVAL;
goto out;
}
@@ -280,7 +276,7 @@ int tmc_read_prepare_etr(struct tmc_drvdata *drvdata)
}
/* Disable the TMC if need be */
- if (val == CS_MODE_SYSFS)
+ if (drvdata->mode == CS_MODE_SYSFS)
tmc_etr_disable_hw(drvdata);
drvdata->reading = true;
@@ -303,7 +299,7 @@ int tmc_read_unprepare_etr(struct tmc_drvdata *drvdata)
spin_lock_irqsave(&drvdata->spinlock, flags);
/* RE-enable the TMC if need be */
- if (local_read(&drvdata->mode) == CS_MODE_SYSFS) {
+ if (drvdata->mode == CS_MODE_SYSFS) {
/*
* The trace run will continue with the same allocated trace
* buffer. The trace buffer is cleared in tmc_etr_enable_hw(),
diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
index 44b3ae346118..51c01851533e 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.h
+++ b/drivers/hwtracing/coresight/coresight-tmc.h
@@ -117,7 +117,7 @@ struct tmc_drvdata {
void __iomem *vaddr;
u32 size;
u32 len;
- local_t mode;
+ u32 mode;
enum tmc_config_type config_type;
enum tmc_mem_intf_width memwidth;
u32 trigger_cntr;
--
2.7.4
^ permalink raw reply related
* [PATCH 4/8] coresight: reset "enable_sink" flag when need be
From: Mathieu Poirier @ 2016-11-10 18:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478801934-26904-1-git-send-email-mathieu.poirier@linaro.org>
When using coresight from the perf interface sinks are specified
as part of the perf command line. As such the sink needs to be
disabled once it has been acknowledged by the coresight framework.
Otherwise the sink stays enabled, which may interfere with other
sessions.
This patch removes the sink selection check from the build path
process and make it a function on it's own. The function is
then used when operating from sysFS or perf to determine what
sink has been selected.
If operated from perf the status of the "enable_sink" flag is
reset so that concurrent session can use a different sink. When
used from sysFS the status of the flag is left untouched since
users have full control.
The implementation doesn't handle a scenario where a sink has
been enabled from sysFS and another sink is selected from the
perf command line as both modes of operation are mutually
exclusive.
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
drivers/hwtracing/coresight/coresight-etm-perf.c | 31 +++++-----
drivers/hwtracing/coresight/coresight-priv.h | 4 +-
drivers/hwtracing/coresight/coresight.c | 74 ++++++++++++++++++++++--
3 files changed, 87 insertions(+), 22 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index 2cd7c718198a..5a346fc8ce06 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -202,6 +202,21 @@ static void *etm_setup_aux(int event_cpu, void **pages,
if (!event_data)
return NULL;
+ /*
+ * In theory nothing prevent tracers in a trace session from being
+ * associated with different sinks, nor having a sink per tracer. But
+ * until we have HW with this kind of topology we need to assume tracers
+ * in a trace session are using the same sink. Therefore go through
+ * the coresight bus and pick the first enabled sink.
+ *
+ * When operated from sysFS users are responsible to enable the sink
+ * while from perf, the perf tools will do it based on the choice made
+ * on the cmd line. As such the "enable_sink" flag in sysFS is reset.
+ */
+ sink = coresight_get_enabled_sink(true);
+ if (!sink)
+ return NULL;
+
INIT_WORK(&event_data->work, free_event_data);
mask = &event_data->mask;
@@ -219,25 +234,11 @@ static void *etm_setup_aux(int event_cpu, void **pages,
* list of devices from source to sink that can be
* referenced later when the path is actually needed.
*/
- event_data->path[cpu] = coresight_build_path(csdev);
+ event_data->path[cpu] = coresight_build_path(csdev, sink);
if (IS_ERR(event_data->path[cpu]))
goto err;
}
- /*
- * In theory nothing prevent tracers in a trace session from being
- * associated with different sinks, nor having a sink per tracer. But
- * until we have HW with this kind of topology and a way to convey
- * sink assignement from the perf cmd line we need to assume tracers
- * in a trace session are using the same sink. Therefore pick the sink
- * found at the end of the first available path.
- */
- cpu = cpumask_first(mask);
- /* Grab the sink at the end of the path */
- sink = coresight_get_sink(event_data->path[cpu]);
- if (!sink)
- goto err;
-
if (!sink_ops(sink)->alloc_buffer)
goto err;
diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
index 196a14be4b3d..ef9d8e93e3b2 100644
--- a/drivers/hwtracing/coresight/coresight-priv.h
+++ b/drivers/hwtracing/coresight/coresight-priv.h
@@ -111,7 +111,9 @@ static inline void CS_UNLOCK(void __iomem *addr)
void coresight_disable_path(struct list_head *path);
int coresight_enable_path(struct list_head *path, u32 mode);
struct coresight_device *coresight_get_sink(struct list_head *path);
-struct list_head *coresight_build_path(struct coresight_device *csdev);
+struct coresight_device *coresight_get_enabled_sink(bool reset);
+struct list_head *coresight_build_path(struct coresight_device *csdev,
+ struct coresight_device *sink);
void coresight_release_path(struct list_head *path);
#ifdef CONFIG_CORESIGHT_SOURCE_ETM3X
diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
index 7bf00a0beb6f..0c37356e417c 100644
--- a/drivers/hwtracing/coresight/coresight.c
+++ b/drivers/hwtracing/coresight/coresight.c
@@ -368,6 +368,52 @@ struct coresight_device *coresight_get_sink(struct list_head *path)
return csdev;
}
+static int coresight_enabled_sink(struct device *dev, void *data)
+{
+ bool *reset = data;
+ struct coresight_device *csdev = to_coresight_device(dev);
+
+ if ((csdev->type == CORESIGHT_DEV_TYPE_SINK ||
+ csdev->type == CORESIGHT_DEV_TYPE_LINKSINK) &&
+ csdev->activated) {
+ /*
+ * Now that we have a handle on the sink for this session,
+ * disable the sysFS "enable_sink" flag so that possible
+ * concurrent perf session that wish to use another sink don't
+ * trip on it. Doing so has no ramification for the current
+ * session.
+ */
+ if (*reset)
+ csdev->activated = false;
+
+ return 1;
+ }
+
+ return 0;
+}
+
+/**
+ * coresight_get_enabled_sink - returns the first enabled sink found on the bus
+ * @deactivate: Whether the 'enable_sink' flag should be reset
+ *
+ * When operated from perf the deactivate parameter should be set to 'true'.
+ * That way the "enabled_sink" flag of the sink that was selected can be reset,
+ * allowing for other concurrent perf sessions to choose a different sink.
+ *
+ * When operated from sysFS users have full control and as such the deactivate
+ * parameter should be set to 'false', hence mandating users to explicitly
+ * clear the flag.
+ */
+struct coresight_device *coresight_get_enabled_sink(bool deactivate)
+{
+ struct device *dev = NULL;
+
+ dev = bus_find_device(&coresight_bustype, NULL, &deactivate,
+ coresight_enabled_sink);
+
+ return dev ? to_coresight_device(dev) : NULL;
+}
+
/**
* _coresight_build_path - recursively build a path from a @csdev to a sink.
* @csdev: The device to start from.
@@ -380,6 +426,7 @@ struct coresight_device *coresight_get_sink(struct list_head *path)
* last one.
*/
static int _coresight_build_path(struct coresight_device *csdev,
+ struct coresight_device *sink,
struct list_head *path)
{
int i;
@@ -387,15 +434,15 @@ static int _coresight_build_path(struct coresight_device *csdev,
struct coresight_node *node;
/* An activated sink has been found. Enqueue the element */
- if ((csdev->type == CORESIGHT_DEV_TYPE_SINK ||
- csdev->type == CORESIGHT_DEV_TYPE_LINKSINK) && csdev->activated)
+ if (csdev == sink)
goto out;
/* Not a sink - recursively explore each port found on this element */
for (i = 0; i < csdev->nr_outport; i++) {
struct coresight_device *child_dev = csdev->conns[i].child_dev;
- if (child_dev && _coresight_build_path(child_dev, path) == 0) {
+ if (child_dev &&
+ _coresight_build_path(child_dev, sink, path) == 0) {
found = true;
break;
}
@@ -422,18 +469,22 @@ static int _coresight_build_path(struct coresight_device *csdev,
return 0;
}
-struct list_head *coresight_build_path(struct coresight_device *csdev)
+struct list_head *coresight_build_path(struct coresight_device *source,
+ struct coresight_device *sink)
{
struct list_head *path;
int rc;
+ if (!sink)
+ return ERR_PTR(-EINVAL);
+
path = kzalloc(sizeof(struct list_head), GFP_KERNEL);
if (!path)
return ERR_PTR(-ENOMEM);
INIT_LIST_HEAD(path);
- rc = _coresight_build_path(csdev, path);
+ rc = _coresight_build_path(source, sink, path);
if (rc) {
kfree(path);
return ERR_PTR(rc);
@@ -497,6 +548,7 @@ static int coresight_validate_source(struct coresight_device *csdev,
int coresight_enable(struct coresight_device *csdev)
{
int cpu, ret = 0;
+ struct coresight_device *sink;
struct list_head *path;
mutex_lock(&coresight_mutex);
@@ -508,7 +560,17 @@ int coresight_enable(struct coresight_device *csdev)
if (csdev->enable)
goto out;
- path = coresight_build_path(csdev);
+ /*
+ * Search for a valid sink for this session but don't reset the
+ * "enable_sink" flag in sysFS. Users get to do that explicitly.
+ */
+ sink = coresight_get_enabled_sink(false);
+ if (!sink) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ path = coresight_build_path(csdev, sink);
if (IS_ERR(path)) {
pr_err("building path(s) failed\n");
ret = PTR_ERR(path);
--
2.7.4
^ permalink raw reply related
* [PATCH 3/8] coresight: etm3x: Adding missing features of Coresight PTM components
From: Mathieu Poirier @ 2016-11-10 18:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478801934-26904-1-git-send-email-mathieu.poirier@linaro.org>
From: Muhammad Abdul WAHAB <muhammadabdul.wahab@centralesupelec.fr>
In the current driver for Coresight components, two features of PTM
components are missing:
1. Branch Broadcasting (present also in ETM but called Branch Output)
2. Return Stack (only present in PTM v1.0 and PTMv1.1)
These features can be added simply to the code using `mode` field of
`etm_config` struct.
1. **Branch Broadcast** : The branch broadcast feature is present in ETM
components as well and is called Branch output. It allows to retrieve
addresses for direct branch addresses alongside the indirect branch
addresses. For example, it could be useful in cases when tracing without
source code.
2. **Return Stack** : The return stack option allows to retrieve the
return addresses of function calls. It can be useful to avoid CRA
(Code Reuse Attacks) by keeping a shadowstack.
Signed-off-by: Muhammad Abdul Wahab <muhammadabdul.wahab@centralesupelec.fr>
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
drivers/hwtracing/coresight/coresight-etm.h | 5 +++++
drivers/hwtracing/coresight/coresight-etm3x-sysfs.c | 10 ++++++++++
2 files changed, 15 insertions(+)
diff --git a/drivers/hwtracing/coresight/coresight-etm.h b/drivers/hwtracing/coresight/coresight-etm.h
index 4a18ee499965..ad063d7444e1 100644
--- a/drivers/hwtracing/coresight/coresight-etm.h
+++ b/drivers/hwtracing/coresight/coresight-etm.h
@@ -89,11 +89,13 @@
/* ETMCR - 0x00 */
#define ETMCR_PWD_DWN BIT(0)
#define ETMCR_STALL_MODE BIT(7)
+#define ETMCR_BRANCH_BROADCAST BIT(8)
#define ETMCR_ETM_PRG BIT(10)
#define ETMCR_ETM_EN BIT(11)
#define ETMCR_CYC_ACC BIT(12)
#define ETMCR_CTXID_SIZE (BIT(14)|BIT(15))
#define ETMCR_TIMESTAMP_EN BIT(28)
+#define ETMCR_RETURN_STACK BIT(29)
/* ETMCCR - 0x04 */
#define ETMCCR_FIFOFULL BIT(23)
/* ETMPDCR - 0x310 */
@@ -110,8 +112,11 @@
#define ETM_MODE_STALL BIT(2)
#define ETM_MODE_TIMESTAMP BIT(3)
#define ETM_MODE_CTXID BIT(4)
+#define ETM_MODE_BBROAD BIT(5)
+#define ETM_MODE_RET_STACK BIT(6)
#define ETM_MODE_ALL (ETM_MODE_EXCLUDE | ETM_MODE_CYCACC | \
ETM_MODE_STALL | ETM_MODE_TIMESTAMP | \
+ ETM_MODE_BBROAD | ETM_MODE_RET_STACK | \
ETM_MODE_CTXID | ETM_MODE_EXCL_KERN | \
ETM_MODE_EXCL_USER)
diff --git a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
index 5ea090955c08..ca98ad13bb8c 100644
--- a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
+++ b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
@@ -164,6 +164,16 @@ static ssize_t mode_store(struct device *dev,
else
config->ctrl &= ~ETMCR_CTXID_SIZE;
+ if (config->mode & ETM_MODE_BBROAD)
+ config->ctrl |= ETMCR_BRANCH_BROADCAST;
+ else
+ config->ctrl &= ~ETMCR_BRANCH_BROADCAST;
+
+ if (config->mode & ETM_MODE_RET_STACK)
+ config->ctrl |= ETMCR_RETURN_STACK;
+ else
+ config->ctrl &= ~ETMCR_RETURN_STACK;
+
if (config->mode & (ETM_MODE_EXCL_KERN | ETM_MODE_EXCL_USER))
etm_config_trace_mode(config);
--
2.7.4
^ permalink raw reply related
* [PATCH 2/8] coresight: etm3x: indentation fix (extra space removed)
From: Mathieu Poirier @ 2016-11-10 18:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478801934-26904-1-git-send-email-mathieu.poirier@linaro.org>
From: Muhammad Abdul WAHAB <muhammadabdul.wahab@centralesupelec.fr>
An extra space is removed.
Signed-off-by: Muhammad Abdul Wahab <muhammadabdul.wahab@centralesupelec.fr>
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
drivers/hwtracing/coresight/coresight-etm3x-sysfs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
index e9b071953f80..5ea090955c08 100644
--- a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
+++ b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
@@ -146,7 +146,7 @@ static ssize_t mode_store(struct device *dev,
goto err_unlock;
}
config->ctrl |= ETMCR_STALL_MODE;
- } else
+ } else
config->ctrl &= ~ETMCR_STALL_MODE;
if (config->mode & ETM_MODE_TIMESTAMP) {
--
2.7.4
^ permalink raw reply related
* [PATCH 1/8] coresight: stm: return error code instead of zero in .packet()
From: Mathieu Poirier @ 2016-11-10 18:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478801934-26904-1-git-send-email-mathieu.poirier@linaro.org>
From: Chunyan Zhang <zhang.chunyan@linaro.org>
In STM framework driver, the trace data writing loop would keep running
until it received a negative return value or the whole trace packet has
been written to STM device. So if the .packet() of STM device always
returns zero since the device is not enabled or the parameter isn't
supported, STM framework driver will stall into a dead loop.
Returning -EACCES (Permission denied) in .packet() if the device is
disabled makes more sense, and this is the same for returning -EINVAL
if the channel passed into is not supported.
Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
drivers/hwtracing/coresight/coresight-stm.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c
index 49e0f1b925a5..d397849c2c6a 100644
--- a/drivers/hwtracing/coresight/coresight-stm.c
+++ b/drivers/hwtracing/coresight/coresight-stm.c
@@ -419,10 +419,10 @@ static ssize_t stm_generic_packet(struct stm_data *stm_data,
struct stm_drvdata, stm);
if (!(drvdata && local_read(&drvdata->mode)))
- return 0;
+ return -EACCES;
if (channel >= drvdata->numsp)
- return 0;
+ return -EINVAL;
ch_addr = (unsigned long)stm_channel_addr(drvdata, channel);
--
2.7.4
^ permalink raw reply related
* [PATCH 0/8] coresight: next v4.9-rc4
From: Mathieu Poirier @ 2016-11-10 18:18 UTC (permalink / raw)
To: linux-arm-kernel
Good morning Greg,
Please consider the following set for inclusion in the 4.10 cycle. It
applies cleanly on today 'char-misc-next' branch (3372592a140d).
Regards,
Mathieu
Chunyan Zhang (1):
coresight: stm: return error code instead of zero in .packet()
Mathieu Poirier (1):
coresight: reset "enable_sink" flag when need be
Muhammad Abdul WAHAB (2):
coresight: etm3x: indentation fix (extra space removed)
coresight: etm3x: Adding missing features of Coresight PTM components
Suzuki K Poulose (1):
coresight: Add support for ARM Coresight STM-500
Suzuki K. Poulose (3):
coresight: tmc: Cleanup operation mode handling
coresight: tmc: Get rid of mode parameter for helper routines
coresight: tmc: Remove duplicate memset
drivers/hwtracing/coresight/coresight-etm-perf.c | 31 ++++-----
drivers/hwtracing/coresight/coresight-etm.h | 5 ++
.../hwtracing/coresight/coresight-etm3x-sysfs.c | 12 +++-
drivers/hwtracing/coresight/coresight-priv.h | 4 +-
drivers/hwtracing/coresight/coresight-stm.c | 9 ++-
drivers/hwtracing/coresight/coresight-tmc-etf.c | 48 ++++++--------
drivers/hwtracing/coresight/coresight-tmc-etr.c | 43 +++++--------
drivers/hwtracing/coresight/coresight-tmc.h | 2 +-
drivers/hwtracing/coresight/coresight.c | 74 ++++++++++++++++++++--
9 files changed, 144 insertions(+), 84 deletions(-)
--
2.7.4
^ permalink raw reply
* [PATCH 1/2] arm64: perf: Move ARMv8 PMU perf event definitions to asm/perf_event.h
From: Wei Huang @ 2016-11-10 18:09 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161110171732.GA17134@arm.com>
On 11/10/2016 11:17 AM, Will Deacon wrote:
> On Thu, Nov 10, 2016 at 03:32:12PM +0000, Marc Zyngier wrote:
>> On 10/11/16 15:12, Wei Huang wrote:
>>>
>>>
>>> On 11/10/2016 03:10 AM, Marc Zyngier wrote:
>>>> Hi Wei,
>>>>
>>>> On 09/11/16 19:57, Wei Huang wrote:
>>>>> This patch moves ARMv8-related perf event definitions from perf_event.c
>>>>> to asm/perf_event.h; so KVM code can use them directly. This also help
>>>>> remove a duplicated definition of SW_INCR in perf_event.h.
>>>>>
>>>>> Signed-off-by: Wei Huang <wei@redhat.com>
>>>>> ---
>>>>> arch/arm64/include/asm/perf_event.h | 161 +++++++++++++++++++++++++++++++++++-
>>>>> arch/arm64/kernel/perf_event.c | 161 ------------------------------------
>>>>> 2 files changed, 160 insertions(+), 162 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm64/include/asm/perf_event.h b/arch/arm64/include/asm/perf_event.h
>>>>> index 2065f46..6c7b18b 100644
>>>>> --- a/arch/arm64/include/asm/perf_event.h
>>>>> +++ b/arch/arm64/include/asm/perf_event.h
>>>>> @@ -46,7 +46,166 @@
>>>>> #define ARMV8_PMU_EVTYPE_MASK 0xc800ffff /* Mask for writable bits */
>>>>> #define ARMV8_PMU_EVTYPE_EVENT 0xffff /* Mask for EVENT bits */
>>>>>
>>>>> -#define ARMV8_PMU_EVTYPE_EVENT_SW_INCR 0 /* Software increment event */
>>>>> +/*
>>>>> + * ARMv8 PMUv3 Performance Events handling code.
>>>>> + * Common event types.
>>>>> + */
>>>>> +
>>>>> +/* Required events. */
>>>>> +#define ARMV8_PMUV3_PERFCTR_SW_INCR 0x00
>>>>> +#define ARMV8_PMUV3_PERFCTR_L1D_CACHE_REFILL 0x03
>>>>> +#define ARMV8_PMUV3_PERFCTR_L1D_CACHE 0x04
>>>>> +#define ARMV8_PMUV3_PERFCTR_BR_MIS_PRED 0x10
>>>>> +#define ARMV8_PMUV3_PERFCTR_CPU_CYCLES 0x11
>>>>> +#define ARMV8_PMUV3_PERFCTR_BR_PRED 0x12
>>>>
>>>> In my initial review, I asked for the "required" events to be moved to a
>>>> shared location. What's the rational for moving absolutely everything?
>>>
>>> I did notice the phrase "required" in the original email. However I
>>> think it is weird to have two places for a same set of PMU definitions.
>>> Other developers might think these two are missing if they don't search
>>> kernel files carefully.
>>>
>>> If Will Deacon and you insist, I can move only two defs to perf_event.h,
>>> consolidated with the 2nd patch into a single one.
>>
>> My personal feeling is that only architected events should be in a
>> public header. The CPU-specific ones are probably better kept private,
>> as it is doubtful that other users would appear).
>>
>> I'll leave it up to Will to decide, as all I want to avoid is the
>> duplication of constants between the PMU and KVM code bases.
>
> Yeah, just take the sets that you need (i.e. the architected events).
Hi Will,
Just to clarify what "architected" means:
We need two for KVM: SW_INCR (architectural) and CPU_CYCLES
(micro-architectural). Looking at perf_event.c file, I can either
relocate the "Required events" (6 events) or the whole set of
ARMV8_PMUV3_PERFCTR_* (~50 events) to perf_event.h. Which way you prefer?
Thanks,
-Wei
>
> Also, check that it builds.
>
> Will
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" 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
* [RFC v2 8/8] iommu/arm-smmu: implement add_reserved_regions callback
From: Auger Eric @ 2016-11-10 18:00 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161110161331.GJ2078@8bytes.org>
Hi Joerg,
On 10/11/2016 17:13, Joerg Roedel wrote:
> On Thu, Nov 10, 2016 at 04:57:51PM +0100, Auger Eric wrote:
>> It does not only serve the purpose to register the MSI IOVA region. We
>> also need to allocate an iova_domain where MSI IOVAs will be allocated
>> upon the request of the relevant MSI controllers. Do you mean you don't
>> like to use the iova allocator for this purpose?
>
> Yes, it looks like the only purpose of iommu_get_dma_msi_region_cookie()
> is to get the msi-region information into into the reserved-list.
>
> Why do you need to 'allocate' the MSI region after all? Except for
> IOMMU_DOMAIN_DMA domains the iommu-core code does not care about address
> allocation. This is up to the users of the domain, which includes that
> the user has to take care of the MSI region.
The purpose is to reuse the transparent MSI IOVA allocation scheme
introduced by Robin in [PATCH v7 00/22] Generic DT bindings for PCI
IOMMUs and ARM SMMU (commit 44bb7e243bd4b4e5c79de2452cd9762582f58925).
GICv2m and GICV3 ITS use dma-mapping iommu_dma_map_msi_msg to allocate
an MSI IOVA on-demand. This relies on the existence of an allocated
iova_domain whose handle is stored in domain->iova_cookie.
the iommu_dma_init_domain could be done in VFIO instead of in the
IOMMU-code, on the basis of dm region info. But we would need to
differentiate the MSI window from P2P windows.
msi-region start/length are arbitrarily chosen and the setup of the
reserved region list does not depend on iommu_get_dma_msi_region_cookie;
but maybe I completely misunderstand your question.
> Besides that, 'iommu_get_dma_msi_region_cookie' is a terrible function
> name and does not describe at all what the function does or is supposed
> to do.
Yes this was discussed with Alex& Robin already
(https://patchwork.kernel.org/patch/9363989/). I proposed to rename into
iommu_setup_dma_msi_region but this was not validated.
Thanks
Eric
>
>
> Joerg
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
^ permalink raw reply
* [PATCH V6 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping
From: Lorenzo Pieralisi @ 2016-11-10 17:58 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <60c1d53146c0aebd3a05095823229224@codeaurora.org>
On Thu, Nov 10, 2016 at 10:02:35AM -0500, agustinv at codeaurora.org wrote:
> Hey Hanjun,
>
> On 2016-11-09 21:36, Hanjun Guo wrote:
> >Hi Marc, Rafael, Lorenzo,
> >
> >Since we agreed to add a probe deferral if we failed to get irq
> >resources which mirroring the DT does (patch 1 in this patch set),
> >I think the last blocker to make things work both for Agustin and
> >me [1] is this patch, which makes the interrupt producer and consumer
> >work in ACPI, we have two different solution for one thing, we'd happy
> >to work together for one solution, could you give some suggestions
> >please?
> >
> >[1]: https://mail-archive.com/linux-kernel at vger.kernel.org/msg1257419.html
> >
> >Agustin, I have some comments below.
> >
> >On 2016/10/29 4:48, Agustin Vega-Frias wrote:
> >>This allows irqchip drivers to associate an ACPI DSDT device to
> >>an IRQ domain and provides support for using the ResourceSource
> >>in Extended IRQ Resources to find the domain and map the IRQs
> >>specified on that domain.
> >>
> >>Signed-off-by: Agustin Vega-Frias <agustinv@codeaurora.org>
> >>---
> >> drivers/acpi/Makefile | 1 +
> >> drivers/acpi/irqdomain.c | 119
> >>+++++++++++++++++++++++++++++++++++++++++++++++
> >
> >Could we just reuse the gsi.c and not introduce a new
> >file, probably we can change the gsi.c to irqdomain.c
> >or something similar, then reuse the code in gsi.c.
>
> I was thinking just that after we chatted off-list.
Yes, that's a fair point.
> I might revisit and see what I come up with given that we already have
> a device argument and we could pass the IRQ source there.
I agree with the approach taken by this patch, I do not like much
passing around struct acpi_resource_source *source (in particular
the dummy struct) I do not think it is needed, I will comment on
the code.
Hopefully there is not any buggy FW out there that does use the
resource source inappropriately otherwise we will notice on x86/ia64
(ie you can't blame FW if it breaks the kernel) but I suspect the
only way to find out is by trying, the patch has to go through Rafael's
review anyway before getting there so it is fine.
Lorenzo
> Thanks
>
> >
> >Thanks
> >Hanjun
> >
> >> drivers/acpi/resource.c | 21 +++++----
> >> include/linux/acpi.h | 25 ++++++++++
> >> 4 files changed, 157 insertions(+), 9 deletions(-)
> >> create mode 100644 drivers/acpi/irqdomain.c
> >>
> >>diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> >>index 9ed0878..880401b 100644
> >>--- a/drivers/acpi/Makefile
> >>+++ b/drivers/acpi/Makefile
> >>@@ -57,6 +57,7 @@ acpi-$(CONFIG_ACPI_PROCFS_POWER) += cm_sbs.o
> >> acpi-y += acpi_lpat.o
> >> acpi-$(CONFIG_ACPI_GENERIC_GSI) += gsi.o
> >> acpi-$(CONFIG_ACPI_WATCHDOG) += acpi_watchdog.o
> >>+acpi-$(CONFIG_IRQ_DOMAIN) += irqdomain.o
> >>
> >> # These are (potentially) separate modules
> >>
> >>diff --git a/drivers/acpi/irqdomain.c b/drivers/acpi/irqdomain.c
> >>new file mode 100644
> >>index 0000000..11d3658
> >>--- /dev/null
> >>+++ b/drivers/acpi/irqdomain.c
> >>@@ -0,0 +1,119 @@
> >>+/*
> >>+ * ACPI ResourceSource/IRQ domain mapping support
> >>+ *
> >>+ * Copyright (c) 2016, The Linux Foundation. All rights reserved.
> >>+ *
> >>+ * This program is free software; you can redistribute it
> >>and/or modify
> >>+ * it under the terms of the GNU General Public License version 2 and
> >>+ * only version 2 as published by the Free Software Foundation.
> >>+ *
> >>+ * This program is distributed in the hope that it will be useful,
> >>+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >>+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> >>+ * GNU General Public License for more details.
> >>+ */
> >>+#include <linux/acpi.h>
> >>+#include <linux/irq.h>
> >>+#include <linux/irqdomain.h>
> >>+
> >>+/**
> >>+ * acpi_irq_domain_register_irq() - Register the mapping for an
> >>IRQ produced
> >>+ * by the given
> >>acpi_resource_source to a
> >>+ * Linux IRQ number
> >>+ * @source: IRQ source
> >>+ * @hwirq: Hardware IRQ number
> >>+ * @trigger: trigger type of the IRQ number to be mapped
> >>+ * @polarity: polarity of the IRQ to be mapped
> >>+ *
> >>+ * Returns: a valid linux IRQ number on success
> >>+ * -ENODEV if the given acpi_resource_source cannot be found
> >>+ * -EPROBE_DEFER if the IRQ domain has not been registered
> >>+ * -EINVAL for all other errors
> >>+ */
> >>+int acpi_irq_domain_register_irq(const struct
> >>acpi_resource_source *source,
> >>+ u32 hwirq, int trigger, int polarity)
> >>+{
> >>+ struct irq_fwspec fwspec;
> >>+ struct acpi_device *device;
> >>+ acpi_handle handle;
> >>+ acpi_status status;
> >>+ int ret;
> >>+
> >>+ /* An empty acpi_resource_source means it is a GSI */
> >>+ if (!source->string_length)
> >>+ return acpi_register_gsi(NULL, hwirq, trigger, polarity);
> >>+
> >>+ status = acpi_get_handle(NULL, source->string_ptr, &handle);
> >>+ if (ACPI_FAILURE(status))
> >>+ return -ENODEV;
> >>+
> >>+ device = acpi_bus_get_acpi_device(handle);
> >>+ if (!device)
> >>+ return -ENODEV;
> >>+
> >>+ if (irq_find_matching_fwnode(&device->fwnode, DOMAIN_BUS_ANY)
> >>== NULL) {
> >>+ ret = -EPROBE_DEFER;
> >>+ goto out_put_device;
> >>+ }
> >>+
> >>+ fwspec.fwnode = &device->fwnode;
> >>+ fwspec.param[0] = hwirq;
> >>+ fwspec.param[1] = acpi_dev_get_irq_type(trigger, polarity);
> >>+ fwspec.param_count = 2;
> >>+
> >>+ ret = irq_create_fwspec_mapping(&fwspec);
> >>+
> >>+out_put_device:
> >>+ acpi_bus_put_acpi_device(device);
> >>+
> >>+ return ret;
> >>+}
> >>+EXPORT_SYMBOL_GPL(acpi_irq_domain_register_irq);
> >>+
> >>+/**
> >>+ * acpi_irq_domain_unregister_irq() - Delete the mapping for an
> >>IRQ produced
> >>+ * by the given
> >>acpi_resource_source to a
> >>+ * Linux IRQ number
> >>+ * @source: IRQ source
> >>+ * @hwirq: Hardware IRQ number
> >>+ *
> >>+ * Returns: 0 on success
> >>+ * -ENODEV if the given acpi_resource_source cannot be found
> >>+ * -EINVAL for all other errors
> >>+ */
> >>+int acpi_irq_domain_unregister_irq(const struct
> >>acpi_resource_source *source,
> >>+ u32 hwirq)
> >>+{
> >>+ struct irq_domain *domain;
> >>+ struct acpi_device *device;
> >>+ acpi_handle handle;
> >>+ acpi_status status;
> >>+ int ret = 0;
> >>+
> >>+ if (!source->string_length) {
> >>+ acpi_unregister_gsi(hwirq);
> >>+ return 0;
> >>+ }
> >>+
> >>+ status = acpi_get_handle(NULL, source->string_ptr, &handle);
> >>+ if (ACPI_FAILURE(status))
> >>+ return -ENODEV;
> >>+
> >>+ device = acpi_bus_get_acpi_device(handle);
> >>+ if (!device)
> >>+ return -ENODEV;
> >>+
> >>+ domain = irq_find_matching_fwnode(&device->fwnode, DOMAIN_BUS_ANY);
> >>+ if (!domain) {
> >>+ ret = -EINVAL;
> >>+ goto out_put_device;
> >>+ }
> >>+
> >>+ irq_dispose_mapping(irq_find_mapping(domain, hwirq));
> >>+
> >>+out_put_device:
> >>+ acpi_bus_put_acpi_device(device);
> >>+
> >>+ return ret;
> >>+}
> >>+EXPORT_SYMBOL_GPL(acpi_irq_domain_unregister_irq);
> >>diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
> >>index 4beda15..ccb6f4f 100644
> >>--- a/drivers/acpi/resource.c
> >>+++ b/drivers/acpi/resource.c
> >>@@ -381,14 +381,15 @@ static void
> >>acpi_dev_irqresource_disabled(struct resource *res, u32 gsi)
> >> res->flags = IORESOURCE_IRQ | IORESOURCE_DISABLED |
> >>IORESOURCE_UNSET;
> >> }
> >>
> >>-static void acpi_dev_get_irqresource(struct resource *res, u32 gsi,
> >>+static void acpi_dev_get_irqresource(struct resource *res, u32 hwirq,
> >>+ const struct acpi_resource_source *source,
> >> u8 triggering, u8 polarity, u8 shareable,
> >> bool legacy)
> >> {
> >> int irq, p, t;
> >>
> >>- if (!valid_IRQ(gsi)) {
> >>- acpi_dev_irqresource_disabled(res, gsi);
> >>+ if ((source->string_length == 0) && !valid_IRQ(hwirq)) {
> >>+ acpi_dev_irqresource_disabled(res, hwirq);
> >> return;
> >> }
> >>
> >>@@ -402,25 +403,25 @@ static void
> >>acpi_dev_get_irqresource(struct resource *res, u32 gsi,
> >> * using extended IRQ descriptors we take the IRQ configuration
> >> * from _CRS directly.
> >> */
> >>- if (legacy && !acpi_get_override_irq(gsi, &t, &p)) {
> >>+ if (legacy && !acpi_get_override_irq(hwirq, &t, &p)) {
> >> u8 trig = t ? ACPI_LEVEL_SENSITIVE : ACPI_EDGE_SENSITIVE;
> >> u8 pol = p ? ACPI_ACTIVE_LOW : ACPI_ACTIVE_HIGH;
> >>
> >> if (triggering != trig || polarity != pol) {
> >>- pr_warning("ACPI: IRQ %d override to %s, %s\n", gsi,
> >>- t ? "level" : "edge", p ? "low" : "high");
> >>+ pr_warn("ACPI: IRQ %d override to %s, %s\n", hwirq,
> >>+ t ? "level" : "edge", p ? "low" : "high");
> >> triggering = trig;
> >> polarity = pol;
> >> }
> >> }
> >>
> >> res->flags = acpi_dev_irq_flags(triggering, polarity, shareable);
> >>- irq = acpi_register_gsi(NULL, gsi, triggering, polarity);
> >>+ irq = acpi_irq_domain_register_irq(source, hwirq, triggering,
> >>polarity);
> >> if (irq >= 0) {
> >> res->start = irq;
> >> res->end = irq;
> >> } else {
> >>- acpi_dev_irqresource_disabled(res, gsi);
> >>+ acpi_dev_irqresource_disabled(res, hwirq);
> >> }
> >> }
> >>
> >>@@ -446,6 +447,7 @@ static void acpi_dev_get_irqresource(struct
> >>resource *res, u32 gsi,
> >> bool acpi_dev_resource_interrupt(struct acpi_resource *ares,
> >>int index,
> >> struct resource *res)
> >> {
> >>+ const struct acpi_resource_source dummy = { 0, 0, NULL };
> >> struct acpi_resource_irq *irq;
> >> struct acpi_resource_extended_irq *ext_irq;
> >>
> >>@@ -460,7 +462,7 @@ bool acpi_dev_resource_interrupt(struct
> >>acpi_resource *ares, int index,
> >> acpi_dev_irqresource_disabled(res, 0);
> >> return false;
> >> }
> >>- acpi_dev_get_irqresource(res, irq->interrupts[index],
> >>+ acpi_dev_get_irqresource(res, irq->interrupts[index], &dummy,
> >> irq->triggering, irq->polarity,
> >> irq->sharable, true);
> >> break;
> >>@@ -471,6 +473,7 @@ bool acpi_dev_resource_interrupt(struct
> >>acpi_resource *ares, int index,
> >> return false;
> >> }
> >> acpi_dev_get_irqresource(res, ext_irq->interrupts[index],
> >>+ &ext_irq->resource_source,
> >> ext_irq->triggering, ext_irq->polarity,
> >> ext_irq->sharable, false);
> >> break;
> >>diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> >>index 40213c4..ce30a12 100644
> >>--- a/include/linux/acpi.h
> >>+++ b/include/linux/acpi.h
> >>@@ -321,6 +321,31 @@ void acpi_set_irq_model(enum
> >>acpi_irq_model_id model,
> >> */
> >> void acpi_unregister_gsi (u32 gsi);
> >>
> >>+#ifdef CONFIG_IRQ_DOMAIN
> >>+
> >>+int acpi_irq_domain_register_irq(const struct
> >>acpi_resource_source *source,
> >>+ u32 hwirq, int trigger, int polarity);
> >>+int acpi_irq_domain_unregister_irq(const struct
> >>acpi_resource_source *source,
> >>+ u32 hwirq);
> >>+
> >>+#else
> >>+
> >>+static inline int acpi_irq_domain_register_irq(
> >>+ const struct acpi_resource_source *source, u32 hwirq, int trigger,
> >>+ int polarity)
> >>+{
> >>+ return acpi_register_gsi(NULL, hwirq, trigger, polarity);
> >>+}
> >>+
> >>+static inline int acpi_irq_domain_unregister_irq(
> >>+ const struct acpi_resource_source *source, u32 hwirq)
> >>+{
> >>+ acpi_unregister_gsi(hwirq);
> >>+ return 0;
> >>+}
> >>+
> >>+#endif /* CONFIG_IRQ_DOMAIN */
> >>+
> >> struct pci_dev;
> >>
> >> int acpi_pci_irq_enable (struct pci_dev *dev);
> >>
>
> --
> Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm
> Technologies, Inc.
> Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a
> Linux Foundation Collaborative Project.
^ permalink raw reply
* [PATCH] ARM: OMAP2+: Remove unused MUSB initialization code
From: Tony Lindgren @ 2016-11-10 17:58 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <2624325.4PiB8SBptT@avalon>
* Laurent Pinchart <laurent.pinchart@ideasonboard.com> [161110 10:56]:
> Hi Tony,
>
> On Thursday 10 Nov 2016 10:54:02 Tony Lindgren wrote:
> > * Laurent Pinchart <laurent.pinchart@ideasonboard.com> [161110 10:43]:
> > > With the removal of board-ldp.c and board-rx51.c, the last users of the
> > > MUSB initialization board code are gone. Remove it.
> >
> > Thanks, I have the same patch already from 3 years ago in omap-for-
> > v3.14/omap3-board-removal branch that I'll use.
>
> v3.14 ? That feels like cheating :-)
:p
> > Still need to rebase and check few other patches there before reposting them
> > all.
>
> Will you get that one in v4.10 ?
Yeah planning to assuming no problems and when out
of the eternal musb regressions land.
Regards,
Tony
^ permalink raw reply
* [PATCH] ARM: OMAP2+: Remove unused MUSB initialization code
From: Laurent Pinchart @ 2016-11-10 17:56 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161110175402.GE27724@atomide.com>
Hi Tony,
On Thursday 10 Nov 2016 10:54:02 Tony Lindgren wrote:
> * Laurent Pinchart <laurent.pinchart@ideasonboard.com> [161110 10:43]:
> > With the removal of board-ldp.c and board-rx51.c, the last users of the
> > MUSB initialization board code are gone. Remove it.
>
> Thanks, I have the same patch already from 3 years ago in omap-for-
> v3.14/omap3-board-removal branch that I'll use.
v3.14 ? That feels like cheating :-)
> Still need to rebase and check few other patches there before reposting them
> all.
Will you get that one in v4.10 ?
> FYI, to avoid duplicate work, the old barnch is at [0].
>
> Regards,
>
> Tony
>
> https://git.kernel.org/cgit/linux/kernel/git/tmlind/linux-omap.git/log/?h=om
> ap-for-v3.14/omap3-board-removal
--
Regards,
Laurent Pinchart
^ permalink raw reply
* PM regression with LED changes in next-20161109
From: Tony Lindgren @ 2016-11-10 17:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161110162925.GA28832@amd>
* Pavel Machek <pavel@ucw.cz> [161110 09:29]:
> Hi!
>
> > >>>Looks like commit 883d32ce3385 ("leds: core: Add support for poll()ing
> > >>>the sysfs brightness attr for changes.") breaks runtime PM for me.
> > >>>
> > >>>On my omap dm3730 based test system, idle power consumption is over 70
> > >>>times higher now with this patch! It goes from about 6mW for the core
> > >>>system to over 440mW during idle meaning there's some busy timer now
> > >>>active.
> > >>>
> > >>>Reverting this patch fixes the issue. Any ideas?
>
> Are you using any LED that toggles with high frequency? Like perhaps
> LED that is lit when CPU is active?
Yeah one of them seems to have cpu0 as the default trigger.
Tony
^ permalink raw reply
* [RESEND PATCH v1 03/11] drivers: soc: hisi: Add support for Hisilicon Djtag driver
From: Mark Rutland @ 2016-11-10 17:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478151727-20250-4-git-send-email-anurup.m@huawei.com>
On Thu, Nov 03, 2016 at 01:41:59AM -0400, Anurup M wrote:
> From: Tan Xiaojun <tanxiaojun@huawei.com>
>
> The Hisilicon Djtag is an independent component which connects
> with some other components in the SoC by Debug Bus. This driver
> can be configured to access the registers of connecting components
> (like L3 cache) during real time debugging.
>
Just to check, is this likely to be used in multi-socket hardware, and
if so, are instances always-on?
> Signed-off-by: Tan Xiaojun <tanxiaojun@huawei.com>
> Signed-off-by: John Garry <john.garry@huawei.com>
> Signed-off-by: Anurup M <anurup.m@huawei.com>
> ---
> drivers/soc/Kconfig | 1 +
> drivers/soc/Makefile | 1 +
> drivers/soc/hisilicon/Kconfig | 12 +
> drivers/soc/hisilicon/Makefile | 1 +
> drivers/soc/hisilicon/djtag.c | 639 ++++++++++++++++++++++++++++++++++++
> include/linux/soc/hisilicon/djtag.h | 38 +++
> 6 files changed, 692 insertions(+)
> create mode 100644 drivers/soc/hisilicon/Kconfig
> create mode 100644 drivers/soc/hisilicon/Makefile
> create mode 100644 drivers/soc/hisilicon/djtag.c
> create mode 100644 include/linux/soc/hisilicon/djtag.h
Other than the PMU driver(s), what is going to use this?
If you don't have something in particular, please also place this under
drivers/perf/hisilicon, along with the PMU driver(s).
We can always move it later if necessary.
[...]
> +#define SC_DJTAG_TIMEOUT 100000 /* 100ms */
This would be better as:
#define SC_DJTAG_TIMEOUT_US (100 * USEC_PER_MSEC)
(you'll need to include <linux/time64.h>)
[...]
> +static void djtag_read32_relaxed(void __iomem *regs_base, u32 off, u32 *value)
> +{
> + void __iomem *reg_addr = regs_base + off;
> +
> + *value = readl_relaxed(reg_addr);
> +}
> +
> +static void djtag_write32(void __iomem *regs_base, u32 off, u32 val)
> +{
> + void __iomem *reg_addr = regs_base + off;
> +
> + writel(val, reg_addr);
> +}
I think these make the driver harder to read, especially given the read
function is void and takes an output pointer.
In either case you can call readl/writel directly with base + off for
the __iomem ptr. Please do that.
> +
> +/*
> + * djtag_readwrite_v1/v2: djtag read/write interface
> + * @reg_base: djtag register base address
> + * @offset: register's offset
> + * @mod_sel: module selection
> + * @mod_mask: mask to select specific modules for write
> + * @is_w: write -> true, read -> false
> + * @wval: value to register for write
> + * @chain_id: which sub module for read
> + * @rval: value in register for read
> + *
> + * Return non-zero if error, else return 0.
> + */
> +static int djtag_readwrite_v1(void __iomem *regs_base, u32 offset, u32 mod_sel,
> + u32 mod_mask, bool is_w, u32 wval, int chain_id, u32 *rval)
> +{
> + u32 rd;
> + int timeout = SC_DJTAG_TIMEOUT;
> +
> + if (!(mod_mask & CHAIN_UNIT_CFG_EN)) {
> + pr_warn("djtag: do nothing.\n");
> + return 0;
> + }
> +
> + /* djtag mster enable & accelerate R,W */
> + djtag_write32(regs_base, SC_DJTAG_MSTR_EN,
> + DJTAG_NOR_CFG | DJTAG_MSTR_EN);
> +
> + /* select module */
> + djtag_write32(regs_base, SC_DJTAG_DEBUG_MODULE_SEL, mod_sel);
> + djtag_write32(regs_base, SC_DJTAG_CHAIN_UNIT_CFG_EN,
> + mod_mask & CHAIN_UNIT_CFG_EN);
> +
> + if (is_w) {
> + djtag_write32(regs_base, SC_DJTAG_MSTR_WR, DJTAG_MSTR_W);
> + djtag_write32(regs_base, SC_DJTAG_MSTR_DATA, wval);
> + } else
> + djtag_write32(regs_base, SC_DJTAG_MSTR_WR, DJTAG_MSTR_R);
> +
> + /* address offset */
> + djtag_write32(regs_base, SC_DJTAG_MSTR_ADDR, offset);
> +
> + /* start to write to djtag register */
> + djtag_write32(regs_base, SC_DJTAG_MSTR_START_EN, DJTAG_MSTR_START_EN);
> +
> + /* ensure the djtag operation is done */
> + do {
> + djtag_read32_relaxed(regs_base, SC_DJTAG_MSTR_START_EN, &rd);
> + if (!(rd & DJTAG_MSTR_EN))
> + break;
> +
> + udelay(1);
> + } while (timeout--);
> +
> + if (timeout < 0) {
> + pr_err("djtag: %s timeout!\n", is_w ? "write" : "read");
> + return -EBUSY;
> + }
> +
> + if (!is_w)
> + djtag_read32_relaxed(regs_base,
> + SC_DJTAG_RD_DATA_BASE + chain_id * 0x4, rval);
> +
> + return 0;
> +}
Please factor out the common bits into helpers and have separate
read/write functions. It's incredibly difficult to follow the code with
read/write hidden behind a boolean parameter.
> +static const struct of_device_id djtag_of_match[] = {
> + /* for hip05(D02) cpu die */
> + { .compatible = "hisilicon,hip05-cpu-djtag-v1",
> + .data = (void *)djtag_readwrite_v1 },
> + /* for hip05(D02) io die */
> + { .compatible = "hisilicon,hip05-io-djtag-v1",
> + .data = (void *)djtag_readwrite_v1 },
> + /* for hip06(D03) cpu die */
> + { .compatible = "hisilicon,hip06-cpu-djtag-v1",
> + .data = (void *)djtag_readwrite_v1 },
> + /* for hip06(D03) io die */
> + { .compatible = "hisilicon,hip06-io-djtag-v2",
> + .data = (void *)djtag_readwrite_v2 },
> + /* for hip07(D05) cpu die */
> + { .compatible = "hisilicon,hip07-cpu-djtag-v2",
> + .data = (void *)djtag_readwrite_v2 },
> + /* for hip07(D05) io die */
> + { .compatible = "hisilicon,hip07-io-djtag-v2",
> + .data = (void *)djtag_readwrite_v2 },
> + {},
> +};
Binding documentation for all of these should be added *before* this
patch, per Documentation/devicetree/bindings/submitting-patches.txt.
Please move any relevant binding documentation earlier in the series.
> +MODULE_DEVICE_TABLE(of, djtag_of_match);
> +
> +static ssize_t
> +show_modalias(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> + struct hisi_djtag_client *client = to_hisi_djtag_client(dev);
> +
> + return sprintf(buf, "%s%s\n", MODULE_PREFIX, client->name);
> +}
> +static DEVICE_ATTR(modalias, 0444, show_modalias, NULL);
> +
> +static struct attribute *hisi_djtag_dev_attrs[] = {
> + NULL,
> + /* modalias helps coldplug: modprobe $(cat .../modalias) */
> + &dev_attr_modalias.attr,
> + NULL
> +};
> +ATTRIBUTE_GROUPS(hisi_djtag_dev);
Why do we need to expose this under sysfs?
> +struct bus_type hisi_djtag_bus = {
> + .name = "hisi-djtag",
> + .match = hisi_djtag_device_match,
> + .probe = hisi_djtag_device_probe,
> + .remove = hisi_djtag_device_remove,
> +};
I take it the core bus code handles reference-counting users of the bus?
> +struct hisi_djtag_client *hisi_djtag_new_device(struct hisi_djtag_host *host,
> + struct device_node *node)
> +{
> + struct hisi_djtag_client *client;
> + int status;
> +
> + client = kzalloc(sizeof(*client), GFP_KERNEL);
> + if (!client)
> + return NULL;
> +
> + client->host = host;
> +
> + client->dev.parent = &client->host->dev;
> + client->dev.bus = &hisi_djtag_bus;
> + client->dev.type = &hisi_djtag_client_type;
> + client->dev.of_node = node;
I suspect that we should do:
client->dev.of_node = of_node_get(node);
... so that it's guaranteed we retain a reference.
> + snprintf(client->name, DJTAG_CLIENT_NAME_LEN, "%s%s",
> + DJTAG_PREFIX, node->name);
> + dev_set_name(&client->dev, "%s", client->name);
> +
> + status = device_register(&client->dev);
> + if (status < 0) {
> + pr_err("error adding new device, status=%d\n", status);
> + kfree(client);
> + return NULL;
> + }
> +
> + return client;
> +}
> +
> +static struct hisi_djtag_client *hisi_djtag_of_register_device(
> + struct hisi_djtag_host *host,
> + struct device_node *node)
> +{
> + struct hisi_djtag_client *client;
> +
> + client = hisi_djtag_new_device(host, node);
> + if (client == NULL) {
> + dev_err(&host->dev, "error registering device %s\n",
> + node->full_name);
> + of_node_put(node);
I don't think this should be here, given what djtag_register_devices()
does.
> + return ERR_PTR(-EINVAL);
> + }
> +
> + return client;
> +}
> +
> +static void djtag_register_devices(struct hisi_djtag_host *host)
> +{
> + struct device_node *node;
> + struct hisi_djtag_client *client;
> +
> + if (!host->of_node)
> + return;
> +
> + for_each_available_child_of_node(host->of_node, node) {
> + if (of_node_test_and_set_flag(node, OF_POPULATED))
> + continue;
> + client = hisi_djtag_of_register_device(host, node);
> + list_add(&client->next, &host->client_list);
> + }
> +}
Given hisi_djtag_of_register_device() can return ERR_PTR(-EINVAL), the
list_add is not safe.
> +static int djtag_host_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct hisi_djtag_host *host;
> + const struct of_device_id *of_id;
> + struct resource *res;
> + int rc;
> +
> + of_id = of_match_device(djtag_of_match, dev);
> + if (!of_id)
> + return -EINVAL;
> +
> + host = kzalloc(sizeof(*host), GFP_KERNEL);
> + if (!host)
> + return -ENOMEM;
> +
> + host->of_node = dev->of_node;
host->of_node = of_node_get(dev->of_node);
> + host->djtag_readwrite = of_id->data;
> + spin_lock_init(&host->lock);
> +
> + INIT_LIST_HEAD(&host->client_list);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + dev_err(&pdev->dev, "No reg resorces!\n");
> + kfree(host);
> + return -EINVAL;
> + }
> +
> + if (!resource_size(res)) {
> + dev_err(&pdev->dev, "Zero reg entry!\n");
> + kfree(host);
> + return -EINVAL;
> + }
> +
> + host->sysctl_reg_map = devm_ioremap_resource(dev, res);
> + if (IS_ERR(host->sysctl_reg_map)) {
> + dev_warn(dev, "Unable to map sysctl registers.\n");
> + kfree(host);
> + return -EINVAL;
> + }
Please have a common error path rather than duplicating this free.
e.g. at the end of the function have:
err_free:
kfree(host);
return err;
... and in cases like this, have:
if (whatever()) {
dev_warn(dev, "this failed xxx\n");
err = -EINVAL;
goto err_free;
}
Thanks,
Mark.
^ 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