* [PATCH 4/8] coresight: reset "enable_sink" flag when need be
From: Mathieu Poirier @ 2016-11-10 18:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478801934-26904-1-git-send-email-mathieu.poirier@linaro.org>
When using coresight from the perf interface sinks are specified
as part of the perf command line. As such the sink needs to be
disabled once it has been acknowledged by the coresight framework.
Otherwise the sink stays enabled, which may interfere with other
sessions.
This patch removes the sink selection check from the build path
process and make it a function on it's own. The function is
then used when operating from sysFS or perf to determine what
sink has been selected.
If operated from perf the status of the "enable_sink" flag is
reset so that concurrent session can use a different sink. When
used from sysFS the status of the flag is left untouched since
users have full control.
The implementation doesn't handle a scenario where a sink has
been enabled from sysFS and another sink is selected from the
perf command line as both modes of operation are mutually
exclusive.
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
drivers/hwtracing/coresight/coresight-etm-perf.c | 31 +++++-----
drivers/hwtracing/coresight/coresight-priv.h | 4 +-
drivers/hwtracing/coresight/coresight.c | 74 ++++++++++++++++++++++--
3 files changed, 87 insertions(+), 22 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index 2cd7c718198a..5a346fc8ce06 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -202,6 +202,21 @@ static void *etm_setup_aux(int event_cpu, void **pages,
if (!event_data)
return NULL;
+ /*
+ * In theory nothing prevent tracers in a trace session from being
+ * associated with different sinks, nor having a sink per tracer. But
+ * until we have HW with this kind of topology we need to assume tracers
+ * in a trace session are using the same sink. Therefore go through
+ * the coresight bus and pick the first enabled sink.
+ *
+ * When operated from sysFS users are responsible to enable the sink
+ * while from perf, the perf tools will do it based on the choice made
+ * on the cmd line. As such the "enable_sink" flag in sysFS is reset.
+ */
+ sink = coresight_get_enabled_sink(true);
+ if (!sink)
+ return NULL;
+
INIT_WORK(&event_data->work, free_event_data);
mask = &event_data->mask;
@@ -219,25 +234,11 @@ static void *etm_setup_aux(int event_cpu, void **pages,
* list of devices from source to sink that can be
* referenced later when the path is actually needed.
*/
- event_data->path[cpu] = coresight_build_path(csdev);
+ event_data->path[cpu] = coresight_build_path(csdev, sink);
if (IS_ERR(event_data->path[cpu]))
goto err;
}
- /*
- * In theory nothing prevent tracers in a trace session from being
- * associated with different sinks, nor having a sink per tracer. But
- * until we have HW with this kind of topology and a way to convey
- * sink assignement from the perf cmd line we need to assume tracers
- * in a trace session are using the same sink. Therefore pick the sink
- * found at the end of the first available path.
- */
- cpu = cpumask_first(mask);
- /* Grab the sink at the end of the path */
- sink = coresight_get_sink(event_data->path[cpu]);
- if (!sink)
- goto err;
-
if (!sink_ops(sink)->alloc_buffer)
goto err;
diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
index 196a14be4b3d..ef9d8e93e3b2 100644
--- a/drivers/hwtracing/coresight/coresight-priv.h
+++ b/drivers/hwtracing/coresight/coresight-priv.h
@@ -111,7 +111,9 @@ static inline void CS_UNLOCK(void __iomem *addr)
void coresight_disable_path(struct list_head *path);
int coresight_enable_path(struct list_head *path, u32 mode);
struct coresight_device *coresight_get_sink(struct list_head *path);
-struct list_head *coresight_build_path(struct coresight_device *csdev);
+struct coresight_device *coresight_get_enabled_sink(bool reset);
+struct list_head *coresight_build_path(struct coresight_device *csdev,
+ struct coresight_device *sink);
void coresight_release_path(struct list_head *path);
#ifdef CONFIG_CORESIGHT_SOURCE_ETM3X
diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
index 7bf00a0beb6f..0c37356e417c 100644
--- a/drivers/hwtracing/coresight/coresight.c
+++ b/drivers/hwtracing/coresight/coresight.c
@@ -368,6 +368,52 @@ struct coresight_device *coresight_get_sink(struct list_head *path)
return csdev;
}
+static int coresight_enabled_sink(struct device *dev, void *data)
+{
+ bool *reset = data;
+ struct coresight_device *csdev = to_coresight_device(dev);
+
+ if ((csdev->type == CORESIGHT_DEV_TYPE_SINK ||
+ csdev->type == CORESIGHT_DEV_TYPE_LINKSINK) &&
+ csdev->activated) {
+ /*
+ * Now that we have a handle on the sink for this session,
+ * disable the sysFS "enable_sink" flag so that possible
+ * concurrent perf session that wish to use another sink don't
+ * trip on it. Doing so has no ramification for the current
+ * session.
+ */
+ if (*reset)
+ csdev->activated = false;
+
+ return 1;
+ }
+
+ return 0;
+}
+
+/**
+ * coresight_get_enabled_sink - returns the first enabled sink found on the bus
+ * @deactivate: Whether the 'enable_sink' flag should be reset
+ *
+ * When operated from perf the deactivate parameter should be set to 'true'.
+ * That way the "enabled_sink" flag of the sink that was selected can be reset,
+ * allowing for other concurrent perf sessions to choose a different sink.
+ *
+ * When operated from sysFS users have full control and as such the deactivate
+ * parameter should be set to 'false', hence mandating users to explicitly
+ * clear the flag.
+ */
+struct coresight_device *coresight_get_enabled_sink(bool deactivate)
+{
+ struct device *dev = NULL;
+
+ dev = bus_find_device(&coresight_bustype, NULL, &deactivate,
+ coresight_enabled_sink);
+
+ return dev ? to_coresight_device(dev) : NULL;
+}
+
/**
* _coresight_build_path - recursively build a path from a @csdev to a sink.
* @csdev: The device to start from.
@@ -380,6 +426,7 @@ struct coresight_device *coresight_get_sink(struct list_head *path)
* last one.
*/
static int _coresight_build_path(struct coresight_device *csdev,
+ struct coresight_device *sink,
struct list_head *path)
{
int i;
@@ -387,15 +434,15 @@ static int _coresight_build_path(struct coresight_device *csdev,
struct coresight_node *node;
/* An activated sink has been found. Enqueue the element */
- if ((csdev->type == CORESIGHT_DEV_TYPE_SINK ||
- csdev->type == CORESIGHT_DEV_TYPE_LINKSINK) && csdev->activated)
+ if (csdev == sink)
goto out;
/* Not a sink - recursively explore each port found on this element */
for (i = 0; i < csdev->nr_outport; i++) {
struct coresight_device *child_dev = csdev->conns[i].child_dev;
- if (child_dev && _coresight_build_path(child_dev, path) == 0) {
+ if (child_dev &&
+ _coresight_build_path(child_dev, sink, path) == 0) {
found = true;
break;
}
@@ -422,18 +469,22 @@ static int _coresight_build_path(struct coresight_device *csdev,
return 0;
}
-struct list_head *coresight_build_path(struct coresight_device *csdev)
+struct list_head *coresight_build_path(struct coresight_device *source,
+ struct coresight_device *sink)
{
struct list_head *path;
int rc;
+ if (!sink)
+ return ERR_PTR(-EINVAL);
+
path = kzalloc(sizeof(struct list_head), GFP_KERNEL);
if (!path)
return ERR_PTR(-ENOMEM);
INIT_LIST_HEAD(path);
- rc = _coresight_build_path(csdev, path);
+ rc = _coresight_build_path(source, sink, path);
if (rc) {
kfree(path);
return ERR_PTR(rc);
@@ -497,6 +548,7 @@ static int coresight_validate_source(struct coresight_device *csdev,
int coresight_enable(struct coresight_device *csdev)
{
int cpu, ret = 0;
+ struct coresight_device *sink;
struct list_head *path;
mutex_lock(&coresight_mutex);
@@ -508,7 +560,17 @@ int coresight_enable(struct coresight_device *csdev)
if (csdev->enable)
goto out;
- path = coresight_build_path(csdev);
+ /*
+ * Search for a valid sink for this session but don't reset the
+ * "enable_sink" flag in sysFS. Users get to do that explicitly.
+ */
+ sink = coresight_get_enabled_sink(false);
+ if (!sink) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ path = coresight_build_path(csdev, sink);
if (IS_ERR(path)) {
pr_err("building path(s) failed\n");
ret = PTR_ERR(path);
--
2.7.4
^ permalink raw reply related
* [PATCH 3/8] coresight: etm3x: Adding missing features of Coresight PTM components
From: Mathieu Poirier @ 2016-11-10 18:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478801934-26904-1-git-send-email-mathieu.poirier@linaro.org>
From: Muhammad Abdul WAHAB <muhammadabdul.wahab@centralesupelec.fr>
In the current driver for Coresight components, two features of PTM
components are missing:
1. Branch Broadcasting (present also in ETM but called Branch Output)
2. Return Stack (only present in PTM v1.0 and PTMv1.1)
These features can be added simply to the code using `mode` field of
`etm_config` struct.
1. **Branch Broadcast** : The branch broadcast feature is present in ETM
components as well and is called Branch output. It allows to retrieve
addresses for direct branch addresses alongside the indirect branch
addresses. For example, it could be useful in cases when tracing without
source code.
2. **Return Stack** : The return stack option allows to retrieve the
return addresses of function calls. It can be useful to avoid CRA
(Code Reuse Attacks) by keeping a shadowstack.
Signed-off-by: Muhammad Abdul Wahab <muhammadabdul.wahab@centralesupelec.fr>
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
drivers/hwtracing/coresight/coresight-etm.h | 5 +++++
drivers/hwtracing/coresight/coresight-etm3x-sysfs.c | 10 ++++++++++
2 files changed, 15 insertions(+)
diff --git a/drivers/hwtracing/coresight/coresight-etm.h b/drivers/hwtracing/coresight/coresight-etm.h
index 4a18ee499965..ad063d7444e1 100644
--- a/drivers/hwtracing/coresight/coresight-etm.h
+++ b/drivers/hwtracing/coresight/coresight-etm.h
@@ -89,11 +89,13 @@
/* ETMCR - 0x00 */
#define ETMCR_PWD_DWN BIT(0)
#define ETMCR_STALL_MODE BIT(7)
+#define ETMCR_BRANCH_BROADCAST BIT(8)
#define ETMCR_ETM_PRG BIT(10)
#define ETMCR_ETM_EN BIT(11)
#define ETMCR_CYC_ACC BIT(12)
#define ETMCR_CTXID_SIZE (BIT(14)|BIT(15))
#define ETMCR_TIMESTAMP_EN BIT(28)
+#define ETMCR_RETURN_STACK BIT(29)
/* ETMCCR - 0x04 */
#define ETMCCR_FIFOFULL BIT(23)
/* ETMPDCR - 0x310 */
@@ -110,8 +112,11 @@
#define ETM_MODE_STALL BIT(2)
#define ETM_MODE_TIMESTAMP BIT(3)
#define ETM_MODE_CTXID BIT(4)
+#define ETM_MODE_BBROAD BIT(5)
+#define ETM_MODE_RET_STACK BIT(6)
#define ETM_MODE_ALL (ETM_MODE_EXCLUDE | ETM_MODE_CYCACC | \
ETM_MODE_STALL | ETM_MODE_TIMESTAMP | \
+ ETM_MODE_BBROAD | ETM_MODE_RET_STACK | \
ETM_MODE_CTXID | ETM_MODE_EXCL_KERN | \
ETM_MODE_EXCL_USER)
diff --git a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
index 5ea090955c08..ca98ad13bb8c 100644
--- a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
+++ b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
@@ -164,6 +164,16 @@ static ssize_t mode_store(struct device *dev,
else
config->ctrl &= ~ETMCR_CTXID_SIZE;
+ if (config->mode & ETM_MODE_BBROAD)
+ config->ctrl |= ETMCR_BRANCH_BROADCAST;
+ else
+ config->ctrl &= ~ETMCR_BRANCH_BROADCAST;
+
+ if (config->mode & ETM_MODE_RET_STACK)
+ config->ctrl |= ETMCR_RETURN_STACK;
+ else
+ config->ctrl &= ~ETMCR_RETURN_STACK;
+
if (config->mode & (ETM_MODE_EXCL_KERN | ETM_MODE_EXCL_USER))
etm_config_trace_mode(config);
--
2.7.4
^ permalink raw reply related
* [PATCH 2/8] coresight: etm3x: indentation fix (extra space removed)
From: Mathieu Poirier @ 2016-11-10 18:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478801934-26904-1-git-send-email-mathieu.poirier@linaro.org>
From: Muhammad Abdul WAHAB <muhammadabdul.wahab@centralesupelec.fr>
An extra space is removed.
Signed-off-by: Muhammad Abdul Wahab <muhammadabdul.wahab@centralesupelec.fr>
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
drivers/hwtracing/coresight/coresight-etm3x-sysfs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
index e9b071953f80..5ea090955c08 100644
--- a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
+++ b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
@@ -146,7 +146,7 @@ static ssize_t mode_store(struct device *dev,
goto err_unlock;
}
config->ctrl |= ETMCR_STALL_MODE;
- } else
+ } else
config->ctrl &= ~ETMCR_STALL_MODE;
if (config->mode & ETM_MODE_TIMESTAMP) {
--
2.7.4
^ permalink raw reply related
* [PATCH 1/8] coresight: stm: return error code instead of zero in .packet()
From: Mathieu Poirier @ 2016-11-10 18:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478801934-26904-1-git-send-email-mathieu.poirier@linaro.org>
From: Chunyan Zhang <zhang.chunyan@linaro.org>
In STM framework driver, the trace data writing loop would keep running
until it received a negative return value or the whole trace packet has
been written to STM device. So if the .packet() of STM device always
returns zero since the device is not enabled or the parameter isn't
supported, STM framework driver will stall into a dead loop.
Returning -EACCES (Permission denied) in .packet() if the device is
disabled makes more sense, and this is the same for returning -EINVAL
if the channel passed into is not supported.
Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
drivers/hwtracing/coresight/coresight-stm.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c
index 49e0f1b925a5..d397849c2c6a 100644
--- a/drivers/hwtracing/coresight/coresight-stm.c
+++ b/drivers/hwtracing/coresight/coresight-stm.c
@@ -419,10 +419,10 @@ static ssize_t stm_generic_packet(struct stm_data *stm_data,
struct stm_drvdata, stm);
if (!(drvdata && local_read(&drvdata->mode)))
- return 0;
+ return -EACCES;
if (channel >= drvdata->numsp)
- return 0;
+ return -EINVAL;
ch_addr = (unsigned long)stm_channel_addr(drvdata, channel);
--
2.7.4
^ permalink raw reply related
* [PATCH 0/8] coresight: next v4.9-rc4
From: Mathieu Poirier @ 2016-11-10 18:18 UTC (permalink / raw)
To: linux-arm-kernel
Good morning Greg,
Please consider the following set for inclusion in the 4.10 cycle. It
applies cleanly on today 'char-misc-next' branch (3372592a140d).
Regards,
Mathieu
Chunyan Zhang (1):
coresight: stm: return error code instead of zero in .packet()
Mathieu Poirier (1):
coresight: reset "enable_sink" flag when need be
Muhammad Abdul WAHAB (2):
coresight: etm3x: indentation fix (extra space removed)
coresight: etm3x: Adding missing features of Coresight PTM components
Suzuki K Poulose (1):
coresight: Add support for ARM Coresight STM-500
Suzuki K. Poulose (3):
coresight: tmc: Cleanup operation mode handling
coresight: tmc: Get rid of mode parameter for helper routines
coresight: tmc: Remove duplicate memset
drivers/hwtracing/coresight/coresight-etm-perf.c | 31 ++++-----
drivers/hwtracing/coresight/coresight-etm.h | 5 ++
.../hwtracing/coresight/coresight-etm3x-sysfs.c | 12 +++-
drivers/hwtracing/coresight/coresight-priv.h | 4 +-
drivers/hwtracing/coresight/coresight-stm.c | 9 ++-
drivers/hwtracing/coresight/coresight-tmc-etf.c | 48 ++++++--------
drivers/hwtracing/coresight/coresight-tmc-etr.c | 43 +++++--------
drivers/hwtracing/coresight/coresight-tmc.h | 2 +-
drivers/hwtracing/coresight/coresight.c | 74 ++++++++++++++++++++--
9 files changed, 144 insertions(+), 84 deletions(-)
--
2.7.4
^ permalink raw reply
* [PATCH 1/2] arm64: perf: Move ARMv8 PMU perf event definitions to asm/perf_event.h
From: Wei Huang @ 2016-11-10 18:09 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161110171732.GA17134@arm.com>
On 11/10/2016 11:17 AM, Will Deacon wrote:
> On Thu, Nov 10, 2016 at 03:32:12PM +0000, Marc Zyngier wrote:
>> On 10/11/16 15:12, Wei Huang wrote:
>>>
>>>
>>> On 11/10/2016 03:10 AM, Marc Zyngier wrote:
>>>> Hi Wei,
>>>>
>>>> On 09/11/16 19:57, Wei Huang wrote:
>>>>> This patch moves ARMv8-related perf event definitions from perf_event.c
>>>>> to asm/perf_event.h; so KVM code can use them directly. This also help
>>>>> remove a duplicated definition of SW_INCR in perf_event.h.
>>>>>
>>>>> Signed-off-by: Wei Huang <wei@redhat.com>
>>>>> ---
>>>>> arch/arm64/include/asm/perf_event.h | 161 +++++++++++++++++++++++++++++++++++-
>>>>> arch/arm64/kernel/perf_event.c | 161 ------------------------------------
>>>>> 2 files changed, 160 insertions(+), 162 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm64/include/asm/perf_event.h b/arch/arm64/include/asm/perf_event.h
>>>>> index 2065f46..6c7b18b 100644
>>>>> --- a/arch/arm64/include/asm/perf_event.h
>>>>> +++ b/arch/arm64/include/asm/perf_event.h
>>>>> @@ -46,7 +46,166 @@
>>>>> #define ARMV8_PMU_EVTYPE_MASK 0xc800ffff /* Mask for writable bits */
>>>>> #define ARMV8_PMU_EVTYPE_EVENT 0xffff /* Mask for EVENT bits */
>>>>>
>>>>> -#define ARMV8_PMU_EVTYPE_EVENT_SW_INCR 0 /* Software increment event */
>>>>> +/*
>>>>> + * ARMv8 PMUv3 Performance Events handling code.
>>>>> + * Common event types.
>>>>> + */
>>>>> +
>>>>> +/* Required events. */
>>>>> +#define ARMV8_PMUV3_PERFCTR_SW_INCR 0x00
>>>>> +#define ARMV8_PMUV3_PERFCTR_L1D_CACHE_REFILL 0x03
>>>>> +#define ARMV8_PMUV3_PERFCTR_L1D_CACHE 0x04
>>>>> +#define ARMV8_PMUV3_PERFCTR_BR_MIS_PRED 0x10
>>>>> +#define ARMV8_PMUV3_PERFCTR_CPU_CYCLES 0x11
>>>>> +#define ARMV8_PMUV3_PERFCTR_BR_PRED 0x12
>>>>
>>>> In my initial review, I asked for the "required" events to be moved to a
>>>> shared location. What's the rational for moving absolutely everything?
>>>
>>> I did notice the phrase "required" in the original email. However I
>>> think it is weird to have two places for a same set of PMU definitions.
>>> Other developers might think these two are missing if they don't search
>>> kernel files carefully.
>>>
>>> If Will Deacon and you insist, I can move only two defs to perf_event.h,
>>> consolidated with the 2nd patch into a single one.
>>
>> My personal feeling is that only architected events should be in a
>> public header. The CPU-specific ones are probably better kept private,
>> as it is doubtful that other users would appear).
>>
>> I'll leave it up to Will to decide, as all I want to avoid is the
>> duplication of constants between the PMU and KVM code bases.
>
> Yeah, just take the sets that you need (i.e. the architected events).
Hi Will,
Just to clarify what "architected" means:
We need two for KVM: SW_INCR (architectural) and CPU_CYCLES
(micro-architectural). Looking at perf_event.c file, I can either
relocate the "Required events" (6 events) or the whole set of
ARMV8_PMUV3_PERFCTR_* (~50 events) to perf_event.h. Which way you prefer?
Thanks,
-Wei
>
> Also, check that it builds.
>
> Will
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* [RFC v2 8/8] iommu/arm-smmu: implement add_reserved_regions callback
From: Auger Eric @ 2016-11-10 18:00 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161110161331.GJ2078@8bytes.org>
Hi Joerg,
On 10/11/2016 17:13, Joerg Roedel wrote:
> On Thu, Nov 10, 2016 at 04:57:51PM +0100, Auger Eric wrote:
>> It does not only serve the purpose to register the MSI IOVA region. We
>> also need to allocate an iova_domain where MSI IOVAs will be allocated
>> upon the request of the relevant MSI controllers. Do you mean you don't
>> like to use the iova allocator for this purpose?
>
> Yes, it looks like the only purpose of iommu_get_dma_msi_region_cookie()
> is to get the msi-region information into into the reserved-list.
>
> Why do you need to 'allocate' the MSI region after all? Except for
> IOMMU_DOMAIN_DMA domains the iommu-core code does not care about address
> allocation. This is up to the users of the domain, which includes that
> the user has to take care of the MSI region.
The purpose is to reuse the transparent MSI IOVA allocation scheme
introduced by Robin in [PATCH v7 00/22] Generic DT bindings for PCI
IOMMUs and ARM SMMU (commit 44bb7e243bd4b4e5c79de2452cd9762582f58925).
GICv2m and GICV3 ITS use dma-mapping iommu_dma_map_msi_msg to allocate
an MSI IOVA on-demand. This relies on the existence of an allocated
iova_domain whose handle is stored in domain->iova_cookie.
the iommu_dma_init_domain could be done in VFIO instead of in the
IOMMU-code, on the basis of dm region info. But we would need to
differentiate the MSI window from P2P windows.
msi-region start/length are arbitrarily chosen and the setup of the
reserved region list does not depend on iommu_get_dma_msi_region_cookie;
but maybe I completely misunderstand your question.
> Besides that, 'iommu_get_dma_msi_region_cookie' is a terrible function
> name and does not describe at all what the function does or is supposed
> to do.
Yes this was discussed with Alex& Robin already
(https://patchwork.kernel.org/patch/9363989/). I proposed to rename into
iommu_setup_dma_msi_region but this was not validated.
Thanks
Eric
>
>
> Joerg
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
^ permalink raw reply
* [PATCH V6 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping
From: Lorenzo Pieralisi @ 2016-11-10 17:58 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <60c1d53146c0aebd3a05095823229224@codeaurora.org>
On Thu, Nov 10, 2016 at 10:02:35AM -0500, agustinv at codeaurora.org wrote:
> Hey Hanjun,
>
> On 2016-11-09 21:36, Hanjun Guo wrote:
> >Hi Marc, Rafael, Lorenzo,
> >
> >Since we agreed to add a probe deferral if we failed to get irq
> >resources which mirroring the DT does (patch 1 in this patch set),
> >I think the last blocker to make things work both for Agustin and
> >me [1] is this patch, which makes the interrupt producer and consumer
> >work in ACPI, we have two different solution for one thing, we'd happy
> >to work together for one solution, could you give some suggestions
> >please?
> >
> >[1]: https://mail-archive.com/linux-kernel at vger.kernel.org/msg1257419.html
> >
> >Agustin, I have some comments below.
> >
> >On 2016/10/29 4:48, Agustin Vega-Frias wrote:
> >>This allows irqchip drivers to associate an ACPI DSDT device to
> >>an IRQ domain and provides support for using the ResourceSource
> >>in Extended IRQ Resources to find the domain and map the IRQs
> >>specified on that domain.
> >>
> >>Signed-off-by: Agustin Vega-Frias <agustinv@codeaurora.org>
> >>---
> >> drivers/acpi/Makefile | 1 +
> >> drivers/acpi/irqdomain.c | 119
> >>+++++++++++++++++++++++++++++++++++++++++++++++
> >
> >Could we just reuse the gsi.c and not introduce a new
> >file, probably we can change the gsi.c to irqdomain.c
> >or something similar, then reuse the code in gsi.c.
>
> I was thinking just that after we chatted off-list.
Yes, that's a fair point.
> I might revisit and see what I come up with given that we already have
> a device argument and we could pass the IRQ source there.
I agree with the approach taken by this patch, I do not like much
passing around struct acpi_resource_source *source (in particular
the dummy struct) I do not think it is needed, I will comment on
the code.
Hopefully there is not any buggy FW out there that does use the
resource source inappropriately otherwise we will notice on x86/ia64
(ie you can't blame FW if it breaks the kernel) but I suspect the
only way to find out is by trying, the patch has to go through Rafael's
review anyway before getting there so it is fine.
Lorenzo
> Thanks
>
> >
> >Thanks
> >Hanjun
> >
> >> drivers/acpi/resource.c | 21 +++++----
> >> include/linux/acpi.h | 25 ++++++++++
> >> 4 files changed, 157 insertions(+), 9 deletions(-)
> >> create mode 100644 drivers/acpi/irqdomain.c
> >>
> >>diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> >>index 9ed0878..880401b 100644
> >>--- a/drivers/acpi/Makefile
> >>+++ b/drivers/acpi/Makefile
> >>@@ -57,6 +57,7 @@ acpi-$(CONFIG_ACPI_PROCFS_POWER) += cm_sbs.o
> >> acpi-y += acpi_lpat.o
> >> acpi-$(CONFIG_ACPI_GENERIC_GSI) += gsi.o
> >> acpi-$(CONFIG_ACPI_WATCHDOG) += acpi_watchdog.o
> >>+acpi-$(CONFIG_IRQ_DOMAIN) += irqdomain.o
> >>
> >> # These are (potentially) separate modules
> >>
> >>diff --git a/drivers/acpi/irqdomain.c b/drivers/acpi/irqdomain.c
> >>new file mode 100644
> >>index 0000000..11d3658
> >>--- /dev/null
> >>+++ b/drivers/acpi/irqdomain.c
> >>@@ -0,0 +1,119 @@
> >>+/*
> >>+ * ACPI ResourceSource/IRQ domain mapping support
> >>+ *
> >>+ * Copyright (c) 2016, The Linux Foundation. All rights reserved.
> >>+ *
> >>+ * This program is free software; you can redistribute it
> >>and/or modify
> >>+ * it under the terms of the GNU General Public License version 2 and
> >>+ * only version 2 as published by the Free Software Foundation.
> >>+ *
> >>+ * This program is distributed in the hope that it will be useful,
> >>+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >>+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> >>+ * GNU General Public License for more details.
> >>+ */
> >>+#include <linux/acpi.h>
> >>+#include <linux/irq.h>
> >>+#include <linux/irqdomain.h>
> >>+
> >>+/**
> >>+ * acpi_irq_domain_register_irq() - Register the mapping for an
> >>IRQ produced
> >>+ * by the given
> >>acpi_resource_source to a
> >>+ * Linux IRQ number
> >>+ * @source: IRQ source
> >>+ * @hwirq: Hardware IRQ number
> >>+ * @trigger: trigger type of the IRQ number to be mapped
> >>+ * @polarity: polarity of the IRQ to be mapped
> >>+ *
> >>+ * Returns: a valid linux IRQ number on success
> >>+ * -ENODEV if the given acpi_resource_source cannot be found
> >>+ * -EPROBE_DEFER if the IRQ domain has not been registered
> >>+ * -EINVAL for all other errors
> >>+ */
> >>+int acpi_irq_domain_register_irq(const struct
> >>acpi_resource_source *source,
> >>+ u32 hwirq, int trigger, int polarity)
> >>+{
> >>+ struct irq_fwspec fwspec;
> >>+ struct acpi_device *device;
> >>+ acpi_handle handle;
> >>+ acpi_status status;
> >>+ int ret;
> >>+
> >>+ /* An empty acpi_resource_source means it is a GSI */
> >>+ if (!source->string_length)
> >>+ return acpi_register_gsi(NULL, hwirq, trigger, polarity);
> >>+
> >>+ status = acpi_get_handle(NULL, source->string_ptr, &handle);
> >>+ if (ACPI_FAILURE(status))
> >>+ return -ENODEV;
> >>+
> >>+ device = acpi_bus_get_acpi_device(handle);
> >>+ if (!device)
> >>+ return -ENODEV;
> >>+
> >>+ if (irq_find_matching_fwnode(&device->fwnode, DOMAIN_BUS_ANY)
> >>== NULL) {
> >>+ ret = -EPROBE_DEFER;
> >>+ goto out_put_device;
> >>+ }
> >>+
> >>+ fwspec.fwnode = &device->fwnode;
> >>+ fwspec.param[0] = hwirq;
> >>+ fwspec.param[1] = acpi_dev_get_irq_type(trigger, polarity);
> >>+ fwspec.param_count = 2;
> >>+
> >>+ ret = irq_create_fwspec_mapping(&fwspec);
> >>+
> >>+out_put_device:
> >>+ acpi_bus_put_acpi_device(device);
> >>+
> >>+ return ret;
> >>+}
> >>+EXPORT_SYMBOL_GPL(acpi_irq_domain_register_irq);
> >>+
> >>+/**
> >>+ * acpi_irq_domain_unregister_irq() - Delete the mapping for an
> >>IRQ produced
> >>+ * by the given
> >>acpi_resource_source to a
> >>+ * Linux IRQ number
> >>+ * @source: IRQ source
> >>+ * @hwirq: Hardware IRQ number
> >>+ *
> >>+ * Returns: 0 on success
> >>+ * -ENODEV if the given acpi_resource_source cannot be found
> >>+ * -EINVAL for all other errors
> >>+ */
> >>+int acpi_irq_domain_unregister_irq(const struct
> >>acpi_resource_source *source,
> >>+ u32 hwirq)
> >>+{
> >>+ struct irq_domain *domain;
> >>+ struct acpi_device *device;
> >>+ acpi_handle handle;
> >>+ acpi_status status;
> >>+ int ret = 0;
> >>+
> >>+ if (!source->string_length) {
> >>+ acpi_unregister_gsi(hwirq);
> >>+ return 0;
> >>+ }
> >>+
> >>+ status = acpi_get_handle(NULL, source->string_ptr, &handle);
> >>+ if (ACPI_FAILURE(status))
> >>+ return -ENODEV;
> >>+
> >>+ device = acpi_bus_get_acpi_device(handle);
> >>+ if (!device)
> >>+ return -ENODEV;
> >>+
> >>+ domain = irq_find_matching_fwnode(&device->fwnode, DOMAIN_BUS_ANY);
> >>+ if (!domain) {
> >>+ ret = -EINVAL;
> >>+ goto out_put_device;
> >>+ }
> >>+
> >>+ irq_dispose_mapping(irq_find_mapping(domain, hwirq));
> >>+
> >>+out_put_device:
> >>+ acpi_bus_put_acpi_device(device);
> >>+
> >>+ return ret;
> >>+}
> >>+EXPORT_SYMBOL_GPL(acpi_irq_domain_unregister_irq);
> >>diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
> >>index 4beda15..ccb6f4f 100644
> >>--- a/drivers/acpi/resource.c
> >>+++ b/drivers/acpi/resource.c
> >>@@ -381,14 +381,15 @@ static void
> >>acpi_dev_irqresource_disabled(struct resource *res, u32 gsi)
> >> res->flags = IORESOURCE_IRQ | IORESOURCE_DISABLED |
> >>IORESOURCE_UNSET;
> >> }
> >>
> >>-static void acpi_dev_get_irqresource(struct resource *res, u32 gsi,
> >>+static void acpi_dev_get_irqresource(struct resource *res, u32 hwirq,
> >>+ const struct acpi_resource_source *source,
> >> u8 triggering, u8 polarity, u8 shareable,
> >> bool legacy)
> >> {
> >> int irq, p, t;
> >>
> >>- if (!valid_IRQ(gsi)) {
> >>- acpi_dev_irqresource_disabled(res, gsi);
> >>+ if ((source->string_length == 0) && !valid_IRQ(hwirq)) {
> >>+ acpi_dev_irqresource_disabled(res, hwirq);
> >> return;
> >> }
> >>
> >>@@ -402,25 +403,25 @@ static void
> >>acpi_dev_get_irqresource(struct resource *res, u32 gsi,
> >> * using extended IRQ descriptors we take the IRQ configuration
> >> * from _CRS directly.
> >> */
> >>- if (legacy && !acpi_get_override_irq(gsi, &t, &p)) {
> >>+ if (legacy && !acpi_get_override_irq(hwirq, &t, &p)) {
> >> u8 trig = t ? ACPI_LEVEL_SENSITIVE : ACPI_EDGE_SENSITIVE;
> >> u8 pol = p ? ACPI_ACTIVE_LOW : ACPI_ACTIVE_HIGH;
> >>
> >> if (triggering != trig || polarity != pol) {
> >>- pr_warning("ACPI: IRQ %d override to %s, %s\n", gsi,
> >>- t ? "level" : "edge", p ? "low" : "high");
> >>+ pr_warn("ACPI: IRQ %d override to %s, %s\n", hwirq,
> >>+ t ? "level" : "edge", p ? "low" : "high");
> >> triggering = trig;
> >> polarity = pol;
> >> }
> >> }
> >>
> >> res->flags = acpi_dev_irq_flags(triggering, polarity, shareable);
> >>- irq = acpi_register_gsi(NULL, gsi, triggering, polarity);
> >>+ irq = acpi_irq_domain_register_irq(source, hwirq, triggering,
> >>polarity);
> >> if (irq >= 0) {
> >> res->start = irq;
> >> res->end = irq;
> >> } else {
> >>- acpi_dev_irqresource_disabled(res, gsi);
> >>+ acpi_dev_irqresource_disabled(res, hwirq);
> >> }
> >> }
> >>
> >>@@ -446,6 +447,7 @@ static void acpi_dev_get_irqresource(struct
> >>resource *res, u32 gsi,
> >> bool acpi_dev_resource_interrupt(struct acpi_resource *ares,
> >>int index,
> >> struct resource *res)
> >> {
> >>+ const struct acpi_resource_source dummy = { 0, 0, NULL };
> >> struct acpi_resource_irq *irq;
> >> struct acpi_resource_extended_irq *ext_irq;
> >>
> >>@@ -460,7 +462,7 @@ bool acpi_dev_resource_interrupt(struct
> >>acpi_resource *ares, int index,
> >> acpi_dev_irqresource_disabled(res, 0);
> >> return false;
> >> }
> >>- acpi_dev_get_irqresource(res, irq->interrupts[index],
> >>+ acpi_dev_get_irqresource(res, irq->interrupts[index], &dummy,
> >> irq->triggering, irq->polarity,
> >> irq->sharable, true);
> >> break;
> >>@@ -471,6 +473,7 @@ bool acpi_dev_resource_interrupt(struct
> >>acpi_resource *ares, int index,
> >> return false;
> >> }
> >> acpi_dev_get_irqresource(res, ext_irq->interrupts[index],
> >>+ &ext_irq->resource_source,
> >> ext_irq->triggering, ext_irq->polarity,
> >> ext_irq->sharable, false);
> >> break;
> >>diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> >>index 40213c4..ce30a12 100644
> >>--- a/include/linux/acpi.h
> >>+++ b/include/linux/acpi.h
> >>@@ -321,6 +321,31 @@ void acpi_set_irq_model(enum
> >>acpi_irq_model_id model,
> >> */
> >> void acpi_unregister_gsi (u32 gsi);
> >>
> >>+#ifdef CONFIG_IRQ_DOMAIN
> >>+
> >>+int acpi_irq_domain_register_irq(const struct
> >>acpi_resource_source *source,
> >>+ u32 hwirq, int trigger, int polarity);
> >>+int acpi_irq_domain_unregister_irq(const struct
> >>acpi_resource_source *source,
> >>+ u32 hwirq);
> >>+
> >>+#else
> >>+
> >>+static inline int acpi_irq_domain_register_irq(
> >>+ const struct acpi_resource_source *source, u32 hwirq, int trigger,
> >>+ int polarity)
> >>+{
> >>+ return acpi_register_gsi(NULL, hwirq, trigger, polarity);
> >>+}
> >>+
> >>+static inline int acpi_irq_domain_unregister_irq(
> >>+ const struct acpi_resource_source *source, u32 hwirq)
> >>+{
> >>+ acpi_unregister_gsi(hwirq);
> >>+ return 0;
> >>+}
> >>+
> >>+#endif /* CONFIG_IRQ_DOMAIN */
> >>+
> >> struct pci_dev;
> >>
> >> int acpi_pci_irq_enable (struct pci_dev *dev);
> >>
>
> --
> Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm
> Technologies, Inc.
> Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a
> Linux Foundation Collaborative Project.
^ permalink raw reply
* [PATCH] ARM: OMAP2+: Remove unused MUSB initialization code
From: Tony Lindgren @ 2016-11-10 17:58 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <2624325.4PiB8SBptT@avalon>
* Laurent Pinchart <laurent.pinchart@ideasonboard.com> [161110 10:56]:
> Hi Tony,
>
> On Thursday 10 Nov 2016 10:54:02 Tony Lindgren wrote:
> > * Laurent Pinchart <laurent.pinchart@ideasonboard.com> [161110 10:43]:
> > > With the removal of board-ldp.c and board-rx51.c, the last users of the
> > > MUSB initialization board code are gone. Remove it.
> >
> > Thanks, I have the same patch already from 3 years ago in omap-for-
> > v3.14/omap3-board-removal branch that I'll use.
>
> v3.14 ? That feels like cheating :-)
:p
> > Still need to rebase and check few other patches there before reposting them
> > all.
>
> Will you get that one in v4.10 ?
Yeah planning to assuming no problems and when out
of the eternal musb regressions land.
Regards,
Tony
^ permalink raw reply
* [PATCH] ARM: OMAP2+: Remove unused MUSB initialization code
From: Laurent Pinchart @ 2016-11-10 17:56 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161110175402.GE27724@atomide.com>
Hi Tony,
On Thursday 10 Nov 2016 10:54:02 Tony Lindgren wrote:
> * Laurent Pinchart <laurent.pinchart@ideasonboard.com> [161110 10:43]:
> > With the removal of board-ldp.c and board-rx51.c, the last users of the
> > MUSB initialization board code are gone. Remove it.
>
> Thanks, I have the same patch already from 3 years ago in omap-for-
> v3.14/omap3-board-removal branch that I'll use.
v3.14 ? That feels like cheating :-)
> Still need to rebase and check few other patches there before reposting them
> all.
Will you get that one in v4.10 ?
> FYI, to avoid duplicate work, the old barnch is at [0].
>
> Regards,
>
> Tony
>
> https://git.kernel.org/cgit/linux/kernel/git/tmlind/linux-omap.git/log/?h=om
> ap-for-v3.14/omap3-board-removal
--
Regards,
Laurent Pinchart
^ permalink raw reply
* PM regression with LED changes in next-20161109
From: Tony Lindgren @ 2016-11-10 17:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161110162925.GA28832@amd>
* Pavel Machek <pavel@ucw.cz> [161110 09:29]:
> Hi!
>
> > >>>Looks like commit 883d32ce3385 ("leds: core: Add support for poll()ing
> > >>>the sysfs brightness attr for changes.") breaks runtime PM for me.
> > >>>
> > >>>On my omap dm3730 based test system, idle power consumption is over 70
> > >>>times higher now with this patch! It goes from about 6mW for the core
> > >>>system to over 440mW during idle meaning there's some busy timer now
> > >>>active.
> > >>>
> > >>>Reverting this patch fixes the issue. Any ideas?
>
> Are you using any LED that toggles with high frequency? Like perhaps
> LED that is lit when CPU is active?
Yeah one of them seems to have cpu0 as the default trigger.
Tony
^ permalink raw reply
* [RESEND PATCH v1 03/11] drivers: soc: hisi: Add support for Hisilicon Djtag driver
From: Mark Rutland @ 2016-11-10 17:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478151727-20250-4-git-send-email-anurup.m@huawei.com>
On Thu, Nov 03, 2016 at 01:41:59AM -0400, Anurup M wrote:
> From: Tan Xiaojun <tanxiaojun@huawei.com>
>
> The Hisilicon Djtag is an independent component which connects
> with some other components in the SoC by Debug Bus. This driver
> can be configured to access the registers of connecting components
> (like L3 cache) during real time debugging.
>
Just to check, is this likely to be used in multi-socket hardware, and
if so, are instances always-on?
> Signed-off-by: Tan Xiaojun <tanxiaojun@huawei.com>
> Signed-off-by: John Garry <john.garry@huawei.com>
> Signed-off-by: Anurup M <anurup.m@huawei.com>
> ---
> drivers/soc/Kconfig | 1 +
> drivers/soc/Makefile | 1 +
> drivers/soc/hisilicon/Kconfig | 12 +
> drivers/soc/hisilicon/Makefile | 1 +
> drivers/soc/hisilicon/djtag.c | 639 ++++++++++++++++++++++++++++++++++++
> include/linux/soc/hisilicon/djtag.h | 38 +++
> 6 files changed, 692 insertions(+)
> create mode 100644 drivers/soc/hisilicon/Kconfig
> create mode 100644 drivers/soc/hisilicon/Makefile
> create mode 100644 drivers/soc/hisilicon/djtag.c
> create mode 100644 include/linux/soc/hisilicon/djtag.h
Other than the PMU driver(s), what is going to use this?
If you don't have something in particular, please also place this under
drivers/perf/hisilicon, along with the PMU driver(s).
We can always move it later if necessary.
[...]
> +#define SC_DJTAG_TIMEOUT 100000 /* 100ms */
This would be better as:
#define SC_DJTAG_TIMEOUT_US (100 * USEC_PER_MSEC)
(you'll need to include <linux/time64.h>)
[...]
> +static void djtag_read32_relaxed(void __iomem *regs_base, u32 off, u32 *value)
> +{
> + void __iomem *reg_addr = regs_base + off;
> +
> + *value = readl_relaxed(reg_addr);
> +}
> +
> +static void djtag_write32(void __iomem *regs_base, u32 off, u32 val)
> +{
> + void __iomem *reg_addr = regs_base + off;
> +
> + writel(val, reg_addr);
> +}
I think these make the driver harder to read, especially given the read
function is void and takes an output pointer.
In either case you can call readl/writel directly with base + off for
the __iomem ptr. Please do that.
> +
> +/*
> + * djtag_readwrite_v1/v2: djtag read/write interface
> + * @reg_base: djtag register base address
> + * @offset: register's offset
> + * @mod_sel: module selection
> + * @mod_mask: mask to select specific modules for write
> + * @is_w: write -> true, read -> false
> + * @wval: value to register for write
> + * @chain_id: which sub module for read
> + * @rval: value in register for read
> + *
> + * Return non-zero if error, else return 0.
> + */
> +static int djtag_readwrite_v1(void __iomem *regs_base, u32 offset, u32 mod_sel,
> + u32 mod_mask, bool is_w, u32 wval, int chain_id, u32 *rval)
> +{
> + u32 rd;
> + int timeout = SC_DJTAG_TIMEOUT;
> +
> + if (!(mod_mask & CHAIN_UNIT_CFG_EN)) {
> + pr_warn("djtag: do nothing.\n");
> + return 0;
> + }
> +
> + /* djtag mster enable & accelerate R,W */
> + djtag_write32(regs_base, SC_DJTAG_MSTR_EN,
> + DJTAG_NOR_CFG | DJTAG_MSTR_EN);
> +
> + /* select module */
> + djtag_write32(regs_base, SC_DJTAG_DEBUG_MODULE_SEL, mod_sel);
> + djtag_write32(regs_base, SC_DJTAG_CHAIN_UNIT_CFG_EN,
> + mod_mask & CHAIN_UNIT_CFG_EN);
> +
> + if (is_w) {
> + djtag_write32(regs_base, SC_DJTAG_MSTR_WR, DJTAG_MSTR_W);
> + djtag_write32(regs_base, SC_DJTAG_MSTR_DATA, wval);
> + } else
> + djtag_write32(regs_base, SC_DJTAG_MSTR_WR, DJTAG_MSTR_R);
> +
> + /* address offset */
> + djtag_write32(regs_base, SC_DJTAG_MSTR_ADDR, offset);
> +
> + /* start to write to djtag register */
> + djtag_write32(regs_base, SC_DJTAG_MSTR_START_EN, DJTAG_MSTR_START_EN);
> +
> + /* ensure the djtag operation is done */
> + do {
> + djtag_read32_relaxed(regs_base, SC_DJTAG_MSTR_START_EN, &rd);
> + if (!(rd & DJTAG_MSTR_EN))
> + break;
> +
> + udelay(1);
> + } while (timeout--);
> +
> + if (timeout < 0) {
> + pr_err("djtag: %s timeout!\n", is_w ? "write" : "read");
> + return -EBUSY;
> + }
> +
> + if (!is_w)
> + djtag_read32_relaxed(regs_base,
> + SC_DJTAG_RD_DATA_BASE + chain_id * 0x4, rval);
> +
> + return 0;
> +}
Please factor out the common bits into helpers and have separate
read/write functions. It's incredibly difficult to follow the code with
read/write hidden behind a boolean parameter.
> +static const struct of_device_id djtag_of_match[] = {
> + /* for hip05(D02) cpu die */
> + { .compatible = "hisilicon,hip05-cpu-djtag-v1",
> + .data = (void *)djtag_readwrite_v1 },
> + /* for hip05(D02) io die */
> + { .compatible = "hisilicon,hip05-io-djtag-v1",
> + .data = (void *)djtag_readwrite_v1 },
> + /* for hip06(D03) cpu die */
> + { .compatible = "hisilicon,hip06-cpu-djtag-v1",
> + .data = (void *)djtag_readwrite_v1 },
> + /* for hip06(D03) io die */
> + { .compatible = "hisilicon,hip06-io-djtag-v2",
> + .data = (void *)djtag_readwrite_v2 },
> + /* for hip07(D05) cpu die */
> + { .compatible = "hisilicon,hip07-cpu-djtag-v2",
> + .data = (void *)djtag_readwrite_v2 },
> + /* for hip07(D05) io die */
> + { .compatible = "hisilicon,hip07-io-djtag-v2",
> + .data = (void *)djtag_readwrite_v2 },
> + {},
> +};
Binding documentation for all of these should be added *before* this
patch, per Documentation/devicetree/bindings/submitting-patches.txt.
Please move any relevant binding documentation earlier in the series.
> +MODULE_DEVICE_TABLE(of, djtag_of_match);
> +
> +static ssize_t
> +show_modalias(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> + struct hisi_djtag_client *client = to_hisi_djtag_client(dev);
> +
> + return sprintf(buf, "%s%s\n", MODULE_PREFIX, client->name);
> +}
> +static DEVICE_ATTR(modalias, 0444, show_modalias, NULL);
> +
> +static struct attribute *hisi_djtag_dev_attrs[] = {
> + NULL,
> + /* modalias helps coldplug: modprobe $(cat .../modalias) */
> + &dev_attr_modalias.attr,
> + NULL
> +};
> +ATTRIBUTE_GROUPS(hisi_djtag_dev);
Why do we need to expose this under sysfs?
> +struct bus_type hisi_djtag_bus = {
> + .name = "hisi-djtag",
> + .match = hisi_djtag_device_match,
> + .probe = hisi_djtag_device_probe,
> + .remove = hisi_djtag_device_remove,
> +};
I take it the core bus code handles reference-counting users of the bus?
> +struct hisi_djtag_client *hisi_djtag_new_device(struct hisi_djtag_host *host,
> + struct device_node *node)
> +{
> + struct hisi_djtag_client *client;
> + int status;
> +
> + client = kzalloc(sizeof(*client), GFP_KERNEL);
> + if (!client)
> + return NULL;
> +
> + client->host = host;
> +
> + client->dev.parent = &client->host->dev;
> + client->dev.bus = &hisi_djtag_bus;
> + client->dev.type = &hisi_djtag_client_type;
> + client->dev.of_node = node;
I suspect that we should do:
client->dev.of_node = of_node_get(node);
... so that it's guaranteed we retain a reference.
> + snprintf(client->name, DJTAG_CLIENT_NAME_LEN, "%s%s",
> + DJTAG_PREFIX, node->name);
> + dev_set_name(&client->dev, "%s", client->name);
> +
> + status = device_register(&client->dev);
> + if (status < 0) {
> + pr_err("error adding new device, status=%d\n", status);
> + kfree(client);
> + return NULL;
> + }
> +
> + return client;
> +}
> +
> +static struct hisi_djtag_client *hisi_djtag_of_register_device(
> + struct hisi_djtag_host *host,
> + struct device_node *node)
> +{
> + struct hisi_djtag_client *client;
> +
> + client = hisi_djtag_new_device(host, node);
> + if (client == NULL) {
> + dev_err(&host->dev, "error registering device %s\n",
> + node->full_name);
> + of_node_put(node);
I don't think this should be here, given what djtag_register_devices()
does.
> + return ERR_PTR(-EINVAL);
> + }
> +
> + return client;
> +}
> +
> +static void djtag_register_devices(struct hisi_djtag_host *host)
> +{
> + struct device_node *node;
> + struct hisi_djtag_client *client;
> +
> + if (!host->of_node)
> + return;
> +
> + for_each_available_child_of_node(host->of_node, node) {
> + if (of_node_test_and_set_flag(node, OF_POPULATED))
> + continue;
> + client = hisi_djtag_of_register_device(host, node);
> + list_add(&client->next, &host->client_list);
> + }
> +}
Given hisi_djtag_of_register_device() can return ERR_PTR(-EINVAL), the
list_add is not safe.
> +static int djtag_host_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct hisi_djtag_host *host;
> + const struct of_device_id *of_id;
> + struct resource *res;
> + int rc;
> +
> + of_id = of_match_device(djtag_of_match, dev);
> + if (!of_id)
> + return -EINVAL;
> +
> + host = kzalloc(sizeof(*host), GFP_KERNEL);
> + if (!host)
> + return -ENOMEM;
> +
> + host->of_node = dev->of_node;
host->of_node = of_node_get(dev->of_node);
> + host->djtag_readwrite = of_id->data;
> + spin_lock_init(&host->lock);
> +
> + INIT_LIST_HEAD(&host->client_list);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + dev_err(&pdev->dev, "No reg resorces!\n");
> + kfree(host);
> + return -EINVAL;
> + }
> +
> + if (!resource_size(res)) {
> + dev_err(&pdev->dev, "Zero reg entry!\n");
> + kfree(host);
> + return -EINVAL;
> + }
> +
> + host->sysctl_reg_map = devm_ioremap_resource(dev, res);
> + if (IS_ERR(host->sysctl_reg_map)) {
> + dev_warn(dev, "Unable to map sysctl registers.\n");
> + kfree(host);
> + return -EINVAL;
> + }
Please have a common error path rather than duplicating this free.
e.g. at the end of the function have:
err_free:
kfree(host);
return err;
... and in cases like this, have:
if (whatever()) {
dev_warn(dev, "this failed xxx\n");
err = -EINVAL;
goto err_free;
}
Thanks,
Mark.
^ permalink raw reply
* [PATCH] ARM: OMAP2+: Remove unused MUSB initialization code
From: Tony Lindgren @ 2016-11-10 17:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478799800-27945-1-git-send-email-laurent.pinchart@ideasonboard.com>
* Laurent Pinchart <laurent.pinchart@ideasonboard.com> [161110 10:43]:
> With the removal of board-ldp.c and board-rx51.c, the last users of the
> MUSB initialization board code are gone. Remove it.
Thanks, I have the same patch already from 3 years ago
in omap-for-v3.14/omap3-board-removal branch that I'll
use. Still need to rebase and check few other patches
there before reposting them all. FYI, to avoid duplicate
work, the old barnch is at [0].
Regards,
Tony
https://git.kernel.org/cgit/linux/kernel/git/tmlind/linux-omap.git/log/?h=omap-for-v3.14/omap3-board-removal
^ permalink raw reply
* [PATCH] Replacement for Arm initrd memblock reserve and free inconsistency.
From: Russell King - ARM Linux @ 2016-11-10 17:46 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <592F4D3D062D79449F140027567F70E8FE080A79@exchmbx03>
On Wed, Nov 09, 2016 at 04:35:37PM +0000, william.helsby at stfc.ac.uk wrote:
> A boot time system crash was noticed with a segmentation fault just after the initrd image had been used to initialise the ramdisk.
> This occurred when the U-Boot loaded the ramdisk image from a FAT partition, but not when loaded by TFTPBOOT. This is not understood?
> However the problem was caused by free_initrd_mem freeing and "poisoning" memory that had been allocted to init/main.c to store the saved_command_line
> This patch reverses "ARM: 8167/1: extend the reserved memory for initrd to be page aligned" because it is safer to leave a partial head or tail page reserved (wasted) than to free a page which is partially still in use.
> If this is not acceptable (particularly if wanting large contiguous physical areas for DMA) then a better solution is required.
> This would extend the region reserved to page boundaries, if possible without overlapping other regions. My previous attempt to fix this coded this scheme, to grow the are reserved.
> However, this? again is not safe if in growing the area it then overlaps a region that is in use.
> Note this path is against the 4.6 kernel, but as far as I can tell applies equally to 4.8.
Please wrap commit messages at or before column 72, the exception is
for lines with a URL.
> diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
> index 370581a..ff3e9c3 100644
> --- a/arch/arm/mm/init.c
> +++ b/arch/arm/mm/init.c
> @@ -770,12 +770,6 @@ static int keep_initrd;
> void free_initrd_mem(unsigned long start, unsigned long end)
> {
> ??????? if (!keep_initrd) {
> -?????????????? if (start == initrd_start)
> -?????????????????????? start = round_down(start, PAGE_SIZE);
> -?????????????? if (end == initrd_end)
> -?????????????????????? end = round_up(end, PAGE_SIZE);
> -
> -???? ??????????poison_init_mem((void *)start, PAGE_ALIGN(end) - start);
We're definitely not getting rid of the poisoning of the pages - the
poisoning there is to detect accesses to this memory which should not
be made.
The point of rounding up and down is to ensure that the partly-used
pages (which would have been previously reserved) are freed.
Probably a better fix is to round the start up/end down of the initrd
when reserving the memory region:
arch/arm/mm/init.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index 370581aeb871..ee8509e4329d 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -255,7 +255,11 @@ void __init arm_memblock_init(const struct machine_desc *mdesc)
phys_initrd_start = phys_initrd_size = 0;
}
if (phys_initrd_size) {
- memblock_reserve(phys_initrd_start, phys_initrd_size);
+ phys_addr_t start, size;
+
+ start = round_down(phys_initrd_start, PAGE_SIZE);
+ end = round_up(phys_initrd_start + phys_initrd_size, PAGE_SIZE);
+ memblock_reserve(start, end - start);
/* Now convert initrd to virtual addresses */
initrd_start = __phys_to_virt(phys_initrd_start);
and this should ensure that memblock_alloc() doesn't try to allocate
memory overlapping the pages containing the initrd.
Intentionally using pages overlapping the initrd is a recipe for
problems...
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply related
* Summary of LPC guest MSI discussion in Santa Fe
From: Alex Williamson @ 2016-11-10 17:46 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <ddd8af9d-ad8f-78d8-3048-3d640b74470e@redhat.com>
On Thu, 10 Nov 2016 12:14:40 +0100
Auger Eric <eric.auger@redhat.com> wrote:
> Hi Will, Alex,
>
> On 10/11/2016 03:01, Will Deacon wrote:
> > On Wed, Nov 09, 2016 at 05:55:17PM -0700, Alex Williamson wrote:
> >> On Thu, 10 Nov 2016 01:14:42 +0100
> >> Auger Eric <eric.auger@redhat.com> wrote:
> >>> On 10/11/2016 00:59, Alex Williamson wrote:
> >>>> On Wed, 9 Nov 2016 23:38:50 +0000
> >>>> Will Deacon <will.deacon@arm.com> wrote:
> >>>>> On Wed, Nov 09, 2016 at 04:24:58PM -0700, Alex Williamson wrote:
> >>>>>> The VFIO_IOMMU_MAP_DMA ioctl is a contract, the user ask to map a range
> >>>>>> of IOVAs to a range of virtual addresses for a given device. If VFIO
> >>>>>> cannot reasonably fulfill that contract, it must fail. It's up to QEMU
> >>>>>> how to manage the hotplug and what memory regions it asks VFIO to map
> >>>>>> for a device, but VFIO must reject mappings that it (or the SMMU by
> >>>>>> virtue of using the IOMMU API) know to overlap reserved ranges. So I
> >>>>>> still disagree with the referenced statement. Thanks,
> >>>>>
> >>>>> I think that's a pity. Not only does it mean that both QEMU and the kernel
> >>>>> have more work to do (the former has to carve up its mapping requests,
> >>>>> whilst the latter has to check that it is indeed doing this), but it also
> >>>>> precludes the use of hugepage mappings on the IOMMU because of reserved
> >>>>> regions. For example, a 4k hole someplace may mean we can't put down 1GB
> >>>>> table entries for the guest memory in the SMMU.
> >>>>>
> >>>>> All this seems to do is add complexity and decrease performance. For what?
> >>>>> QEMU has to go read the reserved regions from someplace anyway. It's also
> >>>>> the way that VFIO works *today* on arm64 wrt reserved regions, it just has
> >>>>> no way to identify those holes at present.
> >>>>
> >>>> Sure, that sucks, but how is the alternative even an option? The user
> >>>> asked to map something, we can't, if we allow that to happen now it's a
> >>>> bug. Put the MSI doorbells somewhere that this won't be an issue. If
> >>>> the platform has it fixed somewhere that this is an issue, don't use
> >>>> that platform. The correctness of the interface is more important than
> >>>> catering to a poorly designed system layout IMO. Thanks,
> >>>
> >>> Besides above problematic, I started to prototype the sysfs API. A first
> >>> issue I face is the reserved regions become global to the iommu instead
> >>> of characterizing the iommu_domain, ie. the "reserved_regions" attribute
> >>> file sits below an iommu instance (~
> >>> /sys/class/iommu/dmar0/intel-iommu/reserved_regions ||
> >>> /sys/class/iommu/arm-smmu0/arm-smmu/reserved_regions).
> >>>
> >>> MSI reserved window can be considered global to the IOMMU. However PCIe
> >>> host bridge P2P regions rather are per iommu-domain.
> >
> > I don't think we can treat them as per-domain, given that we want to
> > enumerate this stuff before we've decided to do a hotplug (and therefore
> > don't have a domain).
> That's the issue indeed. We need to wait for the PCIe device to be
> connected to the iommu. Only on the VFIO SET_IOMMU we get the
> comprehensive list of P2P regions that can impact IOVA mapping for this
> iommu. This removes any advantage of sysfs API over previous VFIO
> capability chain API for P2P IOVA range enumeration at early stage.
For use through vfio we know that an iommu_domain is minimally composed
of an iommu_group and we can find all the p2p resources of that group
referencing /proc/iomem, at least for PCI-based groups. This is the
part that I don't think any sort of iommu sysfs attributes should be
duplicating.
> >>> Do you confirm the attribute file should contain both global reserved
> >>> regions and all per iommu_domain reserved regions?
> >>>
> >>> Thoughts?
> >>
> >> I don't think we have any business describing IOVA addresses consumed
> >> by peer devices in an IOMMU sysfs file. If it's a separate device it
> >> should be exposed by examining the rest of the topology. Regions
> >> consumed by PCI endpoints and interconnects are already exposed in
> >> sysfs. In fact, is this perhaps a more accurate model for these MSI
> >> controllers too? Perhaps they should be exposed in the bus topology
> >> somewhere as consuming the IOVA range.
> Currently on x86 the P2P regions are not checked when allowing
> passthrough. Aren't we more papist that the pope? As Don mentioned,
> shouldn't we simply consider that a platform that does not support
> proper ACS is not candidate for safe passthrough, like Juno?
There are two sides here, there's the kernel side vfio and there's how
QEMU makes use of vfio. On the kernel side, we create iommu groups as
the set of devices we consider isolated, that doesn't necessarily mean
that there isn't p2p within the group, in fact that potential often
determines the composition of the group. It's the user's problem how
to deal with that potential. When I talk about the contract with
userspace, I consider that to be at the iommu mapping, ie. for
transactions that actually make it to the iommu. In the case of x86,
we know that DMA mappings overlapping the MSI doorbells won't be
translated correctly, it's not a valid mapping for that range, and
therefore the iommu driver backing the IOMMU API should describe that
reserved range and reject mappings to it. For devices downstream of
the IOMMU, whether they be p2p or MSI controllers consuming fixed IOVA
space, I consider these to be problems beyond the scope of the IOMMU
API, and maybe that's where we've been going wrong all along.
Users like QEMU can currently discover potential p2p conflicts by
looking at the composition of an iommu group and taking into account
the host PCI resources of each device. We don't currently do this,
though we probably should. The reason we typically don't run into
problems with this is that (again) x86 has a fairly standard memory
layout. Potential p2p regions are typically in an MMIO hole in the
host that sufficiently matches an MMIO hole in the guest. So we don't
often have VM RAM, which could be a DMA target, matching those p2p
addresses. We also hope that any serious device assignment users have
singleton iommu groups, ie. the IO subsystem is designed to support
proper, fine grained isolation.
> At least we can state the feature also is missing on x86 and it would be
> nice to report the risk to the userspace and urge him to opt-in.
Sure, but the information is already there, it's "just" a matter of
QEMU taking it into account, which has some implications that VMs with
any potential of doing device assignment need to be instantiated with
address maps compatible with the host system, which is not an easy feat
for something often considered the ugly step-child of virtualization.
> To me taking into account those P2P still is controversial and induce
> the bulk of the complexity. Considering the migration use case discussed
> at LPC while only handling the MSI problem looks much easier.
> host can choose an MSI base that is QEMU mach-virt friendly, ie. non RAM
> region. Problem is to satisfy all potential uses though. When migrating,
> mach-virt still is being used so there should not be any collision. Am I
> missing some migration weird use cases here? Of course if we take into
> consideration new host PCIe P2P regions this becomes completely different.
Yep, x86 having a standard MSI range is a nice happenstance, so long as
we're running an x86 VM, we don't worry about that being a DMA target.
Running non-x86 VMs on x86 hosts hits this problem, but is several
orders of magnitude lower priority.
> We still have the good old v14 where the user space chose where MSI
> IOVA's are put without any risk of collision ;-)
>
> >> If DMA to an IOVA is consumed
> >> by an intermediate device before it hits the IOMMU vs not being
> >> translated as specified by the user at the IOMMU, I'm less inclined to
> >> call that something VFIO should reject.
> >
> > Oh, so perhaps we've been talking past each other. In all of these cases,
> > the SMMU can translate the access if it makes it that far. The issue is
> > that not all accesses do make it that far, because they may be "consumed"
> > by another device, such as an MSI doorbell or another endpoint. In other
> > words, I don't envisage a scenario where e.g. some address range just
> > bypasses the SMMU straight to memory. I realise now that that's not clear
> > from the slides I presented.
As above, so long as a transaction that does make it to the iommu is
translated as prescribed by the user, I have no basis for rejecting a
user requested translation. Downstream MSI controllers consuming IOVA
space is no different than the existing p2p problem that vfio considers
a userspace issue.
> >> However, instantiating a VM
> >> with holes to account for every potential peer device seems like it
> >> borders on insanity. Thanks,
> >
> > Ok, so rather than having a list of reserved regions under the iommu node,
> > you're proposing that each region is attributed to the device that "owns"
> > (consumes) it? I think that can work, but we need to make sure that:
> >
> > (a) The topology is walkable from userspace (where do you start?)
For PCI devices userspace can examine the topology of the iommu group
and exclude MMIO ranges of peer devices based on the BARs, which are
exposed in various places, pci-sysfs as well as /proc/iomem. For
non-PCI or MSI controllers... ???
> > (b) It also works for platform (non-PCI) devices, that lack much in the
> > way of bus hierarchy
No idea here, without a userspace visible topology the user is in the
dark as to what devices potentially sit between them and the iommu.
> > (c) It doesn't require Linux to have a driver bound to a device in order
> > for the ranges consumed by that device to be advertised (again,
> > more of an issue for non-PCI).
Right, PCI has this problem solved, be more like PCI ;)
> > How is this currently advertised for PCI? I'd really like to use the same
> > scheme irrespective of the bus type.
For all devices within an IOMMU group, /proc/iomem might be the
solution, but I don't know how the MSI controller works. If the MSI
controller belongs to the group, then maybe it's a matter of creating a
struct device for it that consumes resources and making it show up in
both the iommu group and /proc/iomem. An MSI controller shared between
groups, which already sounds like a bit of a violation of iommu groups,
would need to be discoverable some other way. Thanks,
Alex
^ permalink raw reply
* [PATCH] ARM: OMAP2+: Remove unused MUSB initialization code
From: Laurent Pinchart @ 2016-11-10 17:43 UTC (permalink / raw)
To: linux-arm-kernel
With the removal of board-ldp.c and board-rx51.c, the last users of the
MUSB initialization board code are gone. Remove it.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
arch/arm/mach-omap2/Makefile | 1 -
arch/arm/mach-omap2/usb-musb.c | 106 -----------------------------------------
arch/arm/mach-omap2/usb.h | 1 -
3 files changed, 108 deletions(-)
delete mode 100644 arch/arm/mach-omap2/usb-musb.c
diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
index 5b37ec29996e..76e8ba70d952 100644
--- a/arch/arm/mach-omap2/Makefile
+++ b/arch/arm/mach-omap2/Makefile
@@ -242,7 +242,6 @@ obj-y += $(omap-flash-y) $(omap-flash-m)
omap-hsmmc-$(CONFIG_MMC_OMAP_HS) := hsmmc.o
obj-y += $(omap-hsmmc-m) $(omap-hsmmc-y)
-obj-y += usb-musb.o
obj-y += omap_phy_internal.o
obj-$(CONFIG_MACH_OMAP2_TUSB6010) += usb-tusb6010.o
diff --git a/arch/arm/mach-omap2/usb-musb.c b/arch/arm/mach-omap2/usb-musb.c
deleted file mode 100644
index e4562b2b973b..000000000000
--- a/arch/arm/mach-omap2/usb-musb.c
+++ /dev/null
@@ -1,106 +0,0 @@
-/*
- * linux/arch/arm/mach-omap2/usb-musb.c
- *
- * This file will contain the board specific details for the
- * MENTOR USB OTG controller on OMAP3430
- *
- * Copyright (C) 2007-2008 Texas Instruments
- * Copyright (C) 2008 Nokia Corporation
- * Author: Vikram Pandita
- *
- * Generalization by:
- * Felipe Balbi <felipe.balbi@nokia.com>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- */
-
-#include <linux/types.h>
-#include <linux/errno.h>
-#include <linux/delay.h>
-#include <linux/platform_device.h>
-#include <linux/clk.h>
-#include <linux/dma-mapping.h>
-#include <linux/io.h>
-#include <linux/usb/musb.h>
-
-#include "omap_device.h"
-#include "soc.h"
-#include "mux.h"
-#include "usb.h"
-
-static struct musb_hdrc_config musb_config = {
- .multipoint = 1,
- .dyn_fifo = 1,
- .num_eps = 16,
- .ram_bits = 12,
-};
-
-static struct musb_hdrc_platform_data musb_plat = {
- .mode = MUSB_OTG,
-
- /* .clock is set dynamically */
- .config = &musb_config,
-
- /* REVISIT charge pump on TWL4030 can supply up to
- * 100 mA ... but this value is board-specific, like
- * "mode", and should be passed to usb_musb_init().
- */
- .power = 50, /* up to 100 mA */
-};
-
-static u64 musb_dmamask = DMA_BIT_MASK(32);
-
-static struct omap_musb_board_data musb_default_board_data = {
- .interface_type = MUSB_INTERFACE_ULPI,
- .mode = MUSB_OTG,
- .power = 100,
-};
-
-void __init usb_musb_init(struct omap_musb_board_data *musb_board_data)
-{
- struct omap_hwmod *oh;
- struct platform_device *pdev;
- struct device *dev;
- int bus_id = -1;
- const char *oh_name, *name;
- struct omap_musb_board_data *board_data;
-
- if (musb_board_data)
- board_data = musb_board_data;
- else
- board_data = &musb_default_board_data;
-
- /*
- * REVISIT: This line can be removed once all the platforms using
- * musb_core.c have been converted to use use clkdev.
- */
- musb_plat.clock = "ick";
- musb_plat.board_data = board_data;
- musb_plat.power = board_data->power >> 1;
- musb_plat.mode = board_data->mode;
- musb_plat.extvbus = board_data->extvbus;
-
- oh_name = "usb_otg_hs";
- name = "musb-omap2430";
-
- oh = omap_hwmod_lookup(oh_name);
- if (WARN(!oh, "%s: could not find omap_hwmod for %s\n",
- __func__, oh_name))
- return;
-
- pdev = omap_device_build(name, bus_id, oh, &musb_plat,
- sizeof(musb_plat));
- if (IS_ERR(pdev)) {
- pr_err("Could not build omap_device for %s %s\n",
- name, oh_name);
- return;
- }
-
- dev = &pdev->dev;
- get_device(dev);
- dev->dma_mask = &musb_dmamask;
- dev->coherent_dma_mask = musb_dmamask;
- put_device(dev);
-}
diff --git a/arch/arm/mach-omap2/usb.h b/arch/arm/mach-omap2/usb.h
index 3395365ef1db..1951535646d2 100644
--- a/arch/arm/mach-omap2/usb.h
+++ b/arch/arm/mach-omap2/usb.h
@@ -60,7 +60,6 @@ struct usbhs_phy_data {
bool vcc_polarity; /* 1 active high, 0 active low */
};
-extern void usb_musb_init(struct omap_musb_board_data *board_data);
extern void usbhs_init(struct usbhs_omap_platform_data *pdata);
extern int usbhs_init_phys(struct usbhs_phy_data *phy, int num_phys);
--
Regards,
Laurent Pinchart
^ permalink raw reply related
* [PATCHv2] PCI: QDF2432 32 bit config space accessors
From: Bjorn Helgaas @ 2016-11-10 17:42 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAKv+Gu9_sdcoN7rjQp-R=2vKY3rECt0BC3eWGpse32o2Q4LHXg@mail.gmail.com>
On Thu, Nov 10, 2016 at 06:25:16PM +0800, Ard Biesheuvel wrote:
> On 10 November 2016 at 06:49, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Wed, Nov 09, 2016 at 08:29:23PM +0000, Ard Biesheuvel wrote:
> >> Hi Bjorn,
> >>
> >> On 9 November 2016 at 20:06, Bjorn Helgaas <helgaas@kernel.org> wrote:
> >> > On Wed, Nov 09, 2016 at 02:25:56PM -0500, Christopher Covington wrote:
> >> >> Hi Bjorn,
> >> >>
> >> [...]
> >> >>
> >> >> We're working to add the PNP0C02 resource to future firmware, but it's
> >> >> not in the current firmware. Are dmesg and /proc/iomem from the
> >> >> current firmware interesting or should we wait for the update to file?
> >> >
> >> > Note that the ECAM space is not the only thing that should be
> >> > described via these PNP0C02 devices. *All* non-enumerable resources
> >> > should be described by the _CRS method of some ACPI device. Here's a
> >> > sample from my laptop:
> >> >
> >> > PCI: MMCONFIG for domain 0000 [bus 00-3f] at [mem 0xf8000000-0xfbffffff] (base 0xf8000000)
> >> > system 00:01: [io 0x1800-0x189f] could not be reserved
> >> > system 00:01: [io 0x0800-0x087f] has been reserved
> >> > system 00:01: [io 0x0880-0x08ff] has been reserved
> >> > system 00:01: [io 0x0900-0x097f] has been reserved
> >> > system 00:01: [io 0x0980-0x09ff] has been reserved
> >> > system 00:01: [io 0x0a00-0x0a7f] has been reserved
> >> > system 00:01: [io 0x0a80-0x0aff] has been reserved
> >> > system 00:01: [io 0x0b00-0x0b7f] has been reserved
> >> > system 00:01: [io 0x0b80-0x0bff] has been reserved
> >> > system 00:01: [io 0x15e0-0x15ef] has been reserved
> >> > system 00:01: [io 0x1600-0x167f] has been reserved
> >> > system 00:01: [io 0x1640-0x165f] has been reserved
> >> > system 00:01: [mem 0xf8000000-0xfbffffff] could not be reserved
> >> > system 00:01: [mem 0xfed10000-0xfed13fff] has been reserved
> >> > system 00:01: [mem 0xfed18000-0xfed18fff] has been reserved
> >> > system 00:01: [mem 0xfed19000-0xfed19fff] has been reserved
> >> > system 00:01: [mem 0xfeb00000-0xfebfffff] has been reserved
> >> > system 00:01: [mem 0xfed20000-0xfed3ffff] has been reserved
> >> > system 00:01: [mem 0xfed90000-0xfed93fff] has been reserved
> >> > system 00:01: [mem 0xf7fe0000-0xf7ffffff] has been reserved
> >> > system 00:01: Plug and Play ACPI device, IDs PNP0c02 (active)
> >> >
> >> > Do you have firmware in the field that may not get updated? If so,
> >> > I'd like to see the whole solution for that firmware, including the
> >> > MCFG quirk (which tells the PCI core where the ECAM region is) and
> >> > whatever PNP0C02 quirk you figure out to actually reserve the region.
> >> >
> >> > I proposed a PNP0C02 quirk to Duc along these lines of the below. I
> >> > don't actually know if it's feasible, but it didn't look as bad as I
> >> > expected, so I'd kind of like somebody to try it out. I think you
> >> > would have to call this via a DMI hook (do you have DMI on arm64?),
> >> > maybe from pnp_init() or similar.
> >>
> >> We do have SMBIOS/DMI on arm64, but we have been successful so far not
> >> to rely on it for quirks, and we'd very much like to keep it that way.
> >>
> >> Since this ACPI _CRS method has nothing to do with SMBIOS/DMI, surely
> >> there is a better way to wire up the reservation code to the
> >> information exposed by ACPI?
> >
> > I'm open to other ways, feel free to propose one :)
> >
> > If you do a quirk, you need some way to identify the machine/firmware
> > combination, because you don't want to apply the quirk on every
> > machine. You're trying to work around a firmware issue, so you
> > probably want something tied to the firmware version. On x86, that's
> > typically done with DMI.
> >
>
> I think I misunderstood the purpose of the example: that should only
> be necessary if the _CRS methods are missing from the firmware, right?
> If we update the firmware to cover all non-enumerable resources by
> such a method, we shouldn't need any such quirks at all IIUC
Right: if the firmware provides a PNP0C02 device with _CRS that
includes the ECAM area, we don't need any PNP/ACPI quirks. We will
still need the MCFG quirks since the hardware doesn't fully support
ECAM.
For the PNP/ACPI quirks, there are two interesting cases:
1) Firmware provides a PNP0C02 device, but its _CRS doesn't include
the ECAM space, and
2) Firmware doesn't provide a PNP0C02 device at all.
For case 1, we could consider adding the ECAM space to the existing
device. This is essentially what quirk_amd_mmconfig_area() does.
For case 2, we would have to fabricate the PNP0C02 device itself, then
add the ECAM space to it. I don't think there's any existing code
that does this, so this is what the example I proposed in this thread
does.
One could argue that it might be cleaner to use case 2 instead of the
case 1 approach because it avoids mucking with an ACPI device from
firmware. For devices that support _SRS, case 1 might break things
because _CRS and _SRS are supposed to use the same resource descriptor
buffer, and if we add resources the firmware doesn't know about, I
don't think we'll encode the _SRS buffer correctly. But this is only
a theoretical risk because we basically never use _SRS today.
In either case, there has to be a mechanism to do the quirk only on
the machine/firmware that needs it, of course.
Bjorn
^ permalink raw reply
* [PATCH v2 2/6] mfd: dt: Add bindings for the Aspeed SoC Display Controller (GFX)
From: Rob Herring @ 2016-11-10 17:40 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CACPK8XeG2WnpNuHUf9VD+mZenWboq-4wei=LOkN4qVR72QbGXQ@mail.gmail.com>
On Wed, Nov 9, 2016 at 9:19 PM, Joel Stanley <joel@jms.id.au> wrote:
> On Thu, Nov 10, 2016 at 4:56 AM, Rob Herring <robh@kernel.org> wrote:
>> On Thu, Nov 03, 2016 at 01:07:57AM +1030, Andrew Jeffery wrote:
>>> The Aspeed SoC Display Controller is presented as a syscon device to
>>> arbitrate access by display and pinmux drivers. Video pinmux
>>> configuration on fifth generation SoCs depends on bits in both the
>>> System Control Unit and the Display Controller.
>>>
>>> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
>>> ---
>>> Documentation/devicetree/bindings/mfd/aspeed-gfx.txt | 17 +++++++++++++++++
>>
>> The register space can't be split to 2 nodes?
>
> Do you mean splitting the GFX IP and enable register into two nodes?
>
> We can't. Pinmux needs to check bit 6 and 7 in GFX064, which is in the
> middle the IP block:
>
> GFX060: CRT Control Register I
> GFX064: CRT Control Register II
> GFX068: CRT Status Register
> GFX06C: CRT Misc Setting Register
Okay.
>>> +The Aspeed SoC Display Controller primarily does as its name suggests, but also
>>> +participates in pinmux requests on the g5 SoCs. It is therefore considered a
>>> +syscon device.
>>> +
>>> +Required properties:
>>> +- compatible: "aspeed,ast2500-gfx", "syscon"
>>
>> I think perhaps we should drop the syscon here and the driver should
>> just register as a syscon.
>
> We want the regmap to be present whenever the GFX driver or pinmux is
> loaded. If we register it in pinmux but chose to not build in that
> driver, we lack the regmap. Same for the case where a user builds in
> the GFX driver and not pinmux. I think this means we want the syscon
> compatible string, unless my understanding is wrong?
Right.
Acked-by: Rob Herring <robh@kernel.org>
Rob
^ permalink raw reply
* [PATCH v6 2/9] drm/hisilicon/hibmc: Add video memory management
From: Sean Paul @ 2016-11-10 17:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1477639682-22520-3-git-send-email-zourongrong@gmail.com>
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
>
> 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
> +
> + 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
> +
> +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
> +
> + 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)
> + 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);
> + 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.
> + 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.
> +
> + 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;
> +}
> +
> +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'?
> + 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
> + 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
> + 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
> +
> +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
> + return NULL;
> + tt->func = &hibmc_tt_backend_func;
> + if (ttm_tt_init(tt, bdev, size, page_flags, dummy_read_page)) {
Here too?
> + 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?
> + 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?
> + 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
> + 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
> +
> +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?
> + }
> +
> + 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
> + 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
> + 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
> + 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
> + 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?
> + 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
> + 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;
> +}
> --
> 1.9.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH v6 1/9] drm/hisilicon/hibmc: Add hisilicon hibmc drm master driver
From: Sean Paul @ 2016-11-10 17:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1477639682-22520-2-git-send-email-zourongrong@gmail.com>
On Fri, Oct 28, 2016 at 3:27 AM, Rongrong Zou <zourongrong@gmail.com> wrote:
> Add DRM master driver for Hisilicon Hibmc SoC which used for
> Out-of-band management. Blow is the general hardware connection,
> both the Hibmc and the host CPU are on the same mother board.
>
> +----------+ +----------+
> | | PCIe | Hibmc |
> |host CPU( |<----->| display |
> |arm64,x86)| |subsystem |
> +----------+ +----------+
>
> Signed-off-by: Rongrong Zou <zourongrong@gmail.com>
> ---
> drivers/gpu/drm/hisilicon/Kconfig | 1 +
> drivers/gpu/drm/hisilicon/Makefile | 1 +
> drivers/gpu/drm/hisilicon/hibmc/Kconfig | 7 +
> drivers/gpu/drm/hisilicon/hibmc/Makefile | 5 +
> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 269 ++++++++++++++++++++++
> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h | 35 +++
> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_power.c | 85 +++++++
> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_power.h | 28 +++
> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_regs.h | 212 +++++++++++++++++
> 9 files changed, 643 insertions(+)
> create mode 100644 drivers/gpu/drm/hisilicon/hibmc/Kconfig
> create mode 100644 drivers/gpu/drm/hisilicon/hibmc/Makefile
> create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_power.c
> create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_power.h
> create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_regs.h
>
> diff --git a/drivers/gpu/drm/hisilicon/Kconfig b/drivers/gpu/drm/hisilicon/Kconfig
> index 558c61b..2fd2724 100644
> --- a/drivers/gpu/drm/hisilicon/Kconfig
> +++ b/drivers/gpu/drm/hisilicon/Kconfig
> @@ -2,4 +2,5 @@
> # hisilicon drm device configuration.
> # Please keep this list sorted alphabetically
>
> +source "drivers/gpu/drm/hisilicon/hibmc/Kconfig"
> source "drivers/gpu/drm/hisilicon/kirin/Kconfig"
> diff --git a/drivers/gpu/drm/hisilicon/Makefile b/drivers/gpu/drm/hisilicon/Makefile
> index e3f6d49..c8155bf 100644
> --- a/drivers/gpu/drm/hisilicon/Makefile
> +++ b/drivers/gpu/drm/hisilicon/Makefile
> @@ -2,4 +2,5 @@
> # Makefile for hisilicon drm drivers.
> # Please keep this list sorted alphabetically
>
> +obj-$(CONFIG_DRM_HISI_HIBMC) += hibmc/
> obj-$(CONFIG_DRM_HISI_KIRIN) += kirin/
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/Kconfig b/drivers/gpu/drm/hisilicon/hibmc/Kconfig
> new file mode 100644
> index 0000000..a9af90d
> --- /dev/null
> +++ b/drivers/gpu/drm/hisilicon/hibmc/Kconfig
> @@ -0,0 +1,7 @@
> +config DRM_HISI_HIBMC
> + tristate "DRM Support for Hisilicon Hibmc"
> + depends on DRM && PCI
> +
> + help
> + Choose this option if you have a Hisilicon Hibmc soc chipset.
> + If M is selected the module will be called hibmc-drm.
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile
> new file mode 100644
> index 0000000..97cf4a0
> --- /dev/null
> +++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile
> @@ -0,0 +1,5 @@
> +ccflags-y := -Iinclude/drm
> +hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_power.o
> +
> +obj-$(CONFIG_DRM_HISI_HIBMC) +=hibmc-drm.o
nit: Improper spacing here
> +#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
> new file mode 100644
> index 0000000..4669d42
> --- /dev/null
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> @@ -0,0 +1,269 @@
> +/* 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 <linux/module.h>
> +#include <linux/console.h>
> +
> +#include "hibmc_drm_drv.h"
> +#include "hibmc_drm_regs.h"
> +#include "hibmc_drm_power.h"
nit: Alphabetize headers
> +
> +static const struct file_operations hibmc_fops = {
> + .owner = THIS_MODULE,
> + .open = drm_open,
> + .release = drm_release,
> + .unlocked_ioctl = drm_ioctl,
> +#ifdef CONFIG_COMPAT
drm_compat_ioctl is now initialized to NULL, so you can remove the #ifdef
> + .compat_ioctl = drm_compat_ioctl,
> +#endif
> + .poll = drm_poll,
> + .read = drm_read,
> + .llseek = no_llseek,
> +};
> +
> +static int hibmc_enable_vblank(struct drm_device *dev, unsigned int pipe)
> +{
> + return 0;
> +}
> +
> +static void hibmc_disable_vblank(struct drm_device *dev, unsigned int pipe)
> +{
> +}
> +
> +static struct drm_driver hibmc_driver = {
> + .fops = &hibmc_fops,
> + .name = "hibmc",
> + .date = "20160828",
> + .desc = "hibmc drm driver",
> + .major = 1,
> + .minor = 0,
> + .get_vblank_counter = drm_vblank_no_hw_counter,
> + .enable_vblank = hibmc_enable_vblank,
> + .disable_vblank = hibmc_disable_vblank,
> +};
> +
> +static int hibmc_pm_suspend(struct device *dev)
> +{
> + return 0;
> +}
> +
> +static int hibmc_pm_resume(struct device *dev)
> +{
> + return 0;
> +}
> +
> +static const struct dev_pm_ops hibmc_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(hibmc_pm_suspend,
> + hibmc_pm_resume)
> +};
> +
> +static int hibmc_hw_config(struct hibmc_drm_device *hidev)
> +{
> + unsigned int reg;
> +
> + /* On hardware reset, power mode 0 is default. */
> + hibmc_set_power_mode(hidev, HIBMC_PW_MODE_CTL_MODE_MODE0);
> +
> + /* Enable display power gate & LOCALMEM power gate*/
> + reg = readl(hidev->mmio + HIBMC_CURRENT_GATE);
> + reg &= ~HIBMC_CURR_GATE_DISPLAY_MASK;
> + reg &= ~HIBMC_CURR_GATE_LOCALMEM_MASK;
> + reg |= HIBMC_CURR_GATE_DISPLAY(ON);
> + reg |= HIBMC_CURR_GATE_LOCALMEM(ON);
> +
> + hibmc_set_current_gate(hidev, reg);
> +
> + /* Reset the memory controller. If the memory controller
> + * is not reset in chip,the system might hang when sw accesses
> + * the memory.The memory should be resetted after
> + * changing the MXCLK.
> + */
> + reg = readl(hidev->mmio + HIBMC_MISC_CTRL);
> + reg &= ~HIBMC_MSCCTL_LOCALMEM_RESET_MASK;
> + reg |= HIBMC_MSCCTL_LOCALMEM_RESET(RESET);
> + writel(reg, hidev->mmio + HIBMC_MISC_CTRL);
> +
> + reg &= ~HIBMC_MSCCTL_LOCALMEM_RESET_MASK;
> + reg |= HIBMC_MSCCTL_LOCALMEM_RESET(NORMAL);
> +
> + writel(reg, hidev->mmio + HIBMC_MISC_CTRL);
> +
> + /* We can add more initialization as needed. */
> +
> + return 0;
Consider using void return type here to simplify error checking in the
caller, especially since it looks like you aren't checking the return
code, anyways :)
> +}
> +
> +static int hibmc_hw_map(struct hibmc_drm_device *hidev)
> +{
> + struct drm_device *dev = hidev->dev;
> + struct pci_dev *pdev = dev->pdev;
> + resource_size_t addr, size, ioaddr, iosize;
> +
> + ioaddr = pci_resource_start(pdev, 1);
> + iosize = MB(2);
> +
> + hidev->mmio = ioremap_nocache(ioaddr, iosize);
Use devm_ioremap_nocache to avoid managing the resource directly
> +
nit: extra space
> + if (!hidev->mmio) {
> + DRM_ERROR("Cannot map mmio region\n");
> + return -ENOMEM;
> + }
> +
> + addr = pci_resource_start(pdev, 0);
> + size = MB(32);
Pull size and iosize out into #defines with descriptive names
> +
> + hidev->fb_map = ioremap(addr, size);
Use devm_ioremap to avoid managing the resource directly.
> + if (!hidev->fb_map) {
> + DRM_ERROR("Cannot map framebuffer\n");
> + return -ENOMEM;
> + }
> + hidev->fb_base = addr;
> + hidev->fb_size = size;
> +
> + return 0;
> +}
> +
> +static void hibmc_hw_fini(struct hibmc_drm_device *hidev)
> +{
> + if (hidev->mmio)
> + iounmap(hidev->mmio);
> + if (hidev->fb_map)
> + iounmap(hidev->fb_map);
> +}
You don't need this function if you use the devm variants above
> +
> +static int hibmc_hw_init(struct hibmc_drm_device *hidev)
> +{
> + int ret;
> +
> + ret = hibmc_hw_map(hidev);
> + if (ret)
> + return ret;
> +
> + hibmc_hw_config(hidev);
> +
> + return 0;
> +}
> +
> +static int hibmc_unload(struct drm_device *dev)
> +{
> + struct hibmc_drm_device *hidev = dev->dev_private;
> +
> + hibmc_hw_fini(hidev);
> + dev->dev_private = NULL;
> + return 0;
> +}
> +
> +static int hibmc_load(struct drm_device *dev, unsigned long flags)
flags isn't used anywhere?
> +{
> + struct hibmc_drm_device *hidev;
> + int ret;
> +
> + hidev = devm_kzalloc(dev->dev, sizeof(*hidev), GFP_KERNEL);
> + if (!hidev)
Print error here?
> + return -ENOMEM;
> + dev->dev_private = hidev;
> + hidev->dev = dev;
> +
> + ret = hibmc_hw_init(hidev);
> + if (ret)
> + goto err;
> +
> + return 0;
> +
> +err:
> + hibmc_unload(dev);
> + DRM_ERROR("failed to initialize drm driver.\n");
Print the return value
> + return ret;
> +}
> +
> +static int hibmc_pci_probe(struct pci_dev *pdev,
> + const struct pci_device_id *ent)
> +{
> + struct drm_device *dev;
> + int ret;
> +
> + dev = drm_dev_alloc(&hibmc_driver, &pdev->dev);
> + if (!dev)
Print error here
> + return -ENOMEM;
> +
> + dev->pdev = pdev;
> + pci_set_drvdata(pdev, dev);
> +
> + ret = pci_enable_device(pdev);
> + if (ret)
and here, and in other failure paths
> + goto err_free;
> +
> + ret = hibmc_load(dev, 0);
> + if (ret)
> + goto err_disable;
> +
> + ret = drm_dev_register(dev, 0);
> + if (ret)
> + goto err_unload;
> +
> + return 0;
> +
> +err_unload:
> + hibmc_unload(dev);
> +err_disable:
> + pci_disable_device(pdev);
> +err_free:
> + drm_dev_unref(dev);
> +
> + return ret;
> +}
> +
> +static void hibmc_pci_remove(struct pci_dev *pdev)
> +{
> + struct drm_device *dev = pci_get_drvdata(pdev);
> +
> + drm_dev_unregister(dev);
> + hibmc_unload(dev);
> + drm_dev_unref(dev);
> +}
> +
> +static struct pci_device_id hibmc_pci_table[] = {
> + {0x19e5, 0x1711, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0},
> + {0,}
> +};
> +
> +static struct pci_driver hibmc_pci_driver = {
> + .name = "hibmc-drm",
> + .id_table = hibmc_pci_table,
> + .probe = hibmc_pci_probe,
> + .remove = hibmc_pci_remove,
> + .driver.pm = &hibmc_pm_ops,
> +};
> +
> +static int __init hibmc_init(void)
> +{
> + return pci_register_driver(&hibmc_pci_driver);
> +}
> +
> +static void __exit hibmc_exit(void)
> +{
> + return pci_unregister_driver(&hibmc_pci_driver);
> +}
> +
> +module_init(hibmc_init);
> +module_exit(hibmc_exit);
> +
> +MODULE_DEVICE_TABLE(pci, hibmc_pci_table);
> +MODULE_AUTHOR("RongrongZou <zourongrong@huawei.com>");
> +MODULE_DESCRIPTION("DRM Driver for Hisilicon Hibmc");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> new file mode 100644
> index 0000000..0037341
> --- /dev/null
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> @@ -0,0 +1,35 @@
> +/* 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.
> + *
> + */
> +
> +#ifndef HIBMC_DRM_DRV_H
> +#define HIBMC_DRM_DRV_H
> +
> +#include <drm/drmP.h>
> +
> +struct hibmc_drm_device {
nit: Calling this hibmc_drm_priv would probably make things easier to
read. When I read hibmc_drm_device, it makes me think that it's an
extension of drm_device, which this isn't.
> + /* hw */
> + void __iomem *mmio;
> + void __iomem *fb_map;
> + unsigned long fb_base;
> + unsigned long fb_size;
> +
> + /* drm */
> + struct drm_device *dev;
> +};
> +
> +#endif
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_power.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_power.c
> new file mode 100644
> index 0000000..1036542
> --- /dev/null
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_power.c
I don't think you need a new file for these functions. Just stash them
in hibmc_drm_drv.c
> @@ -0,0 +1,85 @@
> +/* 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 "hibmc_drm_regs.h"
> +
> +/*
> + * It can operate in one of three modes: 0, 1 or Sleep.
> + */
> +void hibmc_set_power_mode(struct hibmc_drm_device *hidev,
> + unsigned int power_mode)
> +{
> + unsigned int control_value = 0;
> + void __iomem *mmio = hidev->mmio;
> +
> + if (power_mode > HIBMC_PW_MODE_CTL_MODE_SLEEP)
> + return;
> +
> + control_value = readl(mmio + HIBMC_POWER_MODE_CTRL);
> + control_value &= ~HIBMC_PW_MODE_CTL_MODE_MASK;
> +
> + control_value |= HIBMC_PW_MODE_CTL_MODE(power_mode) &
> + HIBMC_PW_MODE_CTL_MODE_MASK;
> +
> + /* Set up other fields in Power Control Register */
> + if (power_mode == HIBMC_PW_MODE_CTL_MODE_SLEEP) {
> + control_value &= ~HIBMC_PW_MODE_CTL_OSC_INPUT_MASK;
You do this in both branches of the conditional
> + control_value |= HIBMC_PW_MODE_CTL_OSC_INPUT(0) &
> + HIBMC_PW_MODE_CTL_OSC_INPUT_MASK;
> + } else {
> + control_value &= ~HIBMC_PW_MODE_CTL_OSC_INPUT_MASK;
> + control_value |= HIBMC_PW_MODE_CTL_OSC_INPUT(1) &
> + HIBMC_PW_MODE_CTL_OSC_INPUT_MASK;
> + }
I think you could simplify this by adding a new local.
unsigned int input = 1;
if (power_mode == HIBMC_PW_MODE_CTL_MODE_SLEEP)
input = 0;
control_value = readl(mmio + HIBMC_POWER_MODE_CTRL);
control_value &= ~(HIBMC_PW_MODE_CTL_MODE_MASK |
HIBMC_PW_MODE_CTL_OSC_INPUT_MASK);
control_value |= HIBMC_PW_MODE_CTL_MODE(power_mode) &
HIBMC_PW_MODE_CTL_MODE_MASK;
control_value |= HIBMC_PW_MODE_CTL_OSC_INPUT(input) &
HIBMC_PW_MODE_CTL_OSC_INPUT_MASK;
> + /* Program new power mode. */
> + writel(control_value, mmio + HIBMC_POWER_MODE_CTRL);
> +}
> +
> +static unsigned int hibmc_get_power_mode(struct hibmc_drm_device *hidev)
> +{
> + void __iomem *mmio = hidev->mmio;
> +
> + return (readl(mmio + HIBMC_POWER_MODE_CTRL) &
> + HIBMC_PW_MODE_CTL_MODE_MASK) >> HIBMC_PW_MODE_CTL_MODE_SHIFT;
nit: You're doing a lot of work in the return statement here.
> +}
> +
> +void hibmc_set_current_gate(struct hibmc_drm_device *hidev, unsigned int gate)
> +{
> + unsigned int gate_reg;
> + unsigned int mode;
> + void __iomem *mmio = hidev->mmio;
> +
> + /* Get current power mode. */
nit: try to avoid comments that don't add value
> + mode = hibmc_get_power_mode(hidev);
> +
> + switch (mode) {
> + case HIBMC_PW_MODE_CTL_MODE_MODE0:
> + gate_reg = HIBMC_MODE0_GATE;
> + break;
> +
> + case HIBMC_PW_MODE_CTL_MODE_MODE1:
> + gate_reg = HIBMC_MODE1_GATE;
> + break;
> +
> + default:
> + gate_reg = HIBMC_MODE0_GATE;
> + break;
> + }
> + writel(gate, mmio + gate_reg);
> +}
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_power.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_power.h
> new file mode 100644
> index 0000000..e20e1aa
> --- /dev/null
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_power.h
> @@ -0,0 +1,28 @@
> +/* 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.
> + *
> + */
> +
> +#ifndef HIBMC_DRM_POWER_H
> +#define HIBMC_DRM_POWER_H
> +
> +#include "hibmc_drm_drv.h"
> +
> +void hibmc_set_power_mode(struct hibmc_drm_device *hidev,
> + unsigned int power_mode);
> +void hibmc_set_current_gate(struct hibmc_drm_device *hidev,
> + unsigned int gate);
> +#endif
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_regs.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_regs.h
> new file mode 100644
> index 0000000..9c804ca
> --- /dev/null
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_regs.h
> @@ -0,0 +1,212 @@
> +/* 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.
> + *
> + */
> +
> +#ifndef HIBMC_DRM_HW_H
> +#define HIBMC_DRM_HW_H
> +
> +#define OFF 0
> +#define ON 1
> +#define DISABLE 0
> +#define ENABLE 1
These are not good names. I think you can probably hardcode the 0's
and 1's in the code instead of these. However, if you want to use
them, at least add a HIBMC_ prefix
> +
> +/* register definition */
> +#define HIBMC_MISC_CTRL 0x4
> +
> +#define HIBMC_MSCCTL_LOCALMEM_RESET(x) ((x) << 6)
> +#define HIBMC_MSCCTL_LOCALMEM_RESET_MASK 0x40
> +
> +#define RESET 0
> +#define NORMAL 1
Same naming nit here. Add a prefix
> +
> +#define HIBMC_CURRENT_GATE 0x000040
> +#define HIBMC_CURR_GATE_DISPLAY(x) ((x) << 2)
> +#define HIBMC_CURR_GATE_DISPLAY_MASK 0x4
> +
> +#define HIBMC_CURR_GATE_LOCALMEM(x) ((x) << 1)
> +#define HIBMC_CURR_GATE_LOCALMEM_MASK 0x2
> +
> +#define HIBMC_MODE0_GATE 0x000044
> +#define HIBMC_MODE1_GATE 0x000048
> +#define HIBMC_POWER_MODE_CTRL 0x00004C
> +
> +#define HIBMC_PW_MODE_CTL_OSC_INPUT(x) ((x) << 3)
> +#define HIBMC_PW_MODE_CTL_OSC_INPUT_MASK 0x8
> +
> +#define HIBMC_PW_MODE_CTL_MODE(x) ((x) << 0)
> +#define HIBMC_PW_MODE_CTL_MODE_MASK 0x03
> +#define HIBMC_PW_MODE_CTL_MODE_SHIFT 0
> +
> +#define HIBMC_PW_MODE_CTL_MODE_MODE0 0
> +#define HIBMC_PW_MODE_CTL_MODE_MODE1 1
> +#define HIBMC_PW_MODE_CTL_MODE_SLEEP 2
> +
> +#define HIBMC_PANEL_PLL_CTRL 0x00005C
> +#define HIBMC_CRT_PLL_CTRL 0x000060
> +
> +#define HIBMC_PLL_CTRL_BYPASS(x) ((x) << 18)
> +#define HIBMC_PLL_CTRL_BYPASS_MASK 0x40000
> +
> +#define HIBMC_PLL_CTRL_POWER(x) ((x) << 17)
> +#define HIBMC_PLL_CTRL_POWER_MASK 0x20000
> +
> +#define HIBMC_PLL_CTRL_INPUT(x) ((x) << 16)
> +#define HIBMC_PLL_CTRL_INPUT_MASK 0x10000
> +
> +#define OSC 0
Naming
> +#define TESTCLK 1
This doesn't seem to be used?
> +
> +#define HIBMC_PLL_CTRL_POD(x) ((x) << 14)
> +#define HIBMC_PLL_CTRL_POD_MASK 0xC000
> +
> +#define HIBMC_PLL_CTRL_OD(x) ((x) << 12)
> +#define HIBMC_PLL_CTRL_OD_MASK 0x3000
> +
> +#define HIBMC_PLL_CTRL_N(x) ((x) << 8)
> +#define HIBMC_PLL_CTRL_N_MASK 0xF00
> +
> +#define HIBMC_PLL_CTRL_M(x) ((x) << 0)
> +#define HIBMC_PLL_CTRL_M_MASK 0xFF
> +
> +#define HIBMC_CRT_DISP_CTL 0x80200
> +
> +#define HIBMC_CRT_DISP_CTL_CRTSELECT(x) ((x) << 25)
> +#define HIBMC_CRT_DISP_CTL_CRTSELECT_MASK 0x2000000
> +
> +#define CRTSELECT_VGA 0
> +#define CRTSELECT_CRT 1
> +
> +#define HIBMC_CRT_DISP_CTL_CLOCK_PHASE(x) ((x) << 14)
> +#define HIBMC_CRT_DISP_CTL_CLOCK_PHASE_MASK 0x4000
> +
> +#define PHASE_ACTIVE_HIGH 0
> +#define PHASE_ACTIVE_LOW 1
> +
> +#define HIBMC_CRT_DISP_CTL_VSYNC_PHASE(x) ((x) << 13)
> +#define HIBMC_CRT_DISP_CTL_VSYNC_PHASE_MASK 0x2000
> +
> +#define HIBMC_CRT_DISP_CTL_HSYNC_PHASE(x) ((x) << 12)
> +#define HIBMC_CRT_DISP_CTL_HSYNC_PHASE_MASK 0x1000
> +
> +#define HIBMC_CRT_DISP_CTL_TIMING(x) ((x) << 8)
> +#define HIBMC_CRT_DISP_CTL_TIMING_MASK 0x100
> +
> +#define HIBMC_CRT_DISP_CTL_PLANE(x) ((x) << 2)
> +#define HIBMC_CRT_DISP_CTL_PLANE_MASK 4
> +
> +#define HIBMC_CRT_DISP_CTL_FORMAT(x) ((x) << 0)
> +#define HIBMC_CRT_DISP_CTL_FORMAT_MASK 0x03
> +
> +#define HIBMC_CRT_FB_ADDRESS 0x080204
> +
> +#define HIBMC_CRT_FB_WIDTH 0x080208
> +#define HIBMC_CRT_FB_WIDTH_WIDTH(x) ((x) << 16)
> +#define HIBMC_CRT_FB_WIDTH_WIDTH_MASK 0x3FFF0000
> +#define HIBMC_CRT_FB_WIDTH_OFFS(x) ((x) << 0)
> +#define HIBMC_CRT_FB_WIDTH_OFFS_MASK 0x3FFF
> +
> +#define HIBMC_CRT_HORZ_TOTAL 0x08020C
> +#define HIBMC_CRT_HORZ_TOTAL_TOTAL(x) ((x) << 16)
> +#define HIBMC_CRT_HORZ_TOTAL_TOTAL_MASK 0xFFF0000
> +
> +#define HIBMC_CRT_HORZ_TOTAL_DISPLAY_END(x) ((x) << 0)
> +#define HIBMC_CRT_HORZ_TOTAL_DISPLAY_END_MASK 0xFFF
> +
> +#define HIBMC_CRT_HORZ_SYNC 0x080210
> +#define HIBMC_CRT_HORZ_SYNC_WIDTH(x) ((x) << 16)
> +#define HIBMC_CRT_HORZ_SYNC_WIDTH_MASK 0xFF0000
> +
> +#define HIBMC_CRT_HORZ_SYNC_START(x) ((x) << 0)
> +#define HIBMC_CRT_HORZ_SYNC_START_MASK 0xFFF
> +
> +#define HIBMC_CRT_VERT_TOTAL 0x080214
> +#define HIBMC_CRT_VERT_TOTAL_TOTAL(x) ((x) << 16)
> +#define HIBMC_CRT_VERT_TOTAL_TOTAL_MASK 0x7FFF0000
> +
> +#define HIBMC_CRT_VERT_TOTAL_DISPLAY_END(x) ((x) << 0)
> +#define HIBMC_CRT_VERT_TOTAL_DISPLAY_END_MASK 0x7FF
> +
> +#define HIBMC_CRT_VERT_SYNC 0x080218
> +#define HIBMC_CRT_VERT_SYNC_HEIGHT(x) ((x) << 16)
> +#define HIBMC_CRT_VERT_SYNC_HEIGHT_MASK 0x3F0000
> +
> +#define HIBMC_CRT_VERT_SYNC_START(x) ((x) << 0)
> +#define HIBMC_CRT_VERT_SYNC_START_MASK 0x7FF
> +
> +/* Auto Centering */
> +#define HIBMC_CRT_AUTO_CENTERING_TL 0x080280
> +#define HIBMC_CRT_AUTO_CENTERING_TL_TOP(x) ((x) << 16)
> +#define HIBMC_CRT_AUTO_CENTERING_TL_TOP_MSK 0x7FF0000
> +
> +#define HIBMC_CRT_AUTO_CENTERING_TL_LEFT(x) ((x) << 0)
> +#define HIBMC_CRT_AUTO_CENTERING_TL_LEFT_MSK 0x7FF
> +
> +#define HIBMC_CRT_AUTO_CENTERING_BR 0x080284
> +#define HIBMC_CRT_AUTO_CENTERING_BR_BOTTOM(x) ((x) << 16)
> +#define HIBMC_CRT_AUTO_CENTERING_BR_BOTTOM_MASK 0x7FF0000
> +
> +#define HIBMC_CRT_AUTO_CENTERING_BR_RIGHT(x) ((x) << 0)
> +#define HIBMC_CRT_AUTO_CENTERING_BR_RIGHT_MASK 0x7FF
> +
> +/* register to control panel output */
> +#define DISPLAY_CONTROL_HISILE 0x80288
> +
> +#define HIBMC_RAW_INTERRUPT 0x80290
> +#define HIBMC_RAW_INTERRUPT_VBLANK(x) ((x) << 2)
> +#define HIBMC_RAW_INTERRUPT_VBLANK_MASK 0x4
> +
> +#define HIBMC_RAW_INTERRUPT_EN 0x80298
> +#define HIBMC_RAW_INTERRUPT_EN_VBLANK(x) ((x) << 2)
> +#define HIBMC_RAW_INTERRUPT_EN_VBLANK_MASK 0x4
> +
> +/* register and values for PLL control */
> +#define CRT_PLL1_HS 0x802a8
> +#define CRT_PLL1_HS_25MHZ 0x23d40f02
> +#define CRT_PLL1_HS_40MHZ 0x23940801
> +#define CRT_PLL1_HS_65MHZ 0x23940d01
> +#define CRT_PLL1_HS_78MHZ 0x23540F82
> +#define CRT_PLL1_HS_74MHZ 0x23941dc2
> +#define CRT_PLL1_HS_80MHZ 0x23941001
> +#define CRT_PLL1_HS_80MHZ_1152 0x23540fc2
> +#define CRT_PLL1_HS_108MHZ 0x23b41b01
> +#define CRT_PLL1_HS_162MHZ 0x23480681
> +#define CRT_PLL1_HS_148MHZ 0x23541dc2
> +#define CRT_PLL1_HS_193MHZ 0x234807c1
> +
> +#define CRT_PLL2_HS 0x802ac
> +#define CRT_PLL2_HS_25MHZ 0x206B851E
> +#define CRT_PLL2_HS_40MHZ 0x30000000
> +#define CRT_PLL2_HS_65MHZ 0x40000000
> +#define CRT_PLL2_HS_78MHZ 0x50E147AE
> +#define CRT_PLL2_HS_74MHZ 0x602B6AE7
> +#define CRT_PLL2_HS_80MHZ 0x70000000
> +#define CRT_PLL2_HS_108MHZ 0x80000000
> +#define CRT_PLL2_HS_162MHZ 0xA0000000
> +#define CRT_PLL2_HS_148MHZ 0xB0CCCCCD
> +#define CRT_PLL2_HS_193MHZ 0xC0872B02
> +
> +/* Global macros */
> +#define RGB(r, g, b) \
Not used anywhere?
> +( \
> + (unsigned long)(((r) << 16) | ((g) << 8) | (b)) \
> +)
> +
> +#define PADDING(align, data) (((data) + (align) - 1) & (~((align) - 1)))
> +
This is only used in hibmc_drm_de.c, move it in there. It might also
be nice to make it a type-checked function while you're at it.
> +#define MB(x) ((x) << 20)
> +
This is only used in 2 places, I think you can just hardcode the value
in their respective #defines
> +#endif
> --
> 1.9.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Applied "ASoC: sun4i-codec: fix semicolon.cocci warnings" to the asoc tree
From: Mark Brown @ 2016-11-10 17:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161109163518.GA42508@lkp-nex06.lkp.intel.com>
The patch
ASoC: sun4i-codec: fix semicolon.cocci warnings
has been applied to the asoc tree at
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
>From 35db57622c31af687d5cb14104e91897d778a8fc Mon Sep 17 00:00:00 2001
From: kbuild test robot <fengguang.wu@intel.com>
Date: Thu, 10 Nov 2016 00:35:18 +0800
Subject: [PATCH] ASoC: sun4i-codec: fix semicolon.cocci warnings
sound/soc/sunxi/sun4i-codec.c:1339:2-3: Unneeded semicolon
Remove unneeded semicolon.
Generated by: scripts/coccinelle/misc/semicolon.cocci
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
Acked-by: Chen-Yu Tsai <wens@csie.org>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
sound/soc/sunxi/sun4i-codec.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/sunxi/sun4i-codec.c b/sound/soc/sunxi/sun4i-codec.c
index 6379efd21f00..092fdcf6de95 100644
--- a/sound/soc/sunxi/sun4i-codec.c
+++ b/sound/soc/sunxi/sun4i-codec.c
@@ -1336,7 +1336,7 @@ static int sun4i_codec_probe(struct platform_device *pdev)
dev_err(&pdev->dev, "Failed to get reset control\n");
return PTR_ERR(scodec->rst);
}
- };
+ }
scodec->gpio_pa = devm_gpiod_get_optional(&pdev->dev, "allwinner,pa",
GPIOD_OUT_LOW);
--
2.10.2
^ permalink raw reply related
* [PATCH v3 00/10] Add DT support for ohci-da8xx
From: Axel Haslam @ 2016-11-10 17:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161110120253.GA5385@kroah.com>
On Thu, Nov 10, 2016 at 1:02 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Tue, Nov 08, 2016 at 05:37:41PM +0100, Axel Haslam wrote:
>> Hi,
>>
>> On Mon, Nov 7, 2016 at 9:39 PM, Axel Haslam <ahaslam@baylibre.com> wrote:
>> > The purpose of this patch series is to add DT support for the davinci
>> > ohci driver.
>> >
>>
>> To make it easier to review. I will split the arch/arm and driver
>> patches into separate series.
>
> I don't think it's easier, as now I have no idea what order, or what
> tree it should go through :(
Hi Greg,
i will resend the series one by one and wait for each to be merged
before sending the next, so that there is no confusion on the ordering
or on what tree they should apply.
Regards
Axel.
>
> I'm guessing not mine, so all are now deleted from my patch queue...
>
> greg k-h
^ permalink raw reply
* [PATCH 1/5] iommu: Allow taking a reference on a group directly
From: Joerg Roedel @ 2016-11-10 17:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161109180059.GJ17771@arm.com>
On Wed, Nov 09, 2016 at 06:00:59PM +0000, Will Deacon wrote:
> I'd still rather the new function was renamed. We already have the group,
> so calling a weird underscore version of iommu_group_get is really
> counter-intuitive.
>
> Joerg -- do you have a preference?
iommu_group_ref_get sound best to me. Unfortunatly C has no function
overloading ;)
Joerg
^ permalink raw reply
* [PATCH v27 1/9] memblock: add memblock_cap_memory_range()
From: Will Deacon @ 2016-11-10 17:27 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161102045153.12008-1-takahiro.akashi@linaro.org>
On Wed, Nov 02, 2016 at 01:51:53PM +0900, AKASHI Takahiro wrote:
> Add memblock_cap_memory_range() which will remove all the memblock regions
> except the range specified in the arguments.
>
> This function, like memblock_mem_limit_remove_map(), will not remove
> memblocks with MEMMAP_NOMAP attribute as they may be mapped and accessed
> later as "device memory."
> See the commit a571d4eb55d8 ("mm/memblock.c: add new infrastructure to
> address the mem limit issue").
>
> This function is used, in a succeeding patch in the series of arm64 kdump
> suuport, to limit the range of usable memory, System RAM, on crash dump
> kernel.
> (Please note that "mem=" parameter is of little use for this purpose.)
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Cc: linux-mm at kvack.org
> Cc: Andrew Morton <akpm@linux-foundation.org>
> ---
> include/linux/memblock.h | 1 +
> mm/memblock.c | 28 ++++++++++++++++++++++++++++
> 2 files changed, 29 insertions(+)
>
> diff --git a/include/linux/memblock.h b/include/linux/memblock.h
> index 5b759c9..0e770af 100644
> --- a/include/linux/memblock.h
> +++ b/include/linux/memblock.h
> @@ -334,6 +334,7 @@ phys_addr_t memblock_start_of_DRAM(void);
> phys_addr_t memblock_end_of_DRAM(void);
> void memblock_enforce_memory_limit(phys_addr_t memory_limit);
> void memblock_mem_limit_remove_map(phys_addr_t limit);
> +void memblock_cap_memory_range(phys_addr_t base, phys_addr_t size);
> bool memblock_is_memory(phys_addr_t addr);
> int memblock_is_map_memory(phys_addr_t addr);
> int memblock_is_region_memory(phys_addr_t base, phys_addr_t size);
> diff --git a/mm/memblock.c b/mm/memblock.c
> index 7608bc3..eb53876 100644
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -1544,6 +1544,34 @@ void __init memblock_mem_limit_remove_map(phys_addr_t limit)
> (phys_addr_t)ULLONG_MAX);
> }
>
> +void __init memblock_cap_memory_range(phys_addr_t base, phys_addr_t size)
> +{
> + int start_rgn, end_rgn;
> + int i, ret;
> +
> + if (!size)
> + return;
> +
> + ret = memblock_isolate_range(&memblock.memory, base, size,
> + &start_rgn, &end_rgn);
> + if (ret)
> + return;
> +
> + /* remove all the MAP regions */
> + for (i = memblock.memory.cnt - 1; i >= end_rgn; i--)
> + if (!memblock_is_nomap(&memblock.memory.regions[i]))
> + memblock_remove_region(&memblock.memory, i);
> +
> + for (i = start_rgn - 1; i >= 0; i--)
> + if (!memblock_is_nomap(&memblock.memory.regions[i]))
> + memblock_remove_region(&memblock.memory, i);
> +
> + /* truncate the reserved regions */
> + memblock_remove_range(&memblock.reserved, 0, base);
> + memblock_remove_range(&memblock.reserved,
> + base + size, (phys_addr_t)ULLONG_MAX);
> +}
This duplicates a bunch of the logic in memblock_mem_limit_remove_map. Can
you not implement that in terms of your new, more general, function? e.g.
by passing base == 0, and size == limit?
Will
^ permalink raw reply
* [RESEND PATCH v1 02/11] dt-bindings: hisi: Add Hisilicon HiP05/06/07 Sysctrl and Djtag dts bindings
From: Mark Rutland @ 2016-11-10 17:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478151727-20250-3-git-send-email-anurup.m@huawei.com>
Hi,
On Thu, Nov 03, 2016 at 01:41:58AM -0400, Anurup M wrote:
> From: Tan Xiaojun <tanxiaojun@huawei.com>
>
> 1) Add Hisilicon HiP05/06/07 CPU and ALGSUB system controller dts
> bindings.
> 2) Add Hisilicon Djtag dts binding.
>
> Signed-off-by: Tan Xiaojun <tanxiaojun@huawei.com>
> Signed-off-by: Anurup M <anurup.m@huawei.com>
> ---
> .../bindings/arm/hisilicon/hisilicon.txt | 82 ++++++++++++++++++++++
> 1 file changed, 82 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt
> index 7df79a7..341cbb9 100644
> --- a/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt
> +++ b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt
> @@ -270,3 +270,85 @@ Required Properties:
> [1]: bootwrapper size
> [2]: relocation physical address
> [3]: relocation size
> +-----------------------------------------------------------------------
> +The Hisilicon Djtag in CPU die is an independent component which connects with
> +some other components in the SoC by Debug Bus. This driver can be configured
> +to access the registers of connecting components (like L3 cache, l3 cache PMU
> + etc.) during real time debugging by sysctrl. These components appear as child
> +nodes of djtag.
Please put the djtag binding in a new file.
It's clearly unrelated to many other things in this file, which should
also be split out.
> +The Hip05/06/07 CPU system controller(sysctrl) support to manage some important
> +components (such as clock, reset, soft reset, secure debugger, etc.).
> +The CPU sysctrl registers in hip05/06/07 doesnot use syscon but will be mapped
> +by djtag driver for use by connecting components.
The djtag driver is irrelvant here.
If there's a realtionship between the two, please define that in the
binding rather than implicitly assuming it in the driver.
> +
> +Required properties:
> + - compatible : "hisilicon,hip05-cpu-djtag-v1"
> + - reg : Register address and size
> +
> +Hisilicon HiP06 djtag for CPU sysctrl
> +Required properties:
> +- compatible : "hisilicon,hip06-sysctrl", "syscon", "simple-mfd";
This looks messy. Why is this syscon and a simple-mfd?
We should kill off / deprecate the syscon binding. It's completely
meaningless.
> +- reg : Register address and size
> +- djtag :
> + - compatible : "hisilicon,hip06-cpu-djtag-v1"
> + - reg : Register address and size
> +
> +Hisilicon HiP07 djtag for CPU sysctrl
> +Required properties:
> + - compatible : "hisilicon,hip07-cpu-djtag-v2"
> + - reg : Register address and size
> +
> +Example:
> + /* for Hisilicon HiP05 djtag for CPU sysctrl */
> + djtag0: djtag at 80010000 {
> + compatible = "hisilicon,hip05-cpu-djtag-v1";
> + reg = <0x0 0x80010000 0x0 0x10000>;
> +
> + /* 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>;
> + };
This sub-node needs a binding document.
These properties are completely opaque
> + };
> +
> +-----------------------------------------------------------------------
> +The Hisilicon HiP05/06/07 ALGSUB system controller(sysctrl) is in IO die
> +of SoC. It has a similar function as the Hisilicon HiP05/06/07 CPU system
> +controller in CPU die and it manage different components, like RSA, etc.
> +The Hisilicon Djtag in IO die has a similar function as in CPU die and maps
> +the sysctrl registers for use by connecting components.
> +All connecting components shall appear as child nodes of djtag.
I don't follow the above. This describes an ALGSUB system controllerm
but the documentation below is all about djtag.
Thanks,
Mark.
> +Hisilicon HiP05 djtag for ALGSUB sysctrl
> +Required properties:
> + - compatible : "hisilicon,hip05-io-djtag-v1"
> + - reg : Register address and size
> +
> +Hisilicon HiP06 djtag for ALGSUB sysctrl
> +Required properties:
> + - compatible : "hisilicon,hip06-io-djtag-v2"
> + - reg : Register address and size
> +
> +Hisilicon HiP07 djtag for ALGSUB sysctrl
> +Required properties:
> + - compatible : "hisilicon,hip07-io-djtag-v2"
> + - reg : Register address and size
> +
> +Example:
> + /* for Hisilicon HiP05 djtag for alg sysctrl */
> + djtag0: djtag at d0000000 {
> + compatible = "hisilicon,hip05-io-djtag-v1";
> + reg = <0x0 0xd0000000 0x0 0x10000>;
> + };
> --
> 2.1.4
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox