* [PATCH v6 2/9] drm/hisilicon/hibmc: Add video memory management
From: Rongrong Zou @ 2016-11-11 11:16 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAOw6vbKK+kmjZv+su4tg43Pcos-B3MjZcAv461au86SeqCQP=A@mail.gmail.com>
? 2016/11/11 1:35, Sean Paul ??:
> On Fri, Oct 28, 2016 at 3:27 AM, Rongrong Zou <zourongrong@gmail.com> wrote:
>> Hibmc have 32m video memory which can be accessed through PCIe by host,
>> we use ttm to manage these memory.
>>
>> Signed-off-by: Rongrong Zou <zourongrong@gmail.com>
>> ---
>> drivers/gpu/drm/hisilicon/hibmc/Kconfig | 1 +
>> drivers/gpu/drm/hisilicon/hibmc/Makefile | 2 +-
>> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 12 +
>> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h | 46 +++
>> drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c | 490 ++++++++++++++++++++++++
>> 5 files changed, 550 insertions(+), 1 deletion(-)
>> create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
>>
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/Kconfig b/drivers/gpu/drm/hisilicon/hibmc/Kconfig
>> index a9af90d..bcb8c18 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/Kconfig
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/Kconfig
>> @@ -1,6 +1,7 @@
>> config DRM_HISI_HIBMC
>> tristate "DRM Support for Hisilicon Hibmc"
>> depends on DRM && PCI
>> + select DRM_TTM
>>
>> help
>> Choose this option if you have a Hisilicon Hibmc soc chipset.
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile
>> index 97cf4a0..d5c40b8 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-drm-y := hibmc_drm_drv.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 4669d42..81f4301 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> @@ -31,6 +31,7 @@
>> #ifdef CONFIG_COMPAT
>> .compat_ioctl = drm_compat_ioctl,
>> #endif
>> + .mmap = hibmc_mmap,
>> .poll = drm_poll,
>> .read = drm_read,
>> .llseek = no_llseek,
>> @@ -46,6 +47,8 @@ static void hibmc_disable_vblank(struct drm_device *dev, unsigned int pipe)
>> }
>>
>> static struct drm_driver hibmc_driver = {
>> + .driver_features = DRIVER_GEM,
>> +
>
> nit: extra space
>
>> .fops = &hibmc_fops,
>> .name = "hibmc",
>> .date = "20160828",
>> @@ -55,6 +58,10 @@ static void hibmc_disable_vblank(struct drm_device *dev, unsigned int pipe)
>> .get_vblank_counter = drm_vblank_no_hw_counter,
>> .enable_vblank = hibmc_enable_vblank,
>> .disable_vblank = hibmc_disable_vblank,
>> + .gem_free_object_unlocked = hibmc_gem_free_object,
>> + .dumb_create = hibmc_dumb_create,
>> + .dumb_map_offset = hibmc_dumb_mmap_offset,
>> + .dumb_destroy = drm_gem_dumb_destroy,
>> };
>>
>> static int hibmc_pm_suspend(struct device *dev)
>> @@ -163,6 +170,7 @@ static int hibmc_unload(struct drm_device *dev)
>> {
>> struct hibmc_drm_device *hidev = dev->dev_private;
>>
>> + hibmc_mm_fini(hidev);
>> hibmc_hw_fini(hidev);
>> dev->dev_private = NULL;
>> return 0;
>> @@ -183,6 +191,10 @@ static int hibmc_load(struct drm_device *dev, unsigned long flags)
>> if (ret)
>> goto err;
>>
>> + ret = hibmc_mm_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 0037341..db8d80e 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> @@ -20,6 +20,8 @@
>> #define HIBMC_DRM_DRV_H
>>
>> #include <drm/drmP.h>
>> +#include <drm/ttm/ttm_bo_driver.h>
>> +#include <drm/drm_gem.h>
>
> nit: alphabetize
will fix it, thanks.
>
>>
>> struct hibmc_drm_device {
>> /* hw */
>> @@ -30,6 +32,50 @@ struct hibmc_drm_device {
>>
>> /* drm */
>> struct drm_device *dev;
>> +
>> + /* ttm */
>> + struct {
>> + struct drm_global_reference mem_global_ref;
>> + struct ttm_bo_global_ref bo_global_ref;
>> + struct ttm_bo_device bdev;
>> + bool initialized;
>> + } ttm;
>
> I don't think you gain anything other than keystrokes from the substruct
I'm sorry i didn't catch you, i looked at the all drivers used ttm such
as ast/bochs/cirrus/mgag200/qxl/virtio_gpu, they all embedded the ttm substruct
into the driver-private struct.
so do you mean
struct hibmc_drm_device {
/* hw */
void __iomem *mmio;
void __iomem *fb_map;
unsigned long fb_base;
unsigned long fb_size;
/* drm */
struct drm_device *dev;
struct drm_plane plane;
struct drm_crtc crtc;
struct drm_encoder encoder;
struct drm_connector connector;
bool mode_config_initialized;
/* ttm */
struct drm_global_reference mem_global_ref;
struct ttm_bo_global_ref bo_global_ref;
struct ttm_bo_device bdev;
bool initialized;
...
};
?
>
>> +
>> + bool mm_inited;
>> };
>>
>> +struct hibmc_bo {
>> + struct ttm_buffer_object bo;
>> + struct ttm_placement placement;
>> + struct ttm_bo_kmap_obj kmap;
>> + struct drm_gem_object gem;
>> + struct ttm_place placements[3];
>> + int pin_count;
>> +};
>> +
>> +static inline struct hibmc_bo *hibmc_bo(struct ttm_buffer_object *bo)
>> +{
>> + return container_of(bo, struct hibmc_bo, bo);
>> +}
>> +
>> +static inline struct hibmc_bo *gem_to_hibmc_bo(struct drm_gem_object *gem)
>> +{
>> + return container_of(gem, struct hibmc_bo, gem);
>> +}
>> +
>> +#define DRM_FILE_PAGE_OFFSET (0x100000000ULL >> PAGE_SHIFT)
>
> Hide this in ttm.c
ok, will do that.
thanks for pointing it out.
>
>> +
>> +int hibmc_gem_create(struct drm_device *dev, u32 size, bool iskernel,
>> + struct drm_gem_object **obj);
>> +
>> +int hibmc_mm_init(struct hibmc_drm_device *hibmc);
>> +void hibmc_mm_fini(struct hibmc_drm_device *hibmc);
>> +int hibmc_bo_pin(struct hibmc_bo *bo, u32 pl_flag, u64 *gpu_addr);
>> +void hibmc_gem_free_object(struct drm_gem_object *obj);
>> +int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev,
>> + struct drm_mode_create_dumb *args);
>> +int hibmc_dumb_mmap_offset(struct drm_file *file, struct drm_device *dev,
>> + u32 handle, u64 *offset);
>> +int hibmc_mmap(struct file *filp, struct vm_area_struct *vma);
>> +
>> #endif
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
>> new file mode 100644
>> index 0000000..0802ebd
>> --- /dev/null
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
>> @@ -0,0 +1,490 @@
>> +/* 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 "hibmc_drm_drv.h"
>> +#include <ttm/ttm_page_alloc.h>
>> +#include <drm/drm_crtc_helper.h>
>> +#include <drm/drm_atomic_helper.h>
>> +
>> +static inline struct hibmc_drm_device *
>> +hibmc_bdev(struct ttm_bo_device *bd)
>> +{
>> + return container_of(bd, struct hibmc_drm_device, ttm.bdev);
>> +}
>> +
>> +static int
>> +hibmc_ttm_mem_global_init(struct drm_global_reference *ref)
>> +{
>> + return ttm_mem_global_init(ref->object);
>> +}
>> +
>> +static void
>> +hibmc_ttm_mem_global_release(struct drm_global_reference *ref)
>> +{
>> + ttm_mem_global_release(ref->object);
>> +}
>> +
>> +static int hibmc_ttm_global_init(struct hibmc_drm_device *hibmc)
>> +{
>> + struct drm_global_reference *global_ref;
>> + int r;
>
> nit: try not to use one character variable names unless it's for the
> purpose of a loop (ie: i,j). You also use ret elsewhere in the driver,
> so it'd be nice to remain consistent
the whole file is delivered from bochs ttm, i didn't modify anything except
some checkpatch warnings and the 'hibmc_' prefix. Unfortunately, some
problems were delivered too.
>
>> +
>> + global_ref = &hibmc->ttm.mem_global_ref;
>
> I think using the global_ref local obfuscates what you're doing here.
> It saves you 6 characters while typing, but adds a layer of
> indirection for all future readers.
>
>> + global_ref->global_type = DRM_GLOBAL_TTM_MEM;
>> + global_ref->size = sizeof(struct ttm_mem_global);
>> + global_ref->init = &hibmc_ttm_mem_global_init;
>> + global_ref->release = &hibmc_ttm_mem_global_release;
>> + r = drm_global_item_ref(global_ref);
>> + if (r != 0) {
>
> nit: if (r)
will fix it,
thanks.
BTW, i wonder why checkpatch.pl didn't report it.
>
>> + DRM_ERROR("Failed setting up TTM memory accounting subsystem.\n"
>> + );
>
> Breaking up the line for one character is probably not worthwhile, and
> you should really print the error. How about:
>
> DRM_ERROR("Could not get ref on ttm global ret=%d.\n", ret);
i like your solution, thanks.
>
>
>> + return r;
>> + }
>> +
>> + hibmc->ttm.bo_global_ref.mem_glob =
>> + hibmc->ttm.mem_global_ref.object;
>> + global_ref = &hibmc->ttm.bo_global_ref.ref;
>> + global_ref->global_type = DRM_GLOBAL_TTM_BO;
>> + global_ref->size = sizeof(struct ttm_bo_global);
>> + global_ref->init = &ttm_bo_global_init;
>> + global_ref->release = &ttm_bo_global_release;
>> + r = drm_global_item_ref(global_ref);
>> + if (r != 0) {
>> + DRM_ERROR("Failed setting up TTM BO subsystem.\n");
>> + drm_global_item_unref(&hibmc->ttm.mem_global_ref);
>> + return r;
>> + }
>> + return 0;
>> +}
>> +
>> +static void
>> +hibmc_ttm_global_release(struct hibmc_drm_device *hibmc)
>> +{
>> + if (!hibmc->ttm.mem_global_ref.release)
>
> Are you actually hitting this condition? This seems like it's papering
> over something else.
it was also delivered from others, i looked at the xxx_ttm_global_init
function, 'mem_global_ref.release' is assigned unconditionally, so i
think this condition never be hit, it may be hit when release twice,
but this won't take place in my driver.
>
>> + return;
>> +
>> + drm_global_item_unref(&hibmc->ttm.bo_global_ref.ref);
>> + drm_global_item_unref(&hibmc->ttm.mem_global_ref);
>> + hibmc->ttm.mem_global_ref.release = NULL;
>> +}
>> +
>> +static void hibmc_bo_ttm_destroy(struct ttm_buffer_object *tbo)
>> +{
>> + struct hibmc_bo *bo;
>> +
>> + bo = container_of(tbo, struct hibmc_bo, bo);
>
> nit: No need to split this into a separate line.
agreed, thanks.
>
>> +
>> + drm_gem_object_release(&bo->gem);
>> + kfree(bo);
>> +}
>> +
>> +static bool hibmc_ttm_bo_is_hibmc_bo(struct ttm_buffer_object *bo)
>> +{
>> + if (bo->destroy == &hibmc_bo_ttm_destroy)
>> + return true;
>> + return false;
>
> return bo->destroy == &hibmc_bo_ttm_destroy;
looks better to me.
>
>> +}
>> +
>> +static int
>> +hibmc_bo_init_mem_type(struct ttm_bo_device *bdev, u32 type,
>> + struct ttm_mem_type_manager *man)
>> +{
>> + switch (type) {
>> + case TTM_PL_SYSTEM:
>> + man->flags = TTM_MEMTYPE_FLAG_MAPPABLE;
>> + man->available_caching = TTM_PL_MASK_CACHING;
>> + man->default_caching = TTM_PL_FLAG_CACHED;
>> + break;
>> + case TTM_PL_VRAM:
>> + man->func = &ttm_bo_manager_func;
>> + man->flags = TTM_MEMTYPE_FLAG_FIXED |
>> + TTM_MEMTYPE_FLAG_MAPPABLE;
>> + man->available_caching = TTM_PL_FLAG_UNCACHED |
>> + TTM_PL_FLAG_WC;
>> + man->default_caching = TTM_PL_FLAG_WC;
>> + break;
>> + default:
>> + DRM_ERROR("Unsupported memory type %u\n", type);
>> + return -EINVAL;
>> + }
>> + return 0;
>> +}
>> +
>> +void hibmc_ttm_placement(struct hibmc_bo *bo, int domain)
>> +{
>> + u32 c = 0;
>
> Can you please use a more descriptive name than 'c'?
ok, will do that.
>
>> + u32 i;
>> +
>> + bo->placement.placement = bo->placements;
>> + bo->placement.busy_placement = bo->placements;
>> + if (domain & TTM_PL_FLAG_VRAM)
>> + bo->placements[c++].flags = TTM_PL_FLAG_WC |
>> + TTM_PL_FLAG_UNCACHED | TTM_PL_FLAG_VRAM;
>
> nit: you're alignment is off here and below
is it correct?
if (domain & TTM_PL_FLAG_VRAM)
bo->placements[c++].flags = TTM_PL_FLAG_WC |
TTM_PL_FLAG_UNCACHED | TTM_PL_FLAG_VRAM;
if (domain & TTM_PL_FLAG_SYSTEM)
bo->placements[c++].flags = TTM_PL_MASK_CACHING |
TTM_PL_FLAG_SYSTEM;
if (!c)
bo->placements[c++].flags = TTM_PL_MASK_CACHING |
TTM_PL_FLAG_SYSTEM;
>
>> + if (domain & TTM_PL_FLAG_SYSTEM)
>> + bo->placements[c++].flags = TTM_PL_MASK_CACHING |
>> + TTM_PL_FLAG_SYSTEM;
>> + if (!c)
>> + bo->placements[c++].flags = TTM_PL_MASK_CACHING |
>> + TTM_PL_FLAG_SYSTEM;
>> +
>> + bo->placement.num_placement = c;
>> + bo->placement.num_busy_placement = c;
>> + for (i = 0; i < c; ++i) {
>
> nit: we tend towards post-increment in kernel
agreed, thanks.
>
>> + bo->placements[i].fpfn = 0;
>> + bo->placements[i].lpfn = 0;
>> + }
>> +}
>> +
>> +static void
>> +hibmc_bo_evict_flags(struct ttm_buffer_object *bo, struct ttm_placement *pl)
>> +{
>> + struct hibmc_bo *hibmcbo = hibmc_bo(bo);
>> +
>> + if (!hibmc_ttm_bo_is_hibmc_bo(bo))
>> + return;
>> +
>> + hibmc_ttm_placement(hibmcbo, TTM_PL_FLAG_SYSTEM);
>> + *pl = hibmcbo->placement;
>> +}
>> +
>> +static int hibmc_bo_verify_access(struct ttm_buffer_object *bo,
>> + struct file *filp)
>> +{
>> + struct hibmc_bo *hibmcbo = hibmc_bo(bo);
>> +
>> + return drm_vma_node_verify_access(&hibmcbo->gem.vma_node,
>> + filp->private_data);
>> +}
>> +
>> +static int hibmc_ttm_io_mem_reserve(struct ttm_bo_device *bdev,
>> + struct ttm_mem_reg *mem)
>> +{
>> + struct ttm_mem_type_manager *man = &bdev->man[mem->mem_type];
>> + struct hibmc_drm_device *hibmc = hibmc_bdev(bdev);
>> +
>> + mem->bus.addr = NULL;
>> + mem->bus.offset = 0;
>> + mem->bus.size = mem->num_pages << PAGE_SHIFT;
>> + mem->bus.base = 0;
>> + mem->bus.is_iomem = false;
>> + if (!(man->flags & TTM_MEMTYPE_FLAG_MAPPABLE))
>> + return -EINVAL;
>> + switch (mem->mem_type) {
>> + case TTM_PL_SYSTEM:
>> + /* system memory */
>> + return 0;
>> + case TTM_PL_VRAM:
>> + mem->bus.offset = mem->start << PAGE_SHIFT;
>> + mem->bus.base = pci_resource_start(hibmc->dev->pdev, 0);
>> + mem->bus.is_iomem = true;
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> + return 0;
>> +}
>> +
>> +static void hibmc_ttm_io_mem_free(struct ttm_bo_device *bdev,
>> + struct ttm_mem_reg *mem)
>> +{
>> +}
>
> No need to stub this, the caller does a NULL-check before invoking
will delete it, thanks.
>
>> +
>> +static void hibmc_ttm_backend_destroy(struct ttm_tt *tt)
>> +{
>> + ttm_tt_fini(tt);
>> + kfree(tt);
>> +}
>> +
>> +static struct ttm_backend_func hibmc_tt_backend_func = {
>> + .destroy = &hibmc_ttm_backend_destroy,
>> +};
>> +
>> +static struct ttm_tt *hibmc_ttm_tt_create(struct ttm_bo_device *bdev,
>> + unsigned long size,
>> + u32 page_flags,
>> + struct page *dummy_read_page)
>> +{
>> + struct ttm_tt *tt;
>> +
>> + tt = kzalloc(sizeof(*tt), GFP_KERNEL);
>> + if (!tt)
>
> Print error
ok, will do that, thanks.
>
>> + return NULL;
>> + tt->func = &hibmc_tt_backend_func;
>> + if (ttm_tt_init(tt, bdev, size, page_flags, dummy_read_page)) {
>
> Here too?
ditto
>
>> + kfree(tt);
>> + return NULL;
>> + }
>> + return tt;
>> +}
>> +
>> +static int hibmc_ttm_tt_populate(struct ttm_tt *ttm)
>> +{
>> + return ttm_pool_populate(ttm);
>> +}
>> +
>> +static void hibmc_ttm_tt_unpopulate(struct ttm_tt *ttm)
>> +{
>> + ttm_pool_unpopulate(ttm);
>> +}
>> +
>> +struct ttm_bo_driver hibmc_bo_driver = {
>> + .ttm_tt_create = hibmc_ttm_tt_create,
>> + .ttm_tt_populate = hibmc_ttm_tt_populate,
>> + .ttm_tt_unpopulate = hibmc_ttm_tt_unpopulate,
>> + .init_mem_type = hibmc_bo_init_mem_type,
>> + .evict_flags = hibmc_bo_evict_flags,
>> + .move = NULL,
>> + .verify_access = hibmc_bo_verify_access,
>> + .io_mem_reserve = &hibmc_ttm_io_mem_reserve,
>> + .io_mem_free = &hibmc_ttm_io_mem_free,
>> + .lru_tail = &ttm_bo_default_lru_tail,
>> + .swap_lru_tail = &ttm_bo_default_swap_lru_tail,
>> +};
>> +
>> +int hibmc_mm_init(struct hibmc_drm_device *hibmc)
>> +{
>> + int ret;
>> + struct drm_device *dev = hibmc->dev;
>> + struct ttm_bo_device *bdev = &hibmc->ttm.bdev;
>> +
>> + ret = hibmc_ttm_global_init(hibmc);
>> + if (ret)
>> + return ret;
>> +
>> + ret = ttm_bo_device_init(&hibmc->ttm.bdev,
>> + hibmc->ttm.bo_global_ref.ref.object,
>> + &hibmc_bo_driver,
>> + dev->anon_inode->i_mapping,
>> + DRM_FILE_PAGE_OFFSET,
>> + true);
>> + if (ret) {
>
> Call hibmc_ttm_global_release here?
agreed, thanks for pointing it out.
>
>> + DRM_ERROR("Error initialising bo driver; %d\n", ret);
>> + return ret;
>> + }
>> +
>> + ret = ttm_bo_init_mm(bdev, TTM_PL_VRAM,
>> + hibmc->fb_size >> PAGE_SHIFT);
>> + if (ret) {
>
> Clean up here as well?
ditto
>
>> + DRM_ERROR("Failed ttm VRAM init: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + hibmc->mm_inited = true;
>> + return 0;
>> +}
>> +
>> +void hibmc_mm_fini(struct hibmc_drm_device *hibmc)
>> +{
>> + if (!hibmc->mm_inited)
>> + return;
>> +
>> + ttm_bo_device_release(&hibmc->ttm.bdev);
>> + hibmc_ttm_global_release(hibmc);
>> + hibmc->mm_inited = false;
>> +}
>> +
>> +int hibmc_bo_create(struct drm_device *dev, int size, int align,
>> + u32 flags, struct hibmc_bo **phibmcbo)
>> +{
>> + struct hibmc_drm_device *hibmc = dev->dev_private;
>> + struct hibmc_bo *hibmcbo;
>> + size_t acc_size;
>> + int ret;
>> +
>> + hibmcbo = kzalloc(sizeof(*hibmcbo), GFP_KERNEL);
>> + if (!hibmcbo)
>> + return -ENOMEM;
>> +
>> + ret = drm_gem_object_init(dev, &hibmcbo->gem, size);
>> + if (ret) {
>> + kfree(hibmcbo);
>> + return ret;
>> + }
>> +
>> + hibmcbo->bo.bdev = &hibmc->ttm.bdev;
>> +
>> + hibmc_ttm_placement(hibmcbo, TTM_PL_FLAG_VRAM | TTM_PL_FLAG_SYSTEM);
>> +
>> + acc_size = ttm_bo_dma_acc_size(&hibmc->ttm.bdev, size,
>> + sizeof(struct hibmc_bo));
>> +
>> + ret = ttm_bo_init(&hibmc->ttm.bdev, &hibmcbo->bo, size,
>> + ttm_bo_type_device, &hibmcbo->placement,
>> + align >> PAGE_SHIFT, false, NULL, acc_size,
>> + NULL, NULL, hibmc_bo_ttm_destroy);
>> + if (ret)
>
> Missing hibmcbo clean up here
i looked at all other ttm drivers and all of them return directly when ttm_bo_init
failed, however, i think it is better to clean up here, should i call
hibmc_bo_unref(&hibmc_bo) here ?
>
>> + return ret;
>> +
>> + *phibmcbo = hibmcbo;
>> + return 0;
>> +}
>> +
>> +static inline u64 hibmc_bo_gpu_offset(struct hibmc_bo *bo)
>> +{
>> + return bo->bo.offset;
>> +}
>
> I don't think this function provides any value
do you nean i use bo->bo.offset instead of calling hibmc_bo_gpu_offset()?
>
>> +
>> +int hibmc_bo_pin(struct hibmc_bo *bo, u32 pl_flag, u64 *gpu_addr)
>> +{
>> + int i, ret;
>> +
>> + if (bo->pin_count) {
>> + bo->pin_count++;
>> + if (gpu_addr)
>> + *gpu_addr = hibmc_bo_gpu_offset(bo);
>
> Are you missing a return here?
Thanks for pointing it out!
>
>> + }
>> +
>> + hibmc_ttm_placement(bo, pl_flag);
>> + for (i = 0; i < bo->placement.num_placement; i++)
>> + bo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT;
>> + ret = ttm_bo_validate(&bo->bo, &bo->placement, false, false);
>> + if (ret)
>> + return ret;
>> +
>> + bo->pin_count = 1;
>> + if (gpu_addr)
>> + *gpu_addr = hibmc_bo_gpu_offset(bo);
>> + return 0;
>> +}
>> +
>> +int hibmc_bo_push_sysram(struct hibmc_bo *bo)
>> +{
>> + int i, ret;
>> +
>> + if (!bo->pin_count) {
>> + DRM_ERROR("unpin bad %p\n", bo);
>> + return 0;
>> + }
>> + bo->pin_count--;
>> + if (bo->pin_count)
>> + return 0;
>> +
>> + if (bo->kmap.virtual)
>
> ttm_bo_kunmap already does this check so you don't have to
agreed. will remove this condition.
>
>> + ttm_bo_kunmap(&bo->kmap);
>> +
>> + hibmc_ttm_placement(bo, TTM_PL_FLAG_SYSTEM);
>> + for (i = 0; i < bo->placement.num_placement ; i++)
>> + bo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT;
>> +
>> + ret = ttm_bo_validate(&bo->bo, &bo->placement, false, false);
>> + if (ret) {
>> + DRM_ERROR("pushing to VRAM failed\n");
>
> Print ret
ok, thanks.
>
>> + return ret;
>> + }
>> + return 0;
>> +}
>> +
>> +int hibmc_mmap(struct file *filp, struct vm_area_struct *vma)
>> +{
>> + struct drm_file *file_priv;
>> + struct hibmc_drm_device *hibmc;
>> +
>> + if (unlikely(vma->vm_pgoff < DRM_FILE_PAGE_OFFSET))
>> + return -EINVAL;
>> +
>> + file_priv = filp->private_data;
>> + hibmc = file_priv->minor->dev->dev_private;
>> + return ttm_bo_mmap(filp, vma, &hibmc->ttm.bdev);
>> +}
>> +
>> +int hibmc_gem_create(struct drm_device *dev, u32 size, bool iskernel,
>> + struct drm_gem_object **obj)
>> +{
>> + struct hibmc_bo *hibmcbo;
>> + int ret;
>> +
>> + *obj = NULL;
>> +
>> + size = PAGE_ALIGN(size);
>> + if (size == 0)
>
> Print error
ditto
>
>> + return -EINVAL;
>> +
>> + ret = hibmc_bo_create(dev, size, 0, 0, &hibmcbo);
>> + if (ret) {
>> + if (ret != -ERESTARTSYS)
>> + DRM_ERROR("failed to allocate GEM object\n");
>
> Print ret
ditto
>
>> + return ret;
>> + }
>> + *obj = &hibmcbo->gem;
>> + return 0;
>> +}
>> +
>> +int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev,
>> + struct drm_mode_create_dumb *args)
>> +{
>> + struct drm_gem_object *gobj;
>> + u32 handle;
>> + int ret;
>> +
>> + args->pitch = ALIGN(args->width * ((args->bpp + 7) / 8), 16);
>
> What's up with the bpp + 7 here? Perhaps you're looking for DIV_ROUND_UP?
Yes, that sounds sane.
>
>
>> + args->size = args->pitch * args->height;
>> +
>> + ret = hibmc_gem_create(dev, args->size, false,
>> + &gobj);
>> + if (ret)
>> + return ret;
>> +
>> + ret = drm_gem_handle_create(file, gobj, &handle);
>> + drm_gem_object_unreference_unlocked(gobj);
>> + if (ret)
>
> Print error here
agreed.
>
>> + return ret;
>> +
>> + args->handle = handle;
>> + return 0;
>> +}
>> +
>> +static void hibmc_bo_unref(struct hibmc_bo **bo)
>> +{
>> + struct ttm_buffer_object *tbo;
>> +
>> + if ((*bo) == NULL)
>> + return;
>> +
>> + tbo = &((*bo)->bo);
>> + ttm_bo_unref(&tbo);
>> + *bo = NULL;
>> +}
>> +
>> +void hibmc_gem_free_object(struct drm_gem_object *obj)
>> +{
>> + struct hibmc_bo *hibmcbo = gem_to_hibmc_bo(obj);
>> +
>> + hibmc_bo_unref(&hibmcbo);
>> +}
>> +
>> +static u64 hibmc_bo_mmap_offset(struct hibmc_bo *bo)
>> +{
>> + return drm_vma_node_offset_addr(&bo->bo.vma_node);
>> +}
>> +
>> +int hibmc_dumb_mmap_offset(struct drm_file *file, struct drm_device *dev,
>> + u32 handle, u64 *offset)
>> +{
>> + struct drm_gem_object *obj;
>> + struct hibmc_bo *bo;
>> +
>> + obj = drm_gem_object_lookup(file, handle);
>> + if (!obj)
>> + return -ENOENT;
>> +
>> + bo = gem_to_hibmc_bo(obj);
>> + *offset = hibmc_bo_mmap_offset(bo);
>> +
>> + drm_gem_object_unreference_unlocked(obj);
>> + return 0;
>> +}
Regards,
Rongrong.
>> --
>> 1.9.1
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> _______________________________________________
> linuxarm mailing list
> linuxarm at huawei.com
> http://rnd-openeuler.huawei.com/mailman/listinfo/linuxarm
>
> .
>
^ permalink raw reply
* [PATCH v5 03/23] usb: ulpi: Support device discovery via DT
From: Heikki Krogerus @ 2016-11-11 11:02 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161018015636.11701-4-stephen.boyd@linaro.org>
On Mon, Oct 17, 2016 at 06:56:16PM -0700, Stephen Boyd wrote:
> The qcom HSIC ULPI phy doesn't have any bits set in the vendor or
> product ID registers. This makes it impossible to make a ULPI
> driver match against the ID registers. Add support to discover
> the ULPI phys via DT help alleviate this problem. In the DT case,
> we'll look for a ULPI bus node underneath the device registering
> the ULPI viewport (or the parent of that device to support
> chipidea's device layout) and then match up the phy node
> underneath that with the ULPI device that's created.
>
> The side benefit of this is that we can use standard properties
> in the phy node like clks, regulators, gpios, etc. because we
> don't have firmware like ACPI to turn these things on for us. And
> we can use the DT phy binding to point our phy consumer to the
> phy provider.
>
> The ULPI bus code supports native enumeration by reading the
> vendor ID and product ID registers at device creation time, but
> we can't be certain that those register reads will succeed if the
> phy is not powered up. To avoid any problems with reading the ID
> registers before the phy is powered we fallback to DT matching
> when the ID reads fail.
>
> If the ULPI spec had some generic power sequencing for these
> registers we could put that into the ULPI bus layer and power up
> the device before reading the ID registers. Unfortunately this
> doesn't exist and the power sequence is usually device specific.
> By having the device matched up with DT we can avoid this
> problem.
>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Cc: <devicetree@vger.kernel.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org>
Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
--
heikki
^ permalink raw reply
* [PATCH] ARM: davinci: enable PM for DT boot
From: Sekhar Nori @ 2016-11-11 11:02 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <m2shr16dr7.fsf@baylibre.com>
On Tuesday 08 November 2016 11:43 PM, Kevin Hilman wrote:
> Hi Sekhar,
>
> Sekhar Nori <nsekhar@ti.com> writes:
>
>> On Wednesday 26 October 2016 03:17 AM, Kevin Hilman wrote:
>>> Currently system PM is only enabled for legacy (non-DT) boot. Enable
>>> for DT boot also.
>>>
>>> Tested on da850-lcdk using "rtcwake -m mem -s5 -d rtc0".
>>>
>>> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
>>> ---
>>> arch/arm/mach-davinci/da8xx-dt.c | 18 ++++++++++++++++++
>>> 1 file changed, 18 insertions(+)
>>>
>>> diff --git a/arch/arm/mach-davinci/da8xx-dt.c b/arch/arm/mach-davinci/da8xx-dt.c
>>> index c9f7e9274aa8..a8089fa40d86 100644
>>> --- a/arch/arm/mach-davinci/da8xx-dt.c
>>> +++ b/arch/arm/mach-davinci/da8xx-dt.c
>>> @@ -43,8 +43,26 @@ static struct of_dev_auxdata da850_auxdata_lookup[] __initdata = {
>>>
>>> #ifdef CONFIG_ARCH_DAVINCI_DA850
>>>
>>> +static struct davinci_pm_config da850_pm_pdata = {
>>> + .sleepcount = 128,
>>> +};
>>> +
>>> +static struct platform_device da850_pm_device = {
>>> + .name = "pm-davinci",
>>> + .dev = {
>>> + .platform_data = &da850_pm_pdata,
>>> + },
>>> + .id = -1,
>>> +};
>>> +
>>> static void __init da850_init_machine(void)
>>> {
>>> + int ret;
>>> +
>>> + ret = da850_register_pm(&da850_pm_device);
>>
>> I am not sure if it makes sense to keep the "pm device" around anymore.
>> I think for both DT and non-DT boot, we can get rid of the fake PM
>> device and combine da850_register_pm() and davinci_pm_probe() into a
>> single davinci_init_suspend() function which can then be called both for
>> DT and non-DT boot.
>
> Looking closer at this, where do you propose the pdata comes from for
> the non-DT boot?
>
> It seems to me that we can't currently remove the pdata dependency
> without breaking the non-DT platforms, so the approach proposed here is
> the least invasive.
There is a single value of sleep count that is used today (128). So I
was thinking we can hardcode that in pm.c. We are not going to add more
board files anyway so there is no risk here.
For future, if a different sleepcount value is needed, it will need to
be a new DT property.
Thanks,
Sekhar
^ permalink raw reply
* [PATCH] [media] mtk-mdp: allocate video_device dynamically
From: Hans Verkuil @ 2016-11-11 10:51 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478522529-57129-2-git-send-email-minghsiu.tsai@mediatek.com>
Almost correct:
On 11/07/2016 01:42 PM, Minghsiu Tsai wrote:
> It can fix known problems with embedded video_device structs.
>
> Signed-off-by: Minghsiu Tsai <minghsiu.tsai@mediatek.com>
> ---
> drivers/media/platform/mtk-mdp/mtk_mdp_core.h | 2 +-
> drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c | 33 ++++++++++++++++-----------
> 2 files changed, 21 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_core.h b/drivers/media/platform/mtk-mdp/mtk_mdp_core.h
> index 848569d..ad1cff3 100644
> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.h
> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.h
> @@ -167,7 +167,7 @@ struct mtk_mdp_dev {
> struct mtk_mdp_comp *comp[MTK_MDP_COMP_ID_MAX];
> struct v4l2_m2m_dev *m2m_dev;
> struct list_head ctx_list;
> - struct video_device vdev;
> + struct video_device *vdev;
> struct v4l2_device v4l2_dev;
> struct workqueue_struct *job_wq;
> struct platform_device *vpu_dev;
> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c b/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
> index 9a747e7..b8dee1c 100644
> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
> @@ -1236,16 +1236,22 @@ int mtk_mdp_register_m2m_device(struct mtk_mdp_dev *mdp)
> int ret;
>
> mdp->variant = &mtk_mdp_default_variant;
> - mdp->vdev.device_caps = V4L2_CAP_VIDEO_M2M_MPLANE | V4L2_CAP_STREAMING;
> - mdp->vdev.fops = &mtk_mdp_m2m_fops;
> - mdp->vdev.ioctl_ops = &mtk_mdp_m2m_ioctl_ops;
> - mdp->vdev.release = video_device_release_empty;
> - mdp->vdev.lock = &mdp->lock;
> - mdp->vdev.vfl_dir = VFL_DIR_M2M;
> - mdp->vdev.v4l2_dev = &mdp->v4l2_dev;
> - snprintf(mdp->vdev.name, sizeof(mdp->vdev.name), "%s:m2m",
> + mdp->vdev = video_device_alloc();
> + if (!mdp->vdev) {
> + dev_err(dev, "failed to allocate video device\n");
> + ret = -ENOMEM;
> + goto err_video_alloc;
> + }
> + mdp->vdev->device_caps = V4L2_CAP_VIDEO_M2M_MPLANE | V4L2_CAP_STREAMING;
> + mdp->vdev->fops = &mtk_mdp_m2m_fops;
> + mdp->vdev->ioctl_ops = &mtk_mdp_m2m_ioctl_ops;
> + mdp->vdev->release = video_device_release;
> + mdp->vdev->lock = &mdp->lock;
> + mdp->vdev->vfl_dir = VFL_DIR_M2M;
> + mdp->vdev->v4l2_dev = &mdp->v4l2_dev;
> + snprintf(mdp->vdev->name, sizeof(mdp->vdev->name), "%s:m2m",
> MTK_MDP_MODULE_NAME);
> - video_set_drvdata(&mdp->vdev, mdp);
> + video_set_drvdata(mdp->vdev, mdp);
>
> mdp->m2m_dev = v4l2_m2m_init(&mtk_mdp_m2m_ops);
> if (IS_ERR(mdp->m2m_dev)) {
> @@ -1254,26 +1260,27 @@ int mtk_mdp_register_m2m_device(struct mtk_mdp_dev *mdp)
> goto err_m2m_init;
> }
>
> - ret = video_register_device(&mdp->vdev, VFL_TYPE_GRABBER, 2);
> + ret = video_register_device(mdp->vdev, VFL_TYPE_GRABBER, 2);
> if (ret) {
> dev_err(dev, "failed to register video device\n");
> goto err_vdev_register;
> }
>
> v4l2_info(&mdp->v4l2_dev, "driver registered as /dev/video%d",
> - mdp->vdev.num);
> + mdp->vdev->num);
> return 0;
>
> err_vdev_register:
> v4l2_m2m_release(mdp->m2m_dev);
> err_m2m_init:
> - video_device_release(&mdp->vdev);
> + video_unregister_device(mdp->vdev);
This should remain video_device_release: the video_register_device call failed, so
the device hasn't been registered. In that case just release the device (i.e.
free the allocated memory).
> +err_video_alloc:
>
> return ret;
> }
>
> void mtk_mdp_unregister_m2m_device(struct mtk_mdp_dev *mdp)
> {
> - video_device_release(&mdp->vdev);
> + video_unregister_device(mdp->vdev);
> v4l2_m2m_release(mdp->m2m_dev);
> }
>
Regards,
Hans
^ permalink raw reply
* [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06
From: liviu.dudau at arm.com @ 2016-11-11 10:48 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <10334260.ztLXZ2Oynd@wuerfel>
Hi Arnd,
On Thu, Nov 10, 2016 at 05:07:21PM +0100, Arnd Bergmann wrote:
> On Thursday, November 10, 2016 3:36:49 PM CET Gabriele Paoloni wrote:
> >
> > Where should we get the range from? For LPC we know that it is going
> > Work on anything that is not used by PCI I/O space, and this is
> > why we use [0, PCIBIOS_MIN_IO]
>
> It should be allocated the same way we allocate PCI config space
> segments. This is currently done with the io_range list in
> drivers/pci/pci.c, which isn't perfect but could be extended
> if necessary. Based on what others commented here, I'd rather
> make the differences between ISA/LPC and PCI I/O ranges smaller
> than larger.
>
> > > Your current version has
> > >
> > > if (arm64_extio_ops->pfout) \
> > > arm64_extio_ops->pfout(arm64_extio_ops->devpara,\
> > > addr, value, sizeof(type)); \
> > >
> > > Instead, just subtract the start of the range from the logical
> > > port number to transform it back into a bus-local port number:
> >
> > These accessors do not operate on IO tokens:
> >
> > If (arm64_extio_ops->start > addr || arm64_extio_ops->end < addr)
> > addr is not going to be an I/O token; in fact patch 2/3 imposes that
> > the I/O tokens will start at PCIBIOS_MIN_IO. So from 0 to PCIBIOS_MIN_IO
> > we have free physical addresses that the accessors can operate on.
>
> Ah, I missed that part. I'd rather not use PCIBIOS_MIN_IO to refer to
> the logical I/O tokens, the purpose of that macro is really meant
> for allocating PCI I/O port numbers within the address space of
> one bus.
>
> Note that it's equally likely that whichever next platform needs
> non-mapped I/O access like this actually needs them for PCI I/O space,
> and that will use it on addresses registered to a PCI host bridge.
>
> If we separate the two steps:
>
> a) assign a range of logical I/O port numbers to a bus
Except that currently when we add ranges to io_range_list we don't have
a bus number yet, because the parsing happens before the host bridge
has been created. Maybe register_io_range() can take a bus number as an
argument, but I'm not sure how we are going to use that in pci_pio_to_address()
or pci_address_to_pio().
Best regards,
Liviu
> b) register a set of helpers for redirecting logical I/O
> port to a helper function
>
> then I think the code will get cleaner and more flexible.
> It should actually then be able to replace the powerpc
> specific implementation.
>
> Arnd
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
?\_(?)_/?
^ permalink raw reply
* [PATCH v2 2/5] ARM: bus: da8xx-mstpri: new driver
From: Sekhar Nori @ 2016-11-11 10:47 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161109182433.vskk5rn7hfxshfy6@rob-hp-laptop>
On Wednesday 09 November 2016 11:54 PM, Rob Herring wrote:
> On Mon, Oct 31, 2016 at 03:45:35PM +0100, Bartosz Golaszewski wrote:
>> Create the driver for the da8xx master peripheral priority
>> configuration and implement support for writing to the three
>> Master Priority registers on da850 SoCs.
>>
>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>> ---
>> .../devicetree/bindings/bus/ti,da850-mstpri.txt | 20 ++
>
> Acked-by: Rob Herring <robh@kernel.org>
Applied to v4.10/drivers
Thanks,
Sekhar
^ permalink raw reply
* [PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver
From: Sekhar Nori @ 2016-11-11 10:47 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161109182430.yh6uqeu2ufzkctww@rob-hp-laptop>
On Wednesday 09 November 2016 11:54 PM, Rob Herring wrote:
> On Mon, Oct 31, 2016 at 03:45:34PM +0100, Bartosz Golaszewski wrote:
>> Create a new driver for the da8xx DDR2/mDDR controller and implement
>> support for writing to the Peripheral Bus Burst Priority Register.
>>
>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>> ---
>> .../memory-controllers/ti-da8xx-ddrctl.txt | 20 +++
>
> Acked-by: Rob Herring <robh@kernel.org>
Applied to v4.10/drivers
Thanks,
Sekhar
^ permalink raw reply
* [PATCH v4 1/5] arm64: perf: Basic uncore counter support for Cavium ThunderX SOC
From: Jan Glauber @ 2016-11-11 10:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161110165405.GH4418@leverpostej>
Hi Mark,
thanks for reviewing. One question below, for most of your other comments
I think we need to come to a conclusion about the aggregation first.
On Thu, Nov 10, 2016 at 04:54:06PM +0000, Mark Rutland wrote:
> Hi Jan,
>
> Apologies for the delay in getting to this.
>
> On Sat, Oct 29, 2016 at 01:55:29PM +0200, Jan Glauber wrote:
> > diff --git a/drivers/perf/uncore/uncore_cavium.c b/drivers/perf/uncore/uncore_cavium.c
> > new file mode 100644
> > index 0000000..a7b4277
> > --- /dev/null
> > +++ b/drivers/perf/uncore/uncore_cavium.c
> > @@ -0,0 +1,351 @@
> > +/*
> > + * Cavium Thunder uncore PMU support.
> > + *
> > + * Copyright (C) 2015,2016 Cavium Inc.
> > + * Author: Jan Glauber <jan.glauber@cavium.com>
> > + */
> > +
> > +#include <linux/cpufeature.h>
> > +#include <linux/numa.h>
> > +#include <linux/slab.h>
>
> I believe the following includes are necessary for APIs and/or data
> explicitly referenced by the driver code:
>
> #include <linux/atomic.h>
> #include <linux/cpuhotplug.h>
> #include <linux/cpumask.h>
> #include <linux/device.h>
> #include <linux/errno.h>
> #include <linux/io.h>
> #include <linux/kernel.h>
> #include <linux/list.h>
> #include <linux/pci.h>
> #include <linux/perf_event.h>
> #include <linux/printk.h>
> #include <linux/smp.h>
> #include <linux/sysfs.h>
> #include <linux/types.h>
>
> #include <asm/local64.h>
>
> ... please add those here.
Should I also add includes that are already in the included by uncore_cavium.h?
I usually avoid includes that come through the "local" header file.
^ permalink raw reply
* [PATCH 0/6] mfd: audit and demodule drivers/mfd/ab* drivers
From: Lee Jones @ 2016-11-11 10:38 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161030012440.10495-1-paul.gortmaker@windriver.com>
On Sat, 29 Oct 2016, Paul Gortmaker wrote:
> Nothing new here ; just another instance where we had modular remove
> and exit functions -- but the Makefile/Kconfig limited the use case to
> being always built-in; hence the code is useless and needs removing.
>
> So we remove the dead code and the misleading hints of modularity from
> these drivers, hoping that we manage to prevent some new drivers from
> copying these same old mistakes into pending new drivers.
>
> Paul.
> ---
>
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Mattias Wallin <mattias.wallin@stericsson.com>
> Cc: Samuel Ortiz <sameo@linux.intel.com>
> Cc: linux-arm-kernel at lists.infradead.org
>
> Paul Gortmaker (6):
> mfd: ab3100-core: Make it explicitly non-modular
> mfd: ab8500-core: Make it explicitly non-modular
> mfd: ab8500-debugfs: Make it explicitly non-modular
> mfd: ab8500-gpadc: Make it explicitly non-modular
> mfd: ab8500: make sysctrl explicitly non-modular
> mfd: abx500-core: drop unused MODULE_ tags from non-modular code
Applied the series with Linus' Ack.
> drivers/mfd/ab3100-core.c | 39 +++------------------------------------
> drivers/mfd/ab8500-core.c | 37 ++++++-------------------------------
> drivers/mfd/ab8500-debugfs.c | 21 ++-------------------
> drivers/mfd/ab8500-gpadc.c | 19 ++-----------------
> drivers/mfd/ab8500-sysctrl.c | 9 ++++-----
> drivers/mfd/abx500-core.c | 7 ++-----
> 6 files changed, 19 insertions(+), 113 deletions(-)
>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* [PATCH V5 2/3] ARM64 LPC: Add missing range exception for special ISA
From: liviu.dudau at arm.com @ 2016-11-11 10:37 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <EE11001F9E5DDD47B7634E2F8A612F2E1F8F5F06@lhreml507-mbx>
On Thu, Nov 10, 2016 at 04:06:40PM +0000, Gabriele Paoloni wrote:
> Hi Liviu
>
> > -----Original Message-----
> > From: liviu.dudau at arm.com [mailto:liviu.dudau at arm.com]
> > Sent: 09 November 2016 16:51
> > To: Gabriele Paoloni
> > Cc: Yuanzhichang; catalin.marinas at arm.com; will.deacon at arm.com;
> > robh+dt at kernel.org; bhelgaas at google.com; mark.rutland at arm.com;
> > olof at lixom.net; arnd at arndb.de; linux-arm-kernel at lists.infradead.org;
> > lorenzo.pieralisi at arm.com; linux-kernel at vger.kernel.org; Linuxarm;
> > devicetree at vger.kernel.org; linux-pci at vger.kernel.org; linux-
> > serial at vger.kernel.org; minyard at acm.org; benh at kernel.crashing.org;
> > zourongrong at gmail.com; John Garry; zhichang.yuan02 at gmail.com;
> > kantyzc at 163.com; xuwei (O)
> > Subject: Re: [PATCH V5 2/3] ARM64 LPC: Add missing range exception for
> > special ISA
> >
> > On Wed, Nov 09, 2016 at 04:16:17PM +0000, Gabriele Paoloni wrote:
> > > Hi Liviu
> > >
> > > Thanks for reviewing
> > >
> >
> > [removed some irrelevant part of discussion, avoid crazy formatting]
> >
> > > > > +/**
> > > > > + * addr_is_indirect_io - check whether the input taddr is for
> > > > indirectIO.
> > > > > + * @taddr: the io address to be checked.
> > > > > + *
> > > > > + * Returns 1 when taddr is in the range; otherwise return 0.
> > > > > + */
> > > > > +int addr_is_indirect_io(u64 taddr)
> > > > > +{
> > > > > + if (arm64_extio_ops->start > taddr || arm64_extio_ops->end
> > <
> > > > taddr)
> > > >
> > > > start >= taddr ?
> > >
> > > Nope... if (taddr < arm64_extio_ops->start || taddr >
> > arm64_extio_ops->end)
> > > then taddr is outside the range [start; end] and will return 0;
> > otherwise
> > > it will return 1...
> >
> > Oops, sorry, did not pay attention to the returned value. The check is
> > correct as it is, no need to change then.
> >
> > >
> > > >
> > > > > + return 0;
> > > > > +
> > > > > + return 1;
> > > > > +}
> > > > >
> > > > > BUILD_EXTIO(b, u8)
> > > > >
> > > > > diff --git a/drivers/of/address.c b/drivers/of/address.c
> > > > > index 02b2903..cc2a05d 100644
> > > > > --- a/drivers/of/address.c
> > > > > +++ b/drivers/of/address.c
> > > > > @@ -479,6 +479,50 @@ static int of_empty_ranges_quirk(struct
> > > > device_node *np)
> > > > > return false;
> > > > > }
> > > > >
> > > > > +
> > > > > +/*
> > > > > + * of_isa_indirect_io - get the IO address from some isa reg
> > > > property value.
> > > > > + * For some isa/lpc devices, no ranges property in ancestor
> > node.
> > > > > + * The device addresses are described directly in their regs
> > > > property.
> > > > > + * This fixup function will be called to get the IO address of
> > > > isa/lpc
> > > > > + * devices when the normal of_translation failed.
> > > > > + *
> > > > > + * @parent: points to the parent dts node;
> > > > > + * @bus: points to the of_bus which can be used to parse
> > > > address;
> > > > > + * @addr: the address from reg property;
> > > > > + * @na: the address cell counter of @addr;
> > > > > + * @presult: store the address paresed from @addr;
> > > > > + *
> > > > > + * return 1 when successfully get the I/O address;
> > > > > + * 0 will return for some failures.
> > > >
> > > > Bah, you are returning a signed int, why 0 for failure? Return a
> > > > negative value with
> > > > error codes. Otherwise change the return value into a bool.
> > >
> > > Yes we'll move to bool
> > >
> > > >
> > > > > + */
> > > > > +static int of_get_isa_indirect_io(struct device_node *parent,
> > > > > + struct of_bus *bus, __be32 *addr,
> > > > > + int na, u64 *presult)
> > > > > +{
> > > > > + unsigned int flags;
> > > > > + unsigned int rlen;
> > > > > +
> > > > > + /* whether support indirectIO */
> > > > > + if (!indirect_io_enabled())
> > > > > + return 0;
> > > > > +
> > > > > + if (!of_bus_isa_match(parent))
> > > > > + return 0;
> > > > > +
> > > > > + flags = bus->get_flags(addr);
> > > > > + if (!(flags & IORESOURCE_IO))
> > > > > + return 0;
> > > > > +
> > > > > + /* there is ranges property, apply the normal translation
> > > > directly. */
> > > >
> > > > s/there is ranges/if we have a 'ranges'/
> > >
> > > Thanks for spotting this
> > >
> > > >
> > > > > + if (of_get_property(parent, "ranges", &rlen))
> > > > > + return 0;
> > > > > +
> > > > > + *presult = of_read_number(addr + 1, na - 1);
> > > > > + /* this fixup is only valid for specific I/O range. */
> > > > > + return addr_is_indirect_io(*presult);
> > > > > +}
> > > > > +
> > > > > static int of_translate_one(struct device_node *parent, struct
> > > > of_bus *bus,
> > > > > struct of_bus *pbus, __be32 *addr,
> > > > > int na, int ns, int pna, const char *rprop)
> > > > > @@ -595,6 +639,15 @@ static u64 __of_translate_address(struct
> > > > device_node *dev,
> > > > > result = of_read_number(addr, na);
> > > > > break;
> > > > > }
> > > > > + /*
> > > > > + * For indirectIO device which has no ranges
> > property, get
> > > > > + * the address from reg directly.
> > > > > + */
> > > > > + if (of_get_isa_indirect_io(dev, bus, addr, na,
> > &result)) {
> > > > > + pr_debug("isa indirectIO matched(%s)..addr =
> > > > 0x%llx\n",
> > > > > + of_node_full_name(dev), result);
> > > > > + break;
> > > > > + }
> > > > >
> > > > > /* Get new parent bus and counts */
> > > > > pbus = of_match_bus(parent);
> > > > > @@ -688,8 +741,9 @@ static int __of_address_to_resource(struct
> > > > device_node *dev,
> > > > > if (taddr == OF_BAD_ADDR)
> > > > > return -EINVAL;
> > > > > memset(r, 0, sizeof(struct resource));
> > > > > - if (flags & IORESOURCE_IO) {
> > > > > + if (flags & IORESOURCE_IO && taddr >= PCIBIOS_MIN_IO) {
> > > > > unsigned long port;
> > > > > +
> > > > > port = pci_address_to_pio(taddr);
> > > > > if (port == (unsigned long)-1)
> > > > > return -EINVAL;
> > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > > index ba34907..1a08511 100644
> > > > > --- a/drivers/pci/pci.c
> > > > > +++ b/drivers/pci/pci.c
> > > > > @@ -3263,7 +3263,7 @@ int __weak
> > pci_register_io_range(phys_addr_t
> > > > addr, resource_size_t size)
> > > > >
> > > > > #ifdef PCI_IOBASE
> > > > > struct io_range *range;
> > > > > - resource_size_t allocated_size = 0;
> > > > > + resource_size_t allocated_size = PCIBIOS_MIN_IO;
> > > > >
> > > > > /* check if the range hasn't been previously recorded */
> > > > > spin_lock(&io_range_lock);
> > > > > @@ -3312,7 +3312,7 @@ phys_addr_t pci_pio_to_address(unsigned
> > long
> > > > pio)
> > > > >
> > > > > #ifdef PCI_IOBASE
> > > > > struct io_range *range;
> > > > > - resource_size_t allocated_size = 0;
> > > > > + resource_size_t allocated_size = PCIBIOS_MIN_IO;
> > > >
> > > > Have you checked that pci_pio_to_address still returns valid values
> > > > after this? I know that
> > > > you are trying to take into account PCIBIOS_MIN_IO limit when
> > > > allocating reserving the IO ranges,
> > > > but the values added in the io_range_list are still starting from
> > zero,
> > > > no from PCIBIOS_MIN_IO,
> > >
> > > I think you're wrong here as in pci_address_to_pio we have:
> > > + resource_size_t offset = PCIBIOS_MIN_IO;
> > >
> > > This should be enough to guarantee that the PIOs start at
> > > PCIBIOS_MIN_IO...right?
> >
> > I don't think you can guarantee that the pio value that gets passed
> > into
> > pci_pio_to_address() always comes from a previously returned value by
> > pci_address_to_pio(). Maybe you can add a check in pci_pio_to_address()
>
> Maybe I am missing something...could you make an exampleof a case
> where an IO toke doesn?t come from pci_address_to_pio() ?
Don't know, maybe it is coming from some DT or platform data? I was just
asking for confirmation that no one has seen issues there, I'm not saying
I've seen it happening.
Best regards,
Liviu
>
> Thanks
>
> Gab
>
>
> >
> > if (pio < PCIBIOS_MIN_IO)
> > return address;
> >
> > to avoid adding more checks in the list_for_each_entry() loop.
> >
> > Best regards,
> > Liviu
> >
> > >
> > >
> > > > so the calculation of the address in this function could return
> > > > negative values casted to pci_addr_t.
> > > >
> > > > Maybe you want to adjust the range->start value in
> > > > pci_register_io_range() as well to have it
> > > > offset by PCIBIOS_MIN_IO as well.
> > > >
> > > > Best regards,
> > > > Liviu
> > > >
> > > > >
> > > > > if (pio > IO_SPACE_LIMIT)
> > > > > return address;
> > > > > @@ -3335,7 +3335,7 @@ unsigned long __weak
> > > > pci_address_to_pio(phys_addr_t address)
> > > > > {
> > > > > #ifdef PCI_IOBASE
> > > > > struct io_range *res;
> > > > > - resource_size_t offset = 0;
> > > > > + resource_size_t offset = PCIBIOS_MIN_IO;
> > > > > unsigned long addr = -1;
> > > > >
> > > > > spin_lock(&io_range_lock);
> > > > > diff --git a/include/linux/of_address.h
> > b/include/linux/of_address.h
> > > > > index 3786473..deec469 100644
> > > > > --- a/include/linux/of_address.h
> > > > > +++ b/include/linux/of_address.h
> > > > > @@ -24,6 +24,23 @@ struct of_pci_range {
> > > > > #define for_each_of_pci_range(parser, range) \
> > > > > for (; of_pci_range_parser_one(parser, range);)
> > > > >
> > > > > +
> > > > > +#ifndef indirect_io_enabled
> > > > > +#define indirect_io_enabled indirect_io_enabled
> > > > > +static inline bool indirect_io_enabled(void)
> > > > > +{
> > > > > + return false;
> > > > > +}
> > > > > +#endif
> > > > > +
> > > > > +#ifndef addr_is_indirect_io
> > > > > +#define addr_is_indirect_io addr_is_indirect_io
> > > > > +static inline int addr_is_indirect_io(u64 taddr)
> > > > > +{
> > > > > + return 0;
> > > > > +}
> > > > > +#endif
> > > > > +
> > > > > /* Translate a DMA address from device space to CPU space */
> > > > > extern u64 of_translate_dma_address(struct device_node *dev,
> > > > > const __be32 *in_addr);
> > > > > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > > > > index 0e49f70..7f6bbb6 100644
> > > > > --- a/include/linux/pci.h
> > > > > +++ b/include/linux/pci.h
> > > > > @@ -2130,4 +2130,12 @@ static inline bool pci_ari_enabled(struct
> > > > pci_bus *bus)
> > > > > /* provide the legacy pci_dma_* API */
> > > > > #include <linux/pci-dma-compat.h>
> > > > >
> > > > > +/*
> > > > > + * define this macro here to refrain from compilation error for
> > some
> > > > > + * platforms. Please keep this macro at the end of this header
> > file.
> > > > > + */
> > > > > +#ifndef PCIBIOS_MIN_IO
> > > > > +#define PCIBIOS_MIN_IO 0
> > > > > +#endif
> > > > > +
> > > > > #endif /* LINUX_PCI_H */
> > > > > --
> > > > > 1.9.1
> > > > >
> > > > > --
> > > > > To unsubscribe from this list: send the line "unsubscribe linux-
> > pci"
> > > > in
> > > > > the body of a message to majordomo at vger.kernel.org
> > > > > More majordomo info at http://vger.kernel.org/majordomo-
> > info.html
> >
> > --
> > ====================
> > | I would like to |
> > | fix the world, |
> > | but they're not |
> > | giving me the |
> > \ source code! /
> > ---------------
> > ?\_(?)_/?
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
?\_(?)_/?
^ permalink raw reply
* [PATCH v1 03/11] drivers: soc: hisi: Add support for Hisilicon Djtag driver
From: Anurup M @ 2016-11-11 10:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <d009dcdc-2258-ed6a-53be-2211c7493051@huawei.com>
On Wednesday 09 November 2016 02:36 PM, John Garry wrote:
>
>>>>> I'd suggest requiring #address-cells=<1> and #size-cells=<0> in the
>>>>> master
>>>>> node, and listing the children by reg property. If the address is not
>>>>> easily expressed as a single integer, use a larger #address-cells
>>>>> value.
>>>> We already have something equivalent to reg in "module-id" (see patch
>>>> 02/11), which is the slave device bus address; here's a sample:
>>>> + /* For L3 cache PMU */
>>>> + pmul3c0 {
>>>> + compatible = "hisilicon,hisi-pmu-l3c-v1";
>>>> + scl-id = <0x02>;
>>>> + num-events = <0x16>;
>>>> + num-counters = <0x08>;
>>>> + module-id = <0x04>;
>>>> + num-banks = <0x04>;
>>>> + cfgen-map = <0x02 0x04 0x01 0x08>;
>>>> + counter-reg = <0x170>;
>>>> + evctrl-reg = <0x04>;
>>>> + event-en = <0x1000000>;
>>>> + evtype-reg = <0x140>;
>>>> + };
>>>>
>>>> FYI, "module-id" is our own internal hw nomenclature.
>>> Yes, that was my interpretation as well. Please use the standard
>>> "reg" property for this then.
>> Hi Arnd,
>>
>> Firstly my apologies for a mistake in the bindings example in ([PATCH
>> 02/11 ..]).
>> The module-id property is a list as defined in the PMU bindings patch
>> ([PATCH v1 05/11] dt-bindings .. <https://lkml.org/lkml/2016/11/2/323>).
>>
>> + djtag0: djtag at 0 {
>> + compatible = "hisilicon,hip05-cpu-djtag-v1";
>> + pmul3c0 {
>> + compatible = "hisilicon,hisi-pmu-l3c-v1";
>> + scl-id = <0x02>;
>> + num-events = <0x16>;
>> + num-counters = <0x08>;
>> + module-id = <0x04 0x04 0x04 0x04>;
>> + num-banks = <0x04>;
>> + cfgen-map = <0x02 0x04 0x01 0x08>;
>> + counter-reg = <0x170>;
>> + evctrl-reg = <0x04>;
>> + event-en = <0x1000000>;
>> + evtype-reg = <0x140>;
>> + };
>>
>>
>> The L3 cache in hip05/06/07 chips consist of 4 banks (each bank has PMU
>> registers).
>>
>> In hip05/06 all L3 cache banks are identified with same module-id.
>> module-id = <0x04 0x04 0x04 0x04>;
>>
>> But in the case hip07 chip(djtag v2), each L3 cache bank has different
>> module-id
>> module-id = <0x01 0x02 0x03 0x04>;
>>
>> So in this case Please share your opinion on how to model it.
>>
>
> My suggestion is to have a single PMU per module, whether that is 4
> banks or 1 bank per module, as this makes the driver simpler.
>
> I think you mentioned that a separate PMU per bank does not make much
> sense, and you would rather treat all banks as a single bank and
> aggregrate their perf statstics under a single PMU: Can you just use a
> script in userspace which can do this aggregration work if you have
> separate PMUs?
Hi John,
Mark also suggest the same view.
I have some concerns or doubts in having separate PMU for each L3 cache
bank.
We can discuss it in the same thread [[RESEND PATCH v1 07/11]]
<http://www.spinics.net/lists/arm-kernel/msg541938.html>].
Thanks,
Anurup
>
> Maybe perf guys have a view on this also.
>
> John
>
>> Some more detail of L3 cache PMU.
>> ------------------------------------------------
>> The hip05/06/07 chips consists of a multiple Super CPU cluster (16 CPU
>> cores). we call it SCCL.
>> The L3 cache( 4 banks) is shared by all CPU cores in a SCCL.
>> Each L3 cache bank has PMU registers. We always take the sum of the
>> counters to show in perf.
>> Taking individual L3 cache count is not meaningful as there is no
>> mapping of CPU cores to individual
>> L3 cache banks.
>>
>> Please share your suggestion.
>>
>> Thanks,
>> Anurup
>
^ permalink raw reply
* [PATCH v4 1/5] arm64: perf: Basic uncore counter support for Cavium ThunderX SOC
From: Jan Glauber @ 2016-11-11 10:30 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161108235010.GC17771@arm.com>
Hi Will,
thanks for the review!
On Tue, Nov 08, 2016 at 11:50:10PM +0000, Will Deacon wrote:
> Hi Jan,
>
> Thanks for posting an updated series. I have a few minor comments, which
> we can hopefully address in time for 4.10.
>
> Also, have you run the perf fuzzer with this series applied?
No, that's new to me. Will try it.
> https://github.com/deater/perf_event_tests
>
> (build the tests and look under the fuzzer/ directory for the tool)
>
> On Sat, Oct 29, 2016 at 01:55:29PM +0200, Jan Glauber wrote:
> > Provide "uncore" facilities for different non-CPU performance
> > counter units.
> >
> > The uncore PMUs can be found under /sys/bus/event_source/devices.
> > All counters are exported via sysfs in the corresponding events
> > files under the PMU directory so the perf tool can list the event names.
> >
> > There are some points that are special in this implementation:
> >
> > 1) The PMU detection relies on PCI device detection. If a
> > matching PCI device is found the PMU is created. The code can deal
> > with multiple units of the same type, e.g. more than one memory
> > controller.
> >
> > 2) Counters are summarized across different units of the same type
> > on one NUMA node but not across NUMA nodes.
> > For instance L2C TAD 0..7 are presented as a single counter
> > (adding the values from TAD 0 to 7). Although losing the ability
> > to read a single value the merged values are easier to use.
> >
> > 3) The counters are not CPU related. A random CPU is picked regardless
> > of the NUMA node. There is a small performance penalty for accessing
> > counters on a remote note but reading a performance counter is a
> > slow operation anyway.
> >
> > Signed-off-by: Jan Glauber <jglauber@cavium.com>
> > ---
> > drivers/perf/Kconfig | 13 ++
> > drivers/perf/Makefile | 1 +
> > drivers/perf/uncore/Makefile | 1 +
> > drivers/perf/uncore/uncore_cavium.c | 351 ++++++++++++++++++++++++++++++++++++
> > drivers/perf/uncore/uncore_cavium.h | 71 ++++++++
>
> We already have "uncore" PMUs under drivers/perf, so I'd prefer that we
> renamed this a bit to reflect better what's going on. How about:
>
> drivers/perf/cavium/
>
> and then
>
> drivers/perf/cavium/uncore_thunder.[ch]
>
> ?
OK, agreed.
> > include/linux/cpuhotplug.h | 1 +
> > 6 files changed, 438 insertions(+)
> > create mode 100644 drivers/perf/uncore/Makefile
> > create mode 100644 drivers/perf/uncore/uncore_cavium.c
> > create mode 100644 drivers/perf/uncore/uncore_cavium.h
> >
> > diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
> > index 4d5c5f9..3266c87 100644
> > --- a/drivers/perf/Kconfig
> > +++ b/drivers/perf/Kconfig
> > @@ -19,4 +19,17 @@ config XGENE_PMU
> > help
> > Say y if you want to use APM X-Gene SoC performance monitors.
> >
> > +config UNCORE_PMU
> > + bool
>
> This isn't needed.
I thought about a symbol for all uncore pmus. But when drivers/perf is
already that it can be removed.
> > +
> > +config UNCORE_PMU_CAVIUM
> > + depends on PERF_EVENTS && NUMA && ARM64
> > + bool "Cavium uncore PMU support"
>
> Please mentioned Thunder somewhere, since that's the SoC being supported.
OK.
> > + select UNCORE_PMU
> > + default y
> > + help
> > + Say y if you want to access performance counters of subsystems
> > + on a Cavium SOC like cache controller, memory controller or
> > + processor interconnect.
> > +
> > endmenu
> > diff --git a/drivers/perf/Makefile b/drivers/perf/Makefile
> > index b116e98..d6c02c9 100644
> > --- a/drivers/perf/Makefile
> > +++ b/drivers/perf/Makefile
> > @@ -1,2 +1,3 @@
> > obj-$(CONFIG_ARM_PMU) += arm_pmu.o
> > obj-$(CONFIG_XGENE_PMU) += xgene_pmu.o
> > +obj-y += uncore/
> > diff --git a/drivers/perf/uncore/Makefile b/drivers/perf/uncore/Makefile
> > new file mode 100644
> > index 0000000..6130e18
> > --- /dev/null
> > +++ b/drivers/perf/uncore/Makefile
> > @@ -0,0 +1 @@
> > +obj-$(CONFIG_UNCORE_PMU_CAVIUM) += uncore_cavium.o
> > diff --git a/drivers/perf/uncore/uncore_cavium.c b/drivers/perf/uncore/uncore_cavium.c
> > new file mode 100644
> > index 0000000..a7b4277
> > --- /dev/null
> > +++ b/drivers/perf/uncore/uncore_cavium.c
> > @@ -0,0 +1,351 @@
> > +/*
> > + * Cavium Thunder uncore PMU support.
> > + *
> > + * Copyright (C) 2015,2016 Cavium Inc.
> > + * Author: Jan Glauber <jan.glauber@cavium.com>
> > + */
> > +
> > +#include <linux/cpufeature.h>
> > +#include <linux/numa.h>
> > +#include <linux/slab.h>
> > +
> > +#include "uncore_cavium.h"
> > +
> > +/*
> > + * Some notes about the various counters supported by this "uncore" PMU
> > + * and the design:
> > + *
> > + * All counters are 64 bit long.
> > + * There are no overflow interrupts.
> > + * Counters are summarized per node/socket.
> > + * Most devices appear as separate PCI devices per socket with the exception
> > + * of OCX TLK which appears as one PCI device per socket and contains several
> > + * units with counters that are merged.
> > + * Some counters are selected via a control register (L2C TAD) and read by
> > + * a number of counter registers, others (L2C CBC, LMC & OCX TLK) have
> > + * one dedicated counter per event.
> > + * Some counters are not stoppable (L2C CBC & LMC).
> > + * Some counters are read-only (LMC).
> > + * All counters belong to PCI devices, the devices may have additional
> > + * drivers but we assume we are the only user of the counter registers.
> > + * We map the whole PCI BAR so we must be careful to forbid access to
> > + * addresses that contain neither counters nor counter control registers.
> > + */
> > +
> > +void thunder_uncore_read(struct perf_event *event)
> > +{
>
> Rather than have a bunch of global symbols that are called from the
> individual drivers, why don't you pass a struct of function pointers to
> their respective init functions and keep the internals private?
Most of these functions are already in struct pmu. What I can do is
set the shared functions in thunder_uncore_setup as default, and
only override them as needed (like thunder_uncore_read_ocx_tlk)
or the other way around (use default if not set already).
Then I can get rid of them.
> > + struct thunder_uncore *uncore = to_uncore(event->pmu);
> > + struct hw_perf_event *hwc = &event->hw;
> > + struct thunder_uncore_node *node;
> > + struct thunder_uncore_unit *unit;
> > + u64 prev, delta, new = 0;
> > +
> > + node = get_node(hwc->config, uncore);
> > +
> > + /* read counter values from all units on the node */
> > + list_for_each_entry(unit, &node->unit_list, entry)
> > + new += readq(hwc->event_base + unit->map);
> > +
> > + prev = local64_read(&hwc->prev_count);
> > + local64_set(&hwc->prev_count, new);
> > + delta = new - prev;
> > + local64_add(delta, &event->count);
> > +}
> > +
> > +int thunder_uncore_add(struct perf_event *event, int flags, u64 config_base,
> > + u64 event_base)
> > +{
> > + struct thunder_uncore *uncore = to_uncore(event->pmu);
> > + struct hw_perf_event *hwc = &event->hw;
> > + struct thunder_uncore_node *node;
> > + int id;
> > +
> > + node = get_node(hwc->config, uncore);
> > + id = get_id(hwc->config);
> > +
> > + if (!cmpxchg(&node->events[id], NULL, event))
> > + hwc->idx = id;
>
> Does this need to be a full-fat cmpxchg? Who are you racing with?
Just copy'n'paste from the existing drivers. I guess it can be relaxed.
> > +
> > + if (hwc->idx == -1)
> > + return -EBUSY;
>
> This would be much clearer as an else statement after the cmpxchg.
Agreed.
> > +
> > + hwc->config_base = config_base;
> > + hwc->event_base = event_base;
> > + hwc->state = PERF_HES_UPTODATE | PERF_HES_STOPPED;
> > +
> > + if (flags & PERF_EF_START)
> > + uncore->pmu.start(event, PERF_EF_RELOAD);
> > +
> > + return 0;
> > +}
> > +
> > +void thunder_uncore_del(struct perf_event *event, int flags)
> > +{
> > + struct thunder_uncore *uncore = to_uncore(event->pmu);
> > + struct hw_perf_event *hwc = &event->hw;
> > + struct thunder_uncore_node *node;
> > + int i;
> > +
> > + event->pmu->stop(event, PERF_EF_UPDATE);
> > +
> > + /*
> > + * For programmable counters we need to check where we installed it.
> > + * To keep this function generic always test the more complicated
> > + * case (free running counters won't need the loop).
> > + */
> > + node = get_node(hwc->config, uncore);
> > + for (i = 0; i < node->num_counters; i++) {
> > + if (cmpxchg(&node->events[i], event, NULL) == event)
> > + break;
> > + }
> > + hwc->idx = -1;
> > +}
> > +
> > +void thunder_uncore_start(struct perf_event *event, int flags)
> > +{
> > + struct thunder_uncore *uncore = to_uncore(event->pmu);
> > + struct hw_perf_event *hwc = &event->hw;
> > + struct thunder_uncore_node *node;
> > + struct thunder_uncore_unit *unit;
> > + u64 new = 0;
> > +
> > + /* read counter values from all units on the node */
> > + node = get_node(hwc->config, uncore);
> > + list_for_each_entry(unit, &node->unit_list, entry)
> > + new += readq(hwc->event_base + unit->map);
> > + local64_set(&hwc->prev_count, new);
> > +
> > + hwc->state = 0;
> > + perf_event_update_userpage(event);
> > +}
> > +
> > +void thunder_uncore_stop(struct perf_event *event, int flags)
> > +{
> > + struct hw_perf_event *hwc = &event->hw;
> > +
> > + hwc->state |= PERF_HES_STOPPED;
> > +
> > + if ((flags & PERF_EF_UPDATE) && !(hwc->state & PERF_HES_UPTODATE)) {
> > + thunder_uncore_read(event);
> > + hwc->state |= PERF_HES_UPTODATE;
> > + }
> > +}
> > +
> > +int thunder_uncore_event_init(struct perf_event *event)
> > +{
> > + struct hw_perf_event *hwc = &event->hw;
> > + struct thunder_uncore_node *node;
> > + struct thunder_uncore *uncore;
> > +
> > + if (event->attr.type != event->pmu->type)
> > + return -ENOENT;
> > +
> > + /* we do not support sampling */
> > + if (is_sampling_event(event))
> > + return -EINVAL;
> > +
> > + /* counters do not have these bits */
> > + if (event->attr.exclude_user ||
> > + event->attr.exclude_kernel ||
> > + event->attr.exclude_host ||
> > + event->attr.exclude_guest ||
> > + event->attr.exclude_hv ||
> > + event->attr.exclude_idle)
> > + return -EINVAL;
> > +
> > + uncore = to_uncore(event->pmu);
> > + if (!uncore)
> > + return -ENODEV;
> > + if (!uncore->event_valid(event->attr.config & UNCORE_EVENT_ID_MASK))
> > + return -EINVAL;
> > +
> > + /* check NUMA node */
> > + node = get_node(event->attr.config, uncore);
> > + if (!node) {
> > + pr_debug("Invalid NUMA node selected\n");
> > + return -EINVAL;
> > + }
> > +
> > + hwc->config = event->attr.config;
> > + hwc->idx = -1;
> > + return 0;
> > +}
> > +
> > +static ssize_t thunder_uncore_attr_show_cpumask(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + struct pmu *pmu = dev_get_drvdata(dev);
> > + struct thunder_uncore *uncore =
> > + container_of(pmu, struct thunder_uncore, pmu);
> > +
> > + return cpumap_print_to_pagebuf(true, buf, &uncore->active_mask);
> > +}
> > +static DEVICE_ATTR(cpumask, S_IRUGO, thunder_uncore_attr_show_cpumask, NULL);
> > +
> > +static struct attribute *thunder_uncore_attrs[] = {
> > + &dev_attr_cpumask.attr,
> > + NULL,
> > +};
> > +
> > +struct attribute_group thunder_uncore_attr_group = {
> > + .attrs = thunder_uncore_attrs,
> > +};
> > +
> > +ssize_t thunder_events_sysfs_show(struct device *dev,
> > + struct device_attribute *attr,
> > + char *page)
> > +{
> > + struct perf_pmu_events_attr *pmu_attr =
> > + container_of(attr, struct perf_pmu_events_attr, attr);
> > +
> > + if (pmu_attr->event_str)
> > + return sprintf(page, "%s", pmu_attr->event_str);
> > +
> > + return 0;
> > +}
> > +
> > +/* node attribute depending on number of NUMA nodes */
> > +static ssize_t node_show(struct device *dev, struct device_attribute *attr,
> > + char *page)
> > +{
> > + if (NODES_SHIFT)
> > + return sprintf(page, "config:16-%d\n", 16 + NODES_SHIFT - 1);
>
> If NODES_SHIFT is 1, you'll end up with "config:16-16", which might confuse
> userspace.
So should I use "config:16" in that case? Is it OK to use this also for
NODES_SHIFT=0 ?
> > + else
> > + return sprintf(page, "config:16\n");
> > +}
> > +
> > +struct device_attribute format_attr_node = __ATTR_RO(node);
> > +
> > +/*
> > + * Thunder uncore events are independent from CPUs. Provide a cpumask
> > + * nevertheless to prevent perf from adding the event per-cpu and just
> > + * set the mask to one online CPU. Use the same cpumask for all uncore
> > + * devices.
> > + *
> > + * There is a performance penalty for accessing a device from a CPU on
> > + * another socket, but we do not care (yet).
> > + */
> > +static int thunder_uncore_offline_cpu(unsigned int old_cpu, struct hlist_node *node)
> > +{
> > + struct thunder_uncore *uncore = hlist_entry_safe(node, struct thunder_uncore, node);
>
> Why _safe?
Not required, will remove.
> > + int new_cpu;
> > +
> > + if (!cpumask_test_and_clear_cpu(old_cpu, &uncore->active_mask))
> > + return 0;
> > + new_cpu = cpumask_any_but(cpu_online_mask, old_cpu);
> > + if (new_cpu >= nr_cpu_ids)
> > + return 0;
> > + perf_pmu_migrate_context(&uncore->pmu, old_cpu, new_cpu);
> > + cpumask_set_cpu(new_cpu, &uncore->active_mask);
> > + return 0;
> > +}
> > +
> > +static struct thunder_uncore_node * __init alloc_node(struct thunder_uncore *uncore,
> > + int node_id, int counters)
> > +{
> > + struct thunder_uncore_node *node;
> > +
> > + node = kzalloc(sizeof(*node), GFP_KERNEL);
> > + if (!node)
> > + return NULL;
> > + node->num_counters = counters;
> > + INIT_LIST_HEAD(&node->unit_list);
> > + return node;
> > +}
> > +
> > +int __init thunder_uncore_setup(struct thunder_uncore *uncore, int device_id,
> > + struct pmu *pmu, int counters)
> > +{
> > + unsigned int vendor_id = PCI_VENDOR_ID_CAVIUM;
> > + struct thunder_uncore_unit *unit, *tmp;
> > + struct thunder_uncore_node *node;
> > + struct pci_dev *pdev = NULL;
> > + int ret, node_id, found = 0;
> > +
> > + /* detect PCI devices */
> > + while ((pdev = pci_get_device(vendor_id, device_id, pdev))) {
>
> Redundant brackets?
OK
> > + if (!pdev)
> > + break;
>
> Redundant check?
OK
> > + node_id = dev_to_node(&pdev->dev);
> > +
> > + /* allocate node if necessary */
> > + if (!uncore->nodes[node_id])
> > + uncore->nodes[node_id] = alloc_node(uncore, node_id, counters);
> > +
> > + node = uncore->nodes[node_id];
> > + if (!node) {
> > + ret = -ENOMEM;
> > + goto fail;
> > + }
> > +
> > + unit = kzalloc(sizeof(*unit), GFP_KERNEL);
> > + if (!unit) {
> > + ret = -ENOMEM;
> > + goto fail;
> > + }
> > +
> > + unit->pdev = pdev;
> > + unit->map = ioremap(pci_resource_start(pdev, 0),
> > + pci_resource_len(pdev, 0));
> > + list_add(&unit->entry, &node->unit_list);
> > + node->nr_units++;
> > + found++;
> > + }
> > +
> > + if (!found)
> > + return -ENODEV;
> > +
> > + cpuhp_state_add_instance_nocalls(CPUHP_AP_UNCORE_CAVIUM_ONLINE,
> > + &uncore->node);
> > +
> > + /*
> > + * perf PMU is CPU dependent in difference to our uncore devices.
> > + * Just pick a CPU and migrate away if it goes offline.
> > + */
> > + cpumask_set_cpu(smp_processor_id(), &uncore->active_mask);
> > +
> > + uncore->pmu = *pmu;
> > + ret = perf_pmu_register(&uncore->pmu, uncore->pmu.name, -1);
> > + if (ret)
> > + goto fail;
> > +
> > + return 0;
> > +
> > +fail:
> > + node_id = 0;
> > + while (uncore->nodes[node_id]) {
> > + node = uncore->nodes[node_id];
> > +
> > + list_for_each_entry_safe(unit, tmp, &node->unit_list, entry) {
>
> Why do you need the _safe variant?
OK, not needed
> Will
^ permalink raw reply
* [PATCH v1 03/11] drivers: soc: hisi: Add support for Hisilicon Djtag driver
From: Anurup M @ 2016-11-11 10:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <4812554.ng09YGQfZ9@wuerfel>
On Thursday 10 November 2016 03:10 AM, Arnd Bergmann wrote:
> On Wednesday, November 9, 2016 9:58:38 AM CET Anurup M wrote:
>>> I also see that the compatible strings have the version included in
>>> them, and you can probably drop them by requiring them only in the
>>> fallback:
>>>
>>> compatible = "hisilicon,hip05-cpu-djtag", "hisilicon,djtag-v1";
>>> compatible = "hisilicon,hip05-io-djtag", "hisilicon,djtag-v1";
>>> compatible = "hisilicon,hip06-cpu-djtag", "hisilicon,djtag-v1";
>>> compatible = "hisilicon,hip06-io-djtag", "hisilicon,djtag-v2";
>>> compatible = "hisilicon,hip07-cpu-djtag", "hisilicon,djtag-v2";
>>> compatible = "hisilicon,hip07-io-djtag", "hisilicon,djtag-v2";
>>>
>>> We want to have the first entry be as specific as possible, but
>>> the last (second) entry is the one that can be used by the driver
>>> for matching. When a future hip08/hip09/... chip uses an existing
>>> interface, you then don't have to update the driver.
>> Thanks. I had a similar thought on this. So as I have the version string
>> in the
>> second entry "-v(1/2)".
>> I can use it in driver for matching. So i think I will change it as below.
>> Please correct me if my understanding is wrong.
>>
>> static const struct of_device_id djtag_of_match[] = {
>> - /* for hip05(D02) cpu die */
>> - { .compatible = "hisilicon,hip05-cpu-djtag-v1",
>> + /* for hisi djtag-v1 cpu die */
>> + { .compatible = "hisilicon,hisi-cpu-djtag-v1",
>> .data = djtag_readwrite_v1 },
>> - /* for hip05(D02) io die */
>> - { .compatible = "hisilicon,hip05-io-djtag-v1",
>> + /* for hisi djtag-v1 io die */
>> + { .compatible = "hisilicon,hisi-io-djtag-v1",
> From the code it looks like "hisilicon,hisi-io-djtag-v1" and
> "hisilicon,hisi-cpu-djtag-v1" have the same register-level interface,
> so we just need one compatible string for them to match the driver.
>
> Arnd
The djtag versions in CPU die and IO die can be different in the same chip.
For example in hip06, the CPU die has djtag-v1 whereas IO die has djtag-v2.
So I think it need two different compatible string
for hip06 chip CPU DIE "hisilicon,hisi-cpu-djtag-v1"
for hip06 chip IO DIE "hisilicon,hisi-io-djtag-v2"
Thanks,
Anurup
^ permalink raw reply
* [PULL 0/3] KVM/ARM updates for v4.9-rc4
From: Marc Zyngier @ 2016-11-11 10:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <09984771-dfe4-1811-7d2e-29eaf34d99a9@redhat.com>
On 11/11/16 10:14, Paolo Bonzini wrote:
>
>
> On 04/11/2016 19:36, Marc Zyngier wrote:
>> git://git.kernel.org:/pub/scm/linux/kernel/git/kvmarm/kvmarm.git tags/kvm-arm-for-v4.9-rc4
>
> What is the extra colon after "org"? :)
That's obviously me screwing up when hacking the .git/config to have
different fetch and push URLs. The odd thing is that git (2.10.1)
doesn't even complain and happily pulls the branch... Did you get an
error? Anyway, I'll fix that in my local config.
> Pulled now, and I will send it in time for rc5.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* [PULL 0/3] KVM/ARM updates for v4.9-rc4
From: Paolo Bonzini @ 2016-11-11 10:14 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161104183638.28137-1-marc.zyngier@arm.com>
On 04/11/2016 19:36, Marc Zyngier wrote:
> git://git.kernel.org:/pub/scm/linux/kernel/git/kvmarm/kvmarm.git tags/kvm-arm-for-v4.9-rc4
What is the extra colon after "org"? :)
Pulled now, and I will send it in time for rc5.
Thanks,
Paolo
^ permalink raw reply
* [linux-sunxi] [PATCH] clk: sunxi-ng: sun8i-h3: Set CLK_SET_RATE_PARENT for audio module clocks
From: Code Kipper @ 2016-11-11 10:10 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161111100558.14629-2-wens@csie.org>
On 11 November 2016 at 11:05, Chen-Yu Tsai <wens@csie.org> wrote:
> The audio module clocks are supposed to be set according to the sample
> rate of the audio stream. The audio PLL provides the clock signal for
> thees module clocks, and only it is freely tunable.
nick! these
CK
>
> Set CLK_SET_RATE_PARENT for the audio module clocks so their users can
> properly tune the clock rate.
>
> Fixes: 0577e4853bfb ("clk: sunxi-ng: Add H3 clocks")
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> ---
> drivers/clk/sunxi-ng/ccu-sun8i-h3.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-h3.c b/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
> index 4d70590f05e3..21c427d86f28 100644
> --- a/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
> +++ b/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
> @@ -394,16 +394,16 @@ static SUNXI_CCU_MP_WITH_MUX_GATE(spi1_clk, "spi1", mod0_default_parents, 0x0a4,
> static const char * const i2s_parents[] = { "pll-audio-8x", "pll-audio-4x",
> "pll-audio-2x", "pll-audio" };
> static SUNXI_CCU_MUX_WITH_GATE(i2s0_clk, "i2s0", i2s_parents,
> - 0x0b0, 16, 2, BIT(31), 0);
> + 0x0b0, 16, 2, BIT(31), CLK_SET_RATE_PARENT);
>
> static SUNXI_CCU_MUX_WITH_GATE(i2s1_clk, "i2s1", i2s_parents,
> - 0x0b4, 16, 2, BIT(31), 0);
> + 0x0b4, 16, 2, BIT(31), CLK_SET_RATE_PARENT);
>
> static SUNXI_CCU_MUX_WITH_GATE(i2s2_clk, "i2s2", i2s_parents,
> - 0x0b8, 16, 2, BIT(31), 0);
> + 0x0b8, 16, 2, BIT(31), CLK_SET_RATE_PARENT);
>
> static SUNXI_CCU_M_WITH_GATE(spdif_clk, "spdif", "pll-audio",
> - 0x0c0, 0, 4, BIT(31), 0);
> + 0x0c0, 0, 4, BIT(31), CLK_SET_RATE_PARENT);
>
> static SUNXI_CCU_GATE(usb_phy0_clk, "usb-phy0", "osc24M",
> 0x0cc, BIT(8), 0);
> @@ -466,7 +466,7 @@ static SUNXI_CCU_M_WITH_GATE(ve_clk, "ve", "pll-ve",
> 0x13c, 16, 3, BIT(31), 0);
>
> static SUNXI_CCU_GATE(ac_dig_clk, "ac-dig", "pll-audio",
> - 0x140, BIT(31), 0);
> + 0x140, BIT(31), CLK_SET_RATE_PARENT);
> static SUNXI_CCU_GATE(avs_clk, "avs", "osc24M",
> 0x144, BIT(31), 0);
>
> --
> 2.10.2
>
> --
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe at googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply
* [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06
From: zhichang.yuan @ 2016-11-11 10:09 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <10334260.ztLXZ2Oynd@wuerfel>
Hi, Arnd,
On 2016/11/11 0:07, Arnd Bergmann wrote:
> On Thursday, November 10, 2016 3:36:49 PM CET Gabriele Paoloni wrote:
>>
>> Where should we get the range from? For LPC we know that it is going
>> Work on anything that is not used by PCI I/O space, and this is
>> why we use [0, PCIBIOS_MIN_IO]
>
> It should be allocated the same way we allocate PCI config space
> segments. This is currently done with the io_range list in
> drivers/pci/pci.c, which isn't perfect but could be extended
> if necessary. Based on what others commented here, I'd rather
> make the differences between ISA/LPC and PCI I/O ranges smaller
> than larger.
>
>>> Your current version has
>>>
>>> if (arm64_extio_ops->pfout) \
>>> arm64_extio_ops->pfout(arm64_extio_ops->devpara,\
>>> addr, value, sizeof(type)); \
>>>
>>> Instead, just subtract the start of the range from the logical
>>> port number to transform it back into a bus-local port number:
>>
>> These accessors do not operate on IO tokens:
>>
>> If (arm64_extio_ops->start > addr || arm64_extio_ops->end < addr)
>> addr is not going to be an I/O token; in fact patch 2/3 imposes that
>> the I/O tokens will start at PCIBIOS_MIN_IO. So from 0 to PCIBIOS_MIN_IO
>> we have free physical addresses that the accessors can operate on.
>
> Ah, I missed that part. I'd rather not use PCIBIOS_MIN_IO to refer to
> the logical I/O tokens, the purpose of that macro is really meant
> for allocating PCI I/O port numbers within the address space of
> one bus.
>
> Note that it's equally likely that whichever next platform needs
> non-mapped I/O access like this actually needs them for PCI I/O space,
> and that will use it on addresses registered to a PCI host bridge.
>
> If we separate the two steps:
>
> a) assign a range of logical I/O port numbers to a bus
> b) register a set of helpers for redirecting logical I/O
> port to a helper function
>
It seems that we need to add a new bus and the corresponding resource management
which can also cover current PCI pio mapping, is it right?
Thanks,
Zhichang
> then I think the code will get cleaner and more flexible.
> It should actually then be able to replace the powerpc
> specific implementation.
>
> Arnd
>
> .
>
^ permalink raw reply
* [PATCH V5 1/3] ARM64 LPC: Indirect ISA port IO introduced
From: zhichang.yuan @ 2016-11-11 10:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478806353.7430.137.camel@kernel.crashing.org>
Hi, Ben, Mark,
Thanks for your comments! These are helpful!
On 2016/11/11 3:32, Benjamin Herrenschmidt wrote:
> On Thu, 2016-11-10 at 11:22 +0000, Mark Rutland wrote:
>> On POWER8, our PCIe doesn't do IO at all, but we have an LPC bus behind
>>> firmware calls ;-) We use that infrastructure to plumb in the LPC bus.
>>
>> Just to check, do you hook that in your inb/outb/etc?
>
> Yes.
>
>> Generally, it would seem nicer if we could have higher-level
>> isa_{inb,outb,whatever} accessors that we could hook separately from
>> other IO.
>
> Maybe but generally speaking, we don't discriminate accessors per bus,
> ie, readl etc... work on all memory mapped busses, inb... works on all
> busses with an "IO space", at least that's been the idea. It probably
> all comes from the fact that PCI IO and ISA are the same space on
> x86 and most other platforms (not all).
>
>> We don't necessarily have to move all ISA drivers over to that if we had
>> a separate symbol for that interface.
>
> What I do on ppc today is that I have a chunk of virtual address space
> that is reserved for "IO space". The first 64k are "reserved" in that
> they route to "the primary" ISA bus (for legacy crap that uses hard
> coded addresses, though I use that for my LPC bus too). I "allocate"
> space for the PCI IO spaces higher in that space. Was I to support more
> LPC busses I could allocate them up there too.
>
I have similar idea as your PPC MMIO.
We notice the prototype of {in/out()} is something like that:
static inline u8 inb(unsigned long addr)
static inline void outb(u8 value, unsigned long addr)
The type of parameter 'addr' is unsigned long. For I/O space, it is big enough.
So, could you divide this 'addr' into several bit segments? The top 8 bits is
defined as bus index. For normal direct IO, the bus index is 0. For those bus
device which need indirectIO or some special I/O accessors, when these devices
are initializing, can request to allocate an unique ID to them, and register
their own accessors to the entry which is corresponding to the ID.
In this way, we can support multiple domains, I think.
But I am not sure whether it is feasible, for example, are there some
architectures/platforms had populated the top 8 bits? Do we need to request IO
region from ioport_resource for those devices? etc...
Thanks,
Zhichang
> The IO resource of a given device thus becomes the actual IO port plus
> the offset of the base of the segment it's in.
>
> For memory mapped IO, inb/outb will just add the virtual address of
> the base of all IO space to that. The hooking mechanism will pickup
> the stuff that isn't memory mapped.
>
> It's a bit messy but then IO space performance has never been a huge
> worry since IO cycles tend to be very slow to begin with.
>
> Note: We also have the ISA memory and ISA FW spaces that we don't have
> good accessors for. They somewhat exist (I think the fbdev layer uses
> some for vga) but it's messy.
>
> Cheers,
> Ben.
>
>
> .
>
^ permalink raw reply
* [PATCH] clk: sunxi-ng: sun8i-h3: Set CLK_SET_RATE_PARENT for audio module clocks
From: Chen-Yu Tsai @ 2016-11-11 10:05 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161111100558.14629-1-wens@csie.org>
The audio module clocks are supposed to be set according to the sample
rate of the audio stream. The audio PLL provides the clock signal for
thees module clocks, and only it is freely tunable.
Set CLK_SET_RATE_PARENT for the audio module clocks so their users can
properly tune the clock rate.
Fixes: 0577e4853bfb ("clk: sunxi-ng: Add H3 clocks")
Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
drivers/clk/sunxi-ng/ccu-sun8i-h3.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-h3.c b/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
index 4d70590f05e3..21c427d86f28 100644
--- a/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
+++ b/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
@@ -394,16 +394,16 @@ static SUNXI_CCU_MP_WITH_MUX_GATE(spi1_clk, "spi1", mod0_default_parents, 0x0a4,
static const char * const i2s_parents[] = { "pll-audio-8x", "pll-audio-4x",
"pll-audio-2x", "pll-audio" };
static SUNXI_CCU_MUX_WITH_GATE(i2s0_clk, "i2s0", i2s_parents,
- 0x0b0, 16, 2, BIT(31), 0);
+ 0x0b0, 16, 2, BIT(31), CLK_SET_RATE_PARENT);
static SUNXI_CCU_MUX_WITH_GATE(i2s1_clk, "i2s1", i2s_parents,
- 0x0b4, 16, 2, BIT(31), 0);
+ 0x0b4, 16, 2, BIT(31), CLK_SET_RATE_PARENT);
static SUNXI_CCU_MUX_WITH_GATE(i2s2_clk, "i2s2", i2s_parents,
- 0x0b8, 16, 2, BIT(31), 0);
+ 0x0b8, 16, 2, BIT(31), CLK_SET_RATE_PARENT);
static SUNXI_CCU_M_WITH_GATE(spdif_clk, "spdif", "pll-audio",
- 0x0c0, 0, 4, BIT(31), 0);
+ 0x0c0, 0, 4, BIT(31), CLK_SET_RATE_PARENT);
static SUNXI_CCU_GATE(usb_phy0_clk, "usb-phy0", "osc24M",
0x0cc, BIT(8), 0);
@@ -466,7 +466,7 @@ static SUNXI_CCU_M_WITH_GATE(ve_clk, "ve", "pll-ve",
0x13c, 16, 3, BIT(31), 0);
static SUNXI_CCU_GATE(ac_dig_clk, "ac-dig", "pll-audio",
- 0x140, BIT(31), 0);
+ 0x140, BIT(31), CLK_SET_RATE_PARENT);
static SUNXI_CCU_GATE(avs_clk, "avs", "osc24M",
0x144, BIT(31), 0);
--
2.10.2
^ permalink raw reply related
* [PATCH] clk: sunxi-ng: sun8i-a23: Set CLK_SET_RATE_PARENT for audio module clocks
From: Chen-Yu Tsai @ 2016-11-11 10:05 UTC (permalink / raw)
To: linux-arm-kernel
The audio module clocks are supposed to be set according to the sample
rate of the audio stream. The audio PLL provides the clock signal for
thees module clocks, and only it is freely tunable.
Set CLK_SET_RATE_PARENT for the audio module clocks so their users can
properly tune the clock rate.
Fixes: 5690879d93e8 ("clk: sunxi-ng: Add A23 CCU")
Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
drivers/clk/sunxi-ng/ccu-sun8i-a23.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-a23.c b/drivers/clk/sunxi-ng/ccu-sun8i-a23.c
index 2646d980087b..5c6d37bdf247 100644
--- a/drivers/clk/sunxi-ng/ccu-sun8i-a23.c
+++ b/drivers/clk/sunxi-ng/ccu-sun8i-a23.c
@@ -344,10 +344,10 @@ static SUNXI_CCU_MP_WITH_MUX_GATE(spi1_clk, "spi1", mod0_default_parents, 0x0a4,
static const char * const i2s_parents[] = { "pll-audio-8x", "pll-audio-4x",
"pll-audio-2x", "pll-audio" };
static SUNXI_CCU_MUX_WITH_GATE(i2s0_clk, "i2s0", i2s_parents,
- 0x0b0, 16, 2, BIT(31), 0);
+ 0x0b0, 16, 2, BIT(31), CLK_SET_RATE_PARENT);
static SUNXI_CCU_MUX_WITH_GATE(i2s1_clk, "i2s1", i2s_parents,
- 0x0b4, 16, 2, BIT(31), 0);
+ 0x0b4, 16, 2, BIT(31), CLK_SET_RATE_PARENT);
/* TODO: the parent for most of the USB clocks is not known */
static SUNXI_CCU_GATE(usb_phy0_clk, "usb-phy0", "osc24M",
@@ -415,7 +415,7 @@ static SUNXI_CCU_M_WITH_GATE(ve_clk, "ve", "pll-ve",
0x13c, 16, 3, BIT(31), CLK_SET_RATE_PARENT);
static SUNXI_CCU_GATE(ac_dig_clk, "ac-dig", "pll-audio",
- 0x140, BIT(31), 0);
+ 0x140, BIT(31), CLK_SET_RATE_PARENT);
static SUNXI_CCU_GATE(avs_clk, "avs", "osc24M",
0x144, BIT(31), 0);
--
2.10.2
^ permalink raw reply related
* [PATCH v3 3/3] pinctrl: sunxi: Make sunxi_pconf_group_set use sunxi_pconf_reg helper
From: Chen-Yu Tsai @ 2016-11-11 9:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161111095036.11803-1-wens@csie.org>
The sunxi_pconf_reg helper introduced in the last patch gives us the
chance to rework sunxi_pconf_group_set to have it match the structure
of sunxi_pconf_(group_)get and make it easier to understand.
For each config to set, it:
1. checks if the parameter is supported.
2. checks if the argument is within limits.
3. converts argument to the register value.
4. writes to the register with spinlock held.
As a result the function now blocks unsupported config parameters,
instead of silently ignoring them.
Signed-off-by: Chen-Yu Tsai <wens@csie.org>
Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
drivers/pinctrl/sunxi/pinctrl-sunxi.c | 64 +++++++++++++++++------------------
1 file changed, 32 insertions(+), 32 deletions(-)
diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
index ed71bff39869..fa11a3100346 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
@@ -532,23 +532,27 @@ static int sunxi_pconf_group_set(struct pinctrl_dev *pctldev,
{
struct sunxi_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
struct sunxi_pinctrl_group *g = &pctl->groups[group];
- unsigned long flags;
unsigned pin = g->pin - pctl->desc->pin_base;
- u32 val, mask;
- u16 strength;
- u8 dlevel;
int i;
- spin_lock_irqsave(&pctl->lock, flags);
-
for (i = 0; i < num_configs; i++) {
- switch (pinconf_to_config_param(configs[i])) {
+ enum pin_config_param param;
+ unsigned long flags;
+ u32 offset, shift, mask, reg;
+ u16 arg, val;
+ int ret;
+
+ param = pinconf_to_config_param(configs[i]);
+ arg = pinconf_to_config_argument(configs[i]);
+
+ ret = sunxi_pconf_reg(pin, param, &offset, &shift, &mask);
+ if (ret < 0)
+ return ret;
+
+ switch (param) {
case PIN_CONFIG_DRIVE_STRENGTH:
- strength = pinconf_to_config_argument(configs[i]);
- if (strength > 40) {
- spin_unlock_irqrestore(&pctl->lock, flags);
+ if (arg < 10 || arg > 40)
return -EINVAL;
- }
/*
* We convert from mA to what the register expects:
* 0: 10mA
@@ -556,37 +560,33 @@ static int sunxi_pconf_group_set(struct pinctrl_dev *pctldev,
* 2: 30mA
* 3: 40mA
*/
- dlevel = strength / 10 - 1;
- val = readl(pctl->membase + sunxi_dlevel_reg(pin));
- mask = DLEVEL_PINS_MASK << sunxi_dlevel_offset(pin);
- writel((val & ~mask)
- | dlevel << sunxi_dlevel_offset(pin),
- pctl->membase + sunxi_dlevel_reg(pin));
+ val = arg / 10 - 1;
break;
case PIN_CONFIG_BIAS_DISABLE:
- val = readl(pctl->membase + sunxi_pull_reg(pin));
- mask = PULL_PINS_MASK << sunxi_pull_offset(pin);
- writel((val & ~mask),
- pctl->membase + sunxi_pull_reg(pin));
+ val = 0;
break;
case PIN_CONFIG_BIAS_PULL_UP:
- val = readl(pctl->membase + sunxi_pull_reg(pin));
- mask = PULL_PINS_MASK << sunxi_pull_offset(pin);
- writel((val & ~mask) | 1 << sunxi_pull_offset(pin),
- pctl->membase + sunxi_pull_reg(pin));
+ if (arg == 0)
+ return -EINVAL;
+ val = 1;
break;
case PIN_CONFIG_BIAS_PULL_DOWN:
- val = readl(pctl->membase + sunxi_pull_reg(pin));
- mask = PULL_PINS_MASK << sunxi_pull_offset(pin);
- writel((val & ~mask) | 2 << sunxi_pull_offset(pin),
- pctl->membase + sunxi_pull_reg(pin));
+ if (arg == 0)
+ return -EINVAL;
+ val = 2;
break;
default:
- break;
+ /* sunxi_pconf_reg should catch anything unsupported */
+ WARN_ON(1);
+ return -ENOTSUPP;
}
- } /* for each config */
- spin_unlock_irqrestore(&pctl->lock, flags);
+ spin_lock_irqsave(&pctl->lock, flags);
+ reg = readl(pctl->membase + offset);
+ reg &= ~(mask << shift);
+ writel(reg | val << shift, pctl->membase + offset);
+ spin_unlock_irqrestore(&pctl->lock, flags);
+ } /* for each config */
return 0;
}
--
2.10.2
^ permalink raw reply related
* [PATCH v3 2/3] pinctrl: sunxi: Add support for fetching pinconf settings from hardware
From: Chen-Yu Tsai @ 2016-11-11 9:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161111095036.11803-1-wens@csie.org>
The sunxi pinctrl driver only caches whatever pinconf setting was last
set on a given pingroup. This is not particularly helpful, nor is it
correct.
Fix this by actually reading the hardware registers and returning
the correct results or error codes. Also filter out unsupported
pinconf settings. Since this driver has a peculiar setup of 1 pin
per group, we can support both pin and pingroup pinconf setting
read back with the same code. The sunxi_pconf_reg helper and code
structure is inspired by pinctrl-msm.
With this done we can also claim to support generic pinconf, by
setting .is_generic = true in pinconf_ops.
Also remove the cached config value. The behavior of this was never
correct, as it only cached 1 setting instead of all of them. Since
we can now read back settings directly from the hardware, it is no
longer required.
Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
drivers/pinctrl/sunxi/pinctrl-sunxi.c | 86 +++++++++++++++++++++++++++++++++--
drivers/pinctrl/sunxi/pinctrl-sunxi.h | 1 -
2 files changed, 81 insertions(+), 6 deletions(-)
diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
index e04edda8629d..ed71bff39869 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
@@ -438,15 +438,91 @@ static const struct pinctrl_ops sunxi_pctrl_ops = {
.get_group_pins = sunxi_pctrl_get_group_pins,
};
+static int sunxi_pconf_reg(unsigned pin, enum pin_config_param param,
+ u32 *offset, u32 *shift, u32 *mask)
+{
+ switch (param) {
+ case PIN_CONFIG_DRIVE_STRENGTH:
+ *offset = sunxi_dlevel_reg(pin);
+ *shift = sunxi_dlevel_offset(pin);
+ *mask = DLEVEL_PINS_MASK;
+ break;
+
+ case PIN_CONFIG_BIAS_PULL_UP:
+ case PIN_CONFIG_BIAS_PULL_DOWN:
+ case PIN_CONFIG_BIAS_DISABLE:
+ *offset = sunxi_pull_reg(pin);
+ *shift = sunxi_pull_offset(pin);
+ *mask = PULL_PINS_MASK;
+ break;
+
+ default:
+ return -ENOTSUPP;
+ }
+
+ return 0;
+}
+
+static int sunxi_pconf_get(struct pinctrl_dev *pctldev, unsigned pin,
+ unsigned long *config)
+{
+ struct sunxi_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
+ enum pin_config_param param = pinconf_to_config_param(*config);
+ u32 offset, shift, mask, val;
+ u16 arg;
+ int ret;
+
+ pin -= pctl->desc->pin_base;
+
+ ret = sunxi_pconf_reg(pin, param, &offset, &shift, &mask);
+ if (ret < 0)
+ return ret;
+
+ val = (readl(pctl->membase + offset) >> shift) & mask;
+
+ switch (pinconf_to_config_param(*config)) {
+ case PIN_CONFIG_DRIVE_STRENGTH:
+ arg = (val + 1) * 10;
+ break;
+
+ case PIN_CONFIG_BIAS_PULL_UP:
+ if (val != SUN4I_PINCTRL_PULL_UP)
+ return -EINVAL;
+ arg = 1; /* hardware is weak pull-up */
+ break;
+
+ case PIN_CONFIG_BIAS_PULL_DOWN:
+ if (val != SUN4I_PINCTRL_PULL_DOWN)
+ return -EINVAL;
+ arg = 1; /* hardware is weak pull-down */
+ break;
+
+ case PIN_CONFIG_BIAS_DISABLE:
+ if (val != SUN4I_PINCTRL_NO_PULL)
+ return -EINVAL;
+ arg = 0;
+ break;
+
+ default:
+ /* sunxi_pconf_reg should catch anything unsupported */
+ WARN_ON(1);
+ return -ENOTSUPP;
+ }
+
+ *config = pinconf_to_config_packed(param, arg);
+
+ return 0;
+}
+
static int sunxi_pconf_group_get(struct pinctrl_dev *pctldev,
unsigned group,
unsigned long *config)
{
struct sunxi_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
+ struct sunxi_pinctrl_group *g = &pctl->groups[group];
- *config = pctl->groups[group].config;
-
- return 0;
+ /* We only support 1 pin per group. Chain it to the pin callback */
+ return sunxi_pconf_get(pctldev, g->pin, config);
}
static int sunxi_pconf_group_set(struct pinctrl_dev *pctldev,
@@ -508,8 +584,6 @@ static int sunxi_pconf_group_set(struct pinctrl_dev *pctldev,
default:
break;
}
- /* cache the config value */
- g->config = configs[i];
} /* for each config */
spin_unlock_irqrestore(&pctl->lock, flags);
@@ -518,6 +592,8 @@ static int sunxi_pconf_group_set(struct pinctrl_dev *pctldev,
}
static const struct pinconf_ops sunxi_pconf_ops = {
+ .is_generic = true,
+ .pin_config_get = sunxi_pconf_get,
.pin_config_group_get = sunxi_pconf_group_get,
.pin_config_group_set = sunxi_pconf_group_set,
};
diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.h b/drivers/pinctrl/sunxi/pinctrl-sunxi.h
index 0afce1ab12d0..a7efb31d6523 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sunxi.h
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.h
@@ -109,7 +109,6 @@ struct sunxi_pinctrl_function {
struct sunxi_pinctrl_group {
const char *name;
- unsigned long config;
unsigned pin;
};
--
2.10.2
^ permalink raw reply related
* [PATCH v3 1/3] pinctrl: sunxi: Fix PIN_CONFIG_BIAS_PULL_{DOWN, UP} argument
From: Chen-Yu Tsai @ 2016-11-11 9:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161111095036.11803-1-wens@csie.org>
According to pinconf-generic.h, the argument for
PIN_CONFIG_BIAS_PULL_{DOWN,UP} is non-zero if the bias is enabled
with a pull up/down resistor, zero if it is directly connected
to VDD or ground.
Since Allwinner hardware uses a weak pull resistor internally,
the argument should be 1.
Signed-off-by: Chen-Yu Tsai <wens@csie.org>
Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
drivers/pinctrl/sunxi/pinctrl-sunxi.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
index e199d95af8c0..e04edda8629d 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
@@ -291,12 +291,16 @@ static unsigned long *sunxi_pctrl_build_pin_config(struct device_node *node,
if (sunxi_pctrl_has_bias_prop(node)) {
int pull = sunxi_pctrl_parse_bias_prop(node);
+ int arg = 0;
if (pull < 0) {
ret = pull;
goto err_free;
}
- pinconfig[idx++] = pinconf_to_config_packed(pull, 0);
+ if (pull != PIN_CONFIG_BIAS_DISABLE)
+ arg = 1; /* hardware uses weak pull resistors */
+
+ pinconfig[idx++] = pinconf_to_config_packed(pull, arg);
}
--
2.10.2
^ permalink raw reply related
* [PATCH v3 0/3] pinctrl: sunxi: Support generic pinconf functions
From: Chen-Yu Tsai @ 2016-11-11 9:50 UTC (permalink / raw)
To: linux-arm-kernel
Hi everyone,
This series fixes up generic pinconf support for the sunxi pinctrl driver
library. The driver was doing some bits wrong, like a) storing the pinconf
config value in its struct, and not actually reading the hardware to get
the current config, and b) not using the right arguments for the bias
parameters.
Patch 1 fixes the pin bias parameter arguments.
Patch 2 makes the driver read out pinconf settings from the hardware, and
returns the correct value for unsupported features and disable features.
With this in place it also declares itself as generic pinconf compatible,
which enables us to read the config through the debugfs pinconf interface.
Patch 3 makes the sunxi_pconf_group_set callback use the helper function
introduced in patch 1.
Changes since v2:
- Added Maxime's ack.
- Moved the removal of the cached config value from patch 3 to patch 2
at Maxime's request.
Changes since v1:
- Rebased onto the updated sunxi pinctrl driver with support for the
generic pinconf bindings
- Use separate value for what is written to the register in the pinconf
set function, as Maxime requested.
Regards
ChenYu
Chen-Yu Tsai (3):
pinctrl: sunxi: Fix PIN_CONFIG_BIAS_PULL_{DOWN,UP} argument
pinctrl: sunxi: Add support for fetching pinconf settings from
hardware
pinctrl: sunxi: Make sunxi_pconf_group_set use sunxi_pconf_reg helper
drivers/pinctrl/sunxi/pinctrl-sunxi.c | 156 +++++++++++++++++++++++++---------
drivers/pinctrl/sunxi/pinctrl-sunxi.h | 1 -
2 files changed, 118 insertions(+), 39 deletions(-)
--
2.10.2
^ permalink raw reply
* [PATCH v16 0/5] Mediatek MT8173 CMDQ support
From: Horng-Shyang Liao @ 2016-11-11 9:34 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CABb+yY3mi3M0xqNao=DRYN6Np0LOBfS669iUa_hWo2w1MmM1sw@mail.gmail.com>
On Fri, 2016-11-11 at 11:15 +0530, Jassi Brar wrote:
> On Thu, Nov 10, 2016 at 4:45 PM, Horng-Shyang Liao <hs.liao@mediatek.com> wrote:
> > On Tue, 2016-11-01 at 19:28 +0800, HS Liao wrote:
> >> Hi,
> >>
> >> This is Mediatek MT8173 Command Queue(CMDQ) driver. The CMDQ is used
> >> to help write registers with critical time limitation, such as
> >> updating display configuration during the vblank. It controls Global
> >> Command Engine (GCE) hardware to achieve this requirement.
> >>
> >> These patches have a build dependency on top of v4.9-rc1.
> >>
> >> Changes since v15:
> >> - separate "suspend and resume" patch from "save energy" patch
> >> - don't stop running tasks in cmdq_suspend()
> >> (i.e. leave no running tasks guarantee to clients)
> >>
> >> Best regards,
> >> HS Liao
> >>
> >> HS Liao (5):
> >> dt-bindings: soc: Add documentation for the MediaTek GCE unit
> >> CMDQ: Mediatek CMDQ driver
> >> arm64: dts: mt8173: Add GCE node
> >> CMDQ: suspend and resume
> >> CMDQ: save energy
> >>
> >> .../devicetree/bindings/mailbox/mtk-gce.txt | 43 ++
> >> arch/arm64/boot/dts/mediatek/mt8173.dtsi | 10 +
> >> drivers/mailbox/Kconfig | 10 +
> >> drivers/mailbox/Makefile | 2 +
> >> drivers/mailbox/mtk-cmdq-mailbox.c | 632 +++++++++++++++++++++
> >> drivers/soc/mediatek/Kconfig | 11 +
> >> drivers/soc/mediatek/Makefile | 1 +
> >> drivers/soc/mediatek/mtk-cmdq-helper.c | 310 ++++++++++
> >> include/linux/mailbox/mtk-cmdq-mailbox.h | 67 +++
> >> include/linux/soc/mediatek/mtk-cmdq.h | 182 ++++++
> >> 10 files changed, 1268 insertions(+)
> >> create mode 100644 Documentation/devicetree/bindings/mailbox/mtk-gce.txt
> >> create mode 100644 drivers/mailbox/mtk-cmdq-mailbox.c
> >> create mode 100644 drivers/soc/mediatek/mtk-cmdq-helper.c
> >> create mode 100644 include/linux/mailbox/mtk-cmdq-mailbox.h
> >> create mode 100644 include/linux/soc/mediatek/mtk-cmdq.h
> >>
> >
> >
> > Hi Jassi, Matthias,
> >
> > Sorry to disturb you.
> >
> No, you don't disturb, but the controller driver and protocol driver,
> introduced in the same patch, does :) So does the suspend/resume
> support (patch 4&5) added separately as a patch on top. Please
> reorganise the patchset.
>
> Thanks.
Hi Jassi,
Do you mean
1. split controller driver and protocol driver as two patches,
2. merge patch 4&5 into one patch, and
3. reorganize the patchset as "(1) binding doc (2) controller driver
(3) protocol driver (4) devicetree (5) energy patch" ?
Thanks,
HS
^ 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